mirror of
https://github.com/ethereum/go-ethereum.git
synced 2026-06-12 09:51:36 +00:00
trie/bintrie: fix GetAccount/GetStorage non-membership — verify stem before returning values
BinaryTrie.GetAccount returned the wrong account data for non-existent
addresses when the trie root was a StemNode (single-account trie). The
StemNode branch directly returned r.Values without verifying that the
queried address's stem matched the node's stem. Similarly, GetStorage
panicked via StemNode.Get("this should not be called directly") when
the root was a StemNode.
Additionally, Empty.GetValuesAtStem returned a non-nil slice of 256
nil entries instead of nil, creating a semantic trap for callers that
check values != nil to determine membership.
Fix all four bug sites:
1. StemNode.Get: replace panic with proper stem verification and value
lookup, matching InternalNode.Get's contract.
2. GetAccount StemNode branch: delegate to GetValuesAtStem (which
already has the stem equality check) instead of accessing r.Values
directly. This is consistent with the InternalNode branch.
3. Empty.GetValuesAtStem: return nil instead of 256 nil values.
Callers (InternalNode.Get, GetAccount) already handle nil correctly.
4. GetAccount: add explicit nil-values guard before the decode logic
as defense-in-depth, and simplify the now-redundant values != nil
condition in the emptyAccount loop.
This commit is contained in:
parent
58557cb463
commit
169c545693
6 changed files with 219 additions and 17 deletions
|
|
@ -48,8 +48,7 @@ func (e Empty) Hash() common.Hash {
|
|||
}
|
||||
|
||||
func (e Empty) GetValuesAtStem(_ []byte, _ NodeResolverFn) ([][]byte, error) {
|
||||
var values [256][]byte
|
||||
return values[:], nil
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func (e Empty) InsertValuesAtStem(key []byte, values [][]byte, _ NodeResolverFn, depth int) (BinaryNode, error) {
|
||||
|
|
|
|||
|
|
@ -105,7 +105,8 @@ func TestEmptyHash(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// TestEmptyGetValuesAtStem tests the GetValuesAtStem method
|
||||
// TestEmptyGetValuesAtStem tests that GetValuesAtStem returns nil for an empty node,
|
||||
// signaling that no values exist at the queried stem.
|
||||
func TestEmptyGetValuesAtStem(t *testing.T) {
|
||||
node := Empty{}
|
||||
|
||||
|
|
@ -114,16 +115,8 @@ func TestEmptyGetValuesAtStem(t *testing.T) {
|
|||
if err != nil {
|
||||
t.Fatalf("Unexpected error: %v", err)
|
||||
}
|
||||
|
||||
// Should return an array of 256 nil values
|
||||
if len(values) != 256 {
|
||||
t.Errorf("Expected 256 values, got %d", len(values))
|
||||
}
|
||||
|
||||
for i, v := range values {
|
||||
if v != nil {
|
||||
t.Errorf("Expected nil value at index %d, got %x", i, v)
|
||||
}
|
||||
if values != nil {
|
||||
t.Errorf("Expected nil values from Empty.GetValuesAtStem, got %d entries", len(values))
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -35,9 +35,13 @@ type StemNode struct {
|
|||
hash common.Hash // cached hash when mustRecompute == false
|
||||
}
|
||||
|
||||
// Get retrieves the value for the given key.
|
||||
// Get retrieves the value for the given key. Returns (nil, nil) if the
|
||||
// key's stem does not match this node's stem (non-membership).
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
@ -202,6 +202,9 @@ func (t *BinaryTrie) GetAccount(addr common.Address) (*types.StateAccount, error
|
|||
if err != nil {
|
||||
return nil, fmt.Errorf("GetAccount (%x) error: %v", addr, err)
|
||||
}
|
||||
if values == nil {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
// The following code is required for the MPT->Binary conversion.
|
||||
// An account can be partially migrated, where storage slots were moved to the binary
|
||||
|
|
@ -209,7 +212,7 @@ func (t *BinaryTrie) GetAccount(addr common.Address) (*types.StateAccount, error
|
|||
// are in the binary trie but basic account information must be read in the base tree (MPT).
|
||||
// TODO: we can simplify this logic depending if the conversion is in progress or finished.
|
||||
emptyAccount := true
|
||||
for i := 0; values != nil && i <= CodeHashLeafKey && emptyAccount; i++ {
|
||||
for i := 0; i <= CodeHashLeafKey && emptyAccount; i++ {
|
||||
emptyAccount = emptyAccount && values[i] == nil
|
||||
}
|
||||
if emptyAccount {
|
||||
|
|
|
|||
|
|
@ -292,3 +292,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)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue