From 40abaefffbf4b0946f8ac5dde1ab6d1dcc496e00 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Mon, 15 May 2023 02:10:41 +0000 Subject: [PATCH] Attestation verification uses head state fork (#4263) ## Issue Addressed Addresses #4238 ## Proposed Changes - [x] Add tests for the scenarios - [x] Use the fork of the attestation slot for signature verification. --- .../src/attestation_verification/batch.rs | 10 +- beacon_node/beacon_chain/src/test_utils.rs | 62 +++-- .../tests/attestation_verification.rs | 218 +++++++++++++++++- 3 files changed, 260 insertions(+), 30 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification/batch.rs b/beacon_node/beacon_chain/src/attestation_verification/batch.rs index 6f76cce02..6aec2bef6 100644 --- a/beacon_node/beacon_chain/src/attestation_verification/batch.rs +++ b/beacon_node/beacon_chain/src/attestation_verification/batch.rs @@ -65,14 +65,15 @@ where .try_read_for(VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT) .ok_or(BeaconChainError::ValidatorPubkeyCacheLockTimeout)?; - let fork = chain.canonical_head.cached_head().head_fork(); - let mut signature_sets = Vec::with_capacity(num_indexed * 3); // Iterate, flattening to get only the `Ok` values. for indexed in indexing_results.iter().flatten() { let signed_aggregate = &indexed.signed_aggregate; let indexed_attestation = &indexed.indexed_attestation; + let fork = chain + .spec + .fork_at_epoch(indexed_attestation.data.target.epoch); signature_sets.push( signed_aggregate_selection_proof_signature_set( @@ -169,8 +170,6 @@ where &metrics::ATTESTATION_PROCESSING_BATCH_UNAGG_SIGNATURE_SETUP_TIMES, ); - let fork = chain.canonical_head.cached_head().head_fork(); - let pubkey_cache = chain .validator_pubkey_cache .try_read_for(VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT) @@ -181,6 +180,9 @@ where // Iterate, flattening to get only the `Ok` values. for partially_verified in partial_results.iter().flatten() { let indexed_attestation = &partially_verified.indexed_attestation; + let fork = chain + .spec + .fork_at_epoch(indexed_attestation.data.target.epoch); let signature_set = indexed_attestation_signature_set_from_pubkeys( |validator_index| pubkey_cache.get(validator_index).map(Cow::Borrowed), diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 37a2f315c..c5615b618 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -64,7 +64,7 @@ const FORK_NAME_ENV_VAR: &str = "FORK_NAME"; // // You should mutate the `ChainSpec` prior to initialising the harness if you would like to use // a different value. -pub const DEFAULT_TARGET_AGGREGATORS: u64 = u64::max_value(); +pub const DEFAULT_TARGET_AGGREGATORS: u64 = u64::MAX; pub type BaseHarnessType = Witness, TEthSpec, THotStore, TColdStore>; @@ -941,31 +941,31 @@ where head_block_root: SignedBeaconBlockHash, attestation_slot: Slot, ) -> Vec> { - self.make_unaggregated_attestations_with_limit( + let fork = self + .spec + .fork_at_epoch(attestation_slot.epoch(E::slots_per_epoch())); + self.make_unaggregated_attestations_with_opts( attesting_validators, state, state_root, head_block_root, attestation_slot, - None, + MakeAttestationOptions { limit: None, fork }, ) .0 } - pub fn make_unaggregated_attestations_with_limit( + pub fn make_unaggregated_attestations_with_opts( &self, attesting_validators: &[usize], state: &BeaconState, state_root: Hash256, head_block_root: SignedBeaconBlockHash, attestation_slot: Slot, - limit: Option, + opts: MakeAttestationOptions, ) -> (Vec>, Vec) { + let MakeAttestationOptions { limit, fork } = opts; let committee_count = state.get_committee_count_at_slot(state.slot()).unwrap(); - let fork = self - .spec - .fork_at_epoch(attestation_slot.epoch(E::slots_per_epoch())); - let attesters = Mutex::new(vec![]); let attestations = state @@ -1155,16 +1155,35 @@ where slot: Slot, limit: Option, ) -> (HarnessAttestations, Vec) { - let (unaggregated_attestations, attesters) = self - .make_unaggregated_attestations_with_limit( - attesting_validators, - state, - state_root, - block_hash, - slot, - limit, - ); let fork = self.spec.fork_at_epoch(slot.epoch(E::slots_per_epoch())); + self.make_attestations_with_opts( + attesting_validators, + state, + state_root, + block_hash, + slot, + MakeAttestationOptions { limit, fork }, + ) + } + + pub fn make_attestations_with_opts( + &self, + attesting_validators: &[usize], + state: &BeaconState, + state_root: Hash256, + block_hash: SignedBeaconBlockHash, + slot: Slot, + opts: MakeAttestationOptions, + ) -> (HarnessAttestations, Vec) { + let MakeAttestationOptions { fork, .. } = opts; + let (unaggregated_attestations, attesters) = self.make_unaggregated_attestations_with_opts( + attesting_validators, + state, + state_root, + block_hash, + slot, + opts, + ); let aggregated_attestations: Vec>> = unaggregated_attestations @@ -2223,3 +2242,10 @@ impl fmt::Debug for BeaconChainHarness { write!(f, "BeaconChainHarness") } } + +pub struct MakeAttestationOptions { + /// Produce exactly `limit` attestations. + pub limit: Option, + /// Fork to use for signing attestations. + pub fork: Fork, +} diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 3ebc3c8cf..1040521e5 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -1,6 +1,9 @@ #![cfg(not(debug_assertions))] -use beacon_chain::test_utils::HARNESS_GENESIS_TIME; +use beacon_chain::attestation_verification::{ + batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations, Error, +}; +use beacon_chain::test_utils::{MakeAttestationOptions, HARNESS_GENESIS_TIME}; use beacon_chain::{ attestation_verification::Error as AttnError, test_utils::{ @@ -17,8 +20,8 @@ use state_processing::{ use tree_hash::TreeHash; use types::{ test_utils::generate_deterministic_keypair, Address, AggregateSignature, Attestation, - BeaconStateError, BitList, Epoch, EthSpec, Hash256, Keypair, MainnetEthSpec, SecretKey, - SelectionProof, SignedAggregateAndProof, Slot, SubnetId, Unsigned, + BeaconStateError, BitList, ChainSpec, Epoch, EthSpec, ForkName, Hash256, Keypair, + MainnetEthSpec, SecretKey, SelectionProof, SignedAggregateAndProof, Slot, SubnetId, Unsigned, }; pub type E = MainnetEthSpec; @@ -27,6 +30,8 @@ pub type E = MainnetEthSpec; /// have committees where _some_ validators are aggregators but not _all_. pub const VALIDATOR_COUNT: usize = 256; +pub const CAPELLA_FORK_EPOCH: usize = 1; + lazy_static! { /// A cached set of keys. static ref KEYPAIRS: Vec = types::test_utils::generate_deterministic_keypairs(VALIDATOR_COUNT); @@ -54,11 +59,13 @@ fn get_harness(validator_count: usize) -> BeaconChainHarness BeaconChainHarness> { +fn get_harness_capella_spec( + validator_count: usize, +) -> (BeaconChainHarness>, ChainSpec) { let mut spec = E::default_spec(); spec.altair_fork_epoch = Some(Epoch::new(0)); spec.bellatrix_fork_epoch = Some(Epoch::new(0)); - spec.capella_fork_epoch = Some(Epoch::new(1)); + spec.capella_fork_epoch = Some(Epoch::new(CAPELLA_FORK_EPOCH as u64)); let validator_keypairs = KEYPAIRS[0..validator_count].to_vec(); let genesis_state = interop_genesis_state( @@ -71,7 +78,7 @@ fn get_harness_capella_spec(validator_count: usize) -> BeaconChainHarness BeaconChainHarness(first_capella_slot), + ForkName::Capella + ); + + let (state, state_root) = harness.get_current_state_and_root(); + + // Scenario 1: other node signed attestation using the Capella fork epoch. + { + let attesters = (0..VALIDATOR_COUNT / 2).collect::>(); + let capella_fork = spec.fork_for_name(ForkName::Capella).unwrap(); + let committee_attestations = harness + .make_unaggregated_attestations_with_opts( + attesters.as_slice(), + &state, + state_root, + pre_capella_block.canonical_root().into(), + first_capella_slot, + MakeAttestationOptions { + fork: capella_fork, + limit: None, + }, + ) + .0 + .first() + .cloned() + .expect("should have at least one committee"); + let attestations_and_subnets = committee_attestations + .iter() + .map(|(attestation, subnet_id)| (attestation, Some(*subnet_id))); + + assert!( + batch_verify_unaggregated_attestations(attestations_and_subnets, &harness.chain).is_ok(), + "should accept attestations with `data.slot` >= first capella slot signed using the Capella fork" + ); + } + + // Scenario 2: other node forgot to update their node and signed attestations using bellatrix fork + { + let attesters = (VALIDATOR_COUNT / 2..VALIDATOR_COUNT).collect::>(); + let merge_fork = spec.fork_for_name(ForkName::Merge).unwrap(); + let committee_attestations = harness + .make_unaggregated_attestations_with_opts( + attesters.as_slice(), + &state, + state_root, + pre_capella_block.canonical_root().into(), + first_capella_slot, + MakeAttestationOptions { + fork: merge_fork, + limit: None, + }, + ) + .0 + .first() + .cloned() + .expect("should have at least one committee"); + let attestations_and_subnets = committee_attestations + .iter() + .map(|(attestation, subnet_id)| (attestation, Some(*subnet_id))); + + let results = + batch_verify_unaggregated_attestations(attestations_and_subnets, &harness.chain) + .expect("should return attestation results"); + let error = results + .into_iter() + .collect::, _>>() + .err() + .expect("should return an error"); + assert!( + matches!(error, Error::InvalidSignature), + "should reject attestations with `data.slot` >= first capella slot signed using the pre-Capella fork" + ); + } +} + +#[tokio::test] +async fn aggregated_attestation_verification_use_head_state_fork() { + let (harness, spec) = get_harness_capella_spec(VALIDATOR_COUNT); + + // Advance to last block of the pre-Capella fork epoch. Capella is at slot 32. + harness + .extend_chain( + MainnetEthSpec::slots_per_epoch() as usize * CAPELLA_FORK_EPOCH - 1, + BlockStrategy::OnCanonicalHead, + AttestationStrategy::SomeValidators(vec![]), + ) + .await; + + // Assert our head is a block at slot 31 in the pre-Capella fork epoch. + let pre_capella_slot = harness.get_current_slot(); + let pre_capella_block = harness + .chain + .block_at_slot(pre_capella_slot, WhenSlotSkipped::Prev) + .expect("should not error getting block at slot") + .expect("should find block at slot"); + assert_eq!(pre_capella_block.fork_name(&spec).unwrap(), ForkName::Merge); + + // Advance slot clock to Capella fork. + harness.advance_slot(); + let first_capella_slot = harness.get_current_slot(); + assert_eq!( + spec.fork_name_at_slot::(first_capella_slot), + ForkName::Capella + ); + + let (state, state_root) = harness.get_current_state_and_root(); + + // Scenario 1: other node signed attestation using the Capella fork epoch. + { + let attesters = (0..VALIDATOR_COUNT / 2).collect::>(); + let capella_fork = spec.fork_for_name(ForkName::Capella).unwrap(); + let aggregates = harness + .make_attestations_with_opts( + attesters.as_slice(), + &state, + state_root, + pre_capella_block.canonical_root().into(), + first_capella_slot, + MakeAttestationOptions { + fork: capella_fork, + limit: None, + }, + ) + .0 + .into_iter() + .map(|(_, aggregate)| aggregate.expect("should have signed aggregate and proof")) + .collect::>(); + + assert!( + batch_verify_aggregated_attestations(aggregates.iter(), &harness.chain).is_ok(), + "should accept aggregates with `data.slot` >= first capella slot signed using the Capella fork" + ); + } + + // Scenario 2: other node forgot to update their node and signed attestations using bellatrix fork + { + let attesters = (VALIDATOR_COUNT / 2..VALIDATOR_COUNT).collect::>(); + let merge_fork = spec.fork_for_name(ForkName::Merge).unwrap(); + let aggregates = harness + .make_attestations_with_opts( + attesters.as_slice(), + &state, + state_root, + pre_capella_block.canonical_root().into(), + first_capella_slot, + MakeAttestationOptions { + fork: merge_fork, + limit: None, + }, + ) + .0 + .into_iter() + .map(|(_, aggregate)| aggregate.expect("should have signed aggregate and proof")) + .collect::>(); + + let results = batch_verify_aggregated_attestations(aggregates.iter(), &harness.chain) + .expect("should return attestation results"); + let error = results + .into_iter() + .collect::, _>>() + .err() + .expect("should return an error"); + assert!( + matches!(error, Error::InvalidSignature), + "should reject aggregates with `data.slot` >= first capella slot signed using the pre-Capella fork" + ); + } +}