Part 2: Bigtable node read fallback

[Kbhat1]

Description

• Wraps the composite state store: Get/Has below the local prune horizon route to Bigtable; recent state, writes, pruning, and iterators stay local.
• Coverage-gated for safety: an operator-declared floor opens RPC height gates, and reads ahead of ingestion error instead of returning empty state.
• LRU read cache + per-outcome metrics (cache hit / backend hit / miss / behind) to keep latency and Bigtable read costs measurable.

Testing

• Verified on node

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Jul 21, 2026, 6:12 PM

[codecov]

Codecov Report

❌ Patch coverage is 71.35678% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.04%. Comparing base (03c410b) to head (6cc315f).

Files with missing lines	Patch %	Lines
sei-db/state_db/ss/offload/historical/store.go	74.66%	31 Missing and 7 partials ⚠️
sei-db/state_db/ss/store.go	31.81%	14 Missing and 1 partial ⚠️
sei-db/state_db/ss/composite/store.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                  @@
##           mvcc-bigtable-pt1    #3437      +/-   ##
=====================================================
+ Coverage              58.93%   59.04%   +0.11%     
=====================================================
  Files                   2215     2206       -9     
  Lines                 181401   180855     -546     
=====================================================
- Hits                  106911   106791     -120     
+ Misses                 65092    64653     -439     
- Partials                9398     9411      +13     

Flag	Coverage Δ
sei-chain-pr	51.43% <100.00%> (+10.72%)	⬆️
sei-db	70.41% <ø> (ø)
sei-db-state-db-pr	68.31% <70.15%> (+7.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
app/seidb.go	81.91% <100.00%> (+1.68%)	⬆️
sei-db/config/ss_config.go	100.00% <ø> (ø)
sei-db/state_db/ss/offload/historical/metrics.go	100.00% <100.00%> (ø)
sei-db/state_db/ss/composite/store.go	70.27% <0.00%> (ø)
sei-db/state_db/ss/store.go	34.78% <31.81%> (ø)
sei-db/state_db/ss/offload/historical/store.go	74.66% <74.66%> (ø)

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cursor]

PR Summary

High Risk
Changes historical state read semantics for pruned heights and depends on correct Bigtable coverage configuration; mis-set earliest version or consumer lag can cause errors or wrong empty reads on critical query paths.

Overview
Adds node-side historical read fallback so pruned Get/Has can be served from Bigtable while recent state, writes, pruning, and iterators stay on local SS.

Configuration: New [state-store] flags wire Bigtable connection settings and an optional historical-offload-earliest-version coverage floor (manual app.toml only—not in the default template). NewStateStore wraps the composite store with FallbackStateStore when historical-offload-backend = "bigtable".

FallbackStateStore behavior: Routes point reads below the local prune horizon (per-store via GetEarliestVersionForKey on composite). Uses an LRU cache, 10s read timeouts, TTL’d miss caching, and refuses reads ahead of the backend ingestion watermark (errors instead of empty state). GetEarliestVersion can advertise the coverage floor so RPC height gates admit pruned heights. OTel counter historical_fallback_reads_total by op/outcome.

Docs/tests: Consumer README documents operator setup and limits (no iterator offload). Unit tests cover routing, caching, coverage floor, backend lag, and unsupported backends.

^{Reviewed by Cursor Bugbot for commit 6cc315f. Bugbot is set up for automated code reviews on this repo. Configure here.}

[seidroid]

A well-structured, well-tested addition of a Bigtable historical-state offload for pruned point reads. Two correctness concerns (both surfaced by Codex and confirmed here) can cause the node to serve stale/empty or incomplete state at the ingestion frontier and for admitted range queries, and should be resolved or explicitly constrained before merge.

Findings: 2 blocking | 4 non-blocking | 2 posted inline

Blockers

• None at the file/PR level.
• 2 blocking issue(s) flagged inline on specific lines.

Non-blocking

• Shard/family coupling is a sharp operational footgun: bigtableShard uses h.Sum32() % shards, so if the node's historical-offload-bigtable-shards/family differ from what the consumer wrote with, every fallback read silently misses (returns empty state) rather than erroring. It's documented in the README, but consider persisting the shard count/family in Bigtable metadata and validating it on OpenBigtableClient so a mismatch fails loudly.
• BigtableConfig.Configured() returns true if ANY of ProjectID/InstanceID/Table is set (OR), while Validate() requires all of them (AND). A partial config (e.g. only project id) therefore aborts node startup via NewStateStore error instead of being ignored. This is reasonable fail-fast behavior, but worth confirming it's intended vs. silently disabling the fallback.
• Second-opinion coverage: cursor-review.md was empty (Cursor pass produced no output) and REVIEW_GUIDELINES.md was empty/missing, so no repo-specific review standards were applied. Codex's two P1 findings were incorporated and independently confirmed.
• writeVersionMarkers issues a single un-chunked applyBulk for all records in a batch (unlike record rows, which are chunked). Fine at the default MaxBatchRecords=128, but a large operator-configured batch could exceed practical MutateRows limits; consider chunking markers too for defense in depth.

[seidroid]

[blocker] BigtableLastVersion returns the max version marker across all 64 buckets, i.e. the maximum version ingested anywhere, not the highest contiguously-ingested version. Because the consumer shards by Kafka partition and processes partitions concurrently (Consumer in consumer.go preserves order only within a partition), a fast partition can publish a marker for version V1 while a lower version V0 < V1 from another partition is not yet written. ensureBackendCoverage (store.go) only refuses reads above this value, so a point read at a height H with V0 <= H < V1 is admitted even though the key's latest write at V0 hasn't been ingested — the scan then returns an older/empty value that is cached as a hit/miss (miss for up to missCacheTTL), silently serving stale or missing state. This is only safe if the offload topic is single-partition; otherwise consider tracking a contiguous ingestion watermark (min across in-flight partitions / gap-free frontier) rather than the max marker, or documenting and enforcing the single-partition requirement. (Confirms Codex P1.)

[seidroid]

[blocker] GetEarliestVersion advertises coverageFloor globally (below the local prune horizon) to open height gates, but Iterator/ReverseIterator/RawIterate (lines 280-290) delegate straight to the pruned primary and never use the fallback. So a range query at a height between coverageFloor and the local horizon passes the gate yet silently returns incomplete/empty results from local SS. The comment and README "Current Limits" acknowledge this, but a gate that admits heights the store cannot correctly serve is a real correctness hazard for any query path that gates on earliest-version then iterates. Consider having the iterator paths reject (error) versions below the local horizon rather than returning partial data, so callers get a clear failure instead of silently wrong results. (Confirms Codex P1.)

[claude]

🟡 In ensureBackendCoverage, the LastVersion coverage-check RPC and the subsequent reader.Get/Has point-read RPC share the same 10s context created by readContext(), even though the readTimeout doc comment says it 'bounds one backend point read.' A slow-but-healthy LastVersion refresh (Bigtable: up to 4 sequential waves across 64 version buckets, each retried up to 3x on Unavailable) can consume most of that budget and cause the point read to fail with context deadline exceeded even though the backend could have served it. Consider giving the coverage check its own timeout instead of reusing the outer read deadline.

Extended reasoning...

The bug. FallbackStateStore.Get/Has (store.go:176-182 and the Has analog) create a single context.Context via readContext(), which applies the package-level readTimeout constant (10s). That one context is passed first into ensureBackendCoverage(ctx, version) and then, if coverage passes, into reader.Get(ctx, ...) / reader.Has(ctx, ...). Both RPCs therefore draw from the same 10-second deadline, even though the readTimeout doc comment explicitly states it "bounds one backend point read."

The code path that triggers it. ensureBackendCoverage (store.go:156-178) is only a no-op when version <= s.backendLast.Load(). Once every ~30s (backendVersionRecheckInterval) for a version above the cached watermark, it calls reader.LastVersion(ctx) using the same context. For the Bigtable reader this is not cheap: BigtableLastVersion scans all 64 version buckets via an errgroup capped at 16 concurrent workers — i.e. up to 4 sequential waves of RPCs — and each individual bucket read is retried up to 3x on Unavailable with growing backoff (readRowsWithRetry). If the backend is partially degraded (e.g. GFE restarts, tablet moves) such that reads are slow/retrying but still eventually succeed, this multi-RPC health check can burn several seconds of the shared 10s budget. Only after it returns does reader.Get/reader.Has run against whatever time remains on the same context, so a point read that would have easily finished within its own fresh 10s window instead fails immediately with context deadline exceeded.

Why nothing else catches this. The existing test (TestFallbackStateStoreRejectsReadsBeyondBackendCoverage and related tests in store_test.go) uses a fake reader whose LastVersion and Get/Has return instantly, so no test exercises the case where LastVersion is slow but still succeeds within the shared deadline. The doc comment on readTimeout describes single-RPC intent, but the implementation never enforces it — it silently bounds two sequential RPCs when a coverage recheck fires.

