From 4cbf0e314be41d0bb82ea98c4c6c55b66b73f5c8 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Wed, 15 Apr 2026 14:49:50 +0200 Subject: [PATCH] core/state: make bintrieFlatReader authoritative for non-membership Remove the errBintrieFlatStateMiss sentinel error that forced a trie traversal for every absent key. The pathdb diskLayer already gates on genMarker: uncovered keys return errNotCoveredYet (propagated as a non-nil error before the flat reader's absence check), so by the time AccountRLP returns (nil, nil), the key is within the generated region and genuinely absent. Return (nil, nil) directly, matching the MPT flatReader's authoritative pattern and eliminating the double-lookup penalty. --- core/state/reader.go | 46 ++++-------------------- core/state/reader_bintrie_oracle_test.go | 9 +++-- core/state/reader_bintrie_test.go | 37 +++++++------------ 3 files changed, 24 insertions(+), 68 deletions(-) diff --git a/core/state/reader.go b/core/state/reader.go index 27102602ea..833253207d 100644 --- a/core/state/reader.go +++ b/core/state/reader.go @@ -180,23 +180,6 @@ func (r *flatReader) Storage(addr common.Address, key common.Hash) (common.Hash, return value, nil } -// errBintrieFlatStateMiss is returned by bintrieFlatReader's Account -// and Storage methods whenever the flat state has no data for the -// queried key. Returning a non-nil error (rather than (nil, nil) or -// the zero hash) is essential because multiStateReader.Account/Storage -// short-circuits on the first err == nil result — if bintrieFlatReader -// returned "absent" as a success value, the trie reader would never -// run, and any flat-state corruption, in-transition state, or missing -// stem would be silently misreported as "account does not exist". -// -// This sentinel triggers the fall-through to the next reader in the -// chain (typically the trieReader), which serves as the gatekeeper for -// distinguishing "genuinely absent" from "temporarily missing from -// flat state". The merkle flatReader has the same contract via -// pathdb's internal errNotCoveredYet; we define a local sentinel to -// avoid a cross-package import cycle with triedb/pathdb. -var errBintrieFlatStateMiss = errors.New("bintrie flat state: entry not covered") - // bintrieFlatReader is the binary-trie analogue of flatReader. It exposes // the StateReader interface backed by the path database's per-stem flat // state, doing the EIP-7864 key derivation locally so the underlying @@ -254,15 +237,8 @@ func newBintrieFlatReader(reader database.StateReader) *bintrieFlatReader { // Return value contract: // - both leaves 32 bytes → decoded Account, nil error. // - either leaf invalid length → corruption error, surfaced as-is. -// - both leaves absent (nil from AccountRLP) → errBintrieFlatStateMiss -// sentinel. This does NOT mean "account does not exist" — it means -// "the flat state has no data for this stem right now" (possibly -// because the generator hasn't reached it, or because the account -// has simply never been touched by a block that flushed). The -// multiStateReader is responsible for falling through to the trie -// reader, which is the authoritative source. Returning (nil, nil) -// here would short-circuit the fall-through and silently hide -// every corruption mode the other A-commits are fixing. +// - both leaves absent → (nil, nil): authoritative non-membership. +// Uncovered keys already fail with errNotCoveredYet at the pathdb layer. func (r *bintrieFlatReader) Account(addr common.Address) (*Account, error) { basicKey := common.BytesToHash(bintrie.GetBinaryTreeKeyBasicData(addr)) codeKey := common.BytesToHash(bintrie.GetBinaryTreeKeyCodeHash(addr)) @@ -276,10 +252,7 @@ func (r *bintrieFlatReader) Account(addr common.Address) (*Account, error) { return nil, fmt.Errorf("bintrie CodeHash read %x: %w", addr, err) } if len(basicBlob) == 0 && len(codeBlob) == 0 { - // Flat state has no data for this stem. Fall through to the - // next reader in the multi-reader chain (trie reader) rather - // than silently returning "not found". - return nil, fmt.Errorf("%w: addr=%x", errBintrieFlatStateMiss, addr) + return nil, nil // Authoritative absence: pathdb confirmed key is covered } // A bintrie leaf is always either absent or exactly 32 bytes. A // shorter blob is a corruption signal; surface it with enough @@ -322,11 +295,9 @@ func (r *bintrieFlatReader) Account(addr common.Address) (*Account, error) { // Return value contract: // - 32-byte leaf found → decode as common.Hash and return. // - invalid-length leaf → corruption error. -// - no leaf at this slot → errBintrieFlatStateMiss sentinel so -// multiStateReader falls through to the trie reader. A slot that -// was explicitly set to zero is NOT absent — the bintrie tombstone -// convention writes 32 zero bytes, which is a present 32-byte leaf -// and returns common.Hash{} via the normal path. +// - no leaf → (common.Hash{}, nil): authoritative non-membership. +// A slot explicitly set to zero is NOT absent — the bintrie +// tombstone convention writes 32 zero bytes (a present leaf). func (r *bintrieFlatReader) Storage(addr common.Address, slot common.Hash) (common.Hash, error) { fullKey := bintrie.GetBinaryTreeKeyStorageSlot(addr, slot[:]) blob, err := r.reader.AccountRLP(common.BytesToHash(fullKey)) @@ -334,10 +305,7 @@ func (r *bintrieFlatReader) Storage(addr common.Address, slot common.Hash) (comm return common.Hash{}, fmt.Errorf("bintrie storage read %x[%x]: %w", addr, slot, err) } if len(blob) == 0 { - // Flat state has no entry at this slot. Fall through to the - // trie reader to distinguish "never written" from "written - // then flushed out". See errBintrieFlatStateMiss. - return common.Hash{}, fmt.Errorf("%w: addr=%x slot=%x", errBintrieFlatStateMiss, addr, slot) + return common.Hash{}, nil // Authoritative absence: pathdb confirmed key is covered } if len(blob) != 32 { return common.Hash{}, fmt.Errorf("bintrie storage leaf invalid length: addr=%x slot=%x len=%d want=32", addr, slot, len(blob)) diff --git a/core/state/reader_bintrie_oracle_test.go b/core/state/reader_bintrie_oracle_test.go index 33e42a7e95..5302c4c618 100644 --- a/core/state/reader_bintrie_oracle_test.go +++ b/core/state/reader_bintrie_oracle_test.go @@ -97,11 +97,10 @@ func TestBintrieFlatStateConsistencyOracle(t *testing.T) { t.Fatalf("block %d: StateReader: %v", blockNum, err) } - // For each known address, read via the flat reader. The flat - // reader may return errBintrieFlatStateMiss (which the - // multiStateReader falls through to the trie reader for), so - // the final answer comes from the highest-priority reader that - // has data. We compare the FINAL answer to what we expect. + // For each known address, read via the multiStateReader which + // tries the flat reader first (authoritative for covered keys) + // and falls through to the trie reader only if the pathdb + // returns errNotCoveredYet for keys not yet generated. for _, a := range addrs { got, err := flatReader.Account(a) if err != nil { diff --git a/core/state/reader_bintrie_test.go b/core/state/reader_bintrie_test.go index 518b82242a..dfb18aa899 100644 --- a/core/state/reader_bintrie_test.go +++ b/core/state/reader_bintrie_test.go @@ -17,7 +17,6 @@ package state import ( - "errors" "testing" "github.com/ethereum/go-ethereum/common" @@ -135,22 +134,9 @@ func TestBintrieFlatReaderEndToEnd(t *testing.T) { } } -// TestBintrieFlatReaderMissingAccountSentinel verifies that the bintrie -// flat reader returns errBintrieFlatStateMiss (a non-nil error sentinel) -// for an account that was never written to the flat state. -// -// Post-A2: the flat reader returns errBintrieFlatStateMiss so the -// multiStateReader falls through to the trie reader. This is the -// correct behavior: the flat state does not have the entry, so the -// trie reader should be the gatekeeper. -// -// KNOWN ISSUE: BinaryTrie.GetAccount does NOT verify stem membership — -// it returns the closest stem's data for ANY address query. So the trie -// reader currently returns wrong data for non-existent addresses. That -// is a pre-existing bintrie bug (not introduced by A2). This test -// therefore verifies the FLAT READER's sentinel error directly, in -// isolation from the buggy trie reader fallthrough path. -func TestBintrieFlatReaderMissingAccountSentinel(t *testing.T) { +// TestBintrieFlatReaderMissingAccountAuthoritative verifies that the flat +// reader returns (nil, nil) for absent accounts after generation completes. +func TestBintrieFlatReaderMissingAccountAuthoritative(t *testing.T) { disk := rawdb.NewMemoryDatabase() tdb := triedb.NewDatabase(disk, triedb.VerkleDefaults) sdb := NewDatabase(tdb, nil) @@ -166,10 +152,13 @@ func TestBintrieFlatReaderMissingAccountSentinel(t *testing.T) { if err != nil { t.Fatalf("commit: %v", err) } + // Flush to disk so the generator completes (genMarker → nil). + if err := tdb.Commit(root, false); err != nil { + t.Fatalf("tdb.Commit (flush to disk): %v", err) + } // Get the pathdb reader so we can test the bintrieFlatReader in - // isolation — not through multiStateReader (which would fall - // through to the buggy trie reader). + // isolation. pathdbReader, err := tdb.StateReader(root) if err != nil { t.Fatalf("pathdb StateReader: %v", err) @@ -180,12 +169,12 @@ func TestBintrieFlatReaderMissingAccountSentinel(t *testing.T) { } missing := common.HexToAddress("0xfeedfacefeedfacefeedfacefeedfacefeedface") - _, flatErr := br.Account(missing) - if flatErr == nil { - t.Fatal("expected errBintrieFlatStateMiss for missing account, got nil error") + acct, err := br.Account(missing) + if err != nil { + t.Fatalf("expected authoritative nil for missing account, got error: %v", err) } - if !errors.Is(flatErr, errBintrieFlatStateMiss) { - t.Fatalf("expected errBintrieFlatStateMiss, got: %v", flatErr) + if acct != nil { + t.Fatalf("expected nil account for missing address, got: %+v", acct) } }