Repository navigation
Validator and committee queues: contain a runner panic instead of crashing the node #3055
Description
Summary
Nothing on the duty path recovers from a panic. The validator's and the committee's queue consumers run all of a runner's message handling, including its duty starts and QBFT round timeouts, which arrive as queue events. So a nil dereference or any other runtime panic in one runner exits the process and takes every validator on the node offline, not only the one whose runner failed. When the panic is deterministic for a recurring duty, restarting the node doesn't help.
This happened on local_testnet_gloas with the epbs-gloas build (#2901). A spurious release of the proposer's §6 reveal data made the envelope publish dereference nil, and 2 of 4 operators crashed at the committee's first Gloas proposal, leaving the committee below quorum. That bug is fixed on the branch ("proposer: keep the reveal data on a late consensus message"), and the publish now refuses released reveal data instead of dereferencing it. This issue is about the blast radius of the next bug of this kind.
Where
- Validator runners:
Validator.StartQueueConsumercalls the handler for each message with no recover. The handler,ProcessMessage, also runs duty starts (OnExecuteDuty) and QBFT round timeouts. - Committee runners:
Committee.ConsumeQueue, the same way. - Goroutines the runners start themselves run outside both consumers: the per-root reconstruct and submit goroutines of the committee and aggregator-committee runners, and the duty-outcome watcher (
watchDutyOutcome).
The node recovers panics today only in the p2p connection handlers, the HTTP servers and logging. The API server's middlewareRecover is the closest precedent: it logs a panic in one request and keeps serving the others.
Suggested design
Contain a panic in the consumer, per message:
- wrap the handler call in both consumers in a deferred
recover; - log the panic value and the stack at error level, with the message's role, slot and duty fields;
- count it in a new counter, by role, so it can be alerted on;
- conclude the runner's running duty as failed, so the duty watcher reports it and the next duty starts from fresh state. The validator package needs an exported way to do that, for example a
FailRunningDuty(reason error)on the runner, sincemarkDutyFailedis internal to the runner package; - keep consuming.
Slashing protection still guards every signature, so containing a panic puts that validator's duty at risk, not safety: for the rest of that duty, the runner may hold state the panic left half-updated.
Open questions
- Fail fast or contain: is it acceptable to keep going with a runner whose state may be inconsistent, given that its duty is concluded failed and the next duty resets it?
- Should the goroutines the runners start themselves get the same treatment, for example through a small
safeGohelper, or stay a documented gap? - Where to land it:
stage, since it covers every runner on every fork, or together with the ePBS release, which brings a lot of new runner code.
Tests
- A validator runner whose handler panics: the consumer keeps running and processes the next message, the duty concludes failed, and the counter records one panic.
- The same for a committee runner.
Activity