Impact. The failure mode is a spurious, retryable error (context deadline exceeded, surfaced as the generic fallbackOutcomeError) rather than any data corruption or crash — the caller can just retry, and the next read within the 30s window skips LastVersion entirely (since backendLast was already refreshed or the throttle hasn't elapsed) and gets the point read's full 10s. The trigger window is also fairly narrow: it only matters on a read whose version is above backendLast, once per 30s recheck, and only when LastVersion is slow-but-successful (a hard failure returns errBackendBehind/error immediately regardless of the timeout split). In steady state (version ≤ backendLast), ensureBackendCoverage returns immediately and the point read gets the entire 10s uncontested. This narrow blast radius, combined with the fact that the "fix" (giving each RPC its own timeout) trades off against doubling worst-case caller latency to 20s, is why this is best classified as a minor robustness gap rather than a blocking issue.

Step-by-step proof. (1) A read for version = V arrives where V > backendLast and the 30s recheck interval has elapsed. (2) Get calls readContext(), getting a context with a 10s deadline T. (3) Get calls ensureBackendCoverage(ctx, V), which acquires backendMu and calls reader.LastVersion(ctx). Suppose the backend is in a state where each of the 4 sequential 16-worker waves takes ~1.5s due to a few slow/retrying buckets — this alone consumes ~6s of the 10s budget. (4) LastVersion still succeeds, returning a watermark ≥ V, so ensureBackendCoverage returns nil. (5) Control returns to Get, which now calls reader.Get(ctx, storeKey, key, V) — but ctx now has only ~4s left instead of a fresh 10s. (6) If the point-read RPC itself needs, say, 5s under the same partial degradation (a single-row lookup should normally be fast, but network/backend slowness affects it too), it now fails with context deadline exceeded even though a dedicated 10s window would have let it succeed.

Suggested fix. Derive a fresh, independent timeout for the LastVersion coverage check (e.g. its own shorter deadline) rather than reusing the same context/deadline that's about to be handed to the point read, so the two sequential RPCs don't compete for one shared budget.

[seidroid]

Part 2/2 wiring of the Bigtable historical read fallback: a FallbackStateStore that routes pruned point reads to Bigtable with an LRU cache, coverage gating, and per-store-key prune horizons, plus config/flags/README. The changed code is clean and well-tested; the main caveat is the documented point-read-only scope — advertising the coverage floor while iterators stay on the pruned local store can silently return incomplete range results — plus dependence on part-1 mechanisms that Codex flagged as risky.

Findings: 0 blocking | 5 non-blocking | 1 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• REVIEW_GUIDELINES.md is empty/missing, so no repo-specific guidelines were applied. The Cursor second-opinion pass (cursor-review.md) was also empty — it produced no output.
• Codex P1 (consumer/bigtable.go:145 — version markers written independently by concurrent partition workers, so a higher LastVersion marker can appear before lower-version mutations) and Codex P1 (historical/bigtable.go:396 — reads ignore persisted store rename/upgrade rows) are in part-1 files NOT changed by this PR, so they're out of scope to fix here. However, this PR's coverage gate (ensureBackendCoverage relies on reader.LastVersion) and served reads depend on those mechanisms being correct; verify them before enabling the fallback in production, since a premature LastVersion could let a read at a not-yet-fully-ingested version pass the gate and return incomplete state.
• Minor: cacheValue skips caching when v.Bytes is nil (historical/store.go:236), so a backend 'found' result with a nil/empty value is re-fetched on every read rather than cached. Low impact, but worth a comment on the intended semantics of an existing-but-empty value.
• Minor test gap: metrics_test.go TestFallbackMetricsRecordNoPanic omits fallbackOutcomeBackendBehind from the exercised outcomes; consider including it for completeness.
• 1 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] GetEarliestVersion advertises the coverage floor globally so RPC height gates admit pruned heights, but Iterator/ReverseIterator/RawIterate only ever delegate to the pruned primary. A query at a height in [coverageFloor, localEarliest) that passes the advertised-earliest gate and then iterates (e.g. a Cosmos gRPC prefix/state query, or any range scan) will silently return incomplete or empty data rather than erroring. This matches Codex's P2 and is acknowledged in the comment and README as a known limitation, but it's a real correctness footgun for iteration-based historical queries. Consider erroring (or otherwise signaling unsupported) on iteration below the local prune horizon so callers don't consume silently-partial results.

Superseded: latest AI review found no blocking issues.

[yzang2019]

I don't think we want to expose these config yet since it's not for mainnet. We should remove these from the config toml file to avoid being automatically generated

[yzang2019]

shouldFallback silently returns empty state for pruned keys below coverageFloor.

When version < coverageFloor, shouldFallback returns false, routing the read to the primary store. But the primary already pruned that version — that's why fallback exists. The caller gets back an empty []byte, nil with no indication the data was silently dropped. This seems to be a correctness bug: the node reports "key doesn't exist" for data that does exist in Bigtable.

[yzang2019]

StateStore.Get/Has don't accept a context.Context. The internal readTimeout of 10s is hardcoded in newReader. If a block takes 200ms to finalize but a Bigtable read stalls, the node's state-query path blocks for 10s with no way for the caller to cancel. This also means graceful shutdown can't drain in-flight reads.

Consider a context-accepting internal path even if the StateStore interface can't change yet.

[yzang2019]

cacheValue drops any value exceeding maxCacheableValueSize (8 KB). There's no metric or log for this. If a frequently-read key (e.g., a large EVM contract state) exceeds 8 KB, every read bypasses the cache and hits Bigtable. Under load this becomes the dominant source of backend traffic, and the operator has no visibility into it.

We can emit a metric (fallback_cache_skip_oversize) counting skipped entries. Consider a separate size-aware cache (e.g., groupcache or a byte-budget LRU) for large values instead of silently dropping them.

[yzang2019]

GetEarliestVersion() returns coverageFloor when the primary's earliest is higher, implying the store can serve data from coverageFloor onward. But Iterator() and ReverseIterator() delegate straight to primary and never query Bigtable. A range query between coverageFloor and the primary's actual earliest version returns zero rows, violating the contract implied by GetEarliestVersion.

[yzang2019]

Every shouldFallback call hits ensureBackendCoverage, which grabs backendMu and does a synchronous reader.LastVersion() RPC when the check interval expires. During that RPC (up to 10s timeout), every goroutine doing a fallback read blocks on the mutex. Under high read concurrency this might become a bottleneck

[yzang2019]

Suggestion:

1. Move these configs to one struct
2. Add a FallbackSSBackend = "BigTable" config, and default to empty (meaning disabled)

[yzang2019]

The common access pattern would be Has-then-Get, but then the issue here is that it would end up hitting BitTable twice.

Fix: either make Has() also fetch and cache the value, or let Get() recognize a found=true, valueKnown=false entry and upgrade it in-place rather than treating it as a full miss.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 6cc315f. Configure here.}

[cursor]

Coverage check blocks all readers

High Severity

ensureBackendCoverage holds backendMu across the synchronous LastVersion RPC, which fans out across many Bigtable buckets and can run up to the 10s read timeout. Every concurrent fallback Get/Has that needs a coverage check blocks on that mutex for the full RPC duration, creating a process-wide stall under historical read load.

 

^{Reviewed by Cursor Bugbot for commit 6cc315f. Configure here.}

[seidroid]

Well-structured, thoroughly tested opt-in "Part 2" that wraps the composite state store to route pruned point reads to a Bigtable historical backend. The one substantive concern is that setting the coverage floor advertises pruned heights as queryable while iterator/range reads still hit only the pruned local store, so range-style RPC queries at those heights can silently return incomplete results — a limitation the author documents but that warrants an explicit gate or confirmation of the exposed RPC surface.

Findings: 0 blocking | 5 non-blocking | 1 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Coverage/iterator gap (see inline on store.go:304): HistoricalOffloadEarliestVersion opens RPC height gates for pruned heights, but Iterator/ReverseIterator/RawIterate delegate to the pruned primary, so range-based archive queries in [floor, local-earliest) can silently return empty/incomplete state. This matches Codex's High finding and the author's own documented limitation. Point reads are served correctly; the risk is confined to iterator-based query paths. Recommend either rejecting iterator reads below the local prune horizon when a coverage floor is active, or confirming that every RPC endpoint exposed by opening the gate is point-read only.
• metrics.go newFallbackMetrics discards the error from meter.Int64Counter (reads, _ := ...). OTel returns a no-op instrument on error so recordRead stays panic-safe (also covered by TestFallbackMetricsRecordNoPanic), but the counter could silently never record; consider logging the error.
• Second-opinion input: cursor-review.md was empty (no Cursor output produced); Codex's single finding is incorporated above. Note for maintainers, not a code issue.
• Values larger than maxReadCacheValueBytes (8 KiB) are never cached, so large historical values re-hit the backend on every read. This is an intentional memory bound and documented in-code; flagging only for awareness of the read-cost tradeoff.
• 1 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] Advertising the coverage floor as the earliest version opens the RPC height gate for all query types at pruned heights, but Iterator/ReverseIterator/RawIterate (store.go:280-290) delegate straight to the pruned primary. Point reads fall back to the backend correctly, but iterator/range queries in [coverageFloor, local-earliest) will silently return empty or partial results rather than an error — a successful-but-wrong response for archive queries such as prefix/range scans (e.g. "all balances for an account"). You document this ("No offload iterator path...") and it's opt-in/off-by-default, so it's a scoping decision rather than an outright bug, but it's a real correctness footgun. Consider rejecting iterator reads below the local prune horizon when a coverage floor is set, or explicitly confirming that every RPC path unlocked by the floor is point-read only. (Matches Codex's High finding.)

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 sei-db/queue/kafka/consumer.go:195-211 — Decode errors (permanently malformed Kafka payloads) are now retried up to 5x with exponential backoff (~15s) before failing, instead of failing immediately as before the refactor — because decoding moved from Consumer.processBatch into Sink.WriteBatch (e.g. bigtableSink.WriteBatch), which is called from the generic writeBatchWithRetry retry loop.

  Extended reasoning...

  The regression. Before this PR, Consumer.processBatch called DecodeEntry on every Kafka message before invoking writeBatchWithRetry, so a permanently malformed/corrupt protobuf payload returned a decode error immediately, with zero retries, since decode failures are deterministic and non-transient. After the refactor, decoding was moved inside Sink.WriteBatch — bigtableSink.WriteBatch (sei-db/state_db/ss/offload/consumer/bigtable.go) calls DecodeEntry(msg.Value) as its very first step — and WriteBatch is now invoked from writeBatchWithRetry (sei-db/queue/kafka/consumer.go:230-259), which retries any non-cancellation error up to sinkMaxAttempts (5) times with exponential backoff (1s, 2s, 4s, 8s ≈ 15s total).

  The code path. Since msgs passed into writeBatchWithRetry never changes between attempts, a message that fails to decode fails identically on every attempt. The retry loop has no way to distinguish this permanent, payload-inherent error from a transient backend error (e.g. a Bigtable RPC blip) — it only short-circuits on isCancellation(err). So a decode failure burns ~15s of sleeping and 4 redundant calls into WriteBatch (each re-decoding the entire batch) before returning the exact same error it hit on attempt 1.

  Why nothing else catches it. writeBatchWithRetry is a generic, sink-agnostic retry helper; it has no concept of 'permanent vs transient' errors from the sink, and Sink.WriteBatch's error return doesn't carry that distinction either.

  Step-by-step proof. (1) A corrupted/malformed message reaches the consumer (bad protobuf bytes in msg.Value). (2) processBatch calls writeBatchWithRetry(ctx, msgs) with that message in the batch. (3) Attempt 1: sink.WriteBatch calls DecodeEntry(msg.Value), which fails with a decode error; not a cancellation, so the loop logs and sleeps 1s. (4) Attempt 2: identical failure, sleeps 2s. (5) Attempts 3-4: identical failures, sleeping 4s then 8s. (6) Attempt 5: identical failure; loop exhausts attempts and returns fmt.Errorf("sink write failed after %d attempts: %w", ...). (7) This propagates out of workerLoop into the errgroup in Consumer.Run, cancelling the shared context and stopping every other worker/shard, and main.go calls log.Fatalf, crashing the process — the same terminal outcome as before the refactor, just ~15-30s later and with 4 extra wasted WriteBatch calls (each re-decoding the whole batch) in between.

  Impact and why this is a nit, not a blocker. The final outcome is unchanged: the process still crashes via log.Fatalf either way, so there's no incorrect behavior, data loss, or silent failure — only a bounded ~15-30s delay before the crash and some wasted CPU/redundant sink calls in a standalone ops tool (the historical-offload-consumer), not the node's serving path. Triggering it also requires a corrupted/undecodable payload, which given that producers marshal valid protobuf via gogoproto.Marshal before publishing, should be rare (transport corruption or a hostile/buggy producer).

  Suggested fix. Decode once, outside the retryable Sink.WriteBatch call (e.g. back in the consumer loop before calling writeBatchWithRetry), or have Sink implementations return a distinguishable permanent-error type/sentinel so writeBatchWithRetry can skip retrying decode failures the same way it already skips retrying cancellations.

