From 906d6ca647c94feb6f003097e2ea7d2c8f04bf69 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 28 Feb 2025 11:43:36 -0800 Subject: [PATCH 01/17] force finalization endpoint --- beacon_node/beacon_chain/src/beacon_chain.rs | 12 +- beacon_node/beacon_chain/src/migrate.rs | 123 +++++++++++++++++++ beacon_node/http_api/src/lib.rs | 34 ++++- common/eth2/src/types.rs | 7 ++ testing/.DS_Store | Bin 0 -> 6148 bytes 5 files changed, 170 insertions(+), 6 deletions(-) create mode 100644 testing/.DS_Store diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 891a32a6d04..f8ed1b9eba8 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -42,7 +42,7 @@ use crate::light_client_optimistic_update_verification::{ Error as LightClientOptimisticUpdateError, VerifiedLightClientOptimisticUpdate, }; use crate::light_client_server_cache::LightClientServerCache; -use crate::migrate::BackgroundMigrator; +use crate::migrate::{BackgroundMigrator, ManualFinalizationNotification}; use crate::naive_aggregation_pool::{ AggregatedAttestationMap, Error as NaiveAggregationError, NaiveAggregationPool, SyncContributionAggregateMap, @@ -1702,6 +1702,16 @@ impl BeaconChain { } } + pub fn manually_finalize_state(&self, state_root: Hash256, checkpoint: Checkpoint) { + let notif = ManualFinalizationNotification { + state_root: state_root.into(), + checkpoint, + head_tracker: self.head_tracker.clone(), + genesis_block_root: self.genesis_block_root, + }; + self.store_migrator.process_manual_finalization(notif) + } + /// Returns an aggregated `Attestation`, if any, that has a matching `attestation.data`. /// /// The attestation will be obtained from `self.naive_aggregation_pool`. diff --git a/beacon_node/beacon_chain/src/migrate.rs b/beacon_node/beacon_chain/src/migrate.rs index bc4b8e1ed86..735507d8074 100644 --- a/beacon_node/beacon_chain/src/migrate.rs +++ b/beacon_node/beacon_chain/src/migrate.rs @@ -124,6 +124,14 @@ pub enum Notification { Finalization(FinalizationNotification), Reconstruction, PruneBlobs(Epoch), + ManualFinalization(ManualFinalizationNotification), +} + +pub struct ManualFinalizationNotification { + pub state_root: BeaconStateHash, + pub checkpoint: Checkpoint, + pub head_tracker: Arc, + pub genesis_block_root: Hash256, } pub struct FinalizationNotification { @@ -190,6 +198,10 @@ impl, Cold: ItemStore> BackgroundMigrator, Cold: ItemStore> BackgroundMigrator>, + notif: ManualFinalizationNotification, + log: &Logger, + ) { + let state_root = notif.state_root; + let block_root = notif.checkpoint.root; + + let manually_finalized_state = match db.get_state(&state_root.into(), None) { + Ok(Some(state)) => state, + other => { + error!( + log, + "Manual migrator failed to load state"; + "state_root" => ?state_root, + "error" => ?other + ); + return; + } + }; + + let old_finalized_checkpoint = match Self::prune_abandoned_forks( + db.clone(), + notif.head_tracker, + state_root, + &manually_finalized_state, + notif.checkpoint, + notif.genesis_block_root, + log, + ) { + Ok(PruningOutcome::Successful { + old_finalized_checkpoint, + }) => old_finalized_checkpoint, + Ok(PruningOutcome::DeferredConcurrentHeadTrackerMutation) => { + warn!( + log, + "Pruning deferred because of a concurrent mutation"; + "message" => "this is expected only very rarely!" + ); + return; + } + Ok(PruningOutcome::OutOfOrderFinalization { + old_finalized_checkpoint, + new_finalized_checkpoint, + }) => { + warn!( + log, + "Ignoring out of order finalization request"; + "old_finalized_epoch" => old_finalized_checkpoint.epoch, + "new_finalized_epoch" => new_finalized_checkpoint.epoch, + "message" => "this is expected occasionally due to a (harmless) race condition" + ); + return; + } + Err(e) => { + warn!(log, "Block pruning failed"; "error" => ?e); + return; + } + }; + + match migrate_database( + db.clone(), + state_root.into(), + block_root, + &manually_finalized_state, + ) { + Ok(()) => {} + Err(Error::HotColdDBError(HotColdDBError::FreezeSlotUnaligned(slot))) => { + debug!( + log, + "Database migration postponed, unaligned finalized block"; + "slot" => slot.as_u64() + ); + } + Err(e) => { + warn!( + log, + "Database migration failed"; + "error" => format!("{:?}", e) + ); + return; + } + }; + + // Finally, compact the database so that new free space is properly reclaimed. + if let Err(e) = Self::run_compaction( + db, + old_finalized_checkpoint.epoch, + notif.checkpoint.epoch, + log, + ) { + warn!(log, "Database compaction failed"; "error" => format!("{:?}", e)); + } + + debug!(log, "Database consolidation complete"); + } + /// Perform the actual work of `process_finalization`. fn run_migration( db: Arc>, @@ -422,16 +531,27 @@ impl, Cold: ItemStore> BackgroundMigrator reconstruction_notif = Some(notif), Notification::Finalization(fin) => finalization_notif = Some(fin), + Notification::ManualFinalization(fin) => manual_finalization_notif = Some(fin), Notification::PruneBlobs(dab) => prune_blobs_notif = Some(dab), } // Read the rest of the messages in the channel, taking the best of each type. for notif in rx.try_iter() { match notif { Notification::Reconstruction => reconstruction_notif = Some(notif), + Notification::ManualFinalization(fin) => { + if let Some(current) = manual_finalization_notif.as_mut() { + if fin.checkpoint.epoch > current.checkpoint.epoch { + *current = fin; + } + } else { + manual_finalization_notif = Some(fin); + } + } Notification::Finalization(fin) => { if let Some(current) = finalization_notif.as_mut() { if fin.finalized_checkpoint.epoch @@ -454,6 +574,9 @@ impl, Cold: ItemStore> BackgroundMigrator( }, ); + // POST lighthouse/finalize + let post_lighthouse_finalize = warp::path("lighthouse") + .and(warp::path("finalize")) + .and(warp::path::end()) + .and(warp_utils::json::json()) + .and(task_spawner_filter.clone()) + .and(chain_filter.clone()) + .then( + |request_data: api_types::ManualFinalizationRequestData, + task_spawner: TaskSpawner, + chain: Arc>| { + task_spawner.blocking_json_task(Priority::P0, move || { + let checkpoint = Checkpoint { + epoch: request_data.epoch, + root: request_data.block_root, + }; + chain.manually_finalize_state(request_data.state_root, checkpoint); + + Ok(api_types::GenericResponse::from(request_data)) + }) + }, + ); + // POST lighthouse/liveness let post_lighthouse_liveness = warp::path("lighthouse") .and(warp::path("liveness")) @@ -4780,6 +4803,7 @@ pub fn serve( .uor(post_lighthouse_block_rewards) .uor(post_lighthouse_ui_validator_metrics) .uor(post_lighthouse_ui_validator_info) + .uor(post_lighthouse_finalize) .recover(warp_utils::reject::handle_rejection), ), ) diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index 59374f629d6..7d70d242be7 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -1424,6 +1424,13 @@ pub struct StandardLivenessResponseData { pub is_live: bool, } +#[derive(Debug, Serialize, Deserialize)] +pub struct ManualFinalizationRequestData { + pub state_root: Hash256, + pub epoch: Epoch, + pub block_root: Hash256, +} + #[derive(Debug, Serialize, Deserialize)] pub struct LivenessRequestData { pub epoch: Epoch, diff --git a/testing/.DS_Store b/testing/.DS_Store new file mode 100644 index 0000000000000000000000000000000000000000..36ab105f30b607934c8041ff9d164dd6ad2eb50f GIT binary patch literal 6148 zcmeHKy-or_5dK!t5Py;@Ew8dCK7c1VUqBuJgLfbactj+$_ZB7=+S*tfOB-X!TUh!E zezUv45fHI4YG#tzZ*O;IcjhCz!vZkXK~ez%0A;FRX^YJQk$KTMS@R=1L}S-T(Z?0~ zomQnbXRQOufHLsg7?87DMv5ltJh_GQdzN*3672Bqy1vM!o5gXXlQm@SWc2(NJiQz( zwo6vB>z!9><2`RTMqCQJxIl)4QB7m3;}UJO^(Wj!fYO{;#17!^=6XGmwOd6`i|dBUdH|| zlAFrkXMi41IoZG1MBWHkotf8 z`TKv?Nxzf6KLVBpZIpo@W#9u0fRWh% literal 0 HcmV?d00001 From 6cde895caa4ab2319529490bf232c6674ca6c55a Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 28 Feb 2025 13:21:29 -0800 Subject: [PATCH 02/17] cleanup --- beacon_node/beacon_chain/src/migrate.rs | 109 ++++-------------------- 1 file changed, 16 insertions(+), 93 deletions(-) diff --git a/beacon_node/beacon_chain/src/migrate.rs b/beacon_node/beacon_chain/src/migrate.rs index 735507d8074..cfc3807db96 100644 --- a/beacon_node/beacon_chain/src/migrate.rs +++ b/beacon_node/beacon_chain/src/migrate.rs @@ -135,11 +135,11 @@ pub struct ManualFinalizationNotification { } pub struct FinalizationNotification { - finalized_state_root: BeaconStateHash, - finalized_checkpoint: Checkpoint, - head_tracker: Arc, - prev_migration: Arc>, - genesis_block_root: Hash256, + pub finalized_state_root: BeaconStateHash, + pub finalized_checkpoint: Checkpoint, + pub head_tracker: Arc, + pub prev_migration: Arc>, + pub genesis_block_root: Hash256, } impl, Cold: ItemStore> BackgroundMigrator { @@ -306,96 +306,19 @@ impl, Cold: ItemStore> BackgroundMigrator state, - other => { - error!( - log, - "Manual migrator failed to load state"; - "state_root" => ?state_root, - "error" => ?other - ); - return; - } + // We create a "dummy" prev migration + let prev_migration = PrevMigration { + epoch: Epoch::new(1), + epochs_per_migration: 2, }; - - let old_finalized_checkpoint = match Self::prune_abandoned_forks( - db.clone(), - notif.head_tracker, - state_root, - &manually_finalized_state, - notif.checkpoint, - notif.genesis_block_root, - log, - ) { - Ok(PruningOutcome::Successful { - old_finalized_checkpoint, - }) => old_finalized_checkpoint, - Ok(PruningOutcome::DeferredConcurrentHeadTrackerMutation) => { - warn!( - log, - "Pruning deferred because of a concurrent mutation"; - "message" => "this is expected only very rarely!" - ); - return; - } - Ok(PruningOutcome::OutOfOrderFinalization { - old_finalized_checkpoint, - new_finalized_checkpoint, - }) => { - warn!( - log, - "Ignoring out of order finalization request"; - "old_finalized_epoch" => old_finalized_checkpoint.epoch, - "new_finalized_epoch" => new_finalized_checkpoint.epoch, - "message" => "this is expected occasionally due to a (harmless) race condition" - ); - return; - } - Err(e) => { - warn!(log, "Block pruning failed"; "error" => ?e); - return; - } - }; - - match migrate_database( - db.clone(), - state_root.into(), - block_root, - &manually_finalized_state, - ) { - Ok(()) => {} - Err(Error::HotColdDBError(HotColdDBError::FreezeSlotUnaligned(slot))) => { - debug!( - log, - "Database migration postponed, unaligned finalized block"; - "slot" => slot.as_u64() - ); - } - Err(e) => { - warn!( - log, - "Database migration failed"; - "error" => format!("{:?}", e) - ); - return; - } + let notif = FinalizationNotification { + finalized_state_root: notif.state_root, + finalized_checkpoint: notif.checkpoint, + head_tracker: notif.head_tracker, + prev_migration: Arc::new(prev_migration.into()), + genesis_block_root: notif.genesis_block_root, }; - - // Finally, compact the database so that new free space is properly reclaimed. - if let Err(e) = Self::run_compaction( - db, - old_finalized_checkpoint.epoch, - notif.checkpoint.epoch, - log, - ) { - warn!(log, "Database compaction failed"; "error" => format!("{:?}", e)); - } - - debug!(log, "Database consolidation complete"); + Self::run_migration(db, notif, log); } /// Perform the actual work of `process_finalization`. From f31a00917c598a8357c541aff2487140988d008c Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 28 Feb 2025 14:17:15 -0800 Subject: [PATCH 03/17] Remove ds store --- testing/.DS_Store | Bin 6148 -> 0 bytes 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 testing/.DS_Store diff --git a/testing/.DS_Store b/testing/.DS_Store deleted file mode 100644 index 36ab105f30b607934c8041ff9d164dd6ad2eb50f..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 6148 zcmeHKy-or_5dK!t5Py;@Ew8dCK7c1VUqBuJgLfbactj+$_ZB7=+S*tfOB-X!TUh!E zezUv45fHI4YG#tzZ*O;IcjhCz!vZkXK~ez%0A;FRX^YJQk$KTMS@R=1L}S-T(Z?0~ zomQnbXRQOufHLsg7?87DMv5ltJh_GQdzN*3672Bqy1vM!o5gXXlQm@SWc2(NJiQz( zwo6vB>z!9><2`RTMqCQJxIl)4QB7m3;}UJO^(Wj!fYO{;#17!^=6XGmwOd6`i|dBUdH|| zlAFrkXMi41IoZG1MBWHkotf8 z`TKv?Nxzf6KLVBpZIpo@W#9u0fRWh% From 0a427e5b361fabc8e253a273f84b093d70b1d485 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Sat, 1 Mar 2025 09:31:44 +1100 Subject: [PATCH 04/17] Don't import blocks that conflict with the split --- beacon_node/beacon_chain/src/block_verification.rs | 10 +++++++++- consensus/fork_choice/src/fork_choice.rs | 5 +++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 12652763763..4b41818b80c 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1761,6 +1761,14 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< if fork_choice.is_finalized_checkpoint_or_descendant(block.parent_root()) { Ok(block) } else { + // If we have a split block newer than finalization then we also ban blocks which are not + // descended from that split block. + let split = chain.store.get_split_info(); + if !fork_choice.is_descendant(split.block_root, block.parent_root()) { + Err(BlockError::NotFinalizedDescendant { + block_parent_root: block.parent_root(), + }) + } // If fork choice does *not* consider the parent to be a descendant of the finalized block, // then there are two more cases: // @@ -1769,7 +1777,7 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< // pre-finalization or conflicting with finalization. // 2. The parent is unknown to us, we probably want to download it since it might actually // descend from the finalized root. - if chain + else if chain .store .block_exists(&block.parent_root()) .map_err(|e| BlockError::BeaconChainError(e.into()))? diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 4c25be950b0..ddd5923849c 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1255,6 +1255,11 @@ where .is_finalized_checkpoint_or_descendant::(block_root) } + pub fn is_descendant(&self, ancestor_root: Hash256, descendant_root: Hash256) -> bool { + self.proto_array + .is_descendant(ancestor_root, descendant_root) + } + /// Returns `Ok(true)` if `block_root` has been imported optimistically or deemed invalid. /// /// Returns `Ok(false)` if `block_root`'s execution payload has been elected as fully VALID, if From eec94061313c05832ae8173b71f00ce7b331e841 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 28 Feb 2025 16:07:48 -0800 Subject: [PATCH 05/17] Disconnect and ban peer if we get blocks conflicting manual checkpoint --- .../src/network_beacon_processor/sync_methods.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/beacon_node/network/src/network_beacon_processor/sync_methods.rs b/beacon_node/network/src/network_beacon_processor/sync_methods.rs index 338f2bc4c8b..66c98e5a800 100644 --- a/beacon_node/network/src/network_beacon_processor/sync_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/sync_methods.rs @@ -740,6 +740,19 @@ impl NetworkBeaconProcessor { debug!(self.log, "Finalized or earlier block processed";); Ok(()) } + BlockError::NotFinalizedDescendant { block_parent_root } => { + debug!( + self.log, + "Not syncing to a chain that conflicts with the canonical or manual finalized checkpoint" + ); + Err(ChainSegmentFailed { + message: format!( + "Block with parent_root {} conflicts with our checkpoint state", + block_parent_root + ), + peer_action: Some(PeerAction::Fatal), + }) + } BlockError::GenesisBlock => { debug!(self.log, "Genesis block was processed"); Ok(()) From e58945e39818084e74b473115919283e340e496b Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 28 Feb 2025 17:41:36 -0800 Subject: [PATCH 06/17] immediately commit state to cold db --- beacon_node/store/src/hot_cold_store.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 6dee0dc180a..a6595c760be 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -3169,11 +3169,16 @@ pub fn migrate_database, Cold: ItemStore>( // Store slot -> state_root and state_root -> slot mappings. store.store_cold_state_summary(&state_root, slot, &mut cold_db_ops)?; } else { + // TODO(holesky) + // I've updated to immediately commit the state to the cold db + // instead of doing it in batches so we don't load tons of states into memory + let mut immediate_cold_op = vec![]; let state: BeaconState = store .get_hot_state(&state_root)? .ok_or(HotColdDBError::MissingStateToFreeze(state_root))?; - store.store_cold_state(&state_root, &state, &mut cold_db_ops)?; + store.store_cold_state(&state_root, &state, &mut immediate_cold_op)?; + store.cold_db.do_atomically(immediate_cold_op)?; } // Cold states are diffed with respect to each other, so we need to finish writing previous From fa6927bd846cea6c3869afe8a57aac4d4079d026 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 28 Feb 2025 18:37:03 -0800 Subject: [PATCH 07/17] revert --- beacon_node/store/src/hot_cold_store.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index a6595c760be..6dee0dc180a 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -3169,16 +3169,11 @@ pub fn migrate_database, Cold: ItemStore>( // Store slot -> state_root and state_root -> slot mappings. store.store_cold_state_summary(&state_root, slot, &mut cold_db_ops)?; } else { - // TODO(holesky) - // I've updated to immediately commit the state to the cold db - // instead of doing it in batches so we don't load tons of states into memory - let mut immediate_cold_op = vec![]; let state: BeaconState = store .get_hot_state(&state_root)? .ok_or(HotColdDBError::MissingStateToFreeze(state_root))?; - store.store_cold_state(&state_root, &state, &mut immediate_cold_op)?; - store.cold_db.do_atomically(immediate_cold_op)?; + store.store_cold_state(&state_root, &state, &mut cold_db_ops)?; } // Cold states are diffed with respect to each other, so we need to finish writing previous From 38192f7889eeba2620fe57c9921f958ae84c0436 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Sun, 2 Mar 2025 08:55:10 +1100 Subject: [PATCH 08/17] Fix descent from split check --- .../beacon_chain/src/block_verification.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 4b41818b80c..f9064e58000 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1758,17 +1758,14 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< fork_choice: &BeaconForkChoice, block: B, ) -> Result { - if fork_choice.is_finalized_checkpoint_or_descendant(block.parent_root()) { + // If we have a split block newer than finalization then we also ban blocks which are not + // descended from that split block. + let split = chain.store.get_split_info(); + if fork_choice.is_finalized_checkpoint_or_descendant(block.parent_root()) + && fork_choice.is_descendant(split.block_root, block.parent_root()) + { Ok(block) } else { - // If we have a split block newer than finalization then we also ban blocks which are not - // descended from that split block. - let split = chain.store.get_split_info(); - if !fork_choice.is_descendant(split.block_root, block.parent_root()) { - Err(BlockError::NotFinalizedDescendant { - block_parent_root: block.parent_root(), - }) - } // If fork choice does *not* consider the parent to be a descendant of the finalized block, // then there are two more cases: // @@ -1777,7 +1774,7 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< // pre-finalization or conflicting with finalization. // 2. The parent is unknown to us, we probably want to download it since it might actually // descend from the finalized root. - else if chain + if chain .store .block_exists(&block.parent_root()) .map_err(|e| BlockError::BeaconChainError(e.into()))? From 3c519d3b16624297ba4302cf48816e4ecb6c2c0f Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Mon, 3 Mar 2025 16:01:27 +1100 Subject: [PATCH 09/17] Add safety check to checkpoint when doing manual finalization. --- beacon_node/beacon_chain/src/beacon_chain.rs | 33 +++++++++++++++++--- beacon_node/beacon_chain/src/errors.rs | 7 +++-- beacon_node/beacon_chain/src/migrate.rs | 6 +++- beacon_node/http_api/src/lib.rs | 10 ++++-- 4 files changed, 46 insertions(+), 10 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index f8ed1b9eba8..c546bfe7417 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -118,8 +118,8 @@ use std::sync::Arc; use std::time::Duration; use store::iter::{BlockRootsIterator, ParentRootBlockIterator, StateRootsIterator}; use store::{ - BlobSidecarListFromRoot, DatabaseBlock, Error as DBError, HotColdDB, KeyValueStore, - KeyValueStoreOp, StoreItem, StoreOp, + BlobSidecarListFromRoot, DatabaseBlock, Error as DBError, HotColdDB, HotStateSummary, + KeyValueStore, KeyValueStoreOp, StoreItem, StoreOp, }; use task_executor::{ShutdownReason, TaskExecutor}; use tokio::sync::oneshot; @@ -1702,14 +1702,39 @@ impl BeaconChain { } } - pub fn manually_finalize_state(&self, state_root: Hash256, checkpoint: Checkpoint) { + pub fn manually_finalize_state( + &self, + state_root: Hash256, + checkpoint: Checkpoint, + ) -> Result<(), Error> { + let HotStateSummary { + slot, + latest_block_root, + .. + } = self + .store + .load_hot_state_summary(&state_root) + .map_err(BeaconChainError::DBError)? + .ok_or(BeaconChainError::MissingHotStateSummary(state_root))?; + + if slot != checkpoint.epoch.start_slot(T::EthSpec::slots_per_epoch()) + || latest_block_root != *checkpoint.root + { + return Err(BeaconChainError::InvalidCheckpoint { + state_root, + checkpoint, + }); + } + let notif = ManualFinalizationNotification { state_root: state_root.into(), checkpoint, head_tracker: self.head_tracker.clone(), genesis_block_root: self.genesis_block_root, }; - self.store_migrator.process_manual_finalization(notif) + + self.store_migrator.process_manual_finalization(notif); + Ok(()) } /// Returns an aggregated `Attestation`, if any, that has a matching `attestation.data`. diff --git a/beacon_node/beacon_chain/src/errors.rs b/beacon_node/beacon_chain/src/errors.rs index 2e13ab4090e..8509c52c8a9 100644 --- a/beacon_node/beacon_chain/src/errors.rs +++ b/beacon_node/beacon_chain/src/errors.rs @@ -61,6 +61,7 @@ pub enum BeaconChainError { ForkChoiceStoreError(ForkChoiceStoreError), MissingBeaconBlock(Hash256), MissingBeaconState(Hash256), + MissingHotStateSummary(Hash256), SlotProcessingError(SlotProcessingError), EpochProcessingError(EpochProcessingError), StateAdvanceError(StateAdvanceError), @@ -181,9 +182,9 @@ pub enum BeaconChainError { execution_block_hash: Option, }, ForkchoiceUpdate(execution_layer::Error), - FinalizedCheckpointMismatch { - head_state: Checkpoint, - fork_choice: Hash256, + InvalidCheckpoint { + state_root: Hash256, + checkpoint: Checkpoint, }, InvalidSlot(Slot), HeadBlockNotFullyVerified { diff --git a/beacon_node/beacon_chain/src/migrate.rs b/beacon_node/beacon_chain/src/migrate.rs index cfc3807db96..441237bf168 100644 --- a/beacon_node/beacon_chain/src/migrate.rs +++ b/beacon_node/beacon_chain/src/migrate.rs @@ -199,7 +199,11 @@ impl, Cold: ItemStore> BackgroundMigrator( epoch: request_data.epoch, root: request_data.block_root, }; - chain.manually_finalize_state(request_data.state_root, checkpoint); - Ok(api_types::GenericResponse::from(request_data)) + chain + .manually_finalize_state(request_data.state_root, checkpoint) + .map(|_| api_types::GenericResponse::from(request_data)) + .map_err(|e| { + warp_utils::reject::custom_bad_request(format!( + "Failed to finalize state due to error: {e:?}" + )) + }) }) }, ); From 79cb31a9ac0d81753346bcbc69b968fa8bd33b49 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 4 Mar 2025 11:15:32 +1100 Subject: [PATCH 10/17] partial merge of jimmys ci fixes --- beacon_node/beacon_chain/src/block_verification.rs | 6 +++++- beacon_node/beacon_chain/tests/payload_invalidation.rs | 8 ++++++++ consensus/fork_choice/tests/tests.rs | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index f9064e58000..3bb19587cbc 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1761,8 +1761,12 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< // If we have a split block newer than finalization then we also ban blocks which are not // descended from that split block. let split = chain.store.get_split_info(); + let is_descendant_from_split_block = + split.slot == 0 || fork_choice.is_descendant(split.block_root, block.parent_root()); + + println!("Block is descendant: {:?}", is_descendant_from_split_block); if fork_choice.is_finalized_checkpoint_or_descendant(block.parent_root()) - && fork_choice.is_descendant(split.block_root, block.parent_root()) + && is_descendant_from_split_block { Ok(block) } else { diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 01b790bb25b..c16bdebe423 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -471,11 +471,19 @@ async fn immediate_forkchoice_update_payload_invalid() { } #[tokio::test] +// Failing since this change on `holesky-rescue`. We've lift this restriction so the previously +// valid block can be marked invalid on Holesky. +// https://github.com/sigp/lighthouse/commit/19fc31a75be727ff811f54c249db19159003e90b +#[ignore] async fn immediate_forkchoice_update_payload_invalid_block_hash() { immediate_forkchoice_update_invalid_test(|_| Payload::InvalidBlockHash).await } #[tokio::test] +// Failing since this change on `holesky-rescue`. We've lift this restriction so the previously +// valid block can be marked invalid on Holesky. +// https://github.com/sigp/lighthouse/commit/19fc31a75be727ff811f54c249db19159003e90b +#[ignore] async fn immediate_forkchoice_update_payload_invalid_terminal_block() { immediate_forkchoice_update_invalid_test(|_| Payload::Invalid { latest_valid_hash: Some(ExecutionBlockHash::zero()), diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index b224cde048e..7ad2a72c973 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -1,4 +1,4 @@ -#![cfg(not(debug_assertions))] +// #![cfg(not(debug_assertions))] use beacon_chain::test_utils::{ AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType, From 540a4e8982291ec23db1e3ba66d51f50ca81dfb2 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 4 Mar 2025 11:53:08 +1100 Subject: [PATCH 11/17] Remove log and update cli docs. --- beacon_node/beacon_chain/src/block_verification.rs | 1 - book/src/help_bn.md | 3 +++ consensus/fork_choice/tests/tests.rs | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 3bb19587cbc..f78b240e21b 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1764,7 +1764,6 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< let is_descendant_from_split_block = split.slot == 0 || fork_choice.is_descendant(split.block_root, block.parent_root()); - println!("Block is descendant: {:?}", is_descendant_from_split_block); if fork_choice.is_finalized_checkpoint_or_descendant(block.parent_root()) && is_descendant_from_split_block { diff --git a/book/src/help_bn.md b/book/src/help_bn.md index 79c8d8ead85..b52ac24615f 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -385,6 +385,9 @@ Options: Number of validators per chunk stored on disk. --slots-per-restore-point DEPRECATED. This flag has no effect. + --state-cache-headroom + Minimum number of states to cull from the state cache when it gets + full [default: 1] --state-cache-size Specifies the size of the state cache [default: 128] --suggested-fee-recipient diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 7ad2a72c973..b224cde048e 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -1,4 +1,4 @@ -// #![cfg(not(debug_assertions))] +#![cfg(not(debug_assertions))] use beacon_chain::test_utils::{ AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType, From c7aee6f04ade4beb748a89dbf1148b02574a4abf Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 4 Mar 2025 14:53:34 +1100 Subject: [PATCH 12/17] Revert payload invalidation API - wasn't working because of the justification being permanently updated. --- beacon_node/beacon_chain/tests/payload_invalidation.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index c16bdebe423..01b790bb25b 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -471,19 +471,11 @@ async fn immediate_forkchoice_update_payload_invalid() { } #[tokio::test] -// Failing since this change on `holesky-rescue`. We've lift this restriction so the previously -// valid block can be marked invalid on Holesky. -// https://github.com/sigp/lighthouse/commit/19fc31a75be727ff811f54c249db19159003e90b -#[ignore] async fn immediate_forkchoice_update_payload_invalid_block_hash() { immediate_forkchoice_update_invalid_test(|_| Payload::InvalidBlockHash).await } #[tokio::test] -// Failing since this change on `holesky-rescue`. We've lift this restriction so the previously -// valid block can be marked invalid on Holesky. -// https://github.com/sigp/lighthouse/commit/19fc31a75be727ff811f54c249db19159003e90b -#[ignore] async fn immediate_forkchoice_update_payload_invalid_terminal_block() { immediate_forkchoice_update_invalid_test(|_| Payload::Invalid { latest_valid_hash: Some(ExecutionBlockHash::zero()), From ecbb7f2d0b2a6ad36de2ce6aac22b10dbb5648a1 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 10 Mar 2025 14:09:57 -0600 Subject: [PATCH 13/17] merge conflict --- book/src/help_bn.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/book/src/help_bn.md b/book/src/help_bn.md index b52ac24615f..79c8d8ead85 100644 --- a/book/src/help_bn.md +++ b/book/src/help_bn.md @@ -385,9 +385,6 @@ Options: Number of validators per chunk stored on disk. --slots-per-restore-point DEPRECATED. This flag has no effect. - --state-cache-headroom - Minimum number of states to cull from the state cache when it gets - full [default: 1] --state-cache-size Specifies the size of the state cache [default: 128] --suggested-fee-recipient From 54d87c89ab4f237b26d0863f8eda2748a2b05919 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 11 Mar 2025 16:14:58 +1100 Subject: [PATCH 14/17] Fix descent from split check (#7105) --- beacon_node/beacon_chain/src/block_verification.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index f78b240e21b..f12753e1079 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1759,10 +1759,17 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< block: B, ) -> Result { // If we have a split block newer than finalization then we also ban blocks which are not - // descended from that split block. + // descended from that split block. It's important not to try checking `is_descendant` if + // finality is ahead of the split and the split block has been pruned, as `is_descendant` will + // return `false` in this case. + let finalized_slot = fork_choice + .finalized_checkpoint() + .epoch + .start_slot(T::EthSpec::slots_per_epoch()); let split = chain.store.get_split_info(); - let is_descendant_from_split_block = - split.slot == 0 || fork_choice.is_descendant(split.block_root, block.parent_root()); + let is_descendant_from_split_block = split.slot == 0 + || split.slot <= finalized_slot + || fork_choice.is_descendant(split.block_root, block.parent_root()); if fork_choice.is_finalized_checkpoint_or_descendant(block.parent_root()) && is_descendant_from_split_block From 2d0244928315dbc0e98e705b9a32129acdb8df19 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 11 Mar 2025 10:23:24 -0600 Subject: [PATCH 15/17] added test --- beacon_node/beacon_chain/tests/tests.rs | 122 +++++++++++++++++++++++- 1 file changed, 120 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index c641f32b820..27ee3aaa314 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -12,8 +12,8 @@ use operation_pool::PersistedOperationPool; use state_processing::{per_slot_processing, per_slot_processing::Error as SlotProcessingError}; use std::sync::LazyLock; use types::{ - BeaconState, BeaconStateError, BlockImportSource, EthSpec, Hash256, Keypair, MinimalEthSpec, - RelativeEpoch, Slot, + BeaconState, BeaconStateError, BlockImportSource, Checkpoint, EthSpec, Hash256, Keypair, + MinimalEthSpec, RelativeEpoch, Slot, }; // Should ideally be divisible by 3. @@ -869,3 +869,121 @@ async fn block_roots_skip_slot_behaviour() { "WhenSlotSkipped::Prev should return None on a future slot" ); } + +#[tokio::test] +async fn pseudo_finalize_test() { + // This test ensures that after pseudo finalization, we can still finalize the chain without issues + let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 5; + + let harness = get_harness(VALIDATOR_COUNT); + + let one_third = (VALIDATOR_COUNT / 3) * 1; + let attesters = (0..one_third).collect(); + + // extend the chain, but don't finalize + harness + .extend_chain( + num_blocks_produced as usize, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::SomeValidators(attesters), + ) + .await; + + harness.advance_slot(); + + let head = harness.chain.head_snapshot(); + let state = &head.beacon_state; + let split = harness.chain.store.get_split_info(); + + assert_eq!( + state.slot(), + num_blocks_produced, + "head should be at the current slot" + ); + assert_eq!( + state.current_epoch(), + num_blocks_produced / MinimalEthSpec::slots_per_epoch(), + "head should be at the expected epoch" + ); + assert_eq!( + state.current_justified_checkpoint().epoch, + 0, + "There should be no justified checkpoint" + ); + assert_eq!( + state.finalized_checkpoint().epoch, + 0, + "There should be no finalized checkpoint" + ); + assert_eq!(split.slot, 0, "Our split point should be unset"); + + let checkpoint = Checkpoint { + epoch: head.beacon_state.current_epoch(), + root: head.beacon_block_root, + }; + + // pseudo finalize + harness + .chain + .manually_finalize_state(head.beacon_state_root(), checkpoint) + .unwrap(); + + let split = harness.chain.store.get_split_info(); + + assert_eq!( + state.current_justified_checkpoint().epoch, + 0, + "We pseudo finalized, but our justified checkpoint should still be unset" + ); + assert_eq!( + state.finalized_checkpoint().epoch, + 0, + "We pseudo finalized, but our finalized checkpoint should still be unset" + ); + assert_eq!( + split.slot, + head.beacon_state.slot(), + "We pseudo finalized, our split point should be at the current head slot" + ); + + // finalize the chain + harness + .extend_chain( + num_blocks_produced as usize, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + harness.advance_slot(); + + let head = harness.chain.head_snapshot(); + let state = &head.beacon_state; + let split = harness.chain.store.get_split_info(); + + assert_eq!( + state.slot(), + num_blocks_produced * 2, + "head should be at the current slot" + ); + assert_eq!( + state.current_epoch(), + (num_blocks_produced * 2) / MinimalEthSpec::slots_per_epoch(), + "head should be at the expected epoch" + ); + assert_eq!( + state.current_justified_checkpoint().epoch, + state.current_epoch() - 1, + "the head should be justified one behind the current epoch" + ); + assert_eq!( + state.finalized_checkpoint().epoch, + state.current_epoch() - 2, + "the head should be finalized two behind the current epoch" + ); + assert_eq!( + split.slot, + u64::from((state.current_epoch() - 2) * MinimalEthSpec::slots_per_epoch()), + "We finalized, our split point should be at the finalized slot" + ); +} From 77163b2ca52b1935565a1df47518571f3ddfd3b0 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 11 Mar 2025 23:31:33 -0600 Subject: [PATCH 16/17] fix test --- beacon_node/beacon_chain/tests/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index 27ee3aaa314..5b6e595b743 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -877,7 +877,7 @@ async fn pseudo_finalize_test() { let harness = get_harness(VALIDATOR_COUNT); - let one_third = (VALIDATOR_COUNT / 3) * 1; + let one_third = VALIDATOR_COUNT / 3; let attesters = (0..one_third).collect(); // extend the chain, but don't finalize From e3142c5b85e3e94a5efb372097e11e971a326ce1 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 18 Mar 2025 15:25:29 +1100 Subject: [PATCH 17/17] Add second test for lagging split case --- beacon_node/beacon_chain/tests/tests.rs | 78 +++++++++++++++++++++---- 1 file changed, 67 insertions(+), 11 deletions(-) diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index 5b6e595b743..c801361fd5f 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -16,6 +16,8 @@ use types::{ MinimalEthSpec, RelativeEpoch, Slot, }; +type E = MinimalEthSpec; + // Should ideally be divisible by 3. pub const VALIDATOR_COUNT: usize = 48; @@ -24,12 +26,22 @@ static KEYPAIRS: LazyLock> = LazyLock::new(|| types::test_utils::generate_deterministic_keypairs(VALIDATOR_COUNT)); fn get_harness(validator_count: usize) -> BeaconChainHarness> { + get_harness_with_config( + validator_count, + ChainConfig { + reconstruct_historic_states: true, + ..Default::default() + }, + ) +} + +fn get_harness_with_config( + validator_count: usize, + chain_config: ChainConfig, +) -> BeaconChainHarness> { let harness = BeaconChainHarness::builder(MinimalEthSpec) .default_spec() - .chain_config(ChainConfig { - reconstruct_historic_states: true, - ..ChainConfig::default() - }) + .chain_config(chain_config) .keypairs(KEYPAIRS[0..validator_count].to_vec()) .fresh_ephemeral_store() .mock_execution_layer() @@ -870,12 +882,19 @@ async fn block_roots_skip_slot_behaviour() { ); } -#[tokio::test] -async fn pseudo_finalize_test() { +async fn pseudo_finalize_test_generic( + epochs_per_migration: u64, + expect_true_finalization_migration: bool, +) { // This test ensures that after pseudo finalization, we can still finalize the chain without issues let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 5; - let harness = get_harness(VALIDATOR_COUNT); + let chain_config = ChainConfig { + reconstruct_historic_states: true, + epochs_per_migration, + ..Default::default() + }; + let harness = get_harness_with_config(VALIDATOR_COUNT, chain_config); let one_third = VALIDATOR_COUNT / 3; let attesters = (0..one_third).collect(); @@ -929,6 +948,7 @@ async fn pseudo_finalize_test() { .unwrap(); let split = harness.chain.store.get_split_info(); + let pseudo_finalized_slot = split.slot; assert_eq!( state.current_justified_checkpoint().epoch, @@ -976,14 +996,50 @@ async fn pseudo_finalize_test() { state.current_epoch() - 1, "the head should be justified one behind the current epoch" ); + let finalized_epoch = state.finalized_checkpoint().epoch; assert_eq!( - state.finalized_checkpoint().epoch, + finalized_epoch, state.current_epoch() - 2, "the head should be finalized two behind the current epoch" ); + + let expected_split_slot = if pseudo_finalized_slot.epoch(E::slots_per_epoch()) + + epochs_per_migration + > finalized_epoch + { + pseudo_finalized_slot + } else { + finalized_epoch.start_slot(E::slots_per_epoch()) + }; assert_eq!( - split.slot, - u64::from((state.current_epoch() - 2) * MinimalEthSpec::slots_per_epoch()), - "We finalized, our split point should be at the finalized slot" + split.slot, expected_split_slot, + "We finalized, our split point should be updated according to epochs_per_migration" ); + + // In the case that we did not process the true finalization migration (due to + // epochs_per_migration), check that the chain finalized *despite* the absence of the split + // block in fork choice. + // This is a regression test for https://github.com/sigp/lighthouse/pull/7105 + if !expect_true_finalization_migration { + assert_eq!(expected_split_slot, pseudo_finalized_slot); + assert!(!harness + .chain + .canonical_head + .fork_choice_read_lock() + .contains_block(&split.block_root)); + } +} + +#[tokio::test] +async fn pseudo_finalize_basic() { + let epochs_per_migration = 0; + let expect_true_migration = true; + pseudo_finalize_test_generic(epochs_per_migration, expect_true_migration).await; +} + +#[tokio::test] +async fn pseudo_finalize_with_lagging_split_update() { + let epochs_per_migration = 10; + let expect_true_migration = false; + pseudo_finalize_test_generic(epochs_per_migration, expect_true_migration).await; }