txnprovider/composite: implement composite txn provider

[taratorio]

No description provided.

[somnathb1]

Except for some tests, only the so-called composite-transaction provider has been kind of implemented.
I think with this PR, I am totally confused what files are being edited, and why they have been named and re-named the way they are.
My two cents:

• "Provider" seems to not be a good name, as "providing" isn't particularly or entirely what it's doing (in the many meanings it has been used). Seems like it is meant as something that encompasses the mempool functionality plus some, but it's not straightforward
• Distinction between "ordered" and "unordered" pools isn't very clear through code, at the moment, and is cluttering the code
• I'd much rather have 3 different files named pool_legacy.go, pool_mux.go (because multiplexing is what the composite one is doing), and pool_shutter.go: that all implement a TxnPool interface
• Priority for an entire instance of a pool is too abstract of a concept to generalize beyond the use with Shutter. I think Shutter's protocol logic around transaction inclusion priority should be directly encoded with the rest of the shutter logic. That may eliminate the need for the extra abstraction design for the Provider bit

[taratorio]

@somnathb1 I would like to understand better where this confusion is coming from on your side.

1. Is it related to the overall design and usage of the "TxnProvider" interface and its corresponding implementations?
2. Or is it related to the current implementation of the design in terms of the current code structure/packaging/naming?

If it is 2) then note this is a WIP and a lot of this structure will keep on changing as I'm adding more code to the shutter package. We can iron these things out and improve as we go. I prefer to do small and incremental PRs with active refactoring as not everything can be predicted and thought of at such an early stage of development.

If it is 1) then we should probably discuss this during our weekly team meeting. The design has already been discussed and @yperbasis, Mark and Alexey confirmed it makes sense.

To answer some of your points:

• I think the name "TxnProvider" for the interface is totally fine. It does 1 thing - provide transactions for block building - which is why it has 1 function to yield transactions. The second function it has is Priority which is used to compose multiple providers into 1 provider.
• Happy to remove the "unordered" provider example if you think it is not useful - the intent of it is to serve as an example of how we can implement a devp2p pool transaction provider which simply returns all transactions in the devp2p transaction pool without imposing the 5 dimensional ordering function of the txpool described here. This unordered devp2p pool transaction provider is in addition to the ordered one which yields transactions that follow the ordering function. Not sure why you think the 1 extra file txpool/txn_provider_unordered.go with 40 LoC is "cluttering the code", feels a bit like an exaggeration
• As to "composing" vs "multiplexing" I think both are fine - I prefer composite because in my head this provider is composing multiple providers into 1
• Regarding having 3 different files implementing an interface - that is already what the code is doing: composite/txn_provider.go, shutter/txn_provider.go and txpool/txn_provider_ordered.go implement the TxnProvider interface
• Not sure I understand what you mean in your last point. Can you explain with a bit more detail? As I mentioned earlier, the Priority of the TxnProvider exists so that we can compose multiple providers into 1 and determine which one to access first

[somnathb1]

@taratorio
You do make some valid arguments.
I think my confusion was already highlighted well, the takeaway being that this is not very streamlined. But, I still lean on giving you creative freedom around this. The following is merely suggestive.

Let's start with my issues from the perspective of "another" dev who's gonna work on this frequently:

• I have to memorise the purpose of the 10-15 newly created files and folders in the overall design change. That without much of logic change (yet?). Every time I come to fix a bug, i will be dealing with this. Somewhat likely i will introduce a bug.
• When I am looking at things from a neutral Blockchain/Ethereum perspective: I am looking for code that does "mem-pool", but i am dealing with files that aren't quite named that way
• There is a lot of flexibility in the design - flexibility that I am not gonna use, but I am forced to write extensible code around it anyway.

Let's do some more concrete clarifications:

