Only recalculate storage roots once per block (#7021)

src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs

@@ -235,7 +235,7 @@ protected virtual TxReceipt[] ProcessBlock(
         _beaconBlockRootHandler.ApplyContractStateChanges(block, spec, _stateProvider);
         _blockhashStore.ApplyHistoryBlockHashes(block.Header);
 
-        _stateProvider.Commit(spec);
+        _stateProvider.Commit(spec, commitStorageRoots: false);
 
         TxReceipt[] receipts = _blockTransactionsExecutor.ProcessTransactions(block, options, ReceiptsTracer, spec);
 
@@ -249,7 +249,7 @@ protected virtual TxReceipt[] ProcessBlock(
         _withdrawalProcessor.ProcessWithdrawals(block, spec);
         ReceiptsTracer.EndBlockTrace();
 
-        _stateProvider.Commit(spec);
+        _stateProvider.Commit(spec, commitStorageRoots: true);
 
         if (ShouldComputeStateRoot(block.Header))
         {

src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs

@@ -125,7 +125,7 @@ protected virtual TransactionResult Execute(Transaction tx, in BlockExecutionCon
             if (!(result = BuyGas(tx, header, spec, tracer, opts, effectiveGasPrice, out UInt256 premiumPerGas, out UInt256 senderReservedGasPayment))) return result;
             if (!(result = IncrementNonce(tx, header, spec, tracer, opts))) return result;
 
-            if (commit) WorldState.Commit(spec, tracer.IsTracingState ? tracer : NullTxTracer.Instance);
+            if (commit) WorldState.Commit(spec, tracer.IsTracingState ? tracer : NullTxTracer.Instance, commitStorageRoots: false);
 
             ExecutionEnvironment env = BuildExecutionEnvironment(tx, in blCtx, spec, effectiveGasPrice);
 
@@ -153,7 +153,7 @@ protected virtual TransactionResult Execute(Transaction tx, in BlockExecutionCon
             }
             else if (commit)
             {
-                WorldState.Commit(spec, tracer.IsTracingState ? tracer : NullStateTracer.Instance);
+                WorldState.Commit(spec, tracer.IsTracingState ? tracer : NullStateTracer.Instance, commitStorageRoots: !spec.IsEip658Enabled);
             }
 
             if (tracer.IsTracingReceipt)

src/Nethermind/Nethermind.State/IWorldState.cs

@@ -99,9 +99,9 @@ public interface IWorldState : IJournal<Snapshot>, IReadOnlyStateProvider
 
     /* snapshots */
 
-    void Commit(IReleaseSpec releaseSpec, bool isGenesis = false);
+    void Commit(IReleaseSpec releaseSpec, bool isGenesis = false, bool commitStorageRoots = true);
 
-    void Commit(IReleaseSpec releaseSpec, IWorldStateTracer? traver, bool isGenesis = false);
+    void Commit(IReleaseSpec releaseSpec, IWorldStateTracer? tracer, bool isGenesis = false, bool commitStorageRoots = true);
 
     void CommitTree(long blockNumber);
 }

src/Nethermind/Nethermind.State/PartialStorageProviderBase.cs

@@ -145,9 +145,9 @@ public void Restore(int snapshot)
         /// <summary>
         /// Commit persistent storage
         /// </summary>
-        public void Commit()
+        public void Commit(bool commitStorageRoots = true)
         {
-            Commit(NullStateTracer.Instance);
+            Commit(NullStateTracer.Instance, commitStorageRoots);
         }
 
         protected readonly struct ChangeTrace
@@ -172,7 +172,7 @@ public ChangeTrace(byte[]? after)
         /// Commit persistent storage
         /// </summary>
         /// <param name="stateTracer">State tracer</param>
-        public void Commit(IStorageTracer tracer)
+        public void Commit(IStorageTracer tracer, bool commitStorageRoots = true)
         {
             if (_currentPosition == Snapshot.EmptyPosition)
             {
@@ -182,6 +182,16 @@ public void Commit(IStorageTracer tracer)
             {
                 CommitCore(tracer);
             }
+
+            if (commitStorageRoots)
+            {
+                CommitStorageRoots();
+            }
+        }
+
+        protected virtual void CommitStorageRoots()
+        {
+            // Commit storage roots
         }
 
         /// <summary>

src/Nethermind/Nethermind.State/PersistentStorageProvider.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Threading.Tasks;
 using Nethermind.Core;
 using Nethermind.Core.Collections;
 using Nethermind.Core.Crypto;
@@ -25,6 +26,7 @@ internal class PersistentStorageProvider : PartialStorageProviderBase
         private readonly ILogManager? _logManager;
         internal readonly IStorageTreeFactory _storageTreeFactory;
         private readonly ResettableDictionary<AddressAsKey, StorageTree> _storages = new();
+        private readonly HashSet<Address> _toUpdateRoots = new();
 
         /// <summary>
         /// EIP-1283
@@ -53,6 +55,7 @@ public override void Reset()
             _storages.Reset();
             _originalValues.Clear();
             _committedThisRound.Clear();
+            _toUpdateRoots.Clear();
         }
 
         /// <summary>
@@ -179,16 +182,17 @@ protected override void CommitCore(IStorageTracer tracer)
                 }
             }
 
-            // TODO: it seems that we are unnecessarily recalculating root hashes all the time in storage?
             foreach (Address address in toUpdateRoots)
             {
                 // since the accounts could be empty accounts that are removing (EIP-158)
                 if (_stateProvider.AccountExists(address))
                 {
-                    Hash256 root = RecalculateRootHash(address);
-
-                    // _logger.Warn($"Recalculating storage root {address}->{root} ({toUpdateRoots.Count})");
-                    _stateProvider.UpdateStorageRoot(address, root);
+                    _toUpdateRoots.Add(address);
+                }
+                else
+                {
+                    _toUpdateRoots.Remove(address);
+                    _storages.Remove(address);
                 }
             }
 
@@ -202,6 +206,68 @@ protected override void CommitCore(IStorageTracer tracer)
             }
         }
 
