cerc-io/lighthouse
16
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Fix assert in slashing protection import (#2881)

Browse Source 
## Issue Addressed

There was an overeager assert in the import of slashing protection data here:

fff01b24dd/validator_client/slashing_protection/src/slashing_database.rs (L939)

We were asserting that if the import contained any blocks for a validator, then the database should contain only a single block for that validator due to pruning/consolidation. However, we would only prune if the import contained _relevant blocks_ (that would actually change the maximum slot):

fff01b24dd/validator_client/slashing_protection/src/slashing_database.rs (L629-L633)

This lead to spurious failures (in the form of `ConsistencyError`s) when importing an interchange containing no new blocks for any of the validators. This wasn't hard to trigger, e.g. export and then immediately re-import the same file.

## Proposed Changes

This PR fixes the issue by simplifying the import so that it's more like the import for attestations. I.e. we make the assert true by always pruning when the imported file contains blocks.

In practice this doesn't have any downsides: if we import a new block then the behaviour is as before, except that we drop the `signing_root`. If we import an existing block or an old block then we prune the database to a single block. The only time this would be relevant is during extreme clock drift locally _plus_ import of a non-drifted interchange, which should occur infrequently.

## Additional Info

I've also added `Arbitrary` implementations to the slashing protection types so that we can fuzz them. I have a fuzzer sitting in a separate directory which I may or may not commit in a subsequent PR.

There's a new test in the standard interchange tests v5.2.1 that checks for this issue: https://github.com/eth-clients/slashing-protection-interchange-tests/pull/12
This commit is contained in:
Michael Sproul 2022-01-04 20:46:44 +00:00
parent dfc8968201
commit 0b54ff17f2
8 changed files with 56 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Cargo.lock generated
View File

@ -5479,6 +5479,7 @@ dependencies = [
name = "slashing_protection"
version = "0.1.0"
dependencies = [
"arbitrary",
"eth2_serde_utils",
"filesystem",
"lazy_static",

5
Makefile
View File

@ -157,9 +157,10 @@ lint:
make-ef-tests:
make -C $(EF_TESTS)
# Verifies that state_processing feature arbitrary-fuzz will compile
# Verifies that crates compile with fuzzing features enabled
arbitrary-fuzz:
cargo check --manifest-path=consensus/state_processing/Cargo.toml --features arbitrary-fuzz
cargo check -p state_processing --features arbitrary-fuzz
cargo check -p slashing_protection --features arbitrary-fuzz
# Runs cargo audit (Audit Cargo.lock files for crates with security vulnerabilities reported to the RustSec Advisory Database)
audit:

4
validator_client/slashing_protection/Cargo.toml
View File

@ -15,7 +15,11 @@ serde_derive = "1.0.116"
serde_json = "1.0.58"
eth2_serde_utils = "0.1.1"
filesystem = { path = "../../common/filesystem" }
arbitrary = { version = "1.0", features = ["derive"], optional = true }
[dev-dependencies]
lazy_static = "1.4.0"
rayon = "1.4.1"
[features]
arbitrary-fuzz = ["arbitrary", "types/arbitrary-fuzz"]

2
validator_client/slashing_protection/Makefile
View File

@ -1,4 +1,4 @@
TESTS_TAG := v5.2.0
TESTS_TAG := v5.2.1
GENERATE_DIR := generated-tests
OUTPUT_DIR := interchange-tests
TARBALL := $(OUTPUT_DIR)-$(TESTS_TAG).tar.gz

13
validator_client/slashing_protection/src/bin/test_generator.rs
View File

@ -224,6 +224,19 @@ fn main() {
.with_blocks(vec![(0, 20, false)]),
],
),
MultiTestCase::new(
"multiple_interchanges_single_validator_multiple_blocks_out_of_order",
vec![
TestCase::new(interchange(vec![(0, vec![0], vec![])])).with_blocks(vec![
(0, 10, true),
(0, 20, true),
(0, 30, true),
]),
TestCase::new(interchange(vec![(0, vec![20], vec![])]))
.contains_slashable_data()
.with_blocks(vec![(0, 29, false)]),
],
),
MultiTestCase::new(
"multiple_interchanges_single_validator_fail_iff_imported",
vec![

5
validator_client/slashing_protection/src/interchange.rs
View File

@ -7,6 +7,7 @@ use types::{Epoch, Hash256, PublicKeyBytes, Slot};
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct InterchangeMetadata {
#[serde(with = "eth2_serde_utils::quoted_u64::require_quotes")]
pub interchange_format_version: u64,
@ -15,6 +16,7 @@ pub struct InterchangeMetadata {
#[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct InterchangeData {
pub pubkey: PublicKeyBytes,
pub signed_blocks: Vec<SignedBlock>,
@ -23,6 +25,7 @@ pub struct InterchangeData {
#[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct SignedBlock {
#[serde(with = "eth2_serde_utils::quoted_u64::require_quotes")]
pub slot: Slot,
@ -32,6 +35,7 @@ pub struct SignedBlock {
#[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct SignedAttestation {
#[serde(with = "eth2_serde_utils::quoted_u64::require_quotes")]
pub source_epoch: Epoch,
@ -42,6 +46,7 @@ pub struct SignedAttestation {
}
#[derive(Debug, Clone, PartialEq, Deserialize, Serialize)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct Interchange {
pub metadata: InterchangeMetadata,
pub data: Vec<InterchangeData>,

6
validator_client/slashing_protection/src/interchange_test.rs
View File

@ -9,6 +9,7 @@ use tempfile::tempdir;
use types::{Epoch, Hash256, PublicKeyBytes, Slot};
#[derive(Debug, Clone, Deserialize, Serialize)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct MultiTestCase {
pub name: String,
pub genesis_validators_root: Hash256,
@ -16,6 +17,7 @@ pub struct MultiTestCase {
}
#[derive(Debug, Clone, Deserialize, Serialize)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct TestCase {
pub should_succeed: bool,
pub contains_slashable_data: bool,
@ -25,6 +27,7 @@ pub struct TestCase {
}
#[derive(Debug, Clone, Deserialize, Serialize)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct TestBlock {
pub pubkey: PublicKeyBytes,
pub slot: Slot,
@ -33,6 +36,7 @@ pub struct TestBlock {
}
#[derive(Debug, Clone, Deserialize, Serialize)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct TestAttestation {
pub pubkey: PublicKeyBytes,
pub source_epoch: Epoch,
@ -230,7 +234,7 @@ impl TestCase {
}
}
fn check_minification_invariants(interchange: &Interchange, minified: &Interchange) {
pub fn check_minification_invariants(interchange: &Interchange, minified: &Interchange) {
// Metadata should be unchanged.
assert_eq!(interchange.metadata, minified.metadata);

43
validator_client/slashing_protection/src/slashing_database.rs
View File

@ -648,29 +648,17 @@ impl SlashingDatabase {
// Summary of minimum and maximum messages pre-import.
let prev_summary = self.validator_summary(pubkey, txn)?;
// If the interchange contains a new maximum slot block, import it.
// If the interchange contains any blocks, update the database with the new max slot.
let max_block = record.signed_blocks.iter().max_by_key(|b| b.slot);
if let Some(max_block) = max_block {
// Block is relevant if there are no previous blocks, or new block has slot greater than
// previous maximum.
if prev_summary
.max_block_slot
.map_or(true, |max_block_slot| max_block.slot > max_block_slot)
{
self.insert_block_proposal(
txn,
pubkey,
max_block.slot,
max_block
.signing_root
.map(SigningRoot::from)
.unwrap_or_default(),
)?;
// Store new synthetic block with maximum slot and null signing root. Remove all other
// blocks.
let new_max_slot = max_or(prev_summary.max_block_slot, max_block.slot);
let signing_root = SigningRoot::default();
// Prune the database so that it contains *only* the new block.
self.prune_signed_blocks(&record.pubkey, max_block.slot, txn)?;
}
self.clear_signed_blocks(pubkey, txn)?;
self.insert_block_proposal(txn, pubkey, new_max_slot, signing_root)?;
}
// Find the attestations with max source and max target. Unless the input contains slashable
@ -901,6 +889,23 @@ impl SlashingDatabase {
Ok(())
}
/// Remove all blocks signed by a given `public_key`.
///
/// Dangerous, should only be used immediately before inserting a new block in the same
/// transacation.
fn clear_signed_blocks(
&self,
public_key: &PublicKeyBytes,
txn: &Transaction,
) -> Result<(), NotSafe> {
let validator_id = self.get_validator_id_in_txn(txn, public_key)?;
txn.execute(
"DELETE FROM signed_blocks WHERE validator_id = ?1",
params![validator_id],
)?;
Ok(())
}
/// Prune the signed attestations table for the given validator keys.
pub fn prune_all_signed_attestations<'a>(
&self,