Lighthouse: Stop beacon node on unrecoverable DB issues

[michaelsproul]

@AgeManning suggested that we halt the node if we receive an unrecoverable database issue, to prevent the node lingering on the network in a bogus state. Currently this is possible if the database has become inconsistent due to a failure between storing a block and its state (see #692)

We should be able to classify errors into unrecoverable ones and gracefully halt the Beacon Node appropriately in these cases.

[paulhauner]

I like this, I'm just not exactly sure how we define unrecoverable.

We _could_ say that if there's ever a missing beacon state that we're unrecoverable, but we risk the scenario where there's a bug in some old/fork state and but we're still good to produce blocks on the canonical chain. In such a case we'd shutdown prematurely from a validator point of view.

I would be tempted to redefine this as "stop syncing if it's enters an unrecoverable state" as opposed to shutting down the whole client. Mainly because I'm worried about being heavy-handed and killing a client that may be able to continue on gossip alone. Also I'm not exactly sure how we can get the client to gracefully shut itself down. I'm sure it's possible, it's just not immediately obvious to me.

[AgeManning]

For graceful shutdown, I would imagine the main thread would hold a one-shot or an exit_signal channel. A major error in one of the threads would call this one-shot and the main thread would drop beacon_chain.

In regards to keeping a beacon chain alive. Perhaps this is desirable for mainnet when we have a proper peer management in place. For the current iteration, a faulty/corrupt db even without sync, would not propagate blocks/attestations (as it won't be able to verify them, in the worst case) and will degrade the gossip network (as the mesh only has 6 peers in it)

Other peers could try and sync off the faulty node and reject the node when it doesn't return the correct results (this will damage the sync of the requesting peer, because sync still needs to handle malfunctioning peers). In this case, nodes will still go into a kick-rejoin loop, with repeating DB errors.

Given a sophisticated enough peer management, other peers would detect this kind of behaviour and reject/isolate the peer and ultimately it would be kicked from the network and be idle.

I think for testnets, if there is a critical db error (or something we can define as such, i.e prevent a node from participating in RPC requests and/or block propagation) we should kill the client or at the very least, drop its network thread.

I agree that for mainnet, with a sufficiently robust management of peers, we should leave the client limping along, although with loud and constant error logs to the user (as the peer will ultimately be isolated and essentially disconnected/turned off).

[paulhauner]

For graceful shutdown, I would imagine the main thread would hold a one-shot or an exit_signal channel. A major error in one of the threads would call this one-shot and the main thread would drop beacon_chain.

There's lots of different places that could find this error (anywhere that reads the db), so I think it's going to be easier said than done to ensure that the client always shuts down when the DB is inconsistent. We can't just put it into the store because the store has no idea about a state that _must_ exist (e.g., referenced by a block) vs a state that _can maybe_ exist (e.g., query from the API).

It seems to me that what we _really_ want from this is to just get faulty peers off the network until we can get the network to a state where it can tolerate such madness. Given that, it seems we should keep this fairly self-contained in the network service; we give it it the ability to kill itself and the smarts to figure out when it should do so.

Notably, we have already addressed what is likely the cause of the inconsistent DB so I reckon we should wait and see if we keep getting the errors before we go about trying to handle them.

[paulhauner]

I'm tempted to pull this off the v0.1.1 milestone because I have no information to suggest that this is still a pressing issue after we fixed the "storing blocks before states" thing. Has anyone else seen this happen since we made that change?

[michaelsproul]

Yeah, sounds good

[paulhauner]

Pulling this off the v0.1.1, feel free to object @AgeManning

[paulhauner]

I'll close this one since we haven't seen this corruption issue since we started storing blocks before states. Feel free to reopen if you want :)