Skip to content

feat(autobahn): implement new lane ID for epoch (CON-358) - #3862

Open
wen-coding wants to merge 6 commits into
mainfrom
wen/lane_id_in_epoch
Open

feat(autobahn): implement new lane ID for epoch (CON-358)#3862
wen-coding wants to merge 6 commits into
mainfrom
wen/lane_id_in_epoch

Conversation

@wen-coding

@wen-coding wen-coding commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replace LaneID = PublicKey with LaneID = (validator, e_join): stay keeps e_join, leave is terminal for that identity, rejoin allocates a new LaneID (tip from NextBlock, typically 0 for a fresh map).
  • ApplyEpoch seeds joiner lane maps; leavers stay in memory/WAL until tipEpoch (first retained CommitQC) omits them (staleLaneDisposable: e_join < tip and not in tip committee), then DeleteLane + map drop. Persist still flushes leave tips when proposals are non-empty before the first WAL open (allowCreate decided in avail, not persist).
  • Producer sessions: WaitProduce / WaitMustStop; leave clears mempool and rejects inserts (ErrNotProducing). SubscribeLaneProposals binds lane at subscribe and keeps serving until tipEpoch prune (ErrLanePruned); giga pauses and resubscribes without tearing down peer RPC.

Compatibility / ops

  • Hard break for autobahn persistent_state_dir: WAL dirs are hex(pubkey||e_join) (was hex(pubkey)), and BlockHeader / LaneRange wire the new LaneID message (field 5; old field 1 lane reserved). Pre-LaneID state does not migrate — wipe or coordinated reset before upgrade. Autobahn is opt-in / pre-launch; no mixed-version peers.
  • Invalid/legacy lane dirs are warn-and-skipped and leak until wipe (intentional for this PR).

