From deda47f6a1c0588b9e2600ca97552994c2889d37 Mon Sep 17 00:00:00 2001 From: CPerezz <37264926+CPerezz@users.noreply.github.com> Date: Fri, 10 Apr 2026 19:43:48 +0200 Subject: [PATCH] =?UTF-8?q?trie/bintrie:=20fix=20GetAccount/GetStorage=20n?= =?UTF-8?q?on-membership=20=E2=80=94=20verify=20stem=20before=20returning?= =?UTF-8?q?=20values=20(#34690)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix `GetAccount` returning **wrong account data** for non-existent addresses when the trie root is a `StemNode` (single-account trie) — the `StemNode` branch returned `r.Values` without verifying the queried address's stem matches. Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com> --- trie/bintrie/stem_node.go | 5 +- trie/bintrie/stem_node_test.go | 44 +++++++++ trie/bintrie/trie.go | 2 +- trie/bintrie/trie_test.go | 159 +++++++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+), 2 deletions(-) diff --git a/trie/bintrie/stem_node.go b/trie/bintrie/stem_node.go index 3f69261d62..e5729e6182 100644 --- a/trie/bintrie/stem_node.go +++ b/trie/bintrie/stem_node.go @@ -37,7 +37,10 @@ type StemNode struct { // Get retrieves the value for the given key. func (bt *StemNode) Get(key []byte, _ NodeResolverFn) ([]byte, error) { - panic("this should not be called directly") + if !bytes.Equal(bt.Stem, key[:StemSize]) { + return nil, nil + } + return bt.Values[key[StemSize]], nil } // Insert inserts a new key-value pair into the node. diff --git a/trie/bintrie/stem_node_test.go b/trie/bintrie/stem_node_test.go index 92c1b49e02..310c553d39 100644 --- a/trie/bintrie/stem_node_test.go +++ b/trie/bintrie/stem_node_test.go @@ -23,6 +23,50 @@ import ( "github.com/ethereum/go-ethereum/common" ) +// TestStemNodeGet tests the Get method for matching stem, non-matching stem, +// and nil-value suffix scenarios. +func TestStemNodeGet(t *testing.T) { + stem := make([]byte, StemSize) + stem[0] = 0xAB + var values [StemNodeWidth][]byte + values[5] = common.HexToHash("0xdeadbeef").Bytes() + + node := &StemNode{Stem: stem, Values: values[:], depth: 0} + + // Matching stem, populated suffix → returns value. + key := make([]byte, HashSize) + copy(key[:StemSize], stem) + key[StemSize] = 5 + got, err := node.Get(key, nil) + if err != nil { + t.Fatalf("Get error: %v", err) + } + if !bytes.Equal(got, values[5]) { + t.Fatalf("Get = %x, want %x", got, values[5]) + } + + // Matching stem, empty suffix → returns nil (slot not set). + key[StemSize] = 99 + got, err = node.Get(key, nil) + if err != nil { + t.Fatalf("Get error: %v", err) + } + if got != nil { + t.Fatalf("Get(empty suffix) = %x, want nil", got) + } + + // Non-matching stem → returns nil, nil. + otherKey := make([]byte, HashSize) + otherKey[0] = 0xFF + got, err = node.Get(otherKey, nil) + if err != nil { + t.Fatalf("Get error: %v", err) + } + if got != nil { + t.Fatalf("Get(wrong stem) = %x, want nil", got) + } +} + // TestStemNodeInsertSameStem tests inserting values with the same stem func TestStemNodeInsertSameStem(t *testing.T) { stem := make([]byte, 31) diff --git a/trie/bintrie/trie.go b/trie/bintrie/trie.go index 45f9edd46c..b1e3c991c0 100644 --- a/trie/bintrie/trie.go +++ b/trie/bintrie/trie.go @@ -191,7 +191,7 @@ func (t *BinaryTrie) GetAccount(addr common.Address) (*types.StateAccount, error case *InternalNode: values, err = r.GetValuesAtStem(key[:StemSize], t.nodeResolver) case *StemNode: - values = r.Values + values, err = r.GetValuesAtStem(key[:StemSize], t.nodeResolver) case Empty: return nil, nil default: diff --git a/trie/bintrie/trie_test.go b/trie/bintrie/trie_test.go index f420f53ef8..5b104ddde4 100644 --- a/trie/bintrie/trie_test.go +++ b/trie/bintrie/trie_test.go @@ -620,3 +620,162 @@ func TestBinaryTrieWitness(t *testing.T) { t.Fatal("unexpected witness value for path2") } } + +// testAccount is a helper that creates a BinaryTrie with a tracer and +// inserts a single account, returning the trie. +func testAccount(t *testing.T, addr common.Address, nonce uint64, balance uint64) *BinaryTrie { + t.Helper() + tr := &BinaryTrie{ + root: NewBinaryNode(), + tracer: trie.NewPrevalueTracer(), + } + acc := &types.StateAccount{ + Nonce: nonce, + Balance: uint256.NewInt(balance), + CodeHash: types.EmptyCodeHash[:], + } + if err := tr.UpdateAccount(addr, acc, 0); err != nil { + t.Fatalf("UpdateAccount error: %v", err) + } + return tr +} + +// TestGetAccountNonMembershipStemRoot verifies that querying a non-existent +// address returns nil when the trie root is a StemNode (single-account trie). +// This is a regression test: previously the StemNode branch in GetAccount +// returned the root's values without verifying the stem. +func TestGetAccountNonMembershipStemRoot(t *testing.T) { + addr := common.HexToAddress("0x1111111111111111111111111111111111111111") + tr := testAccount(t, addr, 42, 100) + + // Verify root is a StemNode (single stem inserted). + if _, ok := tr.root.(*StemNode); !ok { + t.Fatalf("expected StemNode root, got %T", tr.root) + } + + // Query a completely different address — must return nil. + other := common.HexToAddress("0x2222222222222222222222222222222222222222") + got, err := tr.GetAccount(other) + if err != nil { + t.Fatalf("GetAccount error: %v", err) + } + if got != nil { + t.Fatalf("expected nil for non-existent account, got nonce=%d balance=%s", got.Nonce, got.Balance) + } + + // Original account must still be retrievable. + got, err = tr.GetAccount(addr) + if err != nil { + t.Fatalf("GetAccount(original) error: %v", err) + } + if got == nil { + t.Fatal("expected original account, got nil") + } + if got.Nonce != 42 { + t.Fatalf("expected nonce=42, got %d", got.Nonce) + } +} + +// TestGetAccountNonMembershipInternalRoot verifies that querying a non-existent +// address returns nil when the trie root is an InternalNode (multi-account trie). +func TestGetAccountNonMembershipInternalRoot(t *testing.T) { + tr := &BinaryTrie{ + root: NewBinaryNode(), + tracer: trie.NewPrevalueTracer(), + } + + // Insert two accounts whose binary tree keys have different first bits + // so the root splits into an InternalNode. + addr1 := common.HexToAddress("0x1111111111111111111111111111111111111111") + addr2 := common.HexToAddress("0x9999999999999999999999999999999999999999") + for _, addr := range []common.Address{addr1, addr2} { + acc := &types.StateAccount{ + Nonce: 1, + Balance: uint256.NewInt(1), + CodeHash: types.EmptyCodeHash[:], + } + if err := tr.UpdateAccount(addr, acc, 0); err != nil { + t.Fatalf("UpdateAccount error: %v", err) + } + } + + // Verify root is an InternalNode. + if _, ok := tr.root.(*InternalNode); !ok { + t.Fatalf("expected InternalNode root, got %T", tr.root) + } + + // Query a non-existent address — must return nil. + other := common.HexToAddress("0x5555555555555555555555555555555555555555") + got, err := tr.GetAccount(other) + if err != nil { + t.Fatalf("GetAccount error: %v", err) + } + if got != nil { + t.Fatalf("expected nil for non-existent account, got nonce=%d", got.Nonce) + } +} + +// TestGetStorageNonMembershipStemRoot verifies that querying storage for a +// non-existent address returns nil when the root is a StemNode. This is a +// regression test: previously StemNode.Get panicked unconditionally. +func TestGetStorageNonMembershipStemRoot(t *testing.T) { + addr := common.HexToAddress("0x1111111111111111111111111111111111111111") + tr := testAccount(t, addr, 1, 100) + + // Verify root is a StemNode. + if _, ok := tr.root.(*StemNode); !ok { + t.Fatalf("expected StemNode root, got %T", tr.root) + } + + // Query storage for a different address — must return nil, not panic. + other := common.HexToAddress("0x2222222222222222222222222222222222222222") + slot := common.HexToHash("0x01") + got, err := tr.GetStorage(other, slot[:]) + if err != nil { + t.Fatalf("GetStorage error: %v", err) + } + if len(got) > 0 && !bytes.Equal(got, zero[:]) { + t.Fatalf("expected nil/zero for non-existent storage, got %x", got) + } +} + +// TestGetStorageNonMembershipInternalRoot verifies that querying storage for a +// non-existent address returns nil when the root is an InternalNode. +func TestGetStorageNonMembershipInternalRoot(t *testing.T) { + tr := &BinaryTrie{ + root: NewBinaryNode(), + tracer: trie.NewPrevalueTracer(), + } + + addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678") + acc := &types.StateAccount{ + Nonce: 1, + Balance: uint256.NewInt(1000), + CodeHash: types.EmptyCodeHash[:], + } + if err := tr.UpdateAccount(addr, acc, 0); err != nil { + t.Fatalf("UpdateAccount error: %v", err) + } + + // Add a storage slot so the root becomes an InternalNode (storage + // slots use a different stem than account data). + slot := common.HexToHash("0xFF") + val := common.TrimLeftZeroes(common.HexToHash("0xdeadbeef").Bytes()) + if err := tr.UpdateStorage(addr, slot[:], val); err != nil { + t.Fatalf("UpdateStorage error: %v", err) + } + + if _, ok := tr.root.(*InternalNode); !ok { + t.Fatalf("expected InternalNode root, got %T", tr.root) + } + + // Query storage for a non-existent address — must return nil. + other := common.HexToAddress("0x9999999999999999999999999999999999999999") + got, err := tr.GetStorage(other, slot[:]) + if err != nil { + t.Fatalf("GetStorage error: %v", err) + } + if len(got) > 0 && !bytes.Equal(got, zero[:]) { + t.Fatalf("expected nil/zero for non-existent storage, got %x", got) + } +}