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>
2026-04-11 04:17:30 +00:00 · 2026-04-10 19:23:44 +02:00 · 2026-04-10 19:23:44 +02:00 · f71a884e37
commit f71a884e37
parent ea5448814f
2 changed files with 349 additions and 5 deletions
--- 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
 }

--- 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()