From 0835ce4bb57854e34fc2d16e44e70b7bbf4db3c3 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Wed, 8 Apr 2026 12:50:00 +0200 Subject: [PATCH] trie/bintrie: tighten DeleteAccount docs and helpers Follow-ups from PR review, bundled together because they are all non-functional documentation and helper polish: - Move accountDeletedMarkerKey to key_encoding.go alongside the other leaf-key constants, and drop the stale "Keep this in sync" directive. After this PR both GetAccount and DeleteAccount reference the constant by name, so there is nothing left to manually keep in sync. - Replace the hard-coded "trie.go:219" line references in the DeleteAccount doc and body comments with function-name references ("GetAccount's deletion-detection branch"). Line references rot on any edit above the target line. - Clarify what protects header storage from DeleteAccount: it shares the same stem as BasicData/CodeHash, so the safety comes from non-colliding offsets plus the nil-means-"do not overwrite" semantics of StemNode.InsertValuesAtStem, not from living at a different stem. Mirror the clarification in the TestDeleteAccountDoesNotAffectMainStorage comment and cross-reference the header-storage test. - Rename newTestTrie to newEmptyTestTrie so readers can pick between "empty" (this helper) and "pre-populated with entries" (makeTrie in iterator_test.go) without guessing. --- trie/bintrie/key_encoding.go | 7 +++++++ trie/bintrie/trie.go | 32 ++++++++++++++++---------------- trie/bintrie/trie_test.go | 34 +++++++++++++++++++++------------- 3 files changed, 44 insertions(+), 29 deletions(-) diff --git a/trie/bintrie/key_encoding.go b/trie/bintrie/key_encoding.go index c009f1529f..dcb279e8e2 100644 --- a/trie/bintrie/key_encoding.go +++ b/trie/bintrie/key_encoding.go @@ -31,6 +31,13 @@ const ( BasicDataBalanceOffset = 16 ) +// 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 written by DeleteAccount and read by GetAccount's deletion- +// detection branch. Both sides reference this constant directly, so there +// is no magic number to keep in sync. +const accountDeletedMarkerKey = 10 + var ( zeroInt = uint256.NewInt(0) zeroHash = common.Hash{} diff --git a/trie/bintrie/trie.go b/trie/bintrie/trie.go index be9f3d47ab..4c37576fc0 100644 --- a/trie/bintrie/trie.go +++ b/trie/bintrie/trie.go @@ -305,16 +305,22 @@ func (t *BinaryTrie) UpdateStorage(address common.Address, key, value []byte) er // 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). +// bytes, and a non-nil 32-byte sentinel is written at accountDeletedMarkerKey. +// This matches GetAccount's deletion-detection branch 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. +// Storage slots and code chunks are NOT touched. Header storage (slot +// numbers 0-63) shares the same stem as BasicData/CodeHash but lives at +// offsets 64-127; this function writes only BasicDataLeafKey, CodeHashLeafKey, +// and accountDeletedMarkerKey, so header storage survives by relying on +// StemNode.InsertValuesAtStem treating nil entries in the values slice as +// "do not overwrite". Main storage and code chunks live at different stems +// entirely. +// +// 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 { var ( values = make([][]byte, StemNodeWidth) @@ -325,7 +331,7 @@ func (t *BinaryTrie) DeleteAccount(addr common.Address) error { values[CodeHashLeafKey] = zero[:] // 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. + // subsequent read. See GetAccount's deletion-detection branch. values[accountDeletedMarkerKey] = zero[:] root, err := t.root.InsertValuesAtStem(stem, values, t.nodeResolver, 0) @@ -336,12 +342,6 @@ func (t *BinaryTrie) DeleteAccount(addr common.Address) error { return nil } -// 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 index de04bd7949..29ae364524 100644 --- a/trie/bintrie/trie_test.go +++ b/trie/bintrie/trie_test.go @@ -267,9 +267,11 @@ 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 { +// 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(), @@ -292,7 +294,7 @@ func makeAccount(nonce uint64, balance uint64, codeHash common.Hash) *types.Stat // 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) + tr := newEmptyTestTrie(t) addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") codeHash := common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470") @@ -334,7 +336,7 @@ func TestDeleteAccountRoundTrip(t *testing.T) { // 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) + tr := newEmptyTestTrie(t) addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") // Delete without any prior create. Should not panic or error on an @@ -354,7 +356,7 @@ func TestDeleteAccountOnMissingAccount(t *testing.T) { // TestDeleteAccountPreservesOtherAccounts verifies that deleting one account // does not affect accounts at different stems. func TestDeleteAccountPreservesOtherAccounts(t *testing.T) { - tr := newTestTrie(t) + tr := newEmptyTestTrie(t) addrA := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") addrB := common.HexToAddress("0xabcdef1234567890abcdef1234567890abcdef12") codeHashA := common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470") @@ -403,7 +405,7 @@ func TestDeleteAccountPreservesOtherAccounts(t *testing.T) { // 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) + tr := newEmptyTestTrie(t) addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") codeHash1 := common.HexToHash("1111111111111111111111111111111111111111111111111111111111111111") codeHash2 := common.HexToHash("2222222222222222222222222222222222222222222222222222222222222222") @@ -441,10 +443,16 @@ func TestDeleteAccountThenRecreate(t *testing.T) { // 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. +// 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 := newTestTrie(t) + tr := newEmptyTestTrie(t) addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") codeHash := common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470") @@ -497,7 +505,7 @@ func TestDeleteAccountDoesNotAffectMainStorage(t *testing.T) { // 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 := newTestTrie(t) + tr := newEmptyTestTrie(t) addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") codeHash := common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470") @@ -575,7 +583,7 @@ func TestDeleteAccountHashIsDeterministic(t *testing.T) { acc := makeAccount(42, 1000, codeHash) run := func() common.Hash { - tr := newTestTrie(t) + tr := newEmptyTestTrie(t) if err := tr.UpdateAccount(addr, acc, 0); err != nil { t.Fatalf("UpdateAccount: %v", err) } @@ -595,7 +603,7 @@ func TestDeleteAccountHashIsDeterministic(t *testing.T) { // the empty-trie root, because zero blobs hash to non-zero leaves. If // this assertion ever flips, the tombstone semantics have silently // changed and the deletion-detection branch in GetAccount needs review. - empty := newTestTrie(t).Hash() + empty := newEmptyTestTrie(t).Hash() if first == empty { t.Fatalf("post-delete root unexpectedly equals empty-trie root %x; tombstone semantics changed", empty) }