TxPool: refactoring for blob txs #5953

marcindsobczak · 2023-07-21T10:38:54Z

Changes

add to TxPoolConfig option of enabling blob pool BlobSupportEnabled (false by default for now)
add to TxPoolConfig option of enabling persistent blob txs storage PersistentBlobStorageEnabled (false by default for now)
add to TxPoolConfig configs of persistent blob storage - size of persistent blob db (16384 by default) and size of cache (256 by default)
add to TxPoolConfig size of in-memory pool InMemoryBlobPoolSize (512 by default) which is used if persistent storage is disabled
add to TxPoolConfig max number of pending transactions per one sender for blob txs (MaxPendingBlobTxsPerSender) and non-blob txs (MaxPendingTxsPerSender)
add db to store all blob txs ever received
add BlobTxStorage to handle reads/writes
add in-memory collection of light equivalents of blob txs and recreate it after restart
add collection BlobTxDistinctSortedPool extending TxDistinctSortedPool and overriding to make use of new components
add collection PersistentBlobTxDistinctSortedPool extending BlobTxDistinctSortedPool
add cache for recently used full blob txs
adjust filters, add new one - now it will be allowed for single sender to have pending blob tx OR other type tx, not both at once. After tx inclusion sender can send any type of tx again. It is inline with Geth behavior
add new filtering - reject blob txs if BlobSupportEnabled is false
add new filtering - reject blob txs if MaxPriorityFeePerGas is lower than 1 GWei
add new filtering - reject txs if number of already pending txs for sender is already equal MaxPendingTxsPerSender/(MaxPendingBlobTxsPerSender
add new replacement comparer dedicated for blob txs - requires at least 100% bump of all MaxFeePerGas, MaxPriorityFeePerGas and MaxFeePerDataGas + amount of blobs can't be lower
refactor TxBroadcaster - as blob txs are only announced, add only light equivalents of blob txs to broadcaster collection. Later, requested txs are coming from blob db (or cache) anyway
add plenty of blob-related metrics
add AcceptTxResult ReplacementNotAllowed dedicated for failed replacements - it returns FeeTooLowTooCompete before changes
add AcceptTxResult NonceTooFarInFuture for txs already having max number of txs in the pool (MaxPendingTxsPerSender/(MaxPendingBlobTxsPerSender)
add AcceptTxResult PendingTxsOfOtherType returned when sender has already tx of other type (only blob or non-blob ones are permitted, no mixing)
add AcceptTxResult NotSupportedTxType returned when there is incoming blob tx, but BlobSupportEnabled is false
update TxPool report

Potential improvements:

throttling for sending/receiving blob txs
evicting old blob txs based on timestamp (already saved to db)

How it works?

If BlobSupportEnabled is false, we are not supporting blob transactions (not accepting in the pool).
If PersistentBlobStorageEnabled is false, there is just another in-memory transactions collection dedicated for blobs, with max size of InMemoryBlobPoolSize. Is using different replacement comparer than TxDistinctSortedPool and except of it is behaving the same way. In TxBroadcaster there are only light equivalences of txs, as for picking and announcing there is no need of storing data and when requested, txs are coming from TxPool anyway

If PersistentBlobStorageEnabled is true, newly coming blob tx is added to 3 (or 4) collections

as full tx to persistent db
as full tx to cache
as light equivalences to standard sorted pool - let's name it LightPool
(optional) if tx is local, then light equivalence is added to broadcaster too

LightPool is responsible for:

sorting txs in optimal order
protecting from unsuccessful db reads - blob tx is asked from db only if is present in LightPool and it means that is present in persistent db as well
used in filters, when loading of other pending txs is needed
used for snapshot when building block - full tx is loaded from db only when is picked to the block

When blob tx is asked, it checks:

first in LightPool. If tx is not there, returning not found
if tx was in LightPool, checking cache
if not found in cache, checking persistent db. If there is no bug/error, it should be there

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

If yes, fill in the details here. Remove if not applicable.: ToDo

…gType

# Conflicts: # src/Nethermind/Ethereum.Test.Base/BlockchainTestBase.cs

src/Nethermind/Nethermind.Blockchain.Test/FullPruning/FullPruningDiskTest.cs

src/Nethermind/Nethermind.Db.Test/StandardDbInitializerTests.cs

LukaszRozmej · 2023-10-11T09:13:18Z

src/Nethermind/Nethermind.TxPool/Filters/FutureNonceFilter.cs

+        bool overflow = UInt256.AddOverflow(currentNonce, (UInt256)relevantMaxPendingTxsPerSender, out UInt256 maxAcceptedNonce);
+
+        // Overflow means that gap between current nonce of sender and UInt256.MaxValue is lower than allowed number
+        // of pending transactions. As lower nonces were rejected earlier, here it means tx accepted.
+        // So we are rejecting tx only if there is no overflow.
+        if (tx.Nonce > maxAcceptedNonce && !overflow)
+        {
+            Metrics.PendingTransactionsNonceTooFarInFuture++;
+            return AcceptTxResult.NonceTooFarInFuture;
+        }


I think we kind of should never overflow this value, so I think we can ignore overflow here?

It is exactly what we are doing.
It is an edge case (not real life one) when current nonce is e.g. MaxValue - 10.
So if we have overflow in maxAcceptedNonce = currentNonce + 16 it means, that we can accept this tx as it is not more than 16 nonces in future.
And we are doing it - returning AcceptTxResult.NonceTooFarInFuture only when there is !overflow (not overflow)

My point we may simplify the code by just not tracking overflow at all.

src/Nethermind/Nethermind.TxPool/LightTransaction.cs

LukaszRozmej · 2023-10-11T09:23:12Z

src/Nethermind/Nethermind.TxPool/LightTxDecoder.cs

+
+namespace Nethermind.TxPool;
+
+public class LightTxDecoder : TxDecoder<Transaction>


Should it be <LightTransaction>?

LukaszRozmej · 2023-10-11T09:40:55Z

src/Nethermind/Nethermind.TxPool/Collections/PersistentBlobTxDistinctSortedPool.cs

+        Stopwatch stopwatch = Stopwatch.StartNew();
+        foreach (LightTransaction lightBlobTx in blobTxStorage.GetAll())
+        {
+            if (base.TryInsert(lightBlobTx.Hash, lightBlobTx, out _))


If you make this method [MethodImpl(MethodImplOptions.Synchronized)] then each call to TryInsert won't lock/unlock this object - maybe giving better performance?

LukaszRozmej · 2023-10-11T09:43:29Z

src/Nethermind/Nethermind.TxPool/Collections/PersistentBlobTxDistinctSortedPool.cs

+    public override bool TryInsert(ValueKeccak hash, Transaction fullBlobTx, out Transaction? removed)
+    {
+        if (base.TryInsert(fullBlobTx.Hash, new LightTransaction(fullBlobTx), out removed))
+        {
+            _blobTxCache.Set(fullBlobTx.Hash, fullBlobTx);
+            _blobTxStorage.Add(fullBlobTx);
+            return true;
+        }
+
+        return false;
+    }
+
+    public override bool TryGetValue(ValueKeccak hash, [NotNullWhen(true)] out Transaction? fullBlobTx)
+    {
+        // Firstly check if tx is present in in-memory collection of light blob txs (without actual blobs).
+        // If not, just return false
+        if (base.TryGetValue(hash, out Transaction? lightTx))
+        {
+            // tx is present in light collection. Try to get full blob tx from cache
+            if (_blobTxCache.TryGet(hash, out fullBlobTx))
+            {
+                return true;
+            }
+
+            // tx is present, but not cached, at this point we need to load it from db...
+            if (_blobTxStorage.TryGet(hash, lightTx.SenderAddress!, lightTx.Timestamp, out fullBlobTx))
+            {
+                // ...and we are saving recently used blob tx to cache
+                _blobTxCache.Set(hash, fullBlobTx);
+                return true;
+            }
+        }
+
+        fullBlobTx = default;
+        return false;
+    }
+
+    protected override bool Remove(ValueKeccak hash, Transaction tx)


are overrides auto-synchronized?

LukaszRozmej · 2023-10-11T09:44:09Z

src/Nethermind/Nethermind.TxPool/Collections/TxDistinctSortedPool.cs

+        public virtual void VerifyCapacity()
+        {
+            if (Count > _poolCapacity && _logger.IsWarn)
+                _logger.Warn($"TxPool exceeds the config size {Count}/{_poolCapacity}");


parametrize this in constructor and don't override everywhere

LukaszRozmej · 2023-10-11T10:00:17Z

src/Nethermind/Nethermind.TxPool/BlobTxStorage.cs

+        foreach (byte[] txBytes in _lightBlobTxsDb.GetAllValues())
+        {
+            if (TryDecodeLightTx(txBytes, out LightTransaction? transaction))
+            {
+                yield return transaction!;
+            }
+        }
+    }


potential optimization: we could move to Spans saving on memory allocations.

LukaszRozmej · 2023-10-11T10:00:33Z

src/Nethermind/Nethermind.TxPool/BlobTxStorage.cs

+        byte[]? txBytes = _fullBlobTxsDb.Get(txHashPrefixed);
+        return TryDecodeFullTx(txBytes, sender, out transaction);


potential optimization: we could move to Spans saving on memory allocations.