diff --git a/execution/builder/exec.go b/execution/builder/exec.go index 515a88d1799..c0a4cf5f688 100644 --- a/execution/builder/exec.go +++ b/execution/builder/exec.go @@ -239,22 +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 { - 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 - 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() { @@ -262,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 74d5e83fb73..c8c4003c1af 100644 --- a/execution/stagedsync/exec3_parallel.go +++ b/execution/stagedsync/exec3_parallel.go @@ -3070,28 +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 { - 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 - 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) } @@ -3341,28 +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 { - 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 - 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/rw_v3.go b/execution/state/rw_v3.go index b9ad00c3d67..d62a95cf23d 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,22 +826,50 @@ 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. 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 } - found := 0 + // 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) { - found++ - return false, nil + candidates = append(candidates, bytes.Clone(k)) + return true, nil }); err != nil { return err } - if found > 0 { - panic(fmt.Sprintf("createContract: %x has storage but no account", addr)) + for _, k := range candidates { + cur, _, err := domains.GetLatest(kv.StorageDomain, roTx, k) + if err != nil { + return err + } + if len(cur) > 0 { + panic(fmt.Sprintf("%s: %x has storage but no account", what, addr)) + } } return nil } diff --git a/execution/state/writeset_normalize.go b/execution/state/writeset_normalize.go index 534b4e8ee34..4872d40be8e 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" @@ -57,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 @@ -66,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) { @@ -75,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 @@ -254,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, @@ -488,3 +500,41 @@ 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 +// hasCommittedAccount for why that probe is worth its own read. +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 + } + if !hasAcc { + return nil, assertNoCommittedStorage(domains, tx, av[:], "selfDestruct") + } + 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..968e3dc7f85 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" ) @@ -82,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{} @@ -255,3 +257,87 @@ 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. 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("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, 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 + 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") + }) +} + +// 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 c925cea4c08..f27058f8aa2 100644 --- a/execution/tests/blockgen/chain_makers.go +++ b/execution/tests/blockgen/chain_makers.go @@ -572,22 +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 { - 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 - 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() { @@ -595,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) }