refactor(core): storage journal entry should revert dirtyness too #29641#2078
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 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 state journaling so reverting a storage journal entry can also clear the “dirty” marker for a slot when the reverted write was the first modification in the current execution context, preventing downstream components (e.g., witness/proof generation) from treating the slot as unnecessarily changed.
Changes:
- Refactors storage slot reads to distinguish “dirty in current context” vs “committed value” (
GetState+ new internalgetState). - Changes
storageChangejournal entries to optionally track a previous dirty value (or none), enabling revert to remove dirtiness. - Updates
setStateto support deleting a dirty slot entry on revert.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| core/state/state_object.go | Introduces dirty-aware storage reads and enables setState to delete dirty markers on revert. |
| core/state/journal.go | Updates storageChange to store an optional previous value and uses it during revert. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var prevvalue *common.Hash | ||
| if dirty { | ||
| prevvalue = &prev | ||
| } |
There was a problem hiding this comment.
Using *common.Hash for storageChange.prevvalue causes prevvalue = &prev to escape and allocate on the heap for every subsequent write to an already-dirty slot. Since this is a hot path, consider storing the previous value by value plus a separate boolean flag indicating whether it existed (dirty), avoiding per-write allocations.
| // setState updates a value in account dirty storage. If the value being set is | ||
| // nil (assuming journal revert), the dirtyness is removed. | ||
| func (s *stateObject) setState(key common.Hash, value *common.Hash) { | ||
| // If the first set is being reverted, undo the dirty marker | ||
| if value == nil { | ||
| delete(s.dirtyStorage, key) | ||
| return |
There was a problem hiding this comment.
This change introduces new semantics where reverting the first SetState should remove the dirty marker (delete from dirtyStorage). There isn't a regression test asserting that behavior (e.g., snapshot + one SetState + revert should leave dirtyStorage without the slot). Please add a unit test to lock in this behavior and prevent witness/proof regressions.
gzliudan
force-pushed
the
state-journal-storage-revert
branch
from
bf084b6 to
02dae33
Compare
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
gzliudan
force-pushed
the
state-journal-storage-revert
branch
from
02dae33 to
2c40c1a
Compare
…ereum#29641 Currently our state journal tracks each storage update to a contract, having the ability to revert those changes to the previously set value. For the very first modification however, it behaves a bit wonky. Reverting the update doesn't actually remove the dirty-ness of the slot, rather leaves it as "change this slot to it's original value". This can cause issues down the line with for example write witnesses needing to gather an unneeded proof. This PR modifies the storageChange journal entry to not only track the previous value of a slot, but also whether there was any previous value at all set in the current execution context. In essence, the PR changes the semantic of storageChange so it does not simply track storage changes, rather it tracks dirty storage changes, an important distinction for being able to cleanly revert the journal item.
gzliudan
force-pushed
the
state-journal-storage-revert
branch
from
2c40c1a to
1199d66
Compare
Proposed changes
Currently our state journal tracks each storage update to a contract, having the ability to revert those changes to the previously set value.
For the very first modification however, it behaves a bit wonky. Reverting the update doesn't actually remove the dirty-ness of the slot, rather leaves it as "change this slot to it's original value". This can cause issues down the line with for example write witnesses needing to gather an unneeded proof.
This PR modifies the storageChange journal entry to not only track the previous value of a slot, but also whether there was any previous value at all set in the current execution context. In essence, the PR changes the semantic of storageChange so it does not simply track storage changes, rather it tracks dirty storage changes, an important distinction for being able to cleanly revert the journal item.
Ref: ethereum#29641
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