erigontech / erigontech/erigon

cl/phase1/network/services: deferred-block loop has no recover, so a panic in OnBlock terminates the node

Open
#22,818 1 comment 0 reactions 1 assignee Claimed by @domiwei View on GitHub
Caplin tech debt reduction
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

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.