fix(autobahn): prevent block-sync livelock after restart catch-up - #3831
Conversation
Exclude the validator self-dial from block sync and treat in-memory gap-fills as already fetched so restart catch-up cannot livelock. Also make the major-upgrade height wait assert explicitly so a stalled node cannot false-green as an expected panic. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3831 +/- ##
==========================================
- Coverage 61.28% 60.39% -0.90%
==========================================
Files 2351 2259 -92
Lines 197283 186786 -10497
==========================================
- Hits 120907 112811 -8096
+ Misses 65530 63982 -1548
+ Partials 10846 9993 -853
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core fix is sound: the old runBlockFetcher used TryBlock, which hides gap-fills, so a goroutine for an already-held height never exited and permanently leaked one of the 100 semaphore slots — HaveBlock correctly closes that (and gap-fills above nextBlock are never evicted, since evictBelowBound only drops below first <= nextBlock, so the early return can't strand a height); dropping clientGetBlock from the self dial is likewise correct. No blockers — remaining points are drift risk in the duplicated RunSelfClient, thin test coverage, and an overstated integration-test assertion.
Findings: 0 blocking | 11 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex's single finding (missing regression test for theRunSelfClientwiring) is merged into the inline comment onservice.go. - Only the node-0 post-downgrade
wait_for_height.shcall is hardened; the three calls for nodes 1–3 (major_upgrade_test.yaml:82-87) still silently swallow a timeout. Consider makingwait_for_height.shitself echo a terminalPASS/FAILline and asserting on it at every call site, rather than repeating inline shell in the YAML. - The new
WAIT_FOR_HEIGHT_PASSassertion is largely subsumed by the existingPANIC_AT_BLOCK_HEIGHT_NODE_0 == "PASS"verifier (a live, stalled node makesverify_panic.shprintFAILafter its 60 attempts). Still worth keeping as an earlier, clearer failure signal — just don't rely on it as the sole guard. - Edge case worth a note in the code: on a single-member committee the self dial was the only outbound client, so after this change there is no
clientGetBlockconsumer at all andrunBlockFetcherblocks inutils.Sendinstead of spinning. Behaviorally equivalent (a lone validator has no peer to fetch from either way) and still ctx-cancellable, but it becomes a silent stall rather than a retrying one — worth a sentence if 1-validator committees are a supported configuration. - Verified during review, no action needed:
HaveBlock'sn < nextBlockbranch exactly covers the olderr == nil || errors.Is(err, types.ErrPruned)skip set (TryBlockonly reachedblockFromDB, and henceErrPruned, forn < nextBlock), and it now avoids a BlockDB read on every retry. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| // block sync. The self connection is required for the validator's own votes, | ||
| // but it must not consume catch-up GetBlock requests: by definition this node | ||
| // cannot serve a block that is missing from its own data state. | ||
| func (x *Service) RunSelfClient(ctx context.Context, client rpc.Client[API]) error { |
There was a problem hiding this comment.
[suggestion] RunSelfClient duplicates 7 of RunClient's 9 spawns. Nothing ties the two lists together, so a stream added to RunClient later will silently be missing on the self connection (or, as Codex notes, clientGetBlock could be accidentally re-added here and quietly restore the restart livelock).
Consider factoring the shared set into a helper both entry points call, e.g.:
func (x *Service) spawnConsensusClientStreams(ctx context.Context, s scope.Scope, client rpc.Client[API]) {
s.Spawn(func() error { return x.clientPing(ctx, client) })
s.Spawn(func() error { return x.clientConsensus(ctx, client) })
// ... lane/commit/app streams
}with RunClient additionally spawning clientStreamFullCommitQCs + clientGetBlock. That makes "self = consensus streams only" a single-line difference instead of a copy that can drift.
Also worth a targeted regression test — the 4-node harness in giga_router_validator_test.go could assert the self connection never issues a GetBlock, or a unit test could assert the spawn set of each entry point.
|
|
||
| // RunSelfClient runs validator-local consensus and availability streams without | ||
| // block sync. The self connection is required for the validator's own votes, | ||
| // but it must not consume catch-up GetBlock requests: by definition this node |
There was a problem hiding this comment.
[nit] The comment explains why GetBlock is excluded, but clientStreamFullCommitQCs is also dropped and the PR description only mentions GetBlock. It's harmless (streaming our own QCs back into our own PushQC is a no-op via insertQC's gr.Next <= i.nextQC skip), but since it's a second, unexplained omission it deserves a clause — e.g. "...and the self QC stream, which only re-pushes QCs we already hold."
| // consensus/availability only: a loopback GetBlock consumer always | ||
| // returns empty for missing catch-up heights and can starve the | ||
| // contiguous prefix while higher gap-fills keep retrying. | ||
| selfKey := r.key.Public() |
There was a problem hiding this comment.
[nit] This file now holds two different notions of "self": selfKey (the p2p node key, r.key.Public()) and the validatorKey field (the consensus key, used by EvmProxy). Matching addr.Key against the node key is correct here since GigaNodeAddr.Key is a NodePublicKey — but a one-line comment saying so would save the next reader from wondering whether validatorKey was meant.
| if n < inner.nextBlock { | ||
| return true | ||
| } | ||
| return inner.blocks[n] != nil |
There was a problem hiding this comment.
[nit] insertBlock uses the comma-ok form (if _, ok := i.blocks[n]; ok) for the same lookup. Equivalent today since no nil block is ever stored, but _, ok := inner.blocks[n]; return ok matches the surrounding code and doesn't depend on that invariant.
| _, err := state.TryBlock(last) | ||
| require.ErrorIs(t, err, types.ErrNotFound, "gap-fill above nextBlock must stay hidden") | ||
| have := state.HaveBlock(last) | ||
| require.True(t, have, "HaveBlock must see gap-fills so the fetcher does not re-request them") |
There was a problem hiding this comment.
[suggestion] Good coverage of the gap-fill and hole cases. The one branch not covered is the one that replaced the deleted errors.Is(err, types.ErrPruned) handling in runBlockFetcher: a height below first (evicted from RAM, possibly pruned from BlockDB) must still report true, otherwise the fetcher would loop forever on a height it can never obtain. Worth an assertion after eviction has advanced first.
Minor: require.True/require.False here are re-exported testify vars, so they take the expression directly — require.True(t, state.HaveBlock(last), ...) avoids the intermediate have variable and its reassignment on line 831.
| # The integration runner records non-zero script exits but does not fail the | ||
| # input step. Capture an explicit result so a live, stalled node cannot be | ||
| # mistaken for the expected upgrade panic. | ||
| - cmd: wait_for_height.sh $TARGET_HEIGHT node_admin; WAIT_STATUS=$?; if [ "$WAIT_STATUS" -eq 0 ]; then echo WAIT_FOR_HEIGHT_PASS; else echo WAIT_FOR_HEIGHT_FAIL; fi |
There was a problem hiding this comment.
[suggestion] wait_for_height.sh exits 0 on two paths: the target height was reached, and pgrep found no running seid ("Seid no longer running (panic)" → break → exit 0). So WAIT_FOR_HEIGHT_PASS really asserts "the wait did not time out", not "node 0 reached $TARGET_HEIGHT" — the env name DOWNGRADED_NODE_0_REACHED_TARGET (line 135) overstates what is checked. Either rename it (e.g. DOWNGRADED_NODE_0_WAIT_STATUS) or make the script distinguish the two exits so the assertion matches the comment above it.
Style nit: WAIT_STATUS is used exactly once, so wait_for_height.sh $TARGET_HEIGHT node_admin && echo WAIT_FOR_HEIGHT_PASS || echo WAIT_FOR_HEIGHT_FAIL expresses the same thing in one clause.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any additional bugs beyond what the bug hunting system already ruled out, but this PR touches the Autobahn consensus/p2p block-sync layer (self-dial wiring, in-memory gap-fill visibility, block fetcher backoff) — critical-path networking code with a data-race-sensitive locking scheme — so it's worth a human look before merging.
Extended reasoning...
Overview
The PR changes catch-up block-sync behavior in the Autobahn consensus/p2p stack: (1) it stops a validator's self-dial connection from participating in GetBlock catch-up fetches (RunSelfClient in service.go, wired in giga_router_validator.go), since a node can never serve a block it doesn't have locally; (2) it adds State.HaveBlock in state.go so the block fetcher in giga/data.go treats in-memory gap-fills as already-fetched, avoiding redundant re-requests that could starve the contiguous prefix; and (3) it hardens an integration test (major_upgrade_test.yaml) so a stalled node can't false-green a wait_for_height.sh step as the expected upgrade panic.
Security risks
No new attack surface — this is sync/liveness logic, not auth, crypto, or app-hash-affecting state transition (the PR carries the non-app-hash-breaking label). The main risk category is correctness/liveness: a subtle bug here could reintroduce sync stalls or, in the other direction, cause blocks to be considered 'had' when they actually are not, corrupting the no-gap contract that other code (BlockDB persistence, eviction, AppProposal) depends on.
Level of scrutiny
This warrants a careful human pass. It's a small diff by line count, but it modifies locking-sensitive shared state (inner.blocks/nextBlock accessed under s.inner.Lock()) and changes fan-out wiring for validator-to-validator connections in the consensus networking path. Bugs in this class of code (block-sync livelocks, self-dial semantics) are exactly the kind that are easy to miss in review and expensive to discover in production. The bug hunting system found nothing, and I did not find additional issues in HaveBlock's lock/contract logic, the runBlockFetcher replacement of TryBlock with HaveBlock, or the RunSelfClient fan-out change — but this falls squarely in the 'critical code path, human should weigh in' category rather than the 'obviously safe to auto-approve' category.
Other factors
Test coverage is reasonable: new unit tests in state_test.go directly exercise the gap-fill visibility contract added by HaveBlock, and the PR description states it was manually validated against local docker clusters for both major and minor upgrade scenarios, with CI runs still pending per the PR's own test-plan checklist.
Share consensus client stream spawns between RunClient and RunSelfClient, clarify self-key vs validator-key matching, and use comma-ok HaveBlock lookups consistent with insertBlock. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
The three changes (self-dial no longer consumes catch-up GetBlock, HaveBlock so gap-fills stop being re-requested, explicit pass/fail capture for the downgrade height wait) are coherent and I found no correctness bug: the self-key comparison is backed by the selfAddr.Key == NodeSecretKey(nodeKey).Public() invariant enforced in buildValidatorGigaConfig, and gap-fills can only be dropped by skipTo, which runs during recovery before the fetcher starts. Remaining notes are non-blocking (test-harness semantics and coverage gaps).
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so that review is effectively missing. Codex reported no material findings. - The other
wait_for_height.shsteps are still unasserted and carry the same false-green risk this PR fixes for one call site:integration_test/upgrade_module/major_upgrade_test.yaml:82-86andintegration_test/upgrade_module/minor_upgrade_test.yaml:89-95. Consider folding the$?-capture idiom into a small wrapper script (e.g.wait_for_height_checked.sh) and using it everywhere instead of inlining shell in YAML. HaveBlockhas no test for the evicted-but-below-nextBlockcase (n <first, served from / pruned out of BlockDB). That path replaces the olderrors.Is(err, types.ErrPruned)branch inrunBlockFetcher, so it is worth one assertion alongside the new gap-fill assertions inTestTryBlockHidesGapFills.- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| # The integration runner records non-zero script exits but does not fail the | ||
| # input step. Capture an explicit result so a live, stalled node cannot be | ||
| # mistaken for the expected upgrade panic. | ||
| - cmd: wait_for_height.sh $TARGET_HEIGHT node_admin; WAIT_STATUS=$?; if [ "$WAIT_STATUS" -eq 0 ]; then echo WAIT_FOR_HEIGHT_PASS; else echo WAIT_FOR_HEIGHT_FAIL; fi |
There was a problem hiding this comment.
[suggestion] Two things about this assertion:
-
wait_for_height.shalso exits 0 on thepgrep-miss path (Seid no longer running (panic)→break), soWAIT_FOR_HEIGHT_PASSmeans "did not time out", not "reached $TARGET_HEIGHT". That's the right signal for the stated goal (a live stalled node now fails instead of being read as the expected panic), andverify_upgrade_needed_log.sh $TARGET_HEIGHTcovers the height separately — but the env var nameDOWNGRADED_NODE_0_REACHED_TARGETand the comment claim more than the check delivers. Worth renaming to something likeDOWNGRADED_NODE_0_WAIT_RESULT. -
env:captures the command's whole trimmed stdout, andwait_for_height.shprints aWaiting for block ...line every iteration for up toMAX_WAIT_SECONDS=360. The runner then re-injects that value asdocker exec -e ...on every subsequent step in the case, so a slow run makes a multi-KB single argv entry (and a huget.Logfline). Redirecting the script's chatter (wait_for_height.sh ... >/dev/nullor>&2) so only the marker lands on stdout would keep the capture small — at the cost of losing the progress log, sincedockerExecer.rundiscards stderr on success.
| selfKey := r.key.Public() | ||
| for _, addr := range r.cfg.ValidatorAddrs { | ||
| runClient := r.service.RunClient | ||
| if addr.Key == selfKey { |
There was a problem hiding this comment.
[suggestion] The self-detection is sound — buildValidatorGigaConfig rejects startup unless the committee entry for our own validator key carries exactly NodeSecretKey(nodeKey).Public(), and node keys are deduped in loadAutobahnCommittee — so this comparison can't silently miss. But the fix has no direct test: nothing asserts that the self dial gets RunSelfClient (existing validator router tests only exercise it implicitly via consensus still working). A small unit test that the self connection never consumes from getBlockReqs would lock in the property this PR exists to establish, and would fail loudly if someone later reintroduces block-sync streams into spawnConsensusClientStreams.
| // satisfy this height. Re-fetching them while a lower hole is | ||
| // empty wastes the shared per-peer GetBlock queue and can | ||
| // starve the contiguous prefix. | ||
| if x.data.HaveBlock(n) { |
There was a problem hiding this comment.
[nit] Behavior note, not a defect: the fetcher no longer surfaces store errors here. The old TryBlock path returned fmt.Errorf("TryBlock(%d)", ...) for anything that wasn't ErrNotFound/ErrPruned, aborting the fetcher on a BlockDB read failure; HaveBlock never touches BlockDB, so those failures now go unobserved at this call site. That looks intentional and harmless (the pruned case already returned nil before, and DB failures still surface via Block/persist paths), but it's worth confirming no monitoring relied on the fetcher dying here.
There was a problem hiding this comment.
No new bugs found beyond what's already flagged inline. This PR touches core Autobahn consensus/P2P block-sync paths (self-dial routing, block fetcher dedup logic) — critical-path networking code, so it should get a human look rather than an automated approval.
Extended reasoning...
Overview
The PR fixes a livelock in Autobahn's block-sync catch-up: the self-dial connection (a validator's loopback connection to itself) was spawning the full RunClient handler set, including clientGetBlock, which would consume catch-up GetBlock requests that can never succeed against a node's own missing local data. It also adds a HaveBlock check so the block fetcher treats in-memory gap-fills as already-fetched, avoiding redundant re-requests while a lower hole is open, and hardens an integration test assertion so a stalled node can't be mistaken for the expected upgrade panic.
Security risks
No security-sensitive surface (auth/crypto/permissions) is touched. The self-key comparison in giga_router_validator.go correctly compares against the p2p node key (NodePublicKey), matching GigaNodeAddr.Key's type, so there's no type-confusion risk with the separate validatorKey (consensus signing key) field.
Level of scrutiny
This warrants a careful human look: it's a targeted but non-trivial change to core consensus/P2P block-sync control flow (self-dial handler selection, block fetcher dedup logic) in Autobahn — a critical path where a subtle regression could reintroduce a livelock or cause missed block propagation. I traced through HaveBlock's semantics against the old TryBlock+ErrPruned logic it replaced and confirmed the new logic is equivalent (and removes a possible DB-error propagation path), and confirmed the self-dial loopback connection is real (not a no-op) via dialAndRunConn/tcp.Dial. I did not find additional correctness issues beyond what seidroid already flagged (RunSelfClient/RunClient duplication risk, a missing eviction-boundary test case, and the wait_for_height.sh dual-exit-path ambiguity) — those are legitimate maintainability/coverage suggestions, not bugs blocking approval.
Other factors
Test coverage is reasonable (new HaveBlock unit test covers both the gap-fill-hit and hole-miss branches; patch coverage 95%+ per Codecov) and the PR description documents manual testing on a local docker cluster for both major and minor upgrade scenarios. Given the critical-path nature of the code and the existing unresolved review suggestions (helper duplication, comment omission), a human reviewer should weigh in before merge.
philipsu522
left a comment
philipsu522
left a comment
There was a problem hiding this comment.
this looks good, i just wonder if we need to bound outgoing requests, given previously we had a pseudo-limit via holding the permit for an existing block? otherwise would we oom / overwhelm the node with this new (more performant) behavior?
| // spawnConsensusClientStreams starts the streams shared by peer and self | ||
| // clients. Block-sync streams (FullCommitQCs / GetBlock) are added only by | ||
| // RunClient so the self dial cannot drift back into catch-up fetches. | ||
| func (x *Service) spawnConsensusClientStreams(ctx context.Context, s scope.Scope, client rpc.Client[API]) { |
There was a problem hiding this comment.
don't take scope as an arg, just make it a run...() command and create another scope internally.
pompon0
left a comment
pompon0
left a comment
There was a problem hiding this comment.
The mismatch between TryBlock and fetcher semantics was definitely worth fixing, thank you. Special handling of loopback-connection is something that I'd really like to avoid, because it obfuscates deeper problems: like here - we fix the specific case where node asks itself about the block that it doesn't have, but the problem persists if we have a peer which also doesn't have the block. Indeed the fetching strategy has a lot of room for improvements, but let's make them uniform.
| // nextBlock. Unlike TryBlock, gap-fills count as present so the block fetcher | ||
| // does not keep re-requesting heights it already holds while waiting for a | ||
| // lower contiguous hole to close. Does not consult BlockDB. | ||
| func (s *State) HaveBlock(n types.GlobalBlockNumber) bool { |
There was a problem hiding this comment.
name is misleading, since the block could have been already pruned. One option would be to invert the semantics and call it "NeedBlock", which does not imply the block is accessible (pruned/ahead of nextBlock)
| return scope.Run(ctx, func(ctx context.Context, s scope.Scope) error { | ||
| s.Spawn(func() error { return x.clientPing(ctx, client) }) | ||
| s.Spawn(func() error { return x.clientConsensus(ctx, client) }) | ||
| x.spawnConsensusClientStreams(ctx, s, client) |
There was a problem hiding this comment.
until we implement a robust retry strategy (please add a TODO), for now it would be fine to add a flag to RunClient deciding whether to run GetBlock or not, as a minimal asymmetry addressing the issue at hand.
Rename HaveBlock to NeedBlock, fold self-dial into RunClient(getBlock), and nest consensus client streams in their own scope.Run. Add a TODO for a uniform GetBlock retry strategy so self asymmetry can go away later. Co-authored-by: Cursor <cursoragent@cursor.com>
@philipsu522 we still have MaxConcurrentBlockFetches=100 cap the in flight requests, there is just less waste. |
There was a problem hiding this comment.
The livelock fix is sound: NeedBlock correctly treats verified gap-fills as satisfied (gap-fills are hash-checked against the covering QC and never evicted above nextBlock), and the self-dial getBlock=false gate is exact because startup already enforces that the committee entry's node key equals the local node key. Remaining findings are naming/doc accuracy, an overstated integration-test assertion, and test-coverage gaps — no blockers.
Findings: 0 blocking | 7 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found"; the Cursor review file (
cursor-review.md) is empty, i.e. that pass produced no output. - The other
wait_for_height.shsteps still silently ignore failure —major_upgrade_test.yaml:82,84,86andminor_upgrade_test.yaml:89,91,93,95. They have exactly the same false-green property this PR fixes for one step. Consider either applying the same wrapper there or (better) fixing it centrally:dockerExecer.runswallowing*exec.ExitErrorinto a nil error (integration_test/runner/runner.go:205-210) is the root cause, and an opt-infail_on_error: trueon the input step would avoid the per-step shell boilerplate. - No test covers the new
RunClient(..., false)path or the self-key skip ingigaValidatorRouter.Run;data_test.goonly updates the call site totrue. A router-level test asserting the self dial spawns noclientGetBlock(or thatgetBlockis false exactly for the self entry) would lock in the fix that motivates this PR. NewGigaValidatorRouteritself doesn't check thatkey.Public()appears incfg.ValidatorAddrs— that invariant lives inbuildValidatorGigaConfig(node/setup.go:242-245). In production the two are always paired so the self-skip is exact, but a direct constructor caller with a mismatched key silently gets the old behavior. A one-linelogger.Info/assert when no self entry is found would make that misconfiguration visible rather than silently degrading.- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| // NeedBlock reports whether catch-up still needs to fetch height n. | ||
| // False when n is already past nextBlock (including heights pruned or |
There was a problem hiding this comment.
[nit] The wording is inverted relative to the code. n < inner.nextBlock means nextBlock has already advanced past n, but "n is already past nextBlock" reads as n > nextBlock — which is the branch that returns true. Suggest: "False when the contiguous prefix has already passed n (n < nextBlock, including heights pruned or evicted from RAM)…".
| } | ||
|
|
||
| func (x *Service) RunClient(ctx context.Context, client rpc.Client[API]) error { | ||
| func (x *Service) RunClient(ctx context.Context, client rpc.Client[API], getBlock bool) error { |
There was a problem hiding this comment.
[suggestion] The getBlock flag gates clientStreamFullCommitQCs in addition to clientGetBlock, so the name is narrower than the effect. Disabling the QC stream on the self dial looks fine on its own (serverStreamFullCommitQCs on the loopback just re-pushes QCs the node already has), but it's a second behavior change that neither the parameter name nor the PR description mentions. Consider renaming to something like blockSync/catchUp, or taking a small options struct — a bare bool at the call site (RunClient(ctx, client, getBlock)) doesn't convey that two streams are involved.
| # The integration runner records non-zero script exits but does not fail the | ||
| # input step. Capture an explicit result so a live, stalled node cannot be | ||
| # mistaken for the expected upgrade panic. | ||
| - cmd: wait_for_height.sh $TARGET_HEIGHT node_admin; WAIT_STATUS=$?; if [ "$WAIT_STATUS" -eq 0 ]; then echo WAIT_FOR_HEIGHT_PASS; else echo WAIT_FOR_HEIGHT_FAIL; fi |
There was a problem hiding this comment.
[suggestion] WAIT_FOR_HEIGHT_PASS proves "the script did not time out", not "node 0 reached TARGET_HEIGHT" — so the capture name DOWNGRADED_NODE_0_REACHED_TARGET overstates it. wait_for_height.sh also breaks (exit 0) on the pgrep-miss path ("Seid no longer running (panic)"), so a node that dies at any height before TARGET_HEIGHT still yields PASS. The check does catch the stated failure mode (a live, stalled node hits the 360s timeout → exit 1), so this isn't wrong — just consider renaming the var to something like DOWNGRADED_NODE_0_WAIT_RESULT, or having the script emit distinct markers for "reached height" vs "process gone" so the verifier can assert the former.
Minor: the WAIT_STATUS capture is redundant — if wait_for_height.sh $TARGET_HEIGHT node_admin; then echo WAIT_FOR_HEIGHT_PASS; else echo WAIT_FOR_HEIGHT_FAIL; fi is equivalent and shorter.
There was a problem hiding this comment.
No bugs found. Given this PR touches consensus-critical block-sync/livelock logic in autobahn, I'm deferring to human review rather than auto-approving, since a human reviewer (pompon0) is already actively engaged on this thread.
Extended reasoning...
Overview
This PR fixes a block-sync livelock in Autobahn: (1) validators no longer consume GetBlock on the self-dial connection (a loopback that always returns empty for missing catch-up heights, which could starve the contiguous prefix), (2) the block fetcher now checks NeedBlock instead of TryBlock so in-memory gap-fills count as satisfied and don't get re-requested while a lower hole is open, and (3) an integration test assertion is tightened so a live-but-stalled node during the major-upgrade downgrade step can't be misread as the expected panic.
Security risks
No new external attack surface — this is internal p2p/consensus plumbing between committee members that are already mutually authenticated via the giga transport. The main risk category is correctness/liveness (a bad change here could reintroduce a sync livelock or, worse, a silent data gap), not confidentiality or injection-style vulnerabilities.
Level of scrutiny
This warrants a human look. It touches consensus-critical code paths (block/QC state machine, self-dial peer routing) with subtle invariants around eviction, gap-fills, and the contiguous block prefix — exactly the kind of change where a logic slip is easy to miss and expensive to hit in production. The PR has already been through one round of human review (pompon0), and I can see from the current commit history (2794def refactor(autobahn): address review feedback on NeedBlock and RunClient) that all three of their prior comments (don't pass scope as an arg, rename to NeedBlock, add a getBlock flag with a TODO) have been incorporated.
Other factors
Test coverage looks reasonable (new NeedBlock unit tests for the gap-fill/hole cases, go test and manual docker-cluster runs per the PR description), and the bug-hunting system found no issues. Since a human reviewer is already actively iterating on this PR and the changes are non-trivial, I'm deferring rather than shadow-approving.
| // so connections that lack a height (including self) do not need a separate | ||
| // getBlock=false path to avoid starving the shared fetch queue. | ||
| return scope.Run(ctx, func(ctx context.Context, s scope.Scope) error { | ||
| s.Spawn(func() error { return x.runConsensusClientStreams(ctx, client) }) |
There was a problem hiding this comment.
nit: inline back runConsensusClientStreams
| return scope.Run(ctx, func(ctx context.Context, s scope.Scope) error { | ||
| s.Spawn(func() error { return x.runConsensusClientStreams(ctx, client) }) | ||
| if getBlock { | ||
| s.Spawn(func() error { return x.clientStreamFullCommitQCs(ctx, client) }) |
There was a problem hiding this comment.
clientStreamFullCommitQCs can be unconditional, no need for asymmetry here
Inline consensus client streams and keep FullCommitQC streaming on for all dials; only GetBlock stays behind the getBlock flag. Co-authored-by: Cursor <cursoragent@cursor.com>
5 participants
Summary
GetBlockon the self dial (RunSelfClient), which always returns empty for missing local heights and can starve the contiguous prefix.HaveBlock, so the block fetcher does not keep re-requesting heights it already holds while a lower hole is open.WAIT_FOR_HEIGHT_PASSexplicitly, so a live stalled node cannot false-green as the expected upgrade panic.Test plan
go test ./autobahn/... ./internal/autobahn/...(253 passed)TestUpgradeMajoron local docker clusterTestUpgradeMinoron fresh local docker clusterMade with Cursor