Complete multi-epoch ownership across Autobahn layers (CON-358) - #3929
Complete multi-epoch ownership across Autobahn layers (CON-358)#3929wen-coding wants to merge 12 commits into
Conversation
Publish the next-CommitQC epoch for observation (WaitLane / giga sit on it) and cover subscribe stay, leave, and rejoin with a mid-Recv stay apply. Co-authored-by: Cursor <cursoragent@cursor.com>
Use roads[first].epoch when retained CommitQCs exist, otherwise inner.epoch — no registry lookup or Option error path. Co-authored-by: Cursor <cursoragent@cursor.com>
Deletion follows the Anchor epoch: prune takes that epoch from the matching road (registry once at startup), and collectPersistBatch no longer wakes or drops on hasClosedLane. Co-authored-by: Cursor <cursoragent@cursor.com>
PruneAndPersist matches main again (BlockNumber, always create). runEvict only drops maps; runPersist SyncLanes is the sole runtime WAL deleter. Co-authored-by: Cursor <cursoragent@cursor.com>
Prefer the sealed road's epoch while it is retained; fall back to the registry once it is not, so empty-tip / data-ahead Anchors still prune instead of tearing down State.Run. Co-authored-by: Cursor <cursoragent@cursor.com>
Move lane-lifecycle notes onto State (keeping the original godoc) and TODO a real validation error for dual-epoch PushVote rejects. Co-authored-by: Cursor <cursoragent@cursor.com>
Epoch rotation was spread across the layers: consensus re-derived epochs from the registry, avail exposed a persisted-epoch peek only consensus used, and p2p read the registry directly. Each layer now owns one step and the next consumes its published value. - data seeds the registry, admits CommitQCs with the epoch they verified under, and publishes CommitEpoch for the latest admitted CommitQC - avail applies epochs behind the prune and execution leashes, recounts lane votes under the applied committee, and publishes types.ConsensusSpec - consensus installs the tips it is handed instead of consulting the registry - p2p EvmProxy reads data.CommitEpoch on both the validator and fullnode paths Co-authored-by: Cursor <cursoragent@cursor.com>
|
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.
Catch up applied epoch from durable seal/exec on restart, keep consensus tip when the avail spec lags, and remove ApplyEpoch so tests drive advances through the production sync path. Co-authored-by: Cursor <cursoragent@cursor.com>
Collect headers under the lock, then Sign after unlock so ed25519 does not serialize on the central avail mutex (especially on rejoin rewind). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c25237f. Configure here.
ConsensusSpec backleashes the view after LastRoad(applied), so the old N+2 placeholder lookahead is redundant with runEpochSync catchup. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid re-locking data.State on every EvmProxy call; AtomicRecv.Load is already lock-free once subscribed. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@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
1 participant
Summary
ConsensusSpecTest plan
go test ./sei-tendermint/autobahn/types/ ./sei-tendermint/internal/autobahn/... ./sei-tendermint/internal/p2p/ -count=1Made with Cursor