core/state: remove account reset operation v2 #29520 #1934
Conversation
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
Copilot,
liam-lai and
wanwiset25
|
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 |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the core state management system by removing the account reset operation and introducing a new mutation tracking system. The changes aim to simplify account lifecycle management and improve state handling during contract creation.
Key changes:
- Introduces a mutation tracking system to replace the previous approach of tracking dirty/pending/destruct states separately
- Refactors contract creation logic to conditionally create accounts only when they don't exist
- Adds a
CreateContractmethod to mark accounts as new contracts for EIP-6780 compliance - Replaces the
created/deletedflags with anewContractflag and removes theresetObjectChangejournal entry
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| core/vm/interface.go | Adds CreateContract method to StateDB interface for marking contract accounts |
| core/vm/evm.go | Updates contract creation to check for existing accounts before calling CreateAccount and adds CreateContract call |
| core/state/statedb.go | Major refactoring introducing mutation tracking system, replacing dirty/pending maps with mutations map, changing stateObjectsDestruct to store original account data |
| core/state/statedb_test.go | Adds new tests for copy functionality with dirty journal and object state, updates existing tests to check account existence before creation |
| core/state/state_test.go | Simplifies TestSnapshot2 to TestCreateObjectRevert focusing on account creation/revert behavior |
| core/state/state_object.go | Renames created flag to newContract, removes deleted flag, adds origin field for tracking original state, changes code from Code type to []byte |
| core/state/journal.go | Removes resetObjectChange, adds copy methods to all journal entry types for proper deep copying |
Comments suppressed due to low confidence (1)
core/state/statedb.go:199
- The Reset function clears out ephemeral state but doesn't reset the 'mutations' and 'stateObjectsDestruct' maps that were introduced in this PR. These maps track transaction-level changes and should be reset when the state is reset to maintain consistency with the rest of the state clearing logic.
func (s *StateDB) Reset(root common.Hash) error {
tr, err := s.db.OpenTrie(root)
if err != nil {
return err
}
s.trie = tr
s.stateObjects = make(map[common.Address]*stateObject)
s.thash = common.Hash{}
s.txIndex = 0
s.logs = make(map[common.Hash][]*types.Log)
s.logSize = 0
s.preimages = make(map[common.Hash][]byte)
s.clearJournalAndRefund()
s.accessList = newAccessList()
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s *StateDB) getStateObject(addr common.Address) *stateObject { | ||
| if obj := s.getDeletedStateObject(addr); obj != nil && !obj.deleted { | ||
| return obj | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // getDeletedStateObject is similar to getStateObject, but instead of returning | ||
| // nil for a deleted state object, it returns the actual object with the deleted | ||
| // flag set. This is needed by the state journal to revert to the correct s- | ||
| // destructed object instead of wiping all knowledge about the state object. | ||
| func (s *StateDB) getDeletedStateObject(addr common.Address) *stateObject { | ||
| // Prefer live objects if any is available | ||
| if obj := s.stateObjects[addr]; obj != nil { | ||
| return obj | ||
| } | ||
| // Short circuit if the account is already destructed in this block. | ||
| if _, ok := s.stateObjectsDestruct[addr]; ok { | ||
| return nil | ||
| } | ||
| // Load the object from the database | ||
| start := time.Now() | ||
| data, err := s.trie.GetAccount(addr) |
There was a problem hiding this comment.
The getStateObject method now returns nil for already destructed accounts (lines 602-604), but it doesn't initialize the 'origin' field for accounts loaded from the database. The origin field is used at line 796 to track original account data for destructed accounts. When an account is loaded from the database, the origin field should be initialized with the original account data to ensure proper tracking of the account's original state before modifications.
| for addr, op := range s.mutations { | ||
| if op.applied { | ||
| continue | ||
| } | ||
| op.applied = true | ||
|
|
||
| if op.isDelete() { | ||
| deletedAddrs = append(deletedAddrs, addr) | ||
| } else { | ||
| obj.updateRoot(s.db) | ||
| s.updateStateObject(obj) | ||
| s.updateStateObject(s.stateObjects[addr]) |
There was a problem hiding this comment.
Potential nil pointer dereference at line 851. Similar to line 826, this code accesses s.stateObjects[addr] without verifying the object exists. If a mutation is tracked for an address but the object has been removed from s.stateObjects, this would cause a panic. A nil check should be added before calling updateStateObject.
gzliudan
force-pushed
the
no-reset-object-alt
branch
from
b5a08b2 to
54a51d2
Compare
1 participant
Proposed changes
Ref: ethereum#29520
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