fix(state): honour SELFDESTRUCT marker in PerContractState.StorageRoot#11424
fix(state): honour SELFDESTRUCT marker in PerContractState.StorageRoot#11424stdevMac wants to merge 1 commit into
Conversation
## Bug
Same-block SELFDESTRUCT followed by CREATE-at-the-same-address (the 0age
metamorphic-contract pattern) deterministically reverts on Sepolia
block 4913057. Prewarming-disabled runs reproduce the failure
identically, so the bug is in core state — not the cache layer.
## Root cause
The EVM CREATE collision check is `state.IsNonZeroAccount(X)`.
`ParallelWorldState` (the production decorator over `WorldState`,
registered at `BlockProcessingModule.cs:56-57`) does not override
`IsNonZeroAccount`, so the default interface method on `IWorldState`
runs. That default calls `IsStorageEmpty(X)`, which is also
unoverridden in `ParallelWorldState`, so it routes to the default in
`IAccountStateProvider`:
TryGetAccount(address, out AccountStruct account);
return account.IsStorageEmpty;
`TryGetAccount` lands on `WorldState.IAccountStateProvider.TryGetAccount`,
which builds the `AccountStruct` via:
account = _stateProvider.GetAccount(address)
.WithChangedStorageRoot(_persistentStorageProvider.GetStorageRoot(address))
.ToStruct();
`PerContractState.StorageRoot` calls `EnsureStorageTree` →
`CreateStorageTree` → `_provider._currentScope.CreateStorageTree(_address)`
→ `LookupStorageTree` → `Get(X)?.StorageRoot`. `Get(X)` is
`TrieStoreWorldStateBackendScope.Get`, which serves from
`_loadedAccounts`. That cache was populated when the EVM first read X
earlier in the block (e.g. to load X's code for tx 0x2d's
SELFDESTRUCT call), and it holds the parent (pre-destroy) account
with a non-empty `StorageRoot`.
Result: `GetStorageRoot` returns the parent's non-empty `StorageRoot`,
`WithChangedStorageRoot` stuffs it into the `AccountStruct`,
`account.IsStorageEmpty` is `_storageRoot == EmptyTreeHash` and so
returns false, `IsNonZeroAccount` short-circuits to true, the inner
CREATE inside the metamorphic transient hits a phantom
EIP-684/EIP-7610 collision, the factory's
`require(deployed != 0)` reverts, and the block ends with
`HeaderGasUsedMismatch`.
`PerContractState.IsEmpty` already short-circuits on
`BlockChange.HasClear` (the same-block SELFDESTRUCT marker), but
`PerContractState.StorageRoot` did not.
## Fix
Add the same `BlockChange.HasClear` short-circuit to
`PerContractState.StorageRoot`, matching `IsEmpty`'s contract: while
a HasClear marker is in effect, the storage tree is logically empty
regardless of what cached parent state any consumer has.
## Tests
* `SelfDestructPrewarmerStateTests
.IsNonZeroAccount_via_IWorldState_default_returns_false_after_destroy_and_value_transfer`
exercises `ParallelWorldState` wrapping `WorldState`, primes
`_loadedAccounts` the way the EVM does, runs destroy + value
transfer, then performs the collision check via the `IWorldState`
default interface method. The test fails on the parent commit and
passes with this fix.
* `SelfDestructPrewarmerStateTests.Main_scope_sees_destroyed_account_even_with_prewarmed_cache`
and `Main_scope_sees_recreated_account_as_collision_free_after_value_transfer`
pin the cache-layer correctness for completeness.
* `SelfDestructCreate2SameBlockTests` runs the full
block-production + validation flow (TestBlockchain, prewarming
enabled by default) for two-tx and three-tx variants of the
destroy-then-redeploy pattern with different senders.
Existing suites still pass after the fix:
- Nethermind.State.Test: 707 passed
- Nethermind.Blockchain.Test: 1426 passed
- Nethermind.Evm.Test SELFDESTRUCT/CREATE/Eip7708: 32 passed
## Verification on the affected node
Confirmed by the reporter on a Sepolia archive node running
1.37.1 + this fix: block 4913057 validates and sync resumes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stdevMac
requested review from
LukaszRozmej,
asdacap,
benaadams,
damian-orzechowski and
flcl42
as code owners
|
@claude review |
|
Claude finished @LukaszRozmej's task in 5m 43s —— View job PR Review
SummaryThe fix is correct, minimal, and well-motivated. The bug is subtle — Call-path trace (confirmed against source)
Production fix —
|
| File | Pattern | Notes |
|---|---|---|
SelfDestructPrewarmerStateTests |
Direct state-layer | Correctly exercises the ParallelWorldState DIM path; IsNonZeroAccount_via_IWorldState_default_returns_false_after_destroy_and_value_transfer is the regression test. |
SelfDestructCreate2SameBlockTests |
BasicTestBlockchain + DI |
Correct DI pattern; two-tx and three-tx variants cover both the minimal case and the exact Sepolia block layout. |
Findings
| Severity | Location | Finding |
|---|---|---|
| Low | PersistentStorageProvider.cs:476–483 |
8-line comment for a 1-line fix; could be reduced to 2–3 lines per coding-style guidelines (comment should explain why, not re-derive the entire call graph). |
| Low | SelfDestructPrewarmerStateTests.cs:143 |
IsNonZeroAccount_via_IWorldState_default_returns_false_after_destroy_and_value_transfer duplicates the TrieStore/WorldState construction that BuildSharedCacheWorldStates() already provides; a small refactor would avoid the repetition. Note: manual construction is consistent with existing State tests in this project. |
| Low | SelfDestructCreate2SameBlockTests.cs:1 |
Missing #nullable enable (present in the companion file). |
No Critical, High, or Medium findings.
Correctness note on HasClear + subsequent writes
If ClearStorage is followed by new storage writes in the same tx commit window (exotic re-entrancy), StorageRoot would still return EmptyTreeHash. This is identical to the pre-existing IsEmpty behaviour and is not introduced by this PR.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // (used by ParallelWorldState because it does not provide its own override) flag | ||
| // a phantom EIP-684/7610 collision when a metamorphic factory does | ||
| // CREATE-inside-CREATE2 at the destroyed address in the same block. | ||
| if (BlockChange.HasClear) return Keccak.EmptyTreeHash; |
There was a problem hiding this comment.
The fix is correct and precisely mirrors the existing IsEmpty short-circuit six lines below. A couple of small observations:
-
Comment verbosity: The 8-line block is thorough, but per the coding style guidelines, comments should be concise and explain the why in one short line. The critical "why" is: without this,
TryGetAccount → GetStorageRootbypasses theIsEmptyinvariant and leaks the parent trie root through_loadedAccounts, producing a phantom EIP-684/7610 collision. Something like:// Without this, TryGetAccount → GetStorageRoot leaks the parent trie root from // _loadedAccounts, causing a phantom EIP-684/7610 collision on same-block redeploy. if (BlockChange.HasClear) return Keccak.EmptyTreeHash;
would be sufficient. The full root-cause narrative belongs in the commit message or PR description (where it already lives).
-
Invariant consistency: The fix restores the invariant
StorageRoot == EmptyTreeHash ⟺ IsEmptyunderHasClear, which is exactly correct. No correctness concerns. -
Post-clear writes: If storage slots are written after
ClearStoragewithin the same tx commit window (e.g. a re-entrancy that destroys and re-populates in one tx),StorageRootwould still returnEmptyTreeHash— butIsEmptyhas the same behaviour. This is pre-existing and outside this PR's scope.
| { | ||
| // Use ParallelWorldState (production decorator over WorldState) so we exercise the | ||
| // default-interface-method dispatch path that the EVM actually hits in production. | ||
| IDbProvider dbProvider = TestMemDbProvider.Init(); |
There was a problem hiding this comment.
This test manually constructs TrieStore / NodeStorage / TrieStoreScopeProvider / WorldState / ParallelWorldState. The test-infrastructure rule says to prefer ContainerBuilder + production modules and avoid manual construction (see .agents/rules/test-infrastructure.md).
In practice, this pattern is already used throughout the existing Nethermind.State.Test suite (StorageProviderTests, StateProviderTests, ScopeProviderTests, etc.), so it is at least consistent with the surrounding test style. But it is still worth noting for future refactoring: a shared helper that accepts a lambda to customise the WorldState wrapper (e.g. to swap in ParallelWorldState) would remove the duplication between this test and BuildSharedCacheWorldStates().
| @@ -0,0 +1,196 @@ | |||
| // SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited | |||
There was a problem hiding this comment.
Missing #nullable enable at the top of this file. SelfDestructPrewarmerStateTests.cs (the companion file in this PR) opts in; it is worth being consistent, and the project generally enables nullable analysis. Fix this →
|
Although maybe better fix is to just not register ParallelWorldState? @claude |
|
Claude finished @LukaszRozmej's task in 2m 35s —— View job Analysis: remove
|
| Approach | Blast radius | Fixes root cause | Preserves EIP-7928 infra |
|---|---|---|---|
Remove ParallelWorldState |
High — removes BAL tracking | No — StorageRoot invariant still broken for direct callers |
No |
Override IsNonZeroAccount in WrappedWorldState |
Minimal | Partially — only the DIM path | Yes |
This PR: short-circuit in PerContractState.StorageRoot |
Minimal — 1 line | Yes — invariant correct for all callers | Yes |
The PR is correct as-is. The WrappedWorldState override is optional complementary hardening.
|
@claude we don't need BAL tracking in the release. In master this is already fixed in different way. We don't need BAL's here at all |
|
Claude finished @LukaszRozmej's task in 3m 11s —— View job Updated Analysis: Remove
|
| Approach | Root cause | Lines changed | EIP-7928 infra |
|---|---|---|---|
Current PR (one-line in StorageRoot) |
Fixes invariant violation | 1 | Preserved |
Remove AddDecorator<IWorldState, ParallelWorldState>() |
Removes the faulty DIM dispatch | 1 | Removed (unneeded in 1.37.1) |
| Both | Belt-and-suspenders | 2 | Removed |
Removing the decorator is correct and simpler for this release. Up to you whether to also keep the StorageRoot guard as a defensive invariant fix.
3 participants
Changes
PerContractState.StorageRootnow short-circuits toKeccak.EmptyTreeHashwhen the same-block SELFDESTRUCT marker (BlockChange.HasClear) is set — mirroring the existing short-circuit inPerContractState.IsEmpty. One added line inNethermind.State/PersistentStorageProvider.cs.SelfDestructPrewarmerStateTests.IsNonZeroAccount_via_IWorldState_default_returns_false_after_destroy_and_value_transferreproduces the bug at the state layer (fails on parent, passes with this fix).SelfDestructPrewarmerStateTestsand two block-level integration tests inSelfDestructCreate2SameBlockTestslock in correctness for the same-block SELFDESTRUCT-then-redeploy pattern with both two-tx and three-tx (with intermediate value transfer) variants.Symptom
Sepolia archive node running 1.37.1 deterministically rejects block 4913057 with
HeaderGasUsedMismatch: Expected 9922769, got 9916180(Δ = 6589 gas, exactly one EIP-684 collision revert with refund), and stops processing further blocks. The failing transaction is the metamorphic-contract redeploy at index 0x3a in the block:debug_traceCallreproduces the failure when run against parent state (i.e. without applying tx 0x2d's SELFDESTRUCT first), proving the EVM is correct in isolation and that block-level state propagation between the destroy and the redeploy is broken.Root cause
The EVM CREATE collision check is
state.IsNonZeroAccount(X).ParallelWorldState(the production decorator overWorldState, registered atBlockProcessingModule.cs:56-57) does not overrideIsNonZeroAccount, so the default interface method onIWorldStateruns. That default callsIsStorageEmpty(X), also unoverridden inParallelWorldState, so it routes to the default inIAccountStateProvider:TryGetAccountlands onWorldState.IAccountStateProvider.TryGetAccount, which builds theAccountStructvia:PerContractState.StorageRootcallsEnsureStorageTree→CreateStorageTree→_provider._currentScope.CreateStorageTree(_address)→LookupStorageTree→Get(X)?.StorageRoot.Get(X)isTrieStoreWorldStateBackendScope.Get, which serves from_loadedAccounts. That cache was populated when the EVM first read X earlier in the block (e.g. to load X's code for tx 0x2d's SELFDESTRUCT call), and it holds the parent (pre-destroy) account with a non-emptyStorageRoot.Result:
GetStorageRootreturns the parent's non-emptyStorageRoot,WithChangedStorageRootstuffs it into theAccountStruct,account.IsStorageEmptyis_storageRoot == EmptyTreeHashso returns false,IsNonZeroAccountshort-circuits to true, the inner CREATE inside the metamorphic transient hits a phantom EIP-684/EIP-7610 collision, the factory'srequire(deployed != 0)reverts, and the block ends withHeaderGasUsedMismatch.PerContractState.IsEmptyalready short-circuits onBlockChange.HasClear.PerContractState.StorageRootdid not. This PR adds the same short-circuit, so any consumer (including theIAccountStateProvider.TryGetAccountpath) seesEmptyTreeHashwhile the SELFDESTRUCT marker is in effect.The fix is a one-line short-circuit at the boundary that matches the existing
IsEmptyinvariant; no behavioural change for code paths that don't go throughTryGetAccount/StorageRoot.Types of changes
Testing
Requires testing
If yes, did you write tests?
Notes on testing
SelfDestructPrewarmerStateTests.IsNonZeroAccount_via_IWorldState_default_returns_false_after_destroy_and_value_transfer— exercisesParallelWorldStatewrappingWorldState, primes_loadedAccountsthe way the EVM does, then performs the collision check via theIWorldStatedefault interface method. Fails on the parent commit and passes with this fix.Main_scope_sees_destroyed_account_even_with_prewarmed_cacheandMain_scope_sees_recreated_account_as_collision_free_after_value_transferpin cache-layer correctness.SelfDestructCreate2SameBlockTests(two test cases) drives the fullBlockProcessor+BranchProcessor+ prewarming flow throughBasicTestBlockchainfor both two-tx and three-tx (with intermediate value transfer) variants.Existing suites still pass with the fix:
Nethermind.State.TestNethermind.Blockchain.TestNethermind.Evm.Test(SELFDESTRUCT/CREATE/Eip7708 selectors)Verification on an affected node
Confirmed on a live Sepolia archive node running 1.37.1 + this fix: block 4913057 validates and sync resumes from there.
Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
The same defect class is likely worth checking on
master, althoughNethermind.Statehas been substantially restructured there (cross-block caches, shardedNodeStorageCache, etc.) — the symbol and call path may be different. Worth a quick verification but out of scope for this 1.37.1 backport.