Multi-epoch (#3736)

Production ApplyEpoch / ActivateEpoch wiring, neighbor VerifyInWindow, and accepting prior-epoch CommitQC while 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

Comment thread sei-tendermint/internal/autobahn/consensus/persist/blocks.go
Comment thread sei-tendermint/internal/autobahn/avail/inner.go Outdated
@wen-coding
wen-coding force-pushed the wen/lane_id_in_epoch branch from 8d254d4 to 4f52a00 Compare August 5, 2026 22:23
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedAug 9, 2026, 5:00 PM

Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.97452% with 132 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.76%. Comparing base (a115971) to head (a4db9b8).

Files with missing lines Patch % Lines
sei-tendermint/internal/autobahn/avail/state.go 62.20% 26 Missing and 22 partials ⚠️
...dermint/internal/autobahn/pb/autobahn.wireguard.go 37.50% 15 Missing ⚠️
sei-tendermint/internal/p2p/giga/avail.go 44.44% 13 Missing and 2 partials ⚠️
sei-tendermint/internal/autobahn/producer/state.go 77.58% 10 Missing and 3 partials ⚠️
...mint/internal/autobahn/consensus/persist/blocks.go 66.66% 7 Missing and 4 partials ⚠️
sei-tendermint/internal/autobahn/avail/inner.go 86.53% 3 Missing and 4 partials ⚠️
...i-tendermint/internal/autobahn/producer/mempool.go 61.11% 4 Missing and 3 partials ⚠️
sei-tendermint/internal/autobahn/epoch/registry.go 66.66% 3 Missing and 2 partials ⚠️
sei-tendermint/autobahn/types/committee.go 92.00% 2 Missing and 2 partials ⚠️
sei-tendermint/autobahn/types/lane_id.go 94.59% 1 Missing and 1 partial ⚠️
... and 3 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3862      +/-   ##
==========================================
- Coverage   61.61%   60.76%   -0.85%     
==========================================
  Files        2348     2255      -93     
  Lines      200852   190691   -10161     
==========================================
- Hits       123755   115880    -7875     
+ Misses      66044    64566    -1478     
+ Partials    11053    10245     -808     
Flag Coverage Δ
sei-chain-pr 73.41% <71.97%> (?)
sei-db 70.41% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sei-db/ledger_db/block/blocksim/blocksim.go 7.47% <100.00%> (-1.56%) ⬇️
sei-tendermint/autobahn/types/block.go 83.33% <100.00%> (ø)
sei-tendermint/autobahn/types/proposal.go 92.07% <100.00%> (ø)
...endermint/internal/autobahn/avail/subscriptions.go 100.00% <100.00%> (ø)
...i-tendermint/internal/p2p/giga/pb/api.wireguard.go 100.00% <100.00%> (ø)
...ndermint/internal/autobahn/pb/autobahn.hashable.go 0.00% <0.00%> (ø)
sei-tendermint/autobahn/types/lane_id.go 94.59% <94.59%> (ø)
sei-tendermint/autobahn/types/testonly.go 94.95% <80.00%> (-0.70%) ⬇️
sei-tendermint/internal/autobahn/avail/testonly.go 72.22% <33.33%> (-2.78%) ⬇️
sei-tendermint/autobahn/types/committee.go 94.94% <92.00%> (-3.54%) ⬇️
... and 8 more

... and 94 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread sei-tendermint/internal/p2p/giga/avail.go Outdated
seidroid[bot]
seidroid Bot previously requested changes Aug 5, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • pruneInactiveLanes deletes entries from inner.blocks / inner.votes, but three call sites still index those maps without an ok check and will nil-deref once a leaver is pruned: avail/state.go:654 (headers: q := inner.votes[lr.Lane()] then q.first), avail/state.go:815 (PushQC loop: inner.blocks[lr.Lane()].q[n]), and avail/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 from persist/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 tryPruneLeaveLanes and a lagging reader. TestApplyEpoch_AddsJoinerDefersLeaverUntilCommitQCWatermark verifies the leaver's maps/WAL disappear, but nothing exercises headers() / fullCommitQC / the s.data.PushQC loop 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) to hex(pubkey||e_join) (40B), and BlockHeader.lane / LaneRange.lane change proto type from PublicKey to LaneID on the same field number. Both are hard breaks — existing WAL dirs are silently skipped on restart, and peers on the old binary cannot decode. Since ApplyEpoch/ActivateEpoch have no production callers yet, this is presumably pre-launch and acceptable; please confirm explicitly in the PR description rather than leaving it implicit.
  • ApplyEpoch never returns a non-nil error. Either drop the return value or note that it is reserved for the follow-up wiring.
  • tryPruneLeaveLanes re-Stores the identical latestCommitQC value after the disk delete purely to wake waiters. It is safe today only because markCommitQCsPersisted and tryPruneLeaveLanes are both on the runPersist goroutine — worth stating that in the comment, since a concurrent writer would make this a watermark regression. ctrl.Updated() alone may be enough.
  • markBlockPersisted writes inner.nextBlockToPersist[lane] = next unconditionally, so a pruned lane can be resurrected as a stale map entry (small leak, and it makes the map key sets diverge from blocks/votes).
  • LaneProposalsRecv.Recv allocates an errgroup and two goroutines per block received, plus a fresh LocalLaneUpdates() subscription per iteration. On the hot proposal path this is meaningful churn; consider hoisting the lane-change watcher out of the per-block loop.
  • alignMempoolForLane reads NextBlock(lane) before taking the mempool lock, so the tip can be stale by the time it is applied; and a rejoin silently discards all buffered evmTxs/evmNonces. Both are probably intended, but neither is documented.
  • 6 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/subscriptions.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/producer/state.go
Comment thread sei-tendermint/autobahn/types/committee.go
Comment thread sei-tendermint/autobahn/types/committee.go
Comment thread sei-tendermint/autobahn/types/lane_id.go Outdated
Comment thread sei-tendermint/internal/autobahn/consensus/persist/blocks.go
Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/producer/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/subscriptions.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/inner.go Outdated
seidroid[bot]
seidroid Bot previously requested changes Aug 6, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-empty LaneRange for the leaver). TestApplyEpoch_AddsJoinerDefersLeaverUntilAppQCWatermark and TestTryPruneLeaveLanes_OrphanWALWithoutMaps both persist laneB at block 0 with no anchor, so they pass over the bug in newInner. Please add a restart test with an anchor whose leaver LaneRange.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.VerifyInWindow still only checks the latest committee (existing TODO), and PushBlock/PushVote now return ErrBadLane when inner.blocks/inner.votes lacks the lane. A peer that applies epoch N+1 slightly before us and pushes a proposal on its new rejoin LaneID makes clientStreamLaneProposals return an error (giga/avail.go:194), tearing down that peer's RPC stream. The PR carefully protects the local lane via ErrLaneIdentityChanged; remote lanes need the equivalent soft-failure (or a widened verification window) before ApplyEpoch is wired up.
  • ApplyEpoch swaps inner.epoch wholesale, so inner.laneQC and PushVote immediately evaluate already-accumulated old-epoch votes against the new committee's LaneQuorum() and weights. Votes from departed validators are still in the queues and would be assembled into a LaneQC that fails verification against the new committee. The existing // TODO: filter votes per-epoch committee becomes load-bearing once ApplyEpoch is called in production — worth stating explicitly in the #3736 follow-up.
  • The BlockHeader.lane/LaneRange.lane proto type change (PublicKeyLaneID) 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 via autobahn-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 — alignMempoolForLane replaced its only caller. unused isn't enabled in .golangci.yml so it won't fail lint, but it should be removed.
  • types.GenCommittee (testonly.go) now bypasses the public constructors and calls normalizeWeights + finalizeCommittee directly to inject random e_join values. Randomizing e_join in tests is a genuine improvement (it catches code assuming e_join == 0), but duplicating the constructor body in test-only code means future changes to NewCommittee won't be reflected. Consider a newCommitteeWithEJoins(weights, func(PublicKey) EpochIndex) helper shared by all three.
  • State.ApplyEpoch always returns nil. 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.