[claude]

🟡 Has() caches readCacheValue{found: true} without the value (valueKnown stays false), so a subsequent Get() on the same key sees a cache entry it can't use and falls through to a full backend Get — doubling Bigtable reads for the common Has-then-Get access pattern used by Cosmos SDK store code. The maintainer flagged this directly in review; fix by either having Has() fetch-and-cache the value, or having Get() upgrade a found=true/valueKnown=false entry in place instead of treating it as a miss.

Extended reasoning...

The bug. cacheHas (sei-db/state_db/ss/offload/historical/store.go:246-248) stores readCacheValue{found: true} and leaves valueKnown at its zero value, false. When Get() later looks up the same (storeKey, version, key), it calls getCachedValue (store.go:196-212), which checks value.found (true) and then hits if !value.valueKnown { return nil, false, false } — reporting a full cache miss even though Has() already confirmed the key exists. Get() then proceeds to issue a fresh reader.Get RPC against the historical backend.

Why this matters for this PR specifically. Cosmos SDK store access very commonly does Has() immediately followed by Get() on the same key (e.g. existence checks before a read). For any such pair on a pruned/historical key, this fallback store hits the backend twice — once implicitly confirmed by Has(), and again for Get() — even though the two calls are for the exact same version and key. Since the whole point of the read cache is to keep Bigtable read cost and latency down, this gap undermines that goal for one of the most frequent access patterns.

Why nothing else catches it. This isn't just missing test coverage — it's asserted behavior. TestFallbackStateStoreDoesNotUseHasOnlyCacheForGet in store_test.go explicitly does Has() then Get() and asserts reader.gets == 1, i.e. it documents that Get() still reaches the backend after Has() already ran. The asymmetry is notable: the reverse order (Get() then Has()) is optimized, since cacheValue sets found + valueKnown + value together, and getCachedHas only needs value.found. It's specifically the Has-then-Get order that falls through.

Step-by-step proof.

1. A pruned key k at version v does not yet have a cache entry.
2. Caller invokes Has("bank", v, k). This is below the local prune horizon, so it's routed to ensureBackendCoverage + reader.Has, which returns true. cacheHas stores readCacheValue{found: true} (valueKnown left false) under the composite cache key.
3. Immediately after, the same caller invokes Get("bank", v, k) for the same key/version — the common Cosmos SDK existence-then-read pattern.
4. Get() calls getCachedValue, which reads the cache entry from step 2: value.found == true, but value.valueKnown == false, so the guard clause returns (nil, false, false) — reported as "not in cache."
5. Get() treats this as a cache miss, calls ensureBackendCoverage again and then reader.Get(ctx, "bank", k, v), issuing a second RPC to Bigtable for data whose existence was already established one call earlier.
6. Net effect: 2 backend RPCs for what could have been 1, for every Has-then-Get pair on a pruned key.

Maintainer confirmation. yzang2019 flagged this exact issue in PR review: "The common access pattern would be Has-then-Get, but then the issue here is that it would end up hitting BigTable twice... Fix: either make Has() also fetch and cache the value, or let Get() recognize a found=true, valueKnown=false entry and upgrade it in-place rather than treating it as a full miss." All verifiers independently traced the same code path and reached the same conclusion.

Fix. Either (a) have Has() perform the full value fetch and populate valueKnown/value (trading a slightly heavier Has() for a cheaper subsequent Get()), or (b) have getCachedValue treat a found=true, valueKnown=false entry as a signal to fetch-and-upgrade the cache entry in place rather than as an unconditional miss.

Severity note. This is an efficiency gap, not a correctness bug: Get() still returns the right answer, just after an avoidable extra RPC. No wrong data, crash, or data loss results, and only the pruned/historical-fallback read path is affected (not the hot local-store path). Per the maintainer's own framing this is worth fixing but not blocking.

[claude]

Beyond the two inline findings, I also examined the concern yzang2019 raised in review about shouldFallback routing reads below coverageFloor to the already-pruned primary: that's expected rather than a silent-data-loss bug, since coverageFloor demarcates where the backend guarantees ingestion coverage, so a read below it has no safe fallback destination either way (it's not distinct from the local prune horizon case the design already accepts).

Extended reasoning...

