erigontech / erigontech/erigon
cl/phase1/network/services: deferred-block loop has no recover, so a panic in OnBlock terminates the node
- Dominant language
- Go
- Stars
- 3.6k
- Forks
- 1.5k
- Avg merge
- 1d 16h
- Merged PRs (30d)
- 455
Description
## Problem
`blockService.loop` (`cl/phase1/network/services/block_service.go`), started as `go b.loop(ctx)` from the constructor, drains `blocksScheduledForLaterExecution` on a ticker and calls `processAndStoreBlock`, and therefore `ForkChoiceStore.OnBlock`, with no `recover()`.
Nothing upstream covers it:
- the gossip validator's `recover()` (`cl/phase1/network/gossip/gossip_manager.go`) wraps only the synchronous `ProcessMessage` path, not this goroutine;
- `importBlockOperations` in the same file does have a `recover()`, but that is a separate goroutine covering only operation import;
- `cl/clstages/clstages.go` wraps every stage ActionFunc for exactly this reason — its comment says a panic in a stage "e.g. from a malformed peer response" should degrade "to a stage error instead of terminating the whole node". The deferred-block path gets no equivalent.
Blocks reach the loop through `scheduleBlockForLaterProcessing`, called from four places in `ProcessMessage`: parent not yet seen, parent execution payload not yet seen, and the two early-arrival cases.
## Why it matters
The containment property currently differs by path. The same panic inside `OnBlock` is a rejected gossip message when it happens synchronously and a process exit when it happens on the retry loop, even though both originate from the same gossip message. A block only has to fail its first validation attempt to move from one to the other.
This is defence in depth rather than a fix for a known crash: the schema-mismatch dereference that motivated #22797 is fixed, and I know of no currently reachable panic in `OnBlock`. The point is that `OnBlock` is large, dereferences plenty of schema-dependent fields, and is where the next such bug will surface. The analogous gap in `cl/das` — a per-sidecar `wg.Go` with no recover — is precisely what turned the column-sidecar bug into a remote process termination rather than a dropped response.
## Suggested fix
Wrap the `processAndStoreBlock` call in `loop` the way `clstages` wraps its actions: recover, log with a stack, drop the job, keep the loop running.
Contributor guide
Assessment
This issue has not been assessed yet.