From f71a884e37857e6cbb093e2fc3e03b33bda85d6f Mon Sep 17 00:00:00 2001 From: CPerezz <37264926+CPerezz@users.noreply.github.com> Date: Fri, 10 Apr 2026 19:23:44 +0200 Subject: [PATCH] trie/bintrie: fix DeleteAccount no-op (#34676) `BinaryTrie.DeleteAccount` was a no-op, silently ignoring the caller's deletion request and leaving the old `BasicData` and `CodeHash` in the trie. Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com> --- trie/bintrie/trie.go | 26 ++- trie/bintrie/trie_test.go | 328 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 349 insertions(+), 5 deletions(-) diff --git a/trie/bintrie/trie.go b/trie/bintrie/trie.go index 6c29239a87..45f9edd46c 100644 --- a/trie/bintrie/trie.go +++ b/trie/bintrie/trie.go @@ -216,10 +216,12 @@ func (t *BinaryTrie) GetAccount(addr common.Address) (*types.StateAccount, error return nil, nil } - // If the account has been deleted, then values[10] will be 0 and not nil. If it has - // been recreated after that, then its code keccak will NOT be 0. So return `nil` if - // the nonce, and values[10], and code keccak is 0. - if bytes.Equal(values[BasicDataLeafKey], zero[:]) && len(values) > 10 && len(values[10]) > 0 && bytes.Equal(values[CodeHashLeafKey], zero[:]) { + // If the account has been deleted, BasicData and CodeHash will both be + // 32-byte zero blobs (not nil). If the account is recreated afterwards, + // UpdateAccount overwrites BasicData and CodeHash with non-zero values, + // so this branch won't activate.. + if bytes.Equal(values[BasicDataLeafKey], zero[:]) && + bytes.Equal(values[CodeHashLeafKey], zero[:]) { return nil, nil } @@ -294,8 +296,22 @@ func (t *BinaryTrie) UpdateStorage(address common.Address, key, value []byte) er return nil } -// DeleteAccount is a no-op as it is disabled in stateless. +// DeleteAccount erases an account by overwriting the account +// descriptors with 0s. func (t *BinaryTrie) DeleteAccount(addr common.Address) error { + var ( + values = make([][]byte, StemNodeWidth) + stem = GetBinaryTreeKey(addr, zero[:]) + ) + // Clear BasicData (nonce, balance, code size) and CodeHash. + values[BasicDataLeafKey] = zero[:] + values[CodeHashLeafKey] = zero[:] + + root, err := t.root.InsertValuesAtStem(stem, values, t.nodeResolver, 0) + if err != nil { + return fmt.Errorf("DeleteAccount (%x) error: %v", addr, err) + } + t.root = root return nil } diff --git a/trie/bintrie/trie_test.go b/trie/bintrie/trie_test.go index 256fd218e2..f420f53ef8 100644 --- a/trie/bintrie/trie_test.go +++ b/trie/bintrie/trie_test.go @@ -267,6 +267,334 @@ func TestStorageRoundTrip(t *testing.T) { } } +// newEmptyTestTrie creates a fresh BinaryTrie with an empty root and a +// default prevalue tracer. Use this for tests that populate the trie +// incrementally via Update*; for tests that want a pre-populated trie with +// a fixed entry set, use makeTrie (in iterator_test.go) instead. +func newEmptyTestTrie(t *testing.T) *BinaryTrie { + t.Helper() + return &BinaryTrie{ + root: NewBinaryNode(), + tracer: trie.NewPrevalueTracer(), + } +} + +// makeAccount constructs a StateAccount with the given fields. The Root is +// zeroed out because the bintrie has no per-account storage root. +func makeAccount(nonce uint64, balance uint64, codeHash common.Hash) *types.StateAccount { + return &types.StateAccount{ + Nonce: nonce, + Balance: uint256.NewInt(balance), + CodeHash: codeHash.Bytes(), + } +} + +// TestDeleteAccountRoundTrip verifies the basic delete path: create an +// account, read it back, delete it, confirm subsequent reads return nil. +// Regression test for the no-op DeleteAccount bug where the deletion was +// silently ignored and the old values remained in the trie. +func TestDeleteAccountRoundTrip(t *testing.T) { + tr := newEmptyTestTrie(t) + addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") + codeHash := common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470") + + // Create: write account, verify round-trip. + acc := makeAccount(42, 1000, codeHash) + if err := tr.UpdateAccount(addr, acc, 0); err != nil { + t.Fatalf("UpdateAccount: %v", err) + } + got, err := tr.GetAccount(addr) + if err != nil { + t.Fatalf("GetAccount: %v", err) + } + if got == nil { + t.Fatal("GetAccount returned nil after UpdateAccount") + } + if got.Nonce != 42 { + t.Fatalf("Nonce: got %d, want 42", got.Nonce) + } + if got.Balance.Uint64() != 1000 { + t.Fatalf("Balance: got %s, want 1000", got.Balance) + } + if !bytes.Equal(got.CodeHash, codeHash[:]) { + t.Fatalf("CodeHash: got %x, want %x", got.CodeHash, codeHash) + } + + // Delete: verify GetAccount returns nil afterwards. + if err := tr.DeleteAccount(addr); err != nil { + t.Fatalf("DeleteAccount: %v", err) + } + got, err = tr.GetAccount(addr) + if err != nil { + t.Fatalf("GetAccount after delete: %v", err) + } + if got != nil { + t.Fatalf("GetAccount after delete: got %+v, want nil", got) + } +} + +// TestDeleteAccountOnMissingAccount verifies that deleting an account that +// was never created does not error and subsequent reads still return nil. +func TestDeleteAccountOnMissingAccount(t *testing.T) { + tr := newEmptyTestTrie(t) + addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") + + // Delete without any prior create. Should not panic or error on an + // empty root, and GetAccount should still return nil. + if err := tr.DeleteAccount(addr); err != nil { + t.Fatalf("DeleteAccount on empty trie: %v", err) + } + got, err := tr.GetAccount(addr) + if err != nil { + t.Fatalf("GetAccount after delete on empty trie: %v", err) + } + if got != nil { + t.Fatalf("GetAccount on deleted missing account: got %+v, want nil", got) + } +} + +// TestDeleteAccountPreservesOtherAccounts verifies that deleting one account +// does not affect accounts at different stems. +func TestDeleteAccountPreservesOtherAccounts(t *testing.T) { + tr := newEmptyTestTrie(t) + addrA := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") + addrB := common.HexToAddress("0xabcdef1234567890abcdef1234567890abcdef12") + codeHashA := common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470") + codeHashB := common.HexToHash("f0f1f2f3f4f5f6f7f8f9fafbfcfdfeff0102030405060708090a0b0c0d0e0f10") + + // Create two distinct accounts. + if err := tr.UpdateAccount(addrA, makeAccount(1, 100, codeHashA), 0); err != nil { + t.Fatalf("UpdateAccount(A): %v", err) + } + if err := tr.UpdateAccount(addrB, makeAccount(2, 200, codeHashB), 0); err != nil { + t.Fatalf("UpdateAccount(B): %v", err) + } + + // Delete A. + if err := tr.DeleteAccount(addrA); err != nil { + t.Fatalf("DeleteAccount(A): %v", err) + } + + // A should be gone. + if got, err := tr.GetAccount(addrA); err != nil { + t.Fatalf("GetAccount(A): %v", err) + } else if got != nil { + t.Fatalf("GetAccount(A) after delete: got %+v, want nil", got) + } + + // B should still be readable with its original values. + got, err := tr.GetAccount(addrB) + if err != nil { + t.Fatalf("GetAccount(B): %v", err) + } + if got == nil { + t.Fatal("GetAccount(B) returned nil after unrelated delete") + } + if got.Nonce != 2 { + t.Fatalf("Account B Nonce: got %d, want 2", got.Nonce) + } + if got.Balance.Uint64() != 200 { + t.Fatalf("Account B Balance: got %s, want 200", got.Balance) + } + if !bytes.Equal(got.CodeHash, codeHashB[:]) { + t.Fatalf("Account B CodeHash: got %x, want %x", got.CodeHash, codeHashB) + } +} + +// TestDeleteAccountThenRecreate verifies that an account can be deleted and +// then recreated with different values; the second read must return the new +// values, not the stale ones from before deletion. +func TestDeleteAccountThenRecreate(t *testing.T) { + tr := newEmptyTestTrie(t) + addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") + codeHash1 := common.HexToHash("1111111111111111111111111111111111111111111111111111111111111111") + codeHash2 := common.HexToHash("2222222222222222222222222222222222222222222222222222222222222222") + + // Create. + if err := tr.UpdateAccount(addr, makeAccount(1, 100, codeHash1), 0); err != nil { + t.Fatalf("UpdateAccount #1: %v", err) + } + // Delete. + if err := tr.DeleteAccount(addr); err != nil { + t.Fatalf("DeleteAccount: %v", err) + } + // Recreate with new values. + if err := tr.UpdateAccount(addr, makeAccount(7, 9999, codeHash2), 0); err != nil { + t.Fatalf("UpdateAccount #2: %v", err) + } + // Read: must observe the new values, not the originals. + got, err := tr.GetAccount(addr) + if err != nil { + t.Fatalf("GetAccount: %v", err) + } + if got == nil { + t.Fatal("GetAccount returned nil after recreate") + } + if got.Nonce != 7 { + t.Fatalf("Nonce: got %d, want 7", got.Nonce) + } + if got.Balance.Uint64() != 9999 { + t.Fatalf("Balance: got %s, want 9999", got.Balance) + } + if !bytes.Equal(got.CodeHash, codeHash2[:]) { + t.Fatalf("CodeHash: got %x, want %x", got.CodeHash, codeHash2) + } +} + +// TestDeleteAccountDoesNotAffectMainStorage verifies that DeleteAccount only +// clears the account's BasicData and CodeHash, leaving main storage slots +// untouched. Main storage slots live at different stems entirely (their +// keys route through the non-header branch in GetBinaryTreeKeyStorageSlot), +// so this test exercises the inter-stem isolation. Header-range storage +// slots share the same stem and are covered separately by +// TestDeleteAccountPreservesHeaderStorage. +// +// Wiping storage on self-destruct is a separate concern handled at the +// StateDB level. +func TestDeleteAccountDoesNotAffectMainStorage(t *testing.T) { + tr := newEmptyTestTrie(t) + addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") + codeHash := common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470") + + // Create account. + if err := tr.UpdateAccount(addr, makeAccount(1, 100, codeHash), 0); err != nil { + t.Fatalf("UpdateAccount: %v", err) + } + // Write a main storage slot — i.e. key[31] >= 64 or key[:31] != 0 — so + // it lives at a different stem from the account header. + slot := common.HexToHash("0000000000000000000000000000000000000000000000000000000000000080") + value := common.TrimLeftZeroes(common.HexToHash("00000000000000000000000000000000000000000000000000000000deadbeef").Bytes()) + if err := tr.UpdateStorage(addr, slot[:], value); err != nil { + t.Fatalf("UpdateStorage: %v", err) + } + + // Delete the account. + if err := tr.DeleteAccount(addr); err != nil { + t.Fatalf("DeleteAccount: %v", err) + } + + // Account should be absent. + got, err := tr.GetAccount(addr) + if err != nil { + t.Fatalf("GetAccount after delete: %v", err) + } + if got != nil { + t.Fatalf("GetAccount after delete: got %+v, want nil", got) + } + + // Main storage slot should still be readable — DeleteAccount must not + // have touched it. + stored, err := tr.GetStorage(addr, slot[:]) + if err != nil { + t.Fatalf("GetStorage after DeleteAccount: %v", err) + } + if len(stored) == 0 { + t.Fatal("main storage slot was wiped by DeleteAccount, expected it to survive") + } + var expected [HashSize]byte + copy(expected[HashSize-len(value):], value) + if !bytes.Equal(stored, expected[:]) { + t.Fatalf("main storage slot: got %x, want %x", stored, expected) + } +} + +// TestDeleteAccountPreservesHeaderStorage verifies that DeleteAccount does +// not clobber header-range storage slots (key[31] < 64), which live at the +// SAME stem as BasicData/CodeHash but at offsets 64-127. The safety here +// relies on StemNode.InsertValuesAtStem treating nil entries in the values +// slice as "do not overwrite"; this test pins that invariant so a future +// change cannot silently corrupt slots 0-63 of any contract. +func TestDeleteAccountPreservesHeaderStorage(t *testing.T) { + tr := newEmptyTestTrie(t) + addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") + codeHash := common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470") + + // Create account. + if err := tr.UpdateAccount(addr, makeAccount(1, 100, codeHash), 0); err != nil { + t.Fatalf("UpdateAccount: %v", err) + } + + // Create a second, unrelated account so the root promotes from StemNode + // to InternalNode. BinaryTrie.GetStorage walks via root.Get, which is + // only implemented on InternalNode/Empty — calling it with a StemNode + // root panics. The existing main-storage test gets away with this because + // the main-storage slot lands on a separate stem and forces the same + // promotion implicitly; here we want a same-stem header slot, so the + // promotion has to come from a second account. + other := common.HexToAddress("0xabcdef1234567890abcdef1234567890abcdef12") + if err := tr.UpdateAccount(other, makeAccount(0, 0, common.Hash{}), 0); err != nil { + t.Fatalf("UpdateAccount(other): %v", err) + } + + // Write a header-range storage slot — key[:31] == 0 and key[31] < 64 + // — which routes through the header branch in GetBinaryTreeKeyStorageSlot + // and lands on the same stem as BasicData/CodeHash. + var slot [HashSize]byte + slot[31] = 5 + value := []byte{0xde, 0xad, 0xbe, 0xef} + if err := tr.UpdateStorage(addr, slot[:], value); err != nil { + t.Fatalf("UpdateStorage: %v", err) + } + + // Delete the account. + if err := tr.DeleteAccount(addr); err != nil { + t.Fatalf("DeleteAccount: %v", err) + } + + // Account metadata should be gone. + got, err := tr.GetAccount(addr) + if err != nil { + t.Fatalf("GetAccount after delete: %v", err) + } + if got != nil { + t.Fatalf("GetAccount after delete: got %+v, want nil", got) + } + + // Header storage slot must survive — DeleteAccount only writes offsets + // BasicDataLeafKey, CodeHashLeafKey, and accountDeletedMarkerKey, leaving + // the header-storage offsets (64-127) untouched. + stored, err := tr.GetStorage(addr, slot[:]) + if err != nil { + t.Fatalf("GetStorage after DeleteAccount: %v", err) + } + if len(stored) == 0 { + t.Fatal("header storage slot was wiped by DeleteAccount, expected it to survive") + } + var expected [HashSize]byte + copy(expected[HashSize-len(value):], value) + if !bytes.Equal(stored, expected[:]) { + t.Fatalf("header storage slot: got %x, want %x", stored, expected) + } +} + +func TestDeleteAccountHashIsDeterministic(t *testing.T) { + addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") + codeHash := common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470") + acc := makeAccount(42, 1000, codeHash) + + run := func() common.Hash { + tr := newEmptyTestTrie(t) + if err := tr.UpdateAccount(addr, acc, 0); err != nil { + t.Fatalf("UpdateAccount: %v", err) + } + if err := tr.DeleteAccount(addr); err != nil { + t.Fatalf("DeleteAccount: %v", err) + } + return tr.Hash() + } + + first := run() + second := run() + if first != second { + t.Fatalf("non-deterministic root after Update+Delete: first=%x second=%x", first, second) + } + + empty := newEmptyTestTrie(t).Hash() + if first == empty { + t.Fatalf("post-delete root unexpectedly equals empty-trie root %x", empty) + } +} + func TestBinaryTrieWitness(t *testing.T) { tracer := trie.NewPrevalueTracer()