execution/vm: include SELFDESTRUCT beneficiary in BAL regardless of value transfer#21333
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Block Access List (BAL) generation for SELFDESTRUCT by ensuring the beneficiary is recorded as a state access unconditionally, even when no value is transferred (notably fixing the SYSTEM_ADDRESS + zero-balance edge case described in the PR). This aligns Erigon’s BAL behavior with the EIP-7928 requirement that SELFDESTRUCT implies a beneficiary state access independent of value transfer, preventing BAL hash mismatches and sync failures.
Changes:
- Record
SELFDESTRUCTbeneficiary access unconditionally inmakeSelfdestructGasFn(removing the previous zero-balance gating behavior). - Remove now-dead helper methods from
IntraBlockStatethat only supported the prior zero-balance branch. - Add regression tests covering
SYSTEM_ADDRESSand ordinary beneficiaries for both zero- and non-zero-balanceSELFDESTRUCTscenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
execution/vm/operations_acl.go |
Marks SELFDESTRUCT beneficiary as a non-revertable address access unconditionally for BAL correctness. |
execution/state/intra_block_state.go |
Deletes unused read-snapshot/internal-marking helpers made obsolete by the new unconditional access marking. |
execution/execmodule/bal_selfdestruct_systemaddress_test.go |
Adds regression tests ensuring BAL includes SYSTEM_ADDRESS (and ordinary beneficiaries) for SELFDESTRUCT, including the zero-balance case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yperbasis
left a comment
There was a problem hiding this comment.
LGTM — clear EIP-7928 alignment, the SnapshotVersionedReadKeys/MarkNewReadsInternal removal is correctly dead code (only the removed branch used them; MarkReadsInternal is still load-bearing for the readOnly CALL-with-value path in instructions.go), and the TDD + devnet evidence is exactly the right shape.
Two non-blocking nits for optional follow-up:
-
TestBALIncludesSystemAddressOnSelfdestructToItWithNonZeroBalanceonly assertsNotNil(sysEntry). The body says "the BAL records a balance change on the SystemAddress beneficiary," but the test wouldn't actually catch a future regression that broke that — consider tightening withrequire.NotEmpty(t, sysEntry.BalanceChanges)so the assertion matches the comment's stated guarantee. -
The nested
append([]byte{0x73}, append(addr.Bytes(), 0xff)...)init-code construction is repeated across all three tests; a tiny helper likepushAddrSelfdestruct(addr)would deduplicate. Pure style — feel free to ignore.
addressed in 9cd53ea |
…st (erigontech#21599) minor tidy follow up to erigontech#21333 to move the tests to be EngineApiTester based should've done it back then but was in a rush to fix the issue
added a test for this to EEST ethereum/execution-specs#2893
Summary
makeSelfdestructGasFninexecution/vm/operations_acl.gopreviously gatedMarkAddressAccess(beneficiary, false)on the destructing contract having a non-zero balance, and on the zero-balance path additionally marked the gas-calcEmpty()reads internal so the beneficiary would not appear in the BAL purely from that read.Per EIP-7928, "Beneficiary addresses for
SELFDESTRUCT" are in scope for inclusion unconditionally — the SELFDESTRUCT is itself a state access on the beneficiary, independent of any value transfer. The pre-fix gate caused a BAL hash mismatch in the specific case where a SELFDESTRUCT targetsSYSTEM_ADDRESS(0xff…fffe) with the destructing contract at zero balance:versionedio.go::AsBlockAccessList's "drop SystemAddress unless it experienced state access itself" filter then dropped the entry because no non-revertable access mark was ever recorded, while other clients (which mark the beneficiary unconditionally) kept it. Result: BAL hash mismatch, block rejected, sync stall.Fix
execution/vm/operations_acl.go::makeSelfdestructGasFn: record the beneficiary access unconditionally:The pre-existing
SnapshotVersionedReadKeys/MarkNewReadsInternalhelpers inexecution/state/intra_block_state.gowere only used by the now-removed zero-balance branch and are dropped as dead code (−35 lines).execution/state/versionedio.go::AsBlockAccessList's SystemAddress filter is left unchanged — its&& !account.nonRevertableUserAccessclause is now correctly satisfied by the unconditional mark above.Test coverage
New file
execution/execmodule/bal_selfdestruct_systemaddress_test.goadds three regression tests viablockgen.GenerateChainagainstchain.AllProtocolChanges(Amsterdam active at genesis):TestBALIncludesSystemAddressOnSelfdestructToItWithZeroBalance— the bug scenario: CREATE tx with init codePUSH20 SYSTEM_ADDRESS; SELFDESTRUCTand zero contract balance. Asserts the resulting BAL contains a SystemAddress entry with every change-set empty.TestBALIncludesSystemAddressOnSelfdestructToItWithNonZeroBalance— guard for the non-zero balance variant.TestBALIncludesOrdinaryBeneficiaryOnSelfdestructWithZeroBalance— guard that ordinary EOA beneficiaries (where the SystemAddress filter doesn't apply) are unaffected.Test plan
go test -count=1 -run='TestBALIncludes' ./execution/execmodule/— 3/3 pass.if !balance.IsZero()guard restored,TestBALIncludesSystemAddressOnSelfdestructToItWithZeroBalancefails with "BAL must include a SystemAddress entry"; the other two still pass (confirming the bug is specific to SystemAddress + zero balance).make lint— 0 issues.make eest-spec-statetests-devnet— 65,202 / 65,202 tests pass.make eest-spec-blocktests-devnet— 82,941 / 82,941 tests pass (parallel exec).test_bal_selfdestruct_to_system_address_zero_balanceopened as a PR againstethereum/execution-specs devnets/bal/7(link to follow).