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 29 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.
| } | ||
|
|
||
| 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.
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.
| 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
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
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.)
| 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.
| } | ||
|
|
||
| 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.
| 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.
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.
| 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.
| 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.
| } | ||
| 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.)
The Lock range variable must not reuse m; produce waits on the session mempoolInner returned by alignMempool. Co-authored-by: Cursor <cursoragent@cursor.com>
4c990c0 to
fefd1a1
Compare
| func hasClosedLane(inner *inner, ds *data.State) bool { | ||
| epOfFirst, err := epochOfFirst(inner, ds) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Closed-lane cleanup can stall persist
Medium Severity
When the first retained CommitQC is gone, epochOfFirst treats a missing registry epoch as absent rather than failing, unlike the in-memory roads path. hasClosedLane also maps that error to false, so collectPersistBatch can wait indefinitely instead of aborting, and closed-lane maps and WALs are never dropped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fefd1a1. Configure here.
| func hasClosedLane(inner *inner, ds *data.State) bool { | ||
| epOfFirst, err := epochOfFirst(inner, ds) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Closed-lane cleanup can stall persist
Medium Severity
When the first retained CommitQC is gone, epochOfFirst treats a missing registry epoch as absent rather than failing, unlike the in-memory roads path. hasClosedLane also maps that error to false, so collectPersistBatch can wait indefinitely instead of aborting, and closed-lane maps and WALs are never dropped.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fefd1a1. Configure here.
Rename WaitForNextLane / ErrLaneClosed / setNextBlockToPersist, document future-lane behavior, simplify giga client wait on current membership, and tidy producer/avail nits from review. Co-authored-by: Cursor <cursoragent@cursor.com>
Use roads[first].epoch when retained CommitQCs exist, otherwise inner.epoch — no registry lookup or Option error path. Co-authored-by: Cursor <cursoragent@cursor.com>
Deletion follows the Anchor epoch: prune takes that epoch from the matching road (registry once at startup), and collectPersistBatch no longer wakes or drops on hasClosedLane. Co-authored-by: Cursor <cursoragent@cursor.com>
Accept LaneVotes under applied or Anchor; weigh LaneQCs under applied. Pass the road epoch into headers and abort on IsClosed for that epoch. Co-authored-by: Cursor <cursoragent@cursor.com>
PruneAndPersist matches main again (BlockNumber, always create). runEvict only drops maps; runPersist SyncLanes is the sole runtime WAL deleter. Co-authored-by: Cursor <cursoragent@cursor.com>
Record LaneQC vs FullCommitQC vote retention, note WaitForCapacity does not wait on future lanes, and drop design history from the Consensus RPC. Co-authored-by: Cursor <cursoragent@cursor.com>
| } | ||
| inner.prune(anchor, r.epoch) | ||
| } | ||
| ctrl.Updated() |
There was a problem hiding this comment.
Evict dies when Anchor road is missing
High Severity
runEvict now requires inner.roads.q[idx] and returns a fatal error when the data Anchor sits at or beyond an empty tip (first == next after a tip prune, or data has moved ahead of avail). That used to fast-forward via prune and resolve the epoch from the registry, like newInner still does. The error tears down State.Run, so closed-lane maps are never dropped and subscribers never see ErrLaneClosed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 95a9783. Configure here.
State.mempool is an atomic.Pointer to a per-session Watch[*mempoolInner] with a closed flag, so produce/prune no longer use lock-protected state outside Lock. insertTx waits only on full/closed. Co-authored-by: Cursor <cursoragent@cursor.com>
| } | ||
| // Session lane must still be this node's LocalLane. | ||
| if loc, ok := s.consensus.Avail().LocalLane().Get(); !ok || loc != m.lane { | ||
| return nil, ErrNotProducing |
There was a problem hiding this comment.
InsertTx no longer waits for alignment
Medium Severity
InsertTx now returns ErrNotProducing as soon as mempool is nil or the loaded session’s lane does not match LocalLane. The previous logic waited, when waitIfFull was set, until a session mempool existed for the current LocalLane. Switching State.mempool to atomic.Pointer removed that wait, so a node that is already in committee can reject txs during startup and leave/rejoin while alignMempool has not yet stored the new session.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit befa73d. Configure here.
Prefer the sealed road's epoch while it is retained; fall back to the registry once it is not, so empty-tip / data-ahead Anchors still prune instead of tearing down State.Run. Co-authored-by: Cursor <cursoragent@cursor.com>
Use AtomicSend so inserts can wait until a session is published, restoring the pre-reshape split where InsertTx blocks and TryInsertTx fails fast. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7ff48f4. Configure here.
| if _, ok := s.consensus.Avail().LocalLane().Get(); !wait || !ok { | ||
| return nil, ErrNotProducing | ||
| } | ||
| return s.mempool.Wait(ctx, func(mp *mempool) bool { return mp != nil }) |
There was a problem hiding this comment.
InsertTx wait ignores lane leave
Medium Severity
session snapshots LocalLane once, then Waits only for a non-nil mempool. If the node leaves (or production ends) after that check, clearMempool stores nil and waiters never see ErrNotProducing — InsertTx blocks until its context is cancelled instead of failing like the leave path that already holds a session.
Reviewed by Cursor Bugbot for commit 7ff48f4. Configure here.
Avoid the peer struct; shuffle ValidatorAddrs keys and look up the dial addr, matching the review nit. Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
||
| // anchorEpochOf returns the epoch of anchor's CommitQC, taken from the road it | ||
| // sealed while that road is retained and from the registry once it is not. | ||
| func (i *inner) anchorEpochOf(registry *epoch.Registry, anchor data.Anchor) (*types.Epoch, error) { |
There was a problem hiding this comment.
why not go straight to fallback? My previous comment was referring to the previous semantics, where epoch of the first CommitQC was requested, which is not the same as anchor epoch.
| // epoch is the applied (next-CommitQC) epoch. ApplyEpoch is the sole | ||
| // writer after construction. | ||
| epoch utils.AtomicSend[*types.Epoch] | ||
| // anchorEpoch is the epoch of data's Anchor CommitQC. |
There was a problem hiding this comment.
what if there is no anchor yet?
| // Package avail is the Data Availability Plane and Ordered Event Log: lane | ||
| // blocks, CommitQC/AppQC buffers, and pruning. | ||
| // | ||
| // Lane maps and WALs outlive committee membership until the anchor epoch |
There was a problem hiding this comment.
this should be State's godoc?
|
|
||
| // headers collects headers for the given range. | ||
| func (s *State) headers(ctx context.Context, lr *types.LaneRange) ([]*types.BlockHeader, error) { | ||
| // laneVoteAccepted reports whether vote verifies under ep's committee. |
There was a problem hiding this comment.
add a TODO to actually return a meaningful validation error, in case vote does not match either epoch, and in case verification fails despite the epoch matching
| // consensus state to which published blocks will be reported. | ||
| cfg *Config | ||
| app *proxy.Proxy | ||
| mempool utils.AtomicSend[*mempool] // nil when not producing |
There was a problem hiding this comment.
AtomicSend[Option[*mempool]]
| // session returns the produce session to insert into. With wait set it blocks | ||
| // until a session is published, so that inserts racing the start of production | ||
| // are admitted rather than rejected. | ||
| func (s *State) session(ctx context.Context, wait bool) (*mempool, error) { |
There was a problem hiding this comment.
actually it doesn't even make sense to call a wrapper in case wait == false, so perhaps inline?
| if m.closed { | ||
| return nil, ErrNotProducing | ||
| } | ||
| // Session lane must still be this node's LocalLane. |
There was a problem hiding this comment.
do we really need this check?
| if m.closed { | ||
| return nil, ErrNotProducing | ||
| } | ||
| if loc, ok := s.consensus.Avail().LocalLane().Get(); !ok || loc != m.lane { |
There was a problem hiding this comment.
ditto. We just grabbed a mempool and we should be good to go while at it. It is not like we double confirm if the shard matches.
session() wait ends if LocalLane is gone, and clearMempool always publishes nil so waiters re-evaluate instead of hanging until ctx cancel. Co-authored-by: Cursor <cursoragent@cursor.com>
Move lane-lifecycle notes onto State (keeping the original godoc) and TODO a real validation error for dual-epoch PushVote rejects. Co-authored-by: Cursor <cursoragent@cursor.com>
Publish produce sessions as AtomicSend[Option]; wait only via getMempool. Hoist headers IsClosed out of the wait loop under a fixed epoch. Co-authored-by: Cursor <cursoragent@cursor.com>


Summary
LaneID = PublicKeywithLaneID = (Validator, Joined): stay keepsJoined; leave ends that identity; rejoin allocates a new LaneID withJoinedset to the join epoch.availdoc):ApplyEpoch)epochOfFirstepochOfFirst.IsClosed→ maps dropped;SyncLanesdeletes the WALallowCreatedecided in avail (active committee lane, or non-empty proposals so a leaver still flushes);SyncLanesis race-safe withMaybePruneAndPersistLane(map lock held across persist/delete).alignMempool/clearMempool(runMempool); sessions areWaitForLocalLane→ produce untilWaitUntilClosed.InsertTxwaits for mempool align whenLocalLaneis already set;TryInsertTxfails fast; leave clears mempool and yieldsErrNotProducing.StreamLaneProposals: bind by LaneID; server ends cleanly onErrLaneClosed; client resumes fromNextBlockand only excludes the old LaneID when the applied committee has dropped/replaced it (transient disconnect reconnects to the same identity).Compatibility
Autobahn has not launched. There is no production Autobahn state to migrate, and this PR does not attempt backward compatibility for persisted CommitQC/Block WALs, littblock records, or the wire encoding of
BlockHeader/LaneRange(lane→lane_id). Nodes must wipepersistent_state_dir(including blockdb) / do a coordinated reset when taking this change. Opaque decode failures on old epochs are acceptable in that context; a littblock serialization-version bump is not required for launch readiness.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.Test note
TestMempool_EvmTxByHash: setMaxTxsPerBlock = 1so a 1msBlockIntervalsealing between sequentialInsertTxcalls no longer tripsenv.Run's "block sealed too early" check (pre-existing flake under-race).Made with Cursor