diff --git a/beacon_node/beacon_chain/tests/main.rs b/beacon_node/beacon_chain/tests/main.rs index d31db128c5..e84f561fac 100644 --- a/beacon_node/beacon_chain/tests/main.rs +++ b/beacon_node/beacon_chain/tests/main.rs @@ -12,4 +12,5 @@ mod schema_stability; mod store_tests; mod sync_committee_verification; mod tests; +mod unrealized_checkpoints; mod validator_monitor; diff --git a/beacon_node/beacon_chain/tests/unrealized_checkpoints.rs b/beacon_node/beacon_chain/tests/unrealized_checkpoints.rs new file mode 100644 index 0000000000..01ae86c44d --- /dev/null +++ b/beacon_node/beacon_chain/tests/unrealized_checkpoints.rs @@ -0,0 +1,342 @@ +#![cfg(not(debug_assertions))] + +//! This file contains regression tests for a bug in fork choice whereby the unrealized justified +//! and finalized checkpoints of a block were assumed to carry over to its child. This is NOT TRUE +//! in general, as the child block may contain slashings which invalidate the +//! justification/finalization from the parent. The tests in this file reproduce this scenario using +//! both attester slashings and proposer slashings. + +use beacon_chain::{ + StateSkipConfig, + test_utils::{ + AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType, test_spec, + }, +}; +use state_processing::per_epoch_processing; +use std::sync::Arc; +use types::{Checkpoint, Epoch, EthSpec, MinimalEthSpec, consts::altair::TIMELY_TARGET_FLAG_INDEX}; + +type E = MinimalEthSpec; + +// Proposer slashings are limited to MaxProposerSlashings (16) per block. With 32 validators, +// dropping below the 2/3 justification threshold requires only ~11 slashes, which fits. +const VALIDATOR_COUNT: usize = 32; + +fn ceil_two_thirds(value: u64) -> u64 { + (2 * value).div_ceil(3) +} + +struct SameEpochSlashingChild { + harness: BeaconChainHarness>, + stored_parent_justified: Checkpoint, + stored_parent_finalized: Checkpoint, + stored_child_justified: Checkpoint, + stored_child_finalized: Checkpoint, + expected_child_justified: Checkpoint, + expected_child_finalized: Checkpoint, + parent_epoch: Epoch, +} + +/// Basic test checking that the child has correct unrealized justified/finalized checkpoints in the +/// case where the slashings are attester slashings. +#[tokio::test] +async fn child_unrealized_checkpoints_recomputed_after_same_epoch_slashing() { + let scenario = same_epoch_attester_slashing_child().await; + + assert_eq!( + scenario.stored_child_justified, scenario.expected_child_justified, + "child unrealized justified checkpoint should be recomputed from the child state" + ); + assert_eq!( + scenario.stored_child_finalized, scenario.expected_child_finalized, + "child unrealized finalized checkpoint should be recomputed from the child state" + ); +} + +/// Variation on the attester slashing test, checking that the inferior justification of the child +/// results in fork choice correctly reverting to the justified checkpoint once the child block is +/// no longer considered viable. +#[tokio::test] +async fn child_with_stale_voting_source_not_head_at_epoch_plus_two() { + let scenario = same_epoch_attester_slashing_child().await; + let slots_per_epoch = E::slots_per_epoch(); + let divergence_slot = scenario + .parent_epoch + .saturating_add(2u64) + .start_slot(slots_per_epoch); + + assert_eq!( + scenario.stored_child_justified, scenario.expected_child_justified, + "child unrealized justified checkpoint should be recomputed from the child state" + ); + assert_eq!( + scenario.stored_child_finalized, scenario.expected_child_finalized, + "child unrealized finalized checkpoint should be recomputed from the child state" + ); + assert!( + scenario.expected_child_justified.epoch.saturating_add(2u64) + < divergence_slot.epoch(slots_per_epoch), + "with the spec-computed checkpoint the child is outside the viability window at epoch N + 2" + ); + + while scenario.harness.get_current_slot() < divergence_slot { + scenario.harness.advance_slot(); + } + + let mut fork_choice = scenario + .harness + .chain + .canonical_head + .fork_choice_write_lock(); + let head_result = fork_choice.get_head(divergence_slot, &scenario.harness.chain.spec); + + assert_eq!( + fork_choice.justified_checkpoint(), + scenario.stored_parent_justified, + "the store should realize the parent's unrealized justification at the epoch boundary" + ); + assert_eq!( + fork_choice.finalized_checkpoint(), + scenario.stored_parent_finalized, + "the store should realize the parent's unrealized finalization at the epoch boundary" + ); + + // No epoch N + 1 blocks were produced after the slashing child. Under the spec-computed child + // checkpoint, the child is the only leaf below the justified root and is outside the viability + // window. The spec-correct result is to set the justified checkpoint as the head. + assert_eq!( + head_result.unwrap().0, + fork_choice.justified_checkpoint().root + ); +} + +/// Basic test checking child checkpoints but with proposer slashings instead of attester slashings. +#[tokio::test] +async fn child_unrealized_checkpoints_recomputed_after_same_epoch_proposer_slashing() { + let scenario = same_epoch_proposer_slashing_child().await; + + assert_eq!( + scenario.stored_child_justified, scenario.expected_child_justified, + "child unrealized justified checkpoint should be recomputed from the child state" + ); + assert_eq!( + scenario.stored_child_finalized, scenario.expected_child_finalized, + "child unrealized finalized checkpoint should be recomputed from the child state" + ); +} + +async fn same_epoch_attester_slashing_child() -> SameEpochSlashingChild { + same_epoch_slashing_child(VALIDATOR_COUNT, |harness, slash_indices| { + harness + .add_attester_slashing(slash_indices.to_vec()) + .expect("should add attester slashing to operation pool"); + }) + .await +} + +async fn same_epoch_proposer_slashing_child() -> SameEpochSlashingChild { + same_epoch_slashing_child(VALIDATOR_COUNT, |harness, slash_indices| { + for &index in slash_indices { + harness + .add_proposer_slashing(index) + .expect("should add proposer slashing to operation pool"); + } + }) + .await +} + +/// Generic scenario builder with `inject_slashings` capable of injecting attester or proposer +/// slashings. +async fn same_epoch_slashing_child( + validator_count: usize, + inject_slashings: F, +) -> SameEpochSlashingChild +where + F: FnOnce(&BeaconChainHarness>, &[u64]), +{ + let spec = test_spec::(); + + let harness: BeaconChainHarness> = + BeaconChainHarness::builder(E::default()) + .spec(Arc::new(spec)) + .deterministic_keypairs(validator_count) + .fresh_ephemeral_store() + .mock_execution_layer() + .build(); + + let slots_per_epoch = E::slots_per_epoch(); + + // Minimum warm-up for the parent to reach FFG steady state (justified == epoch, finalized == + // epoch - 1); 2 epochs is too few. + let warmup_epochs: u64 = 3; + harness.advance_slot(); + harness + .extend_chain( + slots_per_epoch as usize * warmup_epochs as usize, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + let parent_epoch = Epoch::new(warmup_epochs); + // ceil(Minimal::slots_per_epoch() * 2/3) = 6 + let parent_slot = parent_epoch.start_slot(slots_per_epoch) + 6; + let parent_root = harness.extend_to_slot(parent_slot).await; + + let mut parent_state = harness + .chain + .state_at_slot(parent_slot, StateSkipConfig::WithStateRoots) + .expect("should load parent state"); + parent_state + .build_caches(&harness.chain.spec) + .expect("should build parent state caches"); + + let total_active_balance = parent_state + .get_total_active_balance() + .expect("should get parent total active balance"); + let required_balance = ceil_two_thirds(total_active_balance); + let effective_balance = parent_state + .validators() + .get(0) + .expect("validator 0 should exist") + .effective_balance; + let slash_count_needed = total_active_balance + .checked_sub(required_balance) + .expect("total active balance should be at least the required balance") + / effective_balance + + 1; + + let child_slot = parent_slot + 1; + let child_proposer = parent_state + .get_beacon_proposer_index(child_slot, &harness.chain.spec) + .expect("should get child proposer") as u64; + + let (_, _, _, current_participation, _, _, _, _) = parent_state + .mutable_validator_fields() + .expect("parent state should have Altair validator fields"); + // Slash this epoch's timely-target voters (they count toward the current-epoch target balance), + // excluding the child proposer, to drop target balance below the 2/3 justification threshold. + let slash_indices = current_participation + .iter() + .enumerate() + .filter_map(|(index, flags)| { + flags + .has_flag(TIMELY_TARGET_FLAG_INDEX) + .ok() + .and_then(|has_flag| has_flag.then_some(index as u64)) + }) + .filter(|index| *index != child_proposer) + .take(slash_count_needed as usize) + .collect::>(); + + assert_eq!( + slash_indices.len(), + slash_count_needed as usize, + "should have enough current target attesters to slash" + ); + + inject_slashings(&harness, &slash_indices); + + harness.advance_slot(); + let child_root = harness + .extend_chain( + 1, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::AllValidators, + ) + .await; + + let ( + stored_parent_justified, + stored_parent_finalized, + stored_child_justified, + stored_child_finalized, + child_slot, + ) = { + let fork_choice = harness.chain.canonical_head.fork_choice_read_lock(); + let parent_block = fork_choice + .get_block(&parent_root) + .expect("parent should be in fork choice"); + let child_block = fork_choice + .get_block(&child_root) + .expect("child should be in fork choice"); + + let parent_justified = parent_block + .unrealized_justified_checkpoint + .expect("parent should have unrealized justified checkpoint"); + let parent_finalized = parent_block + .unrealized_finalized_checkpoint + .expect("parent should have unrealized finalized checkpoint"); + + assert_eq!(parent_block.slot, parent_slot); + assert_eq!(parent_justified.epoch, parent_epoch); + assert_eq!(parent_finalized.epoch.saturating_add(1u64), parent_epoch); + assert_eq!(child_block.slot, parent_slot + 1); + assert_eq!(child_block.slot.epoch(slots_per_epoch), parent_epoch); + + ( + parent_justified, + parent_finalized, + child_block + .unrealized_justified_checkpoint + .expect("child should have unrealized justified checkpoint"), + child_block + .unrealized_finalized_checkpoint + .expect("child should have unrealized finalized checkpoint"), + child_block.slot, + ) + }; + + let mut child_state = harness + .chain + .state_at_slot(child_slot, StateSkipConfig::WithStateRoots) + .expect("should load child state"); + child_state + .build_caches(&harness.chain.spec) + .expect("should build child state caches"); + + let slashed = child_state + .validators() + .iter() + .enumerate() + .filter_map(|(index, validator)| validator.slashed.then_some(index as u64)) + .collect::>(); + let child_total_active_balance = child_state + .get_total_active_balance() + .expect("should get child total active balance"); + let child_current_target_balance = child_state + .progressive_balances_cache() + .current_epoch_target_attesting_balance() + .expect("should get child current target balance"); + let child_justification_and_finalization = + per_epoch_processing::altair::process_justification_and_finalization(&child_state) + .expect("should recompute child justification and finalization"); + let expected_child_justified = + child_justification_and_finalization.current_justified_checkpoint(); + let expected_child_finalized = child_justification_and_finalization.finalized_checkpoint(); + + assert_eq!(slashed, slash_indices); + assert!( + child_current_target_balance < ceil_two_thirds(child_total_active_balance), + "slashings should reduce current target balance below the justification threshold" + ); + assert_ne!( + expected_child_justified, stored_parent_justified, + "test setup should make the child justified checkpoint differ from the parent's" + ); + assert_ne!( + expected_child_finalized, stored_parent_finalized, + "test setup should make the child finalized checkpoint differ from the parent's" + ); + + SameEpochSlashingChild { + harness, + stored_parent_justified, + stored_parent_finalized, + stored_child_justified, + stored_child_finalized, + expected_child_justified, + expected_child_finalized, + parent_epoch, + } +} diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index e648d5669b..aca5ab7851 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -894,22 +894,29 @@ where // Update unrealized justified/finalized checkpoints. let block_epoch = block.slot().epoch(E::slots_per_epoch()); - // If the parent checkpoints are already at the same epoch as the block being imported, - // it's impossible for the unrealized checkpoints to differ from the parent's. This - // holds true because: + // If the block has no slashings and the parent checkpoints are already at the same epoch as + // the block being imported, it's impossible for the unrealized checkpoints to differ from + // the parent's. This holds true because: // // 1. A child block cannot have lower FFG checkpoints than its parent. // 2. A block in epoch `N` cannot contain attestations which would justify an epoch higher than `N`. // 3. A block in epoch `N` cannot contain attestations which would finalize an epoch higher than `N - 1`. // + // Slashings are excluded from this optimization because they can reduce unslashed + // participation in the child state and therefore lower the child's unrealized checkpoints. + // // This is an optimization. It should reduce the amount of times we run // `process_justification_and_finalization` by approximately 1/3rd when the chain is // performing optimally. + let has_slashings = !block.body().proposer_slashings().is_empty() + || block.body().attester_slashings_len() > 0; let parent_checkpoints = parent_block .unrealized_justified_checkpoint .zip(parent_block.unrealized_finalized_checkpoint) .filter(|(parent_justified, parent_finalized)| { - parent_justified.epoch == block_epoch && parent_finalized.epoch + 1 == block_epoch + !has_slashings + && parent_justified.epoch == block_epoch + && parent_finalized.epoch.saturating_add(1u64) == block_epoch }); let (unrealized_justified_checkpoint, unrealized_finalized_checkpoint) =