Comment thread sei-tendermint/internal/autobahn/avail/inner.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/inner.go Outdated
Comment thread sei-tendermint/internal/autobahn/consensus/persist/blocks.go
Comment thread sei-tendermint/internal/autobahn/consensus/persist/blocks.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/subscriptions.go Outdated
Comment thread sei-tendermint/internal/autobahn/producer/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/epoch/registry.go

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/Recv no longer panics on leave+rejoin coalescing - now returns ErrLaneIdentityChanged (avail/subscriptions.go)
  • headers(), the PushQC block-collection loop, and laneQC() now guard missing lane maps instead of nil-dereferencing after a leave prune (avail/state.go, avail/inner.go)
  • produceLocalBlock/streakOpErr now bind to an explicit LaneID per streak, so a stale rejoin race resolves to ErrBadLane/context.Canceled instead of killing producer.Run (producer/state.go)
  • Checked the ruled-out mempool-reset-on-rejoin candidate - alignMempoolForLane only 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.

seidroid[bot]
seidroid Bot previously requested changes Aug 6, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.lane and LaneRange.lane change from PublicKey to the new LaneID message. PublicKey{ed25519: <32 bytes>} and LaneID{validator, e_join} are not wire-compatible, so on an in-place upgrade of an autobahn-enabled node a persisted prune anchor decodes through PruneAnchorConv.Decode -> types.CommitQCConv.Decode -> LaneRangeConv.Decode and fails on the embedded LaneRange. loadPersistedState (sei-tendermint/internal/autobahn/avail/state.go:267) treats that as fatal, so NewState errors and the node will not start until <persistent_state_dir> is wiped. Same for the peer wire format: old and new binaries cannot exchange BlockHeader/LaneRange, and BlockHeader.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 the non-app-hash-breaking label, 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.md is empty), so this review merges only Claude's and Codex's findings.
  • Codex's point, confirmed but downgraded: avail.State.ApplyEpoch, epoch.Registry.ActivateEpoch, and types.ActivateCommittee have no non-test callers, so the entire leave/rejoin/prune path this PR adds is unreachable in production until the advanceEpoch/onAdvance wiring in #3736 lands. That is consistent with the PR being explicitly incremental (the ApplyEpoch doc 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 of tryPruneLeaveLanes rests 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 through producer.Run + runPersist before the wiring PR flips it on.
  • Registry.ActivateEpoch changing RWMutex[registryState] to RWMutex[*registryState] is the right fix (s.latest = next on a value copy would have been lost) -- good catch. Unrelated nit: ActivateEpoch doesn't touch prev's RoadRange, so callers passing OpenRoadRange() for every epoch (as all the new tests do) end up with overlapping open ranges. The doc comment acknowledges this; consider rejecting a roads that overlaps prev so 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 no EJoin() comparison anywhere. The code is fine; the description should match it so future readers don't go looking for the EJoin check.
  • serverStreamLaneProposals can now silently switch the stream to a different LaneID mid-connection (rejoin resets first = 0). clientStreamLaneProposals happens to tolerate this because PushBlock resolves 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 compare Lane().Validator(). Worth updating that comment now while the context is fresh.
  • 6 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread sei-tendermint/internal/autobahn/consensus/persist/blocks.go
Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/subscriptions.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/consensus/persist/blocks.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/producer/state.go
Comment thread sei-tendermint/internal/autobahn/producer/mempool.go
Comment thread sei-tendermint/internal/autobahn/avail/inner.go Outdated
seidroid[bot]
seidroid Bot previously requested changes Aug 6, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_dir is 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 that tryPruneLeaveLanes can never reach, since they are not in bp.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 on blocks.go:234 and autobahn.proto:131.
  • No test covers the upgrade path at all: there is no case that opens a BlockPersister over a 32-byte-hex lane dir, and none that feeds a pre-change (field-1 lane) LaneRange/CommitQC WAL entry through loadAllCommitQCs. 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 in tryPruneLeaveLanes is "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 — Run was its only caller and it went through alignMempoolForLane. unused is not in .golangci.yml, so lint will not catch it.
  • tryPruneLeaveLanes allocates a full KnownLanes() 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 or len(inner.blocks) > current.Lanes().Len() (the guard already used for the in-memory half).
  • LaneID.Bytes() calls l.validator.Bytes() twice (once for the capacity, once for the append). Hoist it into a local.
  • epoch.GenRegistry still builds its genesis committee via NewCommittee (all e_join = 0), so the avail/producer/giga tests only ever exercise e_join = 0 lanes end-to-end; the random-e_join GenCommittee change is confined to the types package. An ActivateEpoch-based fixture with a non-zero e_join stayer would give the new WAL naming and map keying real coverage.
  • ApplyEpoch returns an error that is documented as always nil. Fine as a forward-compat hook for #3736, but every current caller has to require.NoError on something that cannot fail.
  • 6 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread sei-tendermint/internal/autobahn/consensus/persist/blocks.go
