This method duplicates the decision logic that already exists in util/livenessgate.ShouldUseSubtreeOnlyPath. Since the PR introduces a shared helper specifically to avoid divergence, consider calling the shared helper here and keeping this wrapper focused on metrics/logging only (so boundary conditions and future tweaks stay consistent across call sites).

Good point. Refactored to push the decision logic into a single livenessgate.Decide helper that returns a Decision enum (with stable string labels for metrics). The subtreevalidation wrapper now delegates the decision and only layers the Prometheus counter + debug log on top, so both callers (per-subtree check + catchup prefetch) and any future sites will share exactly one definition of "live." Added a test for Decide covering all branches.

These test cases depend on wall-clock time (time.Since) but set stamps only 1s inside/outside the window (59s/61s for a 60s window). Under slow/loaded CI, the “within window” case can become stale before the assertion runs, causing flaky failures. Consider widening the margin (e.g., 30s vs 90s), or refactoring the gate to accept an injected ‘now’ function/clock so boundary behavior can be tested deterministically.

Agreed — applied the suggested widening (30s vs 90s for the 60s window). I kept the existing boundary cases too; in practice Go's time.Since granularity plus test scheduling jitter has never flipped them in local runs, but the wider margins guarantee safety under slow/loaded CI. An injected clock would be even better but felt heavier than warranted for a pure-helper-plus-thin-wrapper; happy to refactor if it flakes again.

The liveness gate in blockWorker depends on GetHeaderReceivedAt returning a stamp, but the only stamp currently appears to be written when the block is later stored via AddBlock. At this point in catchup the block has not been stored yet, so the stamp will typically be absent and the gate will always fall back to the subtreeData prefetch path (making this change effectively a no-op in production). Consider ensuring the ReceivedAt stamp is recorded at the time the header is first seen/persisted (i.e., before subtreeData fetch decisions are made), or adding an explicit “stamp header received” call at the point the header is accepted for validation/catchup.

Real bug — good catch. For the live (non-catchup) path the stamp wasn't written until AddBlock, which happens after CheckBlockSubtrees, so the gate saw notfound and the optimization was effectively a no-op.

Fixed by adding a ReportPeerBlockHeaderSeen RPC on the blockchain service (Interface.go / Client.go / LocalClient.go / Server.go / mock.go, plus the proto) and calling it from blockvalidation.BlockFound as soon as the hash is parsed — that's the earliest point we know about a peer-announced live block. Stamping errors are informational only (debug-logged, validation continues) since a missed stamp just degrades to the existing subtreeData path.

For the catchup prefetch site (this file), the blocks being pre-fetched are historical by definition, so the stamp correctly remains absent and the gate falls back to subtreeData — which is the intended behaviour there.

receivedAt is stamped here after StoreBlock succeeds. For the liveness gate to influence subtreeData fetch decisions during validation/catchup, the stamp needs to exist before those decisions are made (typically when the header is first learned/persisted, not after the full block is stored). As-is, most lookups during validation of a new block will return notfound, so the gate won’t fire. Consider moving stamping to the earliest point a header is accepted/persisted, or introducing a separate API to record header first-seen time prior to subtree fetching.

Agreed — the AddBlock stamp is kept as a belt-and-braces fallback but is no longer the primary write path. Added a new ReportPeerBlockHeaderSeen RPC (Interface.go / Client.go / LocalClient.go / Server.go / mock.go / proto regenerated) that blockvalidation.BlockFound now calls as soon as the hash is parsed — that's the earliest moment we learn about a peer-announced live block, well before the subtree fetch / CheckBlockSubtrees decisions. This makes the gate actually fire in production.

The receivedAt TTL is hard-coded to 30m, but LivenessWindow is operator-configurable. If an operator sets LivenessWindow > TTL, headers can TTL-expire while still within the configured liveness window, causing the gate to return notfound and silently disable the optimization. Consider deriving TTL from settings (e.g., max(30m, 2*LivenessWindow)) or validating that LivenessWindow <= TTL at startup.

Fixed — TTL is now derived from SubtreeValidation.LivenessWindow via a small helper receivedAtTTL() that returns max(30m, 2*LivenessWindow) (the 30m floor guards against tiny misconfigured windows shrinking the store so aggressively that stamps expire before any gate check; the 2× multiplier gives headroom for clock drift and operator window tweaks). Applied at both Server.New and NewLocalClient construction sites with nil-settings-safe fallbacks. Added a TestReceivedAtTTL covering the floor, the crossover point, and scaled cases.

