erigontech / erigontech/erigon
stagedsync: follow-ups from BAL-driven parallel commitment review (#21416)
- Dominant language
- Go
- Stars
- 3.6k
- Forks
- 1.5k
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 455
Description
Follow-ups from the #21416 (BAL-driven parallel commitment) review. None of these block that PR; tracked here so they don't get lost. All file/line references are at PR head `1ea1ba6ac1`.
## 1. Apply-loop early returns can wedge shutdown; one bypasses failure reconciliation
`fb44997c4d` routed the *commitment* infra-error path through record + cancel + keep-draining, so the apply loop can't die while the exec loop sits in one of the new unconditional sends. But seven early returns remain in the apply loop (`execution/stagedsync/exec3_parallel.go`):
- accumulator marshal — line 654
- `BlockByHash` failure — line 674
- nil block — line 677
- block-number mismatch — line 683
- applyCount mismatch — line 687
- `blockValidatorWaiter.Wait()` — line 720
- `ProcessBAL` — line 725
Failure scenario: any of these kills the apply loop while `applyResults` (cap 2048) is full and the exec loop is in the terminal `mustDeliver` send (`sendResult`, line 2369) — or while the calculator is blocked in `publish` (rootResults, cap 64) and the exec loop is in the unconditional `triggerBatchCommitment` send (line 915). `publish`'s rescue arms are the work ctx (still alive) and `cc.done` (closed only by `calculator.Stop()`, which can't run because `executorCancel` is blocked in `pe.wait`). Result: a permanent stage hang instead of an error return. On main these situations resolved via the sends' `ctx.Done` arms; the hang potential is new with the unconditional sends. Error-path-only and low probability, but a hang is strictly worse than the error it replaces.
Separately, the `blockValidatorWaiter.Wait()` return is an exec-side block-validity verdict (receipts/bloom/gasUsed) that bypasses `fail.consider` — a fold wrong-root for an *earlier* block still unread in `rootResults` can be masked by the direct return (select arms are nondeterministic), which is exactly the inversion `failCandidate` exists to prevent.
Suggested fix: route all seven through the same `fail.consider(...)` + `finalized = true` + `deliberateCancel()` + `continue` discipline as the commit-error path. That makes "apply exits only at channel close" hold on every path and closes the taxonomy hole.
## 2. `startCatchup` looks dead — remove or justify
The size-cut catch-up (`exec3_parallel.go` line 1163; `sizeCutPending` lines 974/1205) executes one extra block "so state reaches any block already folded ahead". Under decide-before-send that block can't exist: at blockResult(K)'s stop decision the calculator hasn't consumed blockResult(K) yet, so `foldGateOpen` bounds the fold frontier to ≤ K, and the stopCause published before the send holds it there (the channel send provides the happens-before). The catch-up is a leftover from the `foldFreezeRequest` design (`4b7f03a497`) that the cancel-with-cause rework (`2b7482e8cf`) superseded.
Cost today: with `BAL_DRIVEN_COMMITMENT` defaulting on, every size-limited batch on every chain (including BAL-less mainnet) runs one block past its budget. Either delete it (and `sizeCutPending`), or document the concrete scenario where the fold can outrun the cut. It would become real again in a future fully-BAL-driven mode with a fold-ahead cap C > 1 — the design doc (`docs/plans/20260703-item1-apply-loop-resource-authority.md`) covers that.
## 3. Pinning tests for the shutdown-race fixes
Three fixes from the last two review rounds have no regression tests, unlike the rest of the PR's fixes:
- `handleBlockRequest` drop-guard ordering (`hasFirstBlock` recorded before the drop-guard) — unit-testable in the style of `TestHandleBlockRequest_EmptyBALFallsToIncremental`: deliver blockResult(n) before blockRequest(n), assert `firstBlockNum == n` and that n+1 does not fold.
- `mustDeliver`: the terminal blockResult(M) must reach both the apply loop and the calculator after the stopCause cancel.
- unconditional `triggerBatchCommitment` + the drain-not-die apply-loop discipline (extends naturally to item 1's fix).
The first is straightforward; the other two are concurrency-shaped and may only be practical as targeted harness tests.
## 4. Minor cleanups
- `common/dbg/experiments.go` line 118: the `BALDrivenCommitment` comment names `IGNORE_BAL` as the kill switch — imprecise, since `IGNORE_BAL` also disables BAL-driven tx scheduling (`execution/stagedsync/exec3.go` line 598). `BAL_DRIVEN_COMMITMENT=false` is the precise, narrower switch; the comment should point there.
- `docs/plans/20260703-item1-apply-loop-resource-authority.md`: the trim in `1ea1ba6ac1` left attributed review quotes in place (lines 31, 35, 164, 243). The design and rationale stay; the attribution belongs in PR history.
- `execution/stagedsync/exec3_parallel.go` line 1723 (`blockRequest.firstTxNum/lastTxNum` docstring): still says a step-straddling block "is left to the incremental path, since folding it would need a mid-block step-boundary checkpoint the atomic fold doesn't emit" — stale since `foldStepCheckpoints` landed: straddling blocks stay on the fold path and the pair positions the mid-block checkpoints. (Also flagged by Copilot on the PR.)
cc @mh0lt
Contributor guide
Assessment
This issue has not been assessed yet.