Add test to check that TX selection doesn't prioritize TXs from the same sender

[matthew1001]

PR description

Note that this PR is dependent on these PRs:

• Reverse sort order #6106
• Apply the same reverse sort order as https://github.com/hyperledger/b… #6146

(My testing showed that #6146 was required and had been missed off #6106)

This PR now simply adds unit tests which demonstrate that the PRs ^^ mean we no longer require a specific CLI option --Xtx-pool-disable-sender-grouping

Re-ordering the TX selection under those PRs meant that grouping of transactions per sender was no longer an issue in the legacy TX pool.

The intent of this PR is to give the user the option to control whether the transaction pool selection logic prefers to fill the block with all available transactions from each sender it has transactions for, or to treat the transactions equally, regardless of which sender they came from. Discussions with enterprise users have shown that the existing/default behaviour is not what they expect, and further it can cause an active sender to inadvertently monopolize the TX selection and effectively block other senders from having their transactions mined.

The PR adds an experimental flag --Xtx-pool-disable-sender-grouping. When this flag is set, the transaction selection pays no regard to the account the transaction was from.

Since this is an experimental feature, I have not added the doc-change-required label as I don't believe we document experimental features?

[github-actions]

• I thought about documentation and added the doc-change-required label to this PR if updates are required.
• I thought about the changelog and included a changelog update if required.
• If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

[matthew1001]

@fab-10 I've been looking at a suitable approach to avoid transactions from a single sender monopolizing the transaction selection process and preventing other senders' transactions from being included in a block. The current draft is a working implementation, but I'm not very happy with it.

It feels like the logic in AbstractPendingTransactionsSorter.selectTransactions() ( https://github.com/hyperledger/besu/blob/03a8335bf94e613fff8e4564b432069588e8e6a3/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java#L244C20-L244C20) is fighting against the logic in GasPricePendingTransactionsSorter:

private final NavigableSet<PendingTransaction> prioritizedTransactions =
      new TreeSet<>(
          comparing(PendingTransaction::isReceivedFromLocalSource)
              .thenComparing(PendingTransaction::getGasPrice)
              .thenComparing(PendingTransaction::getAddedAt)
              .thenComparing(PendingTransaction::getSequence)
              .reversed());

prioritizedTransactions returns the transactions in reverse order (i.e. the most recently added transactions, quite likely the ones with the highest nonces, are the first to be considered for selection). AbstractPendingTransactionsSorter.selectTransactions() therefore has to "defer" those with higher nonces, and when it reaches a valid nonce it grabs all of the deferred ones for that sender. Hence the current situation where if the first valid nonce TX is for sender1, and sender1 has 100s of transactions deferred, those are chosen in preference to any of sender2's.

My first go at making this configurable (via --tx-pool-disable-sender-grouping) is the following:

1. Keep the same ordering as today, where most recent TXs are considered first
2. Keep the same deferral mechanism
3. When a valid TX nonce is found, evaluate it but leave the deferred ones in their list
4. At the end, if any deferred transactions for any senders remain, consider them one account at a time (i.e. round robin across accounts giving each one a chance to have its next deferred transaction considered for selection)
5. Continue step 4 until no deferred transactions for any sender accounts remain

The implementation in the PR needs tidying up (I've got a bunch of old-school for-loops to replace with streams etc) but I wanted to get feedback before continuing. The extra unit tests I've added demonstrate that it works. But I can't help thinking that if the order of priorizedTransactions was the oldest transactions to be submitted are considered first then the logic would be simpler and would work for both the original logic and the new logic. I think it would be something like:

private final NavigableSet<PendingTransaction> prioritizedTransactions =
      new TreeSet<>(
          comparing(PendingTransaction::getSequence, Comparator.reverseOrder())
              .thenComparing(PendingTransaction::getAddedAt, Comparator.reverseOrder())
              .thenComparing(PendingTransaction::getGasPrice)
              .thenComparing(PendingTransaction::isReceivedFromLocalSource));

(going by memory - the above may not be correct, but just to give you the idea). However, I think that's a fairly fundamental change to the prioritized transactions ordering so I'm guessing it would have a bunch of side affects.

[fab-10]

@matthew1001 sorry but I could not review this in details until next week.
In the mean time I want to share an approach I was thinking about yesterday when we talked about the replacement of txs in a gas free market.
Now we have the possibility to select the txpool implementation, via the tx-pool options, and thus could be possible to introduce an implementation tailored for a gas free market, that does not order by price, try to be FIFO (or fair) and accept any replacement.
Pretty simple, the only complication is tracking the nonce gaps, but for it we could just reuse the Ready and Sparse layers of the layered txpool, making possible to customize their sorted and the selector.

[matthew1001]

Hi @fab-10 no problem about not reviewing until next week. I'm on vacation next week so that might give you some time to take a look and for us to catch up the week after.

Very interested in your thought about an alternative TX pool. Are you thinking of something like --tx-pool=layered-freegas which is much like the new layered pool but with some free gas behaviours? Or something very different to the current layered implementation?

I think free gas and gas free are slightly different requirements from the users I've spoken to. For example, some users want gas in order to have the option of controlling how much use of the network a given participant has (by funding them a given amount of gas every so often) but want free gas, fifo, and probably the behaviour of always replace without a price bump.

It feels like the following combination is already pretty much available:

• --tx-pool=layered
• --min-gas-price=0 / zeroBaseFee: true
• --tx-pool-disable-locals / --tx-pool-no-local-priority
• --tx-pool-price-bump=0
• --tx-pool-disable-sender-grouping (i.e. this PR)

If all of those behaviours could be baked into a new type of TX pool then I can see that being appealing to users. Just choose that new pool type, and no need for specific settings. It does make me wonder whether the above options would still be available (i.e. to choose --tx-pool=layered-freegas but set --tx-pool-disable-locals back to false) or if they would all be N/A for the new implementation.

I think I might look at revising the PR (or perhaps creating a 2nd draft to keep the two separate) to use similar behaviour to this one, but changing the prioritizedTransactions order to oldest/smallest sequence number TX is chosen first, if the --tx-pool-disable-sender-grouping option is set. Because in its current state it feels like the code is unnecessarily complex because of the default ordering. That might give us an approach we're happy with to give FIFO-like ordering while the alternative TX pool implementation is discussed some more.

[fab-10]

Yes I am suggesting a new implementation, that is specific for use cases that are different from mainnet, with the requirements that you listed.
It could be a completely original implementation, optimized for the use case, or reuse something already implemented only if it make sense, we could also start thinking of making these implementations as plugins in case we see there is interest to make many different variants.

FYI: @7suyash7

[matthew1001]

Would you be opposed to an interim fix along the lines of the change in this PR? I think I agree that a custom pool implementation for these use-cases would be preferable, but a tactical non-breaking change to provide an option for the current pool implementation would be very useful in the mean time.

Out of interest I raised a draft PR (#6106) to see if it regressed any existing behaviour and get feedback on whether it's a valid change to make as a pre-req to this PR. It causes transactions to be sorted by ascending sequence number and added-time, not descending order. I can't think of a downside to that, and it would allow this PR to be much simplified.

[fab-10]

I am fine, finding an interim way to make this txpool fairer, will continue the review after #6106 is sorted out

[fab-10]

since the name of the tx is misleading with its nonce, you could consider to rename

[matthew1001]

Fixed

[fab-10]

suggested renaming to uniform the style

[matthew1001]

@fab-10 after updating the TX selection ordering, transactions are no longer grouped by sender (at least not in a way that is unfair to specific senders). I've repurposed this PR to add unit tests which demonstrate that to be the case.

[matthew1001]

@fab-10 I've marked this as auto-merge if/when you're happy with the new test.

[fab-10]

Suggested change

	## Experimental
	Xtx-pool-disable-sender-grouping=false

[fab-10]

removed since, only stable options are listed here