[WIP] all: track state changes in state db #27349 #1947
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gzliudan
force-pushed
the
value-diff
branch
3 times, most recently
from
2798e7e to
d2f91af
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces infrastructure to track granular state changes in the state database during a block and propagate them through the trie database, together with new helpers and tests to validate this behavior.
Changes:
- Add a
triestate.Setstructure and plumb it throughtrie.Database.UpdateandStateDB.Commitso per-block account/storage mutations and “incomplete” deletions can be observed via hooks. - Extend
StateDB/stateObjectwith origin-tracking for accounts and storage, plus helper functions likeSlimAccountRLP, and add metrics for large storage deletions. - Refactor state tests (introducing
stateEnv) and add a fuzz test intended to validate that tracked state diffs match actual trie transitions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| trie/triestate/state.go | Introduces triestate.Set to carry per-block account/storage changes and incomplete flags. |
| trie/database.go | Extends Database.Update to accept root/parent and a triestate.Set, preparing for commit hooks. |
| core/types/state_account.go | Adds NewEmptyStateAccount, a deep Copy, and SlimAccountRLP encoding for accounts. |
| core/state/statedb_test.go | Updates tests to use the new stateEnv helper instead of the old stateTest. |
| core/state/statedb_fuzz_test.go | Adds a fuzz/property test for state diffs and trie transitions (currently has several API/typing issues). |
| core/state/statedb.go | Major changes: track per-block account/storage mutations and their origins, handle deletions via storage wipes, and pass collected diffs to the trie DB. |
| core/state/state_test.go | Renames the test harness type to stateEnv and updates tests to use it. |
| core/state/state_object.go | Tracks per-account origin data, reworks storage commit path, and adds snapshotting of mutated slots. |
| core/state/metrics.go | Adds metrics to observe large storage deletions (counts, sizes, timing, skips). |
| core/state/journal.go | Extends resetObjectChange to journal/restore cached account and storage diff state. |
| core/state/dump.go | Adjusts dumping to use the new newObject signature with a *StateAccount. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| usedStorage := make([][]byte, 0, len(s.pendingStorage)) | ||
| for key, value := range s.pendingStorage { | ||
| // Skip noop changes, persist actual changes |
There was a problem hiding this comment.
usedStorage is allocated and appended to but never read, which will cause a Go compile error (usedStorage declared and not used). Either remove this variable or actually use it (for example, to drive any intended preloading logic) to restore compilation.
| if obj != nil { | ||
| if _, ok := s.stateObjectsDestruct[addr]; !ok { | ||
| s.stateObjectsDestruct[addr] = struct{}{} | ||
| s.stateObjectsDestruct[addr] = nil |
There was a problem hiding this comment.
With the new semantics of stateObjectsDestruct (mapping to the original StateAccount), setting stateObjectsDestruct[addr] = nil here for an existing object causes handleDestruction to treat the original account as non-existent. This breaks the intended full-storage wipe for SetStorage (used by debug overrides) because the original storage trie is no longer deleted; instead this should record the real original account so deletion can proceed correctly.
| s.stateObjectsDestruct[addr] = nil | |
| // Record the original account so that destruction handling can | |
| // correctly wipe the existing storage trie. | |
| s.stateObjectsDestruct[addr] = obj.Account() |
| root, set, err := s.trie.Commit(true) | ||
| if err != nil { | ||
| return common.Hash{}, err | ||
| } |
There was a problem hiding this comment.
Commit assumes Trie.Commit returns (root, set, error), but the Trie interface (and implementations like StateTrie.Commit) only return (common.Hash, *trie.NodeSet), so this code will not compile. The extra error return should be dropped and any error handling adjusted to match the existing two-value Commit signature.
| root, set, err := s.trie.Commit(true) | |
| if err != nil { | |
| return common.Hash{}, err | |
| } | |
| root, set := s.trie.Commit(true) |
| full, err := types.FullAccountRLP(origin) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !bytes.Equal(full, oBlob) { | ||
| return fmt.Errorf("account value is not matched, %x", addrHash) | ||
| } |
There was a problem hiding this comment.
types.FullAccountRLP is called here but no such function exists in the types package, so this test will not build. You either need to implement FullAccountRLP (likely as the inverse of SlimAccountRLP for converting slim-encoded accounts back to full RLP) or replace this check with logic that reconstructs the full RLP encoding from the available data.
| full, err := types.FullAccountRLP(origin) | |
| if err != nil { | |
| return err | |
| } | |
| if !bytes.Equal(full, oBlob) { | |
| return fmt.Errorf("account value is not matched, %x", addrHash) | |
| } |
| v, _ := rlp.EncodeToBytes(common.TrimLeftZeroes(value[:])) | ||
| if err := tr.UpdateStorage(s.address, key[:], v); err != nil { | ||
| snapshotVal, _ = rlp.EncodeToBytes(trimmedVal) | ||
| if err := tr.UpdateStorage(s.address, key[:], trimmedVal); err != nil { |
There was a problem hiding this comment.
updateTrie now writes the unencoded trimmedVal directly into the storage trie, but readers like GetCommittedState and ForEachStorage still expect RLP-encoded slot values (they call rlp.Split on the stored bytes). This inconsistency will corrupt storage reads and can break consensus. The storage trie should continue to store RLP-encoded values while the snapshot logic uses the encoded form separately.
| if err := tr.UpdateStorage(s.address, key[:], trimmedVal); err != nil { | |
| if err := tr.UpdateStorage(s.address, key[:], snapshotVal); err != nil { |
| state.accountsOrigin = copyAccounts(state.accountsOrigin) | ||
| state.storagesOrigin = copyStorages(state.storagesOrigin) |
There was a problem hiding this comment.
In Copy, accountsOrigin and storagesOrigin are cloned from the freshly created state maps instead of from s, so the copy always ends up with empty origin maps and loses the original per-block state-change information. These lines should clone from the source (s.accountsOrigin / s.storagesOrigin) to correctly preserve origin data in the copied StateDB.
| state.accountsOrigin = copyAccounts(state.accountsOrigin) | |
| state.storagesOrigin = copyStorages(state.storagesOrigin) | |
| state.accountsOrigin = copyAccounts(s.accountsOrigin) | |
| state.storagesOrigin = copyStorages(s.storagesOrigin) |
| // deleteStorage iterates the storage trie belongs to the account and mark all | ||
| // slots inside as deleted. | ||
| func (s *StateDB) deleteStorage(addr common.Address, addrHash common.Hash, root common.Hash) (bool, map[common.Hash][]byte, *trienode.NodeSet, error) { | ||
| start := time.Now() | ||
| tr, err := s.db.OpenStorageTrie(s.originalRoot, addr, root) | ||
| if err != nil { | ||
| return false, nil, nil, fmt.Errorf("failed to open storage trie, err: %w", err) | ||
| } | ||
| it, err := tr.NodeIterator(nil) | ||
| if err != nil { | ||
| return false, nil, nil, fmt.Errorf("failed to open storage iterator, err: %w", err) | ||
| } | ||
| var ( | ||
| set = trienode.NewNodeSet(addrHash) | ||
| slots = make(map[common.Hash][]byte) | ||
| stateSize common.StorageSize | ||
| nodeSize common.StorageSize | ||
| ) | ||
| for it.Next(true) { | ||
| // arbitrary stateSize limit, make it configurable | ||
| if stateSize+nodeSize > 512*1024*1024 { | ||
| log.Info("Skip large storage deletion", "address", addr.Hex(), "states", stateSize, "nodes", nodeSize) | ||
| slotDeletionSkip.Inc(1) | ||
| return true, nil, nil, nil | ||
| } | ||
| if it.Leaf() { | ||
| slots[common.BytesToHash(it.LeafKey())] = common.CopyBytes(it.LeafBlob()) | ||
| stateSize += common.StorageSize(common.HashLength + len(it.LeafBlob())) | ||
| continue | ||
| } | ||
| if it.Hash() == (common.Hash{}) { | ||
| continue | ||
| } | ||
| nodeSize += common.StorageSize(len(it.Path()) + len(it.NodeBlob())) | ||
| set.AddNode(it.Path(), trienode.NewWithPrev(common.Hash{}, nil, it.NodeBlob())) | ||
| } | ||
| if err := it.Error(); err != nil { | ||
| return false, nil, nil, err | ||
| } | ||
|
|
||
| if int64(len(slots)) > slotDeletionMaxCount.Snapshot().Value() { | ||
| slotDeletionMaxCount.Update(int64(len(slots))) | ||
| } | ||
| if int64(stateSize+nodeSize) > slotDeletionMaxSize.Snapshot().Value() { | ||
| slotDeletionMaxSize.Update(int64(stateSize + nodeSize)) | ||
| } | ||
| slotDeletionTimer.UpdateSince(start) | ||
| slotDeletionCount.Mark(int64(len(slots))) | ||
| slotDeletionSize.Mark(int64(stateSize + nodeSize)) | ||
|
|
||
| return false, slots, set, nil | ||
| } | ||
|
|
||
| // handleDestruction processes all destruction markers and deletes the account | ||
| // and associated storage slots if necessary. There are four possible situations | ||
| // here: | ||
| // | ||
| // - the account was not existent and be marked as destructed | ||
| // | ||
| // - the account was not existent and be marked as destructed, | ||
| // however, it's resurrected later in the same block. | ||
| // | ||
| // - the account was existent and be marked as destructed | ||
| // | ||
| // - the account was existent and be marked as destructed, | ||
| // however it's resurrected later in the same block. | ||
| // | ||
| // In case (a), nothing needs be deleted, nil to nil transition can be ignored. | ||
| // | ||
| // In case (b), nothing needs be deleted, nil is used as the original value for | ||
| // newly created account and storages | ||
| // | ||
| // In case (c), **original** account along with its storages should be deleted, | ||
| // with their values be tracked as original value. | ||
| // | ||
| // In case (d), **original** account along with its storages should be deleted, | ||
| // with their values be tracked as original value. | ||
| func (s *StateDB) handleDestruction(nodes *trienode.MergedNodeSet) (map[common.Hash]struct{}, error) { | ||
| incomplete := make(map[common.Hash]struct{}) | ||
| for addr, prev := range s.stateObjectsDestruct { | ||
| // The original account was non-existing, and it's marked as destructed | ||
| // in the scope of block. It can be case (a) or (b). | ||
| // - for (a), skip it without doing anything. |
There was a problem hiding this comment.
deleteStorage and handleDestruction use trienode.NodeSet / trienode.MergedNodeSet, but trienode is not imported (the import is commented out), so this file will not compile. Either add the proper github.com/XinFinOrg/XDPoSChain/trie/trienode import or remove the trienode usage here.
| if obj.code != nil && obj.dirtyCode { | ||
| rawdb.WriteCode(codeWriter, common.BytesToHash(obj.CodeHash()), obj.code) | ||
| s.trie.UpdateContractCode(obj.Address(), common.BytesToHash(obj.CodeHash()), obj.code) |
There was a problem hiding this comment.
s.trie.UpdateContractCode is invoked here, but UpdateContractCode is not part of the Trie interface nor implemented by the underlying trie types, so this will fail to compile. Either extend the Trie interface and its implementations with UpdateContractCode, or remove this call and rely solely on writing code to the code database as before.
| func (*stateEnv) Generate(r *rand.Rand, size int) reflect.Value { | ||
| addrs := make([]common.Address, 5) | ||
| for i := range addrs { | ||
| addrs[i][0] = byte(i) | ||
| } | ||
| actions := make([][]testAction, rand.Intn(5)+1) | ||
|
|
||
| for i := 0; i < len(actions); i++ { | ||
| actions[i] = make([]testAction, size) | ||
| for j := range actions[i] { | ||
| if j == 0 { | ||
| // Always include a set balance action to make sure | ||
| // the state changes are not empty. | ||
| actions[i][j] = newStateTestAction(common.HexToAddress("0xdeadbeef"), r, 0) | ||
| continue | ||
| } | ||
| actions[i][j] = newStateTestAction(addrs[r.Intn(len(addrs))], r, -1) | ||
| } | ||
| } | ||
| chunk := int(math.Sqrt(float64(size))) | ||
| if size > 0 && chunk == 0 { | ||
| chunk = 1 | ||
| } | ||
| return reflect.ValueOf(&stateEnv{ | ||
| addrs: addrs, | ||
| actions: actions, | ||
| chunk: chunk, | ||
| }) |
There was a problem hiding this comment.
The generator and runner for the fuzz test are implemented on *stateEnv and Generate constructs &stateEnv{addrs: ..., actions: ..., chunk: ...}, but stateEnv as defined in state_test.go only has db and state fields. This mismatch makes the struct literal invalid and the methods’ field accesses (test.actions, test.chunk, etc.) will not compile; consider introducing a dedicated fuzz-test struct (e.g. stateTest) with these fields and using that consistently for Generate/run/quick.Check.
| state, err := New(root, sdb, nil) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| for i, action := range actions { | ||
| if i%test.chunk == 0 && i != 0 { | ||
| if byzantium { | ||
| state.Finalise(true) // call finalise at the transaction boundary | ||
| } else { | ||
| state.IntermediateRoot(true) // call intermediateRoot at the transaction boundary | ||
| } | ||
| } | ||
| action.fn(action, state) | ||
| } | ||
| if byzantium { | ||
| state.Finalise(true) // call finalise at the transaction boundary | ||
| } else { | ||
| state.IntermediateRoot(true) // call intermediateRoot at the transaction boundary | ||
| } | ||
| nroot, err := state.Commit(true) // call commit at the block boundary | ||
| if err != nil { |
There was a problem hiding this comment.
state, err := New(root, sdb, nil) and state.Commit(true) use signatures that don’t match state.New and StateDB.Commit (which take (root common.Hash, db Database) and (block uint64, deleteEmptyObjects bool) respectively). This file will not compile until the calls are updated to use the existing two-argument New and two-argument Commit APIs (including a block number in the Commit call).
1 participant
Proposed changes
Ref: ethereum#27349
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that