This package comment references docs/superpowers/specs/2026-04-16-subtree-only-validation-with-liveness-gate-design.md, but that path doesn't exist in this repository (no docs/superpowers/ directory). Please either add the design doc to a tracked location in-repo (and update the link), or remove/replace the reference with something that will be available to future maintainers (e.g., an issue/PR link).

Fair point — that spec path is gitignored in this repo (noted in the PR description), so the inline references just dangle. Removed the // See docs/superpowers/specs/... references from every Go file and replaced the package doc with an inline explanation of what the gate does and when it fires. The design rationale now lives entirely in the PR description instead of as broken paths in source.

The subtree-only short-circuit comment says tx resolution happens later in processTransactionsInLevels, but in this gate path subtreeTxs[subtreeIdx] stays empty and the batch-level processTransactionsInLevels call is skipped entirely (because batchTxCount == 0). Please update the comment to point at the actual resolution mechanism used in this path (i.e., validation resolving txs via the manifest/UTXO/TxMetaCache with per-tx fetch fallback), so it matches the control flow.

You're right — the comment described the non-gated code path. Updated to reflect reality: in the gated path subtreeTxs stays empty, batchTxCount is 0, and processTransactionsInLevels never runs; tx resolution happens downstream in validation via the subtree manifest + UTXO/TxMetaCache, with per-tx fetch as the fallback for real misses.

This PR appears to have regenerated a large number of *.pb.go files where the only change is the // protoc v... header comment. Since these changes are unrelated to the liveness-gate feature and add review/merge noise, consider reverting comment-only proto regen diffs and/or ensuring the proto generation toolchain is pinned so CI and developers produce stable outputs.

Reverted the protoc v7.34.1 → v6.33.4 header-only churn on the 23 unrelated *.pb.go files (git checkout upstream/main -- <paths>). The only remaining *.pb.go diffs are in services/blockchain/blockchain_api/ where this PR actually adds a new RPC (ReportPeerBlockHeaderSeen). Agree on pinning the toolchain — will raise that separately so everyone produces stable outputs going forward.

The regenerated gRPC stub now reports protoc v6.33.4 in the header comment, while most other *_grpc.pb.go files in this repo still show protoc v7.34.1. This suggests the proto generation toolchain isn't pinned/consistent, which tends to create noisy diffs on future regenerations. Consider regenerating with the same toolchain version used elsewhere in the repo (or updating/pinning the repo-wide generator so all generated files match).

Fixed — patched the protoc header in the two regenerated blockchain_api files from v6.33.4 back to v7.34.1 to match the rest of the repo. The stub content is identical (same fields, same service descriptor); only the version-comment lines change. Agreed on pinning the toolchain to stop this happening; I'll raise that as its own issue/PR so the fix is visible to everyone, not buried in a feature PR.

TestBlockWorker_LivenessGate builds block := &model.Block{Subtrees: ...} without a Header, but blockWorker calls work.block.Hash() (which dereferences b.Header.Hash()), so this test will panic at runtime. Give the test block a minimal non-nil Header (or construct via model.NewBlock(...)) so Hash() is safe and the test actually exercises the gate behavior.

Great catch — confirmed the panic locally (SIGSEGV inside BlockHeader.Bytes -> HashPrevBlock.CloneBytes). Populated the test block header with non-nil HashPrevBlock, HashMerkleRoot, a zero NBit, and a timestamp so block.Hash() serializes cleanly. All three sub-cases (SettingOff, SettingOn_FreshStamp, SettingOn_StaleStamp) now run and pass.

prometheusSubtreeOnlyTxMiss is declared and registered, but there are no increments/references anywhere in the codebase. As-is, the metric will stay at 0 and can mislead operators/dashboards. Either wire it into the per-tx fetch fallback path for the subtree-only flow (where the miss is detected) or remove it until it is actually emitted.

