feat(autobahn): Add epoch.Registry to maintain committee/stake (CON-358)#3632
Conversation
PR SummaryHigh Risk Overview
Protobuf adds required Reviewed by Cursor Bugbot for commit db89d88. 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).
|
d764443 to
705d8c9
Compare
ab4c67c to
9cce5d2
Compare
f1f1d55 to
09e677e
Compare
| } | ||
| for cv := range s.commitVotes.Lock() { | ||
| cv.pushVote(s.Data().Committee(), vote) | ||
| cv.pushVote(committee, vote) |
There was a problem hiding this comment.
add a todo to make all the votes structures (commitVotes, etc.) also scoped to a single epoch.
| return nil | ||
| } | ||
| qc := inner.qcs[n] | ||
| var ok bool |
There was a problem hiding this comment.
nit: epoch lookup can be done without lock
| vs := types.ViewSpec{CommitQC: prev, Epoch: ep} | ||
| p := types.NewAppProposal(cqc.GlobalRange().Next-1, vs.View().Index, types.GenAppHash(rng), ep.EpochIndex()) | ||
| appQC = utils.Some(TestAppQC(keys, p)) | ||
| } else { |
There was a problem hiding this comment.
nit: None is the zero value, no need to set it explicitly
| // App hash at that block. | ||
| optional bytes app_hash = 3 [(wireguard.max_size) = 32]; // required | ||
| // Epoch this proposal belongs to. | ||
| optional uint64 epoch_index = 4; |
| optional Timestamp timestamp = 5; // required | ||
| repeated LaneRange lane_ranges = 3 [(wireguard.max_count) = 100]; // Sorted by lane. | ||
| optional AppProposal app = 4; // optional | ||
| optional uint64 global_first = 6; // first global block number of this proposal's global range |
| idx, | ||
| types.RoadIndex(offset), | ||
| types.GenAppHash(rng), | ||
| 0, |
- 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>
…verage in runPersist Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…emove redundant lane checks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… var, fix error wrapping Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fields Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…poch-scoped votes TODO Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
makeEpoch now delegates to GenEpochWithCommittee so epoch index, RoadRange.First, firstBlock, and timestamp are all randomized, preventing tests from silently passing on zero values. GenEpochWithCommittee guarantees Last >= First+10 (was +1) so callers can safely use First+1 as a view index. The two TimeoutQC tests build a CommitQC at RoadRange.First and use First+1 as the timeout view index, satisfying the NextIndexOpt(prev)==view.Index invariant checked by Verify. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CommitQCAt(ep, keys) creates a CommitQC at ep.RoadRange().First signed by the given keys. GenCommitQC(rng) now delegates to it instead of generating random unsigned bytes. committee_test.go: use CommitQCAt for TimeoutQC prev; use ep.EpochIndex() in TestAppQCVerifyChecksWeight instead of hardcoded 0. conv_test.go: use commitQC.Proposal().EpochIndex() instead of hardcoded 0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…vention Per pompon0's review request: annotate proto fields with // required or // optional to clarify intent, since all fields use the optional keyword for presence detection and wireguard bounds checking. Required fields are nil-checked at the boundary (constructor and proto Decode) and trusted everywhere else. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pointer fields are required by default per AGENTS.md convention. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ded 0 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
38db0ce to
3122be2
Compare
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, moving firstBlock/genesisTimestamp into Epoch, adding EpochIndex to View/AppProposal, and threading epoch-binding checks through all QC Verify methods. Test coverage is extensively updated and the new wire fields are correctly covered by the signed message hash; no blocking issues found.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Both second-opinion review files are empty: codex-review.md (OpenAI Codex) and cursor-review.md (Cursor) produced no output, so this synthesis reflects only the primary pass.
- epoch/registry_test.go only exercises EpochByIndex (genesis-found and unknown-not-found). VerifyInWindow, LatestEpoch, FirstBlock, and GenesisTimestamp are only covered indirectly through other packages — consider adding direct unit tests, especially for VerifyInWindow's fn-accept/reject and returned-epoch-slice contract.
- Security model note (verified OK, no action needed): the newly wire-transmitted Proposal.global_first and epoch_index are trusted-but-verified — FullProposal.Verify checks GlobalRange().First == vs.NextGlobalBlock() and data.insertQC enforces contiguity against nextQC, while quorum-signed CommitQCs rely on honest validators having run FullProposal.Verify before signing. Both fields are included in the signed hash via ProposalConv/ViewConv/AppProposalConv.Encode, so field substitution / signature replay is not possible.
- The many
TODO: ... once epoch transitions are wired upcomments (avail, consensus inner/state, registry.VerifyInWindow, runPersist cross-epoch lane union) are appropriate for this staged rollout since the registry is genesis-only and any non-zero epoch_index message is rejected by EpochByIndex; worth tracking these as a follow-up checklist so none are missed when AddEpoch lands. - Decode paths hard-reject messages missing global_first / epoch_index (documented as acceptable because autobahn is pre-production with no rolling-upgrade path) — confirm no persisted WAL / snapshot written by a pre-change binary must be read back, otherwise restart would fail validation.
…mitQC Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large, mechanical, well-tested refactor that introduces epoch.Registry as the single source of truth for committee/stake and threads Epoch through all QC/proposal Verify paths; it also fixes a reproposal global-range double-count bug. No blocking correctness or security issues found; single-epoch invariants and TODOs consistently guard the not-yet-wired multi-epoch paths.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Both configured second-opinion reviews produced no output: ./codex-review.md (OpenAI Codex) and ./cursor-review.md (Cursor) were empty, and ./REVIEW_GUIDELINES.md was empty, so this synthesis relies solely on my own pass.
- Wire-format change is intentionally breaking: Proposal.Decode hard-rejects messages lacking global_first, and View/AppProposal now require epoch_index. This is documented in-code as acceptable because autobahn is pre-production with no rolling-upgrade path — confirm no persisted WALs or in-flight peers from older builds need to be drained before deploy.
- epoch.Registry.VerifyInWindow returns a []*types.Epoch specifically so callers 'can skip re-verification', but its two callers (avail.State.PushBlock / PushVote) discard the slice (
if _, err := ...). This is forward-looking API surface with no current effect — fine, but worth a note so the intended optimization isn't forgotten. - NewRegistry returns (*Registry, error) yet can never return a non-nil error; likewise the per-epoch weight counting in avail (pushVote uses inner.epoch while VerifyInWindow verified against LatestEpoch) is only safe because a single epoch exists today — both are explicitly TODO-tracked for the follow-up epoch-transition PRs.
- In persist FullCommitQC/GlobalBlock persisters,
firstBlockis reassigned to loaded[0]'s first block on restart (previously it was the genesis FirstBlock); since LoadedFirst() consultsloadedbefore ConsumeLoaded empties it, the NewState skipTo path is unaffected, but the post-consume value now differs from prior behavior — low risk, worth a glance.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large, cleanly-structured refactor introducing epoch.Registry and moving firstBlock/genesisTimestamp into Epoch, with EpochIndex binding added to consensus messages and QC verification and a self-contained on-wire GlobalRange (fixing a reproposal double-count bug). Extensive test updates accompany the change and the single-genesis-epoch assumption is consistently documented with TODOs; no blocking correctness issues found.
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion inputs were empty: both ./codex-review.md and ./cursor-review.md contained no content, so this review reflects only this pass. ./REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied.
- Consensus wire-format break: the new proto fields (View.epoch_index, Proposal.global_first, AppProposal.epoch_index) are treated as required and Decode hard-rejects any message lacking them (e.g. proposal.go returns
global_first: missing). The in-code rationale (autobahn is pre-production, no rolling-upgrade path) is reasonable, but this is worth an explicit call-out given the PR carries thenon-app-hash-breakinglabel yet changes the consensus message wire format — any node/persisted message from before this PR will fail to decode. - Divergent epoch-resolution patterns: consensus/inner.go pushCommitQC verifies against the receiver's current epoch (i.epoch), while avail/state.go PushCommitQC and data/state.go insertQC resolve the epoch from the QC via Registry.EpochByIndex(qc.Proposal().EpochIndex()). Both are correct under today's single-epoch invariant, but the two approaches will need reconciling when dynamic epoch transitions land (already noted by several TODOs).
- epoch.Registry.VerifyInWindow returns []*types.Epoch so callers can skip re-verification of already-checked epochs, but the current callers (avail PushBlock/PushVote) discard the slice with
_. Harmless today; just an unused affordance until multi-epoch is wired up. - data/state.go NewState WAL-load path and PushBlock now dereference inner.qcs[n] to resolve the block's epoch; this relies on the invariant that every loaded/awaited block number is covered by a present QC (WAL reconcile + gap checks + WaitUntil(n < inner.nextQC)). The invariant appears to hold, but a defensive nil-check or comment reaffirming it would guard against a future regression turning it into a nil-deref panic.
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.