feat(autobahn): Add epoch.Registry to maintain committee/stake (CON-358)#3632
feat(autobahn): Add epoch.Registry to maintain committee/stake (CON-358)#3632wen-coding wants to merge 3 commits into
Conversation
PR SummaryHigh Risk Overview
Protobuf decode hard-rejects missing Reviewed by Cursor Bugbot for commit 4cae81a. 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).
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
wen-coding
changed the title
wen-coding
force-pushed
the
wen/autobahn_epoch_registry
branch
from
d764443 to
705d8c9
Compare
wen-coding
force-pushed
the
wen/autobahn_epoch_registry
branch
2 times, most recently
from
ab4c67c to
9cce5d2
Compare
wen-coding
force-pushed
the
wen/autobahn_epoch_registry
branch
3 times, most recently
from
f1f1d55 to
09e677e
Compare
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured refactor that introduces epoch.Registry and bakes firstBlock/epoch_index into Proposal so GlobalRange() returns absolute block numbers and messages are self-contained on the wire. No confirmed blocking bugs; test coverage is strong. One latent epoch-index footgun in ViewSpec.View() (raised by Codex) is worth hardening before dynamic epochs land, plus a few minor doc/coverage notes.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor produced no second-opinion output (cursor-review.md was empty), and REVIEW_GUIDELINES.md from the base branch was empty — this review proceeded without repo-specific guidelines or a Cursor pass.
- epoch/registry_test.go only covers EpochByIndex. Consider adding coverage for VerifyInWindow, LatestEpoch, FirstBlock, and GenesisTimestamp, since these are now on the hot path for all committee lookups (avail/data/consensus).
- Doc mismatch on the new Proposal.global_first field: autobahn.proto describes it as "first global block number of this proposal's global range" while the generated pb.go comment says "first global block number of the epoch". The proto wording is the accurate one (for non-first proposals global_first == CommitQC.GlobalRange().Next, not the epoch's first block); the generated comment is misleading. Regenerate/realign the comment. Also AppProposal.epoch_index lacks the "// required" annotation the other fields carry, even though Decode rejects it when nil.
- data/state.go PushBlock now reads the epoch under the lock, verifies outside the lock, then re-acquires the lock to insert — a good pattern, but it relies on inner.qcs[n] still being present on the second lock (guarded by the n < inner.first re-check). Worth a brief comment noting nextQC is monotonic so qcs[n] cannot disappear for first <= n < nextQC.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return view | ||
| } | ||
| return View{Index: idx, Number: 0} | ||
| return View{Index: idx, Number: 0, EpochIndex: vs.Epoch.EpochIndex()} |
There was a problem hiding this comment.
[suggestion] Latent epoch-index bug (also flagged by Codex). The early-return branch above (if view := NextViewOpt(vs.TimeoutQC); view.Index == idx { return view }) returns the view from NextViewOpt, which for a None TimeoutQC yields View{Index:0, Number:0, EpochIndex:0}. So when idx == 0 and there is no TimeoutQC, View() returns EpochIndex: 0 regardless of vs.Epoch.EpochIndex(), and View.Verify(ep) would then reject it for any non-zero epoch. Today this is masked by the road-range invariant (a non-genesis epoch's first road index is > 0, so idx == 0 only occurs for epoch 0), so it is not currently triggerable — but it silently depends on that invariant and will break when dynamic epochs are wired up. Suggest stamping the epoch index unconditionally, e.g. set view.EpochIndex = vs.Epoch.EpochIndex() before returning in the branch above (the value from a real TimeoutQC should already match vs.Epoch).
There was a problem hiding this comment.
A large, internally consistent refactor that introduces epoch.Registry, bakes firstBlock/epochIndex into the wire Proposal/View/AppProposal so GlobalRange() returns absolute numbers, and routes all committee lookups through the registry. The change is well-covered by updated tests (including a new epoch-binding test and a reproposal regression test) and I found no blocking correctness or security issues; a few non-blocking observations follow.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes gave no substantive coverage:
codex-review.mdreports "No material findings" but explicitly says it was static-only (Go 1.25.6 toolchain unavailable, network blocked — no tests run), andcursor-review.mdis empty. This synthesis review is therefore effectively the sole gate;REVIEW_GUIDELINES.mdis also empty, so no repo-specific standards were applied. - Wire compatibility is intentionally broken (Proposal/View/AppProposal decode hard-reject missing
global_first/epoch_index). This is called out in-code as acceptable because Autobahn is pre-production with no rolling-upgrade path — fine, but worth confirming no persisted PersistedInner/AppQC state from older binaries needs to survive a restart, since those decoders now also require the new fields. - Deferred multi-epoch correctness is well-flagged but real:
consensus/inner.gonewInner assumesLatestEpoch()(TODO notes a restart after an epoch transition would fail validation), andRegistry.VerifyInWindowonly verifies against the latest epoch (TODO to expand to neighbor epochs). Until dynamic epochs land these are inert (only genesis epoch 0 exists, and non-zero epochIndex messages are correctly rejected), but they are latent bugs to resolve before enabling epoch transitions. epoch.NewRegistryreturns(*Registry, error)yet never produces a non-nil error; callers must handle a nil error today. Harmless/future-proofing, but slightly misleading — consider documenting that the error is reserved for future validation.- Registry read accessors use the
for s := range r.mu.RLock() { return ... }; panic("unreachable")iterator pattern; correctness depends on the RWMutex iterator yielding exactly once. It matches the surrounding codebase idiom, so no change requested — just noting the reliance.
There was a problem hiding this comment.
Large, mostly-mechanical refactor that introduces epoch.Registry and bakes firstBlock/epochIndex into proposals/QCs so GlobalRange() returns absolute numbers and messages are self-contained on the wire. The change is internally consistent, removes the reproposal double-counting bug, adds epoch-binding verification throughout, and is extensively covered by updated tests; I found no correctness blockers, only minor code-quality notes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Wire-format change is intentionally hard-breaking:
View.epoch_index,Proposal.global_first, andAppProposal.epoch_indexare all required on decode with no rolling-upgrade path (per the code comment, acceptable because autobahn is pre-production). Worth calling out for release/coordination awareness — all nodes must upgrade in lockstep and any persisted pre-change WAL/state is unreadable. - Forward-looking scaffolding currently has no live effect:
epoch.Registry.NewRegistryalways returns a nil error, andRegistry.VerifyInWindowreturns[]*types.Epochthat every caller discards (if _, err := ...). Since the registry only ever holds the genesis epoch (index 0), all the newEpochByIndex(...)/epoch-binding checks effectively reduce toepochIndex == 0today. This matches the stated intent of wiring the read side first, but reviewers should be aware the multi-epoch paths are untested untilAddEpochlands (several TODOs flag this, e.g. consensus/inner.go restart-after-transition and avail per-epoch vote weighting). - Second-opinion inputs: Codex reported "No material findings" (tests not runnable — Go 1.25.6 toolchain download blocked in its sandbox); the Cursor review file and REVIEW_GUIDELINES.md were both empty.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // with a block not finalized by prev. | ||
| func (s *State) WaitForLaneQCs( | ||
| ctx context.Context, prev utils.Option[*types.CommitQC], | ||
| ctx context.Context, ep *types.Epoch, prev utils.Option[*types.CommitQC], |
There was a problem hiding this comment.
[nit] The ep *types.Epoch parameter is unused: the body iterates inner.blocks and resolves the committee via inner.laneQC(...) (which reads i.epoch), never touching ep. Either use ep (e.g. to select the committee/lane set for the requested epoch) or drop the parameter to avoid a misleading signature. Callers currently pass vs.Epoch / registry.LatestEpoch(), implying it matters.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 6 total unresolved issues (including 5 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 3d7872b. Configure here.
There was a problem hiding this comment.
A large but mostly mechanical refactoring that introduces epoch.Registry and bakes firstBlock/epochIndex/globalFirst into the wire messages, threading an *Epoch through all autobahn verification paths; it also fixes a genuine reproposal firstBlock double-counting bug and adds extensive test coverage. Core epoch-binding and globalFirst validation are correctly guarded by signatures, quorum, and FullProposal.Verify; no blocking issues found.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex and Cursor second-opinion passes produced no findings; Codex additionally noted it could not run a targeted Go compile check because the sandbox lacks the required Go 1.25.6 toolchain, so this review is based on static analysis only — recommend confirming CI
go test -raceis green before merge. - Wire/WAL-breaking change: ProposalConv.Decode now hard-rejects messages missing
global_first(and View/AppProposal requireepoch_index). This is deliberate and documented as acceptable for pre-production autobahn, but it means any autobahn state persisted by an older binary (consensus/data WALs) will fail to decode on restart across this upgrade — worth calling out explicitly in release notes for any running test nets. - PR description references
registry.CommitteeFor(roadIndex), but no such method exists — the code routes committee lookups throughEpochByIndex(...)/LatestEpoch().Committee(). Minor description drift, no code impact. Registry.VerifyInWindowreturns[]*types.Epochso callers can skip re-verification, but every current caller ignores the returned slice (if _, err := ...). Fine as a forward-looking API, just currently dead return value.Proposal.Verifynow checksr.first > r.nextinline (proposal.go:245) which is already enforced by the subsequentr.Verify(c)(LaneRange.Verify at proposal.go:56); harmless defense-in-depth but redundant.
There was a problem hiding this comment.
A large, well-tested read-side refactor that introduces epoch.Registry/Epoch and bakes an absolute global_first + epoch_index into Proposal/AppProposal/View, routing all committee lookups through the registry. The change is internally consistent and thoroughly re-tests the affected verification paths; I found no blocking correctness or security issues, only a few non-blocking observations (including a downgrade of Codex's severity assessment).
Findings: 0 blocking | 6 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex (High) —
global_firsttrusted without structural validation in Proposal/CommitQC/FullCommitQC.Verify: I confirmed the type-layer Verify methods do not re-derive/continuity-check global_first, but I disagree with the High severity. The absolute numbering is anchored to the trusted localregistry.FirstBlock()viainner.nextQC, and bothdata.inner.insertQCandState.PushQCrequiregr.First == inner.nextQC(gaps rejected, stale skipped), while a valid signed CommitQC implies a quorum of honest validators already ranFullProposal.Verify(which checksGlobalRange().First == NextGlobalBlock()). So arbitrary absolute block numbers cannot be committed to state under the honest-majority model. Worth considering the suggested continuity assertion in CommitQC.Verify purely as defense-in-depth for a future QC-sync path that bypasses the data layer's nextQC anchoring. - Wire-format breaking change: ProposalConv/AppProposalConv/ViewConv now hard-reject messages missing
global_first/epoch_index. The code comments and PR description state autobahn is pre-production with no rolling-upgrade path, so this is acceptable — flagging only for release-coordination awareness (all nodes must upgrade together). - Behavioral change in
data.State.PushBlock: it now blocks (WaitUntiln < inner.nextQC) until the QC fornis present before verifying, whereas it previously verified+inserted immediately. Worth confirming all callers (e.g. giga data-sync RPC handlers) pass a bounded/cancelable context so a block whose QC never arrives cannot wedge a goroutine indefinitely. - Second-opinion inputs:
cursor-review.mdandREVIEW_GUIDELINES.mdare both empty (Cursor produced no output and no repo-specific review standards were available); reviewed without them. - Registry currently returns the genesis epoch for every lookup and several call sites carry
TODOs for multi-epoch wiring (consensusnewInnerassumesLatestEpoch(),VerifyInWindowonly checks latest, avail vote weighting is single-committee). These are intentional per the PR's read-side-first strategy but represent latent correctness gaps that must be closed before dynamic epochs land — a restart after an epoch transition would currently fail persisted-state validation. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return fmt.Errorf("roadIndex() = %v, want %v", got, want) | ||
| } | ||
| if got, want := m.GlobalNumber(), qc.GlobalRange(c); got < want.First || got >= want.Next { | ||
| if got, want := m.GlobalNumber(), qc.GlobalRange(); got < want.First || got >= want.Next { |
There was a problem hiding this comment.
[nit] Now that qc.GlobalRange() no longer takes a committee, the c *Committee parameter of AppProposal.Verify (line 47) is unused. Consider dropping it and updating the caller in avail/state.go (PushAppVote) to reduce dead surface.
wen-coding
force-pushed
the
wen/autobahn_epoch_registry
branch
from
18a5cd2 to
cd2a1fa
Compare
There was a problem hiding this comment.
A large but disciplined, well-tested refactor that introduces epoch.Registry/types.Epoch as the authoritative committee source and bakes global_first / epoch bindings into proposals so GlobalRange() is self-contained. I found no correctness or security blockers; API replacement is consistent (no dangling call sites) and the new wire fields are quorum-signed, so global_first can't be forged. Non-blocking notes only.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Wire-breaking change: Proposal.global_first, View.epoch_index, and AppProposal.epoch_index are hard-rejected on decode when absent (proposal.go ProposalConv.Decode, app_proposal.go, ViewConv.Decode). There is no rolling-upgrade path, so every node must upgrade in lockstep. This is explicitly acknowledged in the PR as acceptable because autobahn is pre-production — worth surfacing for deploy coordination.
- Second-opinion inputs were empty/negative: REVIEW_GUIDELINES.md and cursor-review.md are empty, and codex-review.md reports no findings (it also notes it could not compile/run tests due to a Go 1.24 vs required 1.25.6 toolchain mismatch in its sandbox). No prompt-injection content was present in the PR.
- Several avail/data entry points resolve the epoch from a message-supplied index before or around verification (avail/state.go PushAppVote uses v.Msg().Proposal().EpochIndex() to pick the committee for the initial sig check; PushCommitQC/insertQC/PushQC key off qc.Proposal().EpochIndex()). This is safe today — only the genesis epoch exists and the values are later cross-checked against the trusted local commitQC (AppProposal.Verify enforces epoch equality) — but the cross-checks should be re-audited when dynamic epochs (AddEpoch) land. The code already flags this with TODOs.
- Numerous TODOs mark the intentionally-unwired dynamic-epoch behavior (consensus/inner.go restart-after-transition assumes LatestEpoch; avail per-epoch vote weighting; Registry.VerifyInWindow neighbor epochs; epoch rotation in pushCommitQC/pushTimeoutQC). Reasonable for an incremental read-side-first PR, but restart correctness after a future epoch transition is not yet covered.
wen-coding
force-pushed
the
wen/autobahn_epoch_registry
branch
from
cd2a1fa to
d31d417
Compare
There was a problem hiding this comment.
A large but well-structured, well-tested refactor introducing epoch.Registry and threading Epoch/epoch_index through the autobahn consensus verification stack; the read-side wiring is consistent and unknown/forged epoch indices are rejected at every call site. No blocking issues found; a few non-blocking notes on wire-compatibility and nil-panic footguns.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Wire-format break:
ProposalConv.Decode,ViewConv.Decode, andAppProposalConv.Decodenow hard-reject any message missingglobal_first/epoch_index, andViewgained a signed field, so old and new nodes cannot interoperate. The PR acknowledges autobahn is pre-production with no rolling-upgrade path (labelnon-app-hash-breaking) — acceptable, but worth confirming no deployed peers still speak the old format before merge. ViewSpec.Epochis now required and dereferenced (viewSpec.Epoch.Committee(),vs.Epoch.EpochIndex()) inNewProposal,ViewSpec.View(),NextGlobalBlock(), andNextTimestamp(); a nil Epoch panics rather than erroring. It's documented in the struct comment and all current call sites set it, but it's a footgun for future callers — a guarded error or a constructor that enforces non-nil would be safer.- Several correctness-critical TODOs are left for follow-up dynamic-epoch PRs and are load-bearing:
Registry.VerifyInWindow/LatestEpochonly ever consult epoch 0, andnewInnerassumesLatestEpoch()(comment notes a restart after an epoch transition would fail validation). Fine for this read-side-only PR since only the genesis epoch exists, but these must land before dynamic epochs are enabled. - Second-opinion passes: OpenAI Codex reported no material findings (targeted tests couldn't run due to a Go 1.24.13 vs required 1.25.6 sandbox mismatch); the Cursor review file was empty (no output produced).
wen-coding
force-pushed
the
wen/autobahn_epoch_registry
branch
12 times, most recently
from
057de21 to
8d77263
Compare
- Introduce epoch.Registry with genesis epoch and EpochByIndex/LatestEpoch/VerifyInWindow - Add Epoch type (index, roads, firstBlock, timestamp, committee) - Wire Registry through avail/consensus/data State constructors and run loops - BuildCommitQC takes *Epoch; WaitForLaneQCs iterates epoch committee lanes - View gains EpochIndex field; View.Less includes EpochIndex in ordering - global_first proto field stores GlobalRange.First directly (not epoch offset) - Remove GlobalFirst() method from Proposal; enforce epoch binding via FullProposal.Verify - Various reviewer feedback: rename registryState field mu→state, add TestViewLess, fix NextGlobalBlock comment, add TestWaitForLaneQCs_OnlyReturnsCommitteeLanes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
wen-coding
force-pushed
the
wen/autobahn_epoch_registry
branch
from
8d77263 to
bbfb131
Compare
…verage in runPersist Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large but mechanical refactor introducing epoch.Registry as the single source of committee/stake, moving firstBlock/genesisTimestamp into Epoch, storing GlobalRange directly on the wire, and adding epoch-binding checks to all QC Verify paths. The change is internally consistent, well-covered by new and updated tests, and the wire fields (global_first, epoch_index) are authenticated via the existing signed-proposal hash; only minor non-blocking nits found.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes produced little: Codex reported "No material findings" (and noted it could not run tests due to a Go 1.24 vs required 1.25.6 mismatch); the Cursor review file was empty (no output). No additional issues were surfaced by either.
- Wire compatibility is intentionally broken: Proposal.global_first, View.epoch_index, and AppProposal.epoch_index are
optionalin the proto but hard-rejected when absent on decode. This is acknowledged in-code as acceptable because Autobahn is pre-production with no rolling-upgrade path — fine, but worth confirming no already-persisted WAL/snapshot from before this change needs to be decodable. - Hot-path lock: EvmProxy (giga_router_common.go / giga_router_validator.go) now resolves the committee via r.data.Registry().LatestEpoch().Committee(), which takes an RWMutex RLock on every EVM proxy request where it previously read an immutable field directly. Negligible today, but a per-request lock on a routing hot path is worth keeping in mind.
- Correctness of the epoch-binding refactor relies on the genesis epoch using OpenRoadRange() (Last = MaxUint), so View.Verify's road-range check is effectively a no-op in production until epoch transitions land. The numerous TODOs (per-epoch weight counting in avail, epoch resolution on restart in consensus/newInner, cross-epoch batch lanes in runPersist) correctly scope this PR to the single-genesis-epoch case.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
…emove redundant lane checks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large but well-structured refactor introducing epoch.Registry as the single source of truth for committee/stake and adding EpochIndex binding to all QC verification paths; the bulk of the diff is mechanical test/API plumbing, the new epoch-binding logic is coherent and well-tested at the types layer, and I found no blocking correctness or security issues. Verdict is neutral only because of a few non-blocking observations and because one of the second-opinion passes produced no output.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage:
cursor-review.mdwas empty (no output produced by that pass), and Codex reported no material findings (its targetedgo testcould not run because the Go 1.25.6 toolchain download was blocked by the sandbox). This review therefore rests primarily on this pass; the epoch-binding paths would benefit from a realgo test ./sei-tendermint/autobahn/... ./sei-tendermint/internal/autobahn/...run in CI before merge. - Wire trust model for
global_first:Proposal.GlobalRange().Firstis now decoded verbatim from the newglobal_firstwire field rather than derived from a trusted localcommittee.FirstBlock(). Continuity (First == prev.GlobalRange().Next) is only re-enforced inFullProposal.VerifyviaNextGlobalBlock(); standaloneCommitQC.Verify/PrepareQC.Verifyaccept the committee-signed value as-is. This is consistent with the honest-quorum trust assumption, but a short comment onnewProposal/NextGlobalBlocknoting that standalone QC verification trusts the quorum-attested offset would help future readers. - Test coverage gap on the new defensive branches: the numerous
unknown epoch_index %derror returns and epoch-mismatch rejections added across the avail/consensus/data state layers (e.g.PushCommitQC,PushAppVote,PushQC,insertQC,PushBlock) are effectively unreachable today because the registry only holds the genesis epoch, so none have direct unit tests. Worth adding coverage whenAddEpoch/epoch transitions land. - Acknowledged future-correctness TODOs are extensive and should be tracked as a follow-up gate before dynamic epochs ship:
consensus/inner.go newInnerassumesLatestEpoch()and will fail validation on restart after an epoch transition;availweight counting and lane-union inrunPersistare single-epoch only;consensusdoes not rotateepochon CommitQC/TimeoutQC advance. All are clearly commented, and the PR explicitly scopes dynamic transitions to follow-up PRs, so this is just tracking, not a blocker. - Breaking wire/persistence format change (added required
View.epoch_index,AppProposal.epoch_index,Proposal.global_first): decode hard-rejects messages/persisted state lacking these fields, so there is no rolling-upgrade path from pre-PR encodings. The code comments state autobahn is pre-production and this is intentional; flagging only so it is a conscious release decision.
2 participants
Summary
epoch.Registryas the single source of truth for committee and stake. All layers (consensus, avail, data) read from it viaLatestEpoch()/EpochByIndex().firstBlockandgenesisTimestampout ofCommitteeand intoEpoch.Proposalnow storesGlobalRangedirectly, making it self-contained on the wire and fixing a reproposal bug wherefirstBlockwas double-counted.EpochIndextoViewandAppProposal. All QCVerifymethods now check epoch binding. The registry currently holds only the genesis epoch — dynamic epoch transitions are wired in follow-up PRs.