Comment thread sei-tendermint/internal/autobahn/autobahn.proto
Comment thread sei-tendermint/internal/autobahn/avail/inner.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/subscriptions.go Outdated
Comment thread sei-tendermint/internal/autobahn/producer/state.go
Comment thread sei-tendermint/autobahn/types/committee.go Outdated
Comment thread sei-tendermint/internal/autobahn/epoch/registry.go
Comment thread sei-tendermint/internal/autobahn/avail/inner.go
Comment thread sei-tendermint/internal/autobahn/avail/state.go
Comment thread sei-tendermint/autobahn/types/testonly.go Outdated
@wen-coding wen-coding changed the title feat(autobahn): identify lanes as (validator, e_join) (CON-358) feat(autobahn): implement new lane ID for epoch (CON-358) Aug 6, 2026
@wen-coding wen-coding added the ai: skip-review Skips Seidroid AI review triggered via Unified CI. label Aug 6, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread sei-tendermint/internal/autobahn/avail/inner.go
@wen-coding
wen-coding force-pushed the wen/lane_id_in_epoch branch from 3c335fa to fcee025 Compare August 8, 2026 00:30
Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/subscriptions.go Outdated
Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
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>
@wen-coding
wen-coding force-pushed the wen/lane_id_in_epoch branch from 26a7c0b to d37679b Compare August 9, 2026 04:53
@wen-coding wen-coding removed the ai: skip-review Skips Seidroid AI review triggered via Unified CI. label Aug 9, 2026
Comment thread sei-tendermint/internal/autobahn/avail/state.go
seidroid[bot]
seidroid Bot previously requested changes Aug 9, 2026

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and TestNewCommittee_RejectsEmptyWeights are removed with no replacement. TestCommitQCVerifyChecksWeight was not added — the old one was deleted and the PrepareQC test renamed into its place. After this PR there is no test anywhere in autobahn/types exercising PrepareQC.Verify, and none exercising AppQC.Verify weight thresholds. None of these depend on LaneID, so nothing in this change requires dropping them. Please restore them (mechanically updating to committee.Lane(...) where needed).
  • 3 blocking issue(s) flagged inline on specific lines.

