Add limits for number of transactions by sender #1716
Conversation
andreibancioiu
changed the title
andreibancioiu
changed the title
andreibancioiu
changed the title
|
|
||
| if nonce == incomingNonce && gasPrice > incomingGasPrice { | ||
| return element | ||
| // The incoming transaction will be placed right after the existing one, which has same nonce but higher price. | ||
| return element, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we should add a another case here for
nonce == incomingNonce && gasPrice <= incomingGasPrice and return the element - 1 position (if that exists) in order to put the transaction just before the existing. Otherwise, will exit on the last function return instruction, adding on the first position.
There was a problem hiding this comment.
Code in findInsertionPlace isn't currently very self-explanatory (and thus is decorated with a lot of comments), but if the I've found a place event does not happen in the iteration where the condition currentTxNonce == incomingNonce && currentTxGasPrice <= incomingGasPrice holds, the flow goes on and then stops at currentTxNonce < incomingNonce which is the most brutal / catch all condition of the iteration - and triggers the I've found a place event (and thus the SORT BY nonce ASC THEN BY gasPrice DESC holds).
Fixed ambiguity by adding an extra test and some comments.
When writing the test, I've discovered that I was incorrectly relying on require.ElementsMatch :)
| @@ -21,9 +21,15 @@ func (args *ArgShardedTxPool) verify() error { | |||
| if config.SizeInBytes < dataRetriever.TxPoolMinSizeInBytes { | |||
| return fmt.Errorf("%w: config.SizeInBytes is less than [dataRetriever.TxPoolMinSizeInBytes]", dataRetriever.ErrCacheConfigInvalidSizeInBytes) | |||
| } | |||
| if config.SizeInBytesPerSender < dataRetriever.TxPoolMinSizeInBytes { | |||
| return fmt.Errorf("%w: config.SizeInBytesPerSender is less than [dataRetriever.TxPoolMinSizeInBytes]", dataRetriever.ErrCacheConfigInvalidSizeInBytes) | |||
There was a problem hiding this comment.
what size does TxPoolMinSizeInBytes refer to?
if it is the size of a transaction then it is fine, although name is confusing, if it refers to the size of the pool itself then I don't think the condition is entirely correct, unless the minimum size we want for txpool is one tx.
There was a problem hiding this comment.
TxPoolMinSizeInBytes is (was) ~ 40kb, and added mostly to ensure that there are no zeros (just positives) in the configuration. This was an arbitrary number to account for the size in bytes of one transaction, indeed (like 1 was used for counts / num shards and so on).
Fixed to remove ambiguity, and now we use a simple condition for "strictly positive".
A sender can have no more than
ktransactions in the pool. The transactions can not exceedsbytes in total. These limits are checked and enforced at transaction insertion (addition) time.Furthermore, removed phase 1 of eviction ("evict high nonce transactions") - redundant. The previous phase 2 ("evict senders with bad score") is now phase 1.
Lastly, fixed the fee scale when computing the score of the senders - because of the new
minGasPrice(the change of 3 orders of magnitude affected the score formula).Note that this PR includes #1725, so that one should enter first in development - thus please skip testnet-related changes when reviewing this PR.