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 43 commits into
Conversation
PR SummaryHigh Risk Overview Types: Availability: Tracks Reviewed by Cursor Bugbot for commit 17cdeb6. 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).
|
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.
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.
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.
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.
Superseded: latest AI review found no blocking issues.
c36ec91 to
d03b4f5
Compare
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.
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.
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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 12 total unresolved issues (including 11 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 6c4daa7. Configure here.
Peer FullCommitQC sync and async BlockDB flush can leave data ahead of avail without corruption; Run catch-up repairs it. Keep avail≥consensus only. 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 12 total unresolved issues (including 11 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 e88eb06. Configure here.
…(CON-358) Prev-epoch AppQC can still be unfinished at a boundary, so tipcuts accept Current|Current-1 and buildProposal clears same-road/ahead AppQC instead of shipping invalid proposals. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Large, coherent refactor introducing a Prev|Current EpochDuo window across avail/data/consensus plus a road-indexed epoch registry with explicit seeding and prune/execution leashes; the design is well documented and unusually well tested (registry, duo, leashes, restart interlock, synctest races on PushVote). No blocking correctness or security defects found — the notes below are hardening, robustness-on-restart, and test-coverage suggestions.
Findings: 0 blocking | 12 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output for this PR, so only Codex ("no material issues found") and this review cover the diff.- No end-to-end multi-node epoch transition test. Because
EpochLengthis aconst(108_000), every boundary test hand-plantsinner.commitQCs.first/next,latestAppQC, and the duo, andRunTestNetworknever leaves epoch 0. MakingEpochLengtha package-levelvar(or injecting it via config) would let one integration test drive an actual Current slide through avail → consensus → data →AdvanceIfNeeded, which is where the interlocking is most likely to be subtly wrong. inner.lanesis now an ever-growing map, andinner.pruneiterates it usingcommitQC.LaneRange(lane), which returnsNewLaneRange(lane, 0, None)for lanes absent from the QC — so retired lanes'blocks/votesqueues are never reclaimed and never pruned. Acknowledged asTODO(lane-expiry)and inert while all epochs share the genesis committee, but it needs to land with real committee rotation.- Related: the
TODO(lane-id)at avail/inner.go:81 and avail/state.go:178 means WAL-only lanes created bygetOrInsertLane(after the anchor-prune loop) keeppersistedBlockStart == 0. Once epochs have different committees,PushBlock/PushVotefor such a lane would block inWaitUntilindefinitely (n <= 0+BlocksPerLane-1). Worth a linked issue so it isn't lost. SetupInitialDuo,EnsureEpoch,EnsureDuoAt, andAdvanceIfNeededare all exported and unguarded, while the documented invariant is "data/ is the sole restart seeder; avail/consensus must not seed." Consider keeping the seeding entry point unexported (or requiring a caller token) so the invariant is enforced rather than commented.runProposereturns nil for an entire view whenWaitForLaneQCs's Current epoch differs fromvs.Epoch(), andPushPrepareVote/PushCommitVote/PushTimeoutVotesilently drop cross-epoch votes. Together the epoch boundary depends entirely on a view timeout for recovery; a counter/metric on both drop paths would make that cost observable in production instead of only in logs.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| func (w EpochDuo) String() string { | ||
| s := "epochs [" | ||
| sep := "" | ||
| for _, oep := range [2]utils.Option[*Epoch]{utils.Some(w.Current), w.Prev} { |
There was a problem hiding this comment.
[nit] utils.Some(w.Current) on a zero-value EpochDuo yields a present option holding a nil *Epoch, so ep.EpochIndex() nil-derefs. DuoAt and WaitForDuo both return types.EpochDuo{} on error, so any future %v/%s on one of those values panics inside a String() method. Cheap guard: if w.Current == nil { return "epochs []" }.
(Also minor: the loop prints Current before Prev, so the output reads epochs [1, 0] — descending. Prev-first would read more naturally for a Prev|Current window.)
| ) | ||
|
|
||
| // EpochLength is the number of road indices per epoch. | ||
| const EpochLength types.RoadIndex = 108_000 |
There was a problem hiding this comment.
[nit] Worth a one-line rationale for 108_000 (wall-clock target at the expected road rate?). As a const it also can't be shrunk from tests, which is why all the boundary tests hand-plant commitQCs.first/next and duos instead of driving a real transition — see the test-coverage note.
| // ErrBlockOutOfOrder is returned by WriteBlock when the supplied | ||
| // GlobalBlockNumber is not strictly greater than every previously written | ||
| // block number. Blocks must be written in strictly ascending order. | ||
| var ErrBlockOutOfOrder = errors.New("block: WriteBlock out of order") |
There was a problem hiding this comment.
[nit] These three exported errors lost their doc comments entirely (previously they spelled out the WriteBlock/WriteQC ordering contracts — strictly-ascending block numbers, GlobalRange().First == prev.Next, QC-before-block). sei-tendermint/AGENTS.md says "Avoid removing comments and logs which are not obviously obsolete," and these documented invariants callers rely on rather than narrating the implementation, so the new bullet added in this PR doesn't cover them. Suggest restoring a condensed one-liner each.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
sei-tendermint/autobahn/types/proposal.go:505-511— buildProposal (proposal.go) never checks the embedded AppQC's epoch against the proposal's own epoch before including it in a tipcut — it only clears the AppQC if it isn't newer than the previous CommitQC's App or is from the future. FullProposal.Verify (this PR, lines ~505-511) now strictly requires app.EpochIndex() == proposal.EpochIndex(), so right after an epoch boundary (when avail.LastAppQC() can legitimately still be one epoch behind Current), the leader embeds a Prev-epoch AppQC into a Current-epoch proposal and every validator rejects it, causing a deterministic view-timeout stall at every epoch boundary. Fix: buildProposal should also clear appQC/app when appQC.Proposal().EpochIndex() != viewSpec.Epoch().EpochIndex().Extended reasoning...
The bug:
buildProposalinsei-tendermint/autobahn/types/proposal.go(~lines 344-364, not touched by this PR) decides whether to keep an incomingappQCusing only two checks:- Clear if the new
AppProposalis not strictly newer than the previous CommitQC's carriedApp(NextOpt(app) <= NextOpt(old)). - Clear if the new
AppProposalis from the future relative toviewSpec.NextGlobalBlock().
Neither check compares
appQC.Proposal().EpochIndex()againstviewSpec.Epoch().EpochIndex()(i.e. Current). This PR'sFullProposal.Verify(proposal.go ~505-511) now strictly requiresapp.EpochIndex() == proposal.EpochIndex()whenever a new app is carried — tightened as part of this PR's multi-epoch work (the comment even says "Same-epoch as the tipcut proposal (main). Prev lag is avail-only.").buildProposalwas never updated to match that stricter contract.The trigger path:
runPropose(internal/autobahn/consensus/state.go~line 276-282) builds each proposal for the view's Current epoch and passess.avail.LastAppQC()intoNewProposal/buildProposalunconditionally — there is no epoch check on the AppQC itself (only a check that avail's lane epoch matches, viaWaitForLaneQCs's returned epoch).avail.LastAppQC()can legitimately still return an epoch N-1 AppQC immediately after avail's Current window has already slid from N-1 to N: the boundary CommitQC admission is gated bywaitCommitEpochLeashes→waitPruneLeash, which only requireslatestAppQC.EpochIndex() >= N-1(not>= N). So the epoch-N window can open before any epoch-N AppQC exists.Why the two existing checks don't save it: by construction, the AppQC covering the last road(s) of epoch N-1 can only finalize after the boundary CommitQC(which carries a strictly older
App) has committed those blocks. Soavail.LastAppQC()'s epoch-(N-1) proposal is always strictly newer than the boundary CommitQC's carriedApp(check 1 doesn't fire) and itsGlobalNumberis still< NextGlobalBlock()since it's from the past, not the future (check 2 doesn't fire either). The stale AppQC is embedded.Impact: the resulting proposal has
App.EpochIndex() = N-1whileProposal.EpochIndex() = N. Every receiving validator'sFullProposal.Verifyrejects it with"app epoch_index %d != proposal epoch_index %d", so noPrepareQCever forms for that view. SinceNewReproposalonly succeeds when a priorPrepareQCalready exists, a view timeout simply re-runsbuildProposalwith the same (still-stale)AppQC/CommitQC.App, reproducing the exact same failure — a deterministic, non-self-healing view-timeout stall at every epoch boundary until avail's AppQC production independently catches up into epoch N.Step-by-step proof:
- Epoch N-1 is closing: the boundary CommitQC (
LastRoad(N-1)) is admitted onceavail.inner.latestAppQC.EpochIndex() >= N-1and registry epoch N exists (waitCommitEpochLeashes/waitPruneLeash). This does not require an epoch-N AppQC to exist yet. - Avail's Current slides N-1 → N.
avail.LastAppQC()still returns the epoch-(N-1) AppQC (call itY), since no epoch-N AppQC can exist yet (no epoch-N road has committed). - Consensus's leader for the first epoch-N view calls
runPropose→NewProposal(..., vs, ..., s.avail.LastAppQC())withvs.Epoch() = NandappQC = Y(epoch N-1). buildProposal:old = Appcarried by the boundary CommitQC (call itX, epoch ≤ N-1, always older thanYsinceYfinalizes afterX's road committed).NextOpt(Y) > NextOpt(X)→ check 1 does not clear.Y.GlobalNumber() < NextGlobalBlock()(it's in the past) → check 2 does not clear.Y(epoch N-1) stays embedded into the epoch-N proposal.- Every validator's
FullProposal.Verifycomputesapp.EpochIndex() = N-1,proposal.EpochIndex() = N, hits the strict equality check added by this PR, and returns an error. NoPrepareQCforms. - The view times out;
NewReproposalneeds a priorPrepareQC, which doesn't exist, so the new leader (or same leader, next view number) callsbuildProposalagain with the sameY/X— steps 4-5 repeat verbatim. This is a real liveness stall, not a one-round hiccup.
Fix: in
buildProposal, also clearappQC/appwhenappQC.Proposal().EpochIndex() != viewSpec.Epoch().EpochIndex(), mirroring the strict same-epoch contractVerifynow enforces.This is not a duplicate of the "AppQC epoch verify" threads on
avail.PushAppQCorconsensus.pushCommitQCelsewhere in the PR discussion — this is specifically the tipcut-construction path (buildProposal) failing to anticipate the epoch check thatFullProposal.Verifynow enforces on the receiving side. It also isn't a version-gate/cross-version execution false positive per the repo's REVIEW.md: epoch-index transitions happen everyEpochLength(108,000) roads in normal operation regardless of committee identity, so this triggers deterministically at the very first epoch boundary crossed after this PR ships. - Clear if the new
buildProposal clears AppQC when road >= view; the helper was tagging AppQC with View.Index so CommitQCs lost their embedded App and pruning tests failed. 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 12 total unresolved issues (including 11 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 b4249b7. Configure here.
There was a problem hiding this comment.
Well-structured multi-epoch window implementation with strong test coverage (~2000 test lines covering boundary slides, leash waits, stale drops, and restart seeding); I traced the leash interlock and found no deadlock cycle, and the two riskiest refactors (LaneQC slice reuse, prune-no-longer-pushBacks) are both safe. No blockers — findings are one pre-authentication blocking wait in PushAppQC, a silent road-skip path, and several doc/description inconsistencies.
Findings: 0 blocking | 10 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty). Codex reported no material issues but could not run tests (read-only Go module cache in its sandbox), so neither external pass contributed verified findings. - PR description is out of sync with the code: it documents
SetupInitialDuo(nextRoadToExecute, commitQCs), anEnsureExecTipcuthelper, "exec without commit span panics", and "past CommitQC tipcut → warn and ignore". None of these exist in the diff —SetupInitialDuotakes onlycommitQCsand there is noEnsureExecTipcutanywhere in the tree. Worth trimming the description to what actually landed so it doesn't mislead the next reader of this design. - Restart now performs full quorum-signature verification on the prune anchor and every persisted CommitQC (
verifyLoadedCommitQCinavail/newInner), where previously restored CommitQCs were trusted. Good hardening, but it adds startup latency proportional to the retained CommitQC WAL. Worth confirming the WAL is bounded tightly enough that this doesn't measurably slow node restart. epoch.Registrynever evicts entries fromstate.m— epochs accumulate for the process lifetime. Bounded in practice (one entry per 108k roads), so not a leak that matters, but the test-onlyLatestEpoch()is now an O(n) map scan rather than an indexed lookup. Fine as-is; noting in case a future caller reaches for it on a hot path.- Consensus now drops prepare/commit/timeout votes whose
View().EpochIndex != Current, with no redelivery — the comment correctly notes recovery is via view timeout, so each epoch transition can cost one timeout round. Negligible at 108k roads per epoch, but the existingTODO: scope prepareVotes, commitVotes, and timeoutVotes to a single epochis the real fix and should stay tracked. TODO(lane-id)inavail/newInnerandavail/NewState: on restart only Current-committee lanes are pre-created and prune-anchored; Prev-committee lanes appear later via WALgetOrInsertLaneand miss the anchor watermark. Harmless today (all epochs share the genesis committee, so lane sets are identical), but it becomes load-bearing the moment real per-epoch committees land — worth making sure it lands in the same series as the committee wiring rather than drifting.- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| return s.runPersist(ctx, s.persisters) | ||
| }) | ||
| // Task inserting FullCommitQCs and local blocks to data state. | ||
| // ErrPruned jumps n forward (AppQC/window prune during catch-up): skipped |
There was a problem hiding this comment.
[suggestion] This comment documents that ErrPruned advances n past the road, and "peers can PushQC into data" — but the skip is completely silent, and the consequence on this node is not local. data.PushQC gates on gr.First <= inner.nextQC, so a skipped road leaves a permanent hole in data's contiguous QC prefix that only an external peer push can fill; there's no local retry.
headers() gained a new way to produce ErrPruned in this PR (state.go:737, unknown lane → ErrPruned where the old code would have nil-derefed inner.votes[lane]). That triggers when a CommitQC references a lane absent from inner.lanes — precisely the restart-at-a-boundary case the TODO(lane-id) in newInner describes, where Prev-committee lanes aren't pre-created. Unreachable today since every epoch shares the genesis committee, so lane sets are identical; it becomes live once real committees land.
Suggest a logger.Warn on the skip (road index + duo) rather than a bare continue, so if this ever fires in production it's diagnosable instead of presenting as an unexplained stall in data's QC prefix.
| firstRoad := FirstRoad(epochIdx) | ||
| epoch := types.NewEpoch(epochIdx, types.RoadRange{First: firstRoad, Next: FirstRoad(epochIdx + 1)}, genesis.FirstTimestamp(), genesis.Committee(), genesis.FirstBlock()) | ||
| s.m[epochIdx] = epoch | ||
| r.epochGen.Store(r.epochGen.Load() + 1) |
There was a problem hiding this comment.
[nit] Non-atomic read-modify-write on epochGen. Correct today only because both makeEpoch call sites (SetupInitialDuo and EnsureEpoch) hold r.state.Lock(), which the doc comment above does state. Since WaitForDuo only needs the value to change, not to be exact, an atomic.Uint64.Add (or an AtomicSend increment helper) would make the correctness self-evident and survive a future caller that forgets the lock precondition.
|
|
||
| "github.com/sei-protocol/sei-chain/sei-tendermint/autobahn/types" | ||
| "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils" | ||
| "github.com/stretchr/testify/require" |
There was a problem hiding this comment.
🟡 sei-tendermint/AGENTS.md mandates using the strongly-typed libs/utils/require package instead of testify/require in tests. This PR's new/extended test files — registry_test.go (which had no testify import before this PR), block_votes_test.go, and epoch_test.go — import and use github.com/stretchr/testify/require instead. The same PR'''s app_qc_test.go correctly uses libs/utils/require, showing the convention was known but applied inconsistently.
Extended reasoning...
sei-tendermint/AGENTS.md (line 8) explicitly states: "use strongly typed utils/require asserts instead of testify/require." This is a documented, non-subjective convention, not a style preference.
Three files touched by this PR violate it:
sei-tendermint/internal/autobahn/epoch/registry_test.go:6-9" — this PR adds thegithub.com/stretchr/testify/requireimport while extending the file. Before this PR (at the pre-change commit) the file used plaint.Fatal/t.Fatalf` with no testify import at all, so this is a net-new violation introduced here, not a pre-existing one being left alone.sei-tendermint/internal/autobahn/avail/block_votes_test.go— new file, importsgithub.com/stretchr/testify/requireand usesrequire.False/True/Equal/Len/Contains/NotContains.sei-tendermint/autobahn/types/epoch_test.go" — new file, importsgithub.com/stretchr/testify/requireand usesrequire.False/True`.
What makes this notable rather than just an isolated oversight is the inconsistency within the same PR: sei-tendermint/autobahn/types/app_qc_test.go, also added by this PR, correctly imports libs/utils/require, as do sibling files in the same package (proposal_test.go, committee_test.go). That shows the author was aware of and applied the convention elsewhere, just not uniformly.
The code path is purely test-only: these imports don't affect runtime behavior, compiled binaries, or production correctness in any way — this is a lint/convention-cleanliness issue only. golangci-lint as configured for this repo does not appear to enforce this specific substitution, so it wouldn't be caught by CI either.
The reason this stays a nit rather than blocking: I checked and testify/require is in fact the dominant pattern across the existing sei-tendermint test suite (roughly 140 files use stretchr/testify/require versus about 69 using libs/utils/require), and several sibling files in the very same avail package (conv_test.go, state_test.go, inner_test.go) already use testify. So while this PR's new files not matching the AGENTS.md directive is a real, verifiable inconsistency, it's also consistent with how most of the codebase already looks — the convention is aspirational and widely unenforced today.
Proof / how to fix: For registry_test.go, epoch_test.go, and block_votes_test.go, swap the import from github.com/stretchr/testify/require to github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils/require. I verified that libs/utils/require re-exports NoError, Error, True, False, Equal, Len, Contains as thin wrappers around testify, so the swap is a drop-in for registry_test.go and epoch_test.go. block_votes_test.go additionally calls require.NotContains (and possibly NotErrorIs), which are not currently re-exported by libs/utils/require and would need to be added there first, or the assertions rewritten, before that file could switch over cleanly.
| ) | ||
|
|
||
| func testDuoEpochs(t *testing.T) (prev, current *types.Epoch) { | ||
| t.Helper() | ||
| rng := utils.TestRng() | ||
| weights := map[types.PublicKey]uint64{} | ||
| for range 3 { | ||
| weights[types.GenSecretKey(rng).Public()] = 1 | ||
| } | ||
| committee := utils.OrPanic1(types.NewCommittee(weights)) | ||
| prev = types.NewEpoch(0, types.RoadRange{First: 0, Next: 100}, utils.GenTimestamp(rng), committee, 1) | ||
| current = types.NewEpoch(1, types.RoadRange{First: 100, Next: 200}, utils.GenTimestamp(rng), committee, 101) | ||
| return prev, current | ||
| } | ||
|
|
||
| func TestNewEpochDuo_PanicsOnNonContiguousIndex(t *testing.T) { | ||
| prev, _ := testDuoEpochs(t) | ||
| rng := utils.TestRng() | ||
| weights := map[types.PublicKey]uint64{types.GenSecretKey(rng).Public(): 1} | ||
| committee := utils.OrPanic1(types.NewCommittee(weights)) | ||
| // Roads abut, but index jumps 0 → 2. | ||
| current := types.NewEpoch(2, types.RoadRange{First: 100, Next: 200}, utils.GenTimestamp(rng), committee, 101) | ||
| defer func() { | ||
| if recover() == nil { | ||
| t.Fatal("NewEpochDuo with non-contiguous indices should panic") |
There was a problem hiding this comment.
🟡 In epoch_duo_test.go, testDuoEpochs (a shared helper) constructs its own utils.TestRng(), and TestNewEpochDuo_PanicsOnNonContiguousIndex constructs a second, independent utils.TestRng() to draw another key. Since TestRng() always replays the same fixed seed, the second draw is byte-for-byte identical to the first key drawn inside testDuoEpochs, silently reusing prev's first validator key for the test's throwaway committee — violating the sei-tendermint/AGENTS.md convention that a TestRng instance should be one per test. No functional impact today since the test only exercises panic behavior, but worth fixing by threading a single rng through (or passing one into) the helper.
Extended reasoning...
sei-tendermint/AGENTS.md states: "TestRng instance should be one per test, constructed directly in the test function... Use TestRng.Split() ... to ensure deterministic entropy across ... goroutines." utils.TestRng() always seeds from the same fixed constant (789345342, per libs/utils/testonly.go), so any two independently-constructed TestRng() instances in the same process replay an identical draw sequence.
In the new sei-tendermint/autobahn/types/epoch_duo_test.go, testDuoEpochs(t) is a shared helper (not itself a test function) that constructs rng := utils.TestRng() and draws 3 SecretKeys from it to build a 3-member committee used for both prev and current epochs.
TestNewEpochDuo_PanicsOnNonContiguousIndex calls testDuoEpochs(t) to get prev, then in its own body constructs a second, independent rng := utils.TestRng() and draws types.GenSecretKey(rng) to build a one-member committee for a throwaway current epoch (used only to trigger the non-contiguous-index panic). Because both rng instances start from the same fixed seed, this second draw is byte-for-byte identical to the very first key drawn inside testDuoEpochs (i.e., a member of prev's committee). So the test ends up minting a 'new' committee that silently reuses prev's first validator key, rather than an independent one.
Existing code doesn't prevent this because NewEpochDuo only validates epoch-index contiguity and road-range abutment — it never inspects committee membership — so the collision is invisible to the current assertions and the test still correctly verifies the panic. There's no bug in application logic; this is purely a test-hygiene violation of the documented one-rng-per-test convention, and it defeats the intended entropy isolation for any future test that adds an assertion relying on prev/current having independent committees.
Step-by-step proof:
testDuoEpochscreatesrngA := utils.TestRng()(seed 789345342) and callstypes.GenSecretKey(rngA)three times to buildweightsforprev/current's shared committee. The first of these calls produces key K1.TestNewEpochDuo_PanicsOnNonContiguousIndexthen createsrngB := utils.TestRng()— also seeded with 789345342 — and callstypes.GenSecretKey(rngB)exactly once to build its own committee.- Since
rngAandrngBare independent instances of the same deterministic generator with the same seed,rngB's first draw is identical torngA's first draw: the key produced in step 2 equals K1 from step 1. - The result: the test's local one-member committee (nominally meant to be a fresh, unrelated committee for the panic scenario) is actually keyed by
prev's first validator, an accidental correlation that violates the 'one rng per test' invariant even though it doesn't currently break any assertion.
Fix: thread the rng out of testDuoEpochs (e.g., have it accept an rng parameter, or return the one it used) so the whole test uses a single shared entropy source, per the documented convention (or use rng.Split() if genuinely independent-but-deterministic streams are needed).
There was a problem hiding this comment.
Beyond the reported nit, I also checked one additional concern this run's verifiers ruled out: whether data.State.PushBlock's narrow Prev|Current epochDuo window (vs. the full Registry) could break full-node catch-up on a road whose epoch fell outside that window. It doesn't — PushQC advances epochDuo in the same lock scope where it makes the QC visible via inner.nextQC, so any road with an already-inserted QC is guaranteed to be covered by the window by the time PushBlock looks it up.
Extended reasoning...
PushBlock resolves the block's epoch via s.epochDuo.Load().EpochForRoad(...) using the road index of the QC already stored at inner.qcs[n]. PushQC only makes a QC visible at inner.qcs[n] (advancing inner.nextQC) inside the same s.inner.Lock() critical section where it conditionally advances epochDuo (on IsLastRoad, after a WaitForDuo for the next epoch). So by construction, any road for which n < inner.nextQC holds already has its epoch covered by epochDuo — there is no window in which a QC is visible to PushBlock's gate but the epoch lookup can miss. This is distinct from the restart/load path (loadFromBlockDB), which uses Registry.EpochAt directly and is unaffected.
…N-358) Peer PushCommitQC can leave Current>0 without an AppQC anchor if seal 0 is exempt; also fail closed on missing data tip QC and document avail-behind-cons recovery. 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 12 total unresolved issues (including 11 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 1f56a3d. Configure here.
There was a problem hiding this comment.
Well-structured, heavily-tested epoch-window refactor with genuinely careful invariant documentation, but bounding genesis epoch 0 to roads [0, 108_000) makes any existing autobahn store past road 108 000 unloadable (persisted CommitQCs carry a signed EpochIndex 0 that no longer matches EpochAt(road)), and the PR body describes an earlier design that no longer matches the code.
Findings: 2 blocking | 15 non-blocking | 7 posted inline
Blockers
- Local-state compatibility:
epoch.NewRegistrynow gives epoch 0 the range[0, FirstRoad(1))=[0, 108_000)instead ofOpenRoadRange(). Pre-PR, every proposal was stampedView.EpochIndex = 0regardless of road (the registry only ever held epoch 0). On restart against an existing store whose retained CommitQC roads are >= 108 000,data.loadFromBlockDBresolvesEpochAt(road)-> epochroad/108_000, andinsertQC->CommitQC.Verify->View.Verifyrejectsepoch_index = 0, want 1;data.NewStatereturns an error and the node cannot start (avail's newverifyLoadedCommitQCfails identically). Roads advance per consensus instance, so any devnet that has been up for more than a few hours is likely past that point. Please confirm no live network is affected, or add a migration/compat path, and call the state-breaking nature out in the PR body (thenon-app-hash-breakinglabel covers app hash, not local WAL/BlockDB compatibility). - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only Codex's single finding with my own. - On Codex's only finding (the added
sei-tendermint/AGENTS.mdline): I disagree that this is a prompt-injection vector.AGENTS.mdis the repository's own canonical contributor/agent guide, and the added text is a benign comment-style convention, not an attempt to influence review outcome. The legitimate objection is scope and consistency — see the inline comment on that line. - PR description is stale relative to the merged code. It documents
SetupInitialDuo(nextRoadToExecute, commitQCs),EnsureExecTipcut,epoch.CommitQCSpan, "exec without commit span panics", "Exec tip fromapp.LastBlockHeight", andp2p.checkRestartTips. None of these exist in the final diff: the signature isSetupInitialDuo(utils.Option[types.RoadRange]) error, there is no execution-tip input, and there is nocheckRestartTips. For a 3400-line consensus change the description is the primary reviewer aid — please refresh it. - Restart seeding bypasses the execution leash.
SetupInitialDuounconditionally seedswindowLast+1andwindowLast+2placeholders, so after any restart theWaitForDuo(FirstRoad(N+1))leash inwaitCommitEpochLeashes("execution finished N-1") is already satisfied regardless of actual execution progress. TheRegistrydoc comment states "Execution cannot pass commit" as an invariant; across a restart it currently is not enforced. Harmless while committees are genesis stubs, but worth stating explicitly next to that invariant so it is not assumed to hold when real committees land. avail.PushCommitQC(state.go:453) omits theinner.epochDuo.Load().Current.RoadRange().Has(idx)guard thatPushAppQCapplies beforeadvanceEpoch(state.go:593). I believe it is unreachable (Current cannot pass N without insertingLastRoad(N), which theidx != inner.commitQCs.nextearly-return already excludes), but the asymmetry reads as an oversight — either add the guard or note why it is unnecessary.- Boundary liveness corner without a test: if a node's avail tip sits exactly at
LastRoad(N)with no AppQC yet in epoch N, thePushAppVotepath cannot break the leash — it blocks inwaitForCommitQC(idx)on the very CommitQC thatwaitCommitEpochLeashesis holding. Only a fullPushAppQCfrom a peer (which passesincoming) escapes.TestPushAppQCBoundaryIncomingAppQCcovers the escape hatch; consider a comment or negative test documenting that the AppVote-only path cannot self-unblock. avail.PushBlockverifies againstduo.Currentand deliberately skips the post-WaitUntilre-check thatPushVoteperforms (state.go:642). The divergence is documented, but the two paths are otherwise identical enough that a shared helper (or a single documentedverifyUnderCurrent) would keep them from drifting.- Cosmetic churn:
giga_router_common.goreformatsdata.NewState(&data.Config{Registry: registry}, blockDB)across three lines with no content change (leftover from the removedLastExecutedBlockfield). Worth reverting. - I could not run
go build,go test,gofmt -s -l, orgoimports -lin this environment (commands were not permitted), so this review is static only. PerAGENTS.md, please confirm both formatters were run over every touched.gofile. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| ) | ||
|
|
||
| // EpochLength is the number of road indices per epoch. | ||
| const EpochLength types.RoadIndex = 108_000 |
There was a problem hiding this comment.
[suggestion] EpochLength is hardcoded with no indication of the intended wall-clock epoch duration and no per-network override (genesis/config). Since it now defines the epoch partition for all signed View.EpochIndex values, changing it later is a state/consensus break. Worth a comment stating the target duration and the assumed road rate, and a TODO for making it genesis-configurable alongside the TODO(autobahn) about real committees.
| // (unchanged by exec). Covers exec tip ahead of persisted CommitQC (N+1) | ||
| // and tip at LastRoad(N) without re-exec (N+2). Goes away next PR when | ||
| // committees are linked to execution. | ||
| r.EnsureEpoch(windowLast + 1) |
There was a problem hiding this comment.
[suggestion] These two unconditional placeholder registrations are what make the execution leash a no-op immediately after restart: waitCommitEpochLeashes only checks WaitForDuo(FirstRoad(epochIdx+1)), and windowLast+1/+2 are derived from the retained CommitQC span rather than from execution progress. The Registry doc comment above states "Execution cannot pass commit" as an invariant — please note here (or there) that the invariant is only enforced on the live AdvanceIfNeeded path, not across restart, so this cannot be silently relied on once committees are derived from execution.
| func (s *laneVoteSet) reset() { | ||
| s.weight = 0 | ||
| s.votes = s.votes[:0] | ||
| s.qc = utils.None[*types.LaneQC]() |
There was a problem hiding this comment.
[suggestion] reset() clears qc before reweight recounts, so a LaneQC that was already returned (via laneQC() -> WaitForLaneQCs -> a proposal) can be silently revoked when Current advances and one of its signers is no longer in the committee. Aliasing is fine (NewLaneQC copies the sigs slice and takes votes[0].hashed by value), so this is not a memory-safety issue — but it does mean the local view can retroactively lose the justification for a QC already in flight.
No-op today because placeholder committees are identical across epochs. Suggest at least logging (or asserting in tests) when a set that had qc present loses it after reweight, so the case surfaces when real rotation lands rather than as a mysterious lane stall.
| // none. | ||
| // Returns an error if nextQC claims a retained QC that is missing from the map | ||
| // (invariant break) — not the same as an empty tip. | ||
| func (s *State) CommitTipCut() (types.RoadIndex, error) { |
There was a problem hiding this comment.
[suggestion] data.State.CommitTipCut is exported but referenced only from consensus/state_test.go and data/state_test.go — no production caller. The PR description says it is the "[r]estart anchor for p2p.checkRestartTips (consensus tip vs data)", but checkRestartTips isn't in this diff. Either wire the data-vs-consensus restart tip check as described, or drop the method (and the accompanying test) until the follow-up needs it.
| optional uint64 index = 1; // required | ||
| optional AppProposal app = 4; // optional | ||
| * 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.
[suggestion] Two concerns, neither of them the prompt-injection reading Codex gave this (this is the repo's own canonical agent guide, and the added text is an ordinary style convention):
- Scope. A repo-wide contributor-guidance change doesn't belong in a 3400-line consensus feature PR; it should land separately so it gets reviewed on its own merits.
- Consistency. It sits directly under "Avoid removing comments and logs which are not obviously obsolete. Keep the original wording, only fixing mistakes or obsolete parts," and this PR then applies the new rule to condense comments unrelated to the epoch window (
types/errors.go,consensus/persist/persist.go,types/epoch.go'sNewEpochdoc,avail/state.go'sErrBadLane). Please either reconcile the two rules explicitly or revert the unrelated doc deletions.
| // reclamation is asynchronous, so an n that returned ErrPruned before a restart | ||
| // may afterward read as present (or as utils.None). Callers should treat | ||
| // ErrPruned as "not currently served," not as a guarantee the record is gone. | ||
| // ErrPruned means the record is below the retention watermark and is not served. |
There was a problem hiding this comment.
[suggestion] This condensation drops information a caller needs and is unrelated to the epoch-window change. The removed text explained that ErrPruned reflects the watermark's current position rather than a permanent verdict, that it is terminal within a session (retrying the same n keeps returning it) but not across restarts because the watermark is re-derived on open — and it explicitly told callers to treat it as "not currently served," not "gone." The replacement keeps the first half of that and loses the actionable guidance.
Same applies just above: ErrBlockOutOfOrder (line 12), ErrQCNonContiguous (line 13) and ErrBlockMissingQC lose their BlockDB ordering-contract docs entirely. Please restore these.
Match TestCommitQC: buildProposal clears AppQC when road >= view. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A large but internally coherent refactor of Autobahn's epoch handling from a single LatestEpoch to a sliding Prev|Current EpochDuo per layer, with well-documented invariants (registry seeding ownership, prune/execution leashes, tipcut admission) and extensive new unit coverage for the window, leash, and restart paths. I found no blocking correctness or security issues — only three small hardening/accuracy nits plus some cross-cutting notes.
Findings: 0 blocking | 9 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found in the PR diff"; the Cursor file (
cursor-review.md) is empty, i.e. that pass produced no output — nothing from it was merged in. - Verification caveat:
go vet/go test/gofmtcould not be run in this review sandbox (commands were blocked), so this is a static review. The correctness argument for the leash chain (seal epoch N ⇒ AppQC in N ⇒ execution past LastRoad(N-1) ⇒AdvanceIfNeededseeded N+1) checks out on paper; rely on the Race Detection + Autobahn Upgrade Module CI jobs for the rest. - Consensus now silently drops prepare/commit/timeout votes whose
View().EpochIndex!= Current, with recovery only via view timeout (documented in the code). That is a real, if bounded, liveness cost at every epoch boundary. Consider a counter/metric for dropped-by-epoch votes so boundary stalls are observable on testnets rather than only visible as an unexplained timeout round. - Two structures now grow monotonically and are only covered by TODOs:
avail.inner.lanes(never deleted —TODO(lane-expiry)) andepoch.registryState.m(epochs are never pruned). Both are harmless while committees are genesis stubs and epochs are 108k roads apart, but they should be closed out before dynamic committees land. - Local-state break is called out in the PR body (bounding epoch 0 to
[0, EpochLength)invalidates any Autobahn store that stampedepoch_index=0past road 108 000). Worth confirming the "wipe PersistentStateDir / BlockDB" instruction actually reaches anyone running dev/test Autobahn nodes, since thenon-app-hash-breakinglabel understates it. epoch.makeEpochstamps every placeholder epoch with the genesisFirstTimestamp/FirstBlock. Today that is inert (both are only read viaViewSpecwhenCommitQCis None, i.e. genesis only), but it meansEpoch.FirstBlock()/FirstTimestamp()are silently wrong for any non-zero placeholder epoch. The existingTODO(autobahn)covers it; a one-line warning on those accessors would prevent a future caller from trusting them.- 3 suggestion(s)/nit(s) flagged inline on specific lines.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
This PR is a large, consensus-critical rewrite of Autobahn's epoch-transition logic (multi-epoch Prev|Current window across avail/data/consensus, restart re-seeding, and AppQC lag handling) spanning 47 files. No new bugs were found in this pass, but given the scope and criticality of the change, a human review is warranted before merge.
Extended reasoning...
Overview
This PR reworks how Autobahn tracks epochs across the avail, data, and consensus layers, replacing a single-epoch model with a sliding Prev|Current EpochDuo window. It touches CommitQC/AppQC admission and verification, epoch-boundary transitions, restart re-seeding (SetupInitialDuo), lane-vote reweighting across epoch changes, and the consensus tipcut/proposal verification paths. This is core BFT consensus logic that determines liveness and safety of block production.
Security risks
No injection/auth-bypass-style risks (this is internal consensus state, not user-facing input handling), but the risk profile here is consensus safety/liveness: incorrect epoch-boundary handling can cause validators to reject valid QCs, diverge on view state, or halt. The PR history shows many now-resolved defects of exactly this kind (e.g. pushCommitQC verifying a CommitQC against a stale cached epoch instead of the QC's own road-derived epoch, which was flagged repeatedly across several review rounds and appears to be fixed in the current code via s.registry.EpochAt(qc.Proposal().Index())).
Level of scrutiny
This warrants the highest level of scrutiny the review process supports. It is not a mechanical or low-ambiguity change — it involves multiple interacting state machines (avail/data/consensus epoch windows), restart recovery invariants, and concurrency between async persistence and live admission paths. The repo-specific guidance about not-yet-created upgrade tags/handlers does not materially apply here since there's no new release-tag gating in this diff.
Other factors
The PR has already been through roughly two weeks of intensive review by multiple human reviewers (wen-coding, pompon0, seidroid) and automated tools (Cursor Bugbot, this bug-hunting system), with numerous rounds of fixes for boundary-condition bugs. That history is valuable context but does not substitute for a final human sign-off given the size (47 files) and the fact that several of the fixed bugs were subtle epoch-boundary races that took multiple iterations to fully resolve — a class of bug that is easy to reintroduce with any further edits.
Fall back to prior CommitQC App instead of None; require Prev for Current-1; clarify PushAppVote EpochAt only bounds registry lookahead. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
The Prev|Current EpochDuo window, half-open RoadRange, registry seeding/leash design, and per-layer window advance all hold up under review — the boundary-slide ordering (advanceEpoch before pushBack, under one lock), prune/execution leashes, and restart seeding invariants are consistent and well tested. No blocking correctness or security issues found; the notes below are one medium-severity hardening suggestion in PushAppQC plus documentation/scope/style nits.
Findings: 0 blocking | 10 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: the Codex review file reports "No material issues found in the reviewed diff." The Cursor review file (
cursor-review.md) is empty — that pass produced no output, so this review is effectively Claude + Codex only. headers()now returnstypes.ErrPrunedwhen a lane is absent frominner.lanes(avail/state.go ~748), andRun'ss.data.PushQCloop treatsErrPrunedas "skip this road". Combined with theTODO(lane-id)that Prev-committee lanes are not seeded on restart, a Prev-epoch road could be silently skipped for local export instead of surfacing. Harmless while all committees are genesis stubs, but worth a warn log or metric so it isn't invisible once real committees land.types.BuildCommitQChardcodesNewEpochDuo(epoch, utils.None[*Epoch]()), so the shared test helper cannot build a CommitQC carrying a Prev-epoch (lagging) AppQC — cross-boundary tipcut construction has to be hand-rolled in each test. Consider having it take anEpochDuoso the new Prev-lag path is exercisable from the common helper.- New tests in
autobahn/types/epoch_duo_test.goandinternal/autobahn/epoch/registry_test.gouse rawt.Fatalf/if err != nilrather thanrequire, unlike the surrounding autobahn test files. Minor consistency nit. ErrAvailBehindConsensusand the epoch-0 WAL/BlockDB break both require operator action (wipe PersistentStateDir / BlockDB). This is documented in the PR body and in code comments, but not anywhere operator-facing. Autobahn isn't on a live network so this isn't blocking, but a line in a changelog or runbook would be durable.giga_router_common.go:104:data.NewState(&data.Config{Registry: registry}, blockDB)was reflowed onto three lines with no semantic change — pure diff churn in an already-large PR.- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| // reclamation is asynchronous, so an n that returned ErrPruned before a restart | ||
| // may afterward read as present (or as utils.None). Callers should treat | ||
| // ErrPruned as "not currently served," not as a guarantee the record is gone. | ||
| // ErrPruned means the record is below the retention watermark and is not served. |
There was a problem hiding this comment.
[nit] This file (and consensus/persist/persist.go, plus the doc comments on RoadRange.Has / NewEpoch in epoch.go) loses documentation unrelated to the epoch-window work: ErrBlockOutOfOrder, ErrQCNonContiguous, and ErrBlockMissingQC are now undocumented exported vars, and the ErrPruned / ErrCorrupt semantics were compressed. sei-tendermint/AGENTS.md asks to "avoid removing comments and logs which are not obviously obsolete" — these describe BlockDB write-ordering contracts that this PR doesn't change. Suggest reverting the doc trimming in this file and in persist.go to keep the diff scoped.
| optional uint64 index = 1; // required | ||
| optional AppProposal app = 4; // optional | ||
| * 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] This PR adds a project-wide comment-style rule in the same change that performs the comment compression in types/errors.go and consensus/persist/persist.go. Guidance edits that will steer every future agent/contributor are easier to review on their own merits in a separate PR — especially since this sits directly below the existing (somewhat opposite-leaning) "avoid removing comments and logs which are not obviously obsolete" rule, and the interaction between the two isn't spelled out.
There was a problem hiding this comment.
No new bugs found in this pass, but this PR rewrites core Autobahn epoch-transition logic across avail/data/consensus (multi-epoch CommitQC/AppQC windowing) and warrants continued human attention given its history of extensive back-and-forth (including several rounds where a real staleness bug in consensus.pushCommitQC's epoch verification took many iterations to actually land a fix for).
Beyond the inline findings, this run also examined and ruled out: data.PushBlock hard-failing when the covering QC's epoch has fallen out of the live Prev|Current window (data/state.go) — confirmed this path is guarded by the same execution/prune leash invariants that keep the window from sliding past a QC still needed for block insertion.
Extended reasoning...
One-line note: this is a follow-up-only comment (no new findings), recording what else was checked this run per the review process; not a full structured review.
…358) Keep PushCommitQC/PushAppQC on admit-before-verify (not EpochAt); quiet boundary chat from stale-drop Info logs. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Large, carefully-structured refactor to a Prev|Current EpochDuo window with prune/execution leashes; I traced the boundary, restart, and locking paths and found no blocking correctness or security defect. Remaining notes are robustness/test-coverage suggestions plus a bundled AGENTS.md doc change that also drives unrelated comment deletions.
Findings: 0 blocking | 14 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass (
cursor-review.md) is empty — that review produced no output, so this synthesis rests on my own pass plus Codex's single finding. - I could not run
go build,go test, orgofmt/goimportsin this environment (commands not approved), so this review is static-only. Please make sure the cited CI jobs (Race Detection + Autobahn Upgrade Module) plusgofmt -s -l ./goimports -l .are green — AGENTS.md requires both formatters on every touched.gofile, and this PR touches 48. epoch.EpochLengthis a hard-codedconst(108_000), so no test can cross a real epoch boundary through a multi-node run. The boundary tests instead hand-plantinner.commitQCs.first/nextand callregisterDuoAtEpochto force the duo. The livePushCommitQC/PushAppQCseal path is covered that way (TestPushCommitQCBoundaryWaitsForAppQCInEpoch, TestPushAppQCWaitsForEpochUnlock), which is good — but nothing exercises the integrated avail→consensus→data slide end to end. Making EpochLength a var/config field would letRunTestNetworkactually cross a boundary and would cover the interactions this PR's comments reason about in prose.avail/inner.go newInnernow callsverifyLoadedCommitQCon the anchor and every restored CommitQC, so startup does quorum-signature verification proportional to the retained WAL. This is good hardening (previously loaded QCs were pushed back unverified), but it is synchronous startup latency — worth a quick measurement on a full retention window.blockVotes.reweightrebuilds eachlaneVoteSetby iterating thebyKeyGo map, so which votes survive intoset.votes— and therefore the signature set of the resulting LaneQC — is nondeterministic across runs. Harmless for validity (any quorum is valid), but two runs of the same node produce different LaneQCs, which can flake any test or metric that assumes determinism.epoch/testonly.gois a normal (non-_test.go) file, soLatestEpoch,EpochAtTip,GenRegistry,GenRegistryAt, andGenRegistryTipare compiled into the production binary; the "tests only" contract is comment-enforced.LatestEpochis newly moved here and does an O(n) map scan. Consider anexport_test.goor a separateepochtestpackage.- Redundant wait:
waitCommitEpochLeashesalready doesWaitForDuo(FirstRoad(epochIdx+1)), and bothPushCommitQCandPushAppQCthen immediately callWaitForDuo(idx+1)for the identical road (sinceclosingimpliesidx == LastRoad(epochIdx)). Having the leash return the duo it already resolved would remove the second call. - The epoch-0 rebound to
[0, EpochLength)is a local WAL/BlockDB break, but it is documented only in the PR description. An operator upgrading a devnet will hit a crypticroad_index N not in epoch roads [0, 108000)verify failure rather than a clear "wipe PersistentStateDir/BlockDB" message. Consider a targeted startup error (or a note in the module's AGENTS.md) that names the required action. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| optional uint64 index = 1; // required | ||
| optional AppProposal app = 4; // optional | ||
| * 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.
[suggestion] Codex flagged this line as a prompt-injection attempt. I disagree with that framing — this is an ordinary in-repo comment-style guideline, not an instruction directed at reviewers or at changing a verdict, and AGENTS.md is this repo's canonical contributor guide, so editing it is legitimate.
The narrower concern is real though: a repo-wide documentation/style change is bundled into a 3.4k-line functional PR, and it's in tension with the bullet immediately above it ("Avoid removing comments and logs which are not obviously obsolete. Keep the original wording, only fixing mistakes or obsolete parts"). This PR then acts on the new guideline by deleting non-obsolete doc comments in files it otherwise doesn't touch functionally. Please split the guideline into its own PR so it can be agreed on separately, and reconcile it with the existing bullet (e.g. state explicitly that it applies to new comments, not to pruning existing ones).
| // after its AppQC floor has made it obsolete). The registry is | ||
| // independent of Availability pruning (restart + admission / | ||
| // execution leash), not the live window. | ||
| ep, err := s.epochDuo.Load().EpochForRoad(anchor.Proposal().Index()) |
There was a problem hiding this comment.
[suggestion] EpochForRoad failing here is fatal: the error propagates out of runPersist, which kills the avail Run scope and takes the node down.
The anchor comes from batch.pruneAnchor, captured under lock in collectPersistBatch, but s.epochDuo.Load() reads the live duo an unbounded time later — the anchor Persist fsync happens in between with no lock held. If Current advances two epochs in that gap, the anchor road falls before the window and this returns ErrRoadBeforeWindow.
I believe this is not reachable in practice: advancing twice requires two boundary seals, and each seal's prune leash requires latestAppQC to be in the newer epoch — which is the very value the batch anchor was taken from — so you'd need ~108k roads of progress inside one persist cycle. But the consequence is a node crash rather than a skipped optimisation, and the union here is only used to widen batchLanes for WAL pruning. Suggest tolerating types.ErrRoadBeforeWindow (skip the lane union, log at debug) and reserving the hard error for ErrRoadAfterWindow.
| upperBound := min(bq.next, inner.nextBlockToPersist[lane]) | ||
| for i := max(bq.first, r.next[lane]); i < upperBound; i++ { | ||
| batch = append(batch, bq.q[i].Msg().Block().Header()) | ||
| for lane, ls := range inner.lanes { |
There was a problem hiding this comment.
[suggestion] This now iterates inner.lanes, which advanceEpoch never prunes (TODO(lane-expiry)), so it includes lanes retired from the committee. Previously inner.blocks was fixed to the committee's lanes at construction.
For a retired lane with retained blocks, r.next[lane] is the map zero value, so this will emit its headers and the node will sign LaneVotes for a lane outside the current committee — peers' PushVote rejects them via vote.Msg().Verify(c), so it's wasted signing and gossip rather than a safety issue, and it can't trigger today because all placeholder epochs share the genesis committee. Worth gating on inner.epochDuo.Load().Current.Committee().Lanes() until lane-expiry lands, so this doesn't become live work the moment committees actually change.
| // (unchanged by exec). Covers exec tip ahead of persisted CommitQC (N+1) | ||
| // and tip at LastRoad(N) without re-exec (N+2). Goes away next PR when | ||
| // committees are linked to execution. | ||
| r.EnsureEpoch(windowLast + 1) |
There was a problem hiding this comment.
[suggestion] These two unconditional EnsureEpoch calls seed windowLast+1 and windowLast+2 on every restart, which bypasses the execution leash documented on Registry above ("Sealing epoch N ... requires registry N+1 (execution leash)"). After any restart, the leash is effectively pre-satisfied for the next two epochs regardless of how far execution has actually progressed.
The comment acknowledges this is temporary while committees are genesis stubs, and it is what makes the restart path work at all — so not a blocker. But since the invariant block above states the leash unconditionally, please add a forward reference there (or here) noting the restart carve-out, so the next reader doesn't take the documented invariant as load-bearing for anything security-relevant.
| // reclamation is asynchronous, so an n that returned ErrPruned before a restart | ||
| // may afterward read as present (or as utils.None). Callers should treat | ||
| // ErrPruned as "not currently served," not as a guarantee the record is gone. | ||
| // ErrPruned means the record is below the retention watermark and is not served. |
There was a problem hiding this comment.
[nit] This file's doc-comment rewrites (and the ones in consensus/persist/persist.go, plus the fully-removed doc comments on ErrBlockOutOfOrder/ErrQCNonContiguous/ErrBlockMissingQC just above, types.NewEpoch, and the dynamic-committee TODO in avail/inner.go) are unrelated to the epoch-window feature and drop information that isn't obsolete — e.g. the old ErrPruned comment explained the utils.None vs ErrPruned distinction and that callers must treat it as "not currently served" rather than a permanent verdict.
AGENTS.md already says "Avoid removing comments and logs which are not obviously obsolete." Suggest reverting the doc-only deletions in this PR; if the new brevity guideline is adopted, apply it in a separate pass where the tradeoff per comment can actually be reviewed.
| func (w EpochDuo) String() string { | ||
| s := "epochs [" | ||
| sep := "" | ||
| for _, oep := range [2]utils.Option[*Epoch]{utils.Some(w.Current), w.Prev} { |
There was a problem hiding this comment.
[nit] The array is {Some(Current), Prev}, so String() renders descending — "epochs [1, 0]" for Prev=0|Current=1. Since this feeds the admitRoadOrDrop drop logs and the EpochForRoad before/after-window errors, which are all reasoning about a [Prev, Current] window, ascending order would read more naturally. Swapping to {w.Prev, Some(w.Current)} gives "epochs [0, 1]" and needs no other change.
There was a problem hiding this comment.
No bugs found this run, but given the size and criticality of this change (a redesign of Autobahn's epoch-transition/admission model across avail, data, consensus, and p2p, with ~47 files touched), this warrants a human pass before merge rather than a shadow approval.
What was reviewed: the CommitQC/AppQC admission and window-sliding logic in avail/state.go and data/state.go, the EpochDuo seeding/registry invariants (epoch/registry.go), and consensus's pushCommitQC epoch resolution. Worth noting: the epoch-verification issue raised repeatedly in earlier review rounds on this PR (pushCommitQC verifying an incoming CommitQC against a stale cached epoch rather than the epoch derived from the QC's own road index) is no longer present in the current code — pushCommitQC now resolves the epoch fresh via registry.EpochAt(qc.Proposal().Index()) before calling qc.Verify(), matching the pattern already used by avail.PushCommitQC and data.PushQC.
Extended reasoning...
Overview
This PR reworks Autobahn's epoch handling to support a sliding Prev|Current EpochDuo window instead of a single epoch, so CommitQC admission can advance ahead of a lagging AppQC by at most one epoch. It touches the epoch registry and duo/window types (autobahn/types/epoch*.go), the avail layer's CommitQC/vote/block admission and pruning (internal/autobahn/avail/*), the data layer's QC/block ingestion and epoch seeding on restart (internal/autobahn/data/state.go), consensus's view/epoch tracking (internal/autobahn/consensus/*), and the p2p giga routers that wire these together. It is a large, interdependent rewrite of consensus-critical admission and restart-seeding logic, not a localized change.
Security risks
No injection/auth/crypto-primitive concerns in the traditional sense, but this is BFT safety-critical code: incorrect epoch/committee resolution when verifying CommitQC/AppQC/vote signatures could let the wrong committee's signatures validate a QC, and incorrect window/leash logic could stall consensus (liveness) or, worse, let a node act on a QC verified against the wrong epoch's committee (safety). The PR's own description flags a local WAL/BlockDB compatibility break for existing Autobahn state.
Level of scrutiny
This deserves the highest level of scrutiny this review tier can give, and beyond that, human sign-off given the size and criticality — not because current findings indicate a problem, but because the failure mode for consensus admission/verification bugs is safety or liveness loss on a live network, and the diff is large enough (47 files, many new invariants around epoch registry seeding, leashes, and duo sliding) that a single review pass has limited confidence of full coverage.
Other factors
This PR has already been through an unusually long and thorough review cycle (multiple rounds from cursor/codex bots, human reviewers, and this reviewer's own prior inline comments across roughly two weeks), with the epoch-verification staleness issue raised repeatedly in earlier rounds now fixed in the current diff. Test coverage is extensive (new epoch_duo_test.go, block_votes_test.go, and many restart/boundary-condition tests). Given the history of subtle boundary bugs found and fixed over the course of this PR, and that this touches the consensus safety path, a human reviewer familiar with the Autobahn design should still sign off before merge.
There was a problem hiding this comment.
Large, unusually well-tested refactor that replaces the single-epoch LatestEpoch() model with a sliding Prev|Current EpochDuo across types/avail/data/consensus, plus a road-indexed epoch registry with explicit seal leashes. I traced the main new invariants (seal prune/exec leashes, restart seeding via SetupInitialDuo, tipcut ordering avail ≥ consensus ≥ data, prune-anchor windowing, PushVote re-check after lock release) and found no confirmed correctness bug; remaining findings are robustness, error-path fatality, docs, and operator-diagnostics level.
Findings: 0 blocking | 11 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only Claude + Codex. Codex reported no material issues, which matches my own conclusion on correctness. sei-tendermint/autobahn/types/errors.godeletes the doc comments forErrBlockOutOfOrder,ErrQCNonContiguousandErrBlockMissingQCoutright, leaving three exported vars undocumented (RoadRange.HasandNewEpochinepoch.golikewise lose theirs).AGENTS.mdsays "Avoid removing comments and logs which are not obviously obsolete. Keep the original wording, only fixing mistakes or obsolete parts," and the newly-added guidance in the same file asks for concise invariant/contract comments — not for dropping the contract entirely. The condensedErrNotFound/ErrPrunedrewrites are fine; consider keeping a one-liner on the three now-bare vars.- Operator diagnostics for the documented WAL/BlockDB break: bounding epoch 0 to
[0, EpochLength)means a pre-existing Autobahn store that stampedepoch_index=0past road 108 000 will fail startup insideverifyLoadedCommitQC/loadFromBlockDBwith a generic "verify"/"epoch not registered" error rather than anything pointing at "wipe PersistentStateDir". Since Autobahn isn't on a live network this is low risk, but a targeted error message (or a store-format marker) would save a debugging cycle for anyone on a devnet. PushCommitQC/PushAppQCpark indefinitely inadmitRoadOrDropon unverified input whose road is ahead of Current (the code comments call this out as intentional backpressure). A peer sending a garbage CommitQC at a far-future road holds one handler goroutine until ctx cancel. This is the same primitive the pre-existingwaitForCommitQC(idx-1)already offered and the blast radius is the offending peer's own stream, so it's not a regression — just worth a note now that the wait is a documented design point.avail.PushBlock/PushVotenow verify only againstduo.Current's committee (replacingRegistry.VerifyInWindow) and return an error rather than a silent drop for a signer/lane that is only in Prev. That error propagates into the giga stream handlers, so once committees actually change per epoch this becomes peer teardown at every boundary. Today it is a no-op (all epochs share the genesis committee) and theTODO(lane-id)/TODO(lane-expiry)markers cover the follow-up; flagging so it doesn't get lost.- Test coverage is genuinely strong here —
GenRegistryrandomizing the start epoch, thesynctest-basedPushVoteepoch-advance race tests, the epoch-0 seal test, and the end-to-endTestRestart_DataTipEpochN_AvailConsensusEpochNPlus1all target the right invariants. No gaps worth blocking on. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil | ||
| } | ||
|
|
||
| func (i *inner) laneQC(lane types.LaneID, n types.BlockNumber) utils.Option[*types.LaneQC] { |
There was a problem hiding this comment.
[suggestion] i.lanes[lane] is dereferenced without an ok-check, so a Current-committee lane that is absent from i.lanes panics with a nil deref. Every sibling accessor added in this PR (NextBlock, Block, PushBlock, PushVote, WaitForLocalCapacity, headers, markBlockPersisted, the Run export loop) does check. It's safe today — newInner seeds startEpochDuo.Current's lanes and advanceEpoch calls getOrInsertLane for the new Current before storing the duo, so the only caller (WaitForLaneQCs, which iterates epochDuo.Load().Current) can't miss — but that's an invariant spanning three functions rather than a local guarantee, and it stops holding the moment committees stop being genesis placeholders. Cheap to make it getOrInsertLane(lane) or return None on miss.
| // after its AppQC floor has made it obsolete). The registry is | ||
| // independent of Availability pruning (restart + admission / | ||
| // execution leash), not the live window. | ||
| ep, err := s.epochDuo.Load().EpochForRoad(anchor.Proposal().Index()) |
There was a problem hiding this comment.
[suggestion] This resolves the prune anchor against epochDuo and treats any miss as fatal — the error returns out of runPersist, which fails avail.Run and takes the node down. The interlock argument in the comment holds for the steady state (leaving epoch N requires an AppQC in N, so latestAppQC's road is never below the window's first), but the anchor here is a value captured by an earlier collectPersistBatch call, so strictly speaking a persist iteration that is descheduled while the duo slides twice would hit ErrRoadBeforeWindow and crash. That needs a full epoch (108 000 roads) of progress inside one loop iteration, so I don't think it's reachable — but given the cost of being wrong, consider mapping ErrRoadBeforeWindow to "skip this anchor and log" and keeping the hard failure only for ErrRoadAfterWindow.
| // view timeout (typically one timeout round per transition). | ||
| i := s.innerRecv.Load() | ||
| if voteEp := vote.Msg().Proposal().View().EpochIndex; voteEp != i.epochs.Current.EpochIndex() { | ||
| logger.Debug("dropping prepare vote for non-current epoch", |
There was a problem hiding this comment.
[nit] The Current-epoch-only contract added here (and identically in PushCommitVote / PushTimeoutVote) means every epoch boundary costs at least one view timeout while peers disagree on Current, with no redelivery. The comment states this explicitly and it's the right trade-off versus verifying against the wrong committee, so no change requested — but with EpochLength = 108_000 this is a recurring, predictable liveness dip, and it would be worth a metric or an Info-level counter (rather than Debug) so boundary-induced timeouts are distinguishable from real ones in production.
| // PushCommitQC may no-op (stale / not yet Current) and still | ||
| // return nil. Only advance once to's tip covers this road so | ||
| // we do not skip an index that was never admitted. | ||
| if err := to.waitForCommitQC(ctx, qc.Index()); err != nil { |
There was a problem hiding this comment.
[nit] Correct fix for the "PushCommitQC may no-op and still return nil" problem, but if a QC is legitimately dropped as stale (behind windowFirst) this now blocks until ctx cancel, turning a test failure into a hang whose cause isn't obvious from the output. Bounding the wait (or asserting the tip advanced with a clear message) would fail faster.
| return nil, fmt.Errorf("road %d before window %v: %w", roadIdx, w, ErrRoadBeforeWindow) | ||
| } | ||
|
|
||
| func (w EpochDuo) String() string { |
There was a problem hiding this comment.
[nit] The loop emits Current before Prev, so a duo of Prev=4|Current=5 renders as epochs [5, 4]. Since this string lands in EpochForRoad's before/after-window errors and the admitRoadOrDrop debug log, ascending order ([4, 5]) would read more naturally next to the [First, Next) range convention this PR standardises on elsewhere.
There was a problem hiding this comment.
No bugs found in this run, but this PR is a large, multi-file rewrite of Autobahn's core consensus/data/avail epoch-transition logic (CommitQC/AppQC admission, epoch registry seeding, restart tip-interlocking) — this is consensus-critical code that warrants a human look regardless of automated findings.
What was reviewed: epoch-duo (Prev|Current) window admission and leash logic in avail/data/consensus PushCommitQC/PushQC/PushAppQC; the CommitQC/AppQC/Proposal Verify epoch-resolution paths; restart seeding via SetupInitialDuo and registry EpochAt/DuoAt; vote crediting and reweighting across epoch boundaries. All prior review threads on this PR (including several rounds of a genuine epoch-verification bug in consensus pushCommitQC flagged repeatedly across revisions) are now marked resolved, and the current diff's pushCommitQC resolves the QC's epoch from its own road index via the registry rather than a cached epoch, consistent with that feedback.
Extended reasoning...
Overview
This PR (CON-358) introduces a sliding Prev|Current EpochDuo window across Autobahn's avail, data, and consensus layers so nodes can finish a lagging AppQC from the previous epoch while CommitQC tipcuts advance in the current epoch. It touches ~47 files: epoch/registry seeding and restart logic, CommitQC/AppQC/Proposal verification, vote crediting per-epoch, and the p2p giga routers that wire these layers together. This is core consensus/liveness/safety-critical code for the chain's BFT engine.
Security risks
No injection/auth-bypass/data-exposure concerns (this is internal consensus-engine logic, not a network-facing trust boundary in the traditional sense), but the risk category here is protocol safety and liveness: an incorrect epoch-window admission or leash check can cause validators to verify against the wrong committee (safety) or deadlock/hard-fail at epoch boundaries (liveness). The PR's own review history bears this out — a real bug was found and iterated on for over a week (consensus.pushCommitQC verifying a CommitQC against a stale cached epoch instead of the epoch resolved from the QC's own road index), taking many revisions before it was actually fixed rather than just re-described.
Level of scrutiny
This warrants the highest level of scrutiny this review system supports. It is a large, non-mechanical change to a consensus-critical subsystem with intricate cross-layer invariants (registry seeding order, admit-then-verify sequencing, boundary leashes) that are easy to get subtly wrong, as the extensive back-and-forth in this PR's own history demonstrates. Approving this without a human sign-off would be inappropriate regardless of the current run's findings.
Other factors
The PR has extensive existing scrutiny: multiple human reviewers (wen-coding, seidroid, pompon0), Cursor Bugbot, and this bot's own prior inline comments, all of which are now marked resolved. Test coverage is substantial (many new/updated unit tests for epoch duo, registry seeding, restart interlocking, and boundary races). Given the resolved history and the current bug-hunting pass finding nothing new, I'm not raising new concerns — but the combination of size, criticality, and past defect density means this should go through human review rather than be shadow-approved.

Summary
Autobahn keeps a sliding Prev|Current
EpochDuowindow so nodes can finish late AppQC from the previous epoch while tipcuts advance in Current.Data flow
AdvanceIfNeeded) registers epoch N+1 for the execution leash.data.NewState→SetupInitialDuo(commitQCs).Some([First, Next)): seed epochs covering the retained CommitQC range,EnsureDuoAt(Next), then placeholderwindowLast+1/+2(genesis committees only; goes away when committees track execution).None: empty store →{0,1}.EpochAt/DuoAthard-fail.<view index.Behavior change
Local-state note
Bounding epoch 0 to
[0, EpochLength)is a local WAL/BlockDB break for any Autobahn store that previously stamped every road asepoch_index=0past road 108 000. Autobahn is not on a live network; wipe PersistentStateDir / BlockDB (or start fresh) when upgrading this branch. Labelnon-app-hash-breakingcovers app hash only.Test plan