Non-blocking

  • cursor-review.md is empty — the Cursor pass produced no output, so this review reflects only Claude + Codex findings.
  • inner.prune(c *types.Committee, ...) (inner.go:228) never references c — its body iterates i.votes. This PR now threads a carefully computed anchorCommittee into 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's WaitUntil predicate fires (new blocks / new commitQC / new appQC). A tip-stale leave lane with nothing pending and an otherwise idle chain lingers in inner.blocks and on disk until unrelated activity wakes the loop. Consider adding staleLaneDisposable to the wake predicate.
  • PushCommitQC still hard-rejects QCs whose EpochIndex differs from the applied epoch (state.go:435). Once ApplyEpoch has 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 a TODO(#3736) at that comparison so it isn't lost.
  • NewBlockPersister logs and skips lane dirs whose names don't parse as the new 80-hex LaneID (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 by LaneID and never has entries removed when dropLanes fires, so it accumulates one entry per historical lane over the node's lifetime. Negligible in size, but easy to clean up alongside dropLanes.
  • 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() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 pruneStaleLeavedropLanes 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() >= BlocksPerLane the predicate is permanently false. The waiter re-evaluates on every unrelated ctrl.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 spurious ctrl.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.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

wen-coding and others added 2 commits August 9, 2026 08:15
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>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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.

Comment thread sei-tendermint/internal/autobahn/avail/state.go Outdated
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>

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/LaneRange wire change, block-hash change) is documented only in the PR description. Consider recording it in a package doc comment or sei-tendermint/AGENTS.md so the "wipe persistent_state_dir before 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 any PushBlock error — including ErrBadLane from transient epoch skew between peers — which drops the whole connection and forces a DialInterval redial. 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:125 and state.go:136 also 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 staleLaneDisposablepruneStaleLeave 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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Two things on the resubscribe path:

  1. No backoff. If ErrLanePruned were ever to recur immediately after resubscribe, this is a tight loop that also emits an Info log per iteration. My reading is that it isn't reachable today (ApplyEpoch adds the map before Store, and dropLanes only touches lanes absent from the tip committee), but the comment on subscriptions.go explicitly admits a "DeleteLane race" as a source — if that race is real, this spins.

  2. first = 0 replays from block 0. For a genuine rejoin that's correct (new LaneID starts at 0). For the race case, LaneProposalsRecv.Recv walks types.ErrPruned one block at a time up to q.first, taking the inner lock per step. Cheap on a fresh lane, O(pruned height) otherwise.

return err
}

g, gctx := errgroup.WithContext(ctx)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@seidroid
seidroid Bot dismissed stale reviews from themself August 9, 2026 15:46

Superseded: latest AI review found no blocking issues.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / the collectPersistBatchpruneStaleLeave tick have no direct test. TestSubscribeLaneProposals_ErrLanePrunedAfterMapDrop reaches into state.inner and calls dropLanes by hand, so the actual disposal predicate and the WAL-delete-then-map-drop ordering in runPersist are 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, and giga/avail.go leans 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 on avail does 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 from hex(pubkey) to hex(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 wherever persistent_state_dir is 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 their rpc.Msg MsgSize caps in giga/api.go (CommitQC 21242 < 22 kB, StreamAppQCsResp 31718 < 32 kB, FullCommitQC 152246 < 300 kB, LaneProposal/GetBlockResp ≈2.056 MB < 2 MB… — LaneProposal 2056293 < 2097152 ✓), NewRoundRobinElection has no remaining references, and Proposal.LaneRange(lane) returns a zero range carrying the requested lane, so inner.prune's i.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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 pruneStaleLeaverunPersistavail.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()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The producing check runs only after CheckTxSafe (line 144). Two consequences after a leave:

  1. An invalid tx returns the application's CheckTx response at line 149 instead of ErrNotProducing, so the caller sees a rejection reason unrelated to the stated "not producing" contract — and TestProducer_LeaveCancelsAndRejoinStartsNewLane only asserts ErrNotProducing for a valid tx, so this path is untested.
  2. Every rejected tx still pays for a full CheckTxSafe execution 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 ErrBadLaneCanceled, 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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@seidroid
seidroid Bot dismissed stale reviews from themself August 9, 2026 15:57

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant