Skip to content

Validator and committee queues: contain a runner panic instead of crashing the node #3055

Open

Description

@iurii-ssv

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

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, since markDutyFailed is 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 safeGo helper, 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions