diff --git a/triedb/pathdb/layertree.go b/triedb/pathdb/layertree.go index 0d7ca5a5b4..b20e40bd05 100644 --- a/triedb/pathdb/layertree.go +++ b/triedb/pathdb/layertree.go @@ -319,8 +319,8 @@ func (tree *layerTree) lookupAccount(accountHash common.Hash, state common.Hash) tree.lock.RLock() defer tree.lock.RUnlock() - tip := tree.lookup.accountTip(accountHash, state, tree.base.root) - if tip == (common.Hash{}) { + tip, ok := tree.lookup.accountTip(accountHash, state, tree.base.root) + if !ok { return nil, fmt.Errorf("[%#x] %w", state, errSnapshotStale) } l := tree.layers[tip] @@ -337,8 +337,8 @@ func (tree *layerTree) lookupStorage(accountHash common.Hash, slotHash common.Ha tree.lock.RLock() defer tree.lock.RUnlock() - tip := tree.lookup.storageTip(accountHash, slotHash, state, tree.base.root) - if tip == (common.Hash{}) { + tip, ok := tree.lookup.storageTip(accountHash, slotHash, state, tree.base.root) + if !ok { return nil, fmt.Errorf("[%#x] %w", state, errSnapshotStale) } l := tree.layers[tip] diff --git a/triedb/pathdb/layertree_test.go b/triedb/pathdb/layertree_test.go index 285ca67b6c..82eb182990 100644 --- a/triedb/pathdb/layertree_test.go +++ b/triedb/pathdb/layertree_test.go @@ -916,3 +916,118 @@ func TestStorageLookup(t *testing.T) { } } } + +// TestLookupZeroBaseRootFallback is a regression test for a sentinel +// collision in accountTip/storageTip: before the fix they returned +// common.Hash{} as both the "stale" marker and the disk-layer fallback +// when the disk root itself happened to be zero. lookupAccount/Storage +// then misreported a legitimate fallback as errSnapshotStale. +// +// On the merkle path the collision was invisible because the empty +// merkle trie hashes to types.EmptyRootHash (a concrete non-zero +// keccak), so the disk layer's root was never the zero hash in +// practice. The bug only surfaces once the disk layer root can +// legitimately be zero (for example a fresh verkle/bintrie database +// where the empty binary trie hashes to EmptyVerkleHash == +// common.Hash{}). +// +// The test constructs a layer tree whose base layer's root IS the zero +// hash, stacks diff layers on top, and exercises four cases: +// +// 1. Look up an account NEVER written → should fall through to the +// disk layer and return (diskLayer, nil). Before the fix this +// returned errSnapshotStale because the fallback hash collided +// with the sentinel. +// 2. Symmetric case for lookupStorage. +// 3. Look up an account written in a diff layer → should return that +// diff layer (the normal happy path is unaffected by the fix). +// 4. Look up any key at a state root that isn't part of the tree +// (neither the disk root nor a descendant of it) → MUST still +// return errSnapshotStale. This pins the "other half" of the +// contract so a future refactor that always returns ok=true would +// fail here. +func TestLookupZeroBaseRootFallback(t *testing.T) { + // Build a layer tree whose disk-layer root is common.Hash{} — + // mirrors the bintrie/verkle configuration where the empty trie + // hashes to EmptyVerkleHash. newTestLayerTree can't be reused + // because it hard-codes common.Hash{0x1}. + db := New(rawdb.NewMemoryDatabase(), nil, false) + base := newDiskLayer(common.Hash{}, 0, db, nil, nil, newBuffer(0, nil, nil, 0), nil) + tr := newLayerTree(base) + + // Stack two diff layers on the zero-rooted disk layer, each + // touching a known account and slot so we have something for the + // happy-path lookups to find later. + if err := tr.add( + common.Hash{0x2}, common.Hash{}, + 1, + NewNodeSetWithOrigin(nil, nil), + NewStateSetWithOrigin( + randomAccountSet("0xa"), + randomStorageSet([]string{"0xa"}, [][]string{{"0x1"}}, nil), + nil, nil, false), + ); err != nil { + t.Fatalf("add first diff layer: %v", err) + } + if err := tr.add( + common.Hash{0x3}, common.Hash{0x2}, + 2, + NewNodeSetWithOrigin(nil, nil), + NewStateSetWithOrigin( + randomAccountSet("0xb"), + nil, nil, nil, false), + ); err != nil { + t.Fatalf("add second diff layer: %v", err) + } + + // Case 1: unknown account queried at the head. The lookup must + // fall through the diff layers, hit the disk-layer fallback at + // base=common.Hash{}, and return the disk layer with no error — + // NOT errSnapshotStale. + l, err := tr.lookupAccount(common.HexToHash("0xdead"), common.Hash{0x3}) + if err != nil { + t.Fatalf("lookupAccount on zero-base disk layer: unexpected error %v", err) + } + if l.rootHash() != (common.Hash{}) { + t.Errorf("expected fall-through to disk layer (root=0), got %x", l.rootHash()) + } + + // Case 2: symmetric check for storage. Slot 0x99 was never written, + // so the lookup must fall through to the disk layer just like + // Case 1. + l, err = tr.lookupStorage( + common.HexToHash("0xdead"), common.HexToHash("0x99"), common.Hash{0x3}) + if err != nil { + t.Fatalf("lookupStorage on zero-base disk layer: unexpected error %v", err) + } + if l.rootHash() != (common.Hash{}) { + t.Errorf("expected fall-through to disk layer (root=0), got %x", l.rootHash()) + } + + // Case 3: happy path. Account 0xa was written at diff layer 0x2. + // The lookup must return that layer, proving the fix didn't break + // the normal resolution path. + l, err = tr.lookupAccount(common.HexToHash("0xa"), common.Hash{0x3}) + if err != nil { + t.Fatalf("lookupAccount(known): %v", err) + } + if l.rootHash() != (common.Hash{0x2}) { + t.Errorf("known account tip: want %x, got %x", + common.Hash{0x2}, l.rootHash()) + } + + // Case 4: truly stale state root. This pins the other half of the + // contract — the boolean must actually signal not-found for an + // unknown state, otherwise a refactor that always returned + // ok=true would still pass cases 1–3. + _, err = tr.lookupAccount(common.HexToHash("0xa"), common.HexToHash("0xdeadbeef")) + if !errors.Is(err, errSnapshotStale) { + t.Errorf("lookupAccount(stale state): want errSnapshotStale, got %v", err) + } + _, err = tr.lookupStorage( + common.HexToHash("0xa"), common.HexToHash("0x1"), + common.HexToHash("0xdeadbeef")) + if !errors.Is(err, errSnapshotStale) { + t.Errorf("lookupStorage(stale state): want errSnapshotStale, got %v", err) + } +} diff --git a/triedb/pathdb/lookup.go b/triedb/pathdb/lookup.go index 719546f410..9b300ec871 100644 --- a/triedb/pathdb/lookup.go +++ b/triedb/pathdb/lookup.go @@ -92,12 +92,16 @@ func newLookup(head layer, descendant func(state common.Hash, ancestor common.Ha // stateID or is a descendant of it. // // If found, the account data corresponding to the supplied stateID resides -// in that layer. Otherwise, two scenarios are possible: +// in the layer identified by the returned hash (ok=true). Otherwise, +// (common.Hash{}, false) is returned to signal that the supplied stateID is +// stale. // -// (a) the account remains unmodified from the current disk layer up to the state -// layer specified by the stateID: fallback to the disk layer for data retrieval, -// (b) or the layer specified by the stateID is stale: reject the data retrieval. -func (l *lookup) accountTip(accountHash common.Hash, stateID common.Hash, base common.Hash) common.Hash { +// Note the returned hash may itself be common.Hash{} when the disk layer's +// root is zero — as is the case for a fresh verkle/bintrie database whose +// empty trie hashes to EmptyVerkleHash. Callers must therefore consult the +// boolean rather than comparing the returned hash against common.Hash{} +// directly. +func (l *lookup) accountTip(accountHash common.Hash, stateID common.Hash, base common.Hash) (common.Hash, bool) { // Traverse the mutation history from latest to oldest one. Several // scenarios are possible: // @@ -123,31 +127,26 @@ func (l *lookup) accountTip(accountHash common.Hash, stateID common.Hash, base c // containing the modified data. Otherwise, the current state may be ahead // of the requested one or belong to a different branch. if list[i] == stateID || l.descendant(stateID, list[i]) { - return list[i] + return list[i], true } } // No layer matching the stateID or its descendants was found. Use the // current disk layer as a fallback. if base == stateID || l.descendant(stateID, base) { - return base + return base, true } // The layer associated with 'stateID' is not the descendant of the current // disk layer, it's already stale, return nothing. - return common.Hash{} + return common.Hash{}, false } // storageTip traverses the layer list associated with the given account and // slot hash in reverse order to locate the first entry that either matches // the specified stateID or is a descendant of it. // -// If found, the storage data corresponding to the supplied stateID resides -// in that layer. Otherwise, two scenarios are possible: -// -// (a) the storage slot remains unmodified from the current disk layer up to -// the state layer specified by the stateID: fallback to the disk layer for -// data retrieval, (b) or the layer specified by the stateID is stale: reject -// the data retrieval. -func (l *lookup) storageTip(accountHash common.Hash, slotHash common.Hash, stateID common.Hash, base common.Hash) common.Hash { +// See accountTip for the returned-hash / ok convention — the same +// bintrie-zero-root caveat applies here. +func (l *lookup) storageTip(accountHash common.Hash, slotHash common.Hash, stateID common.Hash, base common.Hash) (common.Hash, bool) { list := l.storages[storageKey(accountHash, slotHash)] for i := len(list) - 1; i >= 0; i-- { // If the current state matches the stateID, or the requested state is a @@ -155,17 +154,17 @@ func (l *lookup) storageTip(accountHash common.Hash, slotHash common.Hash, state // containing the modified data. Otherwise, the current state may be ahead // of the requested one or belong to a different branch. if list[i] == stateID || l.descendant(stateID, list[i]) { - return list[i] + return list[i], true } } // No layer matching the stateID or its descendants was found. Use the // current disk layer as a fallback. if base == stateID || l.descendant(stateID, base) { - return base + return base, true } // The layer associated with 'stateID' is not the descendant of the current // disk layer, it's already stale, return nothing. - return common.Hash{} + return common.Hash{}, false } // addLayer traverses the state data retained in the specified diff layer and