feat(autobahn): identify lanes as (validator, e_join) (CON-358) - #3862
feat(autobahn): identify lanes as (validator, e_join) (CON-358)#3862wen-coding wants to merge 6 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
LaneID is no longer a pubkey alias: stay keeps e_join, leave is final for that LaneID, and rejoin allocates a new identity. Avail ApplyEpoch seeds joiners and defers leaver WAL prune until a durable CommitQC in the new epoch; producer and lane-proposal subscribe follow LocalLane streaks (stay quiet, leave cancels). Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
8d254d4 to
4f52a00
Compare
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Allow create when the batch has proposals for an inactive lane so a leave that races the first persist does not silently drop tips. Truncate-only calls after DeleteLane still skip recreation. Co-authored-by: Cursor <[email protected]>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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.
The LaneID = (validator, e_join) refactor is coherent and well-tested at the type/proto layer, but the new lane-deletion path (pruneInactiveLanes + DeleteLane) breaks the "lanes are only added, never removed" invariant that three unguarded map reads in avail still rely on, and the leave/rejoin handling in SubscribeLaneProposals and ProduceLocalBlock has races that panic or permanently kill the producer. These are latent until epoch transitions are wired up (#3736), but they are defects in this PR's own feature and are not covered by the added tests.
Findings: 6 blocking | 13 non-blocking | 9 posted inline
Blockers
pruneInactiveLanesdeletes entries frominner.blocks/inner.votes, but three call sites still index those maps without anokcheck and will nil-deref once a leaver is pruned:avail/state.go:654(headers:q := inner.votes[lr.Lane()]thenq.first),avail/state.go:815(PushQC loop:inner.blocks[lr.Lane()].q[n]), andavail/inner.go:189(laneQC:i.votes[lane].q[n]). All three iterate the committee of the QC's epoch, which can be an older epoch that still contains the leaver. The comment removed frompersist/blocks.go("lanes are only added, never removed") was load-bearing for these too — every reader needs an ok-check (or lanes must be retained until the prune anchor passes them).- No test covers the interaction between
tryPruneLeaveLanesand a lagging reader.TestApplyEpoch_AddsJoinerDefersLeaverUntilCommitQCWatermarkverifies the leaver's maps/WAL disappear, but nothing exercisesheaders()/fullCommitQC/ thes.data.PushQCloop against a previous-epoch CommitQC after the prune, which is exactly the crash path. Please add one. - Prune watermark choice needs justification: normal block retention is gated on the durable prune anchor (AppQC-derived,
advancePersistedBlockStart), but a leaver's in-memory queues and WAL are dropped as soon as any durable CommitQC lands in the new epoch. Blocks that are committed but not yet executed/served can still be needed at that point. Either reuse the prune-anchor watermark or document why CommitQC-epoch is sufficient. - 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor review file (
cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings. - Codex P1 #1 (legacy block WALs / wire format): the lane WAL directory name changes from
hex(pubkey)(32B) tohex(pubkey||e_join)(40B), andBlockHeader.lane/LaneRange.lanechange proto type fromPublicKeytoLaneIDon the same field number. Both are hard breaks — existing WAL dirs are silently skipped on restart, and peers on the old binary cannot decode. SinceApplyEpoch/ActivateEpochhave no production callers yet, this is presumably pre-launch and acceptable; please confirm explicitly in the PR description rather than leaving it implicit. ApplyEpochnever returns a non-nil error. Either drop the return value or note that it is reserved for the follow-up wiring.tryPruneLeaveLanesre-Stores the identicallatestCommitQCvalue after the disk delete purely to wake waiters. It is safe today only becausemarkCommitQCsPersistedandtryPruneLeaveLanesare both on therunPersistgoroutine — worth stating that in the comment, since a concurrent writer would make this a watermark regression.ctrl.Updated()alone may be enough.markBlockPersistedwritesinner.nextBlockToPersist[lane] = nextunconditionally, so a pruned lane can be resurrected as a stale map entry (small leak, and it makes the map key sets diverge fromblocks/votes).LaneProposalsRecv.Recvallocates an errgroup and two goroutines per block received, plus a freshLocalLaneUpdates()subscription per iteration. On the hot proposal path this is meaningful churn; consider hoisting the lane-change watcher out of the per-block loop.alignMempoolForLanereadsNextBlock(lane)before taking the mempool lock, so the tip can be stale by the time it is applied; and a rejoin silently discards all bufferedevmTxs/evmNonces. Both are probably intended, but neither is documented.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| if !ok || qc.Proposal().EpochIndex() < inner.epoch.EpochIndex() { | ||
| return nil | ||
| } | ||
| pruned = inner.pruneInactiveLanes(inner.epoch.Committee()) |
There was a problem hiding this comment.
[blocker] This is the first place a lane is ever removed from inner.blocks/inner.votes, which invalidates an invariant three existing readers still assume:
state.go:654—headers()doesq := inner.votes[lr.Lane()]with no ok-check, then dereferencesq.first.state.go:815— thes.data.PushQCloop doesinner.blocks[lr.Lane()].q[n].inner.go:189—laneQC()doesi.votes[lane].q[n].
All three iterate the committee of the QC's epoch (EpochByIndex(qc.Proposal().EpochIndex())), not the current one. Concrete failure: node is in epoch E+1, a CommitQC for E+1 is durable so this prunes leaver lane L; the PushQC goroutine is still catching up at a road index whose QC is in epoch E, whose committee still contains L → inner.blocks[L] is nil → nil-pointer panic in a long-lived background goroutine (node crash). Same shape reachable via fullCommitQC/headers when serving StreamFullCommitQCs.
Either add ok-checks at those three sites, or keep leaver lanes until the prune anchor has advanced past their last committed range.
| return ErrLaneIdentityChanged | ||
| } | ||
| if got != r.lane { | ||
| panic(fmt.Sprintf( |
There was a problem hiding this comment.
[blocker] This panic is reachable. utils.atomicWatch (libs/utils/mutex.go:113) only ever exposes the latest value — Wait re-reads w.ptr.Load() after waking, so intermediate values are coalesced. A leave (Store(None)) immediately followed by a rejoin (Store(Some(v,e2))) collapses into a single observable transition: the waiter's predicate !ok || got != r.lane fires, checkBound(s.LocalLane()) sees Some(v,e2) != r.lane, and this panics. The same applies between SubscribeLaneProposals's LocalLane() read and the first checkBound in Recv.
Note that producer/lifecycle_test.go works around exactly this by explicitly Waiting for None before applying the rejoin epoch — production has no such barrier, and serverStreamLaneProposals runs this inside a p2p server goroutine, so a leave+rejoin in quick succession crashes the node.
Return ErrLaneIdentityChanged here instead of panicking; it is the same recovery action the caller already takes for leave.
| lane := key.Public() | ||
| var result *types.Signed[*types.LaneProposal] | ||
| for inner, ctrl := range s.inner.Lock() { | ||
| lane, ok := inner.epoch.Committee().Lane(key.Public()).Get() |
There was a problem hiding this comment.
[blocker] Re-deriving the lane from the current committee here means produceLocalBlock ignores which lane streak the caller was actually producing for. ApplyEpoch swaps inner.epoch and installs the joiner queues (state.go:79-83) before localLane.Store (state.go:84-86), and the old streak's ctx cancellation is asynchronous even after the Store — so an in-flight ProduceLocalBlock(toProduce) from the pre-rejoin streak can resolve to the new (v, e_join) lane.
Two outcomes, both bad:
toProduce > 0:q.next != n→"unexpected block number". That is notErrBadLane, sostreakOpErr(producer/state.go:204) does not map it tocontext.Canceled— the error escapesIterand killsproducer.Runpermanently.toProduce == 0(left and rejoined before producing anything): the old streak's payload is silently accepted as block 0 of the new lane.
Take the bound lane as a parameter and verify inner.epoch.Committee().HasLane(lane) under the same inner lock, returning ErrBadLane on mismatch. (Codex raised the same race.)
| // Iter continues (rejoin) instead of permanently killing producer.Run. | ||
| // Covers both: LocalLane already Store'd, and maps/epoch swapped before Store. | ||
| func (s *State) streakOpErr(lane types.LaneID, op string, err error) error { | ||
| if errors.Is(err, avail.ErrBadLane) { |
There was a problem hiding this comment.
[suggestion] This only rescues ErrBadLane. The rejoin race described on avail/state.go:755 surfaces as "unexpected block number: got N, want 0", which falls through to the fmt.Errorf below and permanently tears down producer.Run. Once ProduceLocalBlock takes the bound lane and returns ErrBadLane on mismatch this is fixed; until then this guard is incomplete.
| // Lanes is the list of nodes which are eligible to produce blocks. | ||
| func (c *Committee) Lanes() ImSlice[LaneID] { return c.replicas } | ||
| // Lane returns the LaneID for validator v in this committee, if present. | ||
| func (c *Committee) Lane(v PublicKey) utils.Option[LaneID] { |
There was a problem hiding this comment.
[suggestion] HasLane regresses from an O(1) map lookup to O(n) via this linear scan. It is called per lane range in Proposal.Verify (proposal.go:53) and per header in BlockHeader.Verify (block.go:73), so proposal verification becomes O(n²) PublicKey comparisons — up to 10k 32-byte compares per proposal at MaxValidators = 100, on the consensus hot path. Storing map[PublicKey]LaneID alongside lanes restores O(1) for both Lane and HasLane at negligible cost.
| lanes := make([]LaneID, 0, len(weights)) | ||
| for v := range weights { | ||
| eJoin := e | ||
| if prevLane, ok := prev.Lane(v).Get(); ok { |
There was a problem hiding this comment.
[nit] prev is dereferenced without a nil check even though the doc only says "prev is required". A nil prev here is a nil-map read inside Lane → panic rather than an error. Cheap to guard: if prev == nil { return nil, errors.New(...) }.
|
|
||
| // LaneIDFromBytes parses Bytes() encoding. | ||
| func LaneIDFromBytes(b []byte) (LaneID, error) { | ||
| if len(b) < 8 { |
There was a problem hiding this comment.
[nit] len(b) < 8 is looser than the encoding actually permits — Bytes() always produces exactly len(pubkey)+8. Today a shorter/longer input is caught downstream by PublicKeyFromBytes, but an explicit exact-length check makes the WAL-directory parse in NewBlockPersister unambiguous and self-documenting.
| continue | ||
| } | ||
| lane, err := types.PublicKeyFromBytes(laneBytes) | ||
| lane, err := types.LaneIDFromBytes(laneBytes) |
There was a problem hiding this comment.
[suggestion] Codex flagged this: lane directories previously used hex(pubkey) (64 hex chars); LaneIDFromBytes now requires hex(pubkey||e_join) (80 chars), so every pre-existing lane directory decodes to a 24-byte "pubkey", fails, and is skipped with only a Warn — all previously durable lane blocks are dropped on the first restart after upgrade. Given ApplyEpoch/ActivateEpoch have no production callers yet this is presumably fine pre-launch, but it should be stated in the PR description alongside the BlockHeader.lane proto type change (which breaks the wire format for peers on the old binary too).
| if !laneIDOptEqual(s.localLane.Load(), newLane) { | ||
| s.localLane.Store(newLane) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
[nit] ApplyEpoch can never fail — every path returns nil. Either drop the error return or add a comment that it is reserved for the #3736 advanceEpoch wiring, so callers don't add dead error handling.
Prune leavers on the AppQC floor, bind ProduceLocalBlock to the streak LaneID, soft-fail identity coalesces and missing maps, and keep giga peer RPC up across leave/rejoin. Also O(1) committee Lane lookup and exact LaneID byte length. Co-authored-by: Cursor <[email protected]>
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 b3a591f. Configure here.
There was a problem hiding this comment.
Solid, well-documented reshaping of LaneID into (validator, e_join) with good test coverage of the stay/leave/rejoin state machine, but two correctness problems in the persistence/pruning paths are blocking: restored leave-lane queues are never positioned at the prune anchor (restart fails), and the leave-lane retention watermark is keyed on e_join rather than the leave epoch, so long-tenured leavers are dropped before their final tips are committed. Cursor's second-opinion pass produced no output; Codex's two findings are both confirmed and included.
Findings: 3 blocking | 12 non-blocking | 7 posted inline
Blockers
avail: no test covers the restart path that actually breaks — a persisted leave-lane WAL whose surviving blocks start above 0 (i.e. a prune anchor with a non-emptyLaneRangefor the leaver).TestApplyEpoch_AddsJoinerDefersLeaverUntilAppQCWatermarkandTestTryPruneLeaveLanes_OrphanWALWithoutMapsboth persist laneB at block 0 with no anchor, so they pass over the bug innewInner. Please add a restart test with an anchor whose leaverLaneRange.First() > 0.- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's review file (
cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings. - Epoch-boundary peer teardown for remote lanes is unhandled.
Registry.VerifyInWindowstill only checks the latest committee (existing TODO), andPushBlock/PushVotenow returnErrBadLanewheninner.blocks/inner.voteslacks the lane. A peer that applies epoch N+1 slightly before us and pushes a proposal on its new rejoin LaneID makesclientStreamLaneProposalsreturn an error (giga/avail.go:194), tearing down that peer's RPC stream. The PR carefully protects the local lane viaErrLaneIdentityChanged; remote lanes need the equivalent soft-failure (or a widened verification window) beforeApplyEpochis wired up. ApplyEpochswapsinner.epochwholesale, soinner.laneQCandPushVoteimmediately evaluate already-accumulated old-epoch votes against the new committee'sLaneQuorum()and weights. Votes from departed validators are still in the queues and would be assembled into aLaneQCthat fails verification against the new committee. The existing// TODO: filter votes per-epoch committeebecomes load-bearing onceApplyEpochis called in production — worth stating explicitly in the #3736 follow-up.- The
BlockHeader.lane/LaneRange.laneproto type change (PublicKey→LaneID) alters block header hashes and makes previously persisted WAL entries undecodable, so any node with existing autobahn state needs a coordinated state reset. Autobahn is opt-in viaautobahn-config-file, so this is likely fine, but the PR description doesn't mention it and there's no migration note. producer.mempoolFirst()(producer/mempool.go:83) is now dead —alignMempoolForLanereplaced its only caller.unusedisn't enabled in.golangci.ymlso it won't fail lint, but it should be removed.types.GenCommittee(testonly.go) now bypasses the public constructors and callsnormalizeWeights+finalizeCommitteedirectly to inject randome_joinvalues. Randomizinge_joinin tests is a genuine improvement (it catches code assuminge_join == 0), but duplicating the constructor body in test-only code means future changes toNewCommitteewon't be reflected. Consider anewCommitteeWithEJoins(weights, func(PublicKey) EpochIndex)helper shared by all three.State.ApplyEpochalways returnsnil. The comment explains this is retained for #3736, which is reasonable, but every current caller has to handle an error that cannot occur.- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| if anchor, aok := l.pruneAnchor.Get(); aok { | ||
| first = anchor.CommitQC.LaneRange(lane).First() | ||
| } | ||
| i.persistedBlockStart[lane] = first |
There was a problem hiding this comment.
[blocker] The re-attached leave-lane queue is never positioned at the prune anchor, so a restart with any non-trivially-pruned leaver fails.
i.prune(...) at line 95 runs before this loop and only walks i.votes, which at that point contains committee lanes only — so the newly created queues here keep first = next = 0. But loadPersistedState (state.go:284-293) has already dropped every block below anchor.CommitQC.LaneRange(lane).First() for all lanes including leavers. So when the leaver produced blocks and the anchor moved partway through them, bs[0].Number == first > 0 while q.next == 0, and the contiguity check at line 145 returns:
lane <v>@e0: non-contiguous persisted blocks: expected 0, got 5
newInner → NewState → node fails to start, permanently, until the state dir is wiped.
i.nextBlockToPersist[lane] = 0 on the next line has the same off-by-anchor problem (it should be first too, matching what prune() does for active lanes via if i.nextBlockToPersist[...] < lr.First()).
Simplest fix: add the leave lanes to i.blocks/i.votes before the anchor i.prune(...) call at line 95 (they're all known from l.blocks), so the existing positioning logic covers them uniformly instead of being hand-rolled here. Otherwise call q.prune(first) and set nextBlockToPersist[lane] = first explicitly.
(Same finding as Codex's second item.)
| if _, ok := active[lane]; ok { | ||
| continue | ||
| } | ||
| if lane.EJoin() >= watermarkEpoch { |
There was a problem hiding this comment.
[blocker] The retention guard uses the lane's join epoch, not its leave epoch, so it fires far earlier than the invariant it's meant to enforce.
DeleteLane's doc says "the leave-epoch persist watermark has advanced", and tryPruneLeaveLanes' doc says a leaver stays "until the AppQC prune floor has advanced". But lane.EJoin() >= watermarkEpoch only protects lanes that joined at or after the current AppQC epoch — i.e. same-epoch joiners. For a validator with e_join = 0 that serves through epoch 9 and leaves at epoch 10, EJoin() = 0 < watermark is trivially true for any AppQC from epoch ≥ 1, so the lane is pruned on the very first tryPruneLeaveLanes after ApplyEpoch(ep10) — regardless of whether its epoch-9 tips have been committed.
Concrete failure: ApplyEpoch(ep10) lands while the durable AppQC is still at a road index inside epoch 9 whose CommitQCs carry a non-empty LaneRange for (V,0). pruneInactiveLanes drops votes[(V,0)]/blocks[(V,0)] and DeleteLane removes the WAL. headers() (state.go:690) then returns ErrPruned for that range, fullCommitQC propagates it, and the s.data.PushQC loop (state.go:842) continues past the road index. data.PushQC only accepts a QC when gr.First == inner.nextQC, so the local road stalls at that global block number — and since every node prunes on the same criterion at the same epoch boundary, no peer can serve the missing FullCommitQC either. V's final blocks are also gone from disk before commit, so lagging peers can't fetch them.
The same reasoning applies to the NewState call at state.go:347: leave-lane data just loaded from disk is deleted at startup whenever the anchor's AppQC epoch exceeds e_join, even though road indices at/after the anchor still need it.
This is only safe if ApplyEpoch(e) is guaranteed never to run before an AppQC in epoch e exists. That's an ordering constraint on #3736's advanceEpoch that nothing here establishes or asserts. Please either gate on the epoch in which the lane became inactive (or on the anchor's LaneRange(lane) being fully consumed), or make the required ordering an explicit precondition with a check.
The orphan-WAL scan at state.go:138 shares the same lane.EJoin() >= watermark condition and needs the same fix.
| continue | ||
| } | ||
| lane, err := types.PublicKeyFromBytes(laneBytes) | ||
| lane, err := types.LaneIDFromBytes(laneBytes) |
There was a problem hiding this comment.
[suggestion] Existing lane WAL directories are named with 32-byte hex pubkeys; LaneIDFromBytes now requires exactly 40 bytes, so every pre-existing lane dir hex-decodes fine but fails the length check and is skipped with a Warn. The result is that all persisted availability blocks are silently abandoned (and the stale dirs are never reclaimed, since tryPruneLeaveLanes can't see them either).
Given the BlockHeader.lane proto change also makes old WAL entries undecodable, an upgrade genuinely does require a state reset — but a logger.Warn in the middle of startup is the wrong way to communicate that. Prefer returning a hard error naming the incompatible directory ("lane WAL predates the LaneID format; wipe ") so an operator can't silently boot a node that lost its availability tips.
(Same finding as Codex's first item; I'd rate it lower than High since autobahn is opt-in via autobahn-config-file and the format change is unavoidable, but the silence is still worth fixing.)
| } | ||
|
|
||
| lw, err := bp.getOrCreateLane(lane) | ||
| allowCreate := active.HasLane(lane) || len(proposals) > 0 |
There was a problem hiding this comment.
[nit] active.HasLane(lane) nil-derefs if a caller passes a nil committee. Every current caller passes a non-nil one, but this is an exported method on an exported type and the new parameter is undocumented as non-optional. Either guard (active != nil && active.HasLane(lane)) or state the precondition in the doc comment above.
| if err := r.checkBound(r.state.LocalLane()); err != nil { | ||
| return nil, err | ||
| } | ||
| g, gctx := errgroup.WithContext(ctx) |
There was a problem hiding this comment.
[suggestion] This allocates an errgroup, a derived context, and two goroutines per received block on what the code itself calls "the main source of bandwidth consumption" (giga/avail.go:173). The identity check only needs to happen when LocalLane actually changes; consider hoisting a single long-lived watcher goroutine per subscription (or threading a cancel channel into State.Block) so the steady-state path is allocation-free.
Also: the outer for at line 48 can never iterate more than once — both goroutines always return a non-nil error (errGotProposal on success), so err != nil at line 83 is always taken when proposal == nil. Either drop the loop or make the success path return nil and let the loop carry the retry.
| if got, ok := availState.LocalLane().Get(); !ok || got != lane { | ||
| return context.Canceled | ||
| } | ||
| if !availState.HasLane(lane) { |
There was a problem hiding this comment.
[nit] This branch opens a brief busy-spin. ApplyEpoch releases the inner lock (new epoch installed, leaver's maps still present) before localLane.Store(newLane), so there's a window where LocalLane() == lane but HasLane(lane) == false. In that window streakOpErr returns context.Canceled, Iter reloads the same unchanged value, runLaneStreak re-runs, WaitForLocalCapacity fails again — a tight loop until the Store lands. The window is microseconds today, but it's unbounded in principle (nothing orders the two). Storing localLane inside the same s.inner.Lock() critical section in ApplyEpoch would close it.
| // roads / firstTimestamp / firstBlock describe the new epoch's road range and start markers. | ||
| // The previous epoch is left as stored; callers that need a closed RoadRange on the prior | ||
| // epoch should pass a roads.First that continues after the prior end when wiring transitions. | ||
| func (r *Registry) ActivateEpoch( |
There was a problem hiding this comment.
[nit] ActivateEpoch doesn't validate that roads.First continues after the previous epoch's road range, and as the doc notes it leaves the previous epoch's RoadRange as stored. The new tests all pass types.OpenRoadRange(), so epochs 0 and 1 both claim an open-ended road range with the same firstBlock. VerifyInWindow only consults the latest epoch so nothing breaks today, but this is a production API that currently permits an inconsistent registry. Either validate here or mark it explicitly as scaffolding for #3736.
There was a problem hiding this comment.
No new bugs found this run. The latest commit (b3a591f, "harden LaneID leave/rejoin lifecycle") appears to resolve the checkBound panic, nil-map-deref, orphan-WAL, and producer-race issues flagged in earlier reviews on this PR (cursor, seidroid, and my own prior comments). Given the size and consensus-critical scope of this change, I'd still like a human to take a look before merge rather than shadow-approving.
What was reviewed:
checkBound/Recvno longer panics on leave+rejoin coalescing - now returnsErrLaneIdentityChanged(avail/subscriptions.go)headers(), thePushQCblock-collection loop, andlaneQC()now guard missing lane maps instead of nil-dereferencing after a leave prune (avail/state.go, avail/inner.go)produceLocalBlock/streakOpErrnow bind to an explicit LaneID per streak, so a stale rejoin race resolves toErrBadLane/context.Canceledinstead of killingproducer.Run(producer/state.go)- Checked the ruled-out mempool-reset-on-rejoin candidate -
alignMempoolForLaneonly resets state when the streak's LaneID actually changes, so a same-lane stay keeps its tip and txs
Extended reasoning...
This run's bug hunter found no new issues. Cross-referencing the current HEAD (b3a591f, ahead of the PR diff shown) against the earlier cursor-bot, seidroid-bot, and my own prior claude[bot] comments on this thread, the latest commit appears to specifically address nearly every previously flagged blocker: the LaneID-coalescing panic in checkBound (now returns ErrLaneIdentityChanged), the nil-map-dereference in headers()/PushQC/laneQC() after a leave-lane prune (now guarded with ok-checks), the orphan leave-WAL-survives-restart issue (inner.go now reattaches leave-lane WALs into maps so tryPruneLeaveLanes/DeleteLane can reap them), the 'leave tears down peer RPC' issue (giga/avail.go now retries/resubscribes instead of propagating a fatal error out of the multiplexed RunServer scope), the producer lane-rederivation race (produceLocalBlock now takes an explicit bound LaneID and checks HasLane against it), and the HasLane O(n) regression (committee.go now keeps a byValidator map for O(1) lookup). I did not find a case where these fixes are incomplete.
This is nonetheless a large (36-file), consensus-critical change to how lanes are identified across the availability plane, persistence layer, producer, and wire protocol (BlockHeader.lane and LaneRange.lane both change proto type), including a protobuf wire-format change and WAL directory-naming change. ApplyEpoch/ActivateEpoch have no production callers yet (explicitly deferred to #3736), which reduces blast radius today, but the design decisions here (e.g. epoch-scoped LaneID reuse, AppQC-floor-gated lane pruning, coalescing-safe subscription semantics) are exactly the kind of judgment calls that warrant a human's sign-off before the epoch-transition wiring lands on top of them.
No security-sensitive auth/crypto/permission logic is touched beyond the existing signature verification already in place; the main risk surface is correctness/liveness of consensus (panics, stuck goroutines, dropped blocks) rather than exploitable vulnerabilities. Test coverage is substantial (new tests for committee activation, lane WAL orphan pruning, coalesced leave/rejoin subscription behavior, and a producer lifecycle test), which supports confidence in the fixes but doesn't substitute for a maintainer familiar with the epoch-transition roadmap reviewing the design.
Gate leave removal on the first retained CommitQC committee instead of e_join vs AppQC epoch. Skip leave WAL re-attach on restart, and Store LocalLane under the inner lock to avoid a producer soft-cancel spin. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
Solid, well-tested refactor of LaneID from PublicKey to (validator, e_join) with careful prune/leave/rejoin bookkeeping. The blocking concern is that it silently breaks the on-disk autobahn WAL format (and the P2P wire format) with no migration or documented state-dir reset, so an in-place upgrade of an autobahn-enabled node fails to start; several smaller robustness/hot-path issues are noted below.
Findings: 2 blocking | 11 non-blocking | 7 posted inline
Blockers
- On-disk WAL format break with no migration path.
BlockHeader.laneandLaneRange.lanechange fromPublicKeyto the newLaneIDmessage.PublicKey{ed25519: <32 bytes>}andLaneID{validator, e_join}are not wire-compatible, so on an in-place upgrade of an autobahn-enabled node a persisted prune anchor decodes throughPruneAnchorConv.Decode->types.CommitQCConv.Decode->LaneRangeConv.Decodeand fails on the embeddedLaneRange.loadPersistedState(sei-tendermint/internal/autobahn/avail/state.go:267) treats that as fatal, soNewStateerrors and the node will not start until<persistent_state_dir>is wiped. Same for the peer wire format: old and new binaries cannot exchangeBlockHeader/LaneRange, andBlockHeader.Hash()changes. Autobahn is opt-in and off by default, so this is likely acceptable in substance -- but it needs to be explicit: either handle/skip the old encoding, or state the required state-dir reset and the no-mixed-version constraint in the PR description / release notes. Right now the only breakage signal is thenon-app-hash-breakinglabel, which reads as the opposite. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only Claude's and Codex's findings. - Codex's point, confirmed but downgraded:
avail.State.ApplyEpoch,epoch.Registry.ActivateEpoch, andtypes.ActivateCommitteehave no non-test callers, so the entire leave/rejoin/prune path this PR adds is unreachable in production until theadvanceEpoch/onAdvancewiring in #3736 lands. That is consistent with the PR being explicitly incremental (theApplyEpochdoc comment says so), so it is not a defect -- but it does mean the new gates get no production coverage in this PR, and the correctness oftryPruneLeaveLanesrests entirely on unit tests plus a chain of non-local invariants (see the inline notes on state.go:122 and blocks.go:347). Worth an integration test that drives a real epoch transition throughproducer.Run+runPersistbefore the wiring PR flips it on. Registry.ActivateEpochchangingRWMutex[registryState]toRWMutex[*registryState]is the right fix (s.latest = nexton a value copy would have been lost) -- good catch. Unrelated nit:ActivateEpochdoesn't touchprev'sRoadRange, so callers passingOpenRoadRange()for every epoch (as all the new tests do) end up with overlapping open ranges. The doc comment acknowledges this; consider rejecting aroadsthat overlapsprevso the invalid state is unrepresentable rather than a caller contract.- The PR description says leavers are pruned at the "AppQC prune floor (
lane.EJoin() < appQC.EpochIndex())", but the implemented gate is tipcut-committee membership (!current.HasLane(lane) && !tipcut.HasLane(lane)), with noEJoin()comparison anywhere. The code is fine; the description should match it so future readers don't go looking for theEJoincheck. serverStreamLaneProposalscan now silently switch the stream to a differentLaneIDmid-connection (rejoin resetsfirst = 0).clientStreamLaneProposalshappens to tolerate this becausePushBlockresolves the lane from the header, but the commented-out sanity check at giga/avail.go:191 (proposal lane == peer key) can no longer be a simple equality once it's uncommented -- it has to compareLane().Validator(). Worth updating that comment now while the context is fresh.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| q := inner.votes[lr.Lane()] | ||
| q, ok := inner.votes[lr.Lane()] | ||
| if !ok { | ||
| return nil, types.ErrPruned |
There was a problem hiding this comment.
[suggestion] Returning types.ErrPruned for a missing vote queue overloads a sentinel whose caller silently skips work. fullCommitQC (state.go:728) propagates it verbatim, and the s.data.PushQC loop at state.go:838-841 does if errors.Is(err, types.ErrPruned) { continue } -- and continue runs the post statement n = max(n+1, s.FirstCommitQC()). So a single missing vote queue advances n past that road index and the FullCommitQC for a committed range is never pushed to the data state: the ordered event log skips a block range with no error and no log line.
I believe this is currently unreachable (removeLeaveLanes only drops lanes absent from both current and tipcut, and LaneRange.Verify rejects non-committee lanes, so any retained CommitQC's range for a removed lane is empty and headers early-returns at line 681). But that's a four-step argument guarding a silent-data-loss path. Prefer a distinct sentinel (or a hard error) so a future gate regression fails loudly instead of quietly dropping a committed road index.
| if err := r.checkBound(r.state.LocalLane()); err != nil { | ||
| return nil, err | ||
| } | ||
| g, gctx := errgroup.WithContext(ctx) |
There was a problem hiding this comment.
[suggestion] This allocates an errgroup, spawns two goroutines, and creates a fresh LocalLaneUpdates() subscription per block, per peer stream. clientStreamLaneProposals calls this the "main source of bandwidth consumption" path, and it runs for every peer × every lane block; previously Recv was a single blocking call.
The watcher doesn't need to be per-call: subscribe once in SubscribeLaneProposals and store the AtomicRecv on LaneProposalsRecv, then each Recv only needs the select. That also removes the per-call goroutine pair.
Separately, line 77 re-reads LocalLane() after Wait already established the predicate. If that second read ever returned nil (it can't today -- leave is terminal), goroutine 2 returns nil, gctx is not canceled, and Recv blocks in Block until the parent ctx dies. Just return ErrLaneIdentityChanged directly.
| return fmt.Errorf("unknown epoch_index %d for tipcut CommitQC", firstQC.Proposal().EpochIndex()) | ||
| } | ||
| tipcut = ep.Committee() | ||
| if len(inner.blocks) > current.Lanes().Len() { |
There was a problem hiding this comment.
[suggestion] This len() guard couples two invariants in a way that turns a future regression into disk/memory divergence rather than a loud failure. If inner.blocks ever holds a lane outside current while len(inner.blocks) == current.Lanes().Len(), removeLeaveLanes is skipped, but the KnownLanes() pass below (lines 133-141) still adds that lane to removed and DeleteLanes its WAL -- leaving the lane tracked in inner.blocks/votes with its WAL gone, which a later MaybePruneAndPersistLane would silently recreate via the len(proposals) > 0 branch.
The invariant holds today (addCommitteeLanes guarantees blocks ⊇ current.Lanes(), so equality implies no extras), but removeLeaveLanes is already an O(lanes) idempotent scan over at most MaxValidators entries. Dropping the guard costs nothing and removes the coupling.
| } | ||
|
|
||
| lw, err := bp.getOrCreateLane(lane) | ||
| allowCreate := active.HasLane(lane) || len(proposals) > 0 |
There was a problem hiding this comment.
[suggestion] active is a full *types.Committee used for exactly one boolean (active.HasLane(lane)), which drags a consensus type into the persistence layer and forces every test call site to fabricate a synthetic committee (committeeForLane, plus ~15 utils.OrPanic1(types.NewCommittee(...)) calls in state_test.go/blocks_test.go). Passing allowCreate bool (or active bool) instead would keep the persister's dependency surface where it was and delete all that test boilerplate.
Also: active is dereferenced with no nil guard, so a nil committee panics on the nil-map read inside HasLane rather than returning an error. A bool parameter makes that unrepresentable.
| // WaitForLocalCapacity waits until lane has capacity for toProduce. | ||
| func (s *State) WaitForLocalCapacity(ctx context.Context, lane types.LaneID, toProduce types.BlockNumber) error { | ||
| for inner, ctrl := range s.inner.Lock() { | ||
| if _, ok := inner.blocks[lane]; !ok { |
There was a problem hiding this comment.
[nit] The membership check runs once, but the WaitUntil predicate below doesn't re-check it -- and ctrl.Wait releases the lock, so the lane can be removed mid-wait. After removal inner.persistedBlockStart[lane] reads as the zero value, so the predicate silently becomes toProduce < BlocksPerLane: for a low toProduce it returns nil (and the caller then gets ErrBadLane from ProduceLocalBlock, which soft-cancels correctly), for a high toProduce it blocks until the streak ctx is canceled. Both end up in the right place, but only by accident of Go's zero-value map read. Moving the inner.blocks[lane] check inside the predicate and returning ErrBadLane makes the intent explicit.
| m.lane = utils.Some(lane) | ||
| m.first = n | ||
| m.next = n | ||
| m.blocks = map[types.BlockNumber]*blockSpec{} |
There was a problem hiding this comment.
[nit] A new-LaneID streak throws away blocks, evmNonces, and evmTxs with no log line, so every tx admitted during the leave gap is dropped invisibly. Run's not-a-member branch (line 108-111) just blocks on <-ctx.Done() rather than closing admission, so the mempool keeps accepting txs the whole time it is guaranteed they'll be discarded. At minimum log the discarded count here; better would be to stop admitting while LocalLane is None.
Minor: n is read from avail at line 76, outside the mempool lock. Peer-echoed proposals for our own lane can advance the avail tip in that window (PushBlock only requires p.Key() == h.Lane().Validator()), leaving m.first below the real tip and making the first ProduceLocalBlock fail with a non-ErrBadLane "not the next block expected" error that streakOpErr will not soften. Narrow, but it kills producer.Run permanently if hit.
|
|
||
| type mempool struct { | ||
| capacity uint64 | ||
| lane utils.Option[types.LaneID] | ||
| first types.BlockNumber | ||
| next types.BlockNumber | ||
| blocks map[types.BlockNumber]*blockSpec |
There was a problem hiding this comment.
🟡 producer/mempool.go:83 defines (s *State) mempoolFirst(), but this PR's refactor replaced its only call site (in the old Run()) with s.alignMempoolForLane(lane) inside the new runLaneStreak, leaving the method with zero callers anywhere in the repo. It's a small leftover from the LaneID refactor and can be deleted.
Extended reasoning...
mempoolFirst is an unexported method on producer.State defined at sei-tendermint/internal/autobahn/producer/mempool.go:83. A full-repo grep for the identifier turns up exactly one match — its own definition — confirming there are no remaining callers, direct or otherwise (and since it is unexported, no caller outside the producer package is possible either).
The diff shows why: the old producer.State.Run() computed the initial tip for the block-production loop with firstBlock := s.mempoolFirst(). This PR restructures Run to iterate over LocalLaneUpdates() and spawn a runLaneStreak(ctx, availState, lane) goroutine per membership streak; runLaneStreak computes the equivalent starting point via the new s.alignMempoolForLane(lane), which additionally handles the streak-continuity logic (keeping the mempool tip when the LaneID is unchanged, resetting it on a new streak). That method fully subsumes what mempoolFirst used to do, but mempoolFirst itself was never deleted.
This is purely a dead-code cleanup item, not a functional bug — the code compiles and behaves identically with or without it. It also is unlikely to be caught by CI: the project's enabled golangci-lint linters (errcheck, gosec, govet, staticcheck, ineffassign, goconst, prealloc, unconvert, misspell, bodyclose, dogsled) do not include the unused (U1000) linter that would normally flag an unused unexported method, so nothing will force its removal automatically.
Step-by-step confirmation:
grep -rn mempoolFirst sei-tendermint/internal/autobahn/producer/returns onlymempool.go:83(the func definition) — no call sites instate.go, tests, or elsewhere.- The PR diff for
state.goshows the old linefirstBlock := s.mempoolFirst()deleted and replaced byfirstBlock := s.alignMempoolForLane(lane)in the newrunLaneStreak. - Since
mempoolFirstis lowercase (unexported) and defined on*Statein packageproducer, it cannot be reached from any other package.
Fix: delete the mempoolFirst method from mempool.go as part of this PR's cleanup, since it was this PR's own refactor that orphaned it.
| // removeLeaveLanes drops inactive leave lanes that the tipcut committee no | ||
| // longer names. A left LaneID never returns (rejoin is a new ID), so once the | ||
| // first retained CommitQC's committee omits it, no later CQ in the window can | ||
| // need it either. Returns removed LaneIDs for DeleteLane. | ||
| func (i *inner) removeLeaveLanes(current, tipcut *types.Committee) []types.LaneID { | ||
| var removed []types.LaneID | ||
| for lane := range i.blocks { | ||
| if current.HasLane(lane) || tipcut.HasLane(lane) { | ||
| continue | ||
| } | ||
| delete(i.blocks, lane) | ||
| delete(i.votes, lane) | ||
| delete(i.nextBlockToPersist, lane) | ||
| delete(i.persistedBlockStart, lane) | ||
| removed = append(removed, lane) | ||
| } | ||
| return removed | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 removeLeaveLanes (inner.go) decides whether a leave lane is safe to drop by checking only two committees — the latest applied epoch and the committee of the oldest still-retained CommitQC (the 'tipcut') — but the retained CommitQC window can span more than these two epochs whenever AppQC confirmation lags CommitQC production across an epoch boundary. A validator that joins in an epoch strictly between the tipcut epoch and the current epoch, then leaves before the current epoch, is absent from both endpoint committees and gets its lane (and WAL) deleted even though CommitQCs from its join epoch still sit in the retained window and reference it. This is a genuine but currently-dormant gap, since ApplyEpoch/ActivateEpoch have no production callers yet.
Extended reasoning...
tryPruneLeaveLanes (state.go) computes current = inner.epoch.Committee() (the latest applied epoch) and tipcut = committee of the epoch of commitQCs.q[first] (the oldest CommitQC still retained in the window), then calls removeLeaveLanes(current, tipcut) (inner.go), which deletes a lane from inner.blocks/inner.votes — and later has its WAL removed via DeleteLane — if the lane is absent from both committees. The code comment justifies this as: "once the first retained CommitQC's committee omits it, no later CQ in the window can need it either," reasoning that a left LaneID never reappears.
That justification only holds for a lane that existed at-or-before the tipcut epoch and later left. It does not hold for a lane that joins strictly after the tipcut epoch and leaves before the current epoch — such a lane is also absent from both endpoint committees, but CommitQCs from its join epoch, which reference it, can still be sitting in the retained window if the window spans more than two epochs.
The window can indeed span more than two epochs: inner.commitQCs is only trimmed at the front by prune(), which runs when a new AppQC confirms execution progress (inner.go). PushCommitQC (state.go), by contrast, appends new CommitQCs as soon as they arrive from consensus, gated only on matching the current applied epoch — it does not wait for AppQC to catch up. Under any AppQC-execution lag across an epoch transition (a normal condition under load or during catch-up), commitQCs.q[first,next) accumulates entries from several epochs at once.
Concretely: validator V joins at epoch E1 (added to inner.blocks/inner.votes via addCommitteeLanes when E1 is applied), produces blocks referenced by E1 CommitQCs, then leaves at epoch E2 (current epoch). If the AppQC floor is still lagging behind in epoch E0 (E0 < E1 < E2), tipcut resolves to E0's committee, which does not name V (V hadn't joined yet), and current is E2's committee, which no longer names V (V left). removeLeaveLanes sees V absent from both and drops it, and DeleteLane removes its on-disk WAL unrecoverably. But E1 CommitQCs are still sitting in commitQCs.q[first,next) and still reference V's lane range. The next headers()/fullCommitQC() call for that E1 road-index range hits the !ok branch and returns ErrPruned (state.go), and the s.data.PushQC block-collection loop (state.go) sees inner.blocks[lr.Lane()] missing and silently continues past it — even though the AppQC floor never actually advanced past those blocks. That is a false-positive prune: it violates the documented invariant that leave-lane data survives until the AppQC prune floor passes it, and it is a distinct root cause from the already-fixed nil-deref issue on the same reviewer thread (that one added the ok-checks that turn the crash into this silent ErrPruned/skip behavior instead).
The fix needs to consider every epoch actually represented in the retained CommitQC window, not just its two endpoints — e.g. by checking every epoch's committee between tipcut and current (there may be more than one), or by gating removal on the anchor's LaneRange for that lane being fully consumed by the AppQC floor, rather than committee membership at the window's edges.
Like the sibling autobahn epoch-transition findings already raised on this PR (e.g. the join/leave-epoch persistence and streak issues), this cannot be triggered by any code path a running node executes today: State.ApplyEpoch/Registry.ActivateEpoch have no production callers in this PR — they're exercised only by apply_epoch_test.go and producer/lifecycle_test.go, with the real epoch-transition wiring deferred to a follow-up (#3736). So merging as-is does not risk any observable failure right now, but the gap should be closed before that wiring lands, since it would then become a live liveness/correctness issue (stalled road progression and unrecoverable WAL loss for a specific join/leave interleaving).
Reserve former PublicKey "lane" and place LaneID on a new field so Buf WIRE/WIRE_JSON stay additive. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
Solid, well-documented refactor of LaneID from a bare pubkey to (validator, e_join), with good test coverage for the stay/leave/rejoin state machine and correctly regenerated wireguard/proto size tables. Two blocking issues remain: existing autobahn persistent state becomes unreadable with no migration or version gate (commitQC WAL hard-fails, block WAL dirs are silently orphaned), and a restart inside a leave window can make the local PushQC loop permanently skip road indices whose retained CommitQCs still reference the departed lane.
Findings: 5 blocking | 13 non-blocking | 9 posted inline
Blockers
- Upgrade path for an existing autobahn
persistent_state_diris undefined and handled inconsistently: the commitQC WAL / prune anchor hard-fail to decode (startup abort), while legacy block WAL dirs are warn-and-skipped (silent data loss + orphaned dirs thattryPruneLeaveLanescan never reach, since they are not inbp.lanes). Pick one policy — a state-dir version marker that fails fast with an actionable message, or an explicit migration/cleanup — and state it in the PR description. See the two inline comments onblocks.go:234andautobahn.proto:131. - No test covers the upgrade path at all: there is no case that opens a
BlockPersisterover a 32-byte-hex lane dir, and none that feeds a pre-change (field-1lane)LaneRange/CommitQCWAL entry throughloadAllCommitQCs. Whatever policy is chosen for the item above should be pinned by a test, since this is exactly the failure that only shows up on a real operator's disk. - 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor pass (
cursor-review.md) produced no output — the file is empty. Only Codex's three findings were available to merge; all three are reflected above (its P1 #1 as a blocker, P1 #2 as a blocker, P2 #3 as an inline suggestion). - PR description drift: it states the prune floor is the "in-memory AppQC prune floor (
lane.EJoin() < appQC.EpochIndex())", but the implemented gate intryPruneLeaveLanesis "the first retained CommitQC's committee no longer names the lane". The code comments are accurate; the description is not. Worth fixing since this is the subtlest invariant in the change. producer.mempoolFirst()(producer/mempool.go:83) is now dead —Runwas its only caller and it went throughalignMempoolForLane.unusedis not in.golangci.yml, so lint will not catch it.tryPruneLeaveLanesallocates a fullKnownLanes()snapshot and re-scans it after every persist batch, even when nothing has left the committee. Cheap fix: skip the whole body unless the epoch changed since the last check orlen(inner.blocks) > current.Lanes().Len()(the guard already used for the in-memory half).LaneID.Bytes()callsl.validator.Bytes()twice (once for the capacity, once for the append). Hoist it into a local.epoch.GenRegistrystill builds its genesis committee viaNewCommittee(alle_join = 0), so the avail/producer/giga tests only ever exercisee_join = 0lanes end-to-end; the random-e_joinGenCommitteechange is confined to thetypespackage. AnActivateEpoch-based fixture with a non-zeroe_joinstayer would give the new WAL naming and map keying real coverage.ApplyEpochreturns anerrorthat is documented as always nil. Fine as a forward-compat hook for #3736, but every current caller has torequire.NoErroron something that cannot fail.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| continue | ||
| } | ||
| lane, err := types.PublicKeyFromBytes(laneBytes) | ||
| lane, err := types.LaneIDFromBytes(laneBytes) |
There was a problem hiding this comment.
[blocker] Lane WAL directories on existing nodes are named hex(pubkey) — 64 hex chars / 32 bytes. LaneIDFromBytes requires exactly 40 bytes, so on the first start after this change every legacy lane dir takes the continue below and is dropped with only a Warn. Two consequences:
- All persisted lane blocks are silently discarded (Codex P1 Node system requirements #1).
- The dirs are orphaned permanently: they never enter
bp.lanes, soKnownLanes()never returns them andtryPruneLeaveLanes/DeleteLanecan never clean them up. They just sit on disk forever.
A warn-and-skip is the wrong default for state the node is supposed to own. Either fail fast with an actionable message ("incompatible lane WAL layout, wipe
"), or explicitlyos.RemoveAll legacy-format dirs here, so the outcome is deliberate rather than a log line an operator will miss.
| optional uint64 first = 2; // required | ||
| optional uint64 next = 3; // required | ||
| optional bytes last_hash = 4 [(wireguard.max_size) = 32]; // required | ||
| optional LaneID lane_id = 5; // required |
There was a problem hiding this comment.
[blocker] reserved 1 + required lane_id = 5 makes the persisted commitQC WAL unreadable, not just the wire format. NewCommitQCPersister opens its WAL with types.CommitQCConv and loadAllCommitQCs decodes every entry eagerly; a pre-change entry has LaneRange.lane on field 1 and no lane_id, so LaneIDConv.DecodeReq(nil) returns e_join: missing. That error propagates out of NewCommitQCPersister → loadPersistedState → avail.NewState, so a node with an existing autobahn state dir refuses to start, with no migration and no version marker. The prune anchor (PersistedAvailPruneAnchor, which embeds a CommitQC) fails the same way.
Note this is the opposite behaviour from the block WAL, which silently skips legacy dirs (see the comment on blocks.go:234) — one path aborts startup, the other loses data quietly.
Autobahn is opt-in (autobahn-config-file), so "wipe the state dir" may well be an acceptable rollout. But that has to be a stated decision: add a state-dir format version that fails with an explicit "incompatible autobahn state, remove
| // Restore persisted blocks. Since the anchor is persisted first and | ||
| // blocks are written sequentially per lane, gaps, parent-hash | ||
| // mismatches, and over-capacity indicate corruption or a bug. | ||
| // Restore persisted blocks for committee lanes only. Leave WALs stay on |
There was a problem hiding this comment.
[blocker] This restart policy has a hole when the restart lands inside a leave window (Codex P1 #2/#3). Concretely: lane L leaves at epoch 6, the retained CommitQCs still start in epoch 5, and the node restarts. newInner only creates queues for LatestEpoch()'s lanes, so L is absent from blocks/votes; tryPruneLeaveLanes correctly keeps L's WAL on disk because the tipcut committee (epoch 5) still names it — but nothing ever re-attaches it to memory.
Now fullCommitQC(n) for those epoch-5 indices iterates epoch 5's committee, hits headers(qc.LaneRange(L)), finds no votes[L] and returns ErrPruned (state.go:687-690). In Run's s.data.PushQC loop (state.go:839-841) ErrPruned does continue, which advances n — so that road index is permanently skipped locally, silently. Recovery then depends entirely on a peer re-supplying the FullCommitQC over StreamFullCommitQCs; there is no local fallback and no log line saying a QC was dropped.
The surrounding comment explains why re-attaching here would fight tipcut positioning, which I buy — but the resulting gap needs to be closed rather than absorbed. Options: restore blocks for tipcut ∪ current lanes rather than current only; or make the ErrPruned-from-missing-lane case distinguishable from genuine tip pruning so Run can retry/log instead of skipping the index.
| firstQC := inner.commitQCs.q[inner.commitQCs.first] | ||
| ep, ok := s.data.Registry().EpochByIndex(firstQC.Proposal().EpochIndex()) | ||
| if !ok { | ||
| return fmt.Errorf("unknown epoch_index %d for tipcut CommitQC", firstQC.Proposal().EpochIndex()) |
There was a problem hiding this comment.
[suggestion] This turns a missing registry epoch into a hard error, and tryPruneLeaveLanes is called from NewState (line 344) — so it is now a startup-abort path. Epochs are not persisted anywhere: after a restart the registry only contains genesis, so as soon as ActivateEpoch is wired up (#3736) any node whose first retained CommitQC came from epoch ≥ 1 will fail to construct avail.State. Harmless today because nothing calls ActivateEpoch outside tests, but it's a landmine that will fire the moment epoch transitions land. Worth a TODO(#3736) here noting that epoch persistence is a prerequisite, or degrading to "skip pruning this round" instead of erroring.
| // WaitForLocalCapacity waits until lane has capacity for toProduce. | ||
| func (s *State) WaitForLocalCapacity(ctx context.Context, lane types.LaneID, toProduce types.BlockNumber) error { | ||
| for inner, ctrl := range s.inner.Lock() { | ||
| if _, ok := inner.blocks[lane]; !ok { |
There was a problem hiding this comment.
[suggestion] The lane-presence check happens once, before WaitUntil. If removeLeaveLanes deletes the lane while this call is parked in WaitUntil, the predicate keeps reading inner.persistedBlockStart[lane], which now returns the zero value — so for toProduce >= BlocksPerLane the predicate becomes permanently false and the waiter hangs until ctx cancellation, and for smaller toProduce it returns nil and lets the caller proceed on a lane that no longer exists.
Block (line 569) and PushBlock (line 600) have the same shape but worse: they capture q and then wait on q.next, which after deletion is a detached queue that will never advance. Previously safe because lanes were append-only — that invariant is exactly what this PR removes. Re-checking presence inside the WaitUntil predicate and returning ErrBadLane on disappearance would make these fail deterministically instead of depending on the caller's context.
| return r.checkBound(r.state.LocalLane()) | ||
| }) | ||
| err := g.Wait() | ||
| if proposal != nil { |
There was a problem hiding this comment.
[nit] proposal != nil takes precedence over err, so when the block waiter and the membership waiter both complete, Recv returns the proposal even though the bound streak already ended (Codex P2). I think this is benign in substance — the proposal genuinely belongs to r.lane and was legitimately produced, and the next Recv returns ErrLaneIdentityChanged via checkBound — but it does contradict the doc contract above, and the giga handler will forward one post-change proposal before pausing. Either flip the precedence (check ErrLaneIdentityChanged first and drop the in-flight proposal) or amend the doc comment to say that the last in-flight proposal of the old streak may still be delivered.
Separately: g.Wait() can never return nil here (the membership goroutine returns either an error or ErrLaneIdentityChanged, and the block goroutine returns errGotProposal), so the outer for never iterates twice. Worth simplifying or adding a comment, otherwise it reads as a retry loop that isn't one.
| if errors.Is(err, avail.ErrBadLane) { | ||
| availState := s.consensus.Avail() | ||
| if got, ok := availState.LocalLane().Get(); !ok || got != lane { | ||
| return context.Canceled |
There was a problem hiding this comment.
[suggestion] Returning the bare context.Canceled sentinel to mean "streak ended, let Iter pick up the next lane" only works because atomicWatch.Iter wraps g.Wait() in IgnoreCancel. That coupling is invisible from here: if runLaneStreak ever gains another caller, or Iter's error handling changes, a real leave-race error is swallowed as a normal cancellation with no log line. A dedicated errStreakEnded sentinel handled explicitly in Run (and logged) would express the intent without borrowing context.Canceled's meaning.
| // finalizeCommittee sorts by pubkey only and rejects duplicate validators | ||
| // (one pubkey must not appear with multiple e_join values). | ||
| func finalizeCommittee(lanes []LaneID, weights map[PublicKey]uint64, totalWeight uint64) (*Committee, error) { | ||
| slices.SortFunc(lanes, func(a, b LaneID) int { return a.validator.Compare(b.validator) }) |
There was a problem hiding this comment.
[nit] Sorting by a.validator.Compare(b.validator) while the type advertises LaneID.Compare (validator, then e_join) leaves two orderings in play. They're equivalent here only because the duplicate-validator check below rejects the case where they'd differ. Using a.Compare(b) costs nothing and keeps Lanes()'s documented "sorted" ordering consistent with the type's own total order.
| // All layers (consensus, data, avail) read from it. | ||
| type Registry struct { | ||
| state utils.RWMutex[registryState] | ||
| state utils.RWMutex[*registryState] |
There was a problem hiding this comment.
[nit] The switch to *registryState is necessary — RWMutex.Lock() yields T by value, so s.latest = next in ActivateEpoch would have been lost on a value type. Worth a short comment saying that, since the change looks gratuitous otherwise. Side effect to note: RLock() now hands readers a mutable *registryState, so the read lock no longer prevents accidental mutation. All current readers are read-only, but the guardrail is gone.
| i.latestCommitQC.Store(utils.Some(i.commitQCs.q[i.commitQCs.next-1])) | ||
| } | ||
|
|
||
| // Restore persisted blocks. Since the anchor is persisted first and | ||
| // blocks are written sequentially per lane, gaps, parent-hash | ||
| // mismatches, and over-capacity indicate corruption or a bug. | ||
| // Restore persisted blocks for committee lanes only. Leave WALs stay on | ||
| // disk until tryPruneLeaveLanes (tipcut committee no longer names them); | ||
| // re-attaching them here would fight tipcut positioning after prune(). | ||
| // Since the anchor is persisted first and blocks are written sequentially | ||
| // per lane, gaps, parent-hash mismatches, and over-capacity indicate | ||
| // corruption or a bug. | ||
| for lane, bs := range l.blocks { | ||
| q, ok := i.blocks[lane] | ||
| if !ok || len(bs) == 0 { |
There was a problem hiding this comment.
🟡 On restart, newInner only seeds inner.blocks/inner.votes from the loaded epoch's committee lanes, so any leave lane whose WAL tryPruneLeaveLanes is deliberately retaining (because the tipcut CommitQC window still needs it) gets skipped by the persisted-blocks restore loop (q, ok := i.blocks[lane]; if !ok { continue }) and silently dropped from memory even though its data is intact on disk. Concretely this makes Block() return ErrBadLane for that lane after restart, so its still-valid WAL blocks can no longer be served to peers — defeating the PR's own stated invariant that leave-lane data survives until the AppQC prune floor advances. This is dormant today since ApplyEpoch/ActivateEpoch have no production callers yet, but worth fixing before epoch transitions are wired up (#3736).
Extended reasoning...
The bug: newInner (avail/inner.go) builds inner.blocks/inner.votes by iterating only epoch.Committee().Lanes().All() — i.e. the lanes of whichever epoch is loaded from disk at startup. The persisted-blocks restore loop that follows then does:
for lane, bs := range l.blocks {
q, ok := i.blocks[lane]
if !ok || len(bs) == 0 {
continue
}
...
}Any lane present in l.blocks (loaded from the WAL on disk) that is not a key in i.blocks is silently skipped — no error, no log. This is exactly the behavior TestNewInnerLoadedBlocksUnknownLane asserts ('unknown lane data is dropped, no error').
Why this is reachable for a legitimately-retained lane: tryPruneLeaveLanes intentionally keeps a leave lane's WAL on disk (and its inner.blocks/inner.votes map entries) for as long as the tipcut — the committee of the oldest still-retained CommitQC — still names that lane, even after the current epoch's committee no longer does. That is precisely the retention window the PR introduces via the new comment in inner.go ('Leave WALs stay on disk ... re-attaching them here would fight tipcut positioning'). By definition, a lane inside that retention window is absent from the current epoch's committee — which is the only set newInner uses to seed i.blocks/i.votes on restart. So a restart that happens to land inside this window causes the restore loop to hit the !ok branch for that lane and drop its persisted blocks from memory, even though the WAL is fully intact and tryPruneLeaveLanes has not decided to delete it.
Why nothing else catches this: addCommitteeLanes only adds lanes belonging to the current committee when ApplyEpoch runs, so it never repairs the gap left by newInner. There's also no persisted registry entry that would let newInner reconstruct "this lane is still in its retention window" — it only has the current epoch's committee to work from.
Concrete impact and proof: Block() (state.go) resolves a lane's queue via inner.blocks[lane], returning ErrBadLane when the key is absent. Walk through: (1) validator V is a committee lane through epoch 9, produces several blocks; (2) V leaves at epoch 10, but a CommitQC from epoch 9 that still references V's lane range sits in the retained commitQCs window, so tryPruneLeaveLanes correctly keeps V's WAL and map entries; (3) the node restarts while still inside that window; (4) newInner seeds i.blocks/i.votes from only the current (post-leave) epoch's committee, which no longer names V, so V's entries are absent; (5) the persisted-blocks loop sees V's lane in l.blocks (loaded from V's still-open WAL on disk) but i.blocks[V] doesn't exist, so it continues and drops V's blocks; (6) any subsequent call to Block(ctx, vLane, n) returns ErrBadLane, meaning this node can no longer serve V's still-durable WAL blocks to lagging peers — the exact invariant the PR states it preserves (leave-lane data survives until the AppQC prune floor advances) is broken by a mere restart.
Fix direction: on restart, newInner needs to also seed queues for any lane present in the loaded persisted state that the tipcut committee (not just the current committee) still names, mirroring the two-committee check removeLeaveLanes already performs at runtime — i.e., don't gate the restore loop purely on current-committee membership.
Scope: this is dormant today because ApplyEpoch/ActivateEpoch have no production caller in this PR (only exercised by apply_epoch_test.go and producer/lifecycle_test.go), and the epoch registry itself isn't restored across a real restart yet — so no genuine leave lane exists on disk in production. It becomes live once epoch transitions are wired up (tracked as #3736 in the PR description), so it's worth closing before that lands rather than after.
| want := lr.LastHash() | ||
| headers := make([]*types.BlockHeader, lr.Next()-lr.First()) | ||
| for inner, ctrl := range s.inner.Lock() { | ||
| q := inner.votes[lr.Lane()] | ||
| q, ok := inner.votes[lr.Lane()] | ||
| if !ok { | ||
| return nil, types.ErrPruned | ||
| } | ||
| for i := range headers { | ||
| n := lr.Next() - types.BlockNumber(i) - 1 //nolint:gosec // i is bounded by len(headers) which is a small block range; no overflow risk | ||
| for { |
There was a problem hiding this comment.
🟡 In State.headers() (avail/state.go), the per-lane vote queue q is looked up once from inner.votes[lr.Lane()] on entry and then reused across repeated ctrl.Wait(ctx) calls, which release and re-acquire the lock without re-checking that the lane is still present in the map. If tryPruneLeaveLanes/removeLeaveLanes deletes that lane while headers() is mid-wait, the captured queue is orphaned (its map key is gone and a left LaneID never comes back), so if the awaited header wasn't already buffered the wait loop spins forever instead of returning an error, permanently stalling the local PushQC goroutine until restart.
Extended reasoning...
State.headers() in sei-tendermint/internal/autobahn/avail/state.go fetches the per-lane vote queue once when entering the s.inner.Lock() iterator:
for inner, ctrl := range s.inner.Lock() {
q, ok := inner.votes[lr.Lane()]
if !ok {
return nil, types.ErrPruned
}
for i := range headers {
...
for {
// checks against q.first / q.q[n].byHash[want]
if err := ctrl.Wait(ctx); err != nil {
return nil, err
}
}
}
}The !ok guard only covers the lane-presence check taken once, at function entry. ctrl.Wait(ctx) (the underlying WatchCtrl/mutex primitive) unlocks the inner state for the duration of the wait and re-locks it once woken, but the inner per-block loop never re-derives q from inner.votes[lr.Lane()] after that — it keeps using the same local variable captured before the first Wait.
This PR adds exactly the deletion path that can invalidate that assumption mid-wait: tryPruneLeaveLanes / inner.removeLeaveLanes (state.go / inner.go) computes a "tipcut" committee from the oldest still-retained CommitQC and calls delete(i.votes, lane) (along with i.blocks, nextBlockToPersist, persistedBlockStart) for any lane absent from both the current and tipcut committees. Because a lane's leave is terminal — a rejoin always gets a brand-new LaneID with a new e_join — that map key can never be repopulated once removed.
If headers() is blocked inside ctrl.Wait for a given lane when removeLeaveLanes runs against that same lane, the local q it's holding becomes a permanently orphaned queue object: it is no longer reachable from inner.votes, so PushVote (which does its own inner.votes[lane] lookup) can never write into it again. If the specific header headers() is waiting on was not already buffered in q before the deletion, the awaited condition (q.first advancing past it, or q.q[n].byHash[want] appearing) can never become true. Every subsequent ctrl.Updated() from unrelated activity wakes the Wait, the loop re-checks the same unsatisfiable condition on the orphaned q, and re-waits — forever, until the passed-in ctx is canceled.
headers() is called from fullCommitQC, which is called from the s.data.PushQC goroutine in Run(). A hang there permanently stalls that goroutine and, with it, the node's ordered execution log, requiring a restart to recover — a much worse outcome than the ErrPruned this PR already added for the "lane already gone at entry" case.
Step-by-step reproduction sketch:
- Validator V is in the current committee and has an in-flight
CommitQCin the retained window referencing V's lane. - The local
s.data.PushQCgoroutine callsfullCommitQC→headers()for aLaneRangeon V's lane, and blocks insidectrl.Waitwaiting for a header that hasn't arrived yet (e.g. a network partition delaying V's proposal). - Concurrently,
runPersist's periodictryPruneLeaveLanes()call advances the tipcut (via a remotely gossipedAppQC/CommitQCthat movesinner.commitQCs.firstpast the epoch in which V was a committee member) such that V is absent from both the current and tipcut committees, andremoveLeaveLanesdeletes V's lane frominner.votes. - The
qlocal variable in the blockedheaders()call now points to a queue no longer reachable viainner.votes[V's lane]. Even if V's block eventually arrives,PushVotelooks it up viainner.votes[lane], misses, and never lands it inq. headers()'s wait condition is now permanently false; the call hangs untilctxis canceled (e.g. process shutdown), silently stalling thePushQCgoroutine.
This is a real design gap distinct from the !ok check this PR added (which only guards the initial lookup, not deletion during the wait), and distinct from the sibling removeLeaveLanes two-epoch-window finding. However, like the other leave/prune findings on this PR, it is only reachable once ApplyEpoch/ActivateEpoch have a production caller — currently they are invoked only from tests, with real epoch-transition wiring deferred to a follow-up (#3736). Merging this PR as-is cannot trigger the hang in production today, so this doesn't need to block the PR, but the fix (re-validate lane presence, e.g. via inner.votes[lr.Lane()], right after each ctrl.Wait and return ErrPruned if it's gone) should land before epoch transitions go live.
| slices.SortStableFunc(sks, func(a, b SecretKey) int { | ||
| return -cmp.Compare(pks[a.Public()], pks[b.Public()]) | ||
| }) | ||
| return utils.OrPanic1(NewCommittee(pks)), sks | ||
| weights, total, err := normalizeWeights(pks) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| lanes := make([]LaneID, 0, len(weights)) | ||
| for v := range weights { |
There was a problem hiding this comment.
🟡 GenCommittee draws GenEpochIndex(rng) inside for v := range weights { ... }, a nondeterministic Go map iteration, so which validator receives which e_join value varies between runs even with the same utils.TestRng() seed. This breaks the seed-reproducibility guarantee used to debug flaky/failing tests (rerunning with the same seed can produce a different Committee). Fix by iterating a pubkey-sorted slice of validators before drawing, matching the pattern finalizeCommittee already uses for lanes.
Extended reasoning...
The bug
GenCommittee in sei-tendermint/autobahn/types/testonly.go builds committee lanes with:
lanes := make([]LaneID, 0, len(weights))
for v := range weights {
lanes = append(lanes, NewLaneID(v, GenEpochIndex(rng)))
}weights is a map[PublicKey]uint64, and Go deliberately randomizes map iteration order per process (it is not just "some fixed but unspecified order" — it differs across separate runs of the same binary). GenEpochIndex(rng) (testonly.go, a few lines down) is EpochIndex(rng.Uint64() % 100) — it consumes one draw from the shared rng on every loop iteration.
Because the loop body both iterates a randomized-order map and draws from rng inside that loop, the mapping from validator to e_join value is not determined by the rng seed alone — it also depends on the map iteration order for that process run. Two processes started with utils.TestRng() (a fixed seed) will draw the exact same multiset of e_join values (the same number of rng.Uint64() calls happen either way), but which validator ends up bound to which value differs, because the loop visits validators in a different order.
Why this matters
The project's AGENTS.md establishes TestRng() as a deterministic-entropy test helper specifically so that a failing test can be re-run with the same seed and reproduce the same failure. GenCommittee is used pervasively across the autobahn test suite to build test committees. If a test's pass/fail outcome depends on which validator got which e_join (e.g. a test exercising the lane join/leave/rejoin lifecycle, or the pruning behavior gated on e_join), rerunning that failing test with the identical seed can silently produce a different Committee and a different (or no) failure — defeating the purpose of the seeded RNG for debugging.
Why this is newly introduced
Before this PR, GenCommittee called NewCommittee(pks), which stamped e_join = 0 for every member unconditionally and drew no rng value during the map loop. There was no per-validator draw to be affected by map ordering. This PR adds the GenEpochIndex(rng) call inside the loop specifically to give each committee member a random join epoch, introducing the nondeterminism.
Step-by-step proof
utils.TestRng()returns an RNG seeded from a fixed constant (perAGENTS.md, this is meant to give "deterministic entropy").- Call
GenCommittee(rng, 3)in process A. Suppose the mapweightshappens to iterate as{v1, v2, v3}in that process. The loop draws threerng.Uint64()values, sayd1, d2, d3, and assignsv1→d1%100, v2→d2%100, v3→d3%100. - Run the exact same code with an RNG constructed from the identical seed in process B (e.g., rerunning the same test). The three
rng.Uint64()draws are stilld1, d2, d3in that order (rng state is deterministic), but Go's map iteration order is independent of the seed and randomized per process — suppose this time the map iterates as{v2, v3, v1}. - Now the assignment is
v2→d1%100, v3→d2%100, v1→d3%100— a completely different validator-to-e_joinmapping than process A, despite using the identical seed. - Any test whose behavior depends on which validator has which
e_join(lane identity, join/leave/prune timing tests, etc.) can therefore behave differently between two "identically seeded" runs.
The fix
Sort the validators (e.g., by public key, exactly as finalizeCommittee already does when sorting the final lanes slice by validator.Compare) before iterating and drawing GenEpochIndex(rng), so the draw order — and therefore the resulting mapping — depends only on the rng seed, not on map iteration order.
Severity
All three independent verifiers agree this is nit-level: testonly.go is a test-only helper with no production/consensus code path, so this cannot cause a mainnet/testnet issue — it only degrades the reproducibility of seeded test failures, which is inconvenient but not blocking.
1 participant
Summary
LaneID = PublicKeywithLaneID = (validator, e_join): stay keepse_join, leave is terminal for that identity, rejoin allocates a new LaneID (block tip restarts at 0).ApplyEpochseeds joiner lane maps; leavers stay until the in-memory AppQC prune floor (lane.EJoin() < appQC.EpochIndex()), then memory +DeleteLane. Persist still flushes leave tips even if the WAL was not open yet (allowCreatewhen proposals are non-empty).SubscribeLaneProposalsfollow appliedLocalLanestreaks (stay = no churn; leave/rejoin →ErrLaneIdentityChanged; giga resubscribes without tearing down peer RPC).Made with Cursor