From bfabaa10e0caa751081d9e8e74e3cc457b3ccacf Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 10 Nov 2022 16:53:40 +1100 Subject: [PATCH] Merge and test fixups --- Cargo.lock | 1 + beacon_node/beacon_chain/src/migrate.rs | 5 +++ beacon_node/beacon_chain/src/test_utils.rs | 6 +++ .../tests/attestation_verification.rs | 2 +- .../beacon_chain/tests/block_verification.rs | 2 - .../tests/payload_invalidation.rs | 6 +-- beacon_node/beacon_chain/tests/store_tests.rs | 42 ++++++++++++++----- beacon_node/store/src/hot_cold_store.rs | 16 +++---- common/task_executor/Cargo.toml | 1 + common/task_executor/src/test_utils.rs | 10 ++++- 10 files changed, 66 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6bf7122542..546868e3d3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6585,6 +6585,7 @@ dependencies = [ "futures", "lazy_static", "lighthouse_metrics", + "logging", "slog", "sloggers", "tokio", diff --git a/beacon_node/beacon_chain/src/migrate.rs b/beacon_node/beacon_chain/src/migrate.rs index ced469eda2..22e6a3ed0c 100644 --- a/beacon_node/beacon_chain/src/migrate.rs +++ b/beacon_node/beacon_chain/src/migrate.rs @@ -70,6 +70,11 @@ impl MigratorConfig { self.blocking = true; self } + + pub fn epochs_per_run(mut self, epochs_per_run: u64) -> Self { + self.epochs_per_run = epochs_per_run; + self + } } /// Pruning can be successful, or in rare cases deferred to a later point. diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 452cb93e58..49ea85a444 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -316,6 +316,12 @@ where self } + pub fn logger(mut self, log: Logger) -> Self { + self.log = log.clone(); + self.runtime.set_logger(log); + self + } + /// This mutator will be run before the `store_mutator`. pub fn initial_mutator(mut self, mutator: BoxedMutator) -> Self { assert!( diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 6a9e604793..101e17f611 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -981,7 +981,7 @@ async fn attestation_that_skips_epochs() { let block_slot = harness .chain .store - .get_blinded_block(&block_root) + .get_blinded_block(&block_root, None) .expect("should not error getting block") .expect("should find attestation block") .message() diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 8e7d1b2708..998f22f770 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -1141,7 +1141,6 @@ async fn add_base_block_to_altair_chain() { let mut state = state; let mut ctxt = ConsensusContext::new(base_block.slot()); per_slot_processing(&mut state, None, &harness.chain.spec).unwrap(); - let mut ctxt = ConsensusContext::new(state.slot()); assert!(matches!( per_block_processing( &mut state, @@ -1275,7 +1274,6 @@ async fn add_altair_block_to_base_chain() { let mut state = state; let mut ctxt = ConsensusContext::new(altair_block.slot()); per_slot_processing(&mut state, None, &harness.chain.spec).unwrap(); - let mut ctxt = ConsensusContext::new(state.slot()); assert!(matches!( per_block_processing( &mut state, diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 2336c3ba99..f3c476c6d4 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -215,7 +215,7 @@ impl InvalidPayloadRig { let mock_execution_layer = self.harness.mock_execution_layer.as_ref().unwrap(); let head = self.harness.chain.head_snapshot(); - let state = head.beacon_state.clone_with_only_committee_caches(); + let state = head.beacon_state.clone(); let slot = slot_override.unwrap_or(state.slot() + 1); let (block, post_state) = self.harness.make_block(state, slot).await; let block_root = block.canonical_root(); @@ -308,7 +308,7 @@ impl InvalidPayloadRig { self.harness .chain .store - .get_full_block(&block_root) + .get_full_block(&block_root, None) .unwrap() .unwrap(), block, @@ -2000,7 +2000,7 @@ async fn weights_after_resetting_optimistic_status() { .fork_choice_read_lock() .get_block_weight(&head.head_block_root()) .unwrap(), - head.snapshot.beacon_state.validators()[0].effective_balance, + head.snapshot.beacon_state.validators().get(0).unwrap().effective_balance(), "proposer boost should be removed from the head block and the vote of a single validator applied" ); diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index e4e96afb1f..d5076e68c0 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -64,11 +64,20 @@ fn get_harness( store: Arc, LevelDB>>, validator_count: usize, ) -> TestHarness { + // Most tests were written expecting instant migration on finalization. + let migrator_config = MigratorConfig::default().blocking().epochs_per_run(0); + + let log = store.log.clone(); + let harness = BeaconChainHarness::builder(MinimalEthSpec) + .logger(log) .default_spec() .keypairs(KEYPAIRS[0..validator_count].to_vec()) .fresh_disk_store(store) .mock_execution_layer() + .initial_mutator(Box::new(|builder: BeaconChainBuilder<_>| { + builder.store_migrator_config(migrator_config) + })) .build(); harness.advance_slot(); harness @@ -272,6 +281,9 @@ async fn split_slot_restore() { ) .await; + // Uhmm. FIXME(sproul) + // tokio::time::sleep(std::time::Duration::from_secs(10)).await; + store.get_split_slot() }; assert_ne!(split_slot, Slot::new(0)); @@ -561,7 +573,7 @@ async fn delete_blocks_and_states() { ); let faulty_head_block = store - .get_blinded_block(&faulty_head.into()) + .get_blinded_block(&faulty_head.into(), None) .expect("no errors") .expect("faulty head block exists"); @@ -603,7 +615,7 @@ async fn delete_blocks_and_states() { break; } store.delete_block(&block_root).unwrap(); - assert_eq!(store.get_blinded_block(&block_root).unwrap(), None); + assert_eq!(store.get_blinded_block(&block_root, None).unwrap(), None); } // Deleting frozen states should do nothing @@ -847,7 +859,7 @@ fn get_state_for_block(harness: &TestHarness, block_root: Hash256) -> BeaconStat let head_block = harness .chain .store - .get_blinded_block(&block_root) + .get_blinded_block(&block_root, None) .unwrap() .unwrap(); harness @@ -887,9 +899,17 @@ fn check_shuffling_compatible( |committee_cache, _| { let state_cache = head_state.committee_cache(RelativeEpoch::Current).unwrap(); if current_epoch_shuffling_is_compatible { - assert_eq!(committee_cache, state_cache, "block at slot {slot}"); + assert_eq!( + committee_cache, + state_cache.as_ref(), + "block at slot {slot}" + ); } else { - assert_ne!(committee_cache, state_cache, "block at slot {slot}"); + assert_ne!( + committee_cache, + state_cache.as_ref(), + "block at slot {slot}" + ); } Ok(()) }, @@ -919,9 +939,9 @@ fn check_shuffling_compatible( |committee_cache, _| { let state_cache = head_state.committee_cache(RelativeEpoch::Previous).unwrap(); if previous_epoch_shuffling_is_compatible { - assert_eq!(committee_cache, state_cache); + assert_eq!(committee_cache, state_cache.as_ref()); } else { - assert_ne!(committee_cache, state_cache); + assert_ne!(committee_cache, state_cache.as_ref()); } Ok(()) }, @@ -2019,7 +2039,7 @@ async fn weak_subjectivity_sync() { let wss_block = harness .chain .store - .get_full_block(&wss_checkpoint.root) + .get_full_block(&wss_checkpoint.root, None) .unwrap() .unwrap(); let wss_state = full_store @@ -2164,7 +2184,7 @@ async fn weak_subjectivity_sync() { .unwrap() .map(Result::unwrap) { - let block = store.get_blinded_block(&block_root).unwrap().unwrap(); + let block = store.get_blinded_block(&block_root, None).unwrap().unwrap(); assert_eq!(block.slot(), slot); } @@ -2490,8 +2510,8 @@ fn assert_chains_pretty_much_the_same(a: &BeaconChain, b // Clone with committee caches only to prevent other caches from messing with the equality // check. assert_eq!( - a_head.beacon_state.clone_with_only_committee_caches(), - b_head.beacon_state.clone_with_only_committee_caches(), + a_head.beacon_state.clone(), + b_head.beacon_state.clone(), "head states should be equal" ); assert_eq!(a.heads(), b.heads(), "heads() should be equal"); diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 565b1ffc4b..c7def479ca 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -2023,7 +2023,7 @@ pub fn migrate_database, Cold: ItemStore>( } // Store the new finalized state as a full state in the database. It would likely previously - // have been stored as a diff. + // have been stored in memory, or maybe as a diff. store.store_full_state(&finalized_state_root, finalized_state)?; // Copy all of the states between the new finalized state and the split slot, from the hot DB to @@ -2074,13 +2074,15 @@ pub fn migrate_database, Cold: ItemStore>( // Copy the blinded block from the hot database to the freezer. let blinded_block = store - .get_hot_blinded_block(&block_root)? + .get_blinded_block(&block_root, None)? .ok_or(Error::BlockNotFound(block_root))?; - store.blinded_block_as_cold_kv_store_ops( - &block_root, - &blinded_block, - &mut cold_db_block_ops, - )?; + if blinded_block.slot() == slot { + store.blinded_block_as_cold_kv_store_ops( + &block_root, + &blinded_block, + &mut cold_db_block_ops, + )?; + } } // Warning: Critical section. We have to take care not to put any of the two databases in an diff --git a/common/task_executor/Cargo.toml b/common/task_executor/Cargo.toml index 08bb565870..c5d35c6368 100644 --- a/common/task_executor/Cargo.toml +++ b/common/task_executor/Cargo.toml @@ -12,3 +12,4 @@ exit-future = "0.2.0" lazy_static = "1.4.0" lighthouse_metrics = { path = "../lighthouse_metrics" } sloggers = { version = "2.1.1", features = ["json"] } +logging = { path = "../logging" } diff --git a/common/task_executor/src/test_utils.rs b/common/task_executor/src/test_utils.rs index 7d59cdf022..71a451ca84 100644 --- a/common/task_executor/src/test_utils.rs +++ b/common/task_executor/src/test_utils.rs @@ -1,4 +1,5 @@ use crate::TaskExecutor; +use logging::test_logger; use slog::Logger; use sloggers::{null::NullLoggerBuilder, Build}; use std::sync::Arc; @@ -26,7 +27,7 @@ impl Default for TestRuntime { fn default() -> Self { let (runtime_shutdown, exit) = exit_future::signal(); let (shutdown_tx, _) = futures::channel::mpsc::channel(1); - let log = null_logger().unwrap(); + let log = test_logger(); let (runtime, handle) = if let Ok(handle) = runtime::Handle::try_current() { (None, handle) @@ -60,6 +61,13 @@ impl Drop for TestRuntime { } } +impl TestRuntime { + pub fn set_logger(&mut self, log: Logger) { + self.log = log.clone(); + self.task_executor.log = log; + } +} + pub fn null_logger() -> Result { let log_builder = NullLoggerBuilder; log_builder