Complete multi-epoch ownership across Autobahn layers (CON-358) - #3929
Complete multi-epoch ownership across Autobahn layers (CON-358)#3929wen-coding wants to merge 34 commits into
Conversation
|
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}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Substantial and generally well-structured rework of multi-epoch ownership across data/avail/consensus, with good test coverage of the new registry, backleash, and vote-recount paths. One blocking issue: the consensus restore path can install an avail ConsensusSpec tip that is behind the persisted WAL tip, rolling the view backwards and discarding the anti-equivocation vote record; several hot-path locking regressions and a lost CommitQC verification also need attention.
Findings: 1 blocking | 12 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Registry placeholders (
makeEpoch) are stamped with the genesis committee, andloadFromBlockDBverifies retained CommitQCs against them. Once real rotation lands, a restart after a membership/weight change would fail QC verification at startup. The// TODO: replace placeholders with execution-derived epochsacknowledges this andActivateEpochstill has no production caller, so it is not reachable today — but it is worth capturing as a tracked follow-up rather than only a TODO, since the failure mode is "node will not start". (Codex raised this as P1; downgraded here because the rotation path is not wired.) - Related:
ActivateEpochoverwrites an existing placeholder at indexlatest+1, butdata.qcEntry.epoch,avail.road.epochandAnchor.Epochalready hold pointers to the placeholder. After a real rotation those stashed pointers and the registry entry for the same index would disagree. Worth an explicit invariant (e.g. rejectActivateEpochfor an index already handed out) before rotation is enabled. Registry.statemoved fromRWMutextoWatch, soEpochByIndex/EpochAt/LatestEpoch/FirstBlocknow take an exclusive lock.WaitForEpochneeds the update signal, but the pure reads could stay on a read path; consider keeping an RWMutex plus a separate notification channel if registry reads show up in profiles.RoadRange.IsLastRoadandGlobalRange.IsLastBlockare added but never used outsideepoch_test.go(IsLastBlockhas no test at all). Either wire them into the boundary checks that currently open-codeidx+1 == r.Next/p.Index() == LastRoad(...), or drop them.- No test covers restart with the durable avail tip sitting exactly on
LastRoad(M)— the case that triggers the ConsensusSpec walk-back inrefreshConsensusSpec. Given that path is the one that produces the view rollback flagged inline, it deserves a directnewInnertest. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
| return inner{}, err | ||
| } | ||
| logger.Info("restored consensus state", "state", innerProtoConv.Encode(&persisted)) | ||
| return inner{persistedInner: persisted, epoch: spec.Epoch}, nil |
There was a problem hiding this comment.
[suggestion] The persisted CommitQC is now never signature-verified on restore: persistedInner.validate dropped its cqc.Verify(ep) call, this branch returns persisted.CommitQC (not spec.CommitQC), and TestNewInnerCommitQCInvalidSignatureError was deleted. sameTip only compares indices, so a corrupt or tampered WAL entry at the right index is accepted and then used as justification for the node's next proposal.
Since the two are known to be at the same index and spec.CommitQC is the avail-verified one, installing it here keeps the check without re-verifying:
if sameTip {
if err := persisted.validate(spec.Epoch); err != nil { return inner{}, err }
persisted.CommitQC = spec.CommitQC // avail-verified
...
}Worth noting that TimeoutQC.Verify(viewEp, p.CommitQC) justifies against p.CommitQC, so an unverified value there weakens that check too.
| n := h.BlockNumber() | ||
| for inner, ctrl := range s.inner.Lock() { | ||
| applied := inner.epoch.Load() | ||
| if !laneProposalAccepted(applied, p) && |
There was a problem hiding this comment.
[suggestion] Block verification moved from outside the lock (the removed Registry().VerifyInWindow(...) call) to inside s.inner.Lock(). laneProposalAccepted runs block.Verify (which hashes the entire payload, including every tx) plus p.VerifySig, and on the || fallback it runs both a second time against anchorEpoch. That is now serialized on the single mutex that also guards PushVote, PushCommitQC, RecvBatch, headers, and the persist loop, for every inbound block from every peer — a meaningful throughput/latency regression and an easy remote stall vector.
The epochs needed for verification (applied, anchorEpoch) can be snapshotted under the lock and the verification done outside it, as PushCommitQC does with waitBackleash + qc.Verify.
Separately: verification failure now return nil (silent drop) where it previously returned fmt.Errorf("block.Verify(): %w", err), so a peer sending unverifiable blocks produces no error at all. The PushVote TODO about returning a meaningful validation error was narrowed to "accept future-epoch joiner votes" — the missing-error concern now applies to both paths and is worth keeping in a TODO.
| break | ||
| votes := make([]*types.Signed[*types.LaneVote], len(batch)) | ||
| for i, h := range batch { | ||
| votes[i] = types.Sign(r.state.key, types.NewLaneVote(h)) |
There was a problem hiding this comment.
[suggestion] Restructuring the loop to return from inside the Lock() iterator moved types.Sign under the avail inner mutex; previously the break exited the lock scope and signing happened outside it (the removed TODO(gprusak) sat exactly there).
This is worst on the new rewind path: after a localJoinGen bump, clear(r.next) makes the next batch cover every retained block across every lane (up to BlocksPerLane × lanes), so that many signature operations run back to back while holding the lock that gates all block/vote/QC ingest.
Keep the collection under the lock and the signing outside, e.g. assign batch to a variable declared before the Lock() loop and break out as before, or collect into a local and sign after the iterator ends.
| return nil, false | ||
| } | ||
|
|
||
| func (bv blockVotes) header(want types.BlockHeaderHash) (*types.BlockHeader, bool) { |
There was a problem hiding this comment.
[suggestion] header replaces what was an O(1) byHash[want] map lookup with a linear scan that recomputes h.Hash() (protobuf encode + hash) for every retained vote. headers() calls this once per block in a LaneRange, per lane, per road — so it is O(range × committee) header hashes, and it runs while holding the inner mutex.
The reason for the change is sound (byHash no longer holds zero-applied-weight votes, so it can miss headers), but the fix can keep the index: add a byHeaderHash map[BlockHeaderHash]*BlockHeader populated in pushVote unconditionally (independent of weight, so recount never touches it) and look up there.
| if anchorEpoch != nil { | ||
| // Tip epoch after prune: keep applied at least the anchor's verify-epoch. | ||
| if i.epoch.Load().EpochIndex() < anchorEpoch.EpochIndex() { | ||
| i.epoch.Store(anchorEpoch) |
There was a problem hiding this comment.
[suggestion] prune now writes i.epoch directly, which bypasses everything ApplyEpoch does for an epoch change: addLane for the new committee's lanes, recountVotes(ep), and the localJoinGen bump.
Concretely, if anchorEpoch is ahead of applied, after this store laneQC evaluates byHash.weight >= ep.Committee().LaneQuorum() against weights that were accumulated under the old committee, lanes introduced by the new epoch are absent (so PushBlock/PushVote silently drop for them), and a local rejoin is not observed by LaneVotesRecv.
If the intent is only to keep applied from lagging the anchor, routing through the same steps as ApplyEpoch (or calling a shared helper) would make the two paths agree by construction.
Also note this invalidates the comment on line 21: "ApplyEpoch is the sole writer after construction" is no longer true, since prune is also called from runEvict.
| return utils.None[*ethrpc.Client]() | ||
| } | ||
| validator := r.data.Registry().LatestEpoch().Committee().EvmShard(sender) | ||
| validator := r.data.CommitEpoch().Load().Committee().EvmShard(sender) |
There was a problem hiding this comment.
[suggestion] CommitEpoch() takes data.State's inner lock to hand back the subscription, so this acquires the central data-plane mutex on every EvmProxy call — i.e. on every routed EVM request. That lock is also held by QC/block insertion, PushAppHash, and the persist/evict loops.
CommitEpoch() returns a stable AtomicRecv whose Load() is lock-free, so capture it once when the router is constructed and store it on gigaValidatorRouter, then just Load() here. Same applies to giga_router_fullnode.go:68.
| epochIdx, | ||
| types.RoadRange{First: firstRoad, Next: FirstRoad(epochIdx + 1)}, | ||
| ep0.FirstTimestamp(), | ||
| ep0.Committee(), |
There was a problem hiding this comment.
[suggestion] Placeholders are built with ep0.Committee(), ep0.FirstTimestamp() and ep0.FirstBlock(), and loadFromBlockDB verifies retained CommitQCs against whatever EpochByIndex returns. That is correct only while every epoch has the genesis committee.
The TODO above ensureLocked(s, windowLast+1) covers the committee, but the timestamp and first-block fields are silently wrong too, and they feed ViewSpec.NextTimestamp()/NextGlobalBlock() for any view resolved against a placeholder. Worth widening the TODO to name all three fields, or having makeEpoch mark the epoch as a placeholder so a consumer that needs a real timestamp/first-block can fail loudly instead of reading genesis values.
|
@seidroid please re-review |
|
Claude finished @wen-coding's task in 5m 13s —— View job Re-review —
|
| Prior finding | Where it landed |
|---|---|
| [blocker] restore installs a spec tip behind the persisted WAL tip, rolling the view back and dropping the anti-equivocation vote record | restoreInner now branches on specViewIdx < persistedViewIdx and keeps the WAL tip + votes (consensus/inner.go:162-174). Covered by TestNewInner_KeepsPersistedTipWhenSpecBehind, which asserts both View().Index == last+1 and that the PrepareVote survives. |
Persisted CommitQC never signature-verified on restore |
newInner re-verifies against the QC's own certify epoch (consensus/inner.go:136-144). Since View.Verify checks both EpochIndex match and RoadRange.Has(Index), a tampered EpochIndex can't steer verification to a friendlier committee. TestNewInner_RejectsUnverifiedCommitQC pins it. |
prune writing i.epoch directly, bypassing addLane/recountVotes/localJoinGen |
prune now only sets anchorEpoch; installEpoch is genuinely the sole writer after construction (avail/inner.go:319-345), and the doc comment on line 26 is accurate again. |
types.Sign moved under the avail lock in RecvBatch |
Split into recvHeaders (under lock) + signing in RecvBatch (avail/subscriptions.go:77-117). |
Registry().LatestEpoch() taking the data lock on every EvmProxy call |
commitEpoch is captured once at router construction; EvmProxy is now a lock-free Load() on both routers. |
| Original Cursor finding — restart rewinds applied epoch at the boundary | newInner floors applied at the durable tip's verify-epoch and then re-drives via advanceEpochs, with AppQCCoversEpoch supplying the exec leash from data when avail's in-memory road AppQCs are empty. TestAdvanceEpochs_BoundaryTipUsesDataAppQC pins exactly that path. |
Also worth calling out: myView in newState is now seeded with the restored CommitQC/TimeoutQC instead of a bare ViewSpec{Epoch:} — that was a latent restart bug in its own right.
Non-blocking
1. runEpochSync stops republishing ConsensusSpec while parked in WaitForEpoch — avail/state.go:698-703 (new this round)
refreshConsensusSpec is documented as having exactly one live caller, and that caller can be parked in a wait that only registry updates can end:
if ei, ok := waitRegistry.Get(); ok {
if _, err := s.data.Registry().WaitForEpoch(ctx, ei); err != nil { return err }
continue
}waitRegistry is set when leashesMet && canAdvance == nil — i.e. all of epoch M's roads are admitted (roads.next >= FirstRoad(M+1)) and exec is satisfied, but M+1 isn't registered yet (AdvanceIfNeeded only fires when execution reaches LastRoad(M)). While parked there, markCommitQCsPersisted can advance persistedCommitQC several roads and bumpEpochWake has no listener, so consensusSpec stays frozen at the older durable tip and consensus sits out the tail views of epoch M.
Reachable when a node admits epoch M's CommitQCs from peers faster than its own WAL flush — the roads.next leash is satisfied by peer QCs, but the spec tracks the durable tip. It self-heals once execution registers M+1, so it's a participation gap rather than a deadlock. The fix is to not drop the epochWake subscription across the registry wait (re-check epochWake.Load() != seen before parking, or wait on both). Fix this →
2. Block verification is still inside the avail lock, and failures are still silent — avail/state.go:362-367 (carried over, unaddressed)
main ran this outside the lock; the PR moved it in:
for inner, ctrl := range s.inner.Lock() {
applied := inner.epoch.Load()
if !laneProposalAccepted(applied, p) && (… || !laneProposalAccepted(inner.anchorEpoch, p)) {
return nil
}laneProposalAccepted → LaneProposal.Verify → Block.Verify → Payload().Hash(), which protobuf-encodes and hashes the entire tx payload, plus VerifySig — and both run a second time on the anchorEpoch fallback. That is now serialized on the mutex that also gates PushVote, PushCommitQC, RecvBatch, headers, and the persist loop, for every inbound block from every peer. Snapshotting applied/anchorEpoch under the lock and verifying outside it (the shape PushCommitQC already uses with waitBackleash) keeps the semantics. Separately, the verification failure returns nil where main returned fmt.Errorf("block.Verify(): %w", err); the PushVote TODO that used to cover "return a meaningful validation error" was narrowed to joiner votes, so that concern is now unrecorded on both paths. Fix this →
3. blockVotes.header is an O(committee) scan that recomputes Hash() — avail/block_votes.go:75-83 (carried over, unaddressed)
The reason for dropping byHash[want] is sound (it no longer holds zero-applied-weight votes, so it can miss headers), but the index can be kept: a byHeaderHash map[BlockHeaderHash]*BlockHeader populated unconditionally in pushVote is weight-independent, so recount never has to touch it. As written, headers() does O(range × committee) header hashes per road, under the inner lock. Header hashing is far cheaper than block hashing, so this is a real but modest cost.
4. ActivateEpoch — Cursor's new finding is correct but latent, and the argument itself is unguarded — epoch/registry.go:124-144
ActivateEpoch overwrites s.m[next], which by then may be a placeholder already handed out by reference to data.qcEntry.epoch, avail.road.epoch, Anchor.Epoch, and avail's applied i.epoch. Those stashed pointers would then disagree with the registry entry for the same index, and since ApplyEpoch is gone there is no path that re-drives avail off the new committee. Cursor's report is accurate on mechanism. It is not reachable today — grep finds no production caller of ActivateEpoch — which is why I'm keeping it non-blocking, but it should be a tracked follow-up rather than only a TODO, because it must be closed before rotation is enabled.
While there: roads types.RoadRange is caller-supplied and never checked against {FirstRoad(next), FirstRoad(next+1)}. backleash uses IndexForRoad (pure arithmetic) while epochForRoad uses ep.RoadRange().Has(road), so a mismatched range silently splits those two views of the same road. Rejecting an index already handed out, and validating roads, are both one-line guards at the choke point every activation passes through.
5. Registry placeholders: the TODO under-describes the problem — epoch/registry.go:148-160
makeEpoch copies ep0.Committee(), ep0.FirstTimestamp() and ep0.FirstBlock(). The // TODO: replace placeholders with execution-derived epochs names only the committee, but the timestamp and first-block are wrong too and feed ViewSpec.NextTimestamp() / NextGlobalBlock() for any view resolved against a placeholder. Either widen the TODO to name all three, or mark the epoch as a placeholder so a consumer needing a real timestamp fails loudly instead of silently reading genesis.
6. Nits
RoadRange.IsLastRoadandGlobalRange.IsLastBlockare still unused outside tests (IsLastBlockhas no test at all), whileAdvanceIfNeededopen-codesroadIndex != LastRoad(tipEpoch). Wire them in or drop them.Registry.stateonWatchmeansEpochByIndex/EpochAt/LatestEpoch/FirstBlocktake an exclusive lock. OnlyWaitForEpochneeds the notification; anRWMutexplus a separate signal would keep the reads shared if these show up in profiles.giga/avail.go:127-132: whenstreamLaneProposalsOncereturnsnilbecause the server's epoch closed the lane but the locala.Lane(peer)still reports the same LaneID,closedis set toNoneandWaitForNextLanereturns immediately — a redial spin until the local epoch catches up. Pre-existing (the old code passedNoneunconditionally), and this change strictly narrows the window, so purely informational.
Verification notes
I could not run go test or go build in this environment — both were denied by the tool policy, so the findings above are from reading the diff rather than from a green run. The PR's own test plan (go test ./sei-tendermint/autobahn/types/ ./sei-tendermint/internal/autobahn/... ./sei-tendermint/internal/p2p/ -count=1) still needs to be confirmed against 7f8e261. To let me run these on future invocations, add Bash(go test:*) / Bash(go build:*) to --allowedTools.
· branch wen/autobahn_multi_epoch
|
@seidroid please re-review |
|
Claude finished @wen-coding's task in 7m 18s —— View job Re-review —
|
| Prior finding | Where it landed |
|---|---|
1. runEpochSync stops republishing ConsensusSpec while parked in WaitForEpoch |
markCommitQCsPersisted now calls refreshConsensusSpec (avail/state.go:791-796), and so does runEvict after prune. The doc comment names the reason. runEpochAdvance no longer owns spec publication at all. |
4. ActivateEpoch overwrites a placeholder already handed out by reference |
It now scans forward to the first vacant index and never touches a registered entry (epoch/registry.go:134-152). This also closes Cursor's "Stale applied epoch after ActivateEpoch" and "Stale epoch installed after wait" — the *types.Epoch that runEpochAdvance carries across WaitUntil(leashesMet) can no longer be replaced under it, so re-reading at install time is no longer needed. |
4b. roads types.RoadRange caller-supplied and unvalidated |
Parameter removed; the range is derived as {FirstRoad(next), FirstRoad(next+1)} at the choke point. |
| 5. Placeholder TODO named only the committee | Widened to committee + FirstTimestamp + FirstBlock (registry.go:74-75). |
6. IsLastRoad / IsLastBlock unused |
Both deleted along with their tests. |
Two more worth calling out that weren't asked for: ConsensusSpec.CommitQC is now a required *CommitQC with the option moved to the channel (Option[ConsensusSpec]), which removes the "spec present but empty" state that the old walk-back produced; and leashesMet's exec leash is now just anchorEpoch >= applied. I checked that the second one isn't a liveness regression: data.setAnchor anchors on first = NextAppQC-1 (data/state.go:845,902-910), i.e. the anchor tracks the latest AppQC'd block, not a deep retention watermark — so it fires on the same event the old appTipCovers || AppQCCoversEpoch pair did, with one source instead of three.
On Cursor's data/state_test.go finding (ProposalAt / "Broken fake CommitQC")
Not reproducible at HEAD. ProposalAt builds its Proposal with newProposal(..., ep.FirstBlock()) and a single one-block LaneRange, so GlobalRange().First == ep.FirstBlock() == inner.nextQC on a fresh state and PushQC's gr.First <= nextQC contiguity check passes. The test even documents this in the comment above the call (data/state_test.go:475-478). The QC is stamped at LastRoad(0) in road index while sitting at the first global block — which is exactly the point, since PushQC gates on global-block contiguity, not road contiguity.
Non-blocking
1. ActivateEpoch derives the next committee from a placeholder, not from LatestEpoch — epoch/registry.go:142-146 (Cursor's finding, and worse than reported)
next := s.latest + 1
for { if _, ok := s.m[next]; !ok { break }; next++ }
prev := s.m[next-1]
committee, err := prev.Committee().DeriveNext(weights, next)s.m[next-1] is by construction a makeEpoch placeholder — a genesis-committee copy — whenever next > s.latest+1. DeriveNext chains LaneID.Joined off prev.lanes, so every real activation after the first derives its lane IDs from genesis membership rather than from the last activated committee. Two consequences, and the second is the one Cursor didn't name:
- a validator that left in a prior activation and rejoins keeps its stale
Joined(Cursor's case); - a validator that joined in a prior activation and never left gets a brand-new
Joined = next, because it isn't in the genesis lane map. Its lane ID changes while its membership didn't, orphaning its retained blocks/votes and makingEpoch.IsCloseddrop the old lane inavail.prune.
The root tension is that "skip to the next vacant index" and "DeriveNext chains from the previous committee" can't both hold while placeholders occupy the gap. Either derive from s.m[s.latest] and accept that the intervening placeholders disagree, or refuse to activate when next != s.latest+1 and force the placeholder range to be resolved first. Not reachable today — ActivateEpoch still has no production caller — but it must be closed before rotation is enabled, so a tracked issue rather than a TODO. Fix this →
Related and smaller: because NewRegistry now seeds both 0 and 1, the first ActivateEpoch always lands at index ≥ 2. That's presumably intended ("epoch 1 is genesis-seeded and must not be overwritten") but it's an invariant the godoc doesn't state — worth one line, since it means no rotation can ever take effect before epoch 2.
2. Lane-vote cursor rewind on rejoin was removed with nothing replacing it — avail/subscriptions.go:76-100 (Cursor's ee565b4 finding — real, but narrower than described)
localJoinGen is gone, so LaneVotesRecv.next never rewinds. Cursor's mechanism holds only if a LaneVotesRecv outlives a leave/rejoin, and it can: serverStreamLaneVotes (p2p/giga/avail.go:59-71) creates one sub per client connection and loops forever, and the client side (clientStreamLaneVotes, line 172) is likewise not epoch-scoped — unlike clientStreamLaneProposals, which re-derives its lane through WaitForNextLane on every iteration. If the connection survives the membership gap, the cursor advances past blocks whose votes the receiving peer drops in PushVote (unverifiable under its applied/anchor committee), and reweightVotes can't recover them because they were never stored in byKey.
In practice the exposure is small — LaneQuorum() is f+1, and a peer that redials gets a fresh cursor starting at bq.first. But the commit message frames this as "rejoins continue from the vote cursor", and nothing in the code or tests establishes that the stream is actually torn down across a rejoin. Either tear the vote stream down on epoch change (mirroring WaitForNextLane on the proposal path) or state the redial assumption in LaneVotesRecv's doc, with a test that exercises RecvBatch while out of committee — the current joiner test avoids that path, which is precisely why it stays green.
3. Block/vote verification is still inside the avail lock, and failure is still silent — avail/state.go:349-353, 425-429, 452-464 (carried over, unaddressed)
The refactor into laneAcceptedUnder is a readability improvement, but the callback still runs LaneProposal.Verify → Block.Verify → Payload().Hash() (protobuf-encode + hash of the entire tx payload) plus VerifySig, and it does so from inside for inner, ctrl := range s.inner.Lock() — twice, when the anchor fallback fires. That is serialized on the one mutex that also gates PushVote, PushCommitQC, RecvBatch, headers, and collectPersistBatch, for every inbound block from every peer. PushCommitQC already demonstrates the shape that avoids it: resolve the epoch outside the lock (waitUntilApplied), verify outside, re-acquire to insert. Here that means snapshotting applied and anchorEpoch under the lock and calling accept outside it.
Separately, a verification failure is still return nil with no log and no error, and the only TODO in the area now reads "accept future-epoch joiner votes" — so "unverifiable block/vote is dropped silently" is unrecorded on both paths.
4. blockVotes.header is still an O(committee) scan that recomputes Hash() — avail/block_votes.go:70-78 (carried over)
headers() calls it once per block per lane per road, under the inner lock, and each call hashes every retained vote's header until it matches. The qc caching added this round is a nice win on the LaneQC side; the same trick applies here — a byHeaderHash map[BlockHeaderHash]*BlockHeader filled unconditionally in pushVote is weight-independent, so reweight never has to touch it. Header hashing is much cheaper than block hashing, so this stays modest.
5. Three overlapping guards for one invariant; the last one is now dead — consensus/state.go:142-146
if consTip := s.innerRecv.Load().View().Index; durableNext < consTip {ViewSpec.View() returns NextIndexOpt(vs.CommitQC) unconditionally for the index (types/proposal.go:159-166 — the TimeoutQC branch only changes Number), and newInner sets CommitQC to either spec.CommitQC or nothing. So consTip == specViewIdx, and newInner already rejected persistedViewIdx > specViewIdx and persistedViewIdx > durableNext before returning. This third check can never fire. Per the repo's own "guard at the choke point, never at each caller", newInner is that choke point — the durableNext parameter and this post-construction check can both go, leaving persistedViewIdx > specViewIdx as the single statement of the invariant.
6. SetupInitialEpochs does four ensure passes where one range would do — epoch/registry.go:61-79
ensureAround(s, span.Next) is fully subsumed: when span.Next is a boundary it ensures windowLast and windowLast+1 (both covered by the loop and the trailing ensureLocked); when it isn't, it ensures windowLast and windowLast-1, covered by the loop or by ensureAround(s, span.First). The function's actual contract is "register [windowFirst-1, windowLast+1]" — writing that as one loop would make the +1 the TODO refers to legible without tracing four calls. ensureAround then has no remaining caller, so it goes too.
7. Nits
newInner's restart catch-up loop (avail/inner.go:137-143) can installE+1as soon as the blockDB CommitQC window reaches epochE, whereas the live path gates onAdvanceIfNeededfiring atLastRoad(E)in execution. The two paths therefore admit different epochs at different times. Invisible today because placeholders are genesis copies; once activation is real, restart could apply a committee earlier than the live path would.runEpochAdvance'sgot+1 != nextcheck (avail/state.go:694-696) returns an error that tears down the whole avail scope. SincerunEpochAdvanceis the documented sole writer, this is an assertion on an impossible state — apanicwould read more honestly than a node-killing error return, or drop it.epochForRoad's third fallback (avail/inner.go:220-225) is a linear scan over roads on everyrefreshConsensusSpec, which now runs on every persist flush. Bounded and small, just noting it's on a hotter path than before.SubscribeLaneVotesswitched to an unkeyed struct literal (&LaneVotesRecv{s, map[...]{}},subscriptions.go:68) — keyed fields read better for a struct someone will extend.NewRegistryreturns anerrorit can never produce.
Verification notes
go build and go test were both denied by the tool policy in this environment, so everything above comes from reading the code, not from a green run. The PR's own test plan still needs confirming against 952aeca, and the two boundary tests the description leans on do exist and read correctly (TestRefreshConsensusSpec_WithholdsTipUntilNextViewEpochApplied, avail/inner_test.go:319; TestNextInstallableEpoch_BoundaryTipUsesDataAppQC, line 226). To let me run these next time, add Bash(go test:*) / Bash(go build:*) to --allowedTools.
· branch wen/autobahn_multi_epoch
952aeca to
b705b04
Compare
| if !laneAcceptedUnder(inner, func(ep *types.Epoch) bool { | ||
| return laneProposalAcceptedByEpoch(ep, p) | ||
| }) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
does this need to run within the lock? could we move it to outside the lock, since laneProposalAcceptedByEpoch seems quite heavy due to it's impl calling
return p.Msg().Verify(c) == nil && p.VerifySig(c) == nil
There was a problem hiding this comment.
split verification into two parts, the signature now verifies outside the lock.
| require.Equal(t, 3, len(bv.byKey)) | ||
|
|
||
| ep1 := types.NewEpoch(1, types.RoadRange{First: 10, Next: 20}, time.Time{}, | ||
| utils.OrPanic1(ep0.Committee().DeriveNext(map[types.PublicKey]uint64{ |
There was a problem hiding this comment.
should we test a reweighting of weights here for a single validator to test reweight logic?
This is what was suggested:
A useful concrete test arrangement would be:
- Invalidation case:
- Epoch 0: A=3, B=1, C=1, D=1; an A vote alone reaches lane quorum.
- Epoch 1: A=1, B=1, C=5, D=5; everyone stays, but A alone no longer reaches quorum.
- After reweight, bv.qc must be empty.
- Formation case:
- Store votes from A and B while their combined weight is insufficient.
- Increase their weights without changing membership.
- After reweight, the retained votes should form a QC.
This is non-blocking test coverage unless there is evidence the current implementation fails it.
| // Epoch holds the complete context for a single epoch. | ||
| // Retrieved from the local Registry; never transmitted on the wire. | ||
| type Epoch struct { | ||
| utils.ReadOnly |
There was a problem hiding this comment.
nit: ReadOnly is defined in testonly.go
There was a problem hiding this comment.
yeah, we should move it out of testonly.go . Readonly exist to mark struct as suitable for deep comparison in tests, but it is indeed placed on non-test structs.
There was a problem hiding this comment.
moved it to utils/readonly.go
|
|
||
| // VerifyLaneProposalPayloadAndSignature verifies payload hash and signature. It | ||
| // does not check committee membership. | ||
| func VerifyLaneProposalPayloadAndSignature(p *Signed[*LaneProposal]) error { |
There was a problem hiding this comment.
nit: with a name this verbose, one can as well just call VerifyPayload() and VerifySignature separately without loss of readability
There was a problem hiding this comment.
gone, reorganized Verify a bit.
| func (m *LaneProposal) Verify(c *Committee) error { | ||
| return m.block.Verify(c) | ||
| // VerifyPayload checks that the payload hashes to the header payload hash. | ||
| func (m *LaneProposal) VerifyPayload() error { |
There was a problem hiding this comment.
Please keep it as "Verify()", to have a way of checking full internal integrity. The fact that it boils down to payload consistency check is an implementation detail.
Excluding the membership check is ok imo, since lane no longer belongs to a single committee/epoch.
| // Key returns the key whish signed the message. | ||
| func (m *Signed[T]) Key() PublicKey { return m.sig.key } | ||
|
|
||
| // VerifySig verifies the signature of the message. |
There was a problem hiding this comment.
having both "VerifySig" vs "VerifySignature" is a poor naming scheme.
You can remove membership check from VerifySig.
| // that follows it. Avail publishes Option[ConsensusSpec] (None until a tip | ||
| // exists); consensus installs Some values verbatim. | ||
| type ConsensusSpec struct { | ||
| CommitQC *CommitQC |
There was a problem hiding this comment.
Option[CommitQC]? ConsensusSpec for index 0 won't have a CommitQC
| return v | ||
| } | ||
|
|
||
| // ConsensusSpec is the durable CommitQC tip paired with the epoch of the view |
| @@ -13,16 +13,17 @@ import ( | |||
| // inner holds roads and per-LaneID block/vote maps. | |||
| type inner struct { | |||
| persistedCommitQC utils.AtomicSend[utils.Option[*types.CommitQC]] // latest persisted CommitQC | |||
There was a problem hiding this comment.
consensusSpec replaces persistedCommitQC, no?
There was a problem hiding this comment.
also AFAICT consensusSpec should never be None. In particular consensus.NewState should be validating the consensusSpec against the loaded consensus state.
There was a problem hiding this comment.
also please consider if we shouldn't remove i.epoch in favor of consensusSpec.Epoch: afaict it should not slow down consensus at the epoch border, since previous CommitQC has to be already persisted before next consensus instance can be started.
There was a problem hiding this comment.
consensusSpec is normally the same as persistedCommitQC, but with Backleash when PushCommitQC waits on applied, they can diverge.
Changed consensusSpec to never be None
i.epoch removed.
There was a problem hiding this comment.
idea: then what if we merge runPersist and runEpochAdvance, so that they do not diverge?
There was a problem hiding this comment.
You mean we should skip persisting while backleash is false?I think per current code this may cause deadlock, because canAdvanceEpoch requires a durable LastRoad tip. Am I understanding you correctly?
There was a problem hiding this comment.
yes, basically advanceEpoch would be then driven by runPersist and runEvict which would atomically advance epoch together with bumping the consensusSpec.
| } | ||
| i.nextBlockToPersist[lane] = q.next | ||
| } | ||
| // Restart catch-up: install every epoch the durable leashes already allow. |
There was a problem hiding this comment.
why jumping to the right epoch is not supported?
There was a problem hiding this comment.
Right now we jumps first to anchor then check whether for each epoch the backleash is satisfied. Since we have the backleash, in normal cases there should only be two epochs. So it looks scary as a loop, but in reality it should be one or two entries.
Persist only the tip road index for ErrAvailBehindConsensus; runtime tip and epoch come from ConsensusSpec on inner.spec, matching the multi-epoch review. Co-authored-by: Cursor <cursoragent@cursor.com>
Stop passing ep into reweightVotes; installEpoch stores then reweights under i.epoch, and document that laneQC / i.epoch stay distinct from the withheld consensusSpec.Epoch. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace ConsensusSpec "view" with RoadIndex, drop verify-epoch phrasing, and document that anchorEpoch may lead applied while prune does not advance i.epoch. Co-authored-by: Cursor <cursoragent@cursor.com>
Rename install/leashesMet/waitUntilApplied to advanceEpoch, canAdvanceEpoch, and waitUntilAdvanced so epoch verbs match runEpochAdvance and pushSpec. Co-authored-by: Cursor <cursoragent@cursor.com>
insertBlock compares the FullCommitQC header by value instead of hash. PushVote picks applied/Anchor membership first, then Verify on that committee. Co-authored-by: Cursor <cursoragent@cursor.com>
Fold the one-caller epoch lookup into refreshConsensusSpec and record the AvailSpec idea so avail can leave the registry in a later PR. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop LatestEpoch and derive ActivateEpoch from an explicit parent so prune cannot dangle a cursor. Lookups return ErrPruned for dropped indices; PushQC treats a pruned-epoch QC as stale. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Drop twins of the idle-boundary and SetupInitialEpochs cases, and share one RunEpochAdvance leash fixture for registry, AppQC, and parked next-epoch QC. Co-authored-by: Cursor <cursoragent@cursor.com>
Stop documenting payload hash and membership in the godoc; those are implementation and caller concerns. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop a separate applied watch. Seal waits until the last CommitQC is durable so advance never publishes a stale tip with the next epoch. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the admit-stash wording; callers only need what the field is. Co-authored-by: Cursor <cursoragent@cursor.com>
seedApplied already jumps to the restored QC, so construction only needs one more advance. Drop the redundant HasReplica check in FullProposal.Verify. Co-authored-by: Cursor <cursoragent@cursor.com>
Fold waitForEpoch into PushCommitQC's WaitUntil. Proposals require the applied committee; votes still fall back to a lagging Anchor. Co-authored-by: Cursor <cursoragent@cursor.com>
Consensus loads ConsensusSpec once at construction, so a LastRoad WAL tip must already be paired with the next registry epoch. Do that hop in restorePlan instead of seeding genesis on inner and patching afterward. Co-authored-by: Cursor <cursoragent@cursor.com>
Align persistedInner with ConsensusSpec.Index, embed ViewSpec, and reject WAL votes not signed by this node. Co-authored-by: Cursor <cursoragent@cursor.com>
Track supported epochs as [First, Next), wait for nextQC before looking up a PushQC epoch, and mark DriveAdvance as a test helper. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the generated file in sync with autobahn.proto so UCI lint's generated-files check passes. Co-authored-by: Cursor <cursoragent@cursor.com>
When the Anchor or registry floor skips ahead of avail, prune and runEpochAdvance install the live epoch rather than treating ErrPruned or a mismatched applied cursor as fatal. Co-authored-by: Cursor <cursoragent@cursor.com>
Fold overlapping restore and reweight tests into tables and drop cases that were strict subsets of remaining checks. Co-authored-by: Cursor <cursoragent@cursor.com>
Prune only tipcuts. A skipped-ahead Anchor jumps applied to the epoch of the durable tip's following road so ConsensusSpec never pairs LastRoad(N) with epoch N. Co-authored-by: Cursor <cursoragent@cursor.com>
Fold ErrPruned catch-up and following-road skip into a single WaitUntil and install, and drop the extra helper. Co-authored-by: Cursor <cursoragent@cursor.com>
…via EpochAt Co-authored-by: Cursor <cursoragent@cursor.com>
Wait on inner first, then WaitForEpoch, then install. next is advanceTarget so catch-up jumps to the tip's following epoch instead of deadlocking on a carried request. Compare pushSpec against spec.Index() and rename the WAL proto field to index. Co-authored-by: Cursor <cursoragent@cursor.com>
A persist batch collected before prune jumps an empty queue to the Anchor can finish later; ignore those QCs so ConsensusSpec cannot move backward. Co-authored-by: Cursor <cursoragent@cursor.com>
Main's BlockStore split requires blockstore.New around BlockDB; two tests missed the wrap during the replay. Co-authored-by: Cursor <cursoragent@cursor.com>
89707e6 to
b69f855
Compare
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 b69f855. Configure here.
Rename newTestBlockDB/writeToBlockDB to the BlockStore wrappers so tests talk to the store interface instead of constructing BlockDB directly. Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
Summary
Completes multi-epoch ownership for Autobahn (CON-358): each layer owns one epoch step and publishes what the next layer consumes.
NewRegistryseeds 0 and 1 (genesis committee). Live window is[First, Next)(EpochRange); epoch 0 is always kept.SetupInitialEpochsfills placeholders over retained CommitQC history;AdvanceIfNeededseeds only M+1 atLastRoad(M);PruneBeforedrops supported epochs below the data Anchor;ActivateEpochnever overwrites an already-registered index.nextQCis a block cursor;nextCommitRoadis the road after the latest CommitQC). PublishesNextCommitEpoch(road after that tip, or the previous value until the epoch is registered).PushAppHashdrivesAdvanceIfNeededatLastRoad.PushQCwaitsgr.First <= nextQCbefore epoch lookup.runEpochAdvance), one epoch per wake behind seal + Anchor (prune) leashes and registry presence. Construction (restorePlan/nextViewEpoch) may hop to the next-view epoch of the persisted tip. Catch-up:pruneinstallsanchor.Epochwhen it is ahead of applied;WaitForEpochErrPrunedjumps toLive().First; if prune already passednextduring the seal wait, advance continues instead of killingRun. Recounts lane votes under applied; publishesConsensusSpec.Load()of Spec atnewInner(durable CommitQC tip + epoch of the following road). WALIndexis the current view (NextIndexOptof the tip; proto field 9 stillcommit_qc_index). Same-view votes/QCs are kept when tips match; local votes must be signed by this node. WAL view ahead of Spec →ErrAvailBehindConsensus.EvmProxyshards fromdata.NextCommitEpoch(recv handle cached at construction).ConsensusSpec withhold (restart safety)
At an epoch boundary the durable tip sits on
LastRoad(E)while applied is stillE. Spec is withheld (previous Spec stands) until the next-view epoch is applied — never publishes a predecessor tip. Walking tip back would roll consensus view backward and discard that view's votes.persistedCommitQCcan still sit onLastRoad(E)while Spec withholds;markCommitQCsPersistedrefreshes the spec tip whilerunEpochAdvanceis parked. Mid-epoch persist and epoch advance stay separate loops.Three epoch concepts
epoch.Registrydata.Stateavail.StateViewSpecembedsConsensusSpecplusTimeoutQC.Test plan
go test -race -count=1 ./sei-tendermint/autobahn/types/ ./sei-tendermint/internal/autobahn/... ./sei-tendermint/internal/p2p/ -timeout 20mLastRoad(0)after avail catch-up installs epoch 1 and republishes SpecActivateEpochinto epoch 2+ (epoch 1 is genesis-seeded and must not be overwritten)Runstays up, applied jumps to the live floorKnown follow-ups
ActivateEpochhas no production caller yet — real committee weights still land via tests / future wiring; placeholders remain genesis copies (registry.goTODO)avail/state.goTODO)LastRoad(sopersistedCommitQCand Spec tip never diverge) is not in this PR