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 66 commits into
Conversation
PR SummaryHigh Risk Overview Proposals and wire messages now carry Avail/data resolve epochs via Reviewed by Cursor Bugbot for commit aef198e. 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
| TimeoutQC utils.Option[*TimeoutQC] | ||
| CommitQC utils.Option[*CommitQC] | ||
| TimeoutQC utils.Option[*TimeoutQC] | ||
| FirstBlock GlobalBlockNumber // genesis InitialHeight; added to lane-relative block numbers to produce absolute global block numbers |
There was a problem hiding this comment.
It seems to me that ViewSpec should have the whole EpochConfig instead.
There was a problem hiding this comment.
currently treatment of firstblock and genesisTimestamp is inconsistent
There was a problem hiding this comment.
Put Epoch into ViewSpec.
|
|
||
| // Verify verifies the FullProposal against the current view. | ||
| func (m *FullProposal) Verify(c *Committee, vs ViewSpec) error { | ||
| func (m *FullProposal) Verify(c *Committee, vs ViewSpec, genesisTimestamp time.Time) error { |
There was a problem hiding this comment.
genesisTimestamp seems kind of random in this context and that's why IMO ViewSpec should be self-contained.
| Timestamp: TimeConv.Encode(m.timestamp), | ||
| LaneRanges: LaneRangeConv.EncodeSlice(laneRanges), | ||
| App: AppProposalConv.EncodeOpt(m.app), | ||
| GlobalFirst: &globalFirst, |
There was a problem hiding this comment.
nit: utils.Alloc(uint64(m.firstBlock))
There was a problem hiding this comment.
why inconsistent naming (globalFirst vs firstBlock)?
| app, | ||
| ) | ||
| if m.GlobalFirst == nil { | ||
| return nil, fmt.Errorf("global_first is required") |
There was a problem hiding this comment.
nit: "global_first: missing" for consistency
| // GenProposal generates a random Proposal. | ||
| func GenProposal(rng utils.Rng) *Proposal { | ||
| return newProposal(GenView(rng), time.Now(), utils.GenSlice(rng, GenLaneRange), utils.Some(GenAppProposal(rng))) | ||
| return newProposal(GenView(rng), time.Now(), utils.GenSlice(rng, GenLaneRange), utils.Some(GenAppProposal(rng)), 0) |
There was a problem hiding this comment.
randomize the generated data
| Number: GenViewNumber(rng) % view.Number, | ||
| } | ||
| p := newProposal(pView, time.Now(), utils.GenSlice(rng, GenLaneRange), utils.Some(GenAppProposal(rng))) | ||
| p := newProposal(pView, time.Now(), utils.GenSlice(rng, GenLaneRange), utils.Some(GenAppProposal(rng)), 0) |
| // | ||
| // Currently the committee is fixed at genesis. Dynamic committee support | ||
| // will be wired up when the execution layer is ready. | ||
| type Registry struct { |
There was a problem hiding this comment.
I would like to have a type representing a complete context for an epoch (perhaps just named "Epoch"):
committee,
firstBlock, (or globalRange?)
firstTimestamp,
epochID (?)
...
I currently don't understand what "Registry" represents, but that's perhaps because it is not implemented yet.
There was a problem hiding this comment.
types.Epoch added.
Registry is basically a list of Epochs protected by rwlock, so we can keep generating new Epochs and throwing into it, it will return Epoch from EpochId for readers and purge old Epochs.
| // EpochWindow returns the epoch→committee map for message acceptance across | ||
| // epoch transitions. With a fixed genesis committee it always returns a | ||
| // single entry. | ||
| func (r *Registry) EpochWindow() map[Index]*types.Committee { |
There was a problem hiding this comment.
what is "window"? I don't see it in the doc.
There was a problem hiding this comment.
This is mostly for the Epoch transition, we need to accept lane blocks for the current Epoch and possibly the Epoch before (repair blocks) or after it (others have got the commitQC to transition into new Epoch before me).
| // EpochFor returns the epoch index for the given RoadIndex. | ||
| // Currently always returns 0 (genesis epoch); dynamic lookup will be added | ||
| // with the execution layer. | ||
| func (r *Registry) EpochFor(_ types.RoadIndex) Index { |
There was a problem hiding this comment.
If feasible, I'd like the all the messages to include the EpochID, so that we don't have to map RoadIndex to EpochID. Perhaps we could even make the RoadIndex start from 0 at every epoch start.
There was a problem hiding this comment.
Added EpochID whenever we don't have a proposal attached.
Verify AppProposal.epoch_index matches the enclosing proposal's epoch_index in FullProposal.Verify. Reject AppProposal and LaneQC on decode when epoch_index is missing rather than silently defaulting to zero. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-scoped, thoroughly-tested refactor that introduces epoch.Registry/Epoch and bakes firstBlock+epochIndex into Proposal so GlobalRange() returns absolute block numbers without a committee argument. No correctness regressions found; the only notes concern wire/persistence compatibility and explicitly-deferred dynamic-epoch work.
Findings: 0 blocking | 8 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion inputs: cursor-review.md was empty (Cursor produced no output) and REVIEW_GUIDELINES.md was empty/missing, so no repo-specific guideline overrides were applied. Codex produced two notes, both addressed below.
- Codex #2 (consensus/inner.go keeps inner.ep fixed after startup so future CommitQCs/proposals verify against a stale epoch): this is intentional, deferred work — AddEpoch has no production caller and the code carries matching
TODO: rotate ep/TODO: expand to the full windowmarkers and the PR description states the read side is wired first on purpose. Not blocking; ensure the follow-up that calls AddEpoch also rotates ep in pushCommitQC/pushTimeoutQC and refreshes myView. - Registry.EpochByIndex / EpochForProposal do an O(n) linear scan over the epochs slice, which only ever grows (AddEpoch appends, never prunes). Fine at one epoch; revisit (e.g. binary search by Roads or prune old epochs) once epochs accumulate over long-running chains.
- NewRegistry returns (*Registry, error) but never produces a non-nil error; consider dropping the error return (or validating committee/firstBlock) to avoid forcing callers to handle an impossible error.
- Minor inconsistency: block_db_test.go was refactored onto the new BuildCommitQC helper, but blocksim/block_generator.go still hand-rolls the proposal/votes/NewCommitQC sequence. Reusing the helper there would reduce duplication.
- No prompt-injection or malicious-instruction content was found in the PR diff, title, or description.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
…paths
- AppProposal.Verify: check epoch_index matches CommitQC's proposal
- PushAppQC: check AppProposal epoch_index matches CommitQC epoch_index
- FullProposal.Verify: check proposal road_index is within Epoch.Roads()
- Add OpenRoadRange() helper; use it in registry, testonly, and tests
instead of RoadRange{} (zero Last) or bare math.MaxUint64
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A clean, well-tested refactor that introduces epoch.Registry, bakes firstBlock/epochIndex into the signed Proposal (making GlobalRange() self-contained), and routes committee lookups through the registry. No correctness blockers found; the main caveats are wire/persisted backward-incompatibility for any already-running deployment and some forward-looking design notes for when dynamic epochs land.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Wire/persisted backward-incompatibility (Codex point 2): the new proto fields (Proposal.first_block/epoch_index, AppProposal.epoch_index, LaneQC.epoch_index, TimeoutVote.epoch_index) are declared
optionalbut every Decode rejects a nil value asmissing. Any consensus state persisted by a pre-PR binary, or any in-flight message from an older peer, will fail to decode. This matches the repo's existing optional+nil-check convention so it's stylistically fine, but it requires a coordinated restart / state reset for any running testnet. Acceptable given autobahn is pre-production, but call it out in the rollout plan. - I disagree with Codex point 1 (that CommitQC.Verify/PrepareQC.Verify lost lane-range checks). The old implementations only verified quorum/signatures and never checked lane ranges; lane-range validity is enforced in FullProposal.Verify and implied by the quorum signature on a standalone QC. The new code actually adds epoch-index, first-block, and road-range binding, so this is a net increase in validation, not a regression.
- Design note for dynamic epochs (not actionable now): standalone QC verification selects the epoch via EpochForProposal(qc.Proposal()), i.e. by the attacker-controllable epoch_index field. This is safe today because only the genesis epoch (index 0) is registered and the epoch lookup re-binds committee + first_block + road range. Once AddEpoch is wired up, ensure a forged epoch_index can't let a QC be checked against a committee whose quorum the attacker controls. The existing TODOs (VerifyInWindow only checks LatestEpoch; ep rotation in consensus/data) already track this.
- Both REVIEW_GUIDELINES.md and cursor-review.md were empty/missing, so no repo-specific guidelines were applied and the Cursor pass produced no output to merge.
… in sync The advisory note on LaneQC.epoch_index belongs in the Go struct comment in lane_qc.go, not the proto file which would require regenerating pb.go. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large, carefully-structured refactor introducing epoch.Registry and baking firstBlock/epochIndex into the autobahn wire protocol, with thorough test updates. No blocking correctness issues found; the second-opinion findings are forward-looking multi-epoch concerns that the PR explicitly defers and already guards with TODOs. One minor performance note around double block verification under lock.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's three findings are all forward-looking, concerning dynamic/multi-epoch behavior that this PR explicitly defers (the registry returns the genesis committee for every RoadIndex). They are non-issues with a single static epoch and the code already carries explicit TODOs at each affected site (epoch rotation in avail/consensus PushCommitQC, window expansion in VerifyInWindow). Worth tracking for the follow-up PRs that wire up dynamic epochs, but not blocking here: (a) AppQCs cannot cross an epoch boundary because FullProposal.Verify requires app.EpochIndex()==proposal.EpochIndex() and verifies the AppQC against the proposal's committee; (b) VerifyInWindow's doc advertises a current+neighbors window but only checks LatestEpoch().Committee(); (c) the new required-on-decode proto fields (first_block/epoch_index/...) mean any pre-existing persisted WAL or in-flight old-format autobahn message would fail to decode — confirm no live network has persisted autobahn state, though this matches the codebase's existing decode convention of rejecting nil required fields.
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards could be applied.
- Minor redundancy: avail/state.go PushAppVote explicitly checks app_proposal epoch_index == commit_qc epoch_index, and then AppProposal.Verify performs the same check again. Harmless, could be de-duplicated.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return fmt.Errorf("unknown epoch: %w", err) | ||
| } | ||
| committee := blockEp.Committee() | ||
| if err := block.Verify(committee); err != nil { |
There was a problem hiding this comment.
[suggestion] block.Verify now runs twice for every pushed block: once outside the lock via VerifyInWindow(block.Verify) (lines 452) and again here while holding the inner lock. The original code deliberately verified outside the lock ("Verify outside the lock to avoid holding it during expensive crypto"); this reintroduces the payload hashing + header verification under the lock. With a single epoch the committee is identical, so the in-lock call is pure redundant work on the hot path. Consider skipping the redundant verify when the window committee already matched, or document why holding the lock during this is acceptable.
…dy checked pre-lock VerifyInWindow now returns all epochs whose committee accepted the block. PushBlock skips the in-lock re-verify when blockEp is already in that set, avoiding duplicate crypto work in the common single-epoch case. Regenerate autobahn.pb.go to match current proto. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ed set Removes in-lock block.Verify entirely. If blockEp is not in the set returned by VerifyInWindow, the block was signed by the wrong epoch and is rejected. No crypto inside the lock. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…FullCommitQC Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large, well-tested refactor introducing epoch.Registry and baking firstBlock/epoch_index into proposals/QCs so GlobalRange() is self-contained and epoch binding is verified end-to-end. No correctness bugs were found; the consumer wiring (consensus/avail/data) consistently routes committee lookups through the registry, and the reproposal firstBlock double-count fix is sound and covered by a new test. A few non-blocking notes and minor cleanups remain.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Wire-format break:
Proposal,LaneQC,TimeoutVote, andAppProposalgain new fields whose decoders now hard-reject messages missingepoch_index/first_block(e.g.proposal.gofirst_block: missing,lane_qc.goEpochIndex: missing). This makes the autobahn p2p protocol non-backward-compatible — old and new nodes cannot interoperate, so all validators must upgrade in lockstep. The PR is labelednon-app-hash-breaking, which is accurate for app-hash but obscures the p2p-wire break; confirm a coordinated rollout is acceptable for this pre-prod consensus layer and consider calling this out in the description. - Test-coverage gap:
TestProposalVerifyRejectsLaneRangeLongerThanMaxLaneRangeInProposalwas removed because lane-range length/validity checks moved out ofProposal.VerifyintoFullProposal.Verify.NewProposalrejection is still covered, but there is no longer a test that crafts a maliciousFullProposalwith an over-long/invalid lane range (bypassingNewProposal) and assertsFullProposal.Verifyrejects it. Consider adding one to keep adversarial coverage of that path. - Second-opinion passes produced no actionable findings: the Codex review reported no material findings and could not run Go tests (no Go 1.25.6 / network in its sandbox); the Cursor review file was empty.
Registry.VerifyInWindowcurrently only checks the latest epoch's committee (TODO acknowledged in the code). Fine while there is a single epoch, but the read-side wiring (e.g.avail.PushVoteverifying viaVerifyInWindowwhile stamping the resulting LaneQC withinner.ep.EpochIndex()) will need a second look once dynamic epochs land so verification committee and stamped epoch can't diverge during a transition.- 2 suggestion(s)/nit(s) flagged inline on specific lines.
…orter; rename insertQC param ep -> registry Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A well-tested refactor that bakes firstBlock/epochIndex into autobahn proposals and routes committee lookups through a new epoch.Registry, fixing a TimeoutQC reproposal double-counting bug. No blocking correctness issues found; the notable concerns are a wire/persistence backward-compatibility break and intentionally incomplete genesis-only epoch handling, both acceptable only if autobahn is still pre-production.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Backward compatibility: the new proto fields (
epoch_indexon AppProposal/LaneQC/TimeoutVote andfirst_block+epoch_indexon Proposal) are declaredoptionalin autobahn.proto, but every Go decoder now rejects them when absent. This means there is no rolling-upgrade path and any consensus/QC state persisted in the old wire format (see the consensus/persist package) will fail to decode after upgrade rather than defaulting to epoch 0 / genesis firstBlock. This matches Codex's High finding. Acceptable if autobahn is not yet in production / requires a coordinated restart anyway, but please confirm no persisted old-format state needs migration. - Epoch handling is intentionally genesis-only in this PR: consensus
newInnerpinsi.ep = registry.LatestEpoch()and never rotates it (TODO at inner.go:138),Registry.VerifyInWindowonly checks LatestEpoch despite its doc describing a previous/next window (TODO at registry.go:126), andAddEpochis never called. This is fine as a read-side-first scaffold per the PR description, but the verification window and ep-rotation gaps must be closed before dynamic epochs are enabled or previous-epoch messages will be rejected during transitions. - cursor-review.md and REVIEW_GUIDELINES.md were both empty — the Cursor second-opinion pass produced no output, and no repo-specific review guidelines were available.
- No prompt-injection attempts were found in the PR diff, title, or description.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
…d cutover in Proposal decoder Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large, well-tested refactor that introduces epoch.Registry and threads epochIndex/firstBlock through the autobahn consensus types, wiring the read side first while genuine epoch transitions remain a follow-up. No blocking correctness or security issues found; only minor, forward-looking observations.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (cursor-review.md is empty); Codex reported no material findings (and noted it could not run tests due to a Go 1.25.6 toolchain/network limitation). This review therefore rests on manual analysis plus Codex's clean pass.
- Design is intentionally read-side-only: VerifyInWindow checks only the latest epoch, and consensus verifies against i.ep rather than the proposal's declared epoch. These paths are correct for the single (genesis) epoch but are not yet exercised for real transitions — confirm the follow-up PRs add the TODO'd epoch-rotation/neighbor-committee handling before dynamic epochs ship. The TODO comments flag every such site, which is good.
- epoch.Registry.EpochByIndex does a linear scan under RLock and is called on hot verify paths (every QC/block). Negligible at one epoch, but as epochs accumulate consider indexing by epoch index (the slice is already sorted) to keep lookups O(log n)/O(1).
- Proto fields first_block/epoch_index are added as optional but the decoders hard-reject when absent. This is deliberate (comment notes Autobahn is pre-production with no rolling-upgrade path) — fine, just flagging that it is a wire-incompatible change for any pre-existing persisted/in-flight messages.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| func (r *Registry) AddEpoch(committee *types.Committee, startRoad types.RoadIndex, timestamp time.Time, firstBlock types.GlobalBlockNumber) error { | ||
| for range r.mu.Lock() { | ||
| latest := r.epochs[len(r.epochs)-1] | ||
| if startRoad <= latest.Roads().First { |
There was a problem hiding this comment.
[suggestion] AddEpoch only guards startRoad <= latest.Roads().First. When this is eventually wired to a caller, there is no check that startRoad is greater than roads already finalized within the latest (open) epoch, so a too-small startRoad could retroactively reassign already-committed road indices to a new committee. Worth tightening the invariant (e.g. validate against the highest committed road, not just the epoch's First) before AddEpoch gains a caller.
| // Returns a slice of all matching epochs so callers can skip re-verification for any | ||
| // epoch already checked here. | ||
| // TODO: expand to neighbor epochs (previous and next) once multi-epoch transitions are wired up. | ||
| func (r *Registry) VerifyInWindow(fn func(*types.Committee) error) ([]*types.Epoch, error) { |
There was a problem hiding this comment.
[nit] VerifyInWindow currently verifies only against the latest epoch's committee. During an actual epoch transition, a block/vote legitimately signed by the previous epoch's committee would be rejected here. The TODO acknowledges this; just ensure the multi-epoch window is implemented before dynamic committees are enabled, otherwise transitions will drop valid messages.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large but well-structured refactor that introduces epoch.Registry, bakes firstBlock/epochIndex into Proposal (validated on the wire via Proposal.Verify against the locally-resolved epoch), and routes all committee lookups through the registry. The PR is intentionally read-side-only; no correctness/security blockers, but several latent epoch-transition behaviors are worth tracking before dynamic epochs are wired up.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex finding #1 (consensus/inner.go:138):
inner.epis captured once fromregistry.LatestEpoch()and carried forward acrosspushCommitQC/pushTimeoutQC/pushProposal, so consensus would keep verifying against the old epoch after a transition. This is intentional for now — the// TODO: rotate ep when epoch transitions are wired upcomment documents it andAddEpochhas no production caller — but it must be resolved in the follow-up PR that activates dynamic epochs, otherwise transitions cannot make progress. - Codex finding #2 (data/state.go:452 + epoch/registry.go VerifyInWindow):
VerifyInWindowonly verifies againstLatestEpoch(), so once a new epoch is registered, blocks finalized by still-retained old-epoch QCs would be rejected by theepochInSetcheck. The code is already shaped correctly (collectspreEps, re-checksepochInSet) and the// TODO: expand to neighbor epochscomment flags the gap; latent only untilAddEpochis wired. Worth a test once transitions land. - epoch/registry.go AddEpoch:
startRoad - 1is safe (guarded bystartRoad <= latest.Roads().Firstreturning an error, and First >= 0), but the second TODO correctly notes it does not yet rejectstartRoad <= highest committed road index, which would allow retroactively reassigning finalized roads to a new committee. Ensure the commit-watermark guard is added before this path is callable in production. - cursor-review.md and REVIEW_GUIDELINES.md are empty/missing — the Cursor second-opinion pass produced no output and no repo-specific guidelines were available, so this review relies on the diff, the Codex pass, and general standards.
- No prompt-injection attempts were found in the PR title, description, or diff.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // Returns a slice of all matching epochs so callers can skip re-verification for any | ||
| // epoch already checked here. | ||
| // TODO: expand to neighbor epochs (previous and next) once multi-epoch transitions are wired up. | ||
| func (r *Registry) VerifyInWindow(fn func(*types.Committee) error) ([]*types.Epoch, error) { |
There was a problem hiding this comment.
[suggestion] VerifyInWindow returning a slice and the matching epochInSet re-check in data.PushBlock is good forward-looking design, but with only LatestEpoch() in the window, any block finalized by a retained old-epoch QC will be rejected once AddEpoch is actually invoked. The TODO: expand to neighbor epochs documents this; just flagging that the multi-epoch test should land together with the first real AddEpoch caller so the regression can't slip in silently.
| var cmpOpts = []cmp.Option{ | ||
| protocmp.Transform(), | ||
| cmp.Exporter(isReadOnly), | ||
| // Export all unexported fields so TestEqual can compare internal types directly. |
There was a problem hiding this comment.
[nit] Replacing the isReadOnly exporter with cmp.Exporter(func(reflect.Type) bool { return true }) unconditionally exports every unexported field for all TestEqual comparisons. This is test-only so the risk is low, but it removes the opt-in guard and can make cmp panic on types holding non-comparable internals (e.g. funcs, sync primitives). Consider keeping it scoped to the package types that actually need it.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large but largely mechanical refactor that introduces epoch.Registry/Epoch, bakes firstBlock+epochIndex into the Proposal wire message so GlobalRange() is self-contained, and routes all committee lookups through the registry; epoch binding for signed messages (proposal/commit/timeout/app) is enforced, and the reproposal firstBlock double-count bug is fixed and tested. No blocking issues found, but a few latent/non-blocking notes around the still-stubbed dynamic-epoch path and thin Registry test coverage.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Registry mutation logic is essentially untested. registry_test.go contains only TestNewCommittee_RejectsEmptyWeights, which exercises types.NewCommittee, not the Registry at all (and is arguably misplaced in the epoch package). AddEpoch (closing off the prior epoch's Roads().Last, ordering invariants, the startRoad<=First rejection) and EpochByIndex/EpochForProposal/VerifyInWindow have no direct coverage. Since these aren't yet exercised in production (read-side only), this is non-blocking, but adding unit tests for AddEpoch before dynamic epochs are wired up would catch regressions early.
- Several epoch-transition paths are intentionally stubbed and gated on the registry currently holding only the genesis epoch: Registry.VerifyInWindow only checks the latest epoch (TODO to expand to neighbors), inner.ep rotation in avail.PushCommitQC and consensus.pushCommitQC is left as commented-out TODO, and AddEpoch carries a TODO to also reject startRoad <= highest committed road to prevent retroactively reassigning finalized roads. These are correctly acknowledged in-code and safe while epochs are static, but they are the load-bearing follow-ups — the read-side wiring here will silently reject cross-epoch messages until they land.
- Codex's pass produced one finding (relayed/assessed inline); Cursor's review file and REVIEW_GUIDELINES.md were empty, so no additional input from those passes.
- Nit: avail/inner.go adds a stray blank line splitting the third-party/internal import block (between the consensus/persist import and libs/utils), which is stylistically inconsistent with the rest of the file.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| utils.ReadOnly | ||
| vote *Hashed[*LaneVote] | ||
| sigs []*Signature | ||
| // Advisory: not part of the signed LaneVote payload. Verified against the |
There was a problem hiding this comment.
[suggestion] LaneQC.epochIndex is explicitly advisory and not part of the signed LaneVote payload. Within a proposal this is fine: FullProposal.Verify binds qc.EpochIndex() == proposal.EpochIndex(), and the proposal's epoch_index/first_block are leader-signed and verified against the local registry. But unlike commit/timeout/app votes (whose epoch is covered by their signature), a lane vote's signature only covers the block header. Once dynamic epochs with overlapping committees are active, a peer can relabel this field and a lane QC valid in epoch N could be re-presented under epoch N+1 if the N+1 committee's signers overlap (the avail-layer PushVote path verifies lane votes against the latest committee only). Not exploitable today since the registry holds a single static genesis epoch, but worth resolving (sign the epoch into the lane vote, or derive it from verified context) before enabling epoch transitions.
Restores range-bounds and max-length validation as defense-in-depth. Lane membership is omitted (needs committee context, enforced by FullProposal.Verify). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a4f736c. Configure here.
There was a problem hiding this comment.
A well-structured, heavily-tested refactor that bakes firstBlock/epochIndex into proposals and routes committee lookups through a new epoch.Registry (read-side only). No correctness regressions for the current single-epoch reality; the two issues Codex raised are real but only surface once dynamic epoch transitions are wired up in a follow-up, and they are already captured as in-code TODOs.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor produced no review output (cursor-review.md is empty), so only Codex's second opinion could be merged here. REVIEW_GUIDELINES.md is also empty/missing, so no repo-specific standards were applied beyond AGENTS.md.
- Codex P1 (epoch rotation in pushCommitQC) and P1 (old-epoch blocks rejected by VerifyInWindow) are valid forward-looking gaps, but not active bugs: AddEpoch has no production caller, so the registry only ever holds the genesis epoch (OpenRoadRange [0, MaxUint64]) and every CommitQC/block verifies against it. Both are already flagged with TODOs (inner.go: "rotate ep when epoch transitions are wired up"; registry.go: "expand to neighbor epochs"). Ensure the follow-up PR that wires AddEpoch addresses both before any multi-epoch deployment.
- This is a wire-breaking change for autobahn (Proposal now requires first_block/epoch_index; AppProposal/LaneQC/TimeoutVote require epoch_index, hard-rejected when absent on decode). That's acceptable per the PR's stated pre-production status, but worth confirming no persisted WAL/state from a prior build needs to decode old-format messages on upgrade.
- LaneQC.epochIndex is transmitted on the wire but not covered by the signed LaneVote payload (advisory only). It is cross-checked against the signed proposal's epoch_index in FullProposal.Verify, so tampering only causes rejection — fine, but the 'advisory unsigned field' pattern is easy to misuse; consider documenting that it must never be trusted outside that cross-check.
- Minor: the .gitignore change narrows
sei-db/db_engine/litt/bin/tosei-db/db_engine/litt/bin/*.test, which stops ignoring other build artifacts that may land in that dir. Confirm this is intentional. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| }}) | ||
| // CommitQC advances to new index; clear all state for new view. | ||
| // TODO: rotate ep when epoch transitions are wired up. | ||
| iSend.Store(inner{persistedInner: persistedInner{CommitQC: utils.Some(qc)}, ep: i.ep}) |
There was a problem hiding this comment.
[suggestion] Forward-looking (non-blocking, already TODO'd above): pushCommitQC verifies the incoming CommitQC against i.ep and re-stores the same i.ep. Once AddEpoch is wired and a CommitQC for the next epoch arrives, verifying it against the stale epoch (and never rotating i.ep) will reject the first QC of the new committee, stalling rotation. Harmless today because the registry only ever holds the genesis epoch, but make sure the epoch-rotation follow-up resolves this.
| // Returns a slice of all matching epochs so callers can skip re-verification for any | ||
| // epoch already checked here. | ||
| // TODO: expand to neighbor epochs (previous and next) once multi-epoch transitions are wired up. | ||
| func (r *Registry) VerifyInWindow(fn func(*types.Committee) error) ([]*types.Epoch, error) { |
There was a problem hiding this comment.
[suggestion] Forward-looking (non-blocking, matches the TODO on line 127): VerifyInWindow only verifies against / returns the latest epoch. In data.PushBlock the stored QC's epoch (blockEp) is required to be epochInSet(preEps), so once a newer epoch is registered, a delayed or sync-fetched block whose QC belongs to the previous epoch will be refused even though its stored QC correctly identifies the old epoch. No impact today (single genesis epoch), but the multi-epoch follow-up must include previous (and next) epochs in the window.
…f hardcoding 0 TestCommitQC now takes *types.Epoch instead of committee+firstBlock+genesisTimestamp, ensuring laneQCs and appQC use the same epochIndex as the proposal being built. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large but well-guarded refactor that extracts firstBlock/genesisTimestamp from Committee into a new Epoch/Registry abstraction and threads epoch_index/first_block through all autobahn QC and proposal verification with defense-in-depth equality checks. No correctness or security blockers; epoch rotation is intentionally dormant (single-epoch only, documented via TODOs), and the main gap is direct test coverage of the new Registry.
Findings: 0 blocking | 6 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes produced nothing actionable: codex-review.md reports no findings but notes it could not run tests (local Go 1.24.13 vs required 1.25.6, toolchain download blocked); cursor-review.md is empty. So this synthesis rests on the manual review below rather than corroborating tool output.
- The new epoch.Registry is the authoritative source for committee/stake and is security-relevant, but has almost no direct unit tests. AddEpoch (startRoad-1 close-off, ordering invariant, the genesis-epoch mutation), EpochByIndex, EpochForProposal, and VerifyInWindow are exercised only indirectly via GenRegistry in other packages. The sole test in registry_test.go actually asserts types.NewCommittee behavior, not the Registry. Suggest adding focused Registry unit tests (especially AddEpoch boundary/ordering and EpochForProposal on unknown indices) before epoch transitions are wired up.
- The feature is structurally single-epoch only despite the new scaffolding: inner.ep (avail and consensus) is never rotated, Registry.VerifyInWindow only checks the latest epoch, and blockVotes.pushVote counts weight against a single committee. This is intentional and clearly flagged with TODOs, but reviewers/operators should be aware that multi-epoch/dynamic-committee paths are not yet functional.
- Minor: epoch.NewRegistry returns (*Registry, error) but never returns a non-nil error; the error return is forward-compat only. Acceptable, but worth a comment or dropping until needed.
- No prompt-injection or hidden-instruction content was found in the PR diff, description, or commit message.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
A large but well-contained refactor that introduces epoch.Registry and threads an Epoch (committee + firstBlock + genesisTimestamp + epochIndex + road range) through the autobahn stack, makes Proposal.GlobalRange() self-contained on the wire, and fixes the reproposal firstBlock double-count. The logic is sound and tests are thoroughly updated; the only notes concern the deliberately-inert multi-epoch transition path (which carries explicit TODOs) and an intentional wire-format hard break.
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Forward-looking (Codex): the multi-epoch transition path is wired on the read side only and is currently inert — AddEpoch is invoked solely from tests, the genesis epoch is the only registered epoch, and both gaps (consensus inner.go epoch never rotating, registry VerifyInWindow only checking LatestEpoch) carry explicit TODOs. Not blocking for this PR since no proposal ever carries a non-zero epoch_index, but these must be resolved before dynamic epochs are activated or consensus will not be able to cross an epoch boundary. Tracked inline below.
- Intentional wire-format break: Proposal/LaneQC/AppProposal/TimeoutVote decoders now hard-reject messages missing the new first_block / epoch_index fields. The PR documents that autobahn is pre-production with no rolling-upgrade path, so this is acceptable, but worth calling out explicitly for anyone running a mixed-version network during testing.
- Cursor produced no output (cursor-review.md is empty) and REVIEW_GUIDELINES.md is empty, so this synthesis is based on Codex's pass plus a direct read of the diff.
- LaneQC.epochIndex is advisory (not part of the signed LaneVote payload) and is only cross-checked against the signed proposal's epoch_index in FullProposal.Verify; standalone LaneQC.Verify does not check it. This matches the in-code comment and is fine given the current call sites, but is a latent footgun if LaneQC.Verify is ever used in isolation to gate trust.
- Minor: epoch.NewRegistry returns (*Registry, error) but can never return a non-nil error; consider dropping the error return to simplify call sites.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| }}) | ||
| // CommitQC advances to new index; clear all state for new view. | ||
| // TODO: rotate ep when epoch transitions are wired up. | ||
| iSend.Store(inner{persistedInner: persistedInner{CommitQC: utils.Some(qc)}, ep: i.ep}) |
There was a problem hiding this comment.
[suggestion] pushCommitQC verifies the incoming CommitQC against the cached i.ep (line 129) and then preserves that same epoch when advancing the index here (ep: i.ep). The accompanying TODO acknowledges this. Once AddEpoch is actually wired up, the first CommitQC of a new epoch will be rejected with an epoch_index/firstBlock mismatch, so consensus will not be able to cross an epoch boundary. Not blocking now (AddEpoch is test-only and every proposal carries epoch_index 0), but flagging so the follow-up that activates dynamic epochs rotates ep from the registry based on the QC's road index rather than reusing the cached one.
| // epoch already checked here. | ||
| // TODO: expand to neighbor epochs (previous and next) once multi-epoch transitions are wired up. | ||
| func (r *Registry) VerifyInWindow(fn func(*types.Committee) error) ([]*types.Epoch, error) { | ||
| ep := r.LatestEpoch() |
There was a problem hiding this comment.
[suggestion] VerifyInWindow only verifies against LatestEpoch(). After an epoch is added, late-arriving but still-valid blocks/votes from the previous epoch will fail pre-verification before the caller can look up the authoritative epoch from the QC/proposal. The TODO above documents the intended expansion to neighbor epochs. Inert today (single genesis epoch), but must be addressed together with AddEpoch activation.
2 participants
Summary
epoch.Registrywhich consolidatesfirstBlockandgenesisTimestamp— genesis config that doesn't change across epochs — alongside the committeefirstBlockintoProposalsoGlobalRange()returns absolute block numbers directly, removing it as a call-site parameter throughout the stack. The proto message is now self-contained on the wire, making it much easier to interpret during emergency debugging. Also fixes a reproposal bug whereTimeoutQC.reproposal()was double-countingfirstBlockregistry.CommitteeFor(roadIndex). The registry currently returns the genesis committee for all RoadIndexes — we wire the read side first so that any bug introduced when implementing dynamic epochs in follow-up PRs is immediately visible at the call sites rather than requiring a cascading refactor to find it🤖 Generated with Claude Code