From 102a471bbf066732afbbc3508a408e44a062cb0a Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sun, 19 May 2024 00:52:32 +0100 Subject: [PATCH 1/4] Only recalculate storage hashes once per block --- .../Processing/BlockProcessor.cs | 2 +- .../Nethermind.State/IWorldState.cs | 1 + .../PartialStorageProviderBase.cs | 8 ++ .../PersistentStorageProvider.cs | 87 +++++++++++++++++-- src/Nethermind/Nethermind.State/WorldState.cs | 6 ++ 5 files changed, 95 insertions(+), 9 deletions(-) diff --git a/src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs b/src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs index 365f1bef5fa..63c6aa22278 100644 --- a/src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs +++ b/src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs @@ -249,7 +249,7 @@ protected virtual TxReceipt[] ProcessBlock( _withdrawalProcessor.ProcessWithdrawals(block, spec); ReceiptsTracer.EndBlockTrace(); - _stateProvider.Commit(spec); + _stateProvider.PostCommit(spec); if (ShouldComputeStateRoot(block.Header)) { diff --git a/src/Nethermind/Nethermind.State/IWorldState.cs b/src/Nethermind/Nethermind.State/IWorldState.cs index b2ffdfc812f..74d269671c2 100644 --- a/src/Nethermind/Nethermind.State/IWorldState.cs +++ b/src/Nethermind/Nethermind.State/IWorldState.cs @@ -104,4 +104,5 @@ public interface IWorldState : IJournal, IReadOnlyStateProvider void Commit(IReleaseSpec releaseSpec, IWorldStateTracer? traver, bool isGenesis = false); void CommitTree(long blockNumber); + void PostCommit(IReleaseSpec releaseSpec); } diff --git a/src/Nethermind/Nethermind.State/PartialStorageProviderBase.cs b/src/Nethermind/Nethermind.State/PartialStorageProviderBase.cs index e3e4bc676c8..1f0586dc83c 100644 --- a/src/Nethermind/Nethermind.State/PartialStorageProviderBase.cs +++ b/src/Nethermind/Nethermind.State/PartialStorageProviderBase.cs @@ -150,6 +150,14 @@ public void Commit() Commit(NullStateTracer.Instance); } + public void PostCommit() + { + Commit(NullStateTracer.Instance); + PostCommitStorages(); + } + + protected virtual void PostCommitStorages() { } + protected readonly struct ChangeTrace { public ChangeTrace(byte[]? before, byte[]? after) diff --git a/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs b/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs index 820655d53b8..012d7fdbb49 100644 --- a/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs +++ b/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 _storages = new(); + private readonly HashSet
_toUpdateRoots = new(); /// /// EIP-1283 @@ -53,6 +55,7 @@ public override void Reset() _storages.Reset(); _originalValues.Clear(); _committedThisRound.Clear(); + _toUpdateRoots.Clear(); } /// @@ -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,75 @@ protected override void CommitCore(IStorageTracer tracer) } } + protected override void PostCommitStorages() + { + if (_toUpdateRoots.Count == 0) + { + return; + } + + // Is overhead of parallel foreach worth it? + if (_toUpdateRoots.Count <= 4) + { + UpdateRootHashesSingleThread(); + } + else + { + UpdateRootHashesMultiThread(); + } + + void UpdateRootHashesSingleThread() + { + foreach (KeyValuePair kvp in _storages) + { + if (!_toUpdateRoots.Contains(kvp.Key)) + { + // Remove the storage tree if it was only read and not updated + _storages.Remove(kvp.Key); + continue; + } + + StorageTree storageTree = kvp.Value; + storageTree.UpdateRootHash(); + _stateProvider.UpdateStorageRoot(address: kvp.Key, storageTree.RootHash); + } + + _toUpdateRoots.Clear(); + } + + 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; we don't remove here + // in parallel foreach as it would be unsafe on non-concurrent dictionary + return; + } + StorageTree storageTree = kvp.Value; + storageTree.UpdateRootHash(); + }); + + // Update the storage roots in the main thread non in parallel + foreach (KeyValuePair kvp in _storages) + { + if (!_toUpdateRoots.Contains(kvp.Key)) + { + // Remove the storage tree if it was only read and not updated + _storages.Remove(kvp.Key); + continue; + } + + // Update the storage root for the Account + _stateProvider.UpdateStorageRoot(address: kvp.Key, kvp.Value.RootHash); + } + + _toUpdateRoots.Clear(); + } + } + private void SaveToTree(HashSet
toUpdateRoots, Change change) { if (_originalValues.TryGetValue(change.StorageCell, out byte[] initialValue) && @@ -223,14 +296,11 @@ private void SaveToTree(HashSet
toUpdateRoots, Change change) /// Current block number public void CommitTrees(long blockNumber) { - // _logger.Warn($"Storage block commit {blockNumber}"); foreach (KeyValuePair storage in _storages) { storage.Value.Commit(blockNumber); } - // TODO: maybe I could update storage roots only now? - // only needed here as there is no control over cached storage size otherwise _storages.Reset(); } @@ -305,6 +375,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.Add(address); _storages[address] = new StorageTree(_trieStore.GetTrieStore(address.ToAccountPath), Keccak.EmptyTreeHash, _logManager); } diff --git a/src/Nethermind/Nethermind.State/WorldState.cs b/src/Nethermind/Nethermind.State/WorldState.cs index bb88cb42eff..31b4b9fb916 100644 --- a/src/Nethermind/Nethermind.State/WorldState.cs +++ b/src/Nethermind/Nethermind.State/WorldState.cs @@ -205,6 +205,12 @@ public void Commit(IReleaseSpec releaseSpec, IWorldStateTracer tracer, bool isGe _transientStorageProvider.Commit(tracer); _stateProvider.Commit(releaseSpec, tracer, isGenesis); } + public void PostCommit(IReleaseSpec releaseSpec) + { + _persistentStorageProvider.PostCommit(); + _transientStorageProvider.PostCommit(); + _stateProvider.Commit(releaseSpec, isGenesis: false); + } public Snapshot TakeSnapshot(bool newTransactionStart = false) { From a5eb81a665deb2ffb4d8cb79c7053cc5e719b8c1 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sun, 19 May 2024 01:00:31 +0100 Subject: [PATCH 2/4] Remove not add --- src/Nethermind/Nethermind.State/PersistentStorageProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs b/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs index 012d7fdbb49..bd3c6471207 100644 --- a/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs +++ b/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs @@ -375,7 +375,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.Add(address); + _toUpdateRoots.Remove(address); _storages[address] = new StorageTree(_trieStore.GetTrieStore(address.ToAccountPath), Keccak.EmptyTreeHash, _logManager); } From c08268b5361cb7449335d7dbf1e62638bd500a6b Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sun, 19 May 2024 05:12:13 +0100 Subject: [PATCH 3/4] Less changes --- .../Processing/BlockProcessor.cs | 4 ++-- .../TransactionProcessor.cs | 4 ++-- .../Nethermind.State/IWorldState.cs | 5 ++-- .../PartialStorageProviderBase.cs | 24 ++++++++++--------- .../PersistentStorageProvider.cs | 18 +++++++------- src/Nethermind/Nethermind.State/WorldState.cs | 18 +++++--------- 6 files changed, 33 insertions(+), 40 deletions(-) diff --git a/src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs b/src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs index 63c6aa22278..9170e9a8758 100644 --- a/src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs +++ b/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.PostCommit(spec); + _stateProvider.Commit(spec, commitStorageRoots: true); if (ShouldComputeStateRoot(block.Header)) { diff --git a/src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs b/src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs index 4b97a503590..fc529787062 100644 --- a/src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs +++ b/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: false); } if (tracer.IsTracingReceipt) diff --git a/src/Nethermind/Nethermind.State/IWorldState.cs b/src/Nethermind/Nethermind.State/IWorldState.cs index 74d269671c2..f0c8f82ff73 100644 --- a/src/Nethermind/Nethermind.State/IWorldState.cs +++ b/src/Nethermind/Nethermind.State/IWorldState.cs @@ -99,10 +99,9 @@ public interface IWorldState : IJournal, 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); - void PostCommit(IReleaseSpec releaseSpec); } diff --git a/src/Nethermind/Nethermind.State/PartialStorageProviderBase.cs b/src/Nethermind/Nethermind.State/PartialStorageProviderBase.cs index 1f0586dc83c..20af7e01c72 100644 --- a/src/Nethermind/Nethermind.State/PartialStorageProviderBase.cs +++ b/src/Nethermind/Nethermind.State/PartialStorageProviderBase.cs @@ -145,19 +145,11 @@ public void Restore(int snapshot) /// /// Commit persistent storage /// - public void Commit() + public void Commit(bool commitStorageRoots = true) { - Commit(NullStateTracer.Instance); + Commit(NullStateTracer.Instance, commitStorageRoots); } - public void PostCommit() - { - Commit(NullStateTracer.Instance); - PostCommitStorages(); - } - - protected virtual void PostCommitStorages() { } - protected readonly struct ChangeTrace { public ChangeTrace(byte[]? before, byte[]? after) @@ -180,7 +172,7 @@ public ChangeTrace(byte[]? after) /// Commit persistent storage ///
/// State tracer - public void Commit(IStorageTracer tracer) + public void Commit(IStorageTracer tracer, bool commitStorageRoots = true) { if (_currentPosition == Snapshot.EmptyPosition) { @@ -190,6 +182,16 @@ public void Commit(IStorageTracer tracer) { CommitCore(tracer); } + + if (commitStorageRoots) + { + CommitStorageRoots(); + } + } + + protected virtual void CommitStorageRoots() + { + // Commit storage roots } /// diff --git a/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs b/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs index bd3c6471207..c6abed3a839 100644 --- a/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs +++ b/src/Nethermind/Nethermind.State/PersistentStorageProvider.cs @@ -206,7 +206,7 @@ protected override void CommitCore(IStorageTracer tracer) } } - protected override void PostCommitStorages() + protected override void CommitStorageRoots() { if (_toUpdateRoots.Count == 0) { @@ -229,8 +229,7 @@ void UpdateRootHashesSingleThread() { if (!_toUpdateRoots.Contains(kvp.Key)) { - // Remove the storage tree if it was only read and not updated - _storages.Remove(kvp.Key); + // Wasn't updated don't recalculate continue; } @@ -238,8 +237,6 @@ void UpdateRootHashesSingleThread() storageTree.UpdateRootHash(); _stateProvider.UpdateStorageRoot(address: kvp.Key, storageTree.RootHash); } - - _toUpdateRoots.Clear(); } void UpdateRootHashesMultiThread() @@ -249,8 +246,7 @@ void UpdateRootHashesMultiThread() { if (!_toUpdateRoots.Contains(kvp.Key)) { - // Wasn't updated don't recalculate; we don't remove here - // in parallel foreach as it would be unsafe on non-concurrent dictionary + // Wasn't updated don't recalculate return; } StorageTree storageTree = kvp.Value; @@ -262,8 +258,6 @@ void UpdateRootHashesMultiThread() { if (!_toUpdateRoots.Contains(kvp.Key)) { - // Remove the storage tree if it was only read and not updated - _storages.Remove(kvp.Key); continue; } @@ -271,7 +265,6 @@ void UpdateRootHashesMultiThread() _stateProvider.UpdateStorageRoot(address: kvp.Key, kvp.Value.RootHash); } - _toUpdateRoots.Clear(); } } @@ -298,9 +291,14 @@ public void CommitTrees(long blockNumber) { foreach (KeyValuePair storage in _storages) { + if (!_toUpdateRoots.Contains(storage.Key)) + { + continue; + } storage.Value.Commit(blockNumber); } + _toUpdateRoots.Clear(); // only needed here as there is no control over cached storage size otherwise _storages.Reset(); } diff --git a/src/Nethermind/Nethermind.State/WorldState.cs b/src/Nethermind/Nethermind.State/WorldState.cs index 31b4b9fb916..c55ec0cc0e6 100644 --- a/src/Nethermind/Nethermind.State/WorldState.cs +++ b/src/Nethermind/Nethermind.State/WorldState.cs @@ -193,24 +193,18 @@ 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); } - public void PostCommit(IReleaseSpec releaseSpec) - { - _persistentStorageProvider.PostCommit(); - _transientStorageProvider.PostCommit(); - _stateProvider.Commit(releaseSpec, isGenesis: false); - } public Snapshot TakeSnapshot(bool newTransactionStart = false) { From 1657a5ae6e368f742bb3402c771f064692abfd6e Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Sun, 19 May 2024 15:39:14 +0100 Subject: [PATCH 4/4] Legacy fix --- .../TransactionProcessing/TransactionProcessor.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs b/src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs index fc529787062..4c98d6eee6e 100644 --- a/src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs +++ b/src/Nethermind/Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs @@ -153,7 +153,7 @@ protected virtual TransactionResult Execute(Transaction tx, in BlockExecutionCon } else if (commit) { - WorldState.Commit(spec, tracer.IsTracingState ? tracer : NullStateTracer.Instance, commitStorageRoots: false); + WorldState.Commit(spec, tracer.IsTracingState ? tracer : NullStateTracer.Instance, commitStorageRoots: !spec.IsEip658Enabled); } if (tracer.IsTracingReceipt)