diff --git a/Cargo.lock b/Cargo.lock index b7a14e1735..d2d9f799b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5479,6 +5479,7 @@ dependencies = [ name = "slashing_protection" version = "0.1.0" dependencies = [ + "arbitrary", "eth2_serde_utils", "filesystem", "lazy_static", diff --git a/Makefile b/Makefile index 6856635ebd..494f325d26 100644 --- a/Makefile +++ b/Makefile @@ -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: diff --git a/validator_client/slashing_protection/Cargo.toml b/validator_client/slashing_protection/Cargo.toml index 9cfe0ab4ea..634e49feea 100644 --- a/validator_client/slashing_protection/Cargo.toml +++ b/validator_client/slashing_protection/Cargo.toml @@ -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"] diff --git a/validator_client/slashing_protection/Makefile b/validator_client/slashing_protection/Makefile index 5787590260..ea51193a54 100644 --- a/validator_client/slashing_protection/Makefile +++ b/validator_client/slashing_protection/Makefile @@ -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 diff --git a/validator_client/slashing_protection/src/bin/test_generator.rs b/validator_client/slashing_protection/src/bin/test_generator.rs index 2bca9727af..b96dd8eb79 100644 --- a/validator_client/slashing_protection/src/bin/test_generator.rs +++ b/validator_client/slashing_protection/src/bin/test_generator.rs @@ -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![ diff --git a/validator_client/slashing_protection/src/interchange.rs b/validator_client/slashing_protection/src/interchange.rs index a9185e5bb2..3793766b6a 100644 --- a/validator_client/slashing_protection/src/interchange.rs +++ b/validator_client/slashing_protection/src/interchange.rs @@ -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, @@ -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, diff --git a/validator_client/slashing_protection/src/interchange_test.rs b/validator_client/slashing_protection/src/interchange_test.rs index 6bd6ce38b3..dc828773b9 100644 --- a/validator_client/slashing_protection/src/interchange_test.rs +++ b/validator_client/slashing_protection/src/interchange_test.rs @@ -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); diff --git a/validator_client/slashing_protection/src/slashing_database.rs b/validator_client/slashing_protection/src/slashing_database.rs index 725aa6057d..2b187f46ef 100644 --- a/validator_client/slashing_protection/src/slashing_database.rs +++ b/validator_client/slashing_protection/src/slashing_database.rs @@ -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,