EIP-8246 (Remove SELFDESTRUCT Burn)#11949
Conversation
Removes the residual ETH-burn cases left by EIP-6780: - A self-targeting SELFDESTRUCT now moves no ETH and emits no log, regardless of whether the account was created in the same transaction. - At transaction finalization, accounts marked for destruction keep their balance: storage and code are cleared and the nonce is reset to 0. A resulting zero-balance account is still removed as empty per EIP-161, matching the spec and keeping CREATE2 redeployment unblocked. Wires a new IsEip8246Enabled flag through IReleaseSpec / ReleaseSpec / decorators / chainspec (Eip8246TransitionTimestamp) and enables it in the Amsterdam fork. Adds Eip8246Tests covering same-transaction self-destruct to self/other, post-destruct funding, the zero-balance empty-account case, and the unchanged not-in-same-tx no-op, each pinned with EIP on and off. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @Marchhill's task in 5m 8s —— View job EIP-8246 PR Review
SummaryThe implementation is logically correct and the spec wiring is complete. The SELFDESTRUCT opcode change, the two finalization paths, and all the plumbing (IReleaseSpec, decorators, ChainSpec, Amsterdam fork) are internally consistent. However, the test suite has a significant gap and there are two issues in the deferred-finalization path that warrant attention before merge. FindingsHighTests only exercise the non-deferred finalization path — Medium
Low
What looks good
|
| { | ||
| private readonly ISpecProvider _specProvider = | ||
| new TestSpecProvider(new OverridableReleaseSpec(Cancun.Instance) { IsEip8246Enabled = eip8246Enabled }); | ||
|
|
There was a problem hiding this comment.
High: Tests only exercise the non-deferred finalization path
The spec here is Cancun without IsEip8037Enabled or IsEip7708Enabled, so deferFinalization = false in ExecuteEvmTransaction. In Amsterdam (the fork where EIP-8246 is actually enabled), both EIP-8037 and EIP-7708 are also active, which routes destruction through the separate FinalizeDestroyedAccount static helper instead.
That deferred path is not covered by any test in this PR — the EIP-8246 balance-preservation logic there (DeleteAccount followed by CreateAccount(toBeDestroyed, balance)) runs without test coverage.
Consider adding a fixture variant that enables EIP-8037 + EIP-7708 alongside EIP-8246 to drive the deferred path, e.g.:
private readonly ISpecProvider _specProvider =
new TestSpecProvider(new OverridableReleaseSpec(Cancun.Instance)
{
IsEip8246Enabled = eip8246Enabled,
IsEip8037Enabled = eip8246Enabled,
IsEip7708Enabled = eip8246Enabled,
});Or a separate [TestFixture] that targets the Amsterdam spec directly.
| UInt256 balance = worldState.GetBalance(toBeDestroyed); | ||
| if (!balance.IsZero) | ||
| // EIP-7708 logs the burn; suppressed once EIP-8246 stops burning. | ||
| if (!balance.IsZero && !removeSelfdestructBurn) |
There was a problem hiding this comment.
Medium: Log type diverges from the non-deferred path
The non-deferred path (lines ~1278) emits TransferLog.CreateSelfDestruct(...) for the burn event, while this deferred path emits TransferLog.CreateBurn(...). If these are distinct EIP-7708 event types, trace consumers that handle both paths will see different topic/data layouts for the same semantic event.
If they are intentionally different, a brief comment explaining why would help future readers. If they should match, one of the two sites needs updating.
| if (removeSelfdestructBurn && !balance.IsZero) | ||
| { | ||
| worldState.CreateAccount(toBeDestroyed, balance); | ||
| } |
There was a problem hiding this comment.
Medium: Destroy refund not reported to tracer in deferred path
The non-deferred path calls tracer.ReportRefund(spec.GasCosts.DestroyRefund) for each destroyed account. FinalizeDestroyedAccount has no equivalent call, so tracers operating in Amsterdam (EIP-8037 + EIP-7708 active) never see the destroy refund events. This may be pre-existing, but with EIP-8246 now routing through this path in production it deserves explicit resolution — either adding the ReportRefund call here, or documenting that refund tracing is intentionally skipped in the deferred path.
|
|
||
| Assert.That(TestState.GetBalance(inheritor), Is.EqualTo((UInt256)5.Ether)); | ||
| Assert.That(TestState.AccountExists(child), Is.False); | ||
| } |
There was a problem hiding this comment.
Medium: Missing CREATE2 redeployment test
The PR description explicitly says "keeping CREATE2 redeployment unblocked" as a key property of the EIP-8246 zero-balance case. There is no test that verifies a CREATE2 call to the same address succeeds after a zero-balance self-destruct.
Suggested additional test:
- CREATE2-deploy a contract with zero balance that self-destructs to self (same tx).
- Assert the address is gone.
- CREATE2-deploy the same init code to the same salt again in a subsequent transaction.
- Assert the new deployment succeeds and the address exists.
- Add an EIP-8037 + EIP-7708 fixture dimension so every EIP-8246 scenario also runs through the deferred FinalizeDestroyedAccount path (as in Amsterdam), covering the balance-preservation logic there. - Add a CREATE2 redeployment test: a factory re-creates a self-destructed child at the same address across transactions, verifying the nonce reset keeps redeployment unblocked and the preserved balance accumulates. - Report the destroy refund to the tracer in the deferred path, matching the inline path. - Document the pre-existing Burn-vs-SelfDestruct log distinction between the post-fee and pre-fee finalization paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@claude review |
|
Claude finished @Marchhill's task in 4m 40s —— View job EIP-8246 PR Review
SummaryAll findings from the prior review have been resolved. The implementation is correct and the test suite is comprehensive. This PR is ready to merge. Previous findings — all resolved
What looks goodOpcode logic ( The
Finalization balance preservation Both finalization paths produce the same invariant: Destroy-list gate Accounts that hit the EIP-6780 cross-tx no-op are never added to Spec wiring
Test coverage Five scenarios × four fixtures (on/off × deferred/inline) = 20 test runs. The Remaining low notePR description template not cleaned up — the body still contains Verdict: ready to merge (pending description cleanup). |
EIP-8246 is a Draft and is not part of the EEST `for_amsterdam` conformance fixtures. Enabling it in the Amsterdam fork changed SELFDESTRUCT behaviour for that fork, diverging from the reference post-states and failing the Pyspec tests across all shards. Keep the full implementation and spec/chainspec plumbing so the EIP can be activated via the Eip8246Transition chainspec parameter once it is scheduled, but leave it disabled in the named forks. Unit tests toggle the flag directly via OverridableReleaseSpec, so coverage is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eip-8246 # Conflicts: # src/Nethermind/Nethermind.Specs/Forks/25_Amsterdam.cs
EVM Opcode Benchmark DiffAggregated runs: base=3, pr=3 No significant regressions or improvements detected. |
Removes the residual ETH-burn cases left by EIP-6780:
Wires a new IsEip8246Enabled flag through IReleaseSpec / ReleaseSpec / decorators / chainspec (Eip8246TransitionTimestamp) and enables it in the Amsterdam fork. Adds Eip8246Tests covering same-transaction self-destruct to self/other, post-destruct funding, the zero-balance empty-account case, and the unchanged not-in-same-tx no-op, each pinned with EIP on and off.
Fixes Closes Resolves #
Please choose one of the keywords above to refer to the issue this PR solves followed by the issue number (e.g. Fixes #000). If no issue number, remove the line. Also, remove everything marked optional that is not applicable. Remove this note after reading.
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs. Remove if not applicable.Requires explanation in Release Notes
If yes, fill in the details here. Remove if not applicable.
Remarks
Optional. Remove if not applicable.