erigontech / erigontech/erigon

stagedsync: follow-ups from BAL-driven parallel commitment review (#21416)

Open
#22,332 1 comment 0 reactions 2 assignees Claimed by @mh0lt View on GitHub
tech debt reduction
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.