+        protected override void CommitStorageRoots()
+        {
+            if (_toUpdateRoots.Count == 0)
+            {
+                return;
+            }
+
+            // Is overhead of parallel foreach worth it?
+            if (_toUpdateRoots.Count <= 4)
+            {
+                UpdateRootHashesSingleThread();
+            }
+            else
+            {
+                UpdateRootHashesMultiThread();
+            }
+
+            void UpdateRootHashesSingleThread()
+            {
+                foreach (KeyValuePair<AddressAsKey, StorageTree> kvp in _storages)
+                {
+                    if (!_toUpdateRoots.Contains(kvp.Key))
+                    {
+                        // Wasn't updated don't recalculate
+                        continue;
+                    }
+
+                    StorageTree storageTree = kvp.Value;
+                    storageTree.UpdateRootHash();
+                    _stateProvider.UpdateStorageRoot(address: kvp.Key, storageTree.RootHash);
+                }
+            }
+
+            void UpdateRootHashesMultiThread()
+            {
+                // We can recalculate the roots in parallel as they are all independent tries
+                Parallel.ForEach(_storages, kvp =>
+                {
+                    if (!_toUpdateRoots.Contains(kvp.Key))
+                    {
+                        // Wasn't updated don't recalculate
+                        return;
+                    }
+                    StorageTree storageTree = kvp.Value;
+                    storageTree.UpdateRootHash();
+                });
+
+                // Update the storage roots in the main thread non in parallel
+                foreach (KeyValuePair<AddressAsKey, StorageTree> kvp in _storages)
+                {
+                    if (!_toUpdateRoots.Contains(kvp.Key))
+                    {
+                        continue;
+                    }
+
+                    // Update the storage root for the Account
+                    _stateProvider.UpdateStorageRoot(address: kvp.Key, kvp.Value.RootHash);
+                }
+
+            }
+        }
+
         private void SaveToTree(HashSet<Address> toUpdateRoots, Change change)
         {
             if (_originalValues.TryGetValue(change.StorageCell, out byte[] initialValue) &&
@@ -223,14 +289,16 @@ private void SaveToTree(HashSet<Address> toUpdateRoots, Change change)
         /// <param name="blockNumber">Current block number</param>
         public void CommitTrees(long blockNumber)
         {
-            // _logger.Warn($"Storage block commit {blockNumber}");
             foreach (KeyValuePair<AddressAsKey, StorageTree> storage in _storages)
             {
+                if (!_toUpdateRoots.Contains(storage.Key))
+                {
+                    continue;
+                }
                 storage.Value.Commit(blockNumber);
             }
 
-            // TODO: maybe I could update storage roots only now?
-
+            _toUpdateRoots.Clear();
             // only needed here as there is no control over cached storage size otherwise
             _storages.Reset();
         }
@@ -305,6 +373,7 @@ public override void ClearStorage(Address address)
             // by means of CREATE 2 - notice that the cached trie may carry information about items that were not
             // touched in this block, hence were not zeroed above
             // TODO: how does it work with pruning?
+            _toUpdateRoots.Remove(address);
             _storages[address] = new StorageTree(_trieStore.GetTrieStore(address.ToAccountPath), Keccak.EmptyTreeHash, _logManager);
         }
 

src/Nethermind/Nethermind.State/WorldState.cs

@@ -193,16 +193,16 @@ public bool HasStateForRoot(Hash256 stateRoot)
             return _trieStore.HasRoot(stateRoot);
         }
 
-        public void Commit(IReleaseSpec releaseSpec, bool isGenesis = false)
+        public void Commit(IReleaseSpec releaseSpec, bool isGenesis = false, bool commitStorageRoots = true)
         {
-            _persistentStorageProvider.Commit();
-            _transientStorageProvider.Commit();
+            _persistentStorageProvider.Commit(commitStorageRoots);
+            _transientStorageProvider.Commit(commitStorageRoots);
             _stateProvider.Commit(releaseSpec, isGenesis);
         }
-        public void Commit(IReleaseSpec releaseSpec, IWorldStateTracer tracer, bool isGenesis = false)
+        public void Commit(IReleaseSpec releaseSpec, IWorldStateTracer tracer, bool isGenesis = false, bool commitStorageRoots = true)
         {
-            _persistentStorageProvider.Commit(tracer);
-            _transientStorageProvider.Commit(tracer);
+            _persistentStorageProvider.Commit(tracer, commitStorageRoots);
+            _transientStorageProvider.Commit(tracer, commitStorageRoots);
             _stateProvider.Commit(releaseSpec, tracer, isGenesis);
         }