Implement Slashing Protection

[michaelsproul]

Issue Addressed

Closes #254
Replaces #588
Obsoletes #623

Proposed Changes

Builds upon @pscott's previous PR, updating his work for the latest changes in master, and moving to a single slashing protection database to solve an issue with file descriptor limits (one DB per validator broke down at around 100 validators per machine under default Linux limits of 1024 FDs per process).

Additional Info

Completed TODOs:

• Resolve all FIXMEs, printlns, etc
• Refactor/rewrite existing tests
• Write tests that stress the concurrency safety of the slashing protection (same block on diff threads, etc)
• (POSTPONED) Fuzz or property test the concurrency safety as well

[paulhauner]

Hey, not sure it's worth it but locking validators into a genesis_time (once we know it) might also be useful to help a scenario where a validator connects to the wrong network (i.e., testnet node) and starts signing shit in the future (according to their native chain). The genesis_validators_root should stop this from being slashable, but perhaps it would still cause issues?

Sorry bit of a brain dump message, just didn't want to forget.

[michaelsproul]

This is finally ready for review!

Here's a quick overview:

• SlashingDatabase is an interface to an SQLite database stored by default in ~/.lighthouse/validators/slashing_protection.sqlite which records all of the attestations and blocks signed by validators. It contains a table of known (registered) validators, for which it tracks signatures and allows new signatures.
• For now, new validators can only be registered with slashing protection using the --auto-register flag on the validator client. In future we should look at better (and safer) usage patterns like registration from the account manager, or via the HTTP API.
• Concurrency-related bugs (see parallel_tests.rs) are prevented in 3 ways, in the spirit of Defense in Depth:
  1. The database is put into locking_mode=EXCLUSIVE, which instructs SQLite to only allow a single connection to the database (see docs).
  2. All database transactions operate in EXCLUSIVE mode.
  3. Uniqueness constraints on the signed_attestations and signed_blocks tables prevent double votes at the schema-level. Only surrounding attestations could be entered into the database by parallel writers, if they were allowed.
• Attempts to sign the same block or attestation twice will be gently refused. Although this would be safe, the beacon node seems to dislike duplicate blocks and attestations (open to changing this).

[paulhauner]

Beautifully clean and elegant, as always :) I really like how simple this becomes with SQL and the concurrency properties we get from SQLite.

There's a few comments, but nothing structural. I also wanted to raise two things for consideration:

Using domain for uniqueness

I can see that a conflicting vote across different domains is prevented here, whilst not being slashable on-chain. I support this decision, it makes things much simpler and I really doubt we're ever going to support staking on two chains with the same VC instance.

I noticed that some of the tests were using the different domain to simulate "difference" instead of mutating some other field like beacon_block_root. Whilst I completely believe it's functionally equivalent, it did occur to me that the tests are using "same message different domain" to test that something is NoteSafe when technically that is Safe on the chain. Perhaps throwing in a couple of quick tests that mutate at least one of the AttestationData fields might be nice, as a token gesture if nothing else.

Verification of data read from the database

As noted in comments, we can hit panics/unintended behaviour if the Slot, Epoch or Hash256 values stored in the database are not properly formed. You can definitely make the argument that we're already relying upon (a) our input to the database to be consistent and (b) SQLite to maintain that consistency, so double-checking is a waste of time. But, unless it's messy, perhaps we should just double check? Perhaps you already have thought about this; I did notice theToSql/FromSql error types were kinda funky.

[paulhauner]

As mentioned in the main comment, we could use TryFrom<u64> for i64, seeing as we're in a Result context.

[michaelsproul]

Casting u64 <-> i64 is safe and lossless, but I refactored this to use an error type anyway

[paulhauner]

As above regarding Hash256::from_slice

[michaelsproul]

Fixed, good catch!

[michaelsproul]

All review comments have been addressed I think, just waiting on CI to pass.

Regarding the domain, you're right. Unless we stored a mapping from validator -> slot -> domain -> block, we can't support signing on multiple chains. And I agree the complexity wouldn't be worth it. I switched from storing the object root to storing the signing root in the database because it felt right to store the actual data being signed, and it prevents a peculiar case where a beacon node (malicious or otherwise) tries to get us to sign what appears to be "the same" attestation data with a different domain (which at the time I thought was slashable, but as you point out, is not). My general thinking with the slashing database was to only prevent slashing, and not try to prevent other strange signings that aren't slashable, as those can be the responsibility of the rest of the VC (making sure we're on the right chain, etc). But some false positives, if they simplify the design, seem tolerable.

I added a test (invalid_double_vote_diff_data) to check a mutation to the beacon_block_root, although a previous attestation mutating test existed (invalid_double_vote_diff_target which changes the target root).

[paulhauner]

But some false positives, if they simplify the design, seem tolerable.

I agree.

Looks great! This is has my approval. I'm going to flag it as blocked until we cut a v0.1.2 release, then we can include this in the v0.2.0 release :)