From c94663a1388962dd51de98ba4f1177f0f49aea9a Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Wed, 26 Aug 2026 20:41:47 +0700 Subject: [PATCH 1/9] execution/state: skip the self-destruct storage walk when the address has no account A self-destruct emits DELETE for every committed storage slot, which means a prefix walk over the storage domain. That walk seeks the .bt index of every storage .kv file, so it costs the same whether the address owns a thousand slots or none. An address with no committed account owns none: storage is only written for an account that exists, and deleting an account wipes its storage prefix. Probing the account first is served by the per-file existence filters, so the walk is skipped for a contract created and destroyed inside one batch, which never has a committed account. That is the whole of the traffic this was found on -- early 2019 gas-token burns destroy 42-44 storage-less children per transaction, and on mainnet blocks 7319406-7320491 every one of those walks came back empty: sdDomainCalls == sdDomainEmpty in all 167 logged cascades, 1.45s of a 12.65s execution phase. Same argument and the same assert as the create-time wipe, so the four copies of the walk collapse into one helper. --- execution/builder/exec.go | 13 ++--- execution/stagedsync/exec3_parallel.go | 26 +++------- execution/state/writeset_normalize.go | 34 +++++++++++++ execution/state/writeset_normalize_test.go | 55 ++++++++++++++++++++++ execution/tests/blockgen/chain_makers.go | 13 ++--- 5 files changed, 101 insertions(+), 40 deletions(-) diff --git a/execution/builder/exec.go b/execution/builder/exec.go index 515a88d1799..532d6c9805b 100644 --- a/execution/builder/exec.go +++ b/execution/builder/exec.go @@ -241,16 +241,9 @@ func execBlock(ctx context0.Context, sd *execctx.SharedDomains, tx kv.TemporalTx blockRules := blockCtx.Rules(cfg.chainConfig) var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - av := addr.Value() - const addrLen, hashLen = 20, 32 - var keys []accounts.StorageKey - if iterErr := sd.IteratePrefix(kv.StorageDomain, av[:], tx, func(k, _ []byte) (bool, error) { - if len(k) >= addrLen+hashLen { - keys = append(keys, accounts.InternKey(common.BytesToHash(k[addrLen:addrLen+hashLen]))) - } - return true, nil - }); iterErr != nil { - domainKeysErr = iterErr + keys, err := state.CommittedStorageKeys(sd, tx, addr) + if err != nil { + domainKeysErr = err return nil } return keys diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index 6468e07358e..346cf5cf500 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -3057,16 +3057,9 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r // and prior-block storage that vm.StorageKeys doesn't see. var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - av := addr.Value() - const addrLen, hashLen = 20, 32 // StorageDomain composite key = addr ++ slotHash - var keys []accounts.StorageKey - if iterErr := pe.rs.Domains().IteratePrefix(kv.StorageDomain, av[:], applyTx, func(k, _ []byte) (bool, error) { - if len(k) >= addrLen+hashLen { - keys = append(keys, accounts.InternKey(common.BytesToHash(k[addrLen:addrLen+hashLen]))) - } - return true, nil - }); iterErr != nil { - domainKeysErr = iterErr + keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, addr) + if err != nil { + domainKeysErr = err return nil } return keys @@ -3328,16 +3321,9 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r // redundant. var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - av := addr.Value() - const addrLen, hashLen = 20, 32 - var keys []accounts.StorageKey - if iterErr := pe.rs.Domains().IteratePrefix(kv.StorageDomain, av[:], applyTx, func(k, _ []byte) (bool, error) { - if len(k) >= addrLen+hashLen { - keys = append(keys, accounts.InternKey(common.BytesToHash(k[addrLen:addrLen+hashLen]))) - } - return true, nil - }); iterErr != nil { - domainKeysErr = iterErr + keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, addr) + if err != nil { + domainKeysErr = err return nil } return keys diff --git a/execution/state/writeset_normalize.go b/execution/state/writeset_normalize.go index 534b4e8ee34..74383f6caf5 100644 --- a/execution/state/writeset_normalize.go +++ b/execution/state/writeset_normalize.go @@ -19,7 +19,10 @@ package state import ( "github.com/holiman/uint256" + "github.com/erigontech/erigon/common" "github.com/erigontech/erigon/common/log/v3" + "github.com/erigontech/erigon/db/kv" + "github.com/erigontech/erigon/db/state/execctx" "github.com/erigontech/erigon/diagnostics/metrics" "github.com/erigontech/erigon/execution/types" "github.com/erigontech/erigon/execution/types/accounts" @@ -488,3 +491,34 @@ func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, return filtered, nil } + +// CommittedStorageKeys returns every storage slot committed for addr, the +// domainStorageKeys input Normalize needs to emit a full self-destruct cascade. +// +// An address with no committed account holds no committed storage: storage is +// only written for an account that exists, and deleting an account wipes its +// storage prefix. So probe the account first -- that read is served by the +// per-file existence filters, while the prefix walk has to seek the .bt index of +// every storage .kv file, paying the same price whether the address owns a +// thousand slots or none. +func CommittedStorageKeys(domains *execctx.SharedDomains, tx kv.TemporalTx, addr accounts.Address) ([]accounts.StorageKey, error) { + av := addr.Value() + prevAcc, _, err := domains.GetLatest(kv.AccountsDomain, tx, av[:]) + if err != nil { + return nil, err + } + if len(prevAcc) == 0 { + return nil, assertNoCommittedStorage(domains, tx, av[:]) + } + const addrLen, hashLen = 20, 32 // StorageDomain composite key = addr ++ slotHash + var keys []accounts.StorageKey + if err := domains.IteratePrefix(kv.StorageDomain, av[:], tx, func(k, _ []byte) (bool, error) { + if len(k) >= addrLen+hashLen { + keys = append(keys, accounts.InternKey(common.BytesToHash(k[addrLen:addrLen+hashLen]))) + } + return true, nil + }); err != nil { + return nil, err + } + return keys, nil +} diff --git a/execution/state/writeset_normalize_test.go b/execution/state/writeset_normalize_test.go index 4877c4c97d9..7398844d5c0 100644 --- a/execution/state/writeset_normalize_test.go +++ b/execution/state/writeset_normalize_test.go @@ -8,6 +8,8 @@ import ( "github.com/stretchr/testify/require" "github.com/erigontech/erigon/common" + "github.com/erigontech/erigon/common/dbg" + "github.com/erigontech/erigon/db/kv" "github.com/erigontech/erigon/execution/types/accounts" ) @@ -255,3 +257,56 @@ func TestAssertSelfDestructNormalized(t *testing.T) { ws.assertSelfDestructNormalized() }, "balance and storage deletes are legal on a self-destructed address") } + +// TestCommittedStorageKeys pins the account probe that guards the self-destruct +// storage walk. The walk seeks the .bt index of every storage .kv file, so its +// cost is set by the file count rather than by how much storage the address +// owns -- an address that owns none pays full price. A contract created and +// destroyed inside one batch never has a committed account, which is what makes +// the probe worth its own read. +func TestCommittedStorageKeys(t *testing.T) { + _, tx, domains := NewTestRwTx(t) + slot := common.HexToHash("0x01") + + putStorage := func(t *testing.T, addr accounts.Address) { + t.Helper() + av := addr.Value() + key := append(append([]byte{}, av[:]...), slot[:]...) + require.NoError(t, domains.DomainPut(kv.StorageDomain, tx, key, []byte{0x42}, 0, nil)) + } + + t.Run("walks when the account is committed", func(t *testing.T) { + addr := accounts.InternAddress(common.HexToAddress("0xaa")) + av := addr.Value() + acc := accounts.Account{Nonce: 1, CodeHash: accounts.EmptyCodeHash} + require.NoError(t, domains.DomainPut(kv.AccountsDomain, tx, av[:], accounts.SerialiseV3(&acc), 0, nil)) + putStorage(t, addr) + + keys, err := CommittedStorageKeys(domains, tx, addr) + require.NoError(t, err) + require.Equal(t, []accounts.StorageKey{accounts.InternKey(slot)}, keys) + }) + + t.Run("skips the walk without a committed account", func(t *testing.T) { + // Storage with no account is the state the guard treats as unreachable. + // Writing it is what makes the skip observable: a walk would return it. + defer func(v bool) { dbg.AssertEnabled = v }(dbg.AssertEnabled) + dbg.AssertEnabled = false + addr := accounts.InternAddress(common.HexToAddress("0xbb")) + putStorage(t, addr) + + keys, err := CommittedStorageKeys(domains, tx, addr) + require.NoError(t, err) + require.Empty(t, keys, "walked the storage prefix for an address with no committed account") + }) + + t.Run("asserts on storage without an account", func(t *testing.T) { + defer func(v bool) { dbg.AssertEnabled = v }(dbg.AssertEnabled) + dbg.AssertEnabled = true + addr := accounts.InternAddress(common.HexToAddress("0xcc")) + putStorage(t, addr) + + require.Panics(t, func() { _, _ = CommittedStorageKeys(domains, tx, addr) }, + "the skip rests on this invariant, so breaking it must not stay silent") + }) +} diff --git a/execution/tests/blockgen/chain_makers.go b/execution/tests/blockgen/chain_makers.go index 3c5c3c91de8..26db843b646 100644 --- a/execution/tests/blockgen/chain_makers.go +++ b/execution/tests/blockgen/chain_makers.go @@ -575,16 +575,9 @@ func GenerateChain(config *chain.Config, parent *types.Block, engine rules.Engin blockNum := b.header.Number.Uint64() var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - av := addr.Value() - const addrLen, hashLen = 20, 32 - var keys []accounts.StorageKey - if iterErr := domains.IteratePrefix(kv.StorageDomain, av[:], tx, func(k, _ []byte) (bool, error) { - if len(k) >= addrLen+hashLen { - keys = append(keys, accounts.InternKey(common.BytesToHash(k[addrLen:addrLen+hashLen]))) - } - return true, nil - }); iterErr != nil { - domainKeysErr = iterErr + keys, err := state.CommittedStorageKeys(domains, tx, addr) + if err != nil { + domainKeysErr = err return nil } return keys From 246bc88bd1a5651a870c9f7eca4a311530533493 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Wed, 26 Aug 2026 21:45:26 +0700 Subject: [PATCH 2/9] execution/state: keep the self-destruct walk for an address destroyed in this block The account probe alone was wrong. With a block cache active the self-destruct apply path routes the delete through BlockStateCache.DeleteAccount and returns before DomainDelPrefix, so the storage prefix only reaches the domain at the block-end flush. Until then the account reads absent while its pre-block storage is still there and still owed a trie delete, and skipping the walk lost that cascade -- EEST test_recreate_self_destructed_contract_different_txs returned INVALID. A destroy is recorded as a present-but-nil current entry, which separates 'destroyed here' from 'never existed', so the skip now needs both. A nil cache keeps the plain probe: that path wipes the prefix inline, so the window does not exist. Gas-token children are created and destroyed inside one transaction and are never carried into the block cache as a destroy of prior state, so the case this targets still skips. --- execution/builder/exec.go | 2 +- execution/stagedsync/exec3_parallel.go | 4 ++-- execution/state/rw_v3.go | 13 ++++++++++++ execution/state/writeset_normalize.go | 7 +++++-- execution/state/writeset_normalize_test.go | 24 +++++++++++++++++++--- execution/tests/blockgen/chain_makers.go | 2 +- 6 files changed, 43 insertions(+), 9 deletions(-) diff --git a/execution/builder/exec.go b/execution/builder/exec.go index 532d6c9805b..e9dff89dd35 100644 --- a/execution/builder/exec.go +++ b/execution/builder/exec.go @@ -241,7 +241,7 @@ func execBlock(ctx context0.Context, sd *execctx.SharedDomains, tx kv.TemporalTx blockRules := blockCtx.Rules(cfg.chainConfig) var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(sd, tx, addr) + keys, err := state.CommittedStorageKeys(sd, tx, nil, addr) if err != nil { domainKeysErr = err return nil diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index 346cf5cf500..a3ce4bc4f95 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -3057,7 +3057,7 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r // and prior-block storage that vm.StorageKeys doesn't see. var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, addr) + keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, be.blockStateCache, addr) if err != nil { domainKeysErr = err return nil @@ -3321,7 +3321,7 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r // redundant. var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, addr) + keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, be.blockStateCache, addr) if err != nil { domainKeysErr = err return nil diff --git a/execution/state/rw_v3.go b/execution/state/rw_v3.go index b9ad00c3d67..abbb4c33117 100644 --- a/execution/state/rw_v3.go +++ b/execution/state/rw_v3.go @@ -1184,6 +1184,19 @@ func (c *BlockStateCache) DeleteAccount(addr accounts.Address, txNum uint64) { c.mu.Unlock() } +// deletedInBlock reports an address destroyed earlier in this block. DeleteAccount +// records that as a present-but-nil current entry, which the block-end flush turns +// into the domain deletes. Nil receiver: no cache, so no such window. +func (c *BlockStateCache) deletedInBlock(addr accounts.Address) bool { + if c == nil { + return false + } + c.mu.RLock() + defer c.mu.RUnlock() + enc, present := c.currentAccounts[addr] + return present && enc == nil +} + // GetCurrentAccountDecoded returns the latest account (including intra-block // writes), avoiding GetCurrentAccount's re-encode of the committed entry. func (c *BlockStateCache) GetCurrentAccountDecoded(addr accounts.Address) (*accounts.Account, bool, error) { diff --git a/execution/state/writeset_normalize.go b/execution/state/writeset_normalize.go index 74383f6caf5..2dd55fcca53 100644 --- a/execution/state/writeset_normalize.go +++ b/execution/state/writeset_normalize.go @@ -501,13 +501,16 @@ func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, // per-file existence filters, while the prefix walk has to seek the .bt index of // every storage .kv file, paying the same price whether the address owns a // thousand slots or none. -func CommittedStorageKeys(domains *execctx.SharedDomains, tx kv.TemporalTx, addr accounts.Address) ([]accounts.StorageKey, error) { +func CommittedStorageKeys(domains *execctx.SharedDomains, tx kv.TemporalTx, blockCache *BlockStateCache, addr accounts.Address) ([]accounts.StorageKey, error) { av := addr.Value() prevAcc, _, err := domains.GetLatest(kv.AccountsDomain, tx, av[:]) if err != nil { return nil, err } - if len(prevAcc) == 0 { + // A destroy recorded in the block cache only reaches the domain at the + // block-end flush, so until then the account reads absent while its + // pre-block storage is still there and still owed a trie delete. + if len(prevAcc) == 0 && !blockCache.deletedInBlock(addr) { return nil, assertNoCommittedStorage(domains, tx, av[:]) } const addrLen, hashLen = 20, 32 // StorageDomain composite key = addr ++ slotHash diff --git a/execution/state/writeset_normalize_test.go b/execution/state/writeset_normalize_test.go index 7398844d5c0..998baacb685 100644 --- a/execution/state/writeset_normalize_test.go +++ b/execution/state/writeset_normalize_test.go @@ -282,7 +282,7 @@ func TestCommittedStorageKeys(t *testing.T) { require.NoError(t, domains.DomainPut(kv.AccountsDomain, tx, av[:], accounts.SerialiseV3(&acc), 0, nil)) putStorage(t, addr) - keys, err := CommittedStorageKeys(domains, tx, addr) + keys, err := CommittedStorageKeys(domains, tx, nil, addr) require.NoError(t, err) require.Equal(t, []accounts.StorageKey{accounts.InternKey(slot)}, keys) }) @@ -295,18 +295,36 @@ func TestCommittedStorageKeys(t *testing.T) { addr := accounts.InternAddress(common.HexToAddress("0xbb")) putStorage(t, addr) - keys, err := CommittedStorageKeys(domains, tx, addr) + keys, err := CommittedStorageKeys(domains, tx, nil, addr) require.NoError(t, err) require.Empty(t, keys, "walked the storage prefix for an address with no committed account") }) + t.Run("walks when the account was destroyed in this block", func(t *testing.T) { + // The self-destruct apply path leaves the storage prefix in the domain + // until the block-end flush when a block cache is active, so an absent + // account no longer implies absent storage. Skipping here loses the + // cascade the trie needs and the root goes wrong. + defer func(v bool) { dbg.AssertEnabled = v }(dbg.AssertEnabled) + dbg.AssertEnabled = false + addr := accounts.InternAddress(common.HexToAddress("0xdd")) + putStorage(t, addr) + cache := NewBlockStateCache() + cache.DeleteAccount(addr, 0) + + keys, err := CommittedStorageKeys(domains, tx, cache, addr) + require.NoError(t, err) + require.Equal(t, []accounts.StorageKey{accounts.InternKey(slot)}, keys, + "skipped the walk for an address destroyed earlier in this block") + }) + t.Run("asserts on storage without an account", func(t *testing.T) { defer func(v bool) { dbg.AssertEnabled = v }(dbg.AssertEnabled) dbg.AssertEnabled = true addr := accounts.InternAddress(common.HexToAddress("0xcc")) putStorage(t, addr) - require.Panics(t, func() { _, _ = CommittedStorageKeys(domains, tx, addr) }, + require.Panics(t, func() { _, _ = CommittedStorageKeys(domains, tx, nil, addr) }, "the skip rests on this invariant, so breaking it must not stay silent") }) } diff --git a/execution/tests/blockgen/chain_makers.go b/execution/tests/blockgen/chain_makers.go index 26db843b646..c80d3c49275 100644 --- a/execution/tests/blockgen/chain_makers.go +++ b/execution/tests/blockgen/chain_makers.go @@ -575,7 +575,7 @@ func GenerateChain(config *chain.Config, parent *types.Block, engine rules.Engin blockNum := b.header.Number.Uint64() var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(domains, tx, addr) + keys, err := state.CommittedStorageKeys(domains, tx, nil, addr) if err != nil { domainKeysErr = err return nil From 21ab3cfc3927abb7ce805ae79cb0c8b78650a118 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Thu, 27 Aug 2026 10:36:56 +0700 Subject: [PATCH 3/9] execution/state: pin per-txn history across a self-destruct cascade Erigon records history per transaction, so an RPC re-exec of a later transaction must see a destroyed contract's storage already gone while a read at the destroying txNum still sees it. Those history records exist only because the cascade enumerates the committed slots, and nothing covered that: the existing cascade tests stop at Normalize's output, and a trie-root check compares only the block's final state, where the prefix wipe hides a missing per-txn delete. Drives Normalize through Apply and Flush, then reads GetAsOf on both sides of the destroying txNum. Forcing CommittedStorageKeys to always skip turns it red. --- execution/state/writeset_normalize_test.go | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/execution/state/writeset_normalize_test.go b/execution/state/writeset_normalize_test.go index 998baacb685..2c582868aab 100644 --- a/execution/state/writeset_normalize_test.go +++ b/execution/state/writeset_normalize_test.go @@ -10,6 +10,7 @@ import ( "github.com/erigontech/erigon/common" "github.com/erigontech/erigon/common/dbg" "github.com/erigontech/erigon/db/kv" + "github.com/erigontech/erigon/execution/chain" "github.com/erigontech/erigon/execution/types/accounts" ) @@ -328,3 +329,51 @@ func TestCommittedStorageKeys(t *testing.T) { "the skip rests on this invariant, so breaking it must not stay silent") }) } + +// TestSelfDestructCascade_PerTxnHistory pins per-txn history granularity across a +// self-destruct. Erigon records history per transaction, so an RPC re-exec of a +// later transaction must see the destroyed contract's storage already gone while +// a read at the destroying txNum still sees it. Those history records exist only +// because the cascade enumerates the committed slots -- a walk that is skipped +// when it should not be leaves the deletes out of history, which a trie-root +// check cannot see since it only compares the block's final state. +func TestSelfDestructCascade_PerTxnHistory(t *testing.T) { + _, tx, domains := NewTestRwTx(t) + addr := accounts.InternAddress(common.HexToAddress("0x5d")) + slot := common.HexToHash("0x07") + av := addr.Value() + composite := append(append([]byte{}, av[:]...), slot[:]...) + + const preTxNum, sdTxNum = uint64(1), uint64(4) + + acc := accounts.Account{Nonce: 1, CodeHash: accounts.EmptyCodeHash} + require.NoError(t, domains.DomainPut(kv.AccountsDomain, tx, av[:], accounts.SerialiseV3(&acc), preTxNum, nil)) + require.NoError(t, domains.DomainPut(kv.StorageDomain, tx, composite, []byte{0xaa}, preTxNum, nil)) + + ws := &WriteSet{} + ws.SetSelfDestruct(addr, &VersionedWrite[bool]{ + WriteHeader: WriteHeader{Address: addr, Path: SelfDestructPath, Version: Version{TxIndex: 1}}, + Val: true, + }) + domainKeys := func(a accounts.Address) []accounts.StorageKey { + keys, err := CommittedStorageKeys(domains, tx, nil, a) + require.NoError(t, err) + return keys + } + out, err := ws.Normalize(NewVersionMap(nil), 1, 0, &minimalStateReader{}, domainKeys, false, false, false) + require.NoError(t, err) + _, cascaded := out.GetStorage(addr, accounts.InternKey(slot)) + require.True(t, cascaded, "the cascade dropped the committed slot, so history never records its delete") + + require.NoError(t, out.Apply(domains, tx, 0, sdTxNum, nil, &chain.Rules{}, nil, false)) + // History is queried through the tx, so the batch has to reach it first. + require.NoError(t, domains.Flush(t.Context(), tx)) + + before, _, err := tx.GetAsOf(kv.StorageDomain, composite, sdTxNum) + require.NoError(t, err) + require.Equal(t, []byte{0xaa}, before, "history lost the slot value the destroying txn read") + + after, _, err := tx.GetAsOf(kv.StorageDomain, composite, sdTxNum+1) + require.NoError(t, err) + require.Empty(t, after, "a later txNum still sees the destroyed contract's storage") +} From a3df0c65ef3cfb81f47398f0dda9317ffcdaff9e Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Fri, 28 Aug 2026 10:52:35 +0700 Subject: [PATCH 4/9] execution/state: share the committed-account probe, name the assert's caller hasCommittedAccount carries the invariant once, for both the create-time wipe and the self-destruct cascade. assertNoCommittedStorage takes the caller's name so a trip points at the right path. --- execution/state/rw_v3.go | 37 ++++++++++++++-------- execution/state/writeset_normalize.go | 15 +++------ execution/state/writeset_normalize_test.go | 24 ++++++++++---- 3 files changed, 46 insertions(+), 30 deletions(-) diff --git a/execution/state/rw_v3.go b/execution/state/rw_v3.go index abbb4c33117..4f3eecaee36 100644 --- a/execution/state/rw_v3.go +++ b/execution/state/rw_v3.go @@ -224,19 +224,13 @@ func (writes *WriteSet) Apply(domains *execctx.SharedDomains, roTx kv.TemporalTx } // Contract creation: clear stale storage before writing new account. - // - // An address with no committed account holds no committed storage: - // storage is only written for an account that exists, and deleting - // an account wipes its storage prefix. So probe the account first — - // that read is served by the per-file existence filters, while the - // prefix walk has to seek the .bt index of every storage .kv file. if d.createContract { - prevAcc, _, err := domains.GetLatest(kv.AccountsDomain, roTx, address[:]) + hasAcc, err := hasCommittedAccount(domains, roTx, address[:]) if err != nil { return err } - if len(prevAcc) == 0 { - if err := assertNoCommittedStorage(domains, roTx, address[:]); err != nil { + if !hasAcc { + if err := assertNoCommittedStorage(domains, roTx, address[:], "createContract"); err != nil { return err } } else if err := domains.DomainDelPrefix(kv.StorageDomain, roTx, address[:], txNum); err != nil { @@ -832,10 +826,25 @@ func (w *Writer) PrevAndDels() (map[string][]byte, map[string]*accounts.Account, return nil, nil, nil, nil } +// hasCommittedAccount probes the account domain for addr. An address with no +// committed account holds no committed storage: storage is only written for an +// account that exists, and deleting an account wipes its storage prefix. The +// probe is served by the per-file existence filters, while a storage-prefix walk +// has to seek the .bt index of every storage .kv file — the same price whether +// the address owns a thousand slots or none. +func hasCommittedAccount(domains *execctx.SharedDomains, roTx kv.TemporalTx, addr []byte) (bool, error) { + enc, _, err := domains.GetLatest(kv.AccountsDomain, roTx, addr) + if err != nil { + return false, err + } + return len(enc) > 0, nil +} + // assertNoCommittedStorage panics when addr has committed storage but no -// committed account, so a violation of that invariant surfaces instead of -// silently skipping a storage wipe. No-op unless asserts are enabled. -func assertNoCommittedStorage(domains *execctx.SharedDomains, roTx kv.TemporalTx, addr []byte) error { +// committed account, so a violation of the hasCommittedAccount invariant +// surfaces instead of silently skipping a storage wipe. what names the caller so +// a trip points at the right path. No-op unless asserts are enabled. +func assertNoCommittedStorage(domains *execctx.SharedDomains, roTx kv.TemporalTx, addr []byte, what string) error { if !dbg.AssertEnabled { return nil } @@ -847,7 +856,7 @@ func assertNoCommittedStorage(domains *execctx.SharedDomains, roTx kv.TemporalTx return err } if found > 0 { - panic(fmt.Sprintf("createContract: %x has storage but no account", addr)) + panic(fmt.Sprintf("%s: %x has storage but no account", what, addr)) } return nil } @@ -1192,8 +1201,8 @@ func (c *BlockStateCache) deletedInBlock(addr accounts.Address) bool { return false } c.mu.RLock() - defer c.mu.RUnlock() enc, present := c.currentAccounts[addr] + c.mu.RUnlock() return present && enc == nil } diff --git a/execution/state/writeset_normalize.go b/execution/state/writeset_normalize.go index 2dd55fcca53..3c7ae5a2413 100644 --- a/execution/state/writeset_normalize.go +++ b/execution/state/writeset_normalize.go @@ -494,24 +494,19 @@ func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, // CommittedStorageKeys returns every storage slot committed for addr, the // domainStorageKeys input Normalize needs to emit a full self-destruct cascade. -// -// An address with no committed account holds no committed storage: storage is -// only written for an account that exists, and deleting an account wipes its -// storage prefix. So probe the account first -- that read is served by the -// per-file existence filters, while the prefix walk has to seek the .bt index of -// every storage .kv file, paying the same price whether the address owns a -// thousand slots or none. +// The prefix walk is skipped for an address with no committed account -- see +// hasCommittedAccount for why that probe is worth its own read. func CommittedStorageKeys(domains *execctx.SharedDomains, tx kv.TemporalTx, blockCache *BlockStateCache, addr accounts.Address) ([]accounts.StorageKey, error) { av := addr.Value() - prevAcc, _, err := domains.GetLatest(kv.AccountsDomain, tx, av[:]) + hasAcc, err := hasCommittedAccount(domains, tx, av[:]) if err != nil { return nil, err } // A destroy recorded in the block cache only reaches the domain at the // block-end flush, so until then the account reads absent while its // pre-block storage is still there and still owed a trie delete. - if len(prevAcc) == 0 && !blockCache.deletedInBlock(addr) { - return nil, assertNoCommittedStorage(domains, tx, av[:]) + if !hasAcc && !blockCache.deletedInBlock(addr) { + return nil, assertNoCommittedStorage(domains, tx, av[:], "selfDestruct") } const addrLen, hashLen = 20, 32 // StorageDomain composite key = addr ++ slotHash var keys []accounts.StorageKey diff --git a/execution/state/writeset_normalize_test.go b/execution/state/writeset_normalize_test.go index 2c582868aab..7f222f305c3 100644 --- a/execution/state/writeset_normalize_test.go +++ b/execution/state/writeset_normalize_test.go @@ -260,11 +260,8 @@ func TestAssertSelfDestructNormalized(t *testing.T) { } // TestCommittedStorageKeys pins the account probe that guards the self-destruct -// storage walk. The walk seeks the .bt index of every storage .kv file, so its -// cost is set by the file count rather than by how much storage the address -// owns -- an address that owns none pays full price. A contract created and -// destroyed inside one batch never has a committed account, which is what makes -// the probe worth its own read. +// storage walk. A contract created and destroyed inside one batch never has a +// committed account, which is what makes the probe worth its own read. func TestCommittedStorageKeys(t *testing.T) { _, tx, domains := NewTestRwTx(t) slot := common.HexToHash("0x01") @@ -319,6 +316,16 @@ func TestCommittedStorageKeys(t *testing.T) { "skipped the walk for an address destroyed earlier in this block") }) + t.Run("skips silently when neither account nor storage is committed", func(t *testing.T) { + defer func(v bool) { dbg.AssertEnabled = v }(dbg.AssertEnabled) + dbg.AssertEnabled = true + addr := accounts.InternAddress(common.HexToAddress("0xee")) + + keys, err := CommittedStorageKeys(domains, tx, nil, addr) + require.NoError(t, err) + require.Empty(t, keys) + }) + t.Run("asserts on storage without an account", func(t *testing.T) { defer func(v bool) { dbg.AssertEnabled = v }(dbg.AssertEnabled) dbg.AssertEnabled = true @@ -355,12 +362,17 @@ func TestSelfDestructCascade_PerTxnHistory(t *testing.T) { WriteHeader: WriteHeader{Address: addr, Path: SelfDestructPath, Version: Version{TxIndex: 1}}, Val: true, }) + var domainKeysErr error domainKeys := func(a accounts.Address) []accounts.StorageKey { keys, err := CommittedStorageKeys(domains, tx, nil, a) - require.NoError(t, err) + if err != nil { + domainKeysErr = err + return nil + } return keys } out, err := ws.Normalize(NewVersionMap(nil), 1, 0, &minimalStateReader{}, domainKeys, false, false, false) + require.NoError(t, domainKeysErr) require.NoError(t, err) _, cascaded := out.GetStorage(addr, accounts.InternKey(slot)) require.True(t, cascaded, "the cascade dropped the committed slot, so history never records its delete") From 76a2f985cf3599bcfd17005c631bb11a691aea9a Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Fri, 28 Aug 2026 11:30:57 +0700 Subject: [PATCH 5/9] execution/state: drop the block-cache term from the self-destruct probe CI on a branch without it is green on eest-spec-enginextests-devnet-parallel, the only job this PR ever failed, so the term is not what fixed it. It also cannot fire: GetLatest never consults BlockStateCache, so an address with a pre-block account already reads present. The self-destruct and create-time probes are now the same condition. Drops TestSelfDestructCascade_PerTxnHistory with it: both its GetAsOf checks are carried by DomainDelPrefix and pass with the cascade removed entirely. --- execution/builder/exec.go | 2 +- execution/stagedsync/exec3_parallel.go | 4 +- execution/state/rw_v3.go | 13 ---- execution/state/writeset_normalize.go | 7 +- execution/state/writeset_normalize_test.go | 80 ++-------------------- execution/tests/blockgen/chain_makers.go | 2 +- 6 files changed, 10 insertions(+), 98 deletions(-) diff --git a/execution/builder/exec.go b/execution/builder/exec.go index e9dff89dd35..532d6c9805b 100644 --- a/execution/builder/exec.go +++ b/execution/builder/exec.go @@ -241,7 +241,7 @@ func execBlock(ctx context0.Context, sd *execctx.SharedDomains, tx kv.TemporalTx blockRules := blockCtx.Rules(cfg.chainConfig) var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(sd, tx, nil, addr) + keys, err := state.CommittedStorageKeys(sd, tx, addr) if err != nil { domainKeysErr = err return nil diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index ff308fc0663..78da8ee9775 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -3072,7 +3072,7 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r // and prior-block storage that vm.StorageKeys doesn't see. var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, be.blockStateCache, addr) + keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, addr) if err != nil { domainKeysErr = err return nil @@ -3336,7 +3336,7 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r // redundant. var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, be.blockStateCache, addr) + keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, addr) if err != nil { domainKeysErr = err return nil diff --git a/execution/state/rw_v3.go b/execution/state/rw_v3.go index 4f3eecaee36..e17b96677c7 100644 --- a/execution/state/rw_v3.go +++ b/execution/state/rw_v3.go @@ -1193,19 +1193,6 @@ func (c *BlockStateCache) DeleteAccount(addr accounts.Address, txNum uint64) { c.mu.Unlock() } -// deletedInBlock reports an address destroyed earlier in this block. DeleteAccount -// records that as a present-but-nil current entry, which the block-end flush turns -// into the domain deletes. Nil receiver: no cache, so no such window. -func (c *BlockStateCache) deletedInBlock(addr accounts.Address) bool { - if c == nil { - return false - } - c.mu.RLock() - enc, present := c.currentAccounts[addr] - c.mu.RUnlock() - return present && enc == nil -} - // GetCurrentAccountDecoded returns the latest account (including intra-block // writes), avoiding GetCurrentAccount's re-encode of the committed entry. func (c *BlockStateCache) GetCurrentAccountDecoded(addr accounts.Address) (*accounts.Account, bool, error) { diff --git a/execution/state/writeset_normalize.go b/execution/state/writeset_normalize.go index 3c7ae5a2413..2fdee176fa7 100644 --- a/execution/state/writeset_normalize.go +++ b/execution/state/writeset_normalize.go @@ -496,16 +496,13 @@ func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, // domainStorageKeys input Normalize needs to emit a full self-destruct cascade. // The prefix walk is skipped for an address with no committed account -- see // hasCommittedAccount for why that probe is worth its own read. -func CommittedStorageKeys(domains *execctx.SharedDomains, tx kv.TemporalTx, blockCache *BlockStateCache, addr accounts.Address) ([]accounts.StorageKey, error) { +func CommittedStorageKeys(domains *execctx.SharedDomains, tx kv.TemporalTx, addr accounts.Address) ([]accounts.StorageKey, error) { av := addr.Value() hasAcc, err := hasCommittedAccount(domains, tx, av[:]) if err != nil { return nil, err } - // A destroy recorded in the block cache only reaches the domain at the - // block-end flush, so until then the account reads absent while its - // pre-block storage is still there and still owed a trie delete. - if !hasAcc && !blockCache.deletedInBlock(addr) { + if !hasAcc { return nil, assertNoCommittedStorage(domains, tx, av[:], "selfDestruct") } const addrLen, hashLen = 20, 32 // StorageDomain composite key = addr ++ slotHash diff --git a/execution/state/writeset_normalize_test.go b/execution/state/writeset_normalize_test.go index 7f222f305c3..e4dd2eeea92 100644 --- a/execution/state/writeset_normalize_test.go +++ b/execution/state/writeset_normalize_test.go @@ -10,7 +10,6 @@ import ( "github.com/erigontech/erigon/common" "github.com/erigontech/erigon/common/dbg" "github.com/erigontech/erigon/db/kv" - "github.com/erigontech/erigon/execution/chain" "github.com/erigontech/erigon/execution/types/accounts" ) @@ -280,7 +279,7 @@ func TestCommittedStorageKeys(t *testing.T) { require.NoError(t, domains.DomainPut(kv.AccountsDomain, tx, av[:], accounts.SerialiseV3(&acc), 0, nil)) putStorage(t, addr) - keys, err := CommittedStorageKeys(domains, tx, nil, addr) + keys, err := CommittedStorageKeys(domains, tx, addr) require.NoError(t, err) require.Equal(t, []accounts.StorageKey{accounts.InternKey(slot)}, keys) }) @@ -293,35 +292,17 @@ func TestCommittedStorageKeys(t *testing.T) { addr := accounts.InternAddress(common.HexToAddress("0xbb")) putStorage(t, addr) - keys, err := CommittedStorageKeys(domains, tx, nil, addr) + keys, err := CommittedStorageKeys(domains, tx, addr) require.NoError(t, err) require.Empty(t, keys, "walked the storage prefix for an address with no committed account") }) - t.Run("walks when the account was destroyed in this block", func(t *testing.T) { - // The self-destruct apply path leaves the storage prefix in the domain - // until the block-end flush when a block cache is active, so an absent - // account no longer implies absent storage. Skipping here loses the - // cascade the trie needs and the root goes wrong. - defer func(v bool) { dbg.AssertEnabled = v }(dbg.AssertEnabled) - dbg.AssertEnabled = false - addr := accounts.InternAddress(common.HexToAddress("0xdd")) - putStorage(t, addr) - cache := NewBlockStateCache() - cache.DeleteAccount(addr, 0) - - keys, err := CommittedStorageKeys(domains, tx, cache, addr) - require.NoError(t, err) - require.Equal(t, []accounts.StorageKey{accounts.InternKey(slot)}, keys, - "skipped the walk for an address destroyed earlier in this block") - }) - t.Run("skips silently when neither account nor storage is committed", func(t *testing.T) { defer func(v bool) { dbg.AssertEnabled = v }(dbg.AssertEnabled) dbg.AssertEnabled = true addr := accounts.InternAddress(common.HexToAddress("0xee")) - keys, err := CommittedStorageKeys(domains, tx, nil, addr) + keys, err := CommittedStorageKeys(domains, tx, addr) require.NoError(t, err) require.Empty(t, keys) }) @@ -332,60 +313,7 @@ func TestCommittedStorageKeys(t *testing.T) { addr := accounts.InternAddress(common.HexToAddress("0xcc")) putStorage(t, addr) - require.Panics(t, func() { _, _ = CommittedStorageKeys(domains, tx, nil, addr) }, + require.Panics(t, func() { _, _ = CommittedStorageKeys(domains, tx, addr) }, "the skip rests on this invariant, so breaking it must not stay silent") }) } - -// TestSelfDestructCascade_PerTxnHistory pins per-txn history granularity across a -// self-destruct. Erigon records history per transaction, so an RPC re-exec of a -// later transaction must see the destroyed contract's storage already gone while -// a read at the destroying txNum still sees it. Those history records exist only -// because the cascade enumerates the committed slots -- a walk that is skipped -// when it should not be leaves the deletes out of history, which a trie-root -// check cannot see since it only compares the block's final state. -func TestSelfDestructCascade_PerTxnHistory(t *testing.T) { - _, tx, domains := NewTestRwTx(t) - addr := accounts.InternAddress(common.HexToAddress("0x5d")) - slot := common.HexToHash("0x07") - av := addr.Value() - composite := append(append([]byte{}, av[:]...), slot[:]...) - - const preTxNum, sdTxNum = uint64(1), uint64(4) - - acc := accounts.Account{Nonce: 1, CodeHash: accounts.EmptyCodeHash} - require.NoError(t, domains.DomainPut(kv.AccountsDomain, tx, av[:], accounts.SerialiseV3(&acc), preTxNum, nil)) - require.NoError(t, domains.DomainPut(kv.StorageDomain, tx, composite, []byte{0xaa}, preTxNum, nil)) - - ws := &WriteSet{} - ws.SetSelfDestruct(addr, &VersionedWrite[bool]{ - WriteHeader: WriteHeader{Address: addr, Path: SelfDestructPath, Version: Version{TxIndex: 1}}, - Val: true, - }) - var domainKeysErr error - domainKeys := func(a accounts.Address) []accounts.StorageKey { - keys, err := CommittedStorageKeys(domains, tx, nil, a) - if err != nil { - domainKeysErr = err - return nil - } - return keys - } - out, err := ws.Normalize(NewVersionMap(nil), 1, 0, &minimalStateReader{}, domainKeys, false, false, false) - require.NoError(t, domainKeysErr) - require.NoError(t, err) - _, cascaded := out.GetStorage(addr, accounts.InternKey(slot)) - require.True(t, cascaded, "the cascade dropped the committed slot, so history never records its delete") - - require.NoError(t, out.Apply(domains, tx, 0, sdTxNum, nil, &chain.Rules{}, nil, false)) - // History is queried through the tx, so the batch has to reach it first. - require.NoError(t, domains.Flush(t.Context(), tx)) - - before, _, err := tx.GetAsOf(kv.StorageDomain, composite, sdTxNum) - require.NoError(t, err) - require.Equal(t, []byte{0xaa}, before, "history lost the slot value the destroying txn read") - - after, _, err := tx.GetAsOf(kv.StorageDomain, composite, sdTxNum+1) - require.NoError(t, err) - require.Empty(t, after, "a later txNum still sees the destroyed contract's storage") -} diff --git a/execution/tests/blockgen/chain_makers.go b/execution/tests/blockgen/chain_makers.go index b9c962cb6f9..144c88da6b6 100644 --- a/execution/tests/blockgen/chain_makers.go +++ b/execution/tests/blockgen/chain_makers.go @@ -576,7 +576,7 @@ func GenerateChain(config *chain.Config, parent *types.Block, engine rules.Engin blockNum := b.header.Number.Uint64() var domainKeysErr error domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(domains, tx, nil, addr) + keys, err := state.CommittedStorageKeys(domains, tx, addr) if err != nil { domainKeysErr = err return nil From ca5113d6bdf13d63b84acfbdef006e54f08fbeb3 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Fri, 28 Aug 2026 17:14:48 +0700 Subject: [PATCH 6/9] execution/state: resolve the storage assert the way the account probe does IteratePrefix reads sd.mem alone while GetLatest also resolves sd.parent, so an address tombstoned in the parent SD reads as having no account while the walk still returns its committed rows. Under ERIGON_ASSERT that tripped on a state that is valid. --- execution/state/rw_v3.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/execution/state/rw_v3.go b/execution/state/rw_v3.go index e17b96677c7..571dfd2831d 100644 --- a/execution/state/rw_v3.go +++ b/execution/state/rw_v3.go @@ -848,13 +848,29 @@ func assertNoCommittedStorage(domains *execctx.SharedDomains, roTx kv.TemporalTx if !dbg.AssertEnabled { return nil } + // IteratePrefix reads sd.mem alone, while the account probe resolves through + // sd.parent too, so a row this walk returns may already be tombstoned there. + // Re-read each hit the way the probe reads, or a parent-deleted address trips + // the assert on a state that is valid. found := 0 + var iterErr error if err := domains.IteratePrefix(kv.StorageDomain, addr, roTx, func(k, v []byte) (bool, error) { + cur, _, err := domains.GetLatest(kv.StorageDomain, roTx, k) + if err != nil { + iterErr = err + return false, err + } + if len(cur) == 0 { + return true, nil + } found++ return false, nil }); err != nil { return err } + if iterErr != nil { + return iterErr + } if found > 0 { panic(fmt.Sprintf("%s: %x has storage but no account", what, addr)) } From 469c86154e63befa61670b5581282fd398c05225 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Fri, 28 Aug 2026 17:31:11 +0700 Subject: [PATCH 7/9] execution/state: start the assertNoCommittedStorage doc sentence with a word --- execution/state/rw_v3.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/execution/state/rw_v3.go b/execution/state/rw_v3.go index 571dfd2831d..41c07586b7a 100644 --- a/execution/state/rw_v3.go +++ b/execution/state/rw_v3.go @@ -842,8 +842,9 @@ func hasCommittedAccount(domains *execctx.SharedDomains, roTx kv.TemporalTx, add // assertNoCommittedStorage panics when addr has committed storage but no // committed account, so a violation of the hasCommittedAccount invariant -// surfaces instead of silently skipping a storage wipe. what names the caller so -// a trip points at the right path. No-op unless asserts are enabled. +// surfaces instead of silently skipping a storage wipe. The what argument names +// the caller so a trip points at the right path. No-op unless asserts are +// enabled. func assertNoCommittedStorage(domains *execctx.SharedDomains, roTx kv.TemporalTx, addr []byte, what string) error { if !dbg.AssertEnabled { return nil From 306e4326bbd50204d061d31741d23369def3d820 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Fri, 28 Aug 2026 20:28:22 +0700 Subject: [PATCH 8/9] execution/state: resolve the storage assert after the walk, not inside it IteratePrefix holds the domain's RLock across its callback and GetLatest takes it again, so a writer queued between the two deadlocks the walk. Collect the keys first and re-read them once the lock is gone. --- execution/state/rw_v3.go | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/execution/state/rw_v3.go b/execution/state/rw_v3.go index 41c07586b7a..d62a95cf23d 100644 --- a/execution/state/rw_v3.go +++ b/execution/state/rw_v3.go @@ -849,31 +849,27 @@ func assertNoCommittedStorage(domains *execctx.SharedDomains, roTx kv.TemporalTx if !dbg.AssertEnabled { return nil } - // IteratePrefix reads sd.mem alone, while the account probe resolves through - // sd.parent too, so a row this walk returns may already be tombstoned there. - // Re-read each hit the way the probe reads, or a parent-deleted address trips - // the assert on a state that is valid. - found := 0 - var iterErr error + // IteratePrefix does not resolve through sd.parent, while the account probe + // does, so a row this walk returns may already be tombstoned there. Re-read + // each hit the way the probe reads, or a parent-deleted address trips the + // assert on a state that is valid. The re-reads happen after the walk: + // IteratePrefix holds the domain's RLock across the callback, and GetLatest + // takes it again, which a writer queued between the two turns into a deadlock. + var candidates [][]byte if err := domains.IteratePrefix(kv.StorageDomain, addr, roTx, func(k, v []byte) (bool, error) { + candidates = append(candidates, bytes.Clone(k)) + return true, nil + }); err != nil { + return err + } + for _, k := range candidates { cur, _, err := domains.GetLatest(kv.StorageDomain, roTx, k) if err != nil { - iterErr = err - return false, err + return err } - if len(cur) == 0 { - return true, nil + if len(cur) > 0 { + panic(fmt.Sprintf("%s: %x has storage but no account", what, addr)) } - found++ - return false, nil - }); err != nil { - return err - } - if iterErr != nil { - return iterErr - } - if found > 0 { - panic(fmt.Sprintf("%s: %x has storage but no account", what, addr)) } return nil } From 0ccc7f6826569b51021fa9cfcbe7e58e8d2fa803 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Fri, 28 Aug 2026 21:18:59 +0700 Subject: [PATCH 9/9] execution/state: let the storage-key lookup return its error Normalize took a callback that could not fail, so all four callers smuggled the error out through a captured variable and re-checked it afterwards. CommittedStorageKeysFn binds the only function production ever passes. --- execution/builder/exec.go | 13 +------ execution/stagedsync/exec3_parallel.go | 26 ++------------ execution/state/writeset_normalize.go | 41 ++++++++++++++++------ execution/state/writeset_normalize_test.go | 30 ++++++++++++++-- execution/tests/blockgen/chain_makers.go | 13 +------ 5 files changed, 62 insertions(+), 61 deletions(-) diff --git a/execution/builder/exec.go b/execution/builder/exec.go index 532d6c9805b..c0a4cf5f688 100644 --- a/execution/builder/exec.go +++ b/execution/builder/exec.go @@ -239,15 +239,7 @@ func execBlock(ctx context0.Context, sd *execctx.SharedDomains, tx kv.TemporalTx if ibs.IsVersioned() { blockCtx := protocol.NewEVMBlockContext(current.Header, protocol.GetHashFn(current.Header, nil), cfg.engine, accounts.NilAddress, cfg.chainConfig) blockRules := blockCtx.Rules(cfg.chainConfig) - var domainKeysErr error - domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(sd, tx, addr) - if err != nil { - domainKeysErr = err - return nil - } - return keys - } + domainStorageKeys := state.CommittedStorageKeysFn(sd, tx) emptyRemoval := blockHeight != 0 && cfg.chainConfig.IsEIP161Enabled(blockHeight) isAura := cfg.chainConfig.Aura != nil for i, ws := range ba.BalIO().Outputs() { @@ -255,9 +247,6 @@ func execBlock(ctx context0.Context, sd *execctx.SharedDomains, tx kv.TemporalTx continue } normalized, normErr := ws.Normalize(ibs.VersionMap(), i-1, 0, stateReader, domainStorageKeys, emptyRemoval, isAura, blockRules.IsAmsterdam) - if domainKeysErr != nil { - return fmt.Errorf("iterate storage prefix for block write normalization: %w", domainKeysErr) - } if normErr != nil { return fmt.Errorf("normalize block writes: %w", normErr) } diff --git a/execution/stagedsync/exec3_parallel.go b/execution/stagedsync/exec3_parallel.go index d7df98c2eba..c8c4003c1af 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -3070,21 +3070,10 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r // committed for addr (sd.mem + domain files), so a self-destruct // emits the full StoragePath=0 cascade — covers genesis-allocated // and prior-block storage that vm.StorageKeys doesn't see. - var domainKeysErr error - domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, addr) - if err != nil { - domainKeysErr = err - return nil - } - return keys - } + domainStorageKeys := state.CommittedStorageKeysFn(pe.rs.Domains(), applyTx) // Mirror txtask.go's genesis rules-clobber so empty allocs (AuRa ZeroAddress) survive. emptyRemoval := be.number() != 0 && pe.cfg.chainConfig.IsEIP161Enabled(be.number()) normWrites, normErr := rawWrites.Normalize(be.versionMap, txVersion.TxIndex, resultIncarnation, stateReader, domainStorageKeys, emptyRemoval, pe.cfg.chainConfig.Aura != nil, txTask.Rules().IsAmsterdam) - if domainKeysErr != nil { - return nil, fmt.Errorf("[parallel] iterate storage prefix for block write normalization: %w", domainKeysErr) - } if normErr != nil { return nil, fmt.Errorf("[parallel] normalize block writes: %w", normErr) } @@ -3334,21 +3323,10 @@ func (be *blockExecutor) nextResult(ctx context.Context, pe *parallelExecutor, r // so.data via MakeWriteSet. This keeps the parallel commit sourced // solely from versionedWrites so the write-path stateObject is // redundant. - var domainKeysErr error - domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(pe.rs.Domains(), applyTx, addr) - if err != nil { - domainKeysErr = err - return nil - } - return keys - } + domainStorageKeys := state.CommittedStorageKeysFn(pe.rs.Domains(), applyTx) emptyRemoval := be.number() != 0 && pe.cfg.chainConfig.IsEIP161Enabled(be.number()) var normErr error finalizeWrites, normErr = writes.Normalize(be.versionMap, finalVersion.TxIndex, finalVersion.Incarnation, reader, domainStorageKeys, emptyRemoval, pe.cfg.chainConfig.Aura != nil, pe.cfg.chainConfig.IsAmsterdam(tt.Header.Time)) - if domainKeysErr != nil { - return nil, fmt.Errorf("[parallel] finalize iterate storage prefix for block write normalization: %w", domainKeysErr) - } if normErr != nil { return nil, fmt.Errorf("[parallel] normalize finalize writes: %w", normErr) } diff --git a/execution/state/writeset_normalize.go b/execution/state/writeset_normalize.go index 2fdee176fa7..4872d40be8e 100644 --- a/execution/state/writeset_normalize.go +++ b/execution/state/writeset_normalize.go @@ -60,7 +60,7 @@ var codePathRecoveryHashMismatch = metrics.GetOrCreateCounter("exec3_codepath_re // from the trie (wrong root in TestDeleteRecreateAccount / TestSelfDestructReceive // / TestEIP161AccountRemoval, all of which SD a contract whose storage predates // the block). Pass nil in unit tests that don't exercise pre-block storage. -func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, stateReader StateReader, domainStorageKeys func(addr accounts.Address) []accounts.StorageKey, emptyRemoval bool, isAura bool, eip8246 bool) (*WriteSet, error) { +func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, stateReader StateReader, domainStorageKeys StorageKeysFn, emptyRemoval bool, isAura bool, eip8246 bool) (*WriteSet, error) { filtered := &WriteSet{} if writes == nil { return filtered, nil @@ -69,7 +69,7 @@ func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, // sdStorageSlots returns the union of vm.StorageKeys (this batch) and // domainStorageKeys (committed before this batch), deduped — the complete // set of storage slots that must be DELETE'd when addr self-destructs. - sdStorageSlots := func(addr accounts.Address) []accounts.StorageKey { + sdStorageSlots := func(addr accounts.Address) ([]accounts.StorageKey, error) { seen := make(map[accounts.StorageKey]struct{}) var out []accounts.StorageKey for _, k := range vm.StorageKeys(addr) { @@ -78,15 +78,20 @@ func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, out = append(out, k) } } - if domainStorageKeys != nil { - for _, k := range domainStorageKeys(addr) { - if _, ok := seen[k]; !ok { - seen[k] = struct{}{} - out = append(out, k) - } + if domainStorageKeys == nil { + return out, nil + } + committed, err := domainStorageKeys(addr) + if err != nil { + return nil, err + } + for _, k := range committed { + if _, ok := seen[k]; !ok { + seen[k] = struct{}{} + out = append(out, k) } } - return out + return out, nil } // Pre-scan for SD'd addresses. IBS.Selfdestruct emits 3 writes for the @@ -257,7 +262,11 @@ func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, continue } filtered.SetSelfDestruct(h.Address, sdw) - for _, slot := range sdStorageSlots(h.Address) { + slots, err := sdStorageSlots(h.Address) + if err != nil { + return nil, err + } + for _, slot := range slots { filtered.SetStorage(h.Address, slot, &VersionedWrite[uint256.Int]{ WriteHeader: WriteHeader{ Address: h.Address, @@ -492,6 +501,18 @@ func (writes *WriteSet) Normalize(vm *VersionMap, txIndex int, incarnation int, return filtered, nil } +// StorageKeysFn enumerates the storage slots committed for an address. Normalize +// takes one rather than the domains directly so a test can inject a fixed set. +type StorageKeysFn func(addr accounts.Address) ([]accounts.StorageKey, error) + +// CommittedStorageKeysFn binds CommittedStorageKeys to a domains/tx pair, which +// is all production ever passes to Normalize. +func CommittedStorageKeysFn(domains *execctx.SharedDomains, tx kv.TemporalTx) StorageKeysFn { + return func(addr accounts.Address) ([]accounts.StorageKey, error) { + return CommittedStorageKeys(domains, tx, addr) + } +} + // CommittedStorageKeys returns every storage slot committed for addr, the // domainStorageKeys input Normalize needs to emit a full self-destruct cascade. // The prefix walk is skipped for an address with no committed account -- see diff --git a/execution/state/writeset_normalize_test.go b/execution/state/writeset_normalize_test.go index e4dd2eeea92..968e3dc7f85 100644 --- a/execution/state/writeset_normalize_test.go +++ b/execution/state/writeset_normalize_test.go @@ -84,11 +84,11 @@ func TestNormalize_SelfDestructDeletesVmAndDomainStorageSlots(t *testing.T) { kDomain := accounts.InternKey(common.HexToHash("0x02")) // pre-block, in domain only vm := NewVersionMap(nil) vm.WriteStorage(addr, kVM, Version{TxIndex: 0}, *uint256.NewInt(9), true) - domainKeys := func(a accounts.Address) []accounts.StorageKey { + domainKeys := func(a accounts.Address) ([]accounts.StorageKey, error) { if a == addr { - return []accounts.StorageKey{kDomain} + return []accounts.StorageKey{kDomain}, nil } - return nil + return nil, nil } ws := &WriteSet{} @@ -317,3 +317,27 @@ func TestCommittedStorageKeys(t *testing.T) { "the skip rests on this invariant, so breaking it must not stay silent") }) } + +// A storage-key lookup failure must reach the caller as an error, not be +// swallowed into an empty slot list that silently drops the self-destruct +// cascade. +func TestNormalizeReturnsStorageKeysError(t *testing.T) { + addr := accounts.InternAddress(common.HexToAddress("0xbeef")) + want := errors.New("domain read failed") + vm := NewVersionMap(nil) + + ws := &WriteSet{} + ws.SetSelfDestruct(addr, &VersionedWrite[bool]{ + WriteHeader: WriteHeader{Address: addr, Path: SelfDestructPath, Version: Version{TxIndex: 1}}, + Val: true, + }) + ws.SetBalance(addr, &VersionedWrite[uint256.Int]{ + WriteHeader: WriteHeader{Address: addr, Path: BalancePath, Version: Version{TxIndex: 1}}, + Val: *uint256.NewInt(0), + }) + + _, err := ws.Normalize(vm, 1, 0, &minimalStateReader{}, func(accounts.Address) ([]accounts.StorageKey, error) { + return nil, want + }, false, false, false) + require.ErrorIs(t, err, want) +} diff --git a/execution/tests/blockgen/chain_makers.go b/execution/tests/blockgen/chain_makers.go index 00f728ff520..f27058f8aa2 100644 --- a/execution/tests/blockgen/chain_makers.go +++ b/execution/tests/blockgen/chain_makers.go @@ -572,15 +572,7 @@ func GenerateChain(config *chain.Config, parent *types.Block, engine rules.Engin // finalize) is applied in order; applying a phase before normalizing // the next lets the next phase's stateReader fallback see it. blockNum := b.header.Number.Uint64() - var domainKeysErr error - domainStorageKeys := func(addr accounts.Address) []accounts.StorageKey { - keys, err := state.CommittedStorageKeys(domains, tx, addr) - if err != nil { - domainKeysErr = err - return nil - } - return keys - } + domainStorageKeys := state.CommittedStorageKeysFn(domains, tx) emptyRemoval := blockNum != 0 && config.IsEIP161Enabled(blockNum) isAura := config.Aura != nil for i, ws := range b.blockIO.Outputs() { @@ -588,9 +580,6 @@ func GenerateChain(config *chain.Config, parent *types.Block, engine rules.Engin continue } normalized, normErr := ws.Normalize(b.versionMap, i-1, 0, stateReader, domainStorageKeys, emptyRemoval, isAura, config.IsAmsterdam(b.header.Time)) - if domainKeysErr != nil { - return nil, nil, fmt.Errorf("iterate storage prefix for block write normalization: %w", domainKeysErr) - } if normErr != nil { return nil, nil, fmt.Errorf("normalize block writes: %w", normErr) }