Skip to content

Commit deda47f

Browse files
CPerezzgballet
andauthored
trie/bintrie: fix GetAccount/GetStorage non-membership — verify stem before returning values (#34690)
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>
1 parent f71a884 commit deda47f

4 files changed

Lines changed: 208 additions & 2 deletions

File tree

trie/bintrie/stem_node.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,10 @@ type StemNode struct {
3737

3838
// Get retrieves the value for the given key.
3939
func (bt *StemNode) Get(key []byte, _ NodeResolverFn) ([]byte, error) {
40-
panic("this should not be called directly")
40+
if !bytes.Equal(bt.Stem, key[:StemSize]) {
41+
return nil, nil
42+
}
43+
return bt.Values[key[StemSize]], nil
4144
}
4245

4346
// Insert inserts a new key-value pair into the node.

trie/bintrie/stem_node_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,50 @@ import (
2323
"github.com/ethereum/go-ethereum/common"
2424
)
2525

26+
// TestStemNodeGet tests the Get method for matching stem, non-matching stem,
27+
// and nil-value suffix scenarios.
28+
func TestStemNodeGet(t *testing.T) {
29+
stem := make([]byte, StemSize)
30+
stem[0] = 0xAB
31+
var values [StemNodeWidth][]byte
32+
values[5] = common.HexToHash("0xdeadbeef").Bytes()
33+
34+
node := &StemNode{Stem: stem, Values: values[:], depth: 0}
35+
36+
// Matching stem, populated suffix → returns value.
37+
key := make([]byte, HashSize)
38+
copy(key[:StemSize], stem)
39+
key[StemSize] = 5
40+
got, err := node.Get(key, nil)
41+
if err != nil {
42+
t.Fatalf("Get error: %v", err)
43+
}
44+
if !bytes.Equal(got, values[5]) {
45+
t.Fatalf("Get = %x, want %x", got, values[5])
46+
}
47+
48+
// Matching stem, empty suffix → returns nil (slot not set).
49+
key[StemSize] = 99
50+
got, err = node.Get(key, nil)
51+
if err != nil {
52+
t.Fatalf("Get error: %v", err)
53+
}
54+
if got != nil {
55+
t.Fatalf("Get(empty suffix) = %x, want nil", got)
56+
}
57+
58+
// Non-matching stem → returns nil, nil.
59+
otherKey := make([]byte, HashSize)
60+
otherKey[0] = 0xFF
61+
got, err = node.Get(otherKey, nil)
62+
if err != nil {
63+
t.Fatalf("Get error: %v", err)
64+
}
65+
if got != nil {
66+
t.Fatalf("Get(wrong stem) = %x, want nil", got)
67+
}
68+
}
69+
2670
// TestStemNodeInsertSameStem tests inserting values with the same stem
2771
func TestStemNodeInsertSameStem(t *testing.T) {
2872
stem := make([]byte, 31)

trie/bintrie/trie.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ func (t *BinaryTrie) GetAccount(addr common.Address) (*types.StateAccount, error
191191
case *InternalNode:
192192
values, err = r.GetValuesAtStem(key[:StemSize], t.nodeResolver)
193193
case *StemNode:
194-
values = r.Values
194+
values, err = r.GetValuesAtStem(key[:StemSize], t.nodeResolver)
195195
case Empty:
196196
return nil, nil
197197
default:

trie/bintrie/trie_test.go

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,3 +620,162 @@ func TestBinaryTrieWitness(t *testing.T) {
620620
t.Fatal("unexpected witness value for path2")
621621
}
622622
}
623+
624+
// testAccount is a helper that creates a BinaryTrie with a tracer and
625+
// inserts a single account, returning the trie.
626+
func testAccount(t *testing.T, addr common.Address, nonce uint64, balance uint64) *BinaryTrie {
627+
t.Helper()
628+
tr := &BinaryTrie{
629+
root: NewBinaryNode(),
630+
tracer: trie.NewPrevalueTracer(),
631+
}
632+
acc := &types.StateAccount{
633+
Nonce: nonce,
634+
Balance: uint256.NewInt(balance),
635+
CodeHash: types.EmptyCodeHash[:],
636+
}
637+
if err := tr.UpdateAccount(addr, acc, 0); err != nil {
638+
t.Fatalf("UpdateAccount error: %v", err)
639+
}
640+
return tr
641+
}
642+
643+
// TestGetAccountNonMembershipStemRoot verifies that querying a non-existent
644+
// address returns nil when the trie root is a StemNode (single-account trie).
645+
// This is a regression test: previously the StemNode branch in GetAccount
646+
// returned the root's values without verifying the stem.
647+
func TestGetAccountNonMembershipStemRoot(t *testing.T) {
648+
addr := common.HexToAddress("0x1111111111111111111111111111111111111111")
649+
tr := testAccount(t, addr, 42, 100)
650+
651+
// Verify root is a StemNode (single stem inserted).
652+
if _, ok := tr.root.(*StemNode); !ok {
653+
t.Fatalf("expected StemNode root, got %T", tr.root)
654+
}
655+
656+
// Query a completely different address — must return nil.
657+
other := common.HexToAddress("0x2222222222222222222222222222222222222222")
658+
got, err := tr.GetAccount(other)
659+
if err != nil {
660+
t.Fatalf("GetAccount error: %v", err)
661+
}
662+
if got != nil {
663+
t.Fatalf("expected nil for non-existent account, got nonce=%d balance=%s", got.Nonce, got.Balance)
664+
}
665+
666+
// Original account must still be retrievable.
667+
got, err = tr.GetAccount(addr)
668+
if err != nil {
669+
t.Fatalf("GetAccount(original) error: %v", err)
670+
}
671+
if got == nil {
672+
t.Fatal("expected original account, got nil")
673+
}
674+
if got.Nonce != 42 {
675+
t.Fatalf("expected nonce=42, got %d", got.Nonce)
676+
}
677+
}
678+
679+
// TestGetAccountNonMembershipInternalRoot verifies that querying a non-existent
680+
// address returns nil when the trie root is an InternalNode (multi-account trie).
681+
func TestGetAccountNonMembershipInternalRoot(t *testing.T) {
682+
tr := &BinaryTrie{
683+
root: NewBinaryNode(),
684+
tracer: trie.NewPrevalueTracer(),
685+
}
686+
687+
// Insert two accounts whose binary tree keys have different first bits
688+
// so the root splits into an InternalNode.
689+
addr1 := common.HexToAddress("0x1111111111111111111111111111111111111111")
690+
addr2 := common.HexToAddress("0x9999999999999999999999999999999999999999")
691+
for _, addr := range []common.Address{addr1, addr2} {
692+
acc := &types.StateAccount{
693+
Nonce: 1,
694+
Balance: uint256.NewInt(1),
695+
CodeHash: types.EmptyCodeHash[:],
696+
}
697+
if err := tr.UpdateAccount(addr, acc, 0); err != nil {
698+
t.Fatalf("UpdateAccount error: %v", err)
699+
}
700+
}
701+
702+
// Verify root is an InternalNode.
703+
if _, ok := tr.root.(*InternalNode); !ok {
704+
t.Fatalf("expected InternalNode root, got %T", tr.root)
705+
}
706+
707+
// Query a non-existent address — must return nil.
708+
other := common.HexToAddress("0x5555555555555555555555555555555555555555")
709+
got, err := tr.GetAccount(other)
710+
if err != nil {
711+
t.Fatalf("GetAccount error: %v", err)
712+
}
713+
if got != nil {
714+
t.Fatalf("expected nil for non-existent account, got nonce=%d", got.Nonce)
715+
}
716+
}
717+
718+
// TestGetStorageNonMembershipStemRoot verifies that querying storage for a
719+
// non-existent address returns nil when the root is a StemNode. This is a
720+
// regression test: previously StemNode.Get panicked unconditionally.
721+
func TestGetStorageNonMembershipStemRoot(t *testing.T) {
722+
addr := common.HexToAddress("0x1111111111111111111111111111111111111111")
723+
tr := testAccount(t, addr, 1, 100)
724+
725+
// Verify root is a StemNode.
726+
if _, ok := tr.root.(*StemNode); !ok {
727+
t.Fatalf("expected StemNode root, got %T", tr.root)
728+
}
729+
730+
// Query storage for a different address — must return nil, not panic.
731+
other := common.HexToAddress("0x2222222222222222222222222222222222222222")
732+
slot := common.HexToHash("0x01")
733+
got, err := tr.GetStorage(other, slot[:])
734+
if err != nil {
735+
t.Fatalf("GetStorage error: %v", err)
736+
}
737+
if len(got) > 0 && !bytes.Equal(got, zero[:]) {
738+
t.Fatalf("expected nil/zero for non-existent storage, got %x", got)
739+
}
740+
}
741+
742+
// TestGetStorageNonMembershipInternalRoot verifies that querying storage for a
743+
// non-existent address returns nil when the root is an InternalNode.
744+
func TestGetStorageNonMembershipInternalRoot(t *testing.T) {
745+
tr := &BinaryTrie{
746+
root: NewBinaryNode(),
747+
tracer: trie.NewPrevalueTracer(),
748+
}
749+
750+
addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678")
751+
acc := &types.StateAccount{
752+
Nonce: 1,
753+
Balance: uint256.NewInt(1000),
754+
CodeHash: types.EmptyCodeHash[:],
755+
}
756+
if err := tr.UpdateAccount(addr, acc, 0); err != nil {
757+
t.Fatalf("UpdateAccount error: %v", err)
758+
}
759+
760+
// Add a storage slot so the root becomes an InternalNode (storage
761+
// slots use a different stem than account data).
762+
slot := common.HexToHash("0xFF")
763+
val := common.TrimLeftZeroes(common.HexToHash("0xdeadbeef").Bytes())
764+
if err := tr.UpdateStorage(addr, slot[:], val); err != nil {
765+
t.Fatalf("UpdateStorage error: %v", err)
766+
}
767+
768+
if _, ok := tr.root.(*InternalNode); !ok {
769+
t.Fatalf("expected InternalNode root, got %T", tr.root)
770+
}
771+
772+
// Query storage for a non-existent address — must return nil.
773+
other := common.HexToAddress("0x9999999999999999999999999999999999999999")
774+
got, err := tr.GetStorage(other, slot[:])
775+
if err != nil {
776+
t.Fatalf("GetStorage error: %v", err)
777+
}
778+
if len(got) > 0 && !bytes.Equal(got, zero[:]) {
779+
t.Fatalf("expected nil/zero for non-existent storage, got %x", got)
780+
}
781+
}

0 commit comments

Comments
 (0)