diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 4429abc4c..86b43a1a3 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1991,60 +1991,75 @@ impl BeaconChain { target_epoch: Epoch, state: &BeaconState, ) -> bool { - let slots_per_epoch = T::EthSpec::slots_per_epoch(); - let shuffling_lookahead = 1 + self.spec.min_seed_lookahead.as_u64(); - - // Shuffling can't have changed if we're in the first few epochs - if state.current_epoch() < shuffling_lookahead { - return true; - } - - // Otherwise the shuffling is determined by the block at the end of the target epoch - // minus the shuffling lookahead (usually 2). We call this the "pivot". - let pivot_slot = - if target_epoch == state.previous_epoch() || target_epoch == state.current_epoch() { - (target_epoch - shuffling_lookahead).end_slot(slots_per_epoch) - } else { - return false; - }; - - let state_pivot_block_root = match state.get_block_root(pivot_slot) { - Ok(root) => *root, - Err(e) => { - warn!( - &self.log, - "Missing pivot block root for attestation"; - "slot" => pivot_slot, - "error" => ?e, - ); - return false; - } - }; - - // Use fork choice's view of the block DAG to quickly evaluate whether the attestation's - // pivot block is the same as the current state's pivot block. If it is, then the - // attestation's shuffling is the same as the current state's. - // To account for skipped slots, find the first block at *or before* the pivot slot. - let fork_choice_lock = self.canonical_head.fork_choice_read_lock(); - let pivot_block_root = fork_choice_lock - .proto_array() - .core_proto_array() - .iter_block_roots(block_root) - .find(|(_, slot)| *slot <= pivot_slot) - .map(|(block_root, _)| block_root); - drop(fork_choice_lock); - - match pivot_block_root { - Some(root) => root == state_pivot_block_root, - None => { + self.shuffling_is_compatible_result(block_root, target_epoch, state) + .unwrap_or_else(|e| { debug!( - &self.log, - "Discarding attestation because of missing ancestor"; - "pivot_slot" => pivot_slot.as_u64(), + self.log, + "Skipping attestation with incompatible shuffling"; "block_root" => ?block_root, + "target_epoch" => target_epoch, + "reason" => ?e, ); false + }) + } + + fn shuffling_is_compatible_result( + &self, + block_root: &Hash256, + target_epoch: Epoch, + state: &BeaconState, + ) -> Result { + // Compute the shuffling ID for the head state in the `target_epoch`. + let relative_epoch = RelativeEpoch::from_epoch(state.current_epoch(), target_epoch) + .map_err(|e| Error::BeaconStateError(e.into()))?; + let head_shuffling_id = + AttestationShufflingId::new(self.genesis_block_root, state, relative_epoch)?; + + // Load the block's shuffling ID from fork choice. We use the variant of `get_block` that + // checks descent from the finalized block, so there's one case where we'll spuriously + // return `false`: where an attestation for the previous epoch nominates the pivot block + // which is the parent block of the finalized block. Such attestations are not useful, so + // this doesn't matter. + let fork_choice_lock = self.canonical_head.fork_choice_read_lock(); + let block = fork_choice_lock + .get_block(block_root) + .ok_or(Error::AttestationHeadNotInForkChoice(*block_root))?; + drop(fork_choice_lock); + + let block_shuffling_id = if target_epoch == block.current_epoch_shuffling_id.shuffling_epoch + { + block.current_epoch_shuffling_id + } else if target_epoch == block.next_epoch_shuffling_id.shuffling_epoch { + block.next_epoch_shuffling_id + } else if target_epoch > block.next_epoch_shuffling_id.shuffling_epoch { + AttestationShufflingId { + shuffling_epoch: target_epoch, + shuffling_decision_block: *block_root, } + } else { + debug!( + self.log, + "Skipping attestation with incompatible shuffling"; + "block_root" => ?block_root, + "target_epoch" => target_epoch, + "reason" => "target epoch less than block epoch" + ); + return Ok(false); + }; + + if head_shuffling_id == block_shuffling_id { + Ok(true) + } else { + debug!( + self.log, + "Skipping attestation with incompatible shuffling"; + "block_root" => ?block_root, + "target_epoch" => target_epoch, + "head_shuffling_id" => ?head_shuffling_id, + "block_shuffling_id" => ?block_shuffling_id, + ); + Ok(false) } } @@ -4460,7 +4475,7 @@ impl BeaconChain { /// /// If the committee for `(head_block_root, shuffling_epoch)` isn't found in the /// `shuffling_cache`, we will read a state from disk and then update the `shuffling_cache`. - pub(crate) fn with_committee_cache( + pub fn with_committee_cache( &self, head_block_root: Hash256, shuffling_epoch: Epoch, diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 883b871b1..b1907bc96 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -811,7 +811,6 @@ async fn shuffling_compatible_linear_chain() { let store = get_store(&db_path); let harness = get_harness(store.clone(), LOW_VALIDATOR_COUNT); - // Skip the block at the end of the first epoch. let head_block_root = harness .extend_chain( 4 * E::slots_per_epoch() as usize, @@ -824,10 +823,6 @@ async fn shuffling_compatible_linear_chain() { &harness, &get_state_for_block(&harness, head_block_root), head_block_root, - true, - true, - None, - None, ); } @@ -859,10 +854,6 @@ async fn shuffling_compatible_missing_pivot_block() { &harness, &get_state_for_block(&harness, head_block_root), head_block_root, - true, - true, - Some(E::slots_per_epoch() - 2), - Some(E::slots_per_epoch() - 2), ); } @@ -880,10 +871,10 @@ async fn shuffling_compatible_simple_fork() { let head1_state = get_state_for_block(&harness, head1); let head2_state = get_state_for_block(&harness, head2); - check_shuffling_compatible(&harness, &head1_state, head1, true, true, None, None); - check_shuffling_compatible(&harness, &head1_state, head2, false, false, None, None); - check_shuffling_compatible(&harness, &head2_state, head1, false, false, None, None); - check_shuffling_compatible(&harness, &head2_state, head2, true, true, None, None); + check_shuffling_compatible(&harness, &head1_state, head1); + check_shuffling_compatible(&harness, &head1_state, head2); + check_shuffling_compatible(&harness, &head2_state, head1); + check_shuffling_compatible(&harness, &head2_state, head2); drop(db_path); } @@ -902,21 +893,10 @@ async fn shuffling_compatible_short_fork() { let head1_state = get_state_for_block(&harness, head1); let head2_state = get_state_for_block(&harness, head2); - check_shuffling_compatible(&harness, &head1_state, head1, true, true, None, None); - check_shuffling_compatible(&harness, &head1_state, head2, false, true, None, None); - // NOTE: don't check this case, as block 14 from the first chain appears valid on the second - // chain due to it matching the second chain's block 15. - // check_shuffling_compatible(&harness, &head2_state, head1, false, true, None, None); - check_shuffling_compatible( - &harness, - &head2_state, - head2, - true, - true, - // Required because of the skipped slot. - Some(2 * E::slots_per_epoch() - 2), - None, - ); + check_shuffling_compatible(&harness, &head1_state, head1); + check_shuffling_compatible(&harness, &head1_state, head2); + check_shuffling_compatible(&harness, &head2_state, head1); + check_shuffling_compatible(&harness, &head2_state, head2); drop(db_path); } @@ -940,54 +920,82 @@ fn check_shuffling_compatible( harness: &TestHarness, head_state: &BeaconState, head_block_root: Hash256, - current_epoch_valid: bool, - previous_epoch_valid: bool, - current_epoch_cutoff_slot: Option, - previous_epoch_cutoff_slot: Option, ) { - let shuffling_lookahead = harness.chain.spec.min_seed_lookahead.as_u64() + 1; - let current_pivot_slot = - (head_state.current_epoch() - shuffling_lookahead).end_slot(E::slots_per_epoch()); - let previous_pivot_slot = - (head_state.previous_epoch() - shuffling_lookahead).end_slot(E::slots_per_epoch()); - for maybe_tuple in harness .chain .rev_iter_block_roots_from(head_block_root) .unwrap() { let (block_root, slot) = maybe_tuple.unwrap(); - // Shuffling is compatible targeting the current epoch, - // if slot is greater than or equal to the current epoch pivot block. - assert_eq!( - harness.chain.shuffling_is_compatible( - &block_root, + + // Would an attestation to `block_root` at the current epoch be compatible with the head + // state's shuffling? + let current_epoch_shuffling_is_compatible = harness.chain.shuffling_is_compatible( + &block_root, + head_state.current_epoch(), + &head_state, + ); + + // Check for consistency with the more expensive shuffling lookup. + harness + .chain + .with_committee_cache( + block_root, head_state.current_epoch(), - &head_state - ), - current_epoch_valid - && slot >= current_epoch_cutoff_slot.unwrap_or(current_pivot_slot.as_u64()) - ); + |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}"); + } else { + assert_ne!(committee_cache, state_cache, "block at slot {slot}"); + } + Ok(()) + }, + ) + .unwrap_or_else(|e| { + // If the lookup fails then the shuffling must be invalid in some way, e.g. the + // block with `block_root` is from a later epoch than `previous_epoch`. + assert!( + !current_epoch_shuffling_is_compatible, + "block at slot {slot} has compatible shuffling at epoch {} \ + but should be incompatible due to error: {e:?}", + head_state.current_epoch() + ); + }); + // Similarly for the previous epoch - assert_eq!( - harness.chain.shuffling_is_compatible( - &block_root, + let previous_epoch_shuffling_is_compatible = harness.chain.shuffling_is_compatible( + &block_root, + head_state.previous_epoch(), + &head_state, + ); + harness + .chain + .with_committee_cache( + block_root, head_state.previous_epoch(), - &head_state - ), - previous_epoch_valid - && slot >= previous_epoch_cutoff_slot.unwrap_or(previous_pivot_slot.as_u64()) - ); - // Targeting the next epoch should always return false - assert_eq!( - harness.chain.shuffling_is_compatible( - &block_root, - head_state.current_epoch() + 1, - &head_state - ), - false - ); - // Targeting two epochs before the current epoch should also always return false + |committee_cache, _| { + let state_cache = head_state.committee_cache(RelativeEpoch::Previous).unwrap(); + if previous_epoch_shuffling_is_compatible { + assert_eq!(committee_cache, state_cache); + } else { + assert_ne!(committee_cache, state_cache); + } + Ok(()) + }, + ) + .unwrap_or_else(|e| { + // If the lookup fails then the shuffling must be invalid in some way, e.g. the + // block with `block_root` is from a later epoch than `previous_epoch`. + assert!( + !previous_epoch_shuffling_is_compatible, + "block at slot {slot} has compatible shuffling at epoch {} \ + but should be incompatible due to error: {e:?}", + head_state.previous_epoch() + ); + }); + + // Targeting two epochs before the current epoch should always return false if head_state.current_epoch() >= 2 { assert_eq!( harness.chain.shuffling_is_compatible(