From cc2ca559461ed6b1dd9749e69b8b54033daa61df Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Fri, 8 Mar 2024 05:14:25 -0800 Subject: [PATCH 1/8] core/state: in IntermediateRoot perform state object updates before deletions to prevent potential unnecessary trie node resolution. --- core/state/statedb.go | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/core/state/statedb.go b/core/state/statedb.go index f90b30f3994e..18febe218137 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -895,15 +895,28 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { } } usedAddrs := make([][]byte, 0, len(s.stateObjectsPending)) + // Perform updates before deletions. This prevents resolution of unnecessary trie nodes + // in circumstances similar to the following:s + // + // Take value nodes 1, 2 who share the same full node parent 3 and have no other siblings. + // 1 self-destructs specifying a non-existing recipient account which would be a child of 3. + // If the deletion of 1 happens before the account-creating balance transfer, 3 will temporarily + // be collapsed to a short node on 2, requiring 2 to be unecessarily resolved. + var deletedObjects []*stateObject for addr := range s.stateObjectsPending { - if obj := s.stateObjects[addr]; obj.deleted { - s.deleteStateObject(obj) - s.AccountDeleted += 1 - } else { + if obj := s.stateObjects[addr]; !obj.deleted { s.updateStateObject(obj) s.AccountUpdated += 1 + usedAddrs = append(usedAddrs, common.CopyBytes(addr[:])) // Copy needed for closure + } else { + deletedObjects = append(deletedObjects, obj) } - usedAddrs = append(usedAddrs, common.CopyBytes(addr[:])) // Copy needed for closure + usedAddrs = append(usedAddrs, common.CopyBytes(addr[:])) + } + for _, deletedObj := range deletedObjects { + s.deleteStateObject(deletedObj) + s.AccountDeleted += 1 + usedAddrs = append(usedAddrs, common.CopyBytes(deletedObj.address[:])) // Copy needed for closure } if prefetcher != nil { prefetcher.used(common.Hash{}, s.originalRoot, usedAddrs) From 55eb1619229401d2a24b6ee1389013173640673d Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Fri, 8 Mar 2024 05:33:27 -0800 Subject: [PATCH 2/8] remove character --- core/state/statedb.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/state/statedb.go b/core/state/statedb.go index 18febe218137..7e326d803d2d 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -896,7 +896,7 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { } usedAddrs := make([][]byte, 0, len(s.stateObjectsPending)) // Perform updates before deletions. This prevents resolution of unnecessary trie nodes - // in circumstances similar to the following:s + // in circumstances similar to the following: // // Take value nodes 1, 2 who share the same full node parent 3 and have no other siblings. // 1 self-destructs specifying a non-existing recipient account which would be a child of 3. From 9373700e0c844f702aa9cd686b282b76f64ef9a2 Mon Sep 17 00:00:00 2001 From: jwasinger Date: Fri, 8 Mar 2024 06:25:25 -0800 Subject: [PATCH 3/8] Update core/state/statedb.go Co-authored-by: Martin HS --- core/state/statedb.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/core/state/statedb.go b/core/state/statedb.go index 7e326d803d2d..24236ad590a0 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -898,10 +898,13 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { // Perform updates before deletions. This prevents resolution of unnecessary trie nodes // in circumstances similar to the following: // - // Take value nodes 1, 2 who share the same full node parent 3 and have no other siblings. - // 1 self-destructs specifying a non-existing recipient account which would be a child of 3. - // If the deletion of 1 happens before the account-creating balance transfer, 3 will temporarily - // be collapsed to a short node on 2, requiring 2 to be unecessarily resolved. + // Consider nodes `A` and `B` who share the same full node parent `P` and have no other siblings. + // During the execution of a block: + // - `A` self-destructs, + // - `C` is created, and also shares the parent `P`. + // If the self-destruct is handled first, then `P` would be left with only one child, thus collapsed + // into a shortnode. This requires `B` to be resolved from disk. + // Whereas if the created node is handled first, then the collapse is avoided, and `B` is not resolved. var deletedObjects []*stateObject for addr := range s.stateObjectsPending { if obj := s.stateObjects[addr]; !obj.deleted { From c37d1a75e9ce7fe7e25c01ef52d55fe3dee07899 Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Fri, 8 Mar 2024 06:31:18 -0800 Subject: [PATCH 4/8] correct accumulation of usedAddrs in IntermediateRoot --- core/state/statedb.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/core/state/statedb.go b/core/state/statedb.go index 24236ad590a0..73e6bc4c6671 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -900,26 +900,24 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { // // Consider nodes `A` and `B` who share the same full node parent `P` and have no other siblings. // During the execution of a block: - // - `A` self-destructs, - // - `C` is created, and also shares the parent `P`. + // - `A` self-destructs, + // - `C` is created, and also shares the parent `P`. // If the self-destruct is handled first, then `P` would be left with only one child, thus collapsed - // into a shortnode. This requires `B` to be resolved from disk. + // into a shortnode. This requires `B` to be resolved from disk. // Whereas if the created node is handled first, then the collapse is avoided, and `B` is not resolved. var deletedObjects []*stateObject for addr := range s.stateObjectsPending { if obj := s.stateObjects[addr]; !obj.deleted { s.updateStateObject(obj) s.AccountUpdated += 1 - usedAddrs = append(usedAddrs, common.CopyBytes(addr[:])) // Copy needed for closure } else { deletedObjects = append(deletedObjects, obj) } - usedAddrs = append(usedAddrs, common.CopyBytes(addr[:])) + usedAddrs = append(usedAddrs, common.CopyBytes(addr[:])) // Copy needed for closure } for _, deletedObj := range deletedObjects { s.deleteStateObject(deletedObj) s.AccountDeleted += 1 - usedAddrs = append(usedAddrs, common.CopyBytes(deletedObj.address[:])) // Copy needed for closure } if prefetcher != nil { prefetcher.used(common.Hash{}, s.originalRoot, usedAddrs) From 23e6582a20855c3790615977facee36799a73225 Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Sun, 10 Mar 2024 22:04:47 -0700 Subject: [PATCH 5/8] when committing storage trie, apply updates before deletions --- core/state/state_object.go | 42 +++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/core/state/state_object.go b/core/state/state_object.go index 6dea68465baa..dbc493cd882d 100644 --- a/core/state/state_object.go +++ b/core/state/state_object.go @@ -294,11 +294,8 @@ func (s *stateObject) updateTrie() (Trie, error) { } // Insert all the pending storage updates into the trie usedStorage := make([][]byte, 0, len(s.pendingStorage)) - for key, value := range s.pendingStorage { - // Skip noop changes, persist actual changes - if value == s.originStorage[key] { - continue - } + // insertStorageChange updates a storage slot + insertStorageChange := func(key, value common.Hash) error { prev := s.originStorage[key] s.originStorage[key] = value @@ -306,7 +303,7 @@ func (s *stateObject) updateTrie() (Trie, error) { if (value == common.Hash{}) { if err := tr.DeleteStorage(s.address, key[:]); err != nil { s.db.setError(err) - return nil, err + return err } s.db.StorageDeleted += 1 } else { @@ -315,7 +312,7 @@ func (s *stateObject) updateTrie() (Trie, error) { encoded, _ = rlp.EncodeToBytes(trimmed) if err := tr.UpdateStorage(s.address, key[:], trimmed); err != nil { s.db.setError(err) - return nil, err + return err } s.db.StorageUpdated += 1 } @@ -348,6 +345,37 @@ func (s *stateObject) updateTrie() (Trie, error) { } // Cache the items for preloading usedStorage = append(usedStorage, common.CopyBytes(key[:])) // Copy needed for closure + return nil + } + + // Perform trie updates before deletions. This prevents resolution of unnecessary trie nodes + // in circumstances similar to the following: + // + // Consider nodes `A` and `B` who share the same full node parent `P` and have no other siblings. + // During the execution of a block: + // - `A` is deleted, + // - `C` is created, and also shares the parent `P`. + // If the deletion is handled first, then `P` would be left with only one child, thus collapsed + // into a shortnode. This requires `B` to be resolved from disk. + // Whereas if the created node is handled first, then the collapse is avoided, and `B` is not resolved. + deletedStorages := make(map[common.Hash]common.Hash) + for key, value := range s.pendingStorage { + // Skip noop changes, persist actual changes + if value == s.originStorage[key] { + continue + } + if (value != common.Hash{}) { + if err := insertStorageChange(key, value); err != nil { + return nil, err + } + } else { + deletedStorages[key] = value + } + } + for key, value := range deletedStorages { + if err := insertStorageChange(key, value); err != nil { + return nil, err + } } if s.db.prefetcher != nil { s.db.prefetcher.used(s.addrHash, s.data.Root, usedStorage) From d1844fb3461466c1d99bd586718731e162e57262 Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Mon, 11 Mar 2024 11:40:50 -0700 Subject: [PATCH 6/8] make it cleaner --- core/state/state_object.go | 47 +++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/core/state/state_object.go b/core/state/state_object.go index dbc493cd882d..be2b49d269aa 100644 --- a/core/state/state_object.go +++ b/core/state/state_object.go @@ -295,27 +295,15 @@ func (s *stateObject) updateTrie() (Trie, error) { // Insert all the pending storage updates into the trie usedStorage := make([][]byte, 0, len(s.pendingStorage)) // insertStorageChange updates a storage slot - insertStorageChange := func(key, value common.Hash) error { + insertStorageChange := func(key, value common.Hash, updateTr func() ([]byte, error)) error { prev := s.originStorage[key] s.originStorage[key] = value var encoded []byte // rlp-encoded value to be used by the snapshot - if (value == common.Hash{}) { - if err := tr.DeleteStorage(s.address, key[:]); err != nil { - s.db.setError(err) - return err - } - s.db.StorageDeleted += 1 - } else { - // Encoding []byte cannot fail, ok to ignore the error. - trimmed := common.TrimLeftZeroes(value[:]) - encoded, _ = rlp.EncodeToBytes(trimmed) - if err := tr.UpdateStorage(s.address, key[:], trimmed); err != nil { - s.db.setError(err) - return err - } - s.db.StorageUpdated += 1 + if encoded, err = updateTr(); err != nil { + return err } + // Cache the mutated storage slots until commit if storage == nil { if storage = s.db.storages[s.addrHash]; storage == nil { @@ -347,6 +335,29 @@ func (s *stateObject) updateTrie() (Trie, error) { usedStorage = append(usedStorage, common.CopyBytes(key[:])) // Copy needed for closure return nil } + applyDeletion := func(key, value common.Hash) error { + return insertStorageChange(key, value, func() (encoded []byte, err error) { + if err := tr.DeleteStorage(s.address, key[:]); err != nil { + s.db.setError(err) + return nil, err + } + s.db.StorageDeleted += 1 + return nil, nil + }) + } + applyUpdate := func(key, value common.Hash) error { + return insertStorageChange(key, value, func() (encoded []byte, err error) { + // Encoding []byte cannot fail, ok to ignore the error. + trimmed := common.TrimLeftZeroes(value[:]) + encoded, _ = rlp.EncodeToBytes(trimmed) + if err := tr.UpdateStorage(s.address, key[:], trimmed); err != nil { + s.db.setError(err) + return nil, err + } + s.db.StorageUpdated += 1 + return encoded, nil + }) + } // Perform trie updates before deletions. This prevents resolution of unnecessary trie nodes // in circumstances similar to the following: @@ -365,7 +376,7 @@ func (s *stateObject) updateTrie() (Trie, error) { continue } if (value != common.Hash{}) { - if err := insertStorageChange(key, value); err != nil { + if err := applyUpdate(key, value); err != nil { return nil, err } } else { @@ -373,7 +384,7 @@ func (s *stateObject) updateTrie() (Trie, error) { } } for key, value := range deletedStorages { - if err := insertStorageChange(key, value); err != nil { + if err := applyDeletion(key, value); err != nil { return nil, err } } From 8065f98b3686e0ca0050b25f3641fe9c09b101ea Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Mon, 11 Mar 2024 13:18:47 -0700 Subject: [PATCH 7/8] simplify updateTrie Co-authored-by: Martin HS --- core/state/state_object.go | 88 ++++++++++++++------------------------ 1 file changed, 32 insertions(+), 56 deletions(-) diff --git a/core/state/state_object.go b/core/state/state_object.go index be2b49d269aa..d3d7765256dd 100644 --- a/core/state/state_object.go +++ b/core/state/state_object.go @@ -294,16 +294,39 @@ func (s *stateObject) updateTrie() (Trie, error) { } // Insert all the pending storage updates into the trie usedStorage := make([][]byte, 0, len(s.pendingStorage)) - // insertStorageChange updates a storage slot - insertStorageChange := func(key, value common.Hash, updateTr func() ([]byte, error)) error { + + // Perform trie updates before deletions. This prevents resolution of unnecessary trie nodes + // in circumstances similar to the following: + // + // Consider nodes `A` and `B` who share the same full node parent `P` and have no other siblings. + // During the execution of a block: + // - `A` is deleted, + // - `C` is created, and also shares the parent `P`. + // If the deletion is handled first, then `P` would be left with only one child, thus collapsed + // into a shortnode. This requires `B` to be resolved from disk. + // Whereas if the created node is handled first, then the collapse is avoided, and `B` is not resolved. + var deletions []common.Hash + for key, value := range s.pendingStorage { + // Skip noop changes, persist actual changes + if value == s.originStorage[key] { + continue + } prev := s.originStorage[key] s.originStorage[key] = value var encoded []byte // rlp-encoded value to be used by the snapshot - if encoded, err = updateTr(); err != nil { - return err + if (value != common.Hash{}) { + // Encoding []byte cannot fail, ok to ignore the error. + trimmed := common.TrimLeftZeroes(value[:]) + encoded, _ = rlp.EncodeToBytes(trimmed) + if err := tr.UpdateStorage(s.address, key[:], trimmed); err != nil { + s.db.setError(err) + return nil, err + } + s.db.StorageUpdated += 1 + } else { + deletions = append(deletions, key) } - // Cache the mutated storage slots until commit if storage == nil { if storage = s.db.storages[s.addrHash]; storage == nil { @@ -333,60 +356,13 @@ func (s *stateObject) updateTrie() (Trie, error) { } // Cache the items for preloading usedStorage = append(usedStorage, common.CopyBytes(key[:])) // Copy needed for closure - return nil - } - applyDeletion := func(key, value common.Hash) error { - return insertStorageChange(key, value, func() (encoded []byte, err error) { - if err := tr.DeleteStorage(s.address, key[:]); err != nil { - s.db.setError(err) - return nil, err - } - s.db.StorageDeleted += 1 - return nil, nil - }) } - applyUpdate := func(key, value common.Hash) error { - return insertStorageChange(key, value, func() (encoded []byte, err error) { - // Encoding []byte cannot fail, ok to ignore the error. - trimmed := common.TrimLeftZeroes(value[:]) - encoded, _ = rlp.EncodeToBytes(trimmed) - if err := tr.UpdateStorage(s.address, key[:], trimmed); err != nil { - s.db.setError(err) - return nil, err - } - s.db.StorageUpdated += 1 - return encoded, nil - }) - } - - // Perform trie updates before deletions. This prevents resolution of unnecessary trie nodes - // in circumstances similar to the following: - // - // Consider nodes `A` and `B` who share the same full node parent `P` and have no other siblings. - // During the execution of a block: - // - `A` is deleted, - // - `C` is created, and also shares the parent `P`. - // If the deletion is handled first, then `P` would be left with only one child, thus collapsed - // into a shortnode. This requires `B` to be resolved from disk. - // Whereas if the created node is handled first, then the collapse is avoided, and `B` is not resolved. - deletedStorages := make(map[common.Hash]common.Hash) - for key, value := range s.pendingStorage { - // Skip noop changes, persist actual changes - if value == s.originStorage[key] { - continue - } - if (value != common.Hash{}) { - if err := applyUpdate(key, value); err != nil { - return nil, err - } - } else { - deletedStorages[key] = value - } - } - for key, value := range deletedStorages { - if err := applyDeletion(key, value); err != nil { + for _, key := range deletions { + if err := tr.DeleteStorage(s.address, key[:]); err != nil { + s.db.setError(err) return nil, err } + s.db.StorageDeleted += 1 } if s.db.prefetcher != nil { s.db.prefetcher.used(s.addrHash, s.data.Root, usedStorage) From 9058793651b251fb4bd888950333253c5d0f6f99 Mon Sep 17 00:00:00 2001 From: Jared Wasinger Date: Tue, 12 Mar 2024 07:43:52 -0700 Subject: [PATCH 8/8] use address as parameter to deleteStateObject --- core/state/statedb.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/core/state/statedb.go b/core/state/statedb.go index 73e6bc4c6671..877a36d56a9a 100644 --- a/core/state/statedb.go +++ b/core/state/statedb.go @@ -523,12 +523,11 @@ func (s *StateDB) updateStateObject(obj *stateObject) { } // deleteStateObject removes the given object from the state trie. -func (s *StateDB) deleteStateObject(obj *stateObject) { +func (s *StateDB) deleteStateObject(addr common.Address) { // Track the amount of time wasted on deleting the account from the trie defer func(start time.Time) { s.AccountUpdates += time.Since(start) }(time.Now()) // Delete the account from the trie - addr := obj.Address() if err := s.trie.DeleteAccount(addr); err != nil { s.setError(fmt.Errorf("deleteStateObject (%x) error: %v", addr[:], err)) } @@ -905,18 +904,18 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { // If the self-destruct is handled first, then `P` would be left with only one child, thus collapsed // into a shortnode. This requires `B` to be resolved from disk. // Whereas if the created node is handled first, then the collapse is avoided, and `B` is not resolved. - var deletedObjects []*stateObject + var deletedAddrs []common.Address for addr := range s.stateObjectsPending { if obj := s.stateObjects[addr]; !obj.deleted { s.updateStateObject(obj) s.AccountUpdated += 1 } else { - deletedObjects = append(deletedObjects, obj) + deletedAddrs = append(deletedAddrs, obj.address) } usedAddrs = append(usedAddrs, common.CopyBytes(addr[:])) // Copy needed for closure } - for _, deletedObj := range deletedObjects { - s.deleteStateObject(deletedObj) + for _, deletedAddr := range deletedAddrs { + s.deleteStateObject(deletedAddr) s.AccountDeleted += 1 } if prefetcher != nil {