From 2adc6cb5e1c966e21c92494234f4670a439dc023 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 18 May 2021 11:42:19 +1000 Subject: [PATCH 1/5] Speed up JSON load in slashing protection import --- account_manager/src/validator/slashing_protection.rs | 2 ++ validator_client/slashing_protection/src/interchange.rs | 9 +++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/account_manager/src/validator/slashing_protection.rs b/account_manager/src/validator/slashing_protection.rs index 92209f6b558..d65c7769cfd 100644 --- a/account_manager/src/validator/slashing_protection.rs +++ b/account_manager/src/validator/slashing_protection.rs @@ -72,8 +72,10 @@ pub fn cli_run( ) })?; + eprintln!("Loading JSON file into memory & deserializing"); let interchange = Interchange::from_json_reader(&import_file) .map_err(|e| format!("Error parsing file for import: {:?}", e))?; + eprintln!("JSON load complete - performing import"); let slashing_protection_database = SlashingDatabase::open_or_create(&slashing_protection_db_path).map_err(|e| { diff --git a/validator_client/slashing_protection/src/interchange.rs b/validator_client/slashing_protection/src/interchange.rs index 97903a677e0..93053ee41d9 100644 --- a/validator_client/slashing_protection/src/interchange.rs +++ b/validator_client/slashing_protection/src/interchange.rs @@ -1,5 +1,6 @@ use serde_derive::{Deserialize, Serialize}; use std::collections::HashSet; +use std::io; use types::{Epoch, Hash256, PublicKeyBytes, Slot}; #[derive(Debug, Clone, PartialEq, Deserialize, Serialize)] @@ -49,8 +50,12 @@ impl Interchange { serde_json::from_str(json) } - pub fn from_json_reader(reader: impl std::io::Read) -> Result { - serde_json::from_reader(reader) + pub fn from_json_reader(mut reader: impl std::io::Read) -> Result { + // We read the entire file into memory first, as this is *a lot* faster than using + // `serde_json::from_reader`. See https://github.com/serde-rs/json/issues/160 + let mut json_str = String::new(); + reader.read_to_string(&mut json_str)?; + Ok(Interchange::from_json_str(&json_str)?) } pub fn write_to(&self, writer: impl std::io::Write) -> Result<(), serde_json::Error> { From 4c613815ea58d86378e4e2c604237babb9765745 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 31 May 2021 16:54:42 +1000 Subject: [PATCH 2/5] Implement minification for slash protection data --- Cargo.lock | 1 + .../src/validator/slashing_protection.rs | 47 ++++++++++- .../slashing_protection/Cargo.toml | 1 + .../src/bin/test_generator.rs | 4 +- .../slashing_protection/src/interchange.rs | 79 ++++++++++++++++++- .../src/interchange_test.rs | 21 +++-- .../slashing_protection/tests/interop.rs | 23 +++++- 7 files changed, 162 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a0ced6cf841..b9000ce2fb7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5803,6 +5803,7 @@ dependencies = [ name = "slashing_protection" version = "0.1.0" dependencies = [ + "lazy_static", "parking_lot", "r2d2", "r2d2_sqlite", diff --git a/account_manager/src/validator/slashing_protection.rs b/account_manager/src/validator/slashing_protection.rs index d65c7769cfd..94fbac549b6 100644 --- a/account_manager/src/validator/slashing_protection.rs +++ b/account_manager/src/validator/slashing_protection.rs @@ -15,6 +15,8 @@ pub const EXPORT_CMD: &str = "export"; pub const IMPORT_FILE_ARG: &str = "IMPORT-FILE"; pub const EXPORT_FILE_ARG: &str = "EXPORT-FILE"; +pub const MINIFY_FLAG: &str = "minify"; + pub fn cli_app<'a, 'b>() -> App<'a, 'b> { App::new(CMD) .about("Import or export slashing protection data to or from another client") @@ -26,6 +28,17 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .takes_value(true) .value_name("FILE") .help("The slashing protection interchange file to import (.json)"), + ) + .arg( + Arg::with_name(MINIFY_FLAG) + .long(MINIFY_FLAG) + .takes_value(true) + .default_value("true") + .possible_values(&["false", "true"]) + .help( + "Minify the input file before processing. This is *much* faster, \ + but will not detect slashable data in the input.", + ), ), ) .subcommand( @@ -36,6 +49,17 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { .takes_value(true) .value_name("FILE") .help("The filename to export the interchange file to"), + ) + .arg( + Arg::with_name(MINIFY_FLAG) + .long(MINIFY_FLAG) + .takes_value(true) + .default_value("false") + .possible_values(&["false", "true"]) + .help( + "Minify the output file. This will make it smaller and faster to \ + import, but not faster to generate.", + ), ), ) } @@ -64,6 +88,7 @@ pub fn cli_run( match matches.subcommand() { (IMPORT_CMD, Some(matches)) => { let import_filename: PathBuf = clap_utils::parse_required(&matches, IMPORT_FILE_ARG)?; + let minify: bool = clap_utils::parse_required(&matches, MINIFY_FLAG)?; let import_file = File::open(&import_filename).map_err(|e| { format!( "Unable to open import file at {}: {:?}", @@ -73,9 +98,17 @@ pub fn cli_run( })?; eprintln!("Loading JSON file into memory & deserializing"); - let interchange = Interchange::from_json_reader(&import_file) + let mut interchange = Interchange::from_json_reader(&import_file) .map_err(|e| format!("Error parsing file for import: {:?}", e))?; - eprintln!("JSON load complete - performing import"); + eprintln!("JSON load complete"); + + if minify { + eprintln!("Minifying input file for faster loading"); + interchange = interchange + .minify() + .map_err(|e| format!("Minification failed: {:?}", e))?; + eprintln!("Minification complete"); + } let slashing_protection_database = SlashingDatabase::open_or_create(&slashing_protection_db_path).map_err(|e| { @@ -149,6 +182,7 @@ pub fn cli_run( } (EXPORT_CMD, Some(matches)) => { let export_filename: PathBuf = clap_utils::parse_required(&matches, EXPORT_FILE_ARG)?; + let minify: bool = clap_utils::parse_required(&matches, MINIFY_FLAG)?; if !slashing_protection_db_path.exists() { return Err(format!( @@ -166,10 +200,17 @@ pub fn cli_run( ) })?; - let interchange = slashing_protection_database + let mut interchange = slashing_protection_database .export_interchange_info(genesis_validators_root) .map_err(|e| format!("Error during export: {:?}", e))?; + if minify { + eprintln!("Minifying output file"); + interchange = interchange + .minify() + .map_err(|e| format!("Unable to minify output: {:?}", e))?; + } + let output_file = File::create(export_filename) .map_err(|e| format!("Error creating output file: {:?}", e))?; diff --git a/validator_client/slashing_protection/Cargo.toml b/validator_client/slashing_protection/Cargo.toml index a1abb855631..188716ad945 100644 --- a/validator_client/slashing_protection/Cargo.toml +++ b/validator_client/slashing_protection/Cargo.toml @@ -18,4 +18,5 @@ serde_json = "1.0.58" serde_utils = { path = "../../consensus/serde_utils" } [dev-dependencies] +lazy_static = "1.4.0" rayon = "1.4.1" diff --git a/validator_client/slashing_protection/src/bin/test_generator.rs b/validator_client/slashing_protection/src/bin/test_generator.rs index 7788a8a1ca8..7e53de4a56c 100644 --- a/validator_client/slashing_protection/src/bin/test_generator.rs +++ b/validator_client/slashing_protection/src/bin/test_generator.rs @@ -344,8 +344,10 @@ fn main() { let output_dir = Path::new(&args[1]); fs::create_dir_all(output_dir).unwrap(); + let minify = false; + for test in tests { - test.run(); + test.run(minify); let f = File::create(output_dir.join(format!("{}.json", test.name))).unwrap(); serde_json::to_writer_pretty(&f, &test).unwrap(); writeln!(&f).unwrap(); diff --git a/validator_client/slashing_protection/src/interchange.rs b/validator_client/slashing_protection/src/interchange.rs index 93053ee41d9..a93b56d5bea 100644 --- a/validator_client/slashing_protection/src/interchange.rs +++ b/validator_client/slashing_protection/src/interchange.rs @@ -1,5 +1,6 @@ use serde_derive::{Deserialize, Serialize}; -use std::collections::HashSet; +use std::cmp::max; +use std::collections::{HashMap, HashSet}; use std::io; use types::{Epoch, Hash256, PublicKeyBytes, Slot}; @@ -45,6 +46,11 @@ pub struct Interchange { pub data: Vec, } +#[derive(Debug, Clone)] +pub enum InterchangeError { + MinAndMaxInconsistent, +} + impl Interchange { pub fn from_json_str(json: &str) -> Result { serde_json::from_str(json) @@ -78,4 +84,75 @@ impl Interchange { pub fn is_empty(&self) -> bool { self.len() == 0 } + + /// Minify an interchange by constructing a synthetic block & attestation for each validator. + pub fn minify(&self) -> Result { + // Map from pubkey to optional max block and max attestation. + let mut validator_data = + HashMap::, Option)>::new(); + + for data in self.data.iter() { + // Existing maximum attestation and maximum block. + let (max_block, max_attestation) = validator_data + .entry(data.pubkey) + .or_insert_with(|| (None, None)); + + // Find maximum source and target epochs. + let max_source_epoch = data + .signed_attestations + .iter() + .map(|attestation| attestation.source_epoch) + .max(); + let max_target_epoch = data + .signed_attestations + .iter() + .map(|attestation| attestation.target_epoch) + .max(); + + match (max_source_epoch, max_target_epoch) { + (Some(source_epoch), Some(target_epoch)) => { + if let Some(prev_max) = max_attestation { + prev_max.source_epoch = max(prev_max.source_epoch, source_epoch); + prev_max.target_epoch = max(prev_max.target_epoch, target_epoch); + } else { + *max_attestation = Some(SignedAttestation { + source_epoch, + target_epoch, + signing_root: None, + }); + } + } + (None, None) => {} + _ => return Err(InterchangeError::MinAndMaxInconsistent), + }; + + // Find maximum block slot. + let max_block_slot = data.signed_blocks.iter().map(|block| block.slot).max(); + + if let Some(max_slot) = max_block_slot { + if let Some(prev_max) = max_block { + prev_max.slot = max(prev_max.slot, max_slot); + } else { + *max_block = Some(SignedBlock { + slot: max_slot, + signing_root: None, + }); + } + } + } + + let data = validator_data + .into_iter() + .map(|(pubkey, (maybe_block, maybe_att))| InterchangeData { + pubkey, + signed_blocks: maybe_block.into_iter().collect(), + signed_attestations: maybe_att.into_iter().collect(), + }) + .collect(); + + Ok(Self { + metadata: self.metadata.clone(), + data, + }) + } } diff --git a/validator_client/slashing_protection/src/interchange_test.rs b/validator_client/slashing_protection/src/interchange_test.rs index b5a4b86eed5..5cb62e10cf2 100644 --- a/validator_client/slashing_protection/src/interchange_test.rs +++ b/validator_client/slashing_protection/src/interchange_test.rs @@ -58,16 +58,23 @@ impl MultiTestCase { self } - pub fn run(&self) { + pub fn run(&self, minify: bool) { let dir = tempdir().unwrap(); let slashing_db_file = dir.path().join("slashing_protection.sqlite"); let slashing_db = SlashingDatabase::create(&slashing_db_file).unwrap(); + // If minification is used, false positives are allowed, i.e. there may be some situations + // in which signing is safe but the minified file prevents it. + let allow_false_positives = minify; + for test_case in &self.steps { - match slashing_db.import_interchange_info( - test_case.interchange.clone(), - self.genesis_validators_root, - ) { + let interchange = if minify { + test_case.interchange.minify().unwrap() + } else { + test_case.interchange.clone() + }; + + match slashing_db.import_interchange_info(interchange, self.genesis_validators_root) { Ok(import_outcomes) => { let failed_records = import_outcomes .iter() @@ -107,7 +114,7 @@ impl MultiTestCase { i, self.name, safe ); } - Err(e) if block.should_succeed => { + Err(e) if block.should_succeed && !allow_false_positives => { panic!( "block {} from `{}` failed when it should have succeeded: {:?}", i, self.name, e @@ -130,7 +137,7 @@ impl MultiTestCase { i, self.name, safe ); } - Err(e) if att.should_succeed => { + Err(e) if att.should_succeed && !allow_false_positives => { panic!( "attestation {} from `{}` failed when it should have succeeded: {:?}", i, self.name, e diff --git a/validator_client/slashing_protection/tests/interop.rs b/validator_client/slashing_protection/tests/interop.rs index 62cd03e251e..ee5bb114712 100644 --- a/validator_client/slashing_protection/tests/interop.rs +++ b/validator_client/slashing_protection/tests/interop.rs @@ -1,7 +1,12 @@ +use lazy_static::lazy_static; use slashing_protection::interchange_test::MultiTestCase; use std::fs::File; use std::path::PathBuf; +lazy_static! { + pub static ref TEST_ROOT_DIR: PathBuf = test_root_dir(); +} + fn download_tests() { let make_output = std::process::Command::new("make") .current_dir(std::env::var("CARGO_MANIFEST_DIR").unwrap()) @@ -22,7 +27,21 @@ fn test_root_dir() -> PathBuf { #[test] fn generated() { - for entry in test_root_dir() + for entry in TEST_ROOT_DIR + .join("generated") + .read_dir() + .unwrap() + .map(Result::unwrap) + { + let file = File::open(entry.path()).unwrap(); + let test_case: MultiTestCase = serde_json::from_reader(&file).unwrap(); + test_case.run(false); + } +} + +#[test] +fn generated_with_minification() { + for entry in TEST_ROOT_DIR .join("generated") .read_dir() .unwrap() @@ -30,6 +49,6 @@ fn generated() { { let file = File::open(entry.path()).unwrap(); let test_case: MultiTestCase = serde_json::from_reader(&file).unwrap(); - test_case.run(); + test_case.run(true); } } From 17469e8c2ddca8f79b4d1936540f23e0cdb34170 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 15 Jun 2021 16:17:19 +1000 Subject: [PATCH 3/5] Make import atomic, expand testing --- .../src/validator/slashing_protection.rs | 112 ++++++++++------- book/src/slashing-protection.md | 35 +++++- .../src/bin/test_generator.rs | 59 ++++++++- .../slashing_protection/src/interchange.rs | 6 +- .../src/interchange_test.rs | 116 +++++++++++++++--- .../slashing_protection/src/lib.rs | 3 +- .../slashing_protection/src/minify_tests.rs | 1 + .../src/slashing_database.rs | 22 +++- 8 files changed, 273 insertions(+), 81 deletions(-) create mode 100644 validator_client/slashing_protection/src/minify_tests.rs diff --git a/account_manager/src/validator/slashing_protection.rs b/account_manager/src/validator/slashing_protection.rs index 94fbac549b6..00187ccd065 100644 --- a/account_manager/src/validator/slashing_protection.rs +++ b/account_manager/src/validator/slashing_protection.rs @@ -1,7 +1,7 @@ use clap::{App, Arg, ArgMatches}; use environment::Environment; use slashing_protection::{ - interchange::Interchange, InterchangeImportOutcome, SlashingDatabase, + interchange::Interchange, InterchangeError, InterchangeImportOutcome, SlashingDatabase, SLASHING_PROTECTION_FILENAME, }; use std::fs::File; @@ -97,17 +97,17 @@ pub fn cli_run( ) })?; - eprintln!("Loading JSON file into memory & deserializing"); + eprint!("Loading JSON file into memory & deserializing"); let mut interchange = Interchange::from_json_reader(&import_file) .map_err(|e| format!("Error parsing file for import: {:?}", e))?; - eprintln!("JSON load complete"); + eprintln!(" [done]."); if minify { - eprintln!("Minifying input file for faster loading"); + eprint!("Minifying input file for faster loading"); interchange = interchange .minify() .map_err(|e| format!("Minification failed: {:?}", e))?; - eprintln!("Minification complete"); + eprintln!(" [done]."); } let slashing_protection_database = @@ -119,16 +119,6 @@ pub fn cli_run( ) })?; - let outcomes = slashing_protection_database - .import_interchange_info(interchange, genesis_validators_root) - .map_err(|e| { - format!( - "Error during import: {:?}\n\ - IT IS NOT SAFE TO START VALIDATING", - e - ) - })?; - let display_slot = |slot: Option| { slot.map_or("none".to_string(), |slot| format!("{}", slot.as_u64())) }; @@ -140,43 +130,71 @@ pub fn cli_run( (source, target) => format!("{}=>{}", display_epoch(source), display_epoch(target)), }; - let mut num_failed = 0; - - for outcome in &outcomes { - match outcome { - InterchangeImportOutcome::Success { pubkey, summary } => { - eprintln!("- {:?} SUCCESS min block: {}, max block: {}, min attestation: {}, max attestation: {}", - pubkey, - display_slot(summary.min_block_slot), - display_slot(summary.max_block_slot), - display_attestation(summary.min_attestation_source, summary.min_attestation_target), - display_attestation(summary.max_attestation_source, - summary.max_attestation_target), - ); + match slashing_protection_database + .import_interchange_info(interchange, genesis_validators_root) + { + Ok(outcomes) => { + eprintln!("All records imported successfully:"); + for outcome in &outcomes { + match outcome { + InterchangeImportOutcome::Success { pubkey, summary } => { + eprintln!("- {:?}", pubkey); + eprintln!( + " - min block: {}", + display_slot(summary.min_block_slot) + ); + eprintln!( + " - min attestation: {}", + display_attestation( + summary.min_attestation_source, + summary.min_attestation_target + ) + ); + eprintln!( + " - max attestation: {}", + display_attestation( + summary.max_attestation_source, + summary.max_attestation_target + ) + ); + } + InterchangeImportOutcome::Failure { pubkey, error } => { + panic!( + "import should be atomic, but key {:?} was imported despite error: {:?}", + pubkey, error + ); + } + } } - InterchangeImportOutcome::Failure { pubkey, error } => { - eprintln!("- {:?} ERROR: {:?}", pubkey, error); - num_failed += 1; + } + Err(InterchangeError::AtomicBatchAborted(outcomes)) => { + eprintln!("ERROR, slashable data in input:"); + for outcome in &outcomes { + if let InterchangeImportOutcome::Failure { pubkey, error } = outcome { + eprintln!("- {:?}", pubkey); + eprintln!(" - error: {:?}", error); + } } + return Err( + "ERROR: import aborted due to slashable data, see above.\n\ + Please see https://lighthouse-book.sigmaprime.io/slashing-protection.html#slashable-data-in-import\n\ + IT IS NOT SAFE TO START VALIDATING".to_string() + ); + } + Err(e) => { + return Err(format!( + "Fatal error during import: {:?}\n\ + IT IS NOT SAFE TO START VALIDATING", + e + )); } } - if num_failed == 0 { - eprintln!("Import completed successfully."); - eprintln!( - "Please double-check that the minimum and maximum blocks and slots above \ - match your expectations." - ); - } else { - eprintln!( - "WARNING: history was NOT imported for {} of {} records", - num_failed, - outcomes.len() - ); - eprintln!("IT IS NOT SAFE TO START VALIDATING"); - eprintln!("Please see https://lighthouse-book.sigmaprime.io/slashing-protection.html#slashable-data-in-import"); - return Err("Partial import".to_string()); - } + eprintln!("Import completed successfully."); + eprintln!( + "Please double-check that the minimum and maximum blocks and attestations above \ + match your expectations." + ); Ok(()) } diff --git a/book/src/slashing-protection.md b/book/src/slashing-protection.md index 8763f8b109c..5fd536d2323 100644 --- a/book/src/slashing-protection.md +++ b/book/src/slashing-protection.md @@ -91,6 +91,32 @@ up to date. [EIP-3076]: https://eips.ethereum.org/EIPS/eip-3076 +### Minification + +Since version 1.5.0 Lighthouse automatically _minifies_ slashing protection data upon import. +Minification safely shrinks the input file, making it faster to import. + +If an import file contains slashable data, then its minification is still safe to import _even +though_ the non-minified file would fail to be imported. This means that leaving minification +enabled is recommended if the input could contain slashable data. Conversely, if you would like to +double-check that the input file is not slashable with respect to itself, then you should disable +minification. + +Minification can be disabled for imports by adding `--minify=false` to the command: + +``` +lighthouse account validator slashing-protection import --minify=false +``` + +It can also be enabled for exports (disabled by default): + +``` +lighthouse account validator slashing-protection export --minify=true +``` + +Minifying the export file should make it faster to import, and may allow it to be imported into an +implementation that is rejecting the non-minified equivalent due to slashable data. + ## Troubleshooting ### Misplaced Slashing Database @@ -137,11 +163,11 @@ and _could_ indicate a serious error or misconfiguration (see [Avoiding Slashing ### Slashable Data in Import -If you receive a warning when trying to import an [interchange file](#import-and-export) about +During import of an [interchange file](#import-and-export) if you receive an error about the file containing slashable data, then you must carefully consider whether you want to continue. -There are several potential causes for this warning, each of which require a different reaction. If -you have seen the warning for multiple validator keys, the cause could be different for each of them. +There are several potential causes for this error, each of which require a different reaction. If +the error output lists multiple validator keys, the cause could be different for each of them. 1. Your validator has actually signed slashable data. If this is the case, you should assess whether your validator has been slashed (or is likely to be slashed). It's up to you @@ -156,6 +182,9 @@ you have seen the warning for multiple validator keys, the cause could be differ It might be safe to continue as-is, or you could consider a [Drop and Re-import](#drop-and-re-import). +If you are running the import command with `--minify=false`, you should consider enabling +[minification](#minification). + #### Drop and Re-import If you'd like to prioritize an interchange file over any existing database stored by Lighthouse diff --git a/validator_client/slashing_protection/src/bin/test_generator.rs b/validator_client/slashing_protection/src/bin/test_generator.rs index 7e53de4a56c..94ad6933a69 100644 --- a/validator_client/slashing_protection/src/bin/test_generator.rs +++ b/validator_client/slashing_protection/src/bin/test_generator.rs @@ -224,10 +224,37 @@ fn main() { .with_blocks(vec![(0, 20, false)]), ], ), + MultiTestCase::new( + "multiple_interchanges_single_validator_fail_iff_imported", + vec![ + TestCase::new(interchange(vec![(0, vec![40], vec![])])), + TestCase::new(interchange(vec![(0, vec![20, 50], vec![])])) + .allow_partial_import() + .with_blocks(vec![(0, 20, false), (0, 50, false)]), + ], + ), MultiTestCase::single( "single_validator_source_greater_than_target", TestCase::new(interchange(vec![(0, vec![], vec![(8, 7)])])).allow_partial_import(), ), + MultiTestCase::single( + "single_validator_source_greater_than_target_surrounding", + TestCase::new(interchange(vec![(0, vec![], vec![(5, 2)])])) + .allow_partial_import() + .with_attestations(vec![(0, 3, 4, false)]), + ), + MultiTestCase::single( + "single_validator_source_greater_than_target_surrounded", + TestCase::new(interchange(vec![(0, vec![], vec![(5, 2)])])) + .allow_partial_import() + .with_attestations(vec![(0, 6, 1, false)]), + ), + MultiTestCase::single( + "single_validator_source_greater_than_target_sensible_iff_minified", + TestCase::new(interchange(vec![(0, vec![], vec![(5, 2), (6, 7)])])) + .allow_partial_import() + .with_attestations(vec![(0, 5, 8, false), (0, 6, 8, true)]), + ), MultiTestCase::single( "single_validator_out_of_order_blocks", TestCase::new(interchange(vec![(0, vec![6, 5], vec![])])).with_blocks(vec![ @@ -338,16 +365,42 @@ fn main() { .with_blocks(vec![(0, 10, false), (0, 13, false), (0, 14, true)]) .with_attestations(vec![(0, 0, 2, false), (0, 1, 3, false)]), ), + MultiTestCase::single( + "duplicate_pubkey_slashable_block", + TestCase::new(interchange(vec![ + (0, vec![10], vec![(0, 2)]), + (0, vec![10], vec![(1, 3)]), + ])) + .allow_partial_import() + .with_blocks(vec![(0, 10, false), (0, 11, true)]), + ), + MultiTestCase::single( + "duplicate_pubkey_slashable_attestation", + TestCase::new(interchange_with_signing_roots(vec![ + (0, vec![], vec![(0, 3, Some(3))]), + (0, vec![], vec![(1, 2, None)]), + ])) + .allow_partial_import() + .with_attestations(vec![ + (0, 0, 1, false), + (0, 0, 2, false), + (0, 0, 4, false), + (0, 1, 4, true), + ]), + ), ]; let args = std::env::args().collect::>(); let output_dir = Path::new(&args[1]); fs::create_dir_all(output_dir).unwrap(); - let minify = false; - for test in tests { - test.run(minify); + // Check that test case passes without minification + test.run(false); + + // Check that test case passes with minification + test.run(true); + let f = File::create(output_dir.join(format!("{}.json", test.name))).unwrap(); serde_json::to_writer_pretty(&f, &test).unwrap(); writeln!(&f).unwrap(); diff --git a/validator_client/slashing_protection/src/interchange.rs b/validator_client/slashing_protection/src/interchange.rs index a93b56d5bea..faddd788efc 100644 --- a/validator_client/slashing_protection/src/interchange.rs +++ b/validator_client/slashing_protection/src/interchange.rs @@ -1,3 +1,4 @@ +use crate::InterchangeError; use serde_derive::{Deserialize, Serialize}; use std::cmp::max; use std::collections::{HashMap, HashSet}; @@ -46,11 +47,6 @@ pub struct Interchange { pub data: Vec, } -#[derive(Debug, Clone)] -pub enum InterchangeError { - MinAndMaxInconsistent, -} - impl Interchange { pub fn from_json_str(json: &str) -> Result { serde_json::from_str(json) diff --git a/validator_client/slashing_protection/src/interchange_test.rs b/validator_client/slashing_protection/src/interchange_test.rs index 5cb62e10cf2..8e7f98ec6f5 100644 --- a/validator_client/slashing_protection/src/interchange_test.rs +++ b/validator_client/slashing_protection/src/interchange_test.rs @@ -1,9 +1,10 @@ use crate::{ - interchange::Interchange, + interchange::{Interchange, SignedAttestation, SignedBlock}, test_utils::{pubkey, DEFAULT_GENESIS_VALIDATORS_ROOT}, SigningRoot, SlashingDatabase, }; use serde_derive::{Deserialize, Serialize}; +use std::collections::HashSet; use tempfile::tempdir; use types::{Epoch, Hash256, PublicKeyBytes, Slot}; @@ -68,38 +69,43 @@ impl MultiTestCase { let allow_false_positives = minify; for test_case in &self.steps { + // If the test case is marked as containing slashable data, then it is permissible + // that we fail to import the file, in which case execution of the whole test should + // be aborted. + let allow_import_failure = test_case.allow_partial_import; + let interchange = if minify { - test_case.interchange.minify().unwrap() + let minified = test_case.interchange.minify().unwrap(); + check_minification_invariants(&test_case.interchange, &minified); + minified } else { test_case.interchange.clone() }; match slashing_db.import_interchange_info(interchange, self.genesis_validators_root) { Ok(import_outcomes) => { - let failed_records = import_outcomes - .iter() - .filter(|o| o.failed()) - .collect::>(); + let none_failed = import_outcomes.iter().all(|o| !o.failed()); + assert!( + none_failed, + "test `{}` failed to import some records: {:#?}", + self.name, import_outcomes + ); if !test_case.should_succeed { panic!( "test `{}` succeeded on import when it should have failed", self.name ); } - if !failed_records.is_empty() && !test_case.allow_partial_import { + } + Err(e) => { + if test_case.should_succeed && !allow_import_failure { panic!( - "test `{}` failed to import some records but should have succeeded: {:#?}", - self.name, failed_records, + "test `{}` failed on import when it should have succeeded, error: {:?}", + self.name, e ); } + break; } - Err(e) if test_case.should_succeed => { - panic!( - "test `{}` failed on import when it should have succeeded, error: {:?}", - self.name, e - ); - } - _ => (), } for (i, block) in test_case.blocks.iter().enumerate() { @@ -223,3 +229,81 @@ impl TestCase { self } } + +fn check_minification_invariants(interchange: &Interchange, minified: &Interchange) { + // Metadata should be unchanged. + assert_eq!(interchange.metadata, minified.metadata); + + // Minified data should contain one entry per *unique* public key. + let uniq_pubkeys = get_uniq_pubkeys(interchange); + assert_eq!(uniq_pubkeys, get_uniq_pubkeys(minified)); + assert_eq!(uniq_pubkeys.len(), minified.data.len()); + + for &pubkey in uniq_pubkeys.iter() { + // Minified data should contain 1 block per validator, unless the validator never signed any + // blocks. All of those blocks should have slots <= the slot of the minified block. + let original_blocks = get_blocks_of_validator(interchange, pubkey); + let minified_blocks = get_blocks_of_validator(minified, pubkey); + + if original_blocks.is_empty() { + assert!(minified_blocks.is_empty()); + } else { + // Should have exactly 1 block. + assert_eq!(minified_blocks.len(), 1); + + // That block should have no signing root (it's synthetic). + let mini_block = minified_blocks.first().unwrap(); + assert_eq!(mini_block.signing_root, None); + + // All original blocks should have slots <= the mini block. + assert!(original_blocks + .iter() + .all(|block| block.slot <= mini_block.slot)); + } + + // Minified data should contain 1 attestation per validator, unless the validator never + // signed any attestations. All attestations should have source and target <= the source + // and target of the minified attestation. + let original_attestations = get_attestations_of_validator(interchange, pubkey); + let minified_attestations = get_attestations_of_validator(minified, pubkey); + + if original_attestations.is_empty() { + assert!(minified_attestations.is_empty()); + } else { + assert_eq!(minified_attestations.len(), 1); + + let mini_attestation = minified_attestations.first().unwrap(); + assert_eq!(mini_attestation.signing_root, None); + + assert!(original_attestations + .iter() + .all(|att| att.source_epoch <= mini_attestation.source_epoch + && att.target_epoch <= mini_attestation.target_epoch)); + } + } +} + +fn get_uniq_pubkeys(interchange: &Interchange) -> HashSet { + interchange.data.iter().map(|data| data.pubkey).collect() +} + +fn get_blocks_of_validator(interchange: &Interchange, pubkey: PublicKeyBytes) -> Vec<&SignedBlock> { + interchange + .data + .iter() + .filter(|data| data.pubkey == pubkey) + .flat_map(|data| data.signed_blocks.iter()) + .collect() +} + +fn get_attestations_of_validator( + interchange: &Interchange, + pubkey: PublicKeyBytes, +) -> Vec<&SignedAttestation> { + interchange + .data + .iter() + .filter(|data| data.pubkey == pubkey) + .flat_map(|data| data.signed_attestations.iter()) + .collect() +} diff --git a/validator_client/slashing_protection/src/lib.rs b/validator_client/slashing_protection/src/lib.rs index 8f6bdb50e9e..8dbb8c5d986 100644 --- a/validator_client/slashing_protection/src/lib.rs +++ b/validator_client/slashing_protection/src/lib.rs @@ -12,7 +12,8 @@ pub mod test_utils; pub use crate::signed_attestation::{InvalidAttestation, SignedAttestation}; pub use crate::signed_block::{InvalidBlock, SignedBlock}; pub use crate::slashing_database::{ - InterchangeImportOutcome, SlashingDatabase, SUPPORTED_INTERCHANGE_FORMAT_VERSION, + InterchangeError, InterchangeImportOutcome, SlashingDatabase, + SUPPORTED_INTERCHANGE_FORMAT_VERSION, }; use rusqlite::Error as SQLError; use std::io::{Error as IOError, ErrorKind}; diff --git a/validator_client/slashing_protection/src/minify_tests.rs b/validator_client/slashing_protection/src/minify_tests.rs new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/validator_client/slashing_protection/src/minify_tests.rs @@ -0,0 +1 @@ + diff --git a/validator_client/slashing_protection/src/slashing_database.rs b/validator_client/slashing_protection/src/slashing_database.rs index 79bcec7a93d..2a9b79cd673 100644 --- a/validator_client/slashing_protection/src/slashing_database.rs +++ b/validator_client/slashing_protection/src/slashing_database.rs @@ -576,8 +576,8 @@ impl SlashingDatabase { /// Import slashing protection from another client in the interchange format. /// - /// Return a vector of public keys and errors for any validators whose data could not be - /// imported. + /// This function will atomically import the entire interchange, failing if *any* + /// record cannot be imported. pub fn import_interchange_info( &self, interchange: Interchange, @@ -595,25 +595,33 @@ impl SlashingDatabase { }); } + // Create a single transaction for the entire batch, which will only be committed if + // all records are imported successfully. let mut conn = self.conn_pool.get()?; + let txn = conn.transaction()?; let mut import_outcomes = vec![]; + let mut commit = true; for record in interchange.data { let pubkey = record.pubkey; - let txn = conn.transaction()?; match self.import_interchange_record(record, &txn) { Ok(summary) => { import_outcomes.push(InterchangeImportOutcome::Success { pubkey, summary }); - txn.commit()?; } Err(error) => { import_outcomes.push(InterchangeImportOutcome::Failure { pubkey, error }); + commit = false; } } } - Ok(import_outcomes) + if commit { + txn.commit()?; + Ok(import_outcomes) + } else { + Err(InterchangeError::AtomicBatchAborted(import_outcomes)) + } } pub fn import_interchange_record( @@ -928,12 +936,14 @@ pub enum InterchangeError { interchange_file: Hash256, client: Hash256, }, - MinimalAttestationSourceAndTargetInconsistent, + MinAndMaxInconsistent, SQLError(String), SQLPoolError(r2d2::Error), SerdeJsonError(serde_json::Error), InvalidPubkey(String), NotSafe(NotSafe), + /// One or more records were found to be slashable, so the whole batch was aborted. + AtomicBatchAborted(Vec), } impl From for InterchangeError { From f56410c5a98e0113f4e1c7e183c8078091853e1e Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 16 Jun 2021 11:07:53 +1000 Subject: [PATCH 4/5] Bump tests, delete empty file --- validator_client/slashing_protection/Makefile | 2 +- validator_client/slashing_protection/src/minify_tests.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) delete mode 100644 validator_client/slashing_protection/src/minify_tests.rs diff --git a/validator_client/slashing_protection/Makefile b/validator_client/slashing_protection/Makefile index ae1f2e3d08a..05be7089a5e 100644 --- a/validator_client/slashing_protection/Makefile +++ b/validator_client/slashing_protection/Makefile @@ -1,4 +1,4 @@ -TESTS_TAG := f495032df9c26c678536cd2b7854e836ea94c217 +TESTS_TAG := v5.1.0-alpha.1 GENERATE_DIR := generated-tests OUTPUT_DIR := interchange-tests TARBALL := $(OUTPUT_DIR)-$(TESTS_TAG).tar.gz diff --git a/validator_client/slashing_protection/src/minify_tests.rs b/validator_client/slashing_protection/src/minify_tests.rs deleted file mode 100644 index 8b137891791..00000000000 --- a/validator_client/slashing_protection/src/minify_tests.rs +++ /dev/null @@ -1 +0,0 @@ - From b5884a45daad50335077a3ef4b14d1b17ad88798 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 21 Jun 2021 15:33:11 +1000 Subject: [PATCH 5/5] Update tests to v5.1.0 --- validator_client/slashing_protection/Makefile | 2 +- .../src/bin/test_generator.rs | 26 +++++++++---------- .../src/interchange_test.rs | 10 +++---- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/validator_client/slashing_protection/Makefile b/validator_client/slashing_protection/Makefile index 05be7089a5e..0734f55e28d 100644 --- a/validator_client/slashing_protection/Makefile +++ b/validator_client/slashing_protection/Makefile @@ -1,4 +1,4 @@ -TESTS_TAG := v5.1.0-alpha.1 +TESTS_TAG := v5.1.0 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 94ad6933a69..4b725436095 100644 --- a/validator_client/slashing_protection/src/bin/test_generator.rs +++ b/validator_client/slashing_protection/src/bin/test_generator.rs @@ -220,7 +220,7 @@ fn main() { vec![ TestCase::new(interchange(vec![(0, vec![40], vec![])])), TestCase::new(interchange(vec![(0, vec![20], vec![])])) - .allow_partial_import() + .contains_slashable_data() .with_blocks(vec![(0, 20, false)]), ], ), @@ -229,30 +229,30 @@ fn main() { vec![ TestCase::new(interchange(vec![(0, vec![40], vec![])])), TestCase::new(interchange(vec![(0, vec![20, 50], vec![])])) - .allow_partial_import() + .contains_slashable_data() .with_blocks(vec![(0, 20, false), (0, 50, false)]), ], ), MultiTestCase::single( "single_validator_source_greater_than_target", - TestCase::new(interchange(vec![(0, vec![], vec![(8, 7)])])).allow_partial_import(), + TestCase::new(interchange(vec![(0, vec![], vec![(8, 7)])])).contains_slashable_data(), ), MultiTestCase::single( "single_validator_source_greater_than_target_surrounding", TestCase::new(interchange(vec![(0, vec![], vec![(5, 2)])])) - .allow_partial_import() + .contains_slashable_data() .with_attestations(vec![(0, 3, 4, false)]), ), MultiTestCase::single( "single_validator_source_greater_than_target_surrounded", TestCase::new(interchange(vec![(0, vec![], vec![(5, 2)])])) - .allow_partial_import() + .contains_slashable_data() .with_attestations(vec![(0, 6, 1, false)]), ), MultiTestCase::single( "single_validator_source_greater_than_target_sensible_iff_minified", TestCase::new(interchange(vec![(0, vec![], vec![(5, 2), (6, 7)])])) - .allow_partial_import() + .contains_slashable_data() .with_attestations(vec![(0, 5, 8, false), (0, 6, 8, true)]), ), MultiTestCase::single( @@ -331,11 +331,11 @@ fn main() { vec![(10, Some(0)), (10, Some(11))], vec![], )])) - .allow_partial_import(), + .contains_slashable_data(), ), MultiTestCase::single( "single_validator_slashable_blocks_no_root", - TestCase::new(interchange(vec![(0, vec![10, 10], vec![])])).allow_partial_import(), + TestCase::new(interchange(vec![(0, vec![10, 10], vec![])])).contains_slashable_data(), ), MultiTestCase::single( "single_validator_slashable_attestations_double_vote", @@ -344,17 +344,17 @@ fn main() { vec![], vec![(2, 3, Some(0)), (2, 3, Some(1))], )])) - .allow_partial_import(), + .contains_slashable_data(), ), MultiTestCase::single( "single_validator_slashable_attestations_surrounds_existing", TestCase::new(interchange(vec![(0, vec![], vec![(2, 3), (0, 4)])])) - .allow_partial_import(), + .contains_slashable_data(), ), MultiTestCase::single( "single_validator_slashable_attestations_surrounded_by_existing", TestCase::new(interchange(vec![(0, vec![], vec![(0, 4), (2, 3)])])) - .allow_partial_import(), + .contains_slashable_data(), ), MultiTestCase::single( "duplicate_pubkey_not_slashable", @@ -371,7 +371,7 @@ fn main() { (0, vec![10], vec![(0, 2)]), (0, vec![10], vec![(1, 3)]), ])) - .allow_partial_import() + .contains_slashable_data() .with_blocks(vec![(0, 10, false), (0, 11, true)]), ), MultiTestCase::single( @@ -380,7 +380,7 @@ fn main() { (0, vec![], vec![(0, 3, Some(3))]), (0, vec![], vec![(1, 2, None)]), ])) - .allow_partial_import() + .contains_slashable_data() .with_attestations(vec![ (0, 0, 1, false), (0, 0, 2, false), diff --git a/validator_client/slashing_protection/src/interchange_test.rs b/validator_client/slashing_protection/src/interchange_test.rs index 8e7f98ec6f5..9d81d560871 100644 --- a/validator_client/slashing_protection/src/interchange_test.rs +++ b/validator_client/slashing_protection/src/interchange_test.rs @@ -18,7 +18,7 @@ pub struct MultiTestCase { #[derive(Debug, Clone, Deserialize, Serialize)] pub struct TestCase { pub should_succeed: bool, - pub allow_partial_import: bool, + pub contains_slashable_data: bool, pub interchange: Interchange, pub blocks: Vec, pub attestations: Vec, @@ -72,7 +72,7 @@ impl MultiTestCase { // If the test case is marked as containing slashable data, then it is permissible // that we fail to import the file, in which case execution of the whole test should // be aborted. - let allow_import_failure = test_case.allow_partial_import; + let allow_import_failure = test_case.contains_slashable_data; let interchange = if minify { let minified = test_case.interchange.minify().unwrap(); @@ -160,7 +160,7 @@ impl TestCase { pub fn new(interchange: Interchange) -> Self { TestCase { should_succeed: true, - allow_partial_import: false, + contains_slashable_data: false, interchange, blocks: vec![], attestations: vec![], @@ -172,8 +172,8 @@ impl TestCase { self } - pub fn allow_partial_import(mut self) -> Self { - self.allow_partial_import = true; + pub fn contains_slashable_data(mut self) -> Self { + self.contains_slashable_data = true; self }