feat(autobahn): implement new lane ID for epoch (CON-358) - #3862
feat(autobahn): implement new lane ID for epoch (CON-358)#3862wen-coding wants to merge 6 commits into
Conversation
8d254d4 to
4f52a00
Compare
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
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.
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.
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.
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.
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.
There was a problem hiding this comment.
This PR is a large, consensus-critical refactor (LaneID now (validator, e_join) instead of a bare pubkey, touching committee membership, avail WAL persistence, producer lifecycle, and the wire format) — well outside the scope for auto-approval regardless of this run's findings.
No new bugs were found this run. Beyond the reported findings, I also checked three additional candidates and ruled them out: a hang in Block/PushBlock/WaitForLocalCapacity if a lane is deleted mid-wait (the deployment model in this repo means ApplyEpoch/epoch transitions have no production callers yet, so this is dormant); tryPruneLeaveLanes turning a missing registry epoch into a NewState startup-abort (registry only holds genesis today, so unreachable until epoch persistence lands); and unbounded epoch registry growth (bounded in practice since nothing currently activates new epochs).
Extended reasoning...
Given the volume of prior findings on this thread (several genuine correctness gaps around leave-lane pruning windows, restart handling of in-flight leave WALs, and WAL/wire format compatibility flagged by other reviewers) and the fact that this changes core consensus lane-identity semantics, a human should review this rather than relying on automated approval.
3c335fa to
fcee025
Compare
LaneID pairs validator with join epoch so leave/rejoin does not collide with a recycled validator slot. TipEpoch leave prunes maps on the same tick as DeleteLane; Subscribe fails with ErrLanePruned after drop. Co-authored-by: Cursor <cursoragent@cursor.com>
26a7c0b to
d37679b
Compare
There was a problem hiding this comment.
Solid, well-documented reworking of LaneID into (validator, e_join) with good coverage on the persist/DeleteLane paths, but two blocking issues remain: PushBlock/PushVote don't re-check lane presence after waiting (the exact hazard the rest of the PR carefully fixes elsewhere), and four unrelated QC-verification tests were deleted with no replacement, leaving PrepareQC.Verify and AppQC.Verify weight/epoch-binding coverage at zero.
Findings: 4 blocking | 11 non-blocking | 8 posted inline
Blockers
- Unexplained test deletions in
sei-tendermint/autobahn/types/committee_test.go:TestPrepareQCVerifyChecksWeight,TestPrepareQCVerifyChecksEpochBinding,TestAppQCVerifyChecksWeight, andTestNewCommittee_RejectsEmptyWeightsare removed with no replacement.TestCommitQCVerifyChecksWeightwas not added — the old one was deleted and the PrepareQC test renamed into its place. After this PR there is no test anywhere inautobahn/typesexercisingPrepareQC.Verify, and none exercisingAppQC.Verifyweight thresholds. None of these depend onLaneID, so nothing in this change requires dropping them. Please restore them (mechanically updating tocommittee.Lane(...)where needed). - 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output, so this review reflects only Claude + Codex findings.inner.prune(c *types.Committee, ...)(inner.go:228) never referencesc— its body iteratesi.votes. This PR now threads a carefully computedanchorCommitteeinto that ignored parameter, which reads as if the anchor committee scopes the prune when it does not. Either use it or drop the parameter.- Stale-leave pruning only runs when
collectPersistBatch'sWaitUntilpredicate fires (new blocks / new commitQC / new appQC). A tip-stale leave lane with nothing pending and an otherwise idle chain lingers ininner.blocksand on disk until unrelated activity wakes the loop. Consider addingstaleLaneDisposableto the wake predicate. PushCommitQCstill hard-rejects QCs whoseEpochIndexdiffers from the applied epoch (state.go:435). OnceApplyEpochhas production callers, in-flight prior-epoch QCs will surface as stream errors to peers during a transition. The PR body defers this to #3736 — worth aTODO(#3736)at that comparison so it isn't lost.NewBlockPersisterlogs and skips lane dirs whose names don't parse as the new 80-hexLaneID(old 64-hex pubkey dirs). Those directories are then never reclaimed. Given the PR requires a state wipe this is acceptable, but the skip path should say the entry leaks rather than implying it's handled.LaneVotesRecv.next(subscriptions.go:55) is keyed byLaneIDand never has entries removed whendropLanesfires, so it accumulates one entry per historical lane over the node's lifetime. Negligible in size, but easy to clean up alongsidedropLanes.- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| func (s *State) PushBlock(ctx context.Context, p *types.Signed[*types.LaneProposal]) error { | ||
| h := p.Msg().Block().Header() | ||
| if p.Key() != h.Lane() { | ||
| if p.Key() != h.Lane().Validator() { |
There was a problem hiding this comment.
[blocker] PushBlock captures q := inner.blocks[h.Lane()] before ctrl.WaitUntil (l.587–595) and never re-checks the lane afterwards — the same hazard Block, headers, and WaitForLocalCapacity were all restructured to handle in this PR.
After pruneStaleLeave → dropLanes removes the lane, inner.persistedBlockStart[h.Lane()] reads back as the zero value, so the predicate becomes h.BlockNumber() <= min(q.next, BlocksPerLane-1):
- For
h.BlockNumber() >= BlocksPerLanethe predicate is permanently false. The waiter re-evaluates on every unrelatedctrl.Updated()and only exits when the peer stream's ctx is cancelled. - For low block numbers the predicate passes and the block is pushed into a queue that is no longer reachable from
inner.blocks— silently dropped, plus a spuriousctrl.Updated().
Move the inner.blocks[h.Lane()] lookup into the predicate (return true when absent) and re-check for ErrBadLane after the wait, mirroring WaitForLocalCapacity. (Also raised by Codex.)
There was a problem hiding this comment.
Fixed: PushBlock re-checks inner.blocks in the WaitUntil predicate and after wait (return ErrBadLane if the map was tip-pruned), matching Block / WaitForLocalCapacity.
| q.pushBack(newBlockVotes()) | ||
| } | ||
| if _, ok := q.q[h.BlockNumber()].pushVote(inner.epoch, vote); ok { | ||
| if _, ok := q.q[h.BlockNumber()].pushVote(inner.epoch.Load(), vote); ok { |
There was a problem hiding this comment.
[blocker] Same defect as PushBlock: q := inner.votes[h.Lane()] is captured at l.640 before ctrl.WaitUntil (l.644–648), and dropLanes can remove the lane while the caller waits. inner.persistedBlockStart[h.Lane()] then reads 0, so the predicate h.BlockNumber() < 0 + BlocksPerLane is permanently false for any block at or above BlocksPerLane — the waiter never exits until ctx cancellation — and below that it appends votes to a detached queue.
Fold the lane lookup into the predicate and return ErrBadLane after the wait when the lane is gone.
There was a problem hiding this comment.
Fixed: PushVote likewise wakes and returns ErrBadLane if tipEpoch drop removes the vote map mid-wait.
| } | ||
|
|
||
| func TestPrepareQCVerifyChecksWeight(t *testing.T) { | ||
| func TestCommitQCVerifyChecksWeight(t *testing.T) { |
There was a problem hiding this comment.
[blocker] This is the old TestPrepareQCVerifyChecksWeight rewritten to CommitQC, while the pre-existing TestCommitQCVerifyChecksWeight was deleted below — so this is a rename, not new coverage, and the PrepareQC weight check is now untested. Together with the deletions of TestPrepareQCVerifyChecksEpochBinding, TestAppQCVerifyChecksWeight, and TestNewCommittee_RejectsEmptyWeights, PrepareQC.Verify has no test left anywhere in this package. None of these depend on LaneID; please restore them.
| return err | ||
| } | ||
|
|
||
| g, gctx := errgroup.WithContext(ctx) |
There was a problem hiding this comment.
[suggestion] sei-tendermint/AGENTS.md says to use libs/utils/scope.Run for structured concurrency; errgroup appears nowhere else under internal/autobahn and is introduced only here. scope.Run + scope.SpawnBg (which the new test in this same PR already uses) expresses "run the session until this background watcher fires" directly, and avoids returning context.Canceled as a control-flow sentinel from the WaitMustStop goroutine — a value that then has to be unwound by utils.IgnoreCancel(g.Wait()) and by sessionOpErr.
| // so concurrent calls on the same lane serialize correctly. | ||
| func (bp *BlockPersister) MaybePruneAndPersistLane( | ||
| lane types.LaneID, | ||
| active *types.Committee, |
There was a problem hiding this comment.
[suggestion] Threading a whole *types.Committee into the WAL layer only to compute allowCreate := active.HasLane(lane) || len(proposals) > 0 pushes membership policy down into persistence. An allowCreate bool (decided in avail, which already holds the applied committee) would keep the decision at the layer that owns it, remove the nil-active panic hazard on active.HasLane in an exported API, and drop the committeeForLane scaffolding that ~12 test call sites now need.
There was a problem hiding this comment.
Fixed: MaybePruneAndPersistLane now takes allowCreate bool; avail decides active.HasLane(lane) || len(proposals) > 0.
| anchorCommittee = ep.Committee() | ||
| } | ||
| for lane := range l.blocks { | ||
| if anchorCommittee != nil && lane.EJoin() <= anchorEpoch && !anchorCommittee.HasLane(lane) { |
There was a problem hiding this comment.
[nit] This uses lane.EJoin() <= anchorEpoch while staleLaneDisposable (state.go:121) uses lane.EJoin() < ep.EpochIndex() for what the comments describe as the same predicate. They're equivalent given the invariant that e_join == e implies membership in epoch e, but the asymmetry looks like a typo. Align them or state the invariant that makes <= safe here.
| blocksByLane[lane] = append(blocksByLane[lane], proposal) | ||
| } | ||
|
|
||
| active := s.epoch.Load().Committee() |
There was a problem hiding this comment.
[nit] active is read outside the inner lock, while batch was collected under it. ApplyEpoch can install a newer committee in between, flipping allowCreate for a lane whose blocks were collected against the older one. Benign today (worst case is a WAL that isn't created and gets deleted anyway), but snapshotting the committee inside collectPersistBatch alongside tipEpoch would make the batch self-consistent.
| vs := slices.Collect(maps.Keys(weights)) | ||
| slices.SortFunc(vs, PublicKey.Compare) | ||
| for _, v := range vs { | ||
| lanes = append(lanes, NewLaneID(v, GenEpochIndex(rng))) |
There was a problem hiding this comment.
[nit] Giving each member an independent random e_join produces committees that cannot occur in production — e_join can exceed the index of the epoch the committee is installed in. That's fine for round-tripping LaneIDConv, but it means staleLaneDisposable-style logic is never meaningfully exercised by any test built on GenCommittee, and it quietly breaks the e_join <= epochIndex invariant. Consider taking the epoch as a parameter (or bounding e_join by it) and keeping a separate generator for the deliberately-mixed case.
Drop *Committee from MaybePruneAndPersistLane; wake on tipEpoch map drop in PushBlock/PushVote via waitLaneBound; restore Prepare/App/empty committee coverage; tag multi-epoch follow-ups as TODO(#3736). Co-authored-by: Cursor <cursoragent@cursor.com>
…ain into wen/lane_id_in_epoch
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 0fddc55. Configure here.
Document stay/leave/rejoin on LaneID and map/tip dispose on avail. Inline tipEpoch-aware waits; clarify restart tipcut <= vs live <. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Solid, well-tested refactor of LaneID from PublicKey to (validator, e_join), with coherent leave/rejoin lifecycle across avail, persist, producer, and giga; the wire/state break is explicitly scoped and the wireguard MaxSize bumps line up with the RPC limits. No blocking correctness defects found — the main issues are an acknowledged-but-permanent leave-WAL leak on restart, a forward-looking startup dependency on registry epoch history that isn't persisted yet, and a few idiom/comment-style deviations from AGENTS.md.
Findings: 0 blocking | 11 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass (
cursor-review.md) is empty — no findings were merged from it. Codex's single P2 finding is included below (inner.go stale-lane WAL leak) and I confirmed it. - Operator-facing hard break (WAL dirs
hex(pubkey)→hex(pubkey||e_join),BlockHeader/LaneRangewire change, block-hash change) is documented only in the PR description. Consider recording it in a package doc comment orsei-tendermint/AGENTS.mdso the "wipepersistent_state_dirbefore upgrade" requirement survives the PR being merged. - Client-side asymmetry: the PR makes the giga server pause-and-resubscribe on lane pruning without tearing down peer RPC, but
clientStreamLaneProposals(giga/avail.go:191) still bubbles anyPushBlockerror — includingErrBadLanefrom transient epoch skew between peers — which drops the whole connection and forces aDialIntervalredial. Worth revisiting when #3736 wires real epoch transitions. - Several new comments are dense internal shorthand ("back-leash", "tipcut", "leave map", "avail passes true") that a reader new to the epoch design can't follow top-to-bottom, which is what
AGENTS.md§Structural corrections asks the step name + doc comment to carry.state.go:125andstate.go:136also start the sentence with a capitalized verb mid-phrase ("deleteStaleLaneWAL Deletes WALs…"); godoc convention is "deleteStaleLaneWAL deletes …". - 7 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| for lane := range l.blocks { | ||
| if anchorCommittee != nil && lane.EJoin() <= anchorEpoch && !anchorCommittee.HasLane(lane) { | ||
| continue |
There was a problem hiding this comment.
[suggestion] Tip-stale leave lanes skipped here never get their WAL deleted, so the directory (and the exclusive file lock NewBlockPersister takes on it at open) leaks permanently across restarts.
Every path that calls DeleteLane is driven off inner.blocks: NewState's startup truncation loop iterates for lane := range inner.blocks, and collectPersistBatch builds staleLeave from for lane, q := range inner.blocks. A lane continued here is in neither, so it is re-opened by NewBlockPersister on every subsequent boot and never reclaimed.
Concretely: crash after pers.pruneAnchor.Persist but before pruneStaleLeave's DeleteLane, and the leaver's WAL is on disk forever until the operator wipes persistent_state_dir. TestNewInnerSkipsStaleLaneAbsentFromAnchor documents this ("orphan WAL dirs may remain unused on disk"), but it is unbounded in validator churn rather than one-shot.
Cheapest fix: don't skip — re-attach these lanes like any other, and let the existing staleLaneDisposable → pruneStaleLeave path delete them on the first persist tick. Alternatively collect the skipped LaneIDs and DeleteLane them in NewState. (Also raised by the Codex pass.)
| anchorEpoch = anchor.CommitQC.Proposal().EpochIndex() | ||
| ep, ok := registry.EpochByIndex(anchorEpoch) | ||
| if !ok { | ||
| return nil, fmt.Errorf("unknown epoch_index %d for prune anchor", anchorEpoch) |
There was a problem hiding this comment.
[suggestion] This turns a previously-tolerant path into a hard startup failure that depends on state the node does not persist yet. Before this PR, prune was called with epoch.Committee() (latest) regardless; now an anchor whose EpochIndex() is absent from the registry aborts NewState. tipEpochOf (state.go:109-111) has the same shape and kills runPersist/avail.Run.
NewRegistry seeds only epoch 0 and there is no epoch-history persistence, so once #3736 wires real transitions, any node restarting after an epoch bump will have an anchor at epoch N>0 and refuse to start. Harmless today (anchors are always epoch 0), but please make "registry epoch history must be rebuilt before NewState" an explicit prerequisite in #3736 — right now nothing in the tree records the coupling.
| anchorCommittee = ep.Committee() | ||
| } | ||
| for lane := range l.blocks { | ||
| if anchorCommittee != nil && lane.EJoin() <= anchorEpoch && !anchorCommittee.HasLane(lane) { |
There was a problem hiding this comment.
[nit] The comment claims this is "the same rule as staleLaneDisposable", but the predicates differ: e_join <= anchorEpoch here vs e_join < ep.EpochIndex() in staleLaneDisposable (state.go:122). They coincide only under the invariant "membership at epoch e implies e_join <= e" — which GenCommittee deliberately violates in tests (see types/testonly.go:96, independent random e_join per member). Either use the same expression in both places, or state the invariant as the reason for the difference rather than asserting they're the same rule.
| if err != nil { | ||
| if errors.Is(err, avail.ErrLanePruned) { | ||
| logger.Info("StreamLaneProposals: leave-lane tipcut pruned; pausing until resubscribe") | ||
| first = 0 |
There was a problem hiding this comment.
[nit] Two things on the resubscribe path:
-
No backoff. If
ErrLanePrunedwere ever to recur immediately after resubscribe, this is a tight loop that also emits anInfolog per iteration. My reading is that it isn't reachable today (ApplyEpochadds the map beforeStore, anddropLanesonly touches lanes absent from the tip committee), but the comment onsubscriptions.goexplicitly admits a "DeleteLane race" as a source — if that race is real, this spins. -
first = 0replays from block 0. For a genuine rejoin that's correct (new LaneID starts at 0). For the race case,LaneProposalsRecv.Recvwalkstypes.ErrPrunedone block at a time up toq.first, taking the inner lock per step. Cheap on a fresh lane, O(pruned height) otherwise.
| return err | ||
| } | ||
|
|
||
| g, gctx := errgroup.WithContext(ctx) |
There was a problem hiding this comment.
[suggestion] This introduces golang.org/x/sync/errgroup plus a return context.Canceled sentinel (line 133) into a package that expresses exactly this shape with scope everywhere else — including produceSession immediately below, which uses scope.Run + scope.Spawn. scope.Run already cancels siblings as soon as any task returns an error, so the same session/cancel structure is expressible without a second concurrency primitive.
Per AGENTS.md §Structural corrections: "check each step against the shapes the surrounding package already uses rather than a pattern introduced for this one change."
Side effect worth noting either way: because WaitMustStop returns the context.Canceled sentinel, if a leave races a real failure in produceSession (e.g. the mempool mismatched block production error at line 201), errgroup returns whichever landed first — so a genuine bug can be swallowed by IgnoreCancel at line 135.
| } | ||
| cur, hasLane := m.lane.Get() | ||
| if !hasLane || cur != local { | ||
| return nil, ErrNotProducing |
There was a problem hiding this comment.
[nit] There's a window on rejoin where LocalLane() is Some(newLane) but m.lane is still None — between WaitProduce returning in Run and alignMempool setting it in produceSession. Inserts land here and get ErrNotProducing even though the node is a producer, so an RPC submitter sees a spurious rejection rather than a brief wait.
The distinguishable cases are worth splitting: LocalLane() is None → genuinely not producing, reject; LocalLane() is Some but m.lane doesn't match yet → wait on ctrl for alignment. (NewState seeds m.lane from LocalLane(), so this only bites after a clearMempool, not at boot.)
| return fmt.Errorf("close lane %s WAL: %w", lane, err) | ||
| } | ||
| path := filepath.Join(dir, laneDir(lane)) | ||
| if err := os.RemoveAll(path); err != nil { |
There was a problem hiding this comment.
[nit] lw.close() and os.RemoveAll run while holding the bp.lanes write lock, so a directory removal blocks every other lane's getLane fast path (which takes RLock) for its whole duration. Since delete(lanes, lane) at line 357 already makes the lane unreachable to new callers, the close+unlink could move outside the lock — the doc comment above already requires that no in-flight MaybePruneAndPersistLane overlaps this lane.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Solid, well-scoped refactor of LaneID from PublicKey to (validator, e_join) with matching proto/wireguard regeneration, lane-map lifecycle (join/leave/rejoin), and producer session handling; I found no correctness blockers, but a few robustness/ordering nits and a gap in direct test coverage of the new tipEpoch stale-leave prune path.
Findings: 0 blocking | 9 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (
cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's findings with Codex's two. - Test-coverage gap on the core new mechanism:
tipEpochOf/staleLaneDisposable/ thecollectPersistBatch→pruneStaleLeavetick have no direct test.TestSubscribeLaneProposals_ErrLanePrunedAfterMapDropreaches intostate.innerand callsdropLanesby hand, so the actual disposal predicate and the WAL-delete-then-map-drop ordering inrunPersistare never exercised end to end. A test that pushes a CommitQC/AppQC pair whose tip epoch omits a leave lane and then asserts the lane's WAL dir is gone and the map key dropped would cover the part most likely to regress. - Doc-comment style in
avail/state.go,avail/inner.go, andgiga/avail.goleans on undefined project shorthand ("back-leash", "tipcut skip", "tip-stale") and mid-sentence capitalized verbs ("pruneStaleLeave Deletes WALs…", "collectPersistBatch … Deletes their WALs"). AGENTS.md asks the doc comment to carry the why readably for a new engineer; the package doc onavaildoes this well, but the per-function comments assume the reader already has the package doc in their head. - The hard break for autobahn
persistent_state_dir(WAL dirs move fromhex(pubkey)tohex(pubkey||e_join), and legacy dirs are warn-and-skipped and leak until an operator wipe) is documented only in the PR body. Worth landing an operator-facing note next to whereverpersistent_state_diris documented, since the PR body is not discoverable after merge. - Verified as fine, noting so it isn't re-flagged: all bumped
MaxSize()values still fit under theirrpc.MsgMsgSizecaps ingiga/api.go(CommitQC 21242 < 22 kB, StreamAppQCsResp 31718 < 32 kB, FullCommitQC 152246 < 300 kB, LaneProposal/GetBlockResp ≈2.056 MB < 2 MB… — LaneProposal 2056293 < 2097152 ✓),NewRoundRobinElectionhas no remaining references, andProposal.LaneRange(lane)returns a zero range carrying the requested lane, soinner.prune'si.votes[lr.Lane()]cannot hit a missing key for a retained leave lane. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| if !ok { | ||
| return nil | ||
| } | ||
| delete(lanes, lane) |
There was a problem hiding this comment.
[suggestion] The map entry is removed before close() and os.RemoveAll() run, so a failure in either leaves the lane unrecoverable from inside this process: the entry is gone, a retry hits the !ok early return and no-ops, and the fd/lock (on close failure) or the directory (on RemoveAll failure) leaks until restart.
In practice the error propagates out of pruneStaleLeave → runPersist → avail.Run, so the node is likely coming down anyway — but the ordering is free to fix: do lw.close() and os.RemoveAll(path) first and delete(lanes, lane) only after both succeed. (Same point as Codex P2.)
| for m, ctrl := range s.mempool.Lock() { | ||
| if waitIfFull { | ||
| for { | ||
| local, ok := s.consensus.Avail().LocalLane().Get() |
There was a problem hiding this comment.
[suggestion] The producing check runs only after CheckTxSafe (line 144). Two consequences after a leave:
- An invalid tx returns the application's
CheckTxresponse at line 149 instead ofErrNotProducing, so the caller sees a rejection reason unrelated to the stated "not producing" contract — andTestProducer_LeaveCancelsAndRejoinStartsNewLaneonly assertsErrNotProducingfor a valid tx, so this path is untested. - Every rejected tx still pays for a full
CheckTxSafeexecution against app state.
Suggest an early LocalLane() check before CheckTxSafe, keeping this in-lock check as the race guard (the mempool lane can still change between the two). Same as Codex P1.
| if err := availState.WaitMustStop(gctx, lane); err != nil { | ||
| return err | ||
| } | ||
| return context.Canceled |
There was a problem hiding this comment.
[nit] errgroup returns the first non-nil error. When a leave happens, this goroutine returns context.Canceled, which cancels gctx and races produceSession's return — if this one lands first, a genuine error from produceSession is discarded by utils.IgnoreCancel and Run silently loops back to WaitProduce instead of failing.
sessionOpErr already handles the expected leave case by converting ErrBadLane → Canceled, so this watchdog's job is only to cancel, not to decide the outcome. Consider capturing produceSession's error in a variable and returning that, using the watchdog purely for cancellation.
| ) | ||
| // TODO: use the committee of the anchor's epoch once epoch transitions are wired up. | ||
| if _, err := i.prune(epoch.Committee(), anchor.AppQC, anchor.CommitQC); err != nil { | ||
| if _, err := i.prune(anchorCommittee, anchor.AppQC, anchor.CommitQC); err != nil { |
There was a problem hiding this comment.
[nit] This swaps epoch.Committee() for anchorCommittee and drops the TODO above it ("use the committee of the anchor's epoch once epoch transitions are wired up"), but inner.prune never reads its c *types.Committee parameter — the body only uses appQC/commitQC and iterates i.votes. So the call change is a no-op and the TODO it retires isn't actually addressed.
Either wire the committee into prune (e.g. to scope the lane loop) or drop the unused parameter and keep a TODO that still names the real gap. anchorCommittee remains genuinely load-bearing for the tipcut skip above, so that part stands.
Superseded: latest AI review found no blocking issues.
Key WaitForLocalCapacity off blocks so a zero start is not treated as prune. Drive produce leave cancel with scope.Run instead of errgroup. Co-authored-by: Cursor <cursoragent@cursor.com>

Summary
LaneID = PublicKeywithLaneID = (validator, e_join): stay keepse_join, leave is terminal for that identity, rejoin allocates a new LaneID (tip fromNextBlock, typically 0 for a fresh map).ApplyEpochseeds joiner lane maps; leavers stay in memory/WAL until tipEpoch (first retained CommitQC) omits them (staleLaneDisposable:e_join < tipand not in tip committee), thenDeleteLane+ map drop. Persist still flushes leave tips when proposals are non-empty before the first WAL open (allowCreatedecided in avail, not persist).WaitProduce/WaitMustStop; leave clears mempool and rejects inserts (ErrNotProducing).SubscribeLaneProposalsbinds lane at subscribe and keeps serving until tipEpoch prune (ErrLanePruned); giga pauses and resubscribes without tearing down peer RPC.Compatibility / ops
persistent_state_dir: WAL dirs arehex(pubkey||e_join)(washex(pubkey)), andBlockHeader/LaneRangewire the newLaneIDmessage (field 5; old field 1lanereserved). Pre-LaneID state does not migrate — wipe or coordinated reset before upgrade. Autobahn is opt-in / pre-launch; no mixed-version peers.Multi-epoch (#3736)
Production
ApplyEpoch/ActivateEpochwiring, neighborVerifyInWindow, and accepting prior-epochCommitQCwhile tip lags land in #3736. This PR ships LaneID + leave/rejoin scaffolding and unit coverage only; do not expect end-to-end multi-epoch production paths here.Made with Cursor