yzang2019's review comment (2026-07-20) flagged shouldFallback returning false for version < coverageFloor as a correctness bug, reasoning that the primary has already pruned that data so the caller silently gets an empty result. This run's finder agents re-examined that exact code path and verifiers refuted it: coverageFloor is defined as the version below which the backend does not guarantee full ingestion coverage, so a read below the floor has no backend it could safely be routed to either — declining to fall back there is consistent with the same 'no fallback destination' situation that exists below the local prune horizon when no backend is configured at all, not a newly-introduced silent-drop path. This is recorded here since the maintainer's comment is still open in the thread with no visible code change addressing it, and it's worth noting it was independently re-checked and found not to be an additional bug beyond the two already posted as inline comments.

[claude]

🔴 GetEarliestVersionForKey("evm") delegates to EVMStateStore.GetEarliestVersion(), which under evm-ss-separate-dbs takes the MINIMUM earliest version across the 5 independently-pruned EVM sub-DBs — the least-pruned (most optimistic) value, not a safe lower bound. If one sub-DB lags behind the others (e.g. a transient prune failure, since EVMStateStore.Prune only logs errors and never retries), FallbackStateStore.shouldFallback will see the lagging sub-DB's earliest and skip the Bigtable fallback for a read that actually needs to be routed to a different, more-pruned sub-DB — that sub-DB's own MVCC self-gate then silently returns (nil, nil) even though the data exists in Bigtable.

Extended reasoning...

The bug. GetEarliestVersionForKey(storeKey) in sei-db/state_db/ss/composite/store.go:222-231 is new in this PR and exists specifically so FallbackStateStore.shouldFallback (sei-db/state_db/ss/offload/historical/store.go) can consult 'the horizon of the store that will actually serve the read' rather than a single cosmos-only aggregate, per its own doc comment. For storeKey == "evm" it returns s.evmStore.GetEarliestVersion() verbatim. That pre-existing method (sei-db/state_db/ss/evm/store.go:154-165) computes the minimum earliest version across all managedDBs:

func (s *EVMStateStore) GetEarliestVersion() int64 {
    var minVersion int64 = -1
    for _, db := range s.managedDBs {
        if v := db.GetEarliestVersion(); minVersion < 0 || v < minVersion {
            minVersion = v
        }
    }
    ...
}

Why MIN is wrong here. When evm-ss-separate-dbs is enabled, EVMStateStore holds 5 independently-pruned sub-DBs (nonce/storage/code/codeHash/balance), and Get/Has route each key to exactly one specific sub-DB via routeKey. Each sub-DB's own MVCC layer self-gates reads: if targetVersion < db.GetEarliestVersion() { return nil, nil } (pebbledb/mvcc/db.go:401-403). A safe fallback-routing watermark must be a value V such that 'version < V' implies 'every sub-DB that could serve this key has already pruned past version' — i.e. the maximum earliest across sub-DBs, not the minimum. Taking the minimum answers a different, more optimistic question ('has any sub-DB pruned this far'), which is not what shouldFallback needs.

The failure path. EVMStateStore.Prune fans out pruning to all sub-DBs concurrently via a WaitGroup and returns only the first error — every sub-DB's own prune goroutine runs to completion regardless of a sibling's failure. CompositeStateStore.Prune only logs an evmStore.Prune error, it never propagates or retries it. So a single transient failure (e.g. a disk I/O hiccup) in one sub-DB during one prune cycle leaves it behind while its siblings successfully advance. Concretely: code sub-DB fails to prune and stays at earliest=500, while storage successfully advances to earliest=1000. GetEarliestVersion() now reports MIN(500, 1000, ...) = 500.

Step-by-step proof. (1) A read arrives for a storage-family EVM key at version 700, with a Bigtable backend configured and covering this range. (2) FallbackStateStore.shouldFallback("evm", 700) calls earliestVersionFor("evm") → GetEarliestVersionForKey("evm") → evmStore.GetEarliestVersion() = 500. (3) Since 700 < 500 is false, shouldFallback returns false, so the read is not routed to Bigtable and instead goes to the local primary. (4) EVMStateStore.Get routes the key to the storage sub-DB (earliest=1000) via routeKey. (5) That sub-DB's MVCC self-gate checks 700 < 1000 → true → returns (nil, nil), a silent empty result, even though the row exists in Bigtable and would have been served correctly had shouldFallback returned true.

Why nothing catches this today. Before this PR, EVMStateStore.GetEarliestVersion()'s MIN-vs-MAX choice was dormant: CompositeStateStore.GetEarliestVersion() (the only prior consumer, used by the RPC height gate) exclusively reads cosmosStore.GetEarliestVersion() and never touches the EVM aggregate. This PR's GetEarliestVersionForKey is the first caller that uses the EVM aggregate as a read-routing safety decision, so a previously-inert aggregation quirk becomes a genuine, if narrow, silent-data-loss path — precisely the kind of per-store divergence PerKeyEarliestVersioner was introduced to catch, just one level too shallow (it stops at the whole EVMStateStore, not the individual sub-DB that will actually serve the key).

Fix. Either change EVMStateStore.GetEarliestVersion() to take the maximum across managedDBs when used as a safe-to-read watermark (over-eager fallback is harmless since Bigtable rows are immutable), or have GetEarliestVersionForKey route through the specific sub-DB that will actually serve storeKey+key (not achievable today since FallbackStateStore only has the store key, not the specific key, so MAX is the more directly actionable fix).

