trie/bintrie: fix DeleteAccount no-op

CPerezz · CPerezz · commit c1c65b9a56fe · 2026-04-07T16:57:30.000+02:00
BinaryTrie.DeleteAccount was a no-op, silently ignoring the caller's
deletion request and leaving the old BasicData and CodeHash in the trie.
The GetAccount deletion-detection branch (trie.go:219) already expected
a tombstone convention — "BasicData and CodeHash are 32-byte zero blobs
AND a non-nil 32-byte sentinel is present at a reserved offset" — but
nothing was writing that sentinel, so the check was effectively dead
code.

Implement the deletion as an InsertValuesAtStem that:

  - writes a 32-byte zero blob to BasicData (offset 0)
  - writes a 32-byte zero blob to CodeHash (offset 1)
  - writes a 32-byte zero blob to a deletion sentinel offset in the
    EIP-7864 reserved range (offset 10, promoted to the named constant
    accountDeletedMarkerKey for cross-referencing with GetAccount)

This matches the bintrie's existing "write zeros to delete" convention
seen in DeleteStorage, keeps GetAccount's deletion branch consistent,
and still distinguishes "deleted" from "never existed" (the latter has
all-nil slots so the empty-account check fires first).

Storage slots and code chunks are intentionally left untouched. Wiping
storage on self-destruct is a separate concern handled at the StateDB
level — the bintrie's unified keyspace has no cheap way to enumerate
every slot of a given account, so a blanket wipe is not possible here.

Regression tests cover:

  - round-trip: UpdateAccount -&gt; GetAccount -&gt; DeleteAccount -&gt; GetAccount nil
  - delete on missing account: no panic, subsequent read still nil
  - unrelated accounts at different stems are preserved
  - delete + recreate: second read sees the new values, not the old ones
  - main storage slots at different stems survive DeleteAccount
diff --git a/trie/bintrie/trie.go b/trie/bintrie/trie.go
@@ -216,10 +216,15 @@ 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) and the accountDeletedMarkerKey sentinel
+	// will be a non-empty 32-byte zero blob written by DeleteAccount. If the
+	// account has been recreated since, UpdateAccount will have overwritten
+	// BasicData and CodeHash with non-zero values, so this branch won't hit.
+	if bytes.Equal(values[BasicDataLeafKey], zero[:]) &&
+		len(values) > accountDeletedMarkerKey &&
+		len(values[accountDeletedMarkerKey]) > 0 &&
+		bytes.Equal(values[CodeHashLeafKey], zero[:]) {
 		return nil, nil
 	}
 
@@ -294,11 +299,47 @@ 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 removes the account metadata (basic data and code hash) for
+// the given address from the trie.
+//
+// Binary trie leaves cannot be "physically" removed — there is no delete
+// primitive on StemNode. Instead, the bintrie uses a tombstone convention:
+// BasicData (offset 0) and CodeHash (offset 1) are overwritten with 32 zero
+// bytes, and a non-nil 32-byte sentinel is written at the deletion marker
+// offset that GetAccount consults. This matches the detection logic at
+// GetAccount around trie.go:219 which treats "BasicData and CodeHash are
+// zero AND the sentinel is present" as an absent account while still
+// distinguishing it from "never existed" (all-nil slots).
+//
+// Storage slots and code chunks are NOT touched by this function. If the
+// caller needs to wipe storage on self-destruct, it must walk the relevant
+// slots explicitly — bintrie's unified keyspace has no cheap way to
+// enumerate every slot of a given account.
 func (t *BinaryTrie) DeleteAccount(addr common.Address) error {
-	return nil
+	var (
+		err      error
+		zeroBlob [HashSize]byte
+		values   = make([][]byte, StemNodeWidth)
+		stem     = GetBinaryTreeKey(addr, zero[:])
+	)
+	// Clear BasicData (nonce, balance, code size) and CodeHash.
+	values[BasicDataLeafKey] = zeroBlob[:]
+	values[CodeHashLeafKey] = zeroBlob[:]
+	// Write a non-nil 32-byte sentinel at the deletion marker offset so
+	// GetAccount can tell "deleted" apart from "never existed" on a
+	// subsequent read. See GetAccount's deletion branch around trie.go:219.
+	values[accountDeletedMarkerKey] = zeroBlob[:]
+
+	t.root, err = t.root.InsertValuesAtStem(stem, values, t.nodeResolver, 0)
+	return err
 }
 
+// accountDeletedMarkerKey is the stem offset used as a "this account was
+// deleted" sentinel. It lives in the EIP-7864 reserved range (offsets 2-63)
+// and is consulted only by GetAccount's deletion-detection branch. Keep this
+// in sync with the check in GetAccount (trie.go:219).
+const accountDeletedMarkerKey = 10
+
 // DeleteStorage removes any existing value for key from the trie. If a node was not
 // found in the database, a trie.MissingNodeError is returned.
 func (t *BinaryTrie) DeleteStorage(addr common.Address, key []byte) error {
diff --git a/trie/bintrie/trie_test.go b/trie/bintrie/trie_test.go
@@ -267,6 +267,225 @@ func TestStorageRoundTrip(t *testing.T) {
 	}
 }
 
+// newTestTrie creates a fresh BinaryTrie with an empty root and a default
+// prevalue tracer. Used by tests that need a realistic trie instance.
+func newTestTrie(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 := newTestTrie(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 := newTestTrie(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 := newTestTrie(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 := newTestTrie(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
+// (which live at different stems) untouched. Wiping storage on self-destruct
+// is a separate concern handled at the StateDB level.
+func TestDeleteAccountDoesNotAffectMainStorage(t *testing.T) {
+	tr := newTestTrie(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.
+	if got, _ := tr.GetAccount(addr); 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.
+	got, err := tr.GetStorage(addr, slot[:])
+	if err != nil {
+		t.Fatalf("GetStorage after DeleteAccount: %v", err)
+	}
+	if len(got) == 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(got, expected[:]) {
+		t.Fatalf("main storage slot: got %x, want %x", got, expected)
+	}
+}
+
 func TestBinaryTrieWitness(t *testing.T) {
 	tracer := trie.NewPrevalueTracer()
 
