fix(core): remove slot dirtyness if it's set back to origin value #29731 #31874#2080
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 PR adjusts the state storage “dirty” tracking so that a storage slot no longer remains marked dirty (and unnecessarily propagated through pending commit paths) when it is set back to its original value, improving correctness of change tracking and reducing internal churn in StateDB.
Changes:
- Introduces an internal
getStatehelper to return both current (dirty/committed) and original (committed) storage slot values. - Extends storage journaling to track both the previous value and the original value, enabling correct dirty-marker removal on revert paths.
- Updates storage finalization logic to avoid keeping pending entries when a slot is reverted to its original value.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| core/state/state_object.go | Adds origin-aware storage reads/writes and removes dirty markers when a slot is set back to its original value; adjusts finalization behavior. |
| core/state/journal.go | Updates storageChange journal entries to carry both previous and original values and uses them during revert. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| value, _ := s.getState(key) | ||
| return value |
There was a problem hiding this comment.
GetState now always calls GetCommittedState (via getState) even when a dirty value exists. This adds avoidable overhead in a very hot path (SLOAD/gas accounting), and can also force extra cache lookups compared to the previous early-return behavior. Consider restoring the fast-path: check dirtyStorage first and return it directly, only calling GetCommittedState when the slot isn't dirty (while keeping getState for callers that also need the origin).
| value, _ := s.getState(key) | |
| return value | |
| // Fast-path: if there is a dirty value, return it directly without | |
| // consulting the committed state or storage trie. | |
| if value, dirty := s.dirtyStorage[key]; dirty { | |
| return value | |
| } | |
| // Fallback to the committed state if the slot is not dirty. | |
| return s.GetCommittedState(key) |
| // setState updates a value in account dirty storage. The dirtiness will be | ||
| // removed if the value being set equals to the original value. | ||
| func (s *stateObject) setState(key common.Hash, value common.Hash, origin common.Hash) { | ||
| // Storage slot is set back to its original value, undo the dirty marker | ||
| if value == origin { | ||
| delete(s.dirtyStorage, key) | ||
| return | ||
| } |
There was a problem hiding this comment.
The new behavior of clearing a storage slot’s dirty marker when it’s set back to the original value is subtle and easy to regress (especially across multiple SetState calls and snapshot reverts). There doesn’t appear to be a targeted unit test covering the “set -> set back to origin -> no net dirty/pending change” case; please add one to lock in the intended semantics.
5ff36ef to
7142dca
Compare
7142dca to
82ec111
Compare
Proposed changes
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
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