trie/bintrie: use correct key mapping in GetStorage and DeleteStorage (#33807)

GetStorage and DeleteStorage used GetBinaryTreeKey to compute the tree
key, while UpdateStorage used GetBinaryTreeKeyStorageSlot. The latter
applies storage slot remapping (header offset for slots <64, main
storage prefix for the rest), so reads and deletes were targeting
different tree locations than writes.

Replace GetBinaryTreeKey with GetBinaryTreeKeyStorageSlot in both
GetStorage and DeleteStorage to match UpdateStorage. Add a regression
test that verifies the write→read→delete→read round-trip for main
storage slots.
This commit is contained in:
phrwlk 2026-02-11 12:42:17 +02:00 committed by GitHub
parent 15a9e92bbd
commit 30656d714e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 72 additions and 2 deletions

View file

@ -236,7 +236,7 @@ func (t *BinaryTrie) GetAccount(addr common.Address) (*types.StateAccount, error
// not be modified by the caller. If a node was not found in the database, a
// trie.MissingNodeError is returned.
func (t *BinaryTrie) GetStorage(addr common.Address, key []byte) ([]byte, error) {
return t.root.Get(GetBinaryTreeKey(addr, key), t.nodeResolver)
return t.root.Get(GetBinaryTreeKeyStorageSlot(addr, key), t.nodeResolver)
}
// UpdateAccount updates the account information for the given address.
@ -302,7 +302,7 @@ func (t *BinaryTrie) DeleteAccount(addr common.Address) error {
// 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 {
k := GetBinaryTreeKey(addr, key)
k := GetBinaryTreeKeyStorageSlot(addr, key)
var zero [HashSize]byte
root, err := t.root.Insert(k, zero[:], t.nodeResolver, 0)
if err != nil {

View file

@ -22,7 +22,9 @@ import (
"testing"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/trie"
"github.com/holiman/uint256"
)
var (
@ -197,6 +199,74 @@ func TestMerkleizeMultipleEntries(t *testing.T) {
}
}
// TestStorageRoundTrip verifies that GetStorage and DeleteStorage use the same
// key mapping as UpdateStorage (GetBinaryTreeKeyStorageSlot). This is a regression
// test: previously GetStorage and DeleteStorage used GetBinaryTreeKey directly,
// which produced different tree keys and broke the read/delete path.
func TestStorageRoundTrip(t *testing.T) {
tracer := trie.NewPrevalueTracer()
tr := &BinaryTrie{
root: NewBinaryNode(),
tracer: tracer,
}
addr := common.HexToAddress("0x1234567890abcdef1234567890abcdef12345678")
// Create an account first so the root becomes an InternalNode,
// which is the realistic state when storage operations happen.
acc := &types.StateAccount{
Nonce: 1,
Balance: uint256.NewInt(1000),
CodeHash: common.HexToHash("c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470").Bytes(),
}
if err := tr.UpdateAccount(addr, acc, 0); err != nil {
t.Fatalf("UpdateAccount error: %v", err)
}
// Test main storage slots (key[31] >= 64 or key[:31] != 0).
// These produce a different stem than the account data, so after
// UpdateAccount + UpdateStorage the root is an InternalNode.
// Note: header slots (key[31] < 64, key[:31] == 0) share the same
// stem as account data and are covered by GetAccount/UpdateAccount path.
slots := []common.Hash{
common.HexToHash("00000000000000000000000000000000000000000000000000000000000000FF"), // main storage (slot 255)
common.HexToHash("0100000000000000000000000000000000000000000000000000000000000001"), // main storage (non-zero prefix)
}
val := common.TrimLeftZeroes(common.HexToHash("00000000000000000000000000000000000000000000000000000000deadbeef").Bytes())
for _, slot := range slots {
// Write
if err := tr.UpdateStorage(addr, slot[:], val); err != nil {
t.Fatalf("UpdateStorage(%x) error: %v", slot, err)
}
// Read back
got, err := tr.GetStorage(addr, slot[:])
if err != nil {
t.Fatalf("GetStorage(%x) error: %v", slot, err)
}
if len(got) == 0 {
t.Fatalf("GetStorage(%x) returned empty, expected value", slot)
}
// Verify value (right-justified in 32 bytes)
var expected [HashSize]byte
copy(expected[HashSize-len(val):], val)
if !bytes.Equal(got, expected[:]) {
t.Fatalf("GetStorage(%x) = %x, want %x", slot, got, expected)
}
// Delete
if err := tr.DeleteStorage(addr, slot[:]); err != nil {
t.Fatalf("DeleteStorage(%x) error: %v", slot, err)
}
// Verify deleted (should read as zero, not the old value)
got, err = tr.GetStorage(addr, slot[:])
if err != nil {
t.Fatalf("GetStorage(%x) after delete error: %v", slot, err)
}
if len(got) > 0 && !bytes.Equal(got, zero[:]) {
t.Fatalf("GetStorage(%x) after delete = %x, expected zero", slot, got)
}
}
}
func TestBinaryTrieWitness(t *testing.T) {
tracer := trie.NewPrevalueTracer()