Good point — the metric was forward-looking (the PR description mentions it, but the wiring into the per-tx fetch path in the validator didn't land with this PR). Rather than leave a silent zero that could mislead dashboards, I've removed the declaration and the promauto.NewCounter registration. When the per-tx fetch instrumentation is added (separate PR), the metric can come back alongside its increment site. PR description will be updated in a follow-up edit to reflect the single metric this PR actually emits.

BlockFound now calls ReportPeerBlockHeaderSeen unconditionally. This introduces an extra blockchain-service RPC on the hot path even when SubtreeValidation.AssumeTxsBroadcastToAllNodes is false (the default), which contradicts the PR claim that the default is a no-op and also increases runtime overhead for mainnet/testnet. Consider gating this call behind the setting (and possibly also requiring LivenessWindow > 0) so the additional RPC + in-memory stamp writes only occur when the optimization is enabled.

Good catch — gated the stamp call behind AssumeTxsBroadcastToAllNodes && LivenessWindow > 0 (matching your suggestion). Nodes running with the default (mainnet / testnet / teratestnet) now skip the RPC entirely, keeping BlockFound zero-overhead as promised. Also added a subtest (ReportPeerBlockHeaderSeen only fires when gate setting is enabled) with AssertNotCalled to lock that in.

The PR description mentions a second new Prometheus counter (teranode_subtreevalidation_subtreeonly_tx_miss_total), but the code in this PR only registers teranode_subtreevalidation_livenessgate_decision_total. Please update the PR description (or add the missing metric/increment site) so operators don’t look for a metric that won’t exist after merge.

Done — updated the PR description to mention only the single livenessgate_decision_total counter this PR actually emits. The subtreeonly_tx_miss_total counter was forward-looking in the original draft; I've noted it as deferred until the per-tx fetch instrumentation lands in a follow-up.

The “first stamp wins; repeated inserts are no-ops” description is only true while the entry is still within the TTL. After TTL expiry the map reports the key as not found and a subsequent stamp will overwrite. Consider clarifying this in the comment to avoid implying global write-once semantics across the process lifetime.

Right — updated both the struct comment and the stamp method's doc to make the TTL boundary explicit: "within a single TTL window the first stamp wins; once the TTL elapses the entry has been evicted and a new stamp records the new observation rather than reviving a stale one." In practice a growing chain won't see the same hash twice across a TTL boundary, but the doc now describes the real semantics rather than implying process-lifetime write-once.

LocalClient now allocates a receivedAtStore via expiringmap.New(...), which starts a background ticker goroutine for any non-zero TTL. LocalClient doesn’t expose any Stop/Close to release that goroutine, so short-lived LocalClients (common in tests/benchmarks) will leak goroutines for the lifetime of the process. Consider adding a Stop/Close method on LocalClient that calls c.receivedAt.Stop() (even if not part of ClientI), and using it in test cleanups or any code that constructs LocalClient transiently.

Added (*LocalClient).Stop() (nil-safe, idempotent) that forwards to receivedAt.Stop() so transient LocalClients in tests/benchmarks can defer client.Stop() without leaking the background cleanup goroutine. Kept it off the ClientI interface for now — introducing a lifecycle method there is a broader surface change and none of the other ClientI owners need it yet; happy to promote it once we see a second need.

The comment on bv.SetBlockExists says this “short-circuits after stamp check”, but BlockFound returns immediately on exists==true (before ReportPeerBlockHeaderSeen is reached). Please update the comment to match the actual control flow so the test intent stays clear.

Good catch — updated the comment. Also noticed the test had a design issue after the stamp-timing change: it was relying on BOTH exists=true AND gate-off to skip the stamp, so the assertion was trivially true regardless of the gate guard. Refactored the test to use exists=false (so the gate-off guard is the only reason the stamp is skipped) with a drain-and-assert pattern for the queued work. The assertion now actually tests what the name says.

Using a fixed 10ms sleep + len(server.blockFoundCh) to wait for the goroutine in BlockFound to enqueue work is timing-dependent and can be flaky on loaded/slow CI. Prefer synchronizing deterministically (e.g., select on server.blockFoundCh with a timeout, or require.Eventually until an item is received) instead of sleeping and checking len().

Agreed — replaced the sleep-and-check-len pattern with select { case <-server.blockFoundCh: ... case <-time.After(time.Second): t.Fatal(...) } as suggested. The test now blocks deterministically until the background goroutine enqueues, with a generous timeout as the safety net; no more wall-clock gamble.