Scope/severity note. This requires stacking two non-default, internal/Giga-only features (evm-ss-separate-dbs and the historical-offload fallback) plus a transient partial-prune failure creating sub-DB divergence, and it self-heals on the sub-DB's next successful prune cycle. Within that window, though, it is a genuine silent-empty read for data that exists in the historical backend, served over the exact RPC path (EVM height-gated reads) this PR is trying to make correct.

[claude]

🔴 Live writes of a zero-length value (e.g. store.Set(key, []byte{}), which x/staking/keeper/delegation.go does today for UBD/RED index keys) get silently mislabeled as tombstones once they pass through the Kafka offload round-trip, because proto.KVPair.Value is a proto3 bytes field that collapses a non-nil empty slice to nil on unmarshal. bigtableSink.mutationRow treats pair.Value == nil as a delete (writing only DeletedColumn=true, no value cell), so once such a key crosses the local prune horizon, FallbackStateStore.Get/Has (store.go:164-191, 250-282) read it back as not-found and cache that wrong answer as a miss for up to missCacheTTL — even though the local (unpruned) path correctly returns present-with-empty-value for the same key.

Extended reasoning...

The bug. proto.KVPair.Value (sei-db/proto/changeset.pb.go:25) is a proto3 bytes field. gogoproto omits a zero-length bytes field on the wire and restores it to nil on unmarshal — it cannot distinguish "was nil" from "was []byte{}". The offload pipeline always round-trips through this: the producer marshals with gogoproto before publishing to Kafka (sei-db/state_db/ss/offload/kafka.go:44) and the consumer unmarshals on receipt (sei-db/state_db/ss/offload/consumer/sink.go:22). So any KVPair whose real Value has length 0 arrives at the consumer with pair.Value == nil, indistinguishable from an actual delete at the value level (even though the separate Delete bool field correctly stays false).\n\nWhere it gets mislabeled. bigtableSink.mutationRow (sei-db/state_db/ss/offload/consumer/bigtable.go:126-148) computes deleted := pair.Delete || pair.Value == nil. For a live empty-value write this evaluates to true purely because of the Value == nil clause, so the sink writes only a DeletedColumn=true cell to Bigtable with no ValueColumn cell at all — a genuine live write is persisted as a pure tombstone.\n\nWhy this file is where it becomes observable. bigtableReader.Get/Has (sei-db/state_db/ss/offload/historical/bigtable.go:48-82) read that DeletedColumn=true row back and correctly-from-their-own-perspective report the key as deleted/absent. FallbackStateStore.Get (store.go:164-191) treats the reader's ErrNotFound as a real absence, returning (nil, nil) and caching a miss for up to missCacheTTL; FallbackStateStore.Has (store.go:250-282) returns false and caches the same miss. Crucially, the local (unpruned) SS path never has this problem: rootmulti.Store.flush() builds the changeset directly in memory from commitStore.PopChangeSet() and applies it via ApplyChangesetSync with no protobuf round-trip, so a genuinely-empty []byte{} value stays non-nil throughout and Get/Has on the primary correctly report found=true with an empty value. This PR is what first routes live point reads for a pruned key to the lossy Bigtable copy, so it's the point at which the pre-existing ingestion-side mislabeling starts producing observably wrong answers for callers.\n\nConcrete trigger. This is not hypothetical: sei-cosmos/x/staking/keeper/delegation.go:291,496-497 does store.Set(indexKey, []byte{}) for UBD/RED-by-validator-index keys today, with the comment "index, store empty bytes".\n\nStep-by-step proof.\n1. At height H, the staking keeper calls store.Set(indexKey, []byte{}), producing KVPair{Key: indexKey, Value: []byte{}, Delete: false}.\n2. This changeset is marshaled with gogoproto and published to Kafka; on unmarshal at the consumer, pair.Value is nil (proto3 empty-bytes round-trip), while pair.Delete is still correctly false.\n3. mutationRow computes deleted := pair.Delete || pair.Value == nil → true, and writes a Bigtable row with DeletedColumn=true and no value cell.\n4. Time passes and the local prune horizon advances past height H (shouldFallback in store.go:104-109 now routes reads at H to the historical reader).\n5. A caller does Has("staking", H, indexKey): bigtableReader.Has sees DeletedColumn=true and returns false; FallbackStateStore.Has returns false and caches the miss. Get("staking", H, indexKey) similarly returns (nil, nil).\n6. Before pruning, the same query against the primary would have returned Has=true, Get=([]byte{}, nil) — the answer has silently flipped to "never existed" and is now cached as a miss for up to a minute, served to every caller in that window.\n\nWhy nothing catches this today. shouldFallback/ensureBackendCoverage only gate on version and backend lag, not on value shape. The reader side (bigtableValueFromRow, bigtable.go:86-108) has the identical deleted || value == nil ambiguity, so even fixing the consumer alone would leave Get() unable to return a legitimately-stored empty value while Has() (which only checks the Deleted column) reports it present — the two would disagree.\n\nFix. The consumer can key strictly off pair.Delete (which survives the round-trip) rather than pair.Value == nil, fixing Has correctness. Fully fixing Get's empty-vs-absent ambiguity requires the reader to distinguish an absent ValueColumn cell from a present-but-empty one (the row-chunk decoder in sei-db/db_engine/bigtable/bigtable.go also collapses a zero-length cell to a nil Cell.Value), which is a deeper but addressable change.\n\nScope caveat. This feature is currently opt-in and not exposed in the generated default config (internal/Giga-only for now, per this PR's own toml_test.go change), and only affects point Get/Has below the prune horizon on keys whose real value happens to be empty — most such keys (e.g. staking's index keys) are read via iteration, which never uses this fallback. That narrows the practical blast radius today, but it is a genuine, silent, cached correctness divergence introduced by wiring live reads into this path, on a write pattern that already ships in production.

[claude]

🟡 ensureBackendCoverage acquires the single, process-wide backendMu and holds it across the synchronous reader.LastVersion(ctx) call, which for the Bigtable reader fans out across 64 version buckets (up to 4 waves, each retried up to 3x) and can take multiple seconds even when healthy — any other goroutine needing a coverage check blocks on that mutex for the RPC's full duration. In practice this is bounded by the lock-free fast path (version <= backendLast) that normal steady-state pruned reads take, so the real exposure is a one-time cold-start burst and periods when the backend is lagging (whose reads would error out anyway); recommend releasing the lock before the RPC (e.g. single-flight) or refreshing the watermark asynchronously.

Extended reasoning...

ensureBackendCoverage (sei-db/state_db/ss/offload/historical/store.go:118-144) does s.backendMu.Lock() with a deferred Unlock() and, inside that critical section, calls the synchronous s.reader.LastVersion(ctx). For the Bigtable reader this resolves to a fan-out across all 64 version buckets in an errgroup capped at 16 workers (up to 4 sequential waves), with each bucket individually retried up to 3x on Unavailable — a call that can legitimately take multiple seconds even when the backend is healthy, well within the 10s read-timeout budget. backendMu is a single mutex shared by every Get/Has call on the FallbackStateStore, so while one goroutine is inside that RPC, every other goroutine that also needs a coverage check (version > backendLast) blocks on backendMu.Lock() for the RPC's entire duration. This is a textbook hold-a-mutex-across-a-network-call anti-pattern, and it is exactly what the PR author (yzang2019) flagged directly in review: "During that RPC (up to 10s timeout), every goroutine doing a fallback read blocks on the mutex. Under high read concurrency this might become a bottleneck." Cursor's bugbot independently rated the same lines High severity for the same reason.

The important nuance, raised independently by several reviewers, is that the blast radius is narrower than "every 30s recheck stalls all readers." The very first line of the function is a lock-free atomic fast path: if version <= s.backendLast.Load() { return nil }. backendLast is monotonically increasing and tracks the backend's ingestion tip, while shouldFallback only routes a read into this function when version < localEarliest (the local prune horizon). In a healthy deployment the backend's ingestion tip sits far above the local prune horizon, so once backendLast has been populated once, essentially all subsequent pruned reads satisfy version <= backendLast and return before ever touching the mutex — the 30s recheck timer at line ~129 is only even evaluated when version > backendLast, which normal traffic never triggers once warm. So the mutex/RPC path is really only exercised in two situations: (1) a one-time cold-start burst while backendLast == 0, where the first read to arrive issues the RPC and every concurrent read piles up on the mutex until it returns; and (2) genuine consumer lag, where the requests hitting this path are for versions the backend hasn't ingested yet and would return errBackendBehind regardless of the locking strategy.

Neither of those windows is "no big deal" from a serialization standpoint — a cold-start thundering herd or a degraded-backend period can still stall a burst of concurrent RPC/query goroutines for multiple seconds, process-wide, since there is only one backendMu for the whole FallbackStateStore. But it does mean the originally-described impact ("periodic multi-second stall for ALL fallback readers under any meaningful historical-query load, on every successful 30s recheck") overstates steady-state behavior: healthy, warmed-up traffic takes the atomic fast path and never contends on the mutex at all. That is the basis for calibrating this to a nit rather than a blocking issue — there is no incorrect data, crash, or data loss, only added latency concentrated in the cold-start and lagging-backend cases.

Step-by-step proof (cold start):

1. Node starts; backendLast is 0 and backendCheckedAt is 0.
2. A burst of N concurrent pruned reads (version < localEarliest) arrive before any refresh has happened; all fail the fast path (version <= 0 is false) and call ensureBackendCoverage.
3. All N goroutines contend on backendMu.Lock(); the first to acquire it issues s.reader.LastVersion(ctx), which — for Bigtable — can take multiple seconds (64-bucket fan-out, retries on Unavailable).
4. The remaining N-1 goroutines block on the mutex for that entire duration, even though most of them are for versions the backend has long since ingested and could have been served immediately.
5. Once the first goroutine's RPC returns and stores the refreshed watermark, the mutex releases; the other N-1 goroutines each re-check version <= last and typically pass the recheck without issuing their own RPC — but they still paid the full multi-second wait for the first RPC to complete, serialized behind the same mutex.

The suggested fix is exactly what's stated in the finding: release the lock before making the RPC (e.g. single-flight/coalesce the refresh so only one RPC is in flight and everyone else awaits its result without holding a mutex across the wire call) or move the watermark refresh to a background goroutine so the hot Get/Has path never blocks on it synchronously. Given the narrow, self-limiting trigger window (cold start, or a lagging backend whose corresponding reads are erroring anyway) and no correctness impact, this is worth fixing but not blocking on for this PR — hence nit rather than normal.

_{🔬 also observed by review}