• "TxnProvider" interface that is written down in a separate file is fine. It's when you push that back to existing stack is where I have issue with. For instance, many could implement a certain io.reader interface, but I don't have to move all code to the reader package. I would suggest having a the ordering function modularized out within a top level txnPool directory (it's debatable how separate it has to be).
• If Yield is the only thing, the interface should be called Yielder. For instance, it may make sense to say in a sentence that "a transaction Yielder essentially supplies transaction, so it's going to be called supplier.go and all the code around that is going to supplier/supplier_xyz.go
• It may look like creating panic("implement me") functions are okay, but I would much rather have the main branch have concrete fully developed solution merged in. When you do contribute a todo - later type code, it should stop at just a single line of comment, instead of a whole refactoring
• Regarding agreeing to a design: If I were to have agreed to a design about abstracting away the code around TxPool to add more functionality, I do make assumption that this is going to be a work in progress. It is totally fine. The design discussion is just that - a discussion. However, even following that I wouldn't wanna introduce more friction in the codebase when there isn't more functionality to deal with.
• Clarification on point 3: Priority property in a transaction set is not a general function. It's up to shutter's implementation or protocol on how to pick a transaction based on its priority. One list is prioritised over another list, but not necessarily the entire implementation of a pool, over another. The first "composite" transaction manager would (and should) only be dealing with our classic txpool and the new shutter pool. It has fixed rules. There is no reason to complicate this at the moment. Please note that PBS is not to be implemented by the pool, but in a different way by a validator client. I can't think of a reason for a node to be running say 4 or 5 different pools.
• One extra file that does nothing is "cluttering the code", that's not an exaggeration. The unordered pool functionality makes sense if there is a use case for that, otherwise it's just a test.
• The aspects of transaction ordering and sequencing are around the sub-pools and their heaps, along with the ordering bits. I would suggest thinking around modularising that (the subpools and the data structures that hold transactions) instead of the current design. But do note that transaction validity is a protocol thing and can be separate for separate pools.

There are some strong opinions in these points, many of which may interfere with your current through process around this. I still feel obligated to express these.

[taratorio]

@somnathb1 ok, thank you. Let's discuss these in our call tomorrow. To summarise, seems like your comments boil down to the below 4 points (please correct me if I misunderstood some parts):

1. package structure:
   • your point is that having a txnprovider package which defines the TxnProvider interface is ok, however, the shutter and txpool packages should not be sub-packages of it (based on the io.Reader example)
   • for reference, the current package structure in this PR is:

erigon/txnprovider  
   --> exports TxnProvider interface

erigon/txnprovider/txpool 
    --> exports existing devp2p pool implementation 
    --> exports the txn provider for the ordered sub-set of the devp2p pool transactions
    --> exports the txn provider for all the devp2p pool transactions without any ordering (happy to remove this one as you think it won't be necessary in future)

erigon/txnprovider/shutter
    --> exports the shutter implementation (WIP)
    --> exports the txn provider for the shutter transactions

erigon/txnprovider/composite
    --> exports the composite txn provider which relies on Priority() of txn providers

2. naming:
   • you prefer to call TxnProvider a Yielder because it's method is called Yield. Based on your logic then we can keep the name TxnProvider if we rename the function name to Provide instead of Yield. We have 2 options Yielder.Yield() (Result, err) or TxnProvider.Provide() (Result, err)
3. simplifying the composite txn provider:
   • by removing the Priority() int function
   • you are suggesting that at this point it is not needed because you speculate that in future we won't add other providers with different priorities
   • so the composite txn provider can explicitly use the shutter pool and the devp2p pool & first select from the shutter pool & then from the devp2p pool
   • or alternatively we do not have a composite txn provider, instead we have only a shutter txn provider which relies on 2 pools: shutter and devp2p - first gets transactions from the shutter pool and then from the devp2p pool
4. overall design direction:
   • you suggest we try to modularise the sub-pools concept of the txpool package and integrate the shutter ordering in the existing ordering function instead of introducing a new concept of txn provider

[taratorio]

@somnathb1 thought about your feedback further. Here are my thoughts:

Regarding 1), 2) & 3):

• overall agree with your feedback, the current approach can be simplified
• maybe the below package structure works better? (pending discussion)

erigon/txn/parser --> contains existing rlp parsing logic that can be re-used between shutter and txpool mempools (will do in future refactoring)
erigon/txn/provider --> defines the txn provider interface
erigon/txn/mempool/shutter --> shutter pool implementation, directly implementing the txn provider interface
erigon/txn/mempool/txpool --> existing devp2p pool implementation, directly implementing the txn provider interface

• optional future package tidy ups? (pending discussion)

erigon/txn/types --> contains all types of transactions: legacy txns, access list txns, dynamic fee txns, blob txns, etc

• will define TxnProvider in its own separate package
• will rename Yield func to ProvideTxns
• will remove Priority func from TxnProvider
• the devp2p TxPool will directly implement the TxnProvider interface instead of having the "ordered devp2p txn provider"
• will remove the "unordered devp2p txn provider" since you think we would not need it for supporting external MEV searchers at some point in the future (maybe we should discuss this briefly and confirm or maybe we don't think about this now and leave it for when there is a need?)
• the shutter pool will also directly implement the TxnProvider interface
• removing the composite txn provider, instead the Shutter Pool implementation will have a dependency on a "fallback txn provider" (which will be the devp2p txpool). Its logic would be to first wait for receiving decryption keys for a given slot, unencrypt the corresponding transactions and then fill the remaining gas with transactions from the fallback txn provider. In case the decryption keys for the slot never arrive it will return empty transactions without using the fallback devp2p txn provider as per the Shutter Spec. I actually think this is the simplest and most intuitive design.

What do you think?

Regarding 4):
I considered this few times and still think it is best to implement shutter outside of the existing txpool sub-pool and ordering architecture.

The shutter transactions take higher priority over the devp2p transactions and that is something that is possible to incorporate into the sub-pool design. For example, always putting shutter transactions directly into the pendingSubPool which uses the bestSlice and enhancing metaTxn's best and worse functions to give shutter type transactions highest priority and compare the shutter transaction index amongst them to follow the Shutter specified order from the smart contract.

However, there is another complication which doesn't fit well with this design: the shutter pool needs to wait for the decryption keys (hence transactions) to arrive for the given slot. They may come with some delay - that means the txpool needs to implement waiting logic. They may also never arrive (indicating Keyper failure) in which case we must produce an empty block.

I think it will be cleaner to implement this all together separately in the shutter pool as described in the last bullet point about points 1), 2) & 3) above. With that approach there is no need at all for worrying about sub-pools and new ordering. Also the shutter implementation will be directly reflecting the specs making it much clearer.

[taratorio]

Outcome of discussion was:

• we can remove the concept of composite txn provider and priority for now to simplify and instead shutter txn provider will use the devp2p txn provider as a fallback (ie it will be a wrapper around it)
• we keep the package structure unchanged (ie we stick to txnprovider/shutter and txnprovider/txpool) for now