From 6faa9c678ed237d04bea7d317ad225019be8ec7e Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 19 Aug 2024 19:21:07 +1000 Subject: [PATCH] Prevent fd leak in random slasher tests (#6254) * Prevent fd leak in random slasher tests * Clippy --- slasher/src/database.rs | 60 ++++++++++++++++++++++++++++++++++++++-- slasher/src/slasher.rs | 18 ++++++++++++ slasher/tests/random.rs | 61 ++++++++++++++++++++++++++++++----------- 3 files changed, 121 insertions(+), 18 deletions(-) diff --git a/slasher/src/database.rs b/slasher/src/database.rs index 5c22c609828..b5d7ab5ce80 100644 --- a/slasher/src/database.rs +++ b/slasher/src/database.rs @@ -4,8 +4,8 @@ mod mdbx_impl; mod redb_impl; use crate::{ - metrics, AttesterRecord, AttesterSlashingStatus, CompactAttesterRecord, Config, Error, - ProposerSlashingStatus, + metrics, AttesterRecord, AttesterSlashingStatus, CompactAttesterRecord, Config, Database, + Error, ProposerSlashingStatus, }; use byteorder::{BigEndian, ByteOrder}; use interface::{Environment, OpenDatabases, RwTransaction}; @@ -350,6 +350,18 @@ impl SlasherDB { Ok(()) } + pub fn get_config(&self) -> &Config { + &self.config + } + + /// TESTING ONLY. + /// + /// Replace the config for this database. This is only a sane thing to do if the database + /// is empty (has been `reset`). + pub fn update_config(&mut self, config: Arc) { + self.config = config; + } + /// Load a config from disk. /// /// This is generic in order to allow loading of configs for different schema versions. @@ -799,6 +811,50 @@ impl SlasherDB { Ok(()) } + + /// Delete all data from the database, essentially re-initialising it. + /// + /// We use this reset pattern in tests instead of leaking tonnes of file descriptors and + /// exhausting our allocation by creating (and leaking) databases. + /// + /// THIS FUNCTION SHOULD ONLY BE USED IN TESTS. + pub fn reset(&self) -> Result<(), Error> { + // Clear the cache(s) first. + self.attestation_root_cache.lock().clear(); + + // Pattern match to avoid missing any database. + let OpenDatabases { + indexed_attestation_db, + indexed_attestation_id_db, + attesters_db, + attesters_max_targets_db, + min_targets_db, + max_targets_db, + current_epochs_db, + proposers_db, + metadata_db, + } = &self.databases; + let mut txn = self.begin_rw_txn()?; + self.reset_db(&mut txn, indexed_attestation_db)?; + self.reset_db(&mut txn, indexed_attestation_id_db)?; + self.reset_db(&mut txn, attesters_db)?; + self.reset_db(&mut txn, attesters_max_targets_db)?; + self.reset_db(&mut txn, min_targets_db)?; + self.reset_db(&mut txn, max_targets_db)?; + self.reset_db(&mut txn, current_epochs_db)?; + self.reset_db(&mut txn, proposers_db)?; + self.reset_db(&mut txn, metadata_db)?; + txn.commit() + } + + fn reset_db(&self, txn: &mut RwTransaction<'_>, db: &Database<'static>) -> Result<(), Error> { + let mut cursor = txn.cursor(db)?; + if cursor.first_key()?.is_none() { + return Ok(()); + } + cursor.delete_while(|_| Ok(true))?; + Ok(()) + } } #[cfg(test)] diff --git a/slasher/src/slasher.rs b/slasher/src/slasher.rs index 0bb7c9c3ffe..19f2cd138de 100644 --- a/slasher/src/slasher.rs +++ b/slasher/src/slasher.rs @@ -33,6 +33,19 @@ impl Slasher { config.validate()?; let config = Arc::new(config); let db = SlasherDB::open(config.clone(), spec, log.clone())?; + Self::from_config_and_db(config, db, log) + } + + /// TESTING ONLY. + /// + /// Initialise a slasher database from an existing `db`. The caller must ensure that the + /// database's config matches the one provided. + pub fn from_config_and_db( + config: Arc, + db: SlasherDB, + log: Logger, + ) -> Result { + config.validate()?; let attester_slashings = Mutex::new(HashSet::new()); let proposer_slashings = Mutex::new(HashSet::new()); let attestation_queue = AttestationQueue::default(); @@ -48,6 +61,11 @@ impl Slasher { }) } + pub fn into_reset_db(self) -> Result, Error> { + self.db.reset()?; + Ok(self.db) + } + /// Harvest all attester slashings found, removing them from the slasher. pub fn get_attester_slashings(&self) -> HashSet> { std::mem::take(&mut self.attester_slashings.lock()) diff --git a/slasher/tests/random.rs b/slasher/tests/random.rs index 0ba2986d44b..ff234dff3fe 100644 --- a/slasher/tests/random.rs +++ b/slasher/tests/random.rs @@ -7,10 +7,11 @@ use slasher::{ block, chain_spec, indexed_att, slashed_validators_from_attestations, slashed_validators_from_slashings, E, }, - Config, Slasher, + Config, Slasher, SlasherDB, }; use std::cmp::max; -use tempfile::tempdir; +use std::sync::Arc; +use tempfile::{tempdir, TempDir}; use types::{Epoch, EthSpec}; #[derive(Debug)] @@ -32,7 +33,16 @@ impl Default for TestConfig { } } -fn random_test(seed: u64, test_config: TestConfig) { +fn make_db() -> (TempDir, SlasherDB) { + let tempdir = tempdir().unwrap(); + let initial_config = Arc::new(Config::new(tempdir.path().into())); + let logger = test_logger(); + let spec = chain_spec(); + let db = SlasherDB::open(initial_config.clone(), spec, logger).unwrap(); + (tempdir, db) +} + +fn random_test(seed: u64, mut db: SlasherDB, test_config: TestConfig) -> SlasherDB { let check_slashings = test_config.check_slashings; let num_validators = test_config.num_validators; let max_attestations = test_config.max_attestations; @@ -40,18 +50,17 @@ fn random_test(seed: u64, test_config: TestConfig) { println!("Running with seed {}", seed); let mut rng = StdRng::seed_from_u64(seed); - let tempdir = tempdir().unwrap(); - - let mut config = Config::new(tempdir.path().into()); + let mut config = Config::new(db.get_config().database_path.clone()); config.validator_chunk_size = 1 << rng.gen_range(1..4); let chunk_size_exponent = rng.gen_range(1..4); config.chunk_size = 1 << chunk_size_exponent; config.history_length = 1 << rng.gen_range(chunk_size_exponent..chunk_size_exponent + 3); - let spec = chain_spec(); + let config = Arc::new(config); + db.update_config(config.clone()); - let slasher = Slasher::::open(config.clone(), spec, test_logger()).unwrap(); + let slasher = Slasher::::from_config_and_db(config.clone(), db, test_logger()).unwrap(); let validators = (0..num_validators as u64).collect::>(); @@ -121,7 +130,7 @@ fn random_test(seed: u64, test_config: TestConfig) { } if !check_slashings { - return; + return slasher.into_reset_db().unwrap(); } slasher.process_queued(current_epoch).unwrap(); @@ -131,6 +140,9 @@ fn random_test(seed: u64, test_config: TestConfig) { let slashed_validators = slashed_validators_from_slashings(&slashings); let expected_slashed_validators = slashed_validators_from_attestations(&attestations); assert_eq!(slashed_validators, expected_slashed_validators); + + // Return the database for reuse. + slasher.into_reset_db().unwrap() } // Fuzz-like test that runs forever on different seeds looking for crashes. @@ -138,8 +150,9 @@ fn random_test(seed: u64, test_config: TestConfig) { #[ignore] fn no_crash() { let mut rng = thread_rng(); + let (_tempdir, mut db) = make_db(); loop { - random_test(rng.gen(), TestConfig::default()); + db = random_test(rng.gen(), db, TestConfig::default()); } } @@ -148,9 +161,11 @@ fn no_crash() { #[ignore] fn no_crash_with_blocks() { let mut rng = thread_rng(); + let (_tempdir, mut db) = make_db(); loop { - random_test( + db = random_test( rng.gen(), + db, TestConfig { add_blocks: true, ..TestConfig::default() @@ -164,9 +179,11 @@ fn no_crash_with_blocks() { #[ignore] fn check_slashings() { let mut rng = thread_rng(); + let (_tempdir, mut db) = make_db(); loop { - random_test( + db = random_test( rng.gen(), + db, TestConfig { check_slashings: true, ..TestConfig::default() @@ -177,8 +194,10 @@ fn check_slashings() { #[test] fn check_slashings_example1() { + let (_tempdir, db) = make_db(); random_test( 1, + db, TestConfig { check_slashings: true, ..TestConfig::default() @@ -188,8 +207,10 @@ fn check_slashings_example1() { #[test] fn check_slashings_example2() { + let (_tempdir, db) = make_db(); random_test( 2, + db, TestConfig { check_slashings: true, max_attestations: 3, @@ -200,8 +221,10 @@ fn check_slashings_example2() { #[test] fn check_slashings_example3() { + let (_tempdir, db) = make_db(); random_test( 3, + db, TestConfig { check_slashings: true, max_attestations: 100, @@ -212,23 +235,28 @@ fn check_slashings_example3() { #[test] fn no_crash_example1() { - random_test(1, TestConfig::default()); + let (_tempdir, db) = make_db(); + random_test(1, db, TestConfig::default()); } #[test] fn no_crash_example2() { - random_test(2, TestConfig::default()); + let (_tempdir, db) = make_db(); + random_test(2, db, TestConfig::default()); } #[test] fn no_crash_example3() { - random_test(3, TestConfig::default()); + let (_tempdir, db) = make_db(); + random_test(3, db, TestConfig::default()); } #[test] fn no_crash_blocks_example1() { + let (_tempdir, db) = make_db(); random_test( 1, + db, TestConfig { add_blocks: true, ..TestConfig::default() @@ -238,5 +266,6 @@ fn no_crash_blocks_example1() { #[test] fn no_crash_aug_24() { - random_test(13519442335106054152, TestConfig::default()) + let (_tempdir, db) = make_db(); + random_test(13519442335106054152, db, TestConfig::default()); }