Skip to content

Commit 3772bb5

Browse files
authored
triedb/pathdb: fix lookup sentinel collision with zero disk layer root (#34680)
1 parent 68c7058 commit 3772bb5

File tree

3 files changed

+137
-23
lines changed

3 files changed

+137
-23
lines changed

triedb/pathdb/layertree.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -319,8 +319,8 @@ func (tree *layerTree) lookupAccount(accountHash common.Hash, state common.Hash)
319319
tree.lock.RLock()
320320
defer tree.lock.RUnlock()
321321

322-
tip := tree.lookup.accountTip(accountHash, state, tree.base.root)
323-
if tip == (common.Hash{}) {
322+
tip, ok := tree.lookup.accountTip(accountHash, state, tree.base.root)
323+
if !ok {
324324
return nil, fmt.Errorf("[%#x] %w", state, errSnapshotStale)
325325
}
326326
l := tree.layers[tip]
@@ -337,8 +337,8 @@ func (tree *layerTree) lookupStorage(accountHash common.Hash, slotHash common.Ha
337337
tree.lock.RLock()
338338
defer tree.lock.RUnlock()
339339

340-
tip := tree.lookup.storageTip(accountHash, slotHash, state, tree.base.root)
341-
if tip == (common.Hash{}) {
340+
tip, ok := tree.lookup.storageTip(accountHash, slotHash, state, tree.base.root)
341+
if !ok {
342342
return nil, fmt.Errorf("[%#x] %w", state, errSnapshotStale)
343343
}
344344
l := tree.layers[tip]

triedb/pathdb/layertree_test.go

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -916,3 +916,118 @@ func TestStorageLookup(t *testing.T) {
916916
}
917917
}
918918
}
919+
920+
// TestLookupZeroBaseRootFallback is a regression test for a sentinel
921+
// collision in accountTip/storageTip: before the fix they returned
922+
// common.Hash{} as both the "stale" marker and the disk-layer fallback
923+
// when the disk root itself happened to be zero. lookupAccount/Storage
924+
// then misreported a legitimate fallback as errSnapshotStale.
925+
//
926+
// On the merkle path the collision was invisible because the empty
927+
// merkle trie hashes to types.EmptyRootHash (a concrete non-zero
928+
// keccak), so the disk layer's root was never the zero hash in
929+
// practice. The bug only surfaces once the disk layer root can
930+
// legitimately be zero (for example a fresh verkle/bintrie database
931+
// where the empty binary trie hashes to EmptyVerkleHash ==
932+
// common.Hash{}).
933+
//
934+
// The test constructs a layer tree whose base layer's root IS the zero
935+
// hash, stacks diff layers on top, and exercises four cases:
936+
//
937+
// 1. Look up an account NEVER written → should fall through to the
938+
// disk layer and return (diskLayer, nil). Before the fix this
939+
// returned errSnapshotStale because the fallback hash collided
940+
// with the sentinel.
941+
// 2. Symmetric case for lookupStorage.
942+
// 3. Look up an account written in a diff layer → should return that
943+
// diff layer (the normal happy path is unaffected by the fix).
944+
// 4. Look up any key at a state root that isn't part of the tree
945+
// (neither the disk root nor a descendant of it) → MUST still
946+
// return errSnapshotStale. This pins the "other half" of the
947+
// contract so a future refactor that always returns ok=true would
948+
// fail here.
949+
func TestLookupZeroBaseRootFallback(t *testing.T) {
950+
// Build a layer tree whose disk-layer root is common.Hash{} —
951+
// mirrors the bintrie/verkle configuration where the empty trie
952+
// hashes to EmptyVerkleHash. newTestLayerTree can't be reused
953+
// because it hard-codes common.Hash{0x1}.
954+
db := New(rawdb.NewMemoryDatabase(), nil, false)
955+
base := newDiskLayer(common.Hash{}, 0, db, nil, nil, newBuffer(0, nil, nil, 0), nil)
956+
tr := newLayerTree(base)
957+
958+
// Stack two diff layers on the zero-rooted disk layer, each
959+
// touching a known account and slot so we have something for the
960+
// happy-path lookups to find later.
961+
if err := tr.add(
962+
common.Hash{0x2}, common.Hash{},
963+
1,
964+
NewNodeSetWithOrigin(nil, nil),
965+
NewStateSetWithOrigin(
966+
randomAccountSet("0xa"),
967+
randomStorageSet([]string{"0xa"}, [][]string{{"0x1"}}, nil),
968+
nil, nil, false),
969+
); err != nil {
970+
t.Fatalf("add first diff layer: %v", err)
971+
}
972+
if err := tr.add(
973+
common.Hash{0x3}, common.Hash{0x2},
974+
2,
975+
NewNodeSetWithOrigin(nil, nil),
976+
NewStateSetWithOrigin(
977+
randomAccountSet("0xb"),
978+
nil, nil, nil, false),
979+
); err != nil {
980+
t.Fatalf("add second diff layer: %v", err)
981+
}
982+
983+
// Case 1: unknown account queried at the head. The lookup must
984+
// fall through the diff layers, hit the disk-layer fallback at
985+
// base=common.Hash{}, and return the disk layer with no error —
986+
// NOT errSnapshotStale.
987+
l, err := tr.lookupAccount(common.HexToHash("0xdead"), common.Hash{0x3})
988+
if err != nil {
989+
t.Fatalf("lookupAccount on zero-base disk layer: unexpected error %v", err)
990+
}
991+
if l.rootHash() != (common.Hash{}) {
992+
t.Errorf("expected fall-through to disk layer (root=0), got %x", l.rootHash())
993+
}
994+
995+
// Case 2: symmetric check for storage. Slot 0x99 was never written,
996+
// so the lookup must fall through to the disk layer just like
997+
// Case 1.
998+
l, err = tr.lookupStorage(
999+
common.HexToHash("0xdead"), common.HexToHash("0x99"), common.Hash{0x3})
1000+
if err != nil {
1001+
t.Fatalf("lookupStorage on zero-base disk layer: unexpected error %v", err)
1002+
}
1003+
if l.rootHash() != (common.Hash{}) {
1004+
t.Errorf("expected fall-through to disk layer (root=0), got %x", l.rootHash())
1005+
}
1006+
1007+
// Case 3: happy path. Account 0xa was written at diff layer 0x2.
1008+
// The lookup must return that layer, proving the fix didn't break
1009+
// the normal resolution path.
1010+
l, err = tr.lookupAccount(common.HexToHash("0xa"), common.Hash{0x3})
1011+
if err != nil {
1012+
t.Fatalf("lookupAccount(known): %v", err)
1013+
}
1014+
if l.rootHash() != (common.Hash{0x2}) {
1015+
t.Errorf("known account tip: want %x, got %x",
1016+
common.Hash{0x2}, l.rootHash())
1017+
}
1018+
1019+
// Case 4: truly stale state root. This pins the other half of the
1020+
// contract — the boolean must actually signal not-found for an
1021+
// unknown state, otherwise a refactor that always returned
1022+
// ok=true would still pass cases 1–3.
1023+
_, err = tr.lookupAccount(common.HexToHash("0xa"), common.HexToHash("0xdeadbeef"))
1024+
if !errors.Is(err, errSnapshotStale) {
1025+
t.Errorf("lookupAccount(stale state): want errSnapshotStale, got %v", err)
1026+
}
1027+
_, err = tr.lookupStorage(
1028+
common.HexToHash("0xa"), common.HexToHash("0x1"),
1029+
common.HexToHash("0xdeadbeef"))
1030+
if !errors.Is(err, errSnapshotStale) {
1031+
t.Errorf("lookupStorage(stale state): want errSnapshotStale, got %v", err)
1032+
}
1033+
}

triedb/pathdb/lookup.go

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,16 @@ func newLookup(head layer, descendant func(state common.Hash, ancestor common.Ha
9292
// stateID or is a descendant of it.
9393
//
9494
// If found, the account data corresponding to the supplied stateID resides
95-
// in that layer. Otherwise, two scenarios are possible:
95+
// in the layer identified by the returned hash (ok=true). Otherwise,
96+
// (common.Hash{}, false) is returned to signal that the supplied stateID is
97+
// stale.
9698
//
97-
// (a) the account remains unmodified from the current disk layer up to the state
98-
// layer specified by the stateID: fallback to the disk layer for data retrieval,
99-
// (b) or the layer specified by the stateID is stale: reject the data retrieval.
100-
func (l *lookup) accountTip(accountHash common.Hash, stateID common.Hash, base common.Hash) common.Hash {
99+
// Note the returned hash may itself be common.Hash{} when the disk layer's
100+
// root is zero — as is the case for a fresh verkle/bintrie database whose
101+
// empty trie hashes to EmptyVerkleHash. Callers must therefore consult the
102+
// boolean rather than comparing the returned hash against common.Hash{}
103+
// directly.
104+
func (l *lookup) accountTip(accountHash common.Hash, stateID common.Hash, base common.Hash) (common.Hash, bool) {
101105
// Traverse the mutation history from latest to oldest one. Several
102106
// scenarios are possible:
103107
//
@@ -123,49 +127,44 @@ func (l *lookup) accountTip(accountHash common.Hash, stateID common.Hash, base c
123127
// containing the modified data. Otherwise, the current state may be ahead
124128
// of the requested one or belong to a different branch.
125129
if list[i] == stateID || l.descendant(stateID, list[i]) {
126-
return list[i]
130+
return list[i], true
127131
}
128132
}
129133
// No layer matching the stateID or its descendants was found. Use the
130134
// current disk layer as a fallback.
131135
if base == stateID || l.descendant(stateID, base) {
132-
return base
136+
return base, true
133137
}
134138
// The layer associated with 'stateID' is not the descendant of the current
135139
// disk layer, it's already stale, return nothing.
136-
return common.Hash{}
140+
return common.Hash{}, false
137141
}
138142

139143
// storageTip traverses the layer list associated with the given account and
140144
// slot hash in reverse order to locate the first entry that either matches
141145
// the specified stateID or is a descendant of it.
142146
//
143-
// If found, the storage data corresponding to the supplied stateID resides
144-
// in that layer. Otherwise, two scenarios are possible:
145-
//
146-
// (a) the storage slot remains unmodified from the current disk layer up to
147-
// the state layer specified by the stateID: fallback to the disk layer for
148-
// data retrieval, (b) or the layer specified by the stateID is stale: reject
149-
// the data retrieval.
150-
func (l *lookup) storageTip(accountHash common.Hash, slotHash common.Hash, stateID common.Hash, base common.Hash) common.Hash {
147+
// See accountTip for the returned-hash / ok convention — the same
148+
// bintrie-zero-root caveat applies here.
149+
func (l *lookup) storageTip(accountHash common.Hash, slotHash common.Hash, stateID common.Hash, base common.Hash) (common.Hash, bool) {
151150
list := l.storages[storageKey(accountHash, slotHash)]
152151
for i := len(list) - 1; i >= 0; i-- {
153152
// If the current state matches the stateID, or the requested state is a
154153
// descendant of it, return the current state as the most recent one
155154
// containing the modified data. Otherwise, the current state may be ahead
156155
// of the requested one or belong to a different branch.
157156
if list[i] == stateID || l.descendant(stateID, list[i]) {
158-
return list[i]
157+
return list[i], true
159158
}
160159
}
161160
// No layer matching the stateID or its descendants was found. Use the
162161
// current disk layer as a fallback.
163162
if base == stateID || l.descendant(stateID, base) {
164-
return base
163+
return base, true
165164
}
166165
// The layer associated with 'stateID' is not the descendant of the current
167166
// disk layer, it's already stale, return nothing.
168-
return common.Hash{}
167+
return common.Hash{}, false
169168
}
170169

171170
// addLayer traverses the state data retained in the specified diff layer and

0 commit comments

Comments
 (0)