feat(autobahn): multi-epoch window for CommitQC/AppQC flow (CON-358)#3736
feat(autobahn): multi-epoch window for CommitQC/AppQC flow (CON-358)#3736wen-coding wants to merge 86 commits into
Conversation
PR SummaryHigh Risk Overview Proposal / AppQC: verification allows AppQC up to one epoch behind Current; committee and road binding come from Availability: tracks its own sliding duo, reweights lane votes on boundary Consensus: inner holds Call sites move to Reviewed by Cursor Bugbot for commit 50c9088. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
wen-coding
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The EpochTrio refactor is broad and mechanically clean, but bounding epoch 0 to a finite road range now forces a real epoch transition at road 108,000, and the consensus trio-rotation has an off-by-one that will stall consensus at that boundary; there are also committee-staleness and nil-deref latent defects in the epoch-transition machinery.
Findings: 5 blocking | 4 non-blocking | 4 posted inline
Blockers
- Cached placeholder committees survive real epoch registration (Codex P1). registry.TrioAt() generates genesis-committee placeholder epochs and inserts them; AddEpoch() later replaces only the map pointer, so EpochTrio values already cached in the avail/consensus/data
inners keep pointing at the stale placeholder *Epoch. advanceEpoch() additionally early-returns whenever the road index is still inside Current's range, so it never refreshes a staleNext. Once a future epoch's committee actually differs from genesis, next-epoch vote weighting (avail reweightForNextEpoch / pushVote uses trio.Next.Committee()) and QC verification will use the wrong committee. Consider having AddEpoch mutate the existing epoch object in place, or make advanceEpoch/TrioAt re-fetch Next from the registry rather than trusting the cached pointer. - Cursor's second-opinion review (cursor-review.md) and the repo REVIEW_GUIDELINES.md are both empty, so no Cursor findings or repo-specific standards were available to incorporate; Codex's two findings were reviewed and are corroborated above.
- Missing test coverage for the actual epoch-boundary transition: the new tests (TestInsertQCCrossEpochFallback, TestPushCommitQCCrossEpochFallback, TestAdvanceEpochTrio) only exercise the registry.TrioAt fallback with artificially-constructed trios, and every other test uses SingleEpochTrio. No test drives consensus across a real road-108,000 boundary (committing the last road of epoch e and proposing the first road of epoch e+1), which is exactly where the P0 off-by-one manifests.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Signature verification was moved under the hot inner lock in avail PushBlock and PushVote (previously done outside via VerifyInWindow). Holding s.inner across ed25519/BLS verification serializes all per-lane block/vote pushes. Prefer snapshotting the committee(s) under the lock and verifying outside, as PushCommitQC and data.PushQC already do.
- avail.PushAppQC verifies against inner.epochTrio directly without the cross-epoch registry.TrioAt fallback that PushCommitQC uses, so an appQC/commitQC whose epoch is outside the cached trio would fail verification; confirm this cannot occur or add the same fallback for consistency.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| iSend.Store(inner{persistedInner: persistedInner{CommitQC: utils.Some(qc)}, epoch: i.epoch}) | ||
| // Reuse the cached trio when the QC stays within the current epoch. | ||
| trio := i.epochTrio | ||
| if !trio.Current.RoadRange().Has(qc.Proposal().Index()) { |
There was a problem hiding this comment.
[blocker] Off-by-one at the epoch boundary (Codex P0). This condition keys on the committed QC's index, but the trio must cover the next view, which is qc.Proposal().Index()+1 (= View().Index). When committing the last road of epoch e (Index() == Last_e), Has(Last_e) is true, so the trio stays at epoch e; but View() then stamps the next view (road Last_e+1, belonging to epoch e+1) with epoch e's index. That proposal fails Proposal.Verify's road-range check on every node → consensus stalls. Since this PR bounds epoch 0 to [0, EpochLength-1] (was OpenRoadRange), the chain will hit this at road 108,000. Advance based on NextIndexOpt(qc) / the new View's index instead: if !trio.Current.RoadRange().Has(qc.Proposal().Index()+1).
| return fmt.Errorf("unknown epoch_index %d", anchor.Proposal().EpochIndex()) | ||
| var anchorLanes func(func(types.LaneID) bool) | ||
| for inner := range s.inner.Lock() { | ||
| ep, _ := inner.epochTrio.EpochFor(anchor.Proposal().EpochIndex()) |
There was a problem hiding this comment.
[blocker] The ok from EpochFor is dropped and ep is dereferenced unconditionally. EpochFor returns (nil, false) when the epoch isn't in the trio, and Epoch.Committee() dereferences its receiver, so this nil-panics if the prune anchor's epoch has fallen outside the cached 3-epoch window. The previous code returned an explicit unknown epoch_index error here — restore an if !ok { return ... } guard.
| return fmt.Errorf("block.Verify(): %w", err) | ||
| } | ||
| for inner, ctrl := range s.inner.Lock() { | ||
| if err := p.Msg().Verify(inner.epochTrio.Current.Committee()); err != nil { |
There was a problem hiding this comment.
[suggestion] PushBlock verifies the proposal only against epochTrio.Current.Committee(), whereas PushVote (below) accepts votes from either Current or Next. newInner explicitly initializes block queues for next-epoch-only lanes "so that early votes and proposals from validators joining in the next epoch are not rejected" — but a next-epoch-only proposer's block fails p.Msg().Verify(current) (HasLane) here and is rejected, contradicting that intent. Verify against Next as a fallback too, or drop the next-epoch block-queue init if only current-epoch proposals are intended.
| if err := vote.Msg().Verify(currC); err != nil { | ||
| if err := vote.Msg().Verify(nextC); err != nil { | ||
| return fmt.Errorf("vote.Verify(): %w", err) | ||
| } |
There was a problem hiding this comment.
[suggestion] VerifySig (and Msg().Verify above) now run while holding the inner lock. These crypto checks were previously performed outside the lock; on the hot vote path this serializes all pushes behind signature verification. Snapshot currC/nextC under the lock and verify after releasing it.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
sei-tendermint/internal/autobahn/avail/state.go:432-441— PushBlock/PushVote regressed on two fronts vs. pre-PR: (1) signature verification (Ed25519/BLS) now runs insides.inner.Lock(), serialising every peer block/vote against PushCommitQC, PushAppVote, ProduceLocalBlock, WaitForLaneQCs, RecvBatch, etc.; the pre-PR code intentionally routed this throughregistry.VerifyInWindowoutside the lock. (2) PushBlock verifies only againstinner.epochTrio.Current.Committee()— unlike sibling PushVote which does Current→Next fallback — so signed lane proposals from Next-only validators (whose queuesnewInnerexplicitly allocates) get rejected before enqueue. Fix by mirroring the pattern already used inPushCommitQC(state.go:277-287): snapshot the trio under a brief lock, run Verify/VerifySig outside the lock, then re-lock to enqueue.Extended reasoning...
The regression\n\nBefore this PR,
PushBlockandPushVoteverified signatures vias.data.Registry().VerifyInWindow(...)outsides.inner.Lock(). After this PR (state.go:435-441 and 486-497), bothp.Msg().Verify(committee)and the CPU-heavyp.VerifySig(committee)execute inside thefor inner, ctrl := range s.inner.Lock()iterator body.\n\ns.innerisutils.Watch[*inner](state.go:36).Watch.Lock()(libs/utils/mutex.go:214-220) acquiresw.ctrl.mu.Lock()— a plain exclusive mutex, not an RWMutex — and holds it for the entire iterator body. Ed25519 verify is order-of-tens-of-microseconds per call; BLS is heavier. That work now serialises against roughly 26 other call sites (PushCommitQC,PushAppQC,PushAppVote,WaitForLaneQCs,ProduceBlock,RecvBatch, the persister loop,fullCommitQC, …) which all take the same mutex.\n\n### Concrete failure scenario\n\nA validator receives sustained peer traffic — say 100 blocks/s from lane producers and 3000 votes/s from replicas across lanes. Each verify is ~50µs of Ed25519 (worse for BLS). Under the new code the mutex is held for roughly (100+3000)×50µs ≈ 155 ms of wall-clock per second across PushBlock/PushVote alone, blocking every consensus wake-up (ctrl.Updated()) and every others.innerwriter during those windows. Tail latency on QC advancement (PushCommitQC), lane-QC construction (laneQC via RecvBatch), and app-vote acceptance (PushAppVote) all spike. PushVote is worse than PushBlock — it may run Verify+VerifySig twice (Current, then Next fallback) inside the same lock scope.\n\n### The correct pattern already lives in this file\n\nPushCommitQC(state.go:277-287) snapshots the trio under a brief lock, runsqc.Verify(trio)OUTSIDE the lock, and then re-enters to store.PushAppVote(state.go:308-319) uses the same pattern to pull the committee out beforev.VerifySig.Committeeand*Epochvalues are immutable (onlyinneris atomically replaced), so snapshotting Current+Next committees before the verify is safe. Mirroring this pattern in PushBlock/PushVote is a few lines and closes the regression.\n\n### Secondary issue: PushBlock lacks the Next-committee fallback\n\nnewInner(inner.go:76-84) explicitly allocatesblocksandvotesqueues for lanes present only in the Next epoch, with the comment "so that early votes and proposals from validators joining in the next epoch are not rejected."PushVoteimplements the Current→Next fallback matching that intent (state.go:486-497).PushBlockdoes not — it verifies only againstCurrent(state.go:435-441).BlockHeader.Verify(block.go:75-80) fails on!c.HasLane(h.lane), so a signed lane proposal from a Next-only validator is rejected before it can be enqueued in the pre-allocatedinner.blocks[lane]slot.\n\nStep-by-step: trio at road 0 with Current=ep0 committee {A,B,C,D} and Next=ep1 committee {B,C,D,E} (E is new).newInnerallocatesinner.blocks[E]andinner.votes[E]via the Next-only lane loop. E signs and pushes aLaneProposalfor its lane.PushBlockreachesp.Msg().Verify(currC)→BlockHeader.Verify→!currC.HasLane(E)→ returns"E is not a lane".inner.blocks[E]stays empty; peers whose PushVote fallback accepted votes for that would-be block now hold votes referencing a header that will never exist. When the transition completes and Current advances to ep1, E has to re-propose and any prior votes are useless. Note:Registry.AddEpochis only invoked from tests today (production always fabricates Next as a placeholder sharing the genesis committee), so this doesn't manifest in production yet — but the plumbing this PR sets up is explicitly for real epoch transitions.\n\n### Suggested fix\n\nBoth issues fold into the same edit — mirrorPushCommitQC:\n\ngo\nvar trio types.EpochTrio\nfor inner := range s.inner.Lock() {\n trio = inner.epochTrio\n}\ncurrC, nextC := trio.Current.Committee(), trio.Next.Committee()\nif err := p.Msg().Verify(currC); err != nil {\n if err := p.Msg().Verify(nextC); err != nil {\n return fmt.Errorf("block.Verify(): %w", err)\n }\n}\nif err := p.VerifySig(currC); err != nil {\n if err := p.VerifySig(nextC); err != nil {\n return fmt.Errorf("block.VerifySig(): %w", err)\n }\n}\nfor inner, ctrl := range s.inner.Lock() {\n // ... existing enqueue logic ...\n}\n\n\nSame shape forPushVote. This restores the pre-PR concurrency behaviour and givesPushBlockthe Next-fallback thatnewInnerwas built to support.
There was a problem hiding this comment.
Well-structured infrastructure PR that introduces the EpochTrio abstraction and fixes the live-path epoch advancement / restart-trio derivation; the security-critical QC verification correctly binds epoch_index ↔ committee ↔ road-range, and I found no reachable-today blockers. The two second-opinion findings are either about not-yet-reachable future work or contained by downstream checks, so they land as non-blocking notes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) is empty — that pass produced no output. Codex produced two findings, both incorporated below.
- Codex (High): the EpochTrio caching design means a cached trio retains placeholder (genesis-committee) epochs generated by Registry.TrioAt, and reweightForNextEpoch/advanceEpoch never create new per-lane queues (blocks/votes/nextBlockToPersist/persistedBlockStart) for validators that only join in a later committee — so once real committee changes exist, new members' lanes would be permanently rejected with ErrBadLane. This is a genuine limitation but is NOT reachable in this PR: Registry.AddEpoch is defined but never called anywhere in production (confirmed by grep), so all epochs currently resolve to the genesis committee placeholder, and inner.go:14-18 already documents this exact gap as an explicit TODO. Recommend ensuring that follow-up (dynamic committee wiring) adds lane-queue creation on epoch transition and the avail runPersist cross-epoch-lane union (state.go:769-771 TODO) before AddEpoch is ever invoked in production.
- avail.PushVote (state.go:497-531) verifies the LaneVote message and its signature against independently-chosen committees (message may validate against Current while the signature validates against Next), unlike PushBlock (state.go:443-453) which pins a single committee for both checks. Exploitability is limited today because currC==nextC in practice (no committee changes yet) and blockVotes.pushVote / inner.laneQC re-check membership via per-epoch weights, so no forged current-epoch LaneQC is possible; still worth tightening for consistency once committees can differ.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| h := vote.Msg().Header() | ||
| for inner, ctrl := range s.inner.Lock() { | ||
| currC, nextC := inner.epochTrio.Current.Committee(), inner.epochTrio.Next.Committee() | ||
| if err := vote.Msg().Verify(currC); err != nil { |
There was a problem hiding this comment.
[suggestion] Consistency/robustness: unlike PushBlock (which pins a single committee for both the message and signature checks), PushVote validates vote.Msg().Verify and vote.VerifySig against independently-selected committees — the message can pass against currC while the signature only passes against nextC (or vice-versa), admitting a vote that is not valid under any single committee. It's not exploitable today (committees don't yet diverge, and blockVotes.pushVote/laneQC re-gate by per-epoch weight so no forged current-epoch QC results), but consider mirroring PushBlock: select one committee via Msg().Verify, then use that same committee for VerifySig.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
A large, well-tested refactor replacing single-epoch *Epoch lookups with an EpochTrio/EpochTrioCursor abstraction across consensus/data/avail/p2p. The mechanics are sound for the current fixed-committee case, but the newly-added epoch-transition machinery has two latent correctness gaps (surfaced by Codex) that will bite once membership-changing transitions are actually wired up, plus a lock-scope regression in avail PushVote.
Findings: 1 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Performance/concurrency regression in
avail/state.goPushVote: signature verification (vote.Msg().Verifyandvote.VerifySig) now runs while holdings.inner.Lock(). The previous code verified outside the lock viaRegistry().VerifyInWindow(...).PushBlockin the same file still verifies outside the lock and only takes the trio under the lock —PushVoteshould follow the same pattern to avoid serializing expensive signature checks under the state mutex. cursor-review.mdis empty — the Cursor second-opinion pass produced no output; only Codex's review was available to merge.REVIEW_GUIDELINES.md(pulled from the base branch) is empty, so no repo-specific review standards could be applied.- Minor: several exported
Verifymethods lost their doc comments (BlockHeader.Verify,Block.Verify,LaneVote.Verify,LaneRange.Verify,View.Less,TimeoutVote.Verify, etc.). Not a lint failure under the configured linters, but the removals are incidental to the refactor and slightly reduce documentation. registry.TrioAtinserts generated placeholder epochs into the shared map permanently (epochAt); benign given road indices come from verified QCs, but worth a note that far-future lookups would grow the map.- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // TODO: filter votes per-epoch committee once epoch transitions are wired up. | ||
| // reweightForNextEpoch refreshes the per-epoch weight cache in every stored | ||
| // blockVotes entry for the given next epoch. Called once per epoch transition. | ||
| func (i *inner) reweightForNextEpoch(nextEpoch *types.Epoch) { |
There was a problem hiding this comment.
[blocker] reweightForNextEpoch only recomputes weights for queues that already exist; it never creates votes/blocks queues for lanes first introduced by the new Next epoch. newInner seeds queues for the starting Current ∪ Next lanes, but after an Advance (here and in PushCommitQC), the trio's Next becomes a further epoch whose brand-new lanes have no queue. A valid vote/block from such a lane then passes committee verification against Next but fails the inner.votes[h.Lane()] / inner.blocks[...] lookup with ErrBadLane (avail/state.go PushVote/PushBlock). This is latent today (no production AddEpoch caller, and the transition test uses identical committees across epochs so no new lane appears), but it's a real defect in the transition path and should be fixed — create queues for newly-introduced next-epoch lanes on transition — before membership-changing epoch transitions are enabled. (Confirms Codex finding.)
| } | ||
|
|
||
| // Trio returns the current cached trio. | ||
| func (c EpochTrioCursor) Trio() types.EpochTrio { return c.trio } |
There was a problem hiding this comment.
[suggestion] EpochTrioCursor caches *Epoch pointers in trio. registry.TrioAt may return placeholder epochs built from the genesis committee when AddEpoch hasn't yet supplied the real one. Once such a placeholder is cached here, Trio() and the current-epoch fast path in ForVerify keep returning the genesis committee even after AddEpoch overwrites the registry entry — the cursor only refreshes on Advance into a different epoch. This surfaces in data.CurrentTrio() → Committee().EvmShard(...) routing and in current-epoch QC/vote verification. Consider refreshing the cached trio from the registry (or not caching placeholder-backed epochs) so a later AddEpoch is observed. Latent until AddEpoch is wired to production, but worth addressing with the transition work. (Confirms Codex finding.)
There was a problem hiding this comment.
Well-structured refactor replacing single-Epoch verification with an EpochTrio/EpochTrioCursor model across the autobahn layers, with good new test coverage. No shipping-blocking bugs: the multi-epoch concerns raised are currently latent because AddEpoch/non-genesis committees have no production callers yet, so all epochs resolve to the genesis-committee placeholder. A few consistency and locking notes are worth addressing.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion pass produced no output (
cursor-review.mdis empty);REVIEW_GUIDELINES.mdis also empty, so no repo-specific guidelines were applied. - Codex (High) — cached trios retain overridden placeholder epochs:
Registry.epochAtinserts placeholders using the genesis committee, and cursors cache the resultingEpochTrio. OnceAddEpochoverwrites a registry entry with the real committee, existing cursors keep pointing at the stale placeholder (Nextin particular) until theyAdvance. This is currently latent (no productionAddEpochcaller and all committees are the genesis placeholder), but it will produce wrong committees for next-epoch vote weighting / QC verification once real epoch transitions are wired up. Consider a way for cursors/TrioAtto pick up committee updates, or document/enforce the ordering constraint. Registry.TrioAtmutates the map (inserts placeholders) as a side effect of a read-style lookup; over many distinct road indices this grows the map with placeholder epochs that are never GC'd. Minor, but worth a note since it also underlies the staleness issue above.- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| inner.qcs[inner.nextQC] = qc | ||
| inner.nextQC += 1 | ||
| } | ||
| inner.cursor.Advance(qc.QC().Proposal().Index()) |
There was a problem hiding this comment.
[suggestion] Codex (Medium): data advances the cursor with qc.QC().Proposal().Index(), whereas consensus (pushCommitQC) and avail (PushCommitQC) advance with Index()+1. After finalizing the last QC of an epoch, insertQC (line 226) and this path leave the cursor on the old epoch until the first QC of the next epoch arrives, so CurrentTrio() — consumed by the p2p EVM shard router (giga_router_common.go/giga_router_validator.go) — reports the previous committee across the boundary window. Verification itself still works via the ForVerify Next-range fallback, and it's harmless today (all committees are the genesis placeholder), but the divergence from the other two layers looks unintentional; either align on Index()+1 or add a comment explaining why data intentionally lags.
| } | ||
| committee = nextC | ||
| } | ||
| if err := vote.VerifySig(committee); err != nil { |
There was a problem hiding this comment.
[suggestion] PushVote now runs both structural Verify and the cryptographic VerifySig inside the s.inner.Lock() scope, so signature verification serializes on the state mutex. Note PushBlock (just above) deliberately reads the committees under the lock, releases it, and verifies outside. Consider matching that pattern here to keep expensive crypto off the hot lock.
Superseded: latest AI review found no blocking issues.
c36ec91 to
d03b4f5
Compare
wen-coding
changed the title
wen-coding
changed the title
There was a problem hiding this comment.
This PR introduces EpochTrio to move autobahn from a single-epoch to a three-epoch (Prev/Current/Next) view, replacing LatestEpoch()/EpochByIndex with road-index-based window lookups and adding epoch seeding/advancement. The core mechanics look sound and are well tested, but two edge cases flagged by Codex are worth addressing: reweightForNextEpoch under-initializes lane queues, and PushQC now rejects out-of-window QCs; both are latent (masked while all epochs share the genesis committee) but will matter once real committee rotation lands.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor produced no output:
cursor-review.mdis empty.REVIEW_GUIDELINES.mdis also empty, so no repo-specific standards were applied. (Codex did produce output — both its findings are incorporated below.) - The PR description references APIs/tests that are not present in the diff —
advanceEpoch(),data.State.CurrentTrio(),TestInsertQCCrossEpochFallback,TestPushCommitQCCrossEpochFallback,TestAdvanceEpochTrio, and a "cross-epoch verification fallback" indata.PushQC. The actual code exposesEpochTrio()and advances viaTrioAt(idx+1)with no fallback. The description appears stale relative to the committed changes; update it so reviewers/maintainers aren't misled about what shipped. - Nit: several error messages wrap
EpochForRoadfailures with the labelEpochAt(%d)(e.g. avail/state.go PushCommitQC/PushAppVote/PushAppQC). Rename for accurate diagnostics. - Nit:
TestPushAppQCPreviousEpoch(avail/state_test.go) buildsepochNbut discards it (_ = epochN) and never actually advances the state into epoch N, so it does not exercise the "late AppQC after an epoch boundary crossed" scenario its comment describes — it only pushes an epoch N-1 QC/AppQC into a genesis-seeded state. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
This PR wires multi-epoch handling via a new EpochTrio abstraction and a seeding-aware Registry; the core logic and test coverage are solid. No present-tense blockers (all epochs still use the genesis committee placeholder), but there are two latent correctness gaps that will bite once real committee rotation is enabled, plus minor notes.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion review (cursor-review.md) and the repo REVIEW_GUIDELINES.md were both empty — no Cursor output to merge and no repo-specific guidelines applied.
- Codex flag (confirmed, latent): fullnodes never seal the registry. SealSeeding() is only called from consensus.NewState (validator path); NewGigaFullnodeRouter builds only a data.State and no consensus/avail state, so on a fullnode
seedingstays true forever and EpochAt/TrioAt will auto-generate any missing epoch with the genesis committee, permanently bypassing AdvanceIfNeeded's advancement constraints. Harmless today (genesis committee everywhere) but silently defeats this PR's own safety gate. Consider sealing the registry on the fullnode path (or documenting why it must remain open). - After SealSeeding, Registry.EpochAt still acquires the write lock (state.Lock()) for pure reads because it may auto-generate during seeding; consensus hot paths (pushProposal midpoint gate, pushCommitQC) call it per-message. Minor serialization/perf concern worth a fast read path once seeding is sealed.
- Several wrapped errors are mislabeled: e.g. state.go PushCommitQC/PushAppVote/PushAppQC return
fmt.Errorf("EpochAt(%d): ...")while actually calling EpochForRoad. Cosmetic, but confusing in logs. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
This PR wires multi-epoch handling into autobahn via a new EpochTrio abstraction and an epoch Registry that seeds epochs on demand. The refactor is clean and well-tested at the unit level, but the epoch-advance path at data/avail has a correctness gap: crossing an epoch boundary requires epoch N+2 to be registered, which the seeding model (AdvanceIfNeeded seeds only N+1) can never satisfy in production, so nodes will error at the end of epoch 0.
Findings: 3 blocking | 5 non-blocking | 3 posted inline
Blockers
- Root cause of the boundary failure:
Registry.AdvanceIfNeeded(registry.go) seeds onlycurrentIdx+1from an epoch-N AppQC, and an AppQC is fundamentally downstream of its CommitQC (registry.go's own comment states "AppQC never runs ahead of consensus"). ButTrioAtrequires both Current and Next, so entering epoch N+1 needs epoch N+2 registered. N+2 is only seeded by an AppQC in epoch N+1, which cannot exist before the CommitQC that ends epoch N. The PR description's claim that "AppQC has already been processing roads from epoch N+1" by the midpoint of epoch N contradicts registry.go and the AppQC→CommitQC dependency. Fix options: have AdvanceIfNeeded seed two epochs ahead (N+1 and N+2), or relax the boundary path to tolerate a not-yet-seeded Next. This is also Codex's P0 and should be resolved (or the seeding invariant proven) before merge. Unit tests miss it because SealSeeding is only called in consensus.NewState / the fullnode router, so most tests run in the always-auto-generate seeding phase and never exercise a post-seal boundary crossing. - 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass (cursor-review.md) produced no output; only Codex's review was available to merge.
- Consider an integration/e2e test that seals seeding and then drives a full epoch-boundary crossing (road EpochLength-1 → EpochLength) through data/avail PushCommitQC/PushQC, which would catch the N+2 seeding gap that unit tests miss.
- Several EpochForRoad error messages in avail/state.go are mislabeled as
EpochAt(%d)/EpochForRoad(%d)inconsistently — harmless but worth aligning the wording with the method actually called. - consensus/inner.go pushCommitQC derives the next epoch from
qc.Proposal().EpochIndex()rather thani.epoch.EpochIndex(); since the removed explicit epoch-match check is gone, a QC with an inconsistent EpochIndex would be trusted for the lookup. Low risk given QC verification, but worth a defensive assert. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| idx := qc.QC().Proposal().Index() | ||
| if idx == s.epochTrio.Load().Current.RoadRange().Last { | ||
| nextTrio, err := s.cfg.Registry.TrioAt(idx + 1) |
There was a problem hiding this comment.
[blocker] Same boundary bug as avail/state.go: TrioAt(idx+1) at the end of epoch N requires epoch N+2 to be registered, but the seeding model only guarantees N+1 exists when entering N+1. Once seeding is sealed, PushQC will error at the first real epoch boundary. Fix alongside the avail path.
| // block of epoch N — at that point N+2 won't be seeded until execution | ||
| // finalizes end(N), and this gate becomes meaningful back-pressure. | ||
| if proposal.Proposal().Msg().Index() == i.epoch.RoadRange().MidPoint() { | ||
| if _, err := i.registry.EpochAt(types.RoadIndex(i.epoch.EpochIndex()+1) * epoch.EpochLength); err != nil { |
There was a problem hiding this comment.
[suggestion] The midpoint liveness gate checks epoch N+1, but the PR's stated safety condition (and the TODO here) is N+2. Because N+1 is pre-seeded at init, this gate is a no-op today and provides none of the intended back-pressure (Codex P1). This is acknowledged as intentional-for-now via the TODO, so not blocking — but note it does not mitigate the data/avail N+2 boundary failure above, since even an N+2 gate cannot make N+2 seedable under the current AppQC ordering.
There was a problem hiding this comment.
Solid, well-tested groundwork for multi-epoch handling via EpochTrio, but it introduces a liveness-breaking bug for full nodes: they seal registry seeding after only epochs 0/1 and never call AdvanceIfNeeded, so data.State.PushQC's boundary TrioAt(idx+1) fails at the first epoch boundary and full-node sync permanently stalls.
Findings: 3 blocking | 4 non-blocking | 2 posted inline
Blockers
- Full nodes cannot cross the first epoch boundary (confirmed; also raised by Codex).
AdvanceIfNeededis only invoked fromavail/state.go, but full nodes run onlydata.State(no avail/consensus).NewGigaFullnodeRoutercallsSealSeeding()afterdata.NewStatehas seeded only epochs 0 and 1, so no further epochs are ever created. When a QC for the last road of epoch 0 arrives,data.State.PushQCcallsRegistry().TrioAt(idx+1)which requires epoch 2 asNext; it is missing and PushQC returns an error, so the full node stops accepting QCs forever. Fix by either not sealing seeding on full nodes, or having the full-node data path advance/seed epochs from incoming (App)QCs. - Missing test coverage for the full-node epoch-boundary crossing. All tests use
GenRegistry/GenRegistryAt, which pre-seed 3 epochs, masking the bug above. Add a test that constructs a full-node-style data.State (seeded 0/1 + SealSeeding) and pushes a QC at the last road of epoch 0 to assert the boundary is handled. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output.
Registry.EpochAtnow always takes the write lock (r.state.Lock()) even for pure reads afterSealSeeding, whereas the oldEpochByIndexusedRLock.EpochAt/TrioAtare called from consensus (pushCommitQC, midpoint gate) and data/avail restore paths; consider aRLockfast-path for the common post-seal read case to avoid serializing epoch lookups.- Several wrapped errors read
EpochAt(%d)/EpochForRoadinconsistently with the function actually invoked (e.g. avail/state.go PushCommitQC/PushAppVote/PushAppQC wrapEpochForRoadfailures as"EpochAt(%d): %w"). Harmless but misleading in logs — align the message text with the call. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // if the epoch has not been registered via AdvanceIfNeeded. | ||
| func (r *Registry) EpochAt(roadIndex types.RoadIndex) (*types.Epoch, error) { | ||
| epochIdx := types.EpochIndex(roadIndex / EpochLength) | ||
| for s := range r.state.Lock() { |
There was a problem hiding this comment.
[nit] EpochAt now takes the write lock unconditionally, including for pure reads after SealSeeding (the old EpochByIndex used RLock). Since this is hit on consensus/data hot-ish paths, consider an RLock fast-path that only escalates to Lock when a missing epoch must be auto-generated during seeding.
There was a problem hiding this comment.
A well-structured, heavily-tested refactor that wires the EpochTrio machinery for multi-epoch handling across the data/avail/consensus layers. No blocking correctness issues were found; the main caveat is that committee rotation is still a documented placeholder, so the feature is machinery-only for now.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex (P1): every seeded epoch reuses the genesis committee (
makeEpochcopiesgenesis.Committee()), so QC verification, vote weighting, and EVM shard routing remain effectively tied to genesis and no real validator-set change happens across epochs yet. This is intentional and explicitly documented via TODOs inregistry.go/giga_router_common.go(real rotation comes with snapshot/execution-layer committee derivation), and it is not a regression (the prior code also used the genesis epoch everywhere), so I do not treat it as a blocker — but reviewers/mergers should be aware this PR introduces the machinery only and the multi-epoch feature is not functionally complete. - Cross-epoch consistency: several
EpochByIndex(qc.Proposal().EpochIndex())lookups were replaced with road-index-based resolution, and the removed avail checkqc.Proposal().EpochIndex() != inner.epoch.EpochIndex()is gone. The QC's self-reportedEpochIndexfield is therefore no longer cross-checked against its road index. This is harmless while all committees are identical, but should be revisited when committees actually differ per epoch. - The Cursor second-opinion review file (
cursor-review.md) was empty — that pass produced no output. - Restore path in
avail/state.goNewStateonly handles a single epoch-boundary crossing (if inner.commitQCs.next > startTrio.Current.RoadRange().Last). This is fine given prune anchors are recent, but a restart whose loaded CommitQCs span more than one epoch past the anchor would not fully reweight; worth a comment or assertion documenting the single-boundary assumption. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
Superseded: latest AI review found no blocking issues.
Superseded: latest AI review found no blocking issues.
Defense-in-depth: EpochForIndex alone trusts the self-declared epoch; reject when road_index falls outside that epoch's RoadRange. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A well-structured, well-tested change introducing a Prev|Current sliding epoch window across the autobahn layers. One blocking restart-correctness gap: data.NewState requires an epoch registration for every loaded WAL QC/block, but SetupInitialDuo only seeds the two-epoch tip window (plus genesis), so a WAL spanning an intermediate epoch fails startup permanently.
Findings: 1 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- consensus/inner.go pushCommitQC no longer verifies the CommitQC signature, relying on avail having verified it at admit time (documented). Safe because the QC is pulled from local verified avail state rather than the network, but worth calling out since it removes a defense-in-depth check.
- consensus/inner.go pushCommitQC hard-errors (returns from Iter, failing the consensus loop) if DuoAt(nextRoad) is missing at a boundary, unlike avail/data which WaitForDuo. This is an intentional invariant ("seeded before this tip"), but a violation crashes the consensus goroutine rather than backpressuring — ensure the seeding invariant is truly guaranteed for coalesced/boundary tips.
- REVIEW_GUIDELINES.md is empty (no repo-specific standards were available for this review).
- cursor-review.md is empty — the Cursor second-opinion pass produced no output. Codex produced one finding, which is confirmed and reported inline.
| for _, qc := range dataWAL.CommitQCs.ConsumeLoaded() { | ||
| if err := inner.insertQC(cfg.Registry, qc); err != nil { | ||
| for _, qc := range loadedQCs { | ||
| ep, err := cfg.Registry.EpochAt(qc.QC().Proposal().Index()) |
There was a problem hiding this comment.
[blocker] Restart fails when the WAL retains QCs from an intermediate, unseeded epoch. SetupInitialDuo (line 326) only registers {genesis(0), tipEpoch-1, tipEpoch} plus tip-adjacent lookahead, but this loop — and the block loop at line 349 — calls EpochAt(qc.Index()) for every loaded QC/block, which errors on any epoch that isn't registered. If a node has crossed ≥2 epoch boundaries and the data WAL still holds entries from an epoch 1 ≤ e ≤ tipEpoch-2 (e.g. PruneAfter is None or retention exceeds two epochs), startup returns "epoch e (road …) not registered" and the node can never restart. Seed every epoch spanned by the loaded WAL (from the first loaded QC's epoch through the tip) before this loop, or make the lookup seed-on-demand / treat below-window entries as prunable. (Matches Codex's High finding.)
| func (s *State) PushQC(ctx context.Context, qc *types.FullCommitQC, blocks []*types.Block) error { | ||
| // Wait until QC is needed. | ||
| ep, ok := s.cfg.Registry.EpochByIndex(qc.QC().Proposal().EpochIndex()) | ||
| if !ok { | ||
| return fmt.Errorf("unknown epoch_index %d", qc.QC().Proposal().EpochIndex()) | ||
| // Out-of-window ⇒ below prune tip; reject. | ||
| ep, err := s.epochDuo.Load().EpochForRoad(qc.QC().Proposal().Index()) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🔴 data.State.PushQC resolves the CommitQC's epoch via its own 2-epoch Prev|Current window (s.epochDuo.Load().EpochForRoad(...)) and returns the bare lookup error unconditionally — before the needQC no-op check that would otherwise skip an already-applied/duplicate road. Unlike every other admission path added in this PR (avail's waitCurrentForRoad/waitEpochForRoad/admitRoadOrDrop, and avail's own sibling fullCommitQC call which explicitly maps this failure to data.ErrPruned), this failure is not wrapped in the established ErrPruned sentinel, so a stale road tears down the caller instead of being skipped: giga/data.go'''s clientStreamFullCommitQCs kills the whole per-peer connection, and avail/state.go'''s s.data.PushQC forwarder tears down the entire avail scope (persist, propose, outputs). Fix by wrapping the lookup failure as data.ErrPruned (and/or waiting when the road is merely ahead of the window), mirroring the pattern used everywhere else in this PR.
Extended reasoning...
The bug. data.State.PushQC (state.go:417-422) begins with:
ep, err := s.epochDuo.Load().EpochForRoad(qc.QC().Proposal().Index())
if err != nil {
return err
}s.epochDuo is data.State'''s own narrow 2-epoch {Prev, Current} window (independent of avail'''s window and of the shared epoch.Registry, which retains all epochs indefinitely). EpochForRoad (types/epoch_duo.go) returns a bare, unwrapped fmt.Errorf("road %d not in window %v", ...) whenever the road has fallen more than one epoch behind Current. Crucially, this lookup runs before the needQC computation a few lines below (state.go:424-434) that would otherwise treat an already-applied/duplicate road as a harmless no-op — so even a QC that doesn'''t need to be inserted still pays for, and can fail, this lookup.
Why this is a regression. Pre-PR, PushQC resolved the epoch via s.cfg.Registry.EpochByIndex, and the registry retains every epoch it has ever seen, so a stale/duplicate road always resolved fine and the call returned nil after needQC evaluated to false. The PR swapped that for the new 2-epoch epochDuo window without adding the wait-if-early/drop-if-stale handling used everywhere else it introduces a similar lookup: avail.PushCommitQC (waitCurrentForRoad), avail.PushAppQC/PushAppVote (waitEpochForRoad/admitRoadOrDrop), and even avail'''s own sibling call three lines earlier in fullCommitQC (avail/state.go:800-806), which explicitly does errors.Is(err, data.ErrPruned) and continues past an equivalent out-of-window failure.
Why the failure is reachable. data.State'''s retention floor (inner.first) is advanced only by pruneFirst, which is driven by the Cosmos app'''s RetainHeight (see giga_router_common.go) — a policy that is independent of, and can retain roads far older than, the 2-epoch epochDuo window (216k roads at EpochLength=108,000). Meanwhile epochDuo itself advances via any successful PushQC, and data.State is written concurrently by multiple sources: every peer'''s clientStreamFullCommitQCs goroutine (giga/data.go, one per outbound/inbound connection) plus avail'''s own internal forwarder (s.data.PushQC task in avail.State.Run()). A lagging or reconnecting peer stream resuming from an old cursor, or avail'''s own forwarder trailing behind a faster peer stream that has already pushed the window forward, can deliver a road that is still held by data.State (per app RetainHeight) but has fallen >1 epoch behind data'''s current window — exactly the case needQC would resolve to "already applied, skip" if it ever got the chance.
Concrete failure paths, both currently unguarded:
sei-tendermint/internal/p2p/giga/data.go:39—clientStreamFullCommitQCsreturnsPushQC'''s error directly. It runs insideRunClient'''s per-peerscope.Spawn, so this tears down all of that peer'''s streams (ping, consensus, lane proposals/votes, commitQCs, appVotes/appQCs) — not just FullCommitQC resync — even though the road in question needed no action.sei-tendermint/internal/autobahn/avail/state.go:822-824— thes.data.PushQCscope.SpawnNamedtask insideavail.State.Run()propagates the error unwrapped, with noerrors.Is(err, data.ErrPruned)guard, directly contradicting its siblings.fullCommitQC()call three lines above which does treat the equivalent failure as benign. This tears down the entire avail scope: persist, propose, outputs, and the pushCommitQC forwarding — i.e. a liveness failure for that validator'''s consensus participation, not just a data-sync hiccup.
Step-by-step proof (path 2, avail forwarder): Suppose data.State'''s window is {Prev: epoch 5, Current: epoch 6} because a fast peer stream already pushed CommitQCs well into epoch 6. Meanwhile this validator'''s own avail forwarder (s.data.PushQC loop over s.fullCommitQC(ctx, n)) is still catching up on locally-sequenced roads back in epoch 4 (e.g. after a restart, or because avail'''s own sequencing is slower than a peer'''s FullCommitQC stream). When the forwarder reaches a road in epoch 4 and calls s.data.PushQC, epochDuo.EpochForRoad(road) fails (epoch 4 is more than one epoch behind Current=6). The bare error propagates out of PushQC, out of the s.data.PushQC scope.SpawnNamed task, and scope tears down every sibling task in avail.State.Run() — including the persist goroutine — even though this road may have already been fully applied to data.State by the faster peer path, i.e. needQC would have been false had the code reached it.
The fix, matching the established pattern elsewhere in this PR: wrap the EpochForRoad lookup failure as data.ErrPruned (and/or add wait-if-ahead semantics for a genuinely future road), so giga/data.go'''s stream loop and avail/state.go'''s forwarder — the latter of which already checks for data.ErrPruned on its sibling call — can skip the road gracefully instead of tearing down their whole scope.
On severity. All three independent verifiers who analyzed reachability concluded the trigger requires substantial (>=1-2 epoch, i.e. >=108k-216k road) divergence between concurrent writers of data.State, which is unlikely in normal steady-state operation but plausible under sustained catch-up (a validator whose local avail/consensus sequencing lags block-sync by that much, or a long-lived, slowly-advancing peer stream). Two of three verifiers rated this "normal" because the avail-forwarder path halts a validator'''s consensus participation entirely (not merely a data-layer hiccup) and the fix is a trivial, well-precedented change; the third rated it "nit" citing the same reachability bar. Given the asymmetry with every other admission path in this PR, the concrete liveness consequence on the avail path, and the low risk/cost of the fix, this is worth addressing before merge.
| persisted = *decoded | ||
| } | ||
|
|
||
| // TODO: when AddEpoch is wired, resolve the epoch from the persisted QC/proposal | ||
| // rather than assuming LatestEpoch — otherwise a restart after an epoch transition | ||
| // fails validation with an epoch/road mismatch. | ||
| ep := registry.LatestEpoch() | ||
| if err := persisted.validate(ep); err != nil { | ||
| // View epoch = tipcut road; CommitQC may still be the prior epoch's last road. | ||
| // Epoch seeding is owned by data.NewState (SetupInitialDuo); consensus tip | ||
| // must fall in that window (may not lead data across an unseeded epoch). | ||
| nextViewRoad := types.NextIndexOpt(persisted.CommitQC) | ||
| viewEpoch, err := registry.EpochAt(nextViewRoad) | ||
| if err != nil { | ||
| return inner{}, fmt.Errorf("EpochAt(%d): %w", nextViewRoad, err) | ||
| } | ||
| commitEpoch := viewEpoch | ||
| if cqc, ok := persisted.CommitQC.Get(); ok { | ||
| commitEpoch, err = registry.EpochAt(cqc.Proposal().Index()) | ||
| if err != nil { | ||
| return inner{}, fmt.Errorf("EpochAt(%d): %w", cqc.Proposal().Index(), err) | ||
| } | ||
| } | ||
| if err := persisted.validate(commitEpoch, viewEpoch); err != nil { | ||
| return inner{}, err | ||
| } |
There was a problem hiding this comment.
🔴 consensus.newInner (inner.go:119-139) resolves the epoch for its persisted CommitQC road via bare registry.EpochAt() calls and hard-errors if the epoch isn't yet seeded, with no wait/seed guard — unlike avail.NewState and data.NewState, which both explicitly re-seed their restart window. Because data's WAL persistence of CommitQCs runs in an async background goroutine (data/state.go runPersist) that can lag behind both local execution (app.Commit()) and consensus's own (much faster) persisted-CommitQC path, a crash can leave consensus's persisted road pointing at an epoch data.NewState never seeds, and the node fails to start until an operator manually intervenes.
Extended reasoning...
The bug. consensus.newInner (inner.go:119-139) computes nextViewRoad := types.NextIndexOpt(persisted.CommitQC) and then calls registry.EpochAt(nextViewRoad) (and registry.EpochAt(cqc.Proposal().Index()) for the CommitQC's own epoch), returning a hard error if either epoch isn't registered. This runs synchronously inside NewState, before the Run loop and its retry/backoff machinery exist. The registry is data.Registry(), and the only restart-time seeding of that registry is data.NewState's call to Registry.SetupInitialDuo(lastExecuted, lastQC) (epoch/registry.go), which is keyed on data's own WAL tip and app.LastBlockHeight() — it has no visibility into what CommitQC road consensus itself persisted.
Why the two tips can diverge across a crash. Avail admits a CommitQC into its own commitQCs queue via avail.PushCommitQC, and consensus watches that admission directly through avail.LastCommitQC().Iter (consensus/state.go) and durably persists whatever it observes as persistedInner.CommitQC. Separately, avail's s.data.PushQC goroutine (avail/state.go Run) feeds that same CommitQC into data.State, which stores it in memory and hands it to runPersist — an explicitly asynchronous background goroutine (data/state.go, comment: "runPersist is a background goroutine that persists blocks and QCs to WALs") that batches writes to dataWAL.CommitQCs on its own schedule. So the durability of a given CommitQC in data's on-disk WAL is decoupled in time from (a) consensus's own on-disk persistence of that CommitQC, and (b) local block execution / app.Commit().
Why the epoch-boundary guard (from the PR's own review thread) doesn't close this. A refutation raised in review argues the gap is closed: for avail to admit a boundary CommitQC (closing epoch C), avail.PushCommitQC must call Registry.WaitForDuo(FirstRoad(C+1)), which can only succeed once epoch C+1 has been seeded by AdvanceIfNeeded, which itself only fires once local execution reaches LastRoad(C-1) — and since giga_router_common.go's executeBlock calls app.Commit() before calling AdvanceIfNeeded, app.LastBlockHeight() is already durably at LastRoad(C-1) by the time C+1 is seeded. On restart, data.NewState maps app.LastBlockHeight() back to a road via roadForGlobal(loadedQCs, cfg.LastExecutedBlock) and calls EnsureAfterExecuted, which the refutation says restores the seeding.
This chain has a hole: roadForGlobal only succeeds if the CommitQC covering app.LastBlockHeight() is present in loadedQCs — i.e., already durably written to dataWAL.CommitQCs on disk. But that write happens in the async runPersist goroutine, which is not on the critical path of app.Commit() or AdvanceIfNeeded at all. So the window the refutation needs closed (WAL flush of the boundary QC) is exactly the one thing AdvanceIfNeeded/app.Commit() do not wait for. If runPersist simply hasn't caught up when the process crashes, roadForGlobal returns None, EnsureAfterExecuted is skipped entirely (per its own doc comment: "None when ... the QC was pruned / not loaded (SetupInitialDuo then skips EnsureAfterExecuted)"), and SetupInitialDuo falls back to seeding only around data's WAL tip — which is now the lagging value.
Step-by-step reproduction.
- Local execution finishes the last road of epoch C-1 (
LastRoad(C-1)).executeBlockcallsapp.Commit()(durably persistsapp.LastBlockHeight()at that height), then callsr.data.Registry().AdvanceIfNeeded(LastRoad(C-1)), which seeds epoch C+1 in the in-memory registry only. avail.PushCommitQCis now able to admit the boundary CommitQC atLastRoad(C)(closing epoch C), becauseRegistry.WaitForDuo(FirstRoad(C+1))succeeds against the in-memory seeding from step 1. Avail advancesepochDuoand stores the CommitQC.- Consensus's
s.avail.LastCommitQC().Iterwatch observes this CommitQC andpushCommitQCdurably persists it aspersistedInner.CommitQC(consensus/state.gorunOutputs/persist path). This is fast — no dependency on data's WAL. - Meanwhile,
data's asyncrunPersistgoroutine has not yet flushed the CommitQC forLastRoad(C-1)(orLastRoad(C)) todataWAL.CommitQCs— it's still batching/queued. - The node crashes here.
- On restart:
app.LastBlockHeight()correctly reflects execution throughLastRoad(C-1)(durable from step 1). ButdataWAL.CommitQCs.ConsumeLoaded()does not yet contain the QC for that global block (per step 4), soroadForGlobalreturnsNone, andSetupInitialDuoskipsEnsureAfterExecutedentirely. It seeds the registry only fromlastQC(data's WAL tip, which is now behind — some road beforeLastRoad(C-1)), producing a window like{C-2, C-1}(or lower), never reachingC+1. consensus.newInnerloads its persistedCommitQCatLastRoad(C)and computesnextViewRoad = FirstRoad(C+1).registry.EpochAt(FirstRoad(C+1))fails — epoch C+1 was never seeded.NewStatereturns an error, and the node cannot start.
Impact. This fires inside NewState, before Run's retry/backoff/liveness logic exists, so it is not self-healing the way the live pushCommitQC hard-fail (explicitly accepted by a maintainer as an acceptable failure mode) is — the node is stuck failing to start until an operator manually truncates or repairs consensus's persisted state.
Fix. Give consensus.newInner (or consensus.NewState) its own restart-time seeding/wait guard analogous to avail.NewState's DuoAt(commitTip) and data.NewState's SetupInitialDuo — e.g. registry.EnsureDuoAt(nextViewRoad) before calling EpochAt, or seed the registry from max(data's tip, consensus's own persisted CommitQC road) in NewState, rather than relying transitively on data's asynchronously-persisted WAL tip to always be caught up with consensus's independently-persisted tip.
SetupInitialDuo takes Option[CommitQCSpan] and registers every epoch from firstLoaded through tip, standing in for reading epoch info from blocks. Co-authored-by: Cursor <cursoragent@cursor.com>
| } | ||
| inner.commitQCs.pushBack(commitQC) | ||
| metrics.ObserveCommitQC(commitQC) | ||
| } |
There was a problem hiding this comment.
PushAppQC skips CommitQC indices
High Severity
PushAppQC can apply prune and pushBack for a road index ahead of commitQCs.next, which moves the queue’s lower bound past missing indices without ever storing those CommitQCs. Later PushCommitQC calls for those roads exit early when idx != commitQCs.next, so the availability layer can permanently lack contiguous commit history while still advancing the AppQC anchor.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b759b0e. Configure here.
There was a problem hiding this comment.
A large, well-structured refactor introducing a sliding Prev|Current epoch window (EpochDuo) across the autobahn consensus/data/avail layers, backed by substantial new unit tests. No clear correctness bug found, but one notable defense-in-depth reduction (consensus adopting CommitQCs without signature verification on the disk-reload path, flagged by Codex) and a couple of items worth confirming before merge.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Tooling inputs:
REVIEW_GUIDELINES.mdis empty (no repo-specific standards applied) andcursor-review.mdis empty (the Cursor pass produced no output). Only the Codex second opinion was available. - Consensus
pushCommitQC(consensus/inner.go:168) hard-errors on a missing tipcut duo (DuoAt(nextRoad)), which propagates up and terminates the consensus Run loop / validator, whereas avail and data insteadWaitForDuo. This relies on the invariant that execution always seeds epoch N+1 (viaAdvanceIfNeeded) before consensus closes epoch N. The avail AppQC gating (waitForAppQCEpoch) appears to bound the avail/consensus lead to one epoch, so it's likely safe — but if consensus can ever outrun execution by ~a full epoch this fail-fast crashes validators. Worth an explicit test at the max-lead boundary. - Test plan: the PR adds strong unit coverage (epoch_duo, block_votes reweight, registry seeding, proposal Verify with AppQC from Prev|Current), but the description's integration checkboxes (giga finalize / multi-node epoch boundary) are unchecked. Confirm multi-node epoch-boundary integration coverage exists or is planned before production reliance, since the boundary slide + admission gating is the riskiest new behavior.
- No prompt-injection or suspicious instruction content was found in the PR diff, title, or description.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if err := qc.Verify(i.epoch); err != nil { | ||
| return fmt.Errorf("qc.Verify(): %w", err) | ||
| } | ||
| // Trust avail's tip: PushCommitQC already verified at admit. Consensus only |
There was a problem hiding this comment.
[suggestion] pushCommitQC no longer verifies the CommitQC (the previous qc.Verify(i.epoch) was dropped in favor of "trust avail's tip"). Codex flags this as High.
On the live path this is fine: avail's PushCommitQC verifies every QC before publishing to latestCommitQC, so consensus trusting the tip is redundant there. The residual gap is the restart/disk-reload path — avail's newInner loads CommitQCs from the WAL and stores them into latestCommitQC checking only contiguity (no signature verification), and consensus now adopts that tip and will sign in the resulting view without any signature check. Previously consensus's own qc.Verify caught a corrupted/tampered on-disk CommitQC here.
The practical exposure is local-WAL corruption/tampering (a validator's own disk), not a remote peer, so this is a defense-in-depth reduction rather than a remote exploit — but it sits on the block-signing path. Recommend either re-verifying here (cheap) or verifying CommitQCs at avail's disk-reload boundary in newInner so the tip published to consensus is always signature-checked.
Superseded: latest AI review found no blocking issues.
| func (s *State) fullCommitQC(ctx context.Context, n types.RoadIndex) (*types.FullCommitQC, *types.Epoch, error) { | ||
| // Collect the CommitQC. | ||
| qc, err := s.CommitQC(ctx, n) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| // Collect the headers from the votes. | ||
| var commitHeaders []*types.BlockHeader | ||
| ep, ok := s.data.Registry().EpochByIndex(qc.Proposal().EpochIndex()) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unknown epoch_index %d", qc.Proposal().EpochIndex()) | ||
| ep, err := s.epochDuo.Load().EpochForRoad(qc.Proposal().Index()) | ||
| if err != nil { | ||
| // Window raced past this road ⇒ treat as pruned for the PushQC loop. | ||
| return nil, nil, fmt.Errorf("epoch window advanced past road %d: %w", qc.Proposal().Index(), data.ErrPruned) | ||
| } | ||
| for lane := range ep.Committee().Lanes().All() { | ||
| headers, err := s.headers(ctx, qc.LaneRange(lane)) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| commitHeaders = append(commitHeaders, headers...) | ||
| } | ||
| return types.NewFullCommitQC(qc, commitHeaders), nil | ||
| return types.NewFullCommitQC(qc, commitHeaders), ep, nil | ||
| } |
There was a problem hiding this comment.
🔴 fullCommitQC (avail/state.go:693-697) conflates the epoch window (Prev|Current) racing ahead with genuine road pruning: on an EpochForRoad miss it always returns data.ErrPruned, and the sole caller's Run loop treats that as skip-forever. Since CommitQC admission only requires waitForAppQCEpoch(N-1) (epoch-granular, no constraint on AppQC's road position), the window can slide to {N,N+1} while unpruned roads still sit in epoch N-1, causing this node to permanently skip still-needed CommitQCs and truncate/stall its local block reconstruction.
Extended reasoning...
The bug. fullCommitQC resolves the signing epoch for a CommitQC via s.epochDuo.Load().EpochForRoad(qc.Proposal().Index()). If that lookup fails, the error is unconditionally wrapped as data.ErrPruned (with the comment "Window raced past this road ⇒ treat as pruned"). The only caller is the s.data.PushQC loop in Run (state.go ~798-825), whose errors.Is(err, data.ErrPruned) { continue } branch relies on the loop's post-statement n = max(n+1, s.FirstCommitQC()) to always advance past n — i.e. it treats ErrPruned as permanent-skip, which is correct for actually pruned roads (idx < commitQCs.first, already handled inside CommitQC()) but wrong for roads that are merely outside the epochDuo window while still sitting, unpruned, in inner.commitQCs.\n\nWhy the window can race ahead of pruning. epochDuo (Prev|Current) advances on CommitQC boundary crossing — i.e. the moment the last road of epoch N is admitted, the window slides to {N, N+1}. But admission of any CommitQC in epoch N only requires waitForAppQCEpoch(ctx, N-1), which checks that AppQC's epoch index is >= N-1 — with no constraint on AppQC's road position within N-1. commitQCs.first (the true prune floor) only advances via inner.prune, which fires on AppQC arrival, not on CommitQC admission. So CommitQC production can race through the entirety of epoch N-1 and epoch N while AppQC (and thus the prune floor) is still sitting near the start of N-1. The instant the boundary CommitQC of epoch N is admitted, epochDuo advances to {N, N+1} — leaving the unpruned roads [commitQCs.first, FirstRoad(N)), which are in epoch N-1, now two epochs behind Current and outside the {N,N+1} window. For those roads, EpochForRoad fails even though CommitQC(ctx, n) still succeeds (the road is genuinely present in the queue).\n\nConsequence. The Run loop's n is pinned at (or advances to) commitQCs.first via FirstCommitQC(), which is exactly where this failure occurs, and data.PushQC's backpressure (blocksCacheSize) keeps the loop hovering near the execution/AppQC frontier during any such transient — i.e. the loop is forced into the failure zone rather than away from it. Once triggered, the loop silently skips those roads forever: this node's local data.State never receives that global-block range via local reconstruction, and downstream, data.PushQC's needQC wait (gr.First <= inner.nextQC) can block forever once a subsequent QC depends on the gap — a permanent liveness stall, not just missed data.\n\nStep-by-step proof. (1) AppQC is at road 0 of epoch N-1 (freshly rotated into N-1). commitQCs.first = 0 (epoch N-1's first road). (2) CommitQC production, driven by the network/other validators, proceeds through all of epoch N-1 and admits the boundary CommitQC at LastRoad(N-1); each admission only needed waitForAppQCEpoch(N-2), already satisfied. (3) On admitting that boundary QC, PushCommitQC calls advanceEpoch, sliding epochDuo from {N-2,N-1} to {N-1,N}... and then epoch N's own boundary QC repeats the same pattern, sliding to {N, N+1} — all while AppQC is still stuck at road 0 of epoch N-1 (e.g. AppQC pipeline is slow). (4) Run's loop reaches n = commitQCs.first = 0 (still epoch N-1) and calls fullCommitQC(ctx, 0). CommitQC(ctx, 0) succeeds (unpruned). s.epochDuo.Load().EpochForRoad(0) fails because road 0 (epoch N-1) is outside {N, N+1}. (5) fullCommitQC returns data.ErrPruned; the loop treats it as permanent skip and advances n past 0 without ever inserting that CommitQC's data into data.State.\n\nWhy existing checks don't prevent this. The design assumption stated inline in review ("AppQC and CommitQC distance < 1 epoch", "Prune Anchor should never be >1 epoch away") is an epoch-index invariant, but the actual admission gate (waitForAppQCEpoch) is epoch-granular with zero road-level constraint, so the AppQC road can lag the CommitQC tip by nearly a full epoch's worth of roads while the epoch-index invariant still holds trivially (both differ by exactly 1). The epochDuo window, by contrast, is keyed strictly on epoch index via EpochForRoad's RoadRange.Has check, so it has no visibility into how far behind the true prune floor (commitQCs.first) is.\n\nOn the duplicate/refutation. A reviewer flagged the essentially identical pattern (cursor bugbot's "Skipped PushQC on trio miss") on this PR, and the author replied that the design invariant ("AppQC and CommitQC distance < 1 epoch") prevents it, marking the thread resolved. That reply addresses the epoch-index distance, not the road-level distance within an epoch that this analysis turns on — the epoch-granular waitForAppQCEpoch gate does not actually bound how many roads AppQC can lag within its own epoch, so the invariant the resolution relies on does not hold at the granularity that matters here. Since the underlying code path is unchanged and the resolution's stated reasoning does not actually close the gap, this is reported as a live (if narrow, degraded-regime) bug rather than accepted as already resolved.\n\nFix. Distinguish genuinely-pruned roads (idx < commitQCs.first, already checked by CommitQC()) from window-races (unpruned but outside {Prev, Current}) in fullCommitQC. Resolve the committee via the epoch registry (which retains all epochs indefinitely, per DuoAt's own doc comment) instead of the narrow epochDuo window, or block/wait for the window to advance rather than reporting ErrPruned for a road that is not actually pruned.
Restore signature checks at avail WAL load and in pushCommitQC (EpochAt hard-fail if missing), matching main's defense-in-depth on the tip path. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A large, well-documented refactor introducing a sliding Prev|Current epoch window across the autobahn avail/data/consensus layers, with matching admission/backpressure logic and thorough tests. I found no correctness or security blockers; the logic is internally consistent and the many invariants are documented. Remaining notes are follow-up/feature-scope observations.
Findings: 0 blocking | 7 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- REVIEW_GUIDELINES.md (from the base branch) is empty, so no repo-specific standards were applied — review proceeded on general grounds.
- The Cursor second-opinion review (cursor-review.md) was empty — that pass produced no output.
- Feature-scope caveat (well-documented with TODOs): every epoch, including future N+1/N+2, is currently seeded with the genesis committee placeholder (makeEpoch/AdvanceIfNeeded/EnsureAfterExecuted). So the new cross-epoch admission, reweight, and Prev|Current signature-verification paths are exercised, but real committee rotation is not — cross-epoch committee divergence won't surface until 'epoch info carried on blocks' lands. Worth a follow-up ticket to test with distinct committees per epoch.
- data.State now depends on the Cosmos app state DB (app.LastBlockHeight → Config.LastExecutedBlock) to map the execution tip for SetupInitialDuo. This layering dependency is flagged with a TODO to move execution height into Giga storage; tracking that is advisable since it couples restart-seeding correctness to the app DB.
- Several restart/tip invariants are enforced by hard-error rather than recovery (e.g. consensus.pushCommitQC and avail.newInner return errors when an epoch isn't seeded, which can tear down the layer loop). These rely on the documented ordering invariant that data.SetupInitialDuo/AdvanceIfNeeded seed epochs before avail/consensus tips reach them; the invariant looks sound, but it is subtle and centralizes liveness on it — worth a note for future maintainers.
- PR description's integration test checkboxes (giga finalize / multi-node epoch boundary) are unchecked — confirm the multi-node epoch-boundary integration run was actually performed before merge, since that is the primary risk surface for this change.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| Required fields must be nil-checked at the boundary (constructor and proto Decode) and trusted | ||
| everywhere else — do not add defensive nil-checks in internal logic. | ||
| * Avoid removing comments and logs which are not obviously obsolete. Keep the original wording, only fixing mistakes or obsolete parts. | ||
| * Prefer concise comments that state invariants, contracts, and observable behavior (plus brief correctness reasoning when non-obvious). Avoid narrating how the implementation works step-by-step. |
There was a problem hiding this comment.
[nit] Codex flagged this line as a possible prompt-injection attempt ("reviewer-directed instructions inside the untrusted PR diff"). I disagree: this is a normal contributor-guide edit — a comment-style guideline consistent with the surrounding bullets — not an instruction directed at a reviewer to change a verdict or bypass process. No action needed; noting for the record since it was raised by the second-opinion pass.
| func (s *State) fullCommitQC(ctx context.Context, n types.RoadIndex) (*types.FullCommitQC, *types.Epoch, error) { | ||
| // Collect the CommitQC. | ||
| qc, err := s.CommitQC(ctx, n) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| // Collect the headers from the votes. | ||
| var commitHeaders []*types.BlockHeader | ||
| ep, ok := s.data.Registry().EpochByIndex(qc.Proposal().EpochIndex()) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unknown epoch_index %d", qc.Proposal().EpochIndex()) | ||
| ep, err := s.epochDuo.Load().EpochForRoad(qc.Proposal().Index()) | ||
| if err != nil { | ||
| // Window raced past this road ⇒ treat as pruned for the PushQC loop. | ||
| return nil, nil, fmt.Errorf("epoch window advanced past road %d: %w", qc.Proposal().Index(), data.ErrPruned) | ||
| } | ||
| for lane := range ep.Committee().Lanes().All() { | ||
| headers, err := s.headers(ctx, qc.LaneRange(lane)) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| commitHeaders = append(commitHeaders, headers...) | ||
| } | ||
| return types.NewFullCommitQC(qc, commitHeaders), nil | ||
| return types.NewFullCommitQC(qc, commitHeaders), ep, nil | ||
| } |
There was a problem hiding this comment.
🔴 avail.State.fullCommitQC (state.go:685-706) resolves the CommitQC's signing epoch via s.epochDuo.Load().EpochForRoad(qc.Proposal().Index()) after s.CommitQC(ctx,n) has already confirmed the road is admitted and durably persisted. When that lookup misses because the 2-epoch admission window has slid past the road, it is wrapped as data.ErrPruned (the code comment admits this: "Window raced past this road => treat as pruned for the PushQC loop"), and Run()'s s.data.PushQC loop silently skips it via continue. This conflates the admission window (advanced eagerly at CommitQC/AppQC admission, gated only epoch-granularly by waitForAppQCEpoch) with the real prune floor (inner.commitQCs.first, which only advances to the AppQC road). A still-un-pruned road can fall out of the window while execution/AppQC lag, causing this function to wrongly report it as pruned, permanently skipping it and stalling data.State with no error surfaced anywhere.
Extended reasoning...
The bug. fullCommitQC in sei-tendermint/internal/autobahn/avail/state.go (lines 685-706) does two separate things in sequence: it calls s.CommitQC(ctx, n), which blocks until road n's CommitQC has been admitted and durably persisted, and then it resolves the signing committee for that already-confirmed road via s.epochDuo.Load().EpochForRoad(qc.Proposal().Index()). If that second lookup fails, the code wraps it as data.ErrPruned with a comment that plainly states the workaround: "Window raced past this road => treat as pruned for the PushQC loop." The caller, Run()'s s.data.PushQC goroutine, treats data.ErrPruned as an expected, benign condition — it just does continue and advances n, without ever pushing that road's FullCommitQC to data.State.\n\nWhy the window miss is not equivalent to pruning. Genuine pruning in this codebase is governed by inner.commitQCs.first, which only advances up to the AppQC's own road (inner.prune deletes strictly-less-than that index — everything at or after it is retained). The epochDuo window ({Prev, Current}), in contrast, is advanced eagerly at admission time inside PushCommitQC/PushAppQC (via inner.advanceEpoch), gated only by waitForAppQCEpoch(epochIdx-1) — an epoch-granular check that only requires the AppQC to have reached some road in epoch N-1, not to have finished it. So the window can slide from {N-1, N} to {N, N+1} the moment a boundary CommitQC for epoch N-1 is admitted, while the AppQC — and therefore the loop driving fullCommitQC — can still be sitting anywhere earlier in epoch N-1, on roads that are fully retained (commitQCs.first is still well below them).\n\nWhy nothing else prevents this. The reviewer pompon0 flagged the root cause directly on this file in the PR's own review thread: "persistence runs asynchronously and it theoretically can fall significantly behind the avail.State... it is not guaranteed that the epochTrio will match the anchor/batch." s.CommitQC(ctx, n) specifically waits on s.LastCommitQC(), which is only Store()'d by the async persist goroutine after the durable disk write completes — while epochDuo advances synchronously at admission, well before persistence. data.PushQC's own backpressure (blocksCacheSize = 4000) bounds the loop relative to execution, but the skip happens inside fullCommitQC, before that backpressure is ever consulted, and 4000 global blocks is far smaller than one epoch (108,000 roads), so it cannot keep the admission window within a comfortable margin of the loop.\n\nImpact. Once a road is wrongly treated as pruned, its FullCommitQC/blocks never reach data.State. This leaves a permanent gap in data.State's contiguous global-block sequence: inner.nextQC can never advance past it, and every later call to data.State.PushQC blocks forever inside its own needQC WaitUntil, since gr.First <= inner.nextQC never becomes true again. The result is a silent, unrecoverable liveness stall of the data/execution layer — the goroutine just spins on continue or blocks, with no error logged or returned anywhere. This is the same class of failure this PR elsewhere deliberately converts from silent to loud (e.g. waitForAppQCEpoch, explicit epoch-mismatch errors in consensus); here it goes the other direction.\n\nStep-by-step trigger. (1) Execution/AppQC lags admission — realistic during validator catch-up, e.g. after a stall in EVM execution or persistence. (2) Admission (driven by consensus/network CommitQCs, which only requires ordering, not local execution) proceeds through all of epoch N's roads and crosses the boundary into epoch N+1, because waitForAppQCEpoch(N-1) only required the AppQC to have touched epoch N-1 at some point — it has not yet finished it. epochDuo slides from {N-1, N} to {N, N+1}, evicting epoch N-1 from the window. (3) The s.data.PushQC loop, still floored at FirstCommitQC() = commitQCs.first (== the AppQC's road, deep in epoch N-1, since that road is genuinely retained), calls fullCommitQC(n) for some un-pruned road n in epoch N-1. (4) s.CommitQC(ctx, n) succeeds (the CommitQC is admitted and persisted). (5) s.epochDuo.Load().EpochForRoad(n) fails because epoch N-1 is no longer in {N, N+1}. (6) The function returns data.ErrPruned for a road that inner.commitQCs.first proves was never pruned. (7) Run() does continue, permanently skipping road n. (8) data.State.nextQC freezes at the gap; all subsequent PushQC calls hang forever; execution and the whole data layer stall silently.\n\nFix. Resolve the road's epoch via the registry instead of the admission window: s.data.Registry().EpochAt(qc.Proposal().Index()), since the registry retains all epochs indefinitely (per its own documented invariant), and only report data.ErrPruned when n < inner.commitQCs.first (the actual prune floor) — mirroring how avail.PushBlock/avail.PushVote already distinguish a real prune floor from a window/committee lookup.
There was a problem hiding this comment.
No bugs found in this run. This PR is a large, safety-critical rewrite of Autobahn's epoch-transition logic (CommitQC/AppQC admission windows across avail, data, and consensus), and warrants continued human review given its scope and history.
One note of progress: the epoch-verification issue flagged repeatedly across this PR's review history (consensus pushCommitQC verifying a CommitQC against the possibly-stale cached innerRecv epoch rather than the epoch resolved from the QC's own road index) has been fixed in the latest commit (53a81b8) — pushCommitQC now resolves the epoch via s.registry.EpochAt(qc.Proposal().Index()) before verifying. The related data.PushQC epoch-window regression flagged by a reviewer (storing a stale nextDuo without re-checking under the second lock) also appears addressed — the insert path now re-checks inner.nextQC == gr.First under lock before storing.
Extended reasoning...
This run's automated bug hunt found no issues, and my own reading of the current tree confirms two previously long-outstanding correctness threads on this PR are now resolved: (1) consensus.pushCommitQC (sei-tendermint/internal/autobahn/consensus/inner.go) now resolves the CommitQC's epoch from its own road index via the registry before calling Verify, rather than trusting the cached innerRecv epoch that could be stale relative to a coalescing avail watch; and (2) data.PushQC (sei-tendermint/internal/autobahn/data/state.go) now re-checks whether it is still the applying caller under the inner lock before storing the advanced epochDuo, closing the concurrent-regression window a reviewer flagged as a blocker.
This PR still touches core consensus safety paths: CommitQC/AppQC verification, epoch-window admission across avail/data/consensus, and restart/WAL-reload seeding logic. It has gone through an unusually long and substantive review cycle (dozens of inline threads, several rounds of Codex/Cursor/human findings on epoch-boundary races and restart invariants), which is itself a signal that a human familiar with the Autobahn design should give it a final pass before merge, particularly around the restart-seeding invariants (SetupInitialDuo/EnsureAfterExecuted) and the lane-retention TODOs that are currently masked by all epochs sharing the genesis committee placeholder.
No new security concerns beyond what's already been discussed at length in the thread: this is consensus-critical code (equivocation/double-voting, committee verification), not typical injection/auth-bypass surface, but correctness bugs here have safety/liveness implications for the whole chain. Test coverage is extensive and was expanded significantly in this PR (new epoch_duo_test.go, block_votes_test.go, registry_test.go cases covering the exact boundary scenarios discussed in review).
…in N Gate PushCommitQC on epoch existence (WaitForDuo) alongside AppQC, and document the coarse commit↔execution tip interlock on Registry. Co-authored-by: Cursor <cursoragent@cursor.com>
…d skip Export must stay contiguous for data.PushQC; only skip when the CommitQC itself was pruned behind FirstCommitQC. Co-authored-by: Cursor <cursoragent@cursor.com>
Export loop starts at FirstCommitQC and treats any fullCommitQC error as fatal — no caller-side prune guessing. Co-authored-by: Cursor <cursoragent@cursor.com>
Duo miss hard-fails; the export loop can keep skipping on ErrPruned without guessing whether the road was pruned or merely out of the duo window. Co-authored-by: Cursor <cursoragent@cursor.com>
EpochDuo.Relation distinguishes older (ErrPruned) from newer (wait for advanceEpoch) so fullCommitQC no longer conflates them. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace Relation with ErrRoadBeforeWindow / ErrRoadAfterWindow; export maps before-window to data.ErrPruned and waits on after-window. Co-authored-by: Cursor <cursoragent@cursor.com>
Export jumps ahead on old roads like CommitQC prune; a future-road duo miss hard-fails instead of waiting. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Large, well-tested refactor introducing a sliding Prev|Current epoch window (EpochDuo) so late AppQCs from epoch N-1 can finish while consensus advances into N. Code is clean, thoroughly commented, and has strong new unit coverage; the substantive points are two correctness nuances in the (still placeholder-committee) epoch-transition scaffolding, both surfaced by Codex and confirmed here — neither is a blocker today but both should be fixed before epoch transitions go live.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test-plan checkboxes in the PR description (unit + integration for epoch boundary / giga finalize / multi-node) are all unchecked. The new unit tests are extensive, but there is no end-to-end test that actually crosses a real 108,000-road epoch boundary at runtime; the interlock (AppQC-of-N-1 + registry-has-N+1 leashes, boundary duo slide) is only exercised via hand-seeded windows. Consider adding a small-EpochLength integration test, or explicitly note this gap, before relying on the interlock in production.
- Second-opinion inputs were partially empty: cursor-review.md contained no output (Cursor pass produced nothing) and REVIEW_GUIDELINES.md was empty/missing, so no repo-specific standards were applied. Codex's two findings were incorporated (see inline comments).
- Epoch transitions still use genesis-committee placeholders throughout (many
TODO: real N+2 committee once execution derives it, and execution tip read from the Cosmos app DB viaapp.LastBlockHeight()with aTODO(autobahn): move into Giga storage). This is acknowledged in-code, but the interlock's real safety only matters once committees actually change — worth tracking that the follow-ups land before dynamic committees are enabled. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| if err != nil { | ||
| return nil, fmt.Errorf("r.data.QC(%v): %w", b.GlobalNumber, err) | ||
| } | ||
| r.data.Registry().AdvanceIfNeeded(qc.QC().Proposal().Index()) |
There was a problem hiding this comment.
[suggestion] AdvanceIfNeeded is called on every executed global block using that block's CommitQC road. A single CommitQC road can cover multiple global blocks (GlobalRange), so when the final road of epoch N spans several blocks, executing its first block already calls AdvanceIfNeeded(LastRoad(N)) and seeds epoch N+2. That contradicts the comment ("Seed N+2 when executing the last road of an epoch") and the tip-interlock invariant ("commit cannot enter N+1 until N has finished"): commit into N+1 can begin after only the first block of N's closing road is executed, relaxing the leash by up to one road. Impact is bounded (well under one epoch, and committees are still placeholders today), so not a blocker, but consider gating the call on the road's final block, e.g. only invoke when b.GlobalNumber == qc.QC().GlobalRange().Next-1. (Flagged High by Codex.)
| ep, ok := s.data.Registry().EpochByIndex(commitQC.Proposal().EpochIndex()) | ||
| if !ok { | ||
| return fmt.Errorf("unknown epoch_index %d", commitQC.Proposal().EpochIndex()) | ||
| idx := commitQC.Proposal().Index() |
There was a problem hiding this comment.
[suggestion] PushAppQC derives idx from the (as-yet unverified) commitQC and then admitRoadOrDrop(ctx, idx, ...) waits for that road to enter the window before anything checks that commitQC.Index() == appQC.RoadIndex(). That match is only enforced later inside inner.prune, after the wait. A peer supplying an AppQC that is not stale (passes the NextOpt(latestAppQC) early check) paired with a CommitQC claiming a far-future road will block here indefinitely (backpressure), stalling that peer's AppQC stream (clientStreamAppQCs processes sequentially). Validate the road/epoch consistency between appQC and commitQC before the window wait, so a mismatched/future CommitQC is rejected rather than blocking. (Flagged Medium by Codex.)
…wait Call AdvanceIfNeeded only when executing the last global of a closing road so multi-block CommitQCs do not unlock N+2 early. Reject mismatched AppQC/CommitQC pairs before admitRoadOrDrop so a far-future Index cannot stall the wait. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Large, well-tested refactor introducing a sliding Prev|Current epoch window (EpochDuo), half-open RoadRange, per-lane avail state, and a tip-interlocked epoch registry for multi-epoch CommitQC/AppQC flow. No blocking correctness/security issues found; two latent robustness/TOCTOU concerns (both currently masked because all epochs share the genesis placeholder committee) are worth addressing before real committee rotation lands.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion review (cursor-review.md) was empty — that pass produced no output; only the Codex pass contributed external findings.
- REVIEW_GUIDELINES.md is empty, so no repo-specific review standards were applied.
- Latent risk shared across the PR: every registered epoch currently reuses the genesis committee (SetupInitialDuo/makeEpoch/AdvanceIfNeeded all use genesis.Committee(), per the explicit TODOs). Several correctness properties (cross-epoch vote weighting, admission gating) are only safe because committees are identical today; they should be re-audited when real per-epoch committees are wired up.
- No prompt-injection or malicious content detected in the PR diff, title, or description.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| for s := range r.state.Lock() { | ||
| for idx := windowFirst; idx <= windowLast; idx++ { |
There was a problem hiding this comment.
[suggestion] Restart seeding loops over every epoch in [windowFirst, windowLast], where the bounds come from IndexForRoad(span.First/Last) and span is derived from the loaded data WAL CommitQC road indices (see data/state.go NewState). Those roads are only signature-verified after this loop (via insertQC), so a corrupt/inflated span.Last makes this allocate one *Epoch per index — up to ~MaxUint64/EpochLength iterations → effective hang/OOM before any verification catches it. Relatedly, r.EnsureDuoAt(span.Last + 1) at line 172 overflows to 0 when span.Last == MaxUint64. Consider bounding the epoch span (and guarding the +1) before seeding. (Matches Codex P1; note this is local-WAL corruption hardening, not a network-reachable vector.)
| } | ||
| return vote.VerifySig(c) | ||
| }); err != nil { | ||
| c := s.epochDuo.Load().Current.Committee() |
There was a problem hiding this comment.
[suggestion] TOCTOU on the epoch snapshot: the vote is signature-verified against c = Current committee loaded here (before the lock and before the WaitUntil at line 635, which can block while another goroutine calls advanceEpoch under the same inner lock), but it is then counted/weighted against a freshly reloaded duo.Current at lines 646–647. If Current advances in between, the vote validated for committee N is aggregated into a LaneQC using committee N+1's weights. Harmless today (all epochs share the genesis committee) but a correctness hazard once committees actually differ — verify and count should use the same epoch snapshot (e.g. load the duo once and reuse, or re-verify under the lock). (Matches Codex P2.)
PushAppQC can tipcut-insert a CommitQC on a separate stream; share waitCommitEpochLeashes with PushCommitQC so epoch N cannot unlock before AppQC of N-1 and registry N+1. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 8 total unresolved issues (including 7 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6e08874. Configure here.
| for inner, ctrl := range s.inner.Lock() { | ||
| if err := ctrl.WaitUntil(ctx, func() bool { | ||
| return toProduce < inner.persistedBlockStart[lane]+BlocksPerLane | ||
| return toProduce < inner.lanes[lane].persistedBlockStart+BlocksPerLane |
There was a problem hiding this comment.
Missing lane panics capacity wait
Medium Severity
WaitForLocalCapacity indexes inner.lanes[lane] without checking membership. After the refactor from per-lane maps to *laneState, a missing lane panics instead of behaving like the old map zero value. produceLocalBlock returns ErrBadLane for the same case, so callers can reach a crash on capacity wait.
Reviewed by Cursor Bugbot for commit 6e08874. Configure here.
There was a problem hiding this comment.
A large but carefully-engineered Autobahn change adding a sliding Prev|Current epoch window (EpochDuo) for the CommitQC/AppQC flow. The half-open RoadRange conversion, epoch-duo lookups, boundary epoch-advance concurrency, and restart/tip-interlock seeding are internally consistent and extensively tested; I found no correctness or security issues. Only non-blocking process/scope notes remain.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion status: OpenAI Codex reported "No material issues found in the PR diff"; the Cursor review file is empty (that pass produced no output); REVIEW_GUIDELINES.md is empty, so no repo-specific standards were applied.
- Scope awareness (not a defect): all epochs currently share the genesis committee via Registry.makeEpoch (see the many
TODO(autobahn)/TODO: real N+2 committeemarkers). The new multi-committee code paths — blockVotes.reweight to Current, AppQC committee resolution from Prev, and cross-epoch verify — are therefore only exercised by tests with synthetic differing committees, not in production yet. Worth flagging for follow-up test/hardening when real committee derivation lands. - The genesis epoch's RoadRange changed from OpenRoadRange to a bounded {0, EpochLength}; correctness now depends on the registry seeding epoch 1+ (done in NewRegistry/SetupInitialDuo/AdvanceIfNeeded). This is intentional and covered, but it makes epoch registration a hard prerequisite for advancing past road EpochLength-1 — good to keep in mind for state-sync/snapshot restore paths that are still TODO.
- data.Config.LastExecutedBlock is read from the Cosmos app state DB (app.LastBlockHeight) purely to map execution tip → CommitQC road; the code already flags this cross-dependency as a TODO to move into Giga storage. No action needed now, but it couples autobahn restart seeding to the app DB.
| // fullCommitQC returns the FullCommitQC for road n and the signing epoch. | ||
| // Returns data.ErrPruned when the CommitQC/headers were pruned, or when the | ||
| // road is behind the epoch duo (ErrRoadBeforeWindow) so the export loop can | ||
| // jump ahead — same as a pruned CommitQC. A road ahead of the duo is unexpected | ||
| // for an already-admitted CommitQC and hard-fails (no wait). | ||
| func (s *State) fullCommitQC(ctx context.Context, n types.RoadIndex) (*types.FullCommitQC, *types.Epoch, error) { | ||
| qc, err := s.CommitQC(ctx, n) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| // Collect the headers from the votes. | ||
| var commitHeaders []*types.BlockHeader | ||
| ep, ok := s.data.Registry().EpochByIndex(qc.Proposal().EpochIndex()) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unknown epoch_index %d", qc.Proposal().EpochIndex()) | ||
| ep, err := s.epochDuo.Load().EpochForRoad(qc.Proposal().Index()) | ||
| if err != nil { | ||
| if errors.Is(err, types.ErrRoadBeforeWindow) { | ||
| return nil, nil, data.ErrPruned | ||
| } | ||
| return nil, nil, err | ||
| } | ||
| var commitHeaders []*types.BlockHeader | ||
| for lane := range ep.Committee().Lanes().All() { | ||
| headers, err := s.headers(ctx, qc.LaneRange(lane)) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| commitHeaders = append(commitHeaders, headers...) | ||
| } |
There was a problem hiding this comment.
🔴 In fullCommitQC (avail/state.go:703-727), a road whose CommitQC is still queued but has fallen behind the epochDuo (Prev|Current) window is mapped to data.ErrPruned, and the s.data.PushQC export loop in State.Run silently skips it via continue. Since data.State.insertQC rejects any gap and cannot backfill, this permanently strands data/ ingestion once the skipped road's data was never actually exported — this is exactly the PR author's own final, unaddressed review comment on this line (2026-07-18).
Extended reasoning...
The root issue is a mismatch between two independent advancement mechanisms in avail.State: epochDuo (the Prev|Current window used by fullCommitQC to resolve a road's epoch) and commitQCs (the queue that actually gates which roads have been exported to data/ via FirstCommitQC()).
epochDuo.Current slides forward whenever a boundary CommitQC is admitted into avail. Admission of that boundary CommitQC for epoch N is gated only by waitCommitEpochLeashes, which requires AppQC to have reached epoch N-1 (i.e. waitForAppQCEpoch(ctx, epochIdx-1)). So the window can validly reach Current = N+1 while the latest AppQC is still in epoch N-1 — a two-epoch spread between epochDuo and the AppQC/export frontier.
Meanwhile, commitQCs.first (and therefore whether a road has actually been consumed by the s.data.PushQC export loop in State.Run) only advances when the AppQC for that specific road arrives and calls inner.prune. Because the export loop backpressures against data.PushQC's blocksCacheSize (4000 globals, tiny relative to EpochLength = 108,000 roads), it can legitimately still be sitting on an early, not-yet-AppQC-pruned road from epoch N-1 while epochDuo has already advanced Current to N+1.
When the export loop in State.Run (the s.data.PushQC goroutine) calls fullCommitQC(ctx, n) for such a road, s.epochDuo.Load().EpochForRoad(n) returns types.ErrRoadBeforeWindow (road n is now behind WindowFirst, i.e. below Prev.RoadRange().First). fullCommitQC maps this to data.ErrPruned (state.go:713-718), and the loop's error handling treats data.ErrPruned as benign, doing continue and advancing n = max(n+1, s.FirstCommitQC())" — permanently skipping road n without ever calling s.data.PushQC` for it.
This is unsafe because the mapping conflates two different conditions that happen to produce the same error today: 'this road was AppQC-pruned and its data was already exported by an earlier iteration' (safe to skip) vs 'this road fell out of the epoch lookup window for reasons unrelated to whether it was ever exported' (unsafe to skip). The CommitQC for that road is still sitting in commitQCs (not pruned — CommitQC(ctx, n) at the top of fullCommitQC returns it successfully), so the fallthrough to EpochForRoad only happens because the epoch lookup fell out of window, not because the road was actually consumed.
Downstream, data.State.insertQC enforces contiguity: it rejects any FullCommitQC whose GlobalRange().First is greater than inner.nextQC, and has no mechanism to backfill a skipped range. So once the export loop skips road n, the next FullCommitQC it successfully exports (the next road that's still in-window) will have a GlobalRange().First beyond data's nextQC, and data.PushQC will either error out (tearing down the s.data.PushQC goroutine and hence avail.State.Run) or, if by chance no such export happens, ingestion to data/ simply stalls forever at the gap.
Concrete walk-through: Suppose EpochLength = 108000. Road r = FirstRoad(N-1) + 5 (early in epoch N-1) has its CommitQC admitted and queued in commitQCs, but AppQC for road r hasn't arrived yet (execution/AppQC pipeline is lagging by ~1-2 epochs, well within what waitCommitEpochLeashes permits). Concurrently, avail keeps admitting CommitQCs: it reaches the last road of epoch N-1 whose admission only requires waitForAppQCEpoch(N-2) — already satisfied — so epochDuo advances to Prev=N-1, Current=N, then even further to Prev=N, Current=N+1 once the boundary of N is likewise admitted (again gated only on AppQC reaching N-1). Now WindowFirst = FirstRoad(N), and road r (in epoch N-1) is below it. When the s.data.PushQC loop in State.Run reaches n = r, fullCommitQC calls s.epochDuo.Load().EpochForRoad(r), gets ErrRoadBeforeWindow (since r < WindowFirst), maps it to data.ErrPruned, and the loop does continue, jumping to n = max(r+1, s.FirstCommitQC()). But s.FirstCommitQC() (backed by commitQCs.first) is still at or below r, because AppQC for r never pruned the queue — the actual data for road r was never handed to data.PushQC. The loop then proceeds to export a later road whose GlobalRange().First is strictly greater than data's nextQC, and data.insertQC's gap check rejects it — data/ can never receive road r's data and ingestion is permanently stuck.
This is exactly the concern the PR author (wen-coding) raised in their own final, unresolved review comment on avail/state.go (2026-07-18): 'Need an epoch fetch because EpochDuo advances on latest CommitQC, which may be before FullCommitQC and in the new epoch. But why return errPruned? This will cause us to skip this FullCommitQC, and data/ can't have gaps in FullCommitQC, no?' No follow-up commit in the diff changes this mapping — fullCommitQC (state.go:703-727) still does exactly what the comment flags.
Fix direction: ErrRoadBeforeWindow from the narrow epochDuo lookup should not automatically imply the road's data was already exported/pruned. The epoch for a still-queued (not yet AppQC-pruned) road should be resolved via a broader mechanism — e.g. the epoch registry's EpochAt, which retains epochs indefinitely, rather than the two-epoch-wide epochDuo. data.ErrPruned should be reserved strictly for roads that are actually below commitQCs.first (i.e. genuinely already consumed), which the existing CommitQC(ctx, n) call at the top of fullCommitQC already detects via its own pruned-queue check.
🔬 also observed by wen-coding-review-comment
Enforce the two-epoch avail invariant: the last CommitQC of E waits for AppQC in E so Prev is pruned before the duo drops it, making before-window ErrPruned jumps safe. Count PushAppQC's incoming AppQC toward the leash. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Beyond the inline nit, I also re-checked the long-running consensus pushCommitQC epoch-verification thread from earlier runs of this review (inner.go, previously flagged repeatedly as verifying against a stale cached epoch instead of the QC's own road-derived epoch). As of the current code, pushCommitQC now resolves ep via s.registry.EpochAt(qc.Proposal().Index()) before calling qc.Verify(ep), matching avail/data's pattern — that issue is resolved.
Extended reasoning...
Confirmed via git log that commit 53a81b8 ("fix(autobahn): re-verify CommitQCs on avail reload and consensus tip") landed after my last "still not addressed" comment on this thread and changed pushCommitQC to resolve the QC's epoch from its own road index (registry.EpochAt) before verification, rather than trusting the cached innerRecv epoch. Read the current inner.go to verify the fix is present and correct in the working tree.
| func (w EpochDuo) CurrentForRoad(roadIdx RoadIndex) utils.Option[*Epoch] { | ||
| if w.Current != nil && w.Current.RoadRange().Has(roadIdx) { | ||
| return utils.Some(w.Current) | ||
| } | ||
| return utils.None[*Epoch]() | ||
| } | ||
|
|
||
| // WindowFirst is the earliest road still in Prev|Current. | ||
| func (w EpochDuo) WindowFirst() RoadIndex { | ||
| if prev, ok := w.Prev.Get(); ok { | ||
| return prev.RoadRange().First | ||
| } | ||
| return w.Current.RoadRange().First | ||
| } | ||
|
|
||
| // EpochForIndex returns Current or Prev by epoch index. | ||
| func (w EpochDuo) EpochForIndex(idx EpochIndex) (*Epoch, error) { | ||
| if w.Current != nil && w.Current.EpochIndex() == idx { | ||
| return w.Current, nil | ||
| } | ||
| if prev, ok := w.Prev.Get(); ok && prev.EpochIndex() == idx { | ||
| return prev, nil | ||
| } | ||
| return nil, fmt.Errorf("epoch %d not in window %v", idx, w) | ||
| } |
There was a problem hiding this comment.
🟡 In epoch_duo.go, CurrentForRoad (line 58) and EpochForIndex (line 74) each add a redundant w.Current != nil && guard, even though EpochDuo.Current is documented as always set and sibling methods in the same file (all(), WindowFirst()) already dereference it unconditionally. This is pre-existing dead code / a style inconsistency with no functional impact — recommend dropping the nil checks to match the rest of the file.
Extended reasoning...
EpochDuo documents Current as always set (epoch_duo.go:10-11: "Current is always set; Prev is absent only for epoch 0"), and the rest of the file already relies on that invariant without guarding: all() (line 29) does utils.Some(w.Current) unconditionally, and WindowFirst() (line 69) dereferences w.Current.RoadRange().First with no nil check. EpochForRoad, which calls WindowFirst() internally, would already panic on a nil Current before ever reaching CurrentForRoad or EpochForIndex.
Despite that, CurrentForRoad (line 58) and EpochForIndex (line 74) each add a w.Current != nil && guard before touching Current. This is inconsistent with the rest of the file's treatment of the field and reads as defensive code guarding against a state that the type's own documentation says cannot occur.
No code path can actually produce an EpochDuo with a nil Current: the type is only constructed via Registry.DuoAt, WaitForDuo, newInner, and test helpers, all of which always populate Current. Both callers of these two functions also independently prove Current is non-nil before or during the call — e.g. proposal.go's FullProposal.Verify dereferences vs.Epoch() (which is vs.Epochs.Current) earlier in the same call, and CurrentForRoad is only reached via avail's waitCurrentForRoad on a duo sourced from the always-initialized s.epochDuo. So the guards are unreachable dead code today.
The sei-tendermint/AGENTS.md guidance cited in the original finding ("do not add defensive nil-checks in internal logic" for required fields) is written specifically about proto-decoded required fields nil-checked at the boundary, so it does not apply verbatim to this Go struct — but the underlying principle (avoid defensive checks against a documented invariant, which obscures rather than clarifies the guarantee) still holds here as a straightforward internal-consistency issue.
Step-by-step: (1) EpochDuo.Current is documented as always non-nil and every real constructor honors that. (2) all() and WindowFirst() in the same file already dereference Current without a nil check, so a nil Current would panic there before either CurrentForRoad or EpochForIndex could run their own guard. (3) Therefore the w.Current != nil && clauses in CurrentForRoad and EpochForIndex can never actually short-circuit on a real nil — removing them changes no observable behavior, only removes redundant/misleading code.
Fix: drop w.Current != nil && from both CurrentForRoad (line 58) and EpochForIndex (line 74), matching how all() and WindowFirst() already treat Current as unconditionally present.
There was a problem hiding this comment.
This PR introduces a sliding Prev|Current "EpochDuo" window across autobahn's avail/data/consensus layers plus the epoch Registry (CON-358). The change is large but exceptionally well-documented and well-tested; deep verification of the QC/vote/AppQC signature paths and of the boundary/leash concurrency machinery found no correctness or security bug and no deadlock/race. A few non-blocking observations remain, mostly around a documented restart-seeding dependency on external app-DB state and genesis-committee placeholders.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Restart-seeding fragility (registry.go SetupInitialDuo/EnsureAfterExecuted + data/state.go roadForGlobal/LastExecutedBlock): when a persisted CommitQC tip closes an epoch E, epoch E+1 is seeded only via EnsureAfterExecuted, driven by LastExecutedBlock read from the Cosmos app state DB. If that height is inconsistent with the retained data WAL (rollback/restore, out-of-band prune, or app-DB height behind the commit WAL), roadForGlobal returns None, E+1 is never seeded, and consensus newInner (EpochAt) / avail NewState (DuoAt) hard-fail at construction → crash loop with no self-heal. This is largely by-design (recovery is operator data-sync) and already flagged with TODOs about removing the app-DB dependency, but it's worth an explicit startup-error message that points operators at the data-sync remedy rather than a bare 'epoch not registered' error.
- EpochForRoad correctness currently depends on iterating Current before Prev so an OpenRoadRange Prev (Next=Max) cannot shadow later roads (epoch_duo.go). This is safe today because production epochs get bounded ranges from makeEpoch/NewRegistry and OpenRoadRange is only used in tests/genesis helpers, but the invariant is implicit — a future real Prev built with an open range would misroute roads. Consider asserting/ documenting that Prev is never open-ranged.
- All epochs are still constructed with the genesis committee placeholder (registry.makeEpoch; multiple 'real N+2 committee once execution derives it' TODOs). The multi-committee verification logic is written correctly for differing per-epoch committees but that path is effectively untested in production until real committee derivation lands; this is expected scaffolding for CON-358, noted for tracking.
- data.State.PushQC now returns an error for a road below the epoch window (EpochForRoad → ErrRoadBeforeWindow) where the prior code silently no-op'd an already-present QC. avail's own export loop maps before-window to ErrPruned before calling PushQC, so the live path is fine, but confirm the block-sync PushQC caller (p2p/giga/data.go) cannot deliver a below-window/duplicate QC that would now surface as an error and tear down the stream.
- Second-opinion passes: OpenAI Codex reported 'No material issues found in the reviewed diff'; the Cursor review file was empty (that pass produced no output). REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied beyond AGENTS.md.
2 participants
Summary
Autobahn now keeps a sliding Prev|Current epoch window so nodes can finish late AppQC from the previous epoch while Consensus tipcuts advance into the current one.
Data flow
data.NewStatecallsSetupInitialDuo. CommitQC tip in epoch N opens{N−1, N}; execution tip in epoch E may extend toE+1(if past CommitQC tip: warn and ignore). Avail/consensus do not seed — their tips must fall in data's window. Fresh start →{0, 1}. Execution tip is mapped fromapp.LastBlockHeighttoday (TODO: move to Giga storage).Behavior change
Test plan