Conversation
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes consensus-critical state-read validation, and valid EVM transaction reachability remains unverified.
Pull request overview
Fixes EIP-8246 balance reads after balance-preserving SELFDESTRUCT.
Changes:
- Excludes preserved balances from destruct wipe scans and validation.
- Adds regression coverage for retained balance cells.
- Updates self-destruct test fixtures.
File summaries
| File | Description |
|---|---|
execution/state/versionmap.go |
Aligns balance validation with preservation semantics. |
execution/state/versionmap_test.go |
Updates self-destruct validation fixtures. |
execution/state/revival_consistency_test.go |
Changes the synthetic revival fixture. |
execution/state/read_paths.go |
Preserves earlier balance cells during reads. |
execution/state/eip8246_selfdestruct_test.go |
Adds end-to-end state API regression coverage. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+121
to
124
| rs.SetNonce(addr, VersionedRead[uint64]{ | ||
| ReadHeader: ReadHeader{Source: MapRead, Version: Version{TxIndex: 0}}, | ||
| Val: *uint256.NewInt(1_000), | ||
| Val: 1, | ||
| }) |
| vm := NewVersionMap(nil) | ||
| writeFor(vm, addr, BalancePath, accounts.NilKey, Version{TxIndex: 0, Incarnation: 0}, *uint256.NewInt(1_000), true) | ||
| writeFor(vm, addr, SelfDestructPath, accounts.NilKey, Version{TxIndex: 2, Incarnation: 0}, true, true) | ||
| vm.WriteBalance(addr, Version{TxIndex: 2}, uint256.Int{}, true) |
Collaborator
There was a problem hiding this comment.
L589: test: the new balance write at tx 2 makes this fail on the plain version check, so it no longer reaches the SD-staleness branch it is named after. A nonce read, as in the next test, reaches it again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #23238.
When a balance-preserving SELFDESTRUCT publishes no
BalancePathcell, a later transaction can read zero from an older balance cell whileapplySubFieldWritespreserves its value. The validator also rejects the preserved nonzero read after the reader is corrected.Exclude balance cells from both destruct wipe scans. A burning SELFDESTRUCT writes zero explicitly; a preserving one leaves the prior balance intact. Balance version and value checks still detect changed writes.
Add a regression that flushes a real preserving self-destruct write set, verifies the missing balance cell, and checks the later balance against reconstruction, its recorded dependencies, and read-set validation. Update the existing wipe fixtures to use an explicit balance burn or a nonce that SELFDESTRUCT actually clears.
Validation:
go test ./execution/state -count=1go test -race ./execution/state -count=1make lintmake erigon integrationThe regression exercises the state APIs directly. Reachability through a valid EVM transaction remains unverified.