From a74098044af57d41e6850d991e39c04b89a59523 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Thu, 13 Jun 2024 19:04:30 -0400 Subject: [PATCH 01/53] Rust 1.79 lints (#5927) * max_value -> MAX * remove unnecesary closures * a couple more max_value -> MAX * a couple more max_value -> MAX * Revert "a couple more max_value -> MAX" This reverts commit 807fe7cae9df4c3349fc9993499139404c838455. * unused spec field -> phantom data * ignore some dead code warnings * update kurtosis repo location --- beacon_node/beacon_chain/src/eth1_chain.rs | 2 +- .../beacon_chain/src/validator_monitor.rs | 2 +- beacon_node/client/src/notifier.rs | 6 +- beacon_node/eth1/src/service.rs | 9 +-- .../execution_layer/src/engine_api/http.rs | 2 +- beacon_node/execution_layer/src/lib.rs | 2 +- beacon_node/execution_layer/src/metrics.rs | 2 +- beacon_node/http_api/tests/tests.rs | 6 +- .../lighthouse_network/src/discovery/mod.rs | 4 +- .../src/network_beacon_processor/tests.rs | 4 +- beacon_node/operation_pool/src/lib.rs | 4 +- common/deposit_contract/src/lib.rs | 2 +- .../src/fork_choice_test_definition.rs | 6 +- .../src/fork_choice_test_definition/votes.rs | 2 +- .../src/proto_array_fork_choice.rs | 2 +- consensus/safe_arith/src/lib.rs | 14 ++--- .../base/validator_statuses.rs | 2 +- .../src/compute_shuffled_index.rs | 6 +- .../swap_or_not_shuffle/src/shuffle_list.rs | 7 +-- consensus/types/benches/benches.rs | 4 +- .../types/src/beacon_state/committee_cache.rs | 2 +- consensus/types/src/slot_epoch.rs | 8 +-- consensus/types/src/slot_epoch_macros.rs | 56 +++++++++---------- consensus/types/src/validator.rs | 10 ++-- crypto/eth2_key_derivation/tests/tests.rs | 4 +- crypto/eth2_wallet/src/wallet.rs | 2 +- scripts/local_testnet/start_local_testnet.sh | 2 +- scripts/tests/network_params.yaml | 2 +- validator_client/src/doppelganger_service.rs | 4 +- watch/src/blockprint/database.rs | 1 + watch/src/blockprint/mod.rs | 1 + watch/src/server/error.rs | 1 + watch/src/updater/error.rs | 1 + watch/src/updater/handler.rs | 5 +- 34 files changed, 92 insertions(+), 95 deletions(-) diff --git a/beacon_node/beacon_chain/src/eth1_chain.rs b/beacon_node/beacon_chain/src/eth1_chain.rs index 31297244e3..ee50e3b384 100644 --- a/beacon_node/beacon_chain/src/eth1_chain.rs +++ b/beacon_node/beacon_chain/src/eth1_chain.rs @@ -685,7 +685,7 @@ mod test { fn get_eth1_data(i: u64) -> Eth1Data { Eth1Data { block_hash: Hash256::from_low_u64_be(i), - deposit_root: Hash256::from_low_u64_be(u64::max_value() - i), + deposit_root: Hash256::from_low_u64_be(u64::MAX - i), deposit_count: i, } } diff --git a/beacon_node/beacon_chain/src/validator_monitor.rs b/beacon_node/beacon_chain/src/validator_monitor.rs index a63940074b..f6cd5e857e 100644 --- a/beacon_node/beacon_chain/src/validator_monitor.rs +++ b/beacon_node/beacon_chain/src/validator_monitor.rs @@ -2068,7 +2068,7 @@ pub fn timestamp_now() -> Duration { } fn u64_to_i64(n: impl Into) -> i64 { - i64::try_from(n.into()).unwrap_or(i64::max_value()) + i64::try_from(n.into()).unwrap_or(i64::MAX) } /// Returns the delay between the start of `block.slot` and `seen_timestamp`. diff --git a/beacon_node/client/src/notifier.rs b/beacon_node/client/src/notifier.rs index 0a2f24748d..7c224671ff 100644 --- a/beacon_node/client/src/notifier.rs +++ b/beacon_node/client/src/notifier.rs @@ -721,10 +721,10 @@ fn eth1_logging(beacon_chain: &BeaconChain, log: &Logger } } -/// Returns the peer count, returning something helpful if it's `usize::max_value` (effectively a +/// Returns the peer count, returning something helpful if it's `usize::MAX` (effectively a /// `None` value). fn peer_count_pretty(peer_count: usize) -> String { - if peer_count == usize::max_value() { + if peer_count == usize::MAX { String::from("--") } else { format!("{}", peer_count) @@ -824,7 +824,7 @@ impl Speedo { /// Returns the average of the speeds between each observation. /// - /// Does not gracefully handle slots that are above `u32::max_value()`. + /// Does not gracefully handle slots that are above `u32::MAX`. pub fn slots_per_second(&self) -> Option { let speeds = self .0 diff --git a/beacon_node/eth1/src/service.rs b/beacon_node/eth1/src/service.rs index d68a8b6f28..9cc1da1382 100644 --- a/beacon_node/eth1/src/service.rs +++ b/beacon_node/eth1/src/service.rs @@ -853,7 +853,7 @@ impl Service { let max_log_requests_per_update = self .config() .max_log_requests_per_update - .unwrap_or_else(usize::max_value); + .unwrap_or(usize::MAX); let range = { match new_block_numbers { @@ -996,10 +996,7 @@ impl Service { ) -> Result { let client = self.client(); let block_cache_truncation = self.config().block_cache_truncation; - let max_blocks_per_update = self - .config() - .max_blocks_per_update - .unwrap_or_else(usize::max_value); + let max_blocks_per_update = self.config().max_blocks_per_update.unwrap_or(usize::MAX); let range = { match new_block_numbers { @@ -1025,7 +1022,7 @@ impl Service { let range_size = range.end() - range.start(); let max_size = block_cache_truncation .map(|n| n as u64) - .unwrap_or_else(u64::max_value); + .unwrap_or_else(|| u64::MAX); if range_size > max_size { // If the range of required blocks is larger than `max_size`, drop all // existing blocks and download `max_size` count of blocks. diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index 9c8a91909c..ef65119414 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -412,7 +412,7 @@ pub mod deposit_methods { .ok_or("Block number was not string")?, )?; - if number <= usize::max_value() as u64 { + if number <= usize::MAX as u64 { Ok(Block { hash, timestamp, diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index f8806bcd32..de8493ef60 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -2178,7 +2178,7 @@ fn verify_builder_bid( // Avoid logging values that we can't represent with our Prometheus library. let payload_value_gwei = bid.data.message.value() / 1_000_000_000; - if payload_value_gwei <= Uint256::from(i64::max_value()) { + if payload_value_gwei <= Uint256::from(i64::MAX) { metrics::set_gauge_vec( &metrics::EXECUTION_LAYER_PAYLOAD_BIDS, &[metrics::BUILDER], diff --git a/beacon_node/execution_layer/src/metrics.rs b/beacon_node/execution_layer/src/metrics.rs index 3ed99ca606..6aaada3dff 100644 --- a/beacon_node/execution_layer/src/metrics.rs +++ b/beacon_node/execution_layer/src/metrics.rs @@ -81,7 +81,7 @@ lazy_static::lazy_static! { ); pub static ref EXECUTION_LAYER_PAYLOAD_BIDS: Result = try_create_int_gauge_vec( "execution_layer_payload_bids", - "The gwei bid value of payloads received by local EEs or builders. Only shows values up to i64::max_value.", + "The gwei bid value of payloads received by local EEs or builders. Only shows values up to i64::MAX.", &["source"] ); } diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 2828b15a93..bf5dbe359e 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -2266,9 +2266,9 @@ impl ApiTester { vec![validator_count], vec![validator_count, 1], vec![validator_count, 1, 3], - vec![u64::max_value()], - vec![u64::max_value(), 1], - vec![u64::max_value(), 1, 3], + vec![u64::MAX], + vec![u64::MAX, 1], + vec![u64::MAX, 1, 3], ]; interesting.push((0..validator_count).collect()); diff --git a/beacon_node/lighthouse_network/src/discovery/mod.rs b/beacon_node/lighthouse_network/src/discovery/mod.rs index 5c937a1e0b..73f51c001a 100644 --- a/beacon_node/lighthouse_network/src/discovery/mod.rs +++ b/beacon_node/lighthouse_network/src/discovery/mod.rs @@ -561,8 +561,8 @@ impl Discovery { /// Updates the `eth2` field of our local ENR. pub fn update_eth2_enr(&mut self, enr_fork_id: EnrForkId) { // to avoid having a reference to the spec constant, for the logging we assume - // FAR_FUTURE_EPOCH is u64::max_value() - let next_fork_epoch_log = if enr_fork_id.next_fork_epoch == u64::max_value() { + // FAR_FUTURE_EPOCH is u64::MAX + let next_fork_epoch_log = if enr_fork_id.next_fork_epoch == u64::MAX { String::from("No other fork") } else { format!("{:?}", enr_fork_id.next_fork_epoch) diff --git a/beacon_node/network/src/network_beacon_processor/tests.rs b/beacon_node/network/src/network_beacon_processor/tests.rs index 06b12c14ae..a9b9f64a79 100644 --- a/beacon_node/network/src/network_beacon_processor/tests.rs +++ b/beacon_node/network/src/network_beacon_processor/tests.rs @@ -793,9 +793,7 @@ async fn aggregate_attestation_to_unknown_block(import_method: BlockImportMethod let mut rig = TestRig::new(SMALL_CHAIN).await; // Empty the op pool. - rig.chain - .op_pool - .prune_attestations(u64::max_value().into()); + rig.chain.op_pool.prune_attestations(u64::MAX.into()); assert_eq!(rig.chain.op_pool.num_attestations(), 0); // Send the attestation but not the block, and check that it was not imported. diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index 6e744ccf62..c1b4aeb3e4 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -602,7 +602,7 @@ impl OperationPool { }) }, |address_change| address_change.as_inner().clone(), - usize::max_value(), + usize::MAX, ); changes.shuffle(&mut thread_rng()); changes @@ -1343,7 +1343,7 @@ mod release_tests { // Set of indices covered by previous attestations in `best_attestations`. let mut seen_indices = BTreeSet::::new(); // Used for asserting that rewards are in decreasing order. - let mut prev_reward = u64::max_value(); + let mut prev_reward = u64::MAX; let mut reward_cache = RewardCache::default(); reward_cache.update(&state).unwrap(); diff --git a/common/deposit_contract/src/lib.rs b/common/deposit_contract/src/lib.rs index 785b952213..5b54a05396 100644 --- a/common/deposit_contract/src/lib.rs +++ b/common/deposit_contract/src/lib.rs @@ -96,7 +96,7 @@ mod tests { let mut deposit_data = DepositData { pubkey: keypair.pk.into(), withdrawal_credentials: Hash256::from_slice(&[42; 32]), - amount: u64::max_value(), + amount: u64::MAX, signature: Signature::empty().into(), }; deposit_data.signature = deposit_data.create_signature(&keypair.sk, spec); diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index ebb639819d..5764849975 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -285,17 +285,17 @@ impl ForkChoiceTestDefinition { } } -/// Gives a root that is not the zero hash (unless i is `usize::max_value)`. +/// Gives a root that is not the zero hash (unless i is `usize::MAX)`. fn get_root(i: u64) -> Hash256 { Hash256::from_low_u64_be(i + 1) } -/// Gives a hash that is not the zero hash (unless i is `usize::max_value)`. +/// Gives a hash that is not the zero hash (unless i is `usize::MAX)`. fn get_hash(i: u64) -> ExecutionBlockHash { ExecutionBlockHash::from_root(get_root(i)) } -/// Gives a checkpoint with a root that is not the zero hash (unless i is `usize::max_value)`. +/// Gives a checkpoint with a root that is not the zero hash (unless i is `usize::MAX)`. /// `Epoch` will always equal `i`. fn get_checkpoint(i: u64) -> Checkpoint { Checkpoint { diff --git a/consensus/proto_array/src/fork_choice_test_definition/votes.rs b/consensus/proto_array/src/fork_choice_test_definition/votes.rs index 58ac6af60b..01994fff9b 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/votes.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/votes.rs @@ -738,7 +738,7 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // Ensure that pruning below the prune threshold does not prune. ops.push(Operation::Prune { finalized_root: get_root(5), - prune_threshold: usize::max_value(), + prune_threshold: usize::MAX, expected_len: 11, }); diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index e1faba369f..4b7050df7d 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -995,7 +995,7 @@ mod test_compute_deltas { use super::*; use types::MainnetEthSpec; - /// Gives a hash that is not the zero hash (unless i is `usize::max_value)`. + /// Gives a hash that is not the zero hash (unless i is `usize::MAX)`. fn hash_from_index(i: usize) -> Hash256 { Hash256::from_low_u64_be(i as u64 + 1) } diff --git a/consensus/safe_arith/src/lib.rs b/consensus/safe_arith/src/lib.rs index c1dbff4c7c..aa397c0603 100644 --- a/consensus/safe_arith/src/lib.rs +++ b/consensus/safe_arith/src/lib.rs @@ -155,12 +155,12 @@ mod test { #[test] fn errors() { - assert!(u32::max_value().safe_add(1).is_err()); - assert!(u32::min_value().safe_sub(1).is_err()); - assert!(u32::max_value().safe_mul(2).is_err()); - assert!(u32::max_value().safe_div(0).is_err()); - assert!(u32::max_value().safe_rem(0).is_err()); - assert!(u32::max_value().safe_shl(32).is_err()); - assert!(u32::max_value().safe_shr(32).is_err()); + assert!(u32::MAX.safe_add(1).is_err()); + assert!(u32::MIN.safe_sub(1).is_err()); + assert!(u32::MAX.safe_mul(2).is_err()); + assert!(u32::MAX.safe_div(0).is_err()); + assert!(u32::MAX.safe_rem(0).is_err()); + assert!(u32::MAX.safe_shl(32).is_err()); + assert!(u32::MAX.safe_shr(32).is_err()); } } diff --git a/consensus/state_processing/src/per_epoch_processing/base/validator_statuses.rs b/consensus/state_processing/src/per_epoch_processing/base/validator_statuses.rs index 7e24405803..86a2f20985 100644 --- a/consensus/state_processing/src/per_epoch_processing/base/validator_statuses.rs +++ b/consensus/state_processing/src/per_epoch_processing/base/validator_statuses.rs @@ -30,7 +30,7 @@ impl Default for InclusionInfo { /// Defaults to `delay` at its maximum value and `proposer_index` at zero. fn default() -> Self { Self { - delay: u64::max_value(), + delay: u64::MAX, proposer_index: 0, } } diff --git a/consensus/swap_or_not_shuffle/src/compute_shuffled_index.rs b/consensus/swap_or_not_shuffle/src/compute_shuffled_index.rs index e71f3ca18e..5f25c517b0 100644 --- a/consensus/swap_or_not_shuffle/src/compute_shuffled_index.rs +++ b/consensus/swap_or_not_shuffle/src/compute_shuffled_index.rs @@ -17,7 +17,7 @@ use std::cmp::max; /// - `list_size == 0` /// - `index >= list_size` /// - `list_size > 2**24` -/// - `list_size > usize::max_value() / 2` +/// - `list_size > usize::MAX / 2` pub fn compute_shuffled_index( index: usize, list_size: usize, @@ -26,7 +26,7 @@ pub fn compute_shuffled_index( ) -> Option { if list_size == 0 || index >= list_size - || list_size > usize::max_value() / 2 + || list_size > usize::MAX / 2 || list_size > 2_usize.pow(24) { return None; @@ -140,7 +140,7 @@ mod tests { fn returns_none_for_too_large_list() { assert_eq!( None, - compute_shuffled_index(100, usize::max_value() / 2, &[42, 42], 90) + compute_shuffled_index(100, usize::MAX / 2, &[42, 42], 90) ); } } diff --git a/consensus/swap_or_not_shuffle/src/shuffle_list.rs b/consensus/swap_or_not_shuffle/src/shuffle_list.rs index 2b9a256554..b49a26cc37 100644 --- a/consensus/swap_or_not_shuffle/src/shuffle_list.rs +++ b/consensus/swap_or_not_shuffle/src/shuffle_list.rs @@ -75,7 +75,7 @@ impl Buf { /// Returns `None` under any of the following conditions: /// - `list_size == 0` /// - `list_size > 2**24` -/// - `list_size > usize::max_value() / 2` +/// - `list_size > usize::MAX / 2` pub fn shuffle_list( mut input: Vec, rounds: u8, @@ -84,10 +84,7 @@ pub fn shuffle_list( ) -> Option> { let list_size = input.len(); - if input.is_empty() - || list_size > usize::max_value() / 2 - || list_size > 2_usize.pow(24) - || rounds == 0 + if input.is_empty() || list_size > usize::MAX / 2 || list_size > 2_usize.pow(24) || rounds == 0 { return None; } diff --git a/consensus/types/benches/benches.rs b/consensus/types/benches/benches.rs index c6dda142b2..56c48e6cb1 100644 --- a/consensus/types/benches/benches.rs +++ b/consensus/types/benches/benches.rs @@ -36,8 +36,8 @@ fn get_state(validator_count: usize) -> BeaconState { slashed: false, activation_eligibility_epoch: Epoch::new(0), activation_epoch: Epoch::new(0), - exit_epoch: Epoch::from(u64::max_value()), - withdrawable_epoch: Epoch::from(u64::max_value()), + exit_epoch: Epoch::from(u64::MAX), + withdrawable_epoch: Epoch::from(u64::MAX), }) .collect(), ) diff --git a/consensus/types/src/beacon_state/committee_cache.rs b/consensus/types/src/beacon_state/committee_cache.rs index 7913df8e00..209659ea88 100644 --- a/consensus/types/src/beacon_state/committee_cache.rs +++ b/consensus/types/src/beacon_state/committee_cache.rs @@ -86,7 +86,7 @@ impl CommitteeCache { } // The use of `NonZeroUsize` reduces the maximum number of possible validators by one. - if state.validators().len() == usize::max_value() { + if state.validators().len() == usize::MAX { return Err(Error::TooManyValidators); } diff --git a/consensus/types/src/slot_epoch.rs b/consensus/types/src/slot_epoch.rs index 79d0064911..8c8f2d073d 100644 --- a/consensus/types/src/slot_epoch.rs +++ b/consensus/types/src/slot_epoch.rs @@ -70,7 +70,7 @@ impl Slot { } pub fn max_value() -> Slot { - Slot(u64::max_value()) + Slot(u64::MAX) } } @@ -80,7 +80,7 @@ impl Epoch { } pub fn max_value() -> Epoch { - Epoch(u64::max_value()) + Epoch(u64::MAX) } /// The first slot in the epoch. @@ -176,10 +176,10 @@ mod epoch_tests { let slots_per_epoch = 32; // The last epoch which can be represented by u64. - let epoch = Epoch::new(u64::max_value() / slots_per_epoch); + let epoch = Epoch::new(u64::MAX / slots_per_epoch); // A slot number on the epoch should be equal to u64::max_value. - assert_eq!(epoch.end_slot(slots_per_epoch), Slot::new(u64::max_value())); + assert_eq!(epoch.end_slot(slots_per_epoch), Slot::new(u64::MAX)); } #[test] diff --git a/consensus/types/src/slot_epoch_macros.rs b/consensus/types/src/slot_epoch_macros.rs index fafc455ef8..00e1af3918 100644 --- a/consensus/types/src/slot_epoch_macros.rs +++ b/consensus/types/src/slot_epoch_macros.rs @@ -352,7 +352,7 @@ macro_rules! new_tests { fn new() { assert_eq!($type(0), $type::new(0)); assert_eq!($type(3), $type::new(3)); - assert_eq!($type(u64::max_value()), $type::new(u64::max_value())); + assert_eq!($type(u64::MAX), $type::new(u64::MAX)); } }; } @@ -368,17 +368,17 @@ macro_rules! from_into_tests { let x: $other = $type(3).into(); assert_eq!(x, 3); - let x: $other = $type(u64::max_value()).into(); + let x: $other = $type(u64::MAX).into(); // Note: this will fail on 32 bit systems. This is expected as we don't have a proper // 32-bit system strategy in place. - assert_eq!(x, $other::max_value()); + assert_eq!(x, $other::MAX); } #[test] fn from() { assert_eq!($type(0), $type::from(0_u64)); assert_eq!($type(3), $type::from(3_u64)); - assert_eq!($type(u64::max_value()), $type::from($other::max_value())); + assert_eq!($type(u64::MAX), $type::from($other::MAX)); } }; } @@ -396,8 +396,8 @@ macro_rules! math_between_tests { assert_partial_ord(1, Ordering::Less, 2); assert_partial_ord(2, Ordering::Greater, 1); - assert_partial_ord(0, Ordering::Less, u64::max_value()); - assert_partial_ord(u64::max_value(), Ordering::Greater, 0); + assert_partial_ord(0, Ordering::Less, u64::MAX); + assert_partial_ord(u64::MAX, Ordering::Greater, 0); } #[test] @@ -412,9 +412,9 @@ macro_rules! math_between_tests { assert_partial_eq(1, 0, false); assert_partial_eq(1, 1, true); - assert_partial_eq(u64::max_value(), u64::max_value(), true); - assert_partial_eq(0, u64::max_value(), false); - assert_partial_eq(u64::max_value(), 0, false); + assert_partial_eq(u64::MAX, u64::MAX, true); + assert_partial_eq(0, u64::MAX, false); + assert_partial_eq(u64::MAX, 0, false); } #[test] @@ -436,8 +436,8 @@ macro_rules! math_between_tests { assert_add(7, 7, 14); // Addition should be saturating. - assert_add(u64::max_value(), 1, u64::max_value()); - assert_add(u64::max_value(), u64::max_value(), u64::max_value()); + assert_add(u64::MAX, 1, u64::MAX); + assert_add(u64::MAX, u64::MAX, u64::MAX); } #[test] @@ -455,8 +455,8 @@ macro_rules! math_between_tests { assert_sub(1, 0, 1); assert_sub(2, 1, 1); assert_sub(14, 7, 7); - assert_sub(u64::max_value(), 1, u64::max_value() - 1); - assert_sub(u64::max_value(), u64::max_value(), 0); + assert_sub(u64::MAX, 1, u64::MAX - 1); + assert_sub(u64::MAX, u64::MAX, 0); // Subtraction should be saturating assert_sub(0, 1, 0); @@ -480,7 +480,7 @@ macro_rules! math_between_tests { assert_mul(0, 2, 0); // Multiplication should be saturating. - assert_mul(u64::max_value(), 2, u64::max_value()); + assert_mul(u64::MAX, 2, u64::MAX); } #[test] @@ -499,7 +499,7 @@ macro_rules! math_between_tests { assert_div(2, 2, 1); assert_div(100, 50, 2); assert_div(128, 2, 64); - assert_div(u64::max_value(), 2, 2_u64.pow(63) - 1); + assert_div(u64::MAX, 2, 2_u64.pow(63) - 1); } #[test] @@ -544,8 +544,8 @@ macro_rules! math_tests { assert_saturating_sub(1, 0, 1); assert_saturating_sub(2, 1, 1); assert_saturating_sub(14, 7, 7); - assert_saturating_sub(u64::max_value(), 1, u64::max_value() - 1); - assert_saturating_sub(u64::max_value(), u64::max_value(), 0); + assert_saturating_sub(u64::MAX, 1, u64::MAX - 1); + assert_saturating_sub(u64::MAX, u64::MAX, 0); // Subtraction should be saturating assert_saturating_sub(0, 1, 0); @@ -565,8 +565,8 @@ macro_rules! math_tests { assert_saturating_add(7, 7, 14); // Addition should be saturating. - assert_saturating_add(u64::max_value(), 1, u64::max_value()); - assert_saturating_add(u64::max_value(), u64::max_value(), u64::max_value()); + assert_saturating_add(u64::MAX, 1, u64::MAX); + assert_saturating_add(u64::MAX, u64::MAX, u64::MAX); } #[test] @@ -581,11 +581,11 @@ macro_rules! math_tests { assert_checked_div(2, 2, Some(1)); assert_checked_div(100, 50, Some(2)); assert_checked_div(128, 2, Some(64)); - assert_checked_div(u64::max_value(), 2, Some(2_u64.pow(63) - 1)); + assert_checked_div(u64::MAX, 2, Some(2_u64.pow(63) - 1)); assert_checked_div(2, 0, None); assert_checked_div(0, 0, None); - assert_checked_div(u64::max_value(), 0, None); + assert_checked_div(u64::MAX, 0, None); } #[test] @@ -607,7 +607,7 @@ macro_rules! math_tests { assert_is_power_of_two(4, true); assert_is_power_of_two(2_u64.pow(4), true); - assert_is_power_of_two(u64::max_value(), false); + assert_is_power_of_two(u64::MAX, false); } #[test] @@ -619,8 +619,8 @@ macro_rules! math_tests { assert_ord(1, Ordering::Less, 2); assert_ord(2, Ordering::Greater, 1); - assert_ord(0, Ordering::Less, u64::max_value()); - assert_ord(u64::max_value(), Ordering::Greater, 0); + assert_ord(0, Ordering::Less, u64::MAX); + assert_ord(u64::MAX, Ordering::Greater, 0); } }; } @@ -647,8 +647,8 @@ macro_rules! all_tests { let x = $type(3).as_u64(); assert_eq!(x, 3); - let x = $type(u64::max_value()).as_u64(); - assert_eq!(x, u64::max_value()); + let x = $type(u64::MAX).as_u64(); + assert_eq!(x, u64::MAX); } } @@ -665,8 +665,8 @@ macro_rules! all_tests { let x = $type(3).as_usize(); assert_eq!(x, 3); - let x = $type(u64::max_value()).as_usize(); - assert_eq!(x, usize::max_value()); + let x = $type(u64::MAX).as_usize(); + assert_eq!(x, usize::MAX); } } }; diff --git a/consensus/types/src/validator.rs b/consensus/types/src/validator.rs index 9e26d1eeca..ba99a0a68d 100644 --- a/consensus/types/src/validator.rs +++ b/consensus/types/src/validator.rs @@ -254,12 +254,12 @@ impl Default for Validator { Self { pubkey: PublicKeyBytes::empty(), withdrawal_credentials: Hash256::default(), - activation_eligibility_epoch: Epoch::from(std::u64::MAX), - activation_epoch: Epoch::from(std::u64::MAX), - exit_epoch: Epoch::from(std::u64::MAX), - withdrawable_epoch: Epoch::from(std::u64::MAX), + activation_eligibility_epoch: Epoch::from(u64::MAX), + activation_epoch: Epoch::from(u64::MAX), + exit_epoch: Epoch::from(u64::MAX), + withdrawable_epoch: Epoch::from(u64::MAX), slashed: false, - effective_balance: std::u64::MAX, + effective_balance: u64::MAX, } } } diff --git a/crypto/eth2_key_derivation/tests/tests.rs b/crypto/eth2_key_derivation/tests/tests.rs index b18a7b0e26..a344b7b3fc 100644 --- a/crypto/eth2_key_derivation/tests/tests.rs +++ b/crypto/eth2_key_derivation/tests/tests.rs @@ -22,7 +22,7 @@ fn deterministic() { fn children_deterministic() { let master = DerivedKey::from_seed(&[42]).unwrap(); assert_eq!( - master.child(u32::max_value()).secret(), - master.child(u32::max_value()).secret(), + master.child(u32::MAX).secret(), + master.child(u32::MAX).secret(), ) } diff --git a/crypto/eth2_wallet/src/wallet.rs b/crypto/eth2_wallet/src/wallet.rs index 7a7d65f654..8bf7091216 100644 --- a/crypto/eth2_wallet/src/wallet.rs +++ b/crypto/eth2_wallet/src/wallet.rs @@ -179,7 +179,7 @@ impl Wallet { /// /// - If `wallet_password` is unable to decrypt `self`. /// - If `keystore_password.is_empty()`. - /// - If `self.nextaccount == u32::max_value()`. + /// - If `self.nextaccount == u32::MAX`. pub fn next_validator( &mut self, wallet_password: &[u8], diff --git a/scripts/local_testnet/start_local_testnet.sh b/scripts/local_testnet/start_local_testnet.sh index e0172e6b28..4b03b1e010 100755 --- a/scripts/local_testnet/start_local_testnet.sh +++ b/scripts/local_testnet/start_local_testnet.sh @@ -78,6 +78,6 @@ fi # Stop local testnet kurtosis enclave rm -f $ENCLAVE_NAME 2>/dev/null || true -kurtosis run --enclave $ENCLAVE_NAME github.com/kurtosis-tech/ethereum-package --args-file $NETWORK_PARAMS_FILE +kurtosis run --enclave $ENCLAVE_NAME github.com/ethpandaops/ethereum-package --args-file $NETWORK_PARAMS_FILE echo "Started!" diff --git a/scripts/tests/network_params.yaml b/scripts/tests/network_params.yaml index 1725203138..21114df0e8 100644 --- a/scripts/tests/network_params.yaml +++ b/scripts/tests/network_params.yaml @@ -1,4 +1,4 @@ -# Full configuration reference [here](https://github.com/kurtosis-tech/ethereum-package?tab=readme-ov-file#configuration). +# Full configuration reference [here](https://github.com/ethpandaops/ethereum-package?tab=readme-ov-file#configuration). participants: - el_type: geth el_image: ethereum/client-go:latest diff --git a/validator_client/src/doppelganger_service.rs b/validator_client/src/doppelganger_service.rs index 442a950ddc..9f93795e29 100644 --- a/validator_client/src/doppelganger_service.rs +++ b/validator_client/src/doppelganger_service.rs @@ -1115,7 +1115,7 @@ mod test { ) // All validators should still be disabled. .assert_all_disabled() - // The states of all validators should be jammed with `u64::max_value()`. + // The states of all validators should be jammed with `u64:MAX`. .assert_all_states(&DoppelgangerState { next_check_epoch: starting_epoch + 1, remaining_epochs: u64::MAX, @@ -1347,7 +1347,7 @@ mod test { ) .assert_all_states(&DoppelgangerState { next_check_epoch: initial_epoch + 1, - remaining_epochs: u64::max_value(), + remaining_epochs: u64::MAX, }); } diff --git a/watch/src/blockprint/database.rs b/watch/src/blockprint/database.rs index afa35c81b6..f0bc3f8ac8 100644 --- a/watch/src/blockprint/database.rs +++ b/watch/src/blockprint/database.rs @@ -33,6 +33,7 @@ pub struct WatchBlockprint { } #[derive(Debug, QueryableByName, diesel::FromSqlRow)] +#[allow(dead_code)] pub struct WatchValidatorBlockprint { #[diesel(sql_type = Integer)] pub proposer_index: i32, diff --git a/watch/src/blockprint/mod.rs b/watch/src/blockprint/mod.rs index 532776f425..319090c656 100644 --- a/watch/src/blockprint/mod.rs +++ b/watch/src/blockprint/mod.rs @@ -24,6 +24,7 @@ pub use server::blockprint_routes; const TIMEOUT: Duration = Duration::from_secs(50); #[derive(Debug)] +#[allow(dead_code)] pub enum Error { Reqwest(reqwest::Error), Url(url::ParseError), diff --git a/watch/src/server/error.rs b/watch/src/server/error.rs index 0db3df2a0d..e2c8f0f42a 100644 --- a/watch/src/server/error.rs +++ b/watch/src/server/error.rs @@ -6,6 +6,7 @@ use serde_json::json; use std::io::Error as IoError; #[derive(Debug)] +#[allow(dead_code)] pub enum Error { Axum(AxumError), Hyper(HyperError), diff --git a/watch/src/updater/error.rs b/watch/src/updater/error.rs index 74091c8f21..13c83bcf01 100644 --- a/watch/src/updater/error.rs +++ b/watch/src/updater/error.rs @@ -5,6 +5,7 @@ use eth2::{Error as Eth2Error, SensitiveError}; use std::fmt; #[derive(Debug)] +#[allow(dead_code)] pub enum Error { BeaconChain(BeaconChainError), Eth2(Eth2Error), diff --git a/watch/src/updater/handler.rs b/watch/src/updater/handler.rs index a0bfc0b9a4..3ee32560ad 100644 --- a/watch/src/updater/handler.rs +++ b/watch/src/updater/handler.rs @@ -9,6 +9,7 @@ use eth2::{ }; use log::{debug, error, info, warn}; use std::collections::HashSet; +use std::marker::PhantomData; use types::{BeaconBlockHeader, EthSpec, Hash256, SignedBeaconBlock, Slot}; use crate::updater::{get_beacon_block, get_header, get_validators}; @@ -47,7 +48,7 @@ pub struct UpdateHandler { pub blockprint: Option, pub config: Config, pub slots_per_epoch: u64, - pub spec: WatchSpec, + pub _phantom: PhantomData, } impl UpdateHandler { @@ -84,7 +85,7 @@ impl UpdateHandler { blockprint, config: config.updater, slots_per_epoch: spec.slots_per_epoch(), - spec, + _phantom: PhantomData, }) } From 35e07eb0a917d0b067e947b5421cdb9d6f15b2da Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 13 Jun 2024 19:27:36 -0700 Subject: [PATCH 02/53] Fix slasher tests (#5906) * Fix electra tests * Add electra attestations to double vote tests --- slasher/src/lib.rs | 71 +++++---- slasher/src/test_utils.rs | 13 +- slasher/tests/attester_slashings.rs | 228 ++++++++++++++++++++-------- 3 files changed, 201 insertions(+), 111 deletions(-) diff --git a/slasher/src/lib.rs b/slasher/src/lib.rs index 339ca1f7d3..0c097ac77f 100644 --- a/slasher/src/lib.rs +++ b/slasher/src/lib.rs @@ -60,49 +60,46 @@ impl AttesterSlashingStatus { Ok(match self { NotSlashable => None, AlreadyDoubleVoted => None, - DoubleVote(existing) | SurroundedByExisting(existing) => match *existing { - IndexedAttestation::Base(existing_att) => { - Some(AttesterSlashing::Base(AttesterSlashingBase { - attestation_1: existing_att, - attestation_2: new_attestation - .as_base() - .map_err(|e| format!("{e:?}"))? - .clone(), - })) - } - IndexedAttestation::Electra(existing_att) => { - Some(AttesterSlashing::Electra(AttesterSlashingElectra { - attestation_1: existing_att, - // A double vote should never convert, a surround vote where the surrounding - // vote is electra may convert. + DoubleVote(existing) | SurroundedByExisting(existing) => { + match (&*existing, new_attestation) { + (IndexedAttestation::Base(existing_att), IndexedAttestation::Base(new)) => { + Some(AttesterSlashing::Base(AttesterSlashingBase { + attestation_1: existing_att.clone(), + attestation_2: new.clone(), + })) + } + // A slashing involving an electra attestation type must return an `AttesterSlashingElectra` type + (_, _) => Some(AttesterSlashing::Electra(AttesterSlashingElectra { + attestation_1: existing + .clone() + .to_electra() + .map_err(|e| format!("{e:?}"))?, attestation_2: new_attestation .clone() .to_electra() .map_err(|e| format!("{e:?}"))?, - })) - } - }, - SurroundsExisting(existing) => { - match new_attestation { - IndexedAttestation::Base(new_attestation) => { - Some(AttesterSlashing::Base(AttesterSlashingBase { - attestation_1: existing - .as_base() - .map_err(|e| format!("{e:?}"))? - .clone(), - attestation_2: new_attestation.clone(), - })) - } - IndexedAttestation::Electra(new_attestation) => { - Some(AttesterSlashing::Electra(AttesterSlashingElectra { - attestation_1: existing.to_electra().map_err(|e| format!("{e:?}"))?, - // A double vote should never convert, a surround vote where the surrounding - // vote is electra may convert. - attestation_2: new_attestation.clone(), - })) - } + })), } } + SurroundsExisting(existing) => match (&*existing, new_attestation) { + (IndexedAttestation::Base(existing_att), IndexedAttestation::Base(new)) => { + Some(AttesterSlashing::Base(AttesterSlashingBase { + attestation_1: new.clone(), + attestation_2: existing_att.clone(), + })) + } + // A slashing involving an electra attestation type must return an `AttesterSlashingElectra` type + (_, _) => Some(AttesterSlashing::Electra(AttesterSlashingElectra { + attestation_1: new_attestation + .clone() + .to_electra() + .map_err(|e| format!("{e:?}"))?, + attestation_2: existing + .clone() + .to_electra() + .map_err(|e| format!("{e:?}"))?, + })), + }, }) } } diff --git a/slasher/src/test_utils.rs b/slasher/src/test_utils.rs index 8bbf042c2d..f22a680bb7 100644 --- a/slasher/src/test_utils.rs +++ b/slasher/src/test_utils.rs @@ -63,7 +63,6 @@ pub fn att_slashing( attestation_1: &IndexedAttestation, attestation_2: &IndexedAttestation, ) -> AttesterSlashing { - // TODO(electra): fix this one we superstruct IndexedAttestation (return the correct type) match (attestation_1, attestation_2) { (IndexedAttestation::Base(att1), IndexedAttestation::Base(att2)) => { AttesterSlashing::Base(AttesterSlashingBase { @@ -71,13 +70,11 @@ pub fn att_slashing( attestation_2: att2.clone(), }) } - (IndexedAttestation::Electra(att1), IndexedAttestation::Electra(att2)) => { - AttesterSlashing::Electra(AttesterSlashingElectra { - attestation_1: att1.clone(), - attestation_2: att2.clone(), - }) - } - _ => panic!("attestations must be of the same type"), + // A slashing involving an electra attestation type must return an electra AttesterSlashing type + (_, _) => AttesterSlashing::Electra(AttesterSlashingElectra { + attestation_1: attestation_1.clone().to_electra().unwrap(), + attestation_2: attestation_2.clone().to_electra().unwrap(), + }), } } diff --git a/slasher/tests/attester_slashings.rs b/slasher/tests/attester_slashings.rs index 40d9fa511c..b74e491e4b 100644 --- a/slasher/tests/attester_slashings.rs +++ b/slasher/tests/attester_slashings.rs @@ -5,7 +5,9 @@ use maplit::hashset; use rayon::prelude::*; use slasher::{ config::DEFAULT_CHUNK_SIZE, - test_utils::{att_slashing, indexed_att, slashed_validators_from_slashings, E}, + test_utils::{ + att_slashing, indexed_att, indexed_att_electra, slashed_validators_from_slashings, E, + }, Config, Slasher, }; use std::collections::HashSet; @@ -15,23 +17,35 @@ use types::{AttesterSlashing, Epoch, IndexedAttestation}; #[test] fn double_vote_single_val() { let v = vec![99]; - let att1 = indexed_att(&v, 0, 1, 0); - let att2 = indexed_att(&v, 0, 1, 1); - let slashings = hashset![att_slashing(&att1, &att2)]; - let attestations = vec![att1, att2]; - slasher_test_indiv(&attestations, &slashings, 1); - slasher_test_indiv(&attestations, &slashings, 1000); + for (att1, att2) in [ + (indexed_att(&v, 0, 1, 0), indexed_att(&v, 0, 1, 1)), + ( + indexed_att_electra(&v, 0, 1, 0), + indexed_att_electra(&v, 0, 1, 1), + ), + ] { + let slashings = hashset![att_slashing(&att1, &att2)]; + let attestations = vec![att1, att2]; + slasher_test_indiv(&attestations, &slashings, 1); + slasher_test_indiv(&attestations, &slashings, 1000); + } } #[test] fn double_vote_multi_vals() { let v = vec![0, 1, 2]; - let att1 = indexed_att(&v, 0, 1, 0); - let att2 = indexed_att(&v, 0, 1, 1); - let slashings = hashset![att_slashing(&att1, &att2)]; - let attestations = vec![att1, att2]; - slasher_test_indiv(&attestations, &slashings, 1); - slasher_test_indiv(&attestations, &slashings, 1000); + for (att1, att2) in [ + (indexed_att(&v, 0, 1, 0), indexed_att(&v, 0, 1, 1)), + ( + indexed_att_electra(&v, 0, 1, 0), + indexed_att_electra(&v, 0, 1, 1), + ), + ] { + let slashings = hashset![att_slashing(&att1, &att2)]; + let attestations = vec![att1, att2]; + slasher_test_indiv(&attestations, &slashings, 1); + slasher_test_indiv(&attestations, &slashings, 1000); + } } // A subset of validators double vote. @@ -39,12 +53,18 @@ fn double_vote_multi_vals() { fn double_vote_some_vals() { let v1 = vec![0, 1, 2, 3, 4, 5, 6]; let v2 = vec![0, 2, 4, 6]; - let att1 = indexed_att(v1, 0, 1, 0); - let att2 = indexed_att(v2, 0, 1, 1); - let slashings = hashset![att_slashing(&att1, &att2)]; - let attestations = vec![att1, att2]; - slasher_test_indiv(&attestations, &slashings, 1); - slasher_test_indiv(&attestations, &slashings, 1000); + for (att1, att2) in [ + (indexed_att(&v1, 0, 1, 0), indexed_att(&v2, 0, 1, 1)), + ( + indexed_att_electra(&v1, 0, 1, 0), + indexed_att_electra(&v2, 0, 1, 1), + ), + ] { + let slashings = hashset![att_slashing(&att1, &att2)]; + let attestations = vec![att1, att2]; + slasher_test_indiv(&attestations, &slashings, 1); + slasher_test_indiv(&attestations, &slashings, 1000); + } } // A subset of validators double vote, others vote twice for the same thing. @@ -53,13 +73,23 @@ fn double_vote_some_vals_repeat() { let v1 = vec![0, 1, 2, 3, 4, 5, 6]; let v2 = vec![0, 2, 4, 6]; let v3 = vec![1, 3, 5]; - let att1 = indexed_att(v1, 0, 1, 0); - let att2 = indexed_att(v2, 0, 1, 1); - let att3 = indexed_att(v3, 0, 1, 0); - let slashings = hashset![att_slashing(&att1, &att2)]; - let attestations = vec![att1, att2, att3]; - slasher_test_indiv(&attestations, &slashings, 1); - slasher_test_indiv(&attestations, &slashings, 1000); + for (att1, att2, att3) in [ + ( + indexed_att(&v1, 0, 1, 0), + indexed_att(&v2, 0, 1, 1), + indexed_att(&v3, 0, 1, 0), + ), + ( + indexed_att_electra(&v1, 0, 1, 0), + indexed_att_electra(&v2, 0, 1, 1), + indexed_att_electra(&v3, 0, 1, 0), + ), + ] { + let slashings = hashset![att_slashing(&att1, &att2)]; + let attestations = vec![att1, att2, att3]; + slasher_test_indiv(&attestations, &slashings, 1); + slasher_test_indiv(&attestations, &slashings, 1000); + } } // Nobody double votes, nobody gets slashed. @@ -67,11 +97,17 @@ fn double_vote_some_vals_repeat() { fn no_double_vote_same_target() { let v1 = vec![0, 1, 2, 3, 4, 5, 6]; let v2 = vec![0, 1, 2, 3, 4, 5, 7, 8]; - let att1 = indexed_att(v1, 0, 1, 0); - let att2 = indexed_att(v2, 0, 1, 0); - let attestations = vec![att1, att2]; - slasher_test_indiv(&attestations, &hashset! {}, 1); - slasher_test_indiv(&attestations, &hashset! {}, 1000); + for (att1, att2) in [ + (indexed_att(&v1, 0, 1, 0), indexed_att(&v2, 0, 1, 0)), + ( + indexed_att_electra(&v1, 0, 1, 0), + indexed_att_electra(&v2, 0, 1, 0), + ), + ] { + let attestations = vec![att1, att2]; + slasher_test_indiv(&attestations, &hashset! {}, 1); + slasher_test_indiv(&attestations, &hashset! {}, 1000); + } } // Two groups votes for different things, no slashings. @@ -79,73 +115,133 @@ fn no_double_vote_same_target() { fn no_double_vote_distinct_vals() { let v1 = vec![0, 1, 2, 3]; let v2 = vec![4, 5, 6, 7]; - let att1 = indexed_att(v1, 0, 1, 0); - let att2 = indexed_att(v2, 0, 1, 1); - let attestations = vec![att1, att2]; - slasher_test_indiv(&attestations, &hashset! {}, 1); - slasher_test_indiv(&attestations, &hashset! {}, 1000); + for (att1, att2) in [ + (indexed_att(&v1, 0, 1, 0), indexed_att(&v2, 0, 1, 0)), + ( + indexed_att_electra(&v1, 0, 1, 0), + indexed_att_electra(&v2, 0, 1, 1), + ), + ] { + let attestations = vec![att1, att2]; + slasher_test_indiv(&attestations, &hashset! {}, 1); + slasher_test_indiv(&attestations, &hashset! {}, 1000); + } } #[test] fn no_double_vote_repeated() { let v = vec![0, 1, 2, 3, 4]; - let att1 = indexed_att(v, 0, 1, 0); - let att2 = att1.clone(); - let attestations = vec![att1, att2]; - slasher_test_indiv(&attestations, &hashset! {}, 1); - slasher_test_batch(&attestations, &hashset! {}, 1); - parallel_slasher_test(&attestations, hashset! {}, 1); + for att1 in [indexed_att(&v, 0, 1, 0), indexed_att_electra(&v, 0, 1, 0)] { + let att2 = att1.clone(); + let attestations = vec![att1, att2]; + slasher_test_indiv(&attestations, &hashset! {}, 1); + slasher_test_batch(&attestations, &hashset! {}, 1); + parallel_slasher_test(&attestations, hashset! {}, 1); + } } #[test] fn surrounds_existing_single_val_single_chunk() { let v = vec![0]; - let att1 = indexed_att(&v, 1, 2, 0); - let att2 = indexed_att(&v, 0, 3, 0); - let slashings = hashset![att_slashing(&att2, &att1)]; - slasher_test_indiv(&[att1, att2], &slashings, 3); + for (att1, att2) in [ + (indexed_att(&v, 1, 2, 0), indexed_att(&v, 0, 3, 0)), + (indexed_att(&v, 1, 2, 0), indexed_att_electra(&v, 0, 3, 0)), + ( + indexed_att_electra(&v, 1, 2, 0), + indexed_att_electra(&v, 0, 3, 0), + ), + ] { + let slashings = hashset![att_slashing(&att2, &att1)]; + slasher_test_indiv(&[att1, att2], &slashings, 3); + } } #[test] fn surrounds_existing_multi_vals_single_chunk() { let validators = vec![0, 16, 1024, 300_000, 300_001]; - let att1 = indexed_att(validators.clone(), 1, 2, 0); - let att2 = indexed_att(validators, 0, 3, 0); - let slashings = hashset![att_slashing(&att2, &att1)]; - slasher_test_indiv(&[att1, att2], &slashings, 3); + for (att1, att2) in [ + ( + indexed_att(&validators, 1, 2, 0), + indexed_att(&validators, 0, 3, 0), + ), + ( + indexed_att(&validators, 1, 2, 0), + indexed_att_electra(&validators, 0, 3, 0), + ), + ( + indexed_att_electra(&validators, 1, 2, 0), + indexed_att_electra(&validators, 0, 3, 0), + ), + ] { + let slashings = hashset![att_slashing(&att2, &att1)]; + slasher_test_indiv(&[att1, att2], &slashings, 3); + } } #[test] fn surrounds_existing_many_chunks() { let v = vec![0]; let chunk_size = DEFAULT_CHUNK_SIZE as u64; - let att1 = indexed_att(&v, 3 * chunk_size, 3 * chunk_size + 1, 0); - let att2 = indexed_att(&v, 0, 3 * chunk_size + 2, 0); - let slashings = hashset![att_slashing(&att2, &att1)]; - let attestations = vec![att1, att2]; - slasher_test_indiv(&attestations, &slashings, 4 * chunk_size); + for (att1, att2) in [ + ( + indexed_att(&v, 3 * chunk_size, 3 * chunk_size + 1, 0), + indexed_att(&v, 0, 3 * chunk_size + 2, 0), + ), + ( + indexed_att(&v, 3 * chunk_size, 3 * chunk_size + 1, 0), + indexed_att_electra(&v, 0, 3 * chunk_size + 2, 0), + ), + ( + indexed_att_electra(&v, 3 * chunk_size, 3 * chunk_size + 1, 0), + indexed_att_electra(&v, 0, 3 * chunk_size + 2, 0), + ), + ] { + let slashings = hashset![att_slashing(&att2, &att1)]; + let attestations = vec![att1, att2]; + slasher_test_indiv(&attestations, &slashings, 4 * chunk_size); + } } #[test] fn surrounded_by_single_val_single_chunk() { let v = vec![0]; - let att1 = indexed_att(&v, 0, 15, 0); - let att2 = indexed_att(&v, 1, 14, 0); - let slashings = hashset![att_slashing(&att1, &att2)]; - let attestations = vec![att1, att2]; - slasher_test_indiv(&attestations, &slashings, 15); + for (att1, att2) in [ + (indexed_att(&v, 0, 15, 0), indexed_att(&v, 1, 14, 0)), + (indexed_att(&v, 0, 15, 0), indexed_att_electra(&v, 1, 14, 0)), + ( + indexed_att_electra(&v, 0, 15, 0), + indexed_att_electra(&v, 1, 14, 0), + ), + ] { + let slashings = hashset![att_slashing(&att1, &att2)]; + let attestations = vec![att1, att2]; + slasher_test_indiv(&attestations, &slashings, 15); + } } #[test] fn surrounded_by_single_val_multi_chunk() { let v = vec![0]; let chunk_size = DEFAULT_CHUNK_SIZE as u64; - let att1 = indexed_att(&v, 0, 3 * chunk_size, 0); - let att2 = indexed_att(&v, chunk_size, chunk_size + 1, 0); - let slashings = hashset![att_slashing(&att1, &att2)]; - let attestations = vec![att1, att2]; - slasher_test_indiv(&attestations, &slashings, 3 * chunk_size); - slasher_test_indiv(&attestations, &slashings, 4 * chunk_size); + for (att1, att2) in [ + ( + indexed_att(&v, 0, 3 * chunk_size, 0), + indexed_att(&v, chunk_size, chunk_size + 1, 0), + ), + ( + indexed_att(&v, 0, 3 * chunk_size, 0), + indexed_att_electra(&v, chunk_size, chunk_size + 1, 0), + ), + ( + indexed_att_electra(&v, 0, 3 * chunk_size, 0), + indexed_att_electra(&v, chunk_size, chunk_size + 1, 0), + ), + ] { + let slashings = hashset![att_slashing(&att1, &att2)]; + let attestations = vec![att1, att2]; + slasher_test_indiv(&attestations, &slashings, 3 * chunk_size); + slasher_test_indiv(&attestations, &slashings, 4 * chunk_size); + } } // Process each attestation individually, and confirm that the slashings produced are as expected. From d7f3c9583e455ee5d586c6104dc8f217981ed887 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 14 Jun 2024 12:32:20 +1000 Subject: [PATCH 03/53] Update superstruct to 0.8 --- Cargo.lock | 3 ++- Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5c165cec89..3d6fddc035 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8041,7 +8041,8 @@ checksum = "81cdd64d312baedb58e21336b31bc043b77e01cc99033ce76ef539f78e965ebc" [[package]] name = "superstruct" version = "0.8.0" -source = "git+https://github.com/sigp/superstruct?rev=45eecbfb9708c9fe11dbb6a6a5bd8d618f02269e#45eecbfb9708c9fe11dbb6a6a5bd8d618f02269e" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bf0f31f730ad9e579364950e10d6172b4a9bd04b447edf5988b066a860cc340e" dependencies = [ "darling", "itertools", diff --git a/Cargo.toml b/Cargo.toml index 1059c74625..d67f6edf1d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -162,7 +162,7 @@ smallvec = "1.11.2" snap = "1" ssz_types = "0.6" strum = { version = "0.24", features = ["derive"] } -superstruct = { git = "https://github.com/sigp/superstruct", rev = "45eecbfb9708c9fe11dbb6a6a5bd8d618f02269e" } +superstruct = "0.8" syn = "1" sysinfo = "0.26" tempfile = "3" From c4f2284dbe00ade38e93b1bac35c235386759cb9 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 14 Jun 2024 12:50:18 +1000 Subject: [PATCH 04/53] Small cleanup in slasher tests --- slasher/src/test_utils.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/slasher/src/test_utils.rs b/slasher/src/test_utils.rs index f22a680bb7..ef623911bf 100644 --- a/slasher/src/test_utils.rs +++ b/slasher/src/test_utils.rs @@ -39,7 +39,6 @@ pub fn indexed_att( target_epoch: u64, target_root: u64, ) -> IndexedAttestation { - // TODO(electra) make fork-agnostic IndexedAttestation::Base(IndexedAttestationBase { attesting_indices: attesting_indices.as_ref().to_vec().into(), data: AttestationData { @@ -121,11 +120,9 @@ pub fn slashed_validators_from_attestations( continue; } - // TODO(electra) in a nested loop, copying to vecs feels bad - let attesting_indices_1 = att1.attesting_indices_to_vec(); - let attesting_indices_2 = att2.attesting_indices_to_vec(); - if att1.is_double_vote(att2) || att1.is_surround_vote(att2) { + let attesting_indices_1 = att1.attesting_indices_to_vec(); + let attesting_indices_2 = att2.attesting_indices_to_vec(); slashed_validators.extend(hashset_intersection( &attesting_indices_1, &attesting_indices_2, From 1eb8694a862fcb92fbce2309b783bbb04f3db742 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 17 Jun 2024 23:32:39 +1000 Subject: [PATCH 05/53] Remove some easy Electra TODOs (#5928) * Remove some easy Electra TODOs --- beacon_node/execution_layer/src/lib.rs | 37 ++----------------- beacon_node/store/src/partial_beacon_state.rs | 1 - .../types/src/execution_payload_header.rs | 1 - 3 files changed, 4 insertions(+), 35 deletions(-) diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index de8493ef60..3a744e3e71 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -1985,39 +1985,10 @@ impl ExecutionLayer { excess_blob_gas: deneb_block.excess_blob_gas, }) } - ExecutionBlockWithTransactions::Electra(electra_block) => { - let withdrawals = VariableList::new( - electra_block - .withdrawals - .into_iter() - .map(Into::into) - .collect(), - ) - .map_err(ApiError::DeserializeWithdrawals)?; - ExecutionPayload::Electra(ExecutionPayloadElectra { - parent_hash: electra_block.parent_hash, - fee_recipient: electra_block.fee_recipient, - state_root: electra_block.state_root, - receipts_root: electra_block.receipts_root, - logs_bloom: electra_block.logs_bloom, - prev_randao: electra_block.prev_randao, - block_number: electra_block.block_number, - gas_limit: electra_block.gas_limit, - gas_used: electra_block.gas_used, - timestamp: electra_block.timestamp, - extra_data: electra_block.extra_data, - base_fee_per_gas: electra_block.base_fee_per_gas, - block_hash: electra_block.block_hash, - transactions: convert_transactions(electra_block.transactions)?, - withdrawals, - blob_gas_used: electra_block.blob_gas_used, - excess_blob_gas: electra_block.excess_blob_gas, - // TODO(electra) - // deposit_receipts: electra_block.deposit_receipts, - // withdrawal_requests: electra_block.withdrawal_requests, - deposit_receipts: <_>::default(), - withdrawal_requests: <_>::default(), - }) + ExecutionBlockWithTransactions::Electra(_) => { + return Err(ApiError::UnsupportedForkVariant(format!( + "legacy payload construction for {fork} is not implemented" + ))); } }; diff --git a/beacon_node/store/src/partial_beacon_state.rs b/beacon_node/store/src/partial_beacon_state.rs index e56d0580ac..5e6054bc06 100644 --- a/beacon_node/store/src/partial_beacon_state.rs +++ b/beacon_node/store/src/partial_beacon_state.rs @@ -133,7 +133,6 @@ where #[superstruct(only(Electra))] pub earliest_consolidation_epoch: Epoch, - // TODO(electra) should these be optional? #[superstruct(only(Electra))] pub pending_balance_deposits: List, #[superstruct(only(Electra))] diff --git a/consensus/types/src/execution_payload_header.rs b/consensus/types/src/execution_payload_header.rs index 324d7b9747..98ed3d9f36 100644 --- a/consensus/types/src/execution_payload_header.rs +++ b/consensus/types/src/execution_payload_header.rs @@ -120,7 +120,6 @@ impl ExecutionPayloadHeader { #[allow(clippy::arithmetic_side_effects)] pub fn ssz_max_var_len_for_fork(fork_name: ForkName) -> usize { // Matching here in case variable fields are added in future forks. - // TODO(electra): review electra changes match fork_name { ForkName::Base | ForkName::Altair From 3ac3ddb2b7d4e4a824593db0a57fc6f9e39ec710 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 18 Jun 2024 00:23:02 +1000 Subject: [PATCH 06/53] Clean up Electra observed aggregates (#5929) * Use consistent key in observed_attestations * Remove unwraps from observed aggregates --- .../src/attestation_verification.rs | 15 ++-- beacon_node/beacon_chain/src/lib.rs | 2 +- .../beacon_chain/src/observed_aggregates.rs | 74 +++++++++++++------ .../tests/attestation_verification.rs | 6 +- 4 files changed, 61 insertions(+), 36 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index d82685c48c..f2e3565ae6 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -35,8 +35,10 @@ mod batch; use crate::{ - beacon_chain::VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT, metrics, - observed_aggregates::ObserveOutcome, observed_attesters::Error as ObservedAttestersError, + beacon_chain::VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT, + metrics, + observed_aggregates::{ObserveOutcome, ObservedAttestationKey}, + observed_attesters::Error as ObservedAttestersError, BeaconChain, BeaconChainError, BeaconChainTypes, }; use bls::verify_signature_sets; @@ -58,9 +60,8 @@ use state_processing::{ use std::borrow::Cow; use strum::AsRefStr; use tree_hash::TreeHash; -use tree_hash_derive::TreeHash; use types::{ - Attestation, AttestationData, AttestationRef, BeaconCommittee, BeaconStateError, + Attestation, AttestationRef, BeaconCommittee, BeaconStateError, BeaconStateError::NoCommitteeFound, ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, SelectionProof, SignedAggregateAndProof, Slot, SubnetId, }; @@ -309,12 +310,6 @@ struct IndexedAggregatedAttestation<'a, T: BeaconChainTypes> { observed_attestation_key_root: Hash256, } -#[derive(TreeHash)] -pub struct ObservedAttestationKey { - pub committee_index: u64, - pub attestation_data: AttestationData, -} - /// Wraps a `Attestation` that has been verified up until the point that an `IndexedAttestation` can /// be derived. /// diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index f419429e09..466ab0b67e 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -39,7 +39,7 @@ mod light_client_server_cache; pub mod metrics; pub mod migrate; mod naive_aggregation_pool; -mod observed_aggregates; +pub mod observed_aggregates; mod observed_attesters; mod observed_blob_sidecars; pub mod observed_block_producers; diff --git a/beacon_node/beacon_chain/src/observed_aggregates.rs b/beacon_node/beacon_chain/src/observed_aggregates.rs index 5e161a998b..b02bff96a2 100644 --- a/beacon_node/beacon_chain/src/observed_aggregates.rs +++ b/beacon_node/beacon_chain/src/observed_aggregates.rs @@ -6,11 +6,14 @@ use ssz_types::{BitList, BitVector}; use std::collections::HashMap; use std::marker::PhantomData; use tree_hash::TreeHash; +use tree_hash_derive::TreeHash; use types::consts::altair::{ SYNC_COMMITTEE_SUBNET_COUNT, TARGET_AGGREGATORS_PER_SYNC_SUBCOMMITTEE, }; use types::slot_data::SlotData; -use types::{Attestation, AttestationRef, EthSpec, Hash256, Slot, SyncCommitteeContribution}; +use types::{ + Attestation, AttestationData, AttestationRef, EthSpec, Hash256, Slot, SyncCommitteeContribution, +}; pub type ObservedSyncContributions = ObservedAggregates< SyncCommitteeContribution, @@ -20,6 +23,17 @@ pub type ObservedSyncContributions = ObservedAggregates< pub type ObservedAggregateAttestations = ObservedAggregates, E, BitList<::MaxValidatorsPerSlot>>; +/// Attestation data augmented with committee index +/// +/// This is hashed and used to key the map of observed aggregate attestations. This is important +/// post-Electra where the attestation data committee index is 0 and we want to avoid accidentally +/// comparing aggregation bits for *different* committees. +#[derive(TreeHash)] +pub struct ObservedAttestationKey { + pub committee_index: u64, + pub attestation_data: AttestationData, +} + /// A trait use to associate capacity constants with the type being stored in `ObservedAggregates`. pub trait Consts { /// The default capacity of items stored per slot, in a single `SlotHashSet`. @@ -92,11 +106,11 @@ pub trait SubsetItem { /// Returns the item that gets stored in `ObservedAggregates` for later subset /// comparison with incoming aggregates. - fn get_item(&self) -> Self::Item; + fn get_item(&self) -> Result; /// Returns a unique value that keys the object to the item that is being stored /// in `ObservedAggregates`. - fn root(&self) -> Hash256; + fn root(&self) -> Result; } impl<'a, E: EthSpec> SubsetItem for AttestationRef<'a, E> { @@ -126,19 +140,22 @@ impl<'a, E: EthSpec> SubsetItem for AttestationRef<'a, E> { } /// Returns the sync contribution aggregation bits. - fn get_item(&self) -> Self::Item { + fn get_item(&self) -> Result { match self { - Self::Base(att) => { - // TODO(electra) fix unwrap - att.extend_aggregation_bits().unwrap() - } - Self::Electra(att) => att.aggregation_bits.clone(), + Self::Base(att) => att + .extend_aggregation_bits() + .map_err(|_| Error::GetItemError), + Self::Electra(att) => Ok(att.aggregation_bits.clone()), } } - /// Returns the hash tree root of the attestation data. - fn root(&self) -> Hash256 { - self.data().tree_hash_root() + /// Returns the hash tree root of the attestation data augmented with the committee index. + fn root(&self) -> Result { + Ok(ObservedAttestationKey { + committee_index: self.committee_index().ok_or(Error::RootError)?, + attestation_data: self.data().clone(), + } + .tree_hash_root()) } } @@ -153,19 +170,19 @@ impl<'a, E: EthSpec> SubsetItem for &'a SyncCommitteeContribution { } /// Returns the sync contribution aggregation bits. - fn get_item(&self) -> Self::Item { - self.aggregation_bits.clone() + fn get_item(&self) -> Result { + Ok(self.aggregation_bits.clone()) } /// Returns the hash tree root of the root, slot and subcommittee index /// of the sync contribution. - fn root(&self) -> Hash256 { - SyncCommitteeData { + fn root(&self) -> Result { + Ok(SyncCommitteeData { root: self.beacon_block_root, slot: self.slot, subcommittee_index: self.subcommittee_index, } - .tree_hash_root() + .tree_hash_root()) } } @@ -192,6 +209,8 @@ pub enum Error { expected: Slot, attestation: Slot, }, + GetItemError, + RootError, } /// A `HashMap` that contains entries related to some `Slot`. @@ -234,7 +253,7 @@ impl SlotHashSet { // If true, we replace the new item with its existing subset. This allows us // to hold fewer items in the list. } else if item.is_superset(existing) { - *existing = item.get_item(); + *existing = item.get_item()?; return Ok(ObserveOutcome::New); } } @@ -252,7 +271,7 @@ impl SlotHashSet { return Err(Error::ReachedMaxObservationsPerSlot(self.max_capacity)); } - let item = item.get_item(); + let item = item.get_item()?; self.map.entry(root).or_default().push(item); Ok(ObserveOutcome::New) } @@ -345,7 +364,7 @@ where root_opt: Option, ) -> Result { let index = self.get_set_index(item.get_slot())?; - let root = root_opt.unwrap_or_else(|| item.root()); + let root = root_opt.map_or_else(|| item.root(), Ok)?; self.sets .get_mut(index) @@ -487,7 +506,10 @@ mod tests { for a in &items { assert_eq!( - store.is_known_subset(a.as_reference(), a.as_reference().root()), + store.is_known_subset( + a.as_reference(), + a.as_reference().root().unwrap() + ), Ok(false), "should indicate an unknown attestation is unknown" ); @@ -500,12 +522,18 @@ mod tests { for a in &items { assert_eq!( - store.is_known_subset(a.as_reference(), a.as_reference().root()), + store.is_known_subset( + a.as_reference(), + a.as_reference().root().unwrap() + ), Ok(true), "should indicate a known attestation is known" ); assert_eq!( - store.observe_item(a.as_reference(), Some(a.as_reference().root())), + store.observe_item( + a.as_reference(), + Some(a.as_reference().root().unwrap()) + ), Ok(ObserveOutcome::Subset), "should acknowledge an existing attestation" ); diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 63740c4736..3e35e58296 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -2,8 +2,8 @@ use beacon_chain::attestation_verification::{ batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations, Error, - ObservedAttestationKey, }; +use beacon_chain::observed_aggregates::ObservedAttestationKey; use beacon_chain::test_utils::{MakeAttestationOptions, HARNESS_GENESIS_TIME}; use beacon_chain::{ attestation_verification::Error as AttnError, @@ -852,7 +852,9 @@ async fn aggregated_gossip_verification() { err, AttnError::AttestationSupersetKnown(hash) if hash == ObservedAttestationKey { - committee_index: tester.valid_aggregate.message().aggregate().expect("should get committee index"), + committee_index: tester.valid_aggregate.message().aggregate() + .committee_index() + .expect("should get committee index"), attestation_data: tester.valid_aggregate.message().aggregate().data().clone(), }.tree_hash_root() )) From bc044ed27518be3495262ba36a9c7f40a2a73e8f Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Tue, 18 Jun 2024 01:05:15 +1000 Subject: [PATCH 07/53] Fix `CommandLineTest` port conflicts on CI (#5908) * Fix port conflicts on CI. --- lighthouse/src/main.rs | 56 +++++++++++++++++---------------- lighthouse/tests/beacon_node.rs | 34 ++++++++++++++++---- lighthouse/tests/exec.rs | 18 ++++++++--- 3 files changed, 71 insertions(+), 37 deletions(-) diff --git a/lighthouse/src/main.rs b/lighthouse/src/main.rs index 5743bedfd7..abee30737c 100644 --- a/lighthouse/src/main.rs +++ b/lighthouse/src/main.rs @@ -688,9 +688,15 @@ fn run( let executor = context.executor.clone(); let mut config = beacon_node::get_config::(matches, &context)?; config.logger_config = logger_config; - let shutdown_flag = matches.get_flag("immediate-shutdown"); // Dump configs if `dump-config` or `dump-chain-config` flags are set clap_utils::check_dump_configs::<_, E>(matches, &config, &context.eth2_config.spec)?; + + let shutdown_flag = matches.get_flag("immediate-shutdown"); + if shutdown_flag { + info!(log, "Beacon node immediate shutdown triggered."); + return Ok(()); + } + executor.clone().spawn( async move { if let Err(e) = ProductionBeaconNode::new(context.clone(), config).await { @@ -700,10 +706,6 @@ fn run( let _ = executor .shutdown_sender() .try_send(ShutdownReason::Failure("Failed to start beacon node")); - } else if shutdown_flag { - let _ = executor.shutdown_sender().try_send(ShutdownReason::Success( - "Beacon node immediate shutdown triggered.", - )); } }, "beacon_node", @@ -715,31 +717,31 @@ fn run( let executor = context.executor.clone(); let config = validator_client::Config::from_cli(matches, context.log()) .map_err(|e| format!("Unable to initialize validator config: {}", e))?; - let shutdown_flag = matches.get_flag("immediate-shutdown"); // Dump configs if `dump-config` or `dump-chain-config` flags are set clap_utils::check_dump_configs::<_, E>(matches, &config, &context.eth2_config.spec)?; - if !shutdown_flag { - executor.clone().spawn( - async move { - if let Err(e) = ProductionValidatorClient::new(context, config) - .and_then(|mut vc| async move { vc.start_service().await }) - .await - { - crit!(log, "Failed to start validator client"; "reason" => e); - // Ignore the error since it always occurs during normal operation when - // shutting down. - let _ = executor.shutdown_sender().try_send(ShutdownReason::Failure( - "Failed to start validator client", - )); - } - }, - "validator_client", - ); - } else { - let _ = executor.shutdown_sender().try_send(ShutdownReason::Success( - "Validator client immediate shutdown triggered.", - )); + + let shutdown_flag = matches.get_flag("immediate-shutdown"); + if shutdown_flag { + info!(log, "Validator client immediate shutdown triggered."); + return Ok(()); } + + executor.clone().spawn( + async move { + if let Err(e) = ProductionValidatorClient::new(context, config) + .and_then(|mut vc| async move { vc.start_service().await }) + .await + { + crit!(log, "Failed to start validator client"; "reason" => e); + // Ignore the error since it always occurs during normal operation when + // shutting down. + let _ = executor + .shutdown_sender() + .try_send(ShutdownReason::Failure("Failed to start validator client")); + } + }, + "validator_client", + ); } _ => { crit!(log, "No subcommand supplied. See --help ."); diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs index 73badac913..caadf208e3 100644 --- a/lighthouse/tests/beacon_node.rs +++ b/lighthouse/tests/beacon_node.rs @@ -58,13 +58,22 @@ impl CommandLineTest { fn run_with_zero_port(&mut self) -> CompletedTest { // Required since Deneb was enabled on mainnet. - self.cmd.arg("--allow-insecure-genesis-sync"); - self.run_with_zero_port_and_no_genesis_sync() + self.set_allow_insecure_genesis_sync() + .run_with_zero_port_and_no_genesis_sync() } fn run_with_zero_port_and_no_genesis_sync(&mut self) -> CompletedTest { + self.set_zero_port().run() + } + + fn set_allow_insecure_genesis_sync(&mut self) -> &mut Self { + self.cmd.arg("--allow-insecure-genesis-sync"); + self + } + + fn set_zero_port(&mut self) -> &mut Self { self.cmd.arg("-z"); - self.run() + self } } @@ -101,9 +110,20 @@ fn staking_flag() { } #[test] -#[should_panic] fn allow_insecure_genesis_sync_default() { - CommandLineTest::new().run_with_zero_port_and_no_genesis_sync(); + CommandLineTest::new() + .run_with_zero_port_and_no_genesis_sync() + .with_config(|config| { + assert_eq!(config.allow_insecure_genesis_sync, false); + }); +} + +#[test] +#[should_panic] +fn insecure_genesis_sync_should_panic() { + CommandLineTest::new() + .set_zero_port() + .run_with_immediate_shutdown(false); } #[test] @@ -2252,7 +2272,9 @@ fn ensure_panic_on_failed_launch() { CommandLineTest::new() .flag("slasher", None) .flag("slasher-chunk-size", Some("10")) - .run_with_zero_port() + .set_allow_insecure_genesis_sync() + .set_zero_port() + .run_with_immediate_shutdown(false) .with_config(|config| { let slasher_config = config .slasher diff --git a/lighthouse/tests/exec.rs b/lighthouse/tests/exec.rs index 61e0677ca8..9d6453908c 100644 --- a/lighthouse/tests/exec.rs +++ b/lighthouse/tests/exec.rs @@ -21,12 +21,19 @@ pub trait CommandLineTestExec { self } + fn run(&mut self) -> CompletedTest { + self.run_with_immediate_shutdown(true) + } + /// Executes the `Command` returned by `Self::cmd_mut` with temporary data directory, dumps - /// the configuration and shuts down immediately. + /// the configuration and shuts down immediately if `immediate_shutdown` is set to true. /// /// Options `--datadir`, `--dump-config`, `--dump-chain-config` and `--immediate-shutdown` must /// not be set on the command. - fn run(&mut self) -> CompletedTest { + fn run_with_immediate_shutdown( + &mut self, + immediate_shutdown: bool, + ) -> CompletedTest { // Setup temp directory. let tmp_dir = TempDir::new().expect("Unable to create temporary directory"); let tmp_config_path: PathBuf = tmp_dir.path().join("config.json"); @@ -39,8 +46,11 @@ pub trait CommandLineTestExec { .arg(format!("--{}", "dump-config")) .arg(tmp_config_path.as_os_str()) .arg(format!("--{}", "dump-chain-config")) - .arg(tmp_chain_config_path.as_os_str()) - .arg("--immediate-shutdown"); + .arg(tmp_chain_config_path.as_os_str()); + + if immediate_shutdown { + cmd.arg("--immediate-shutdown"); + } // Run the command. let output = output_result(cmd); From 44c03d5d17eafc5b048f76bee0366d1756a83463 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Tue, 18 Jun 2024 01:05:18 +1000 Subject: [PATCH 08/53] Add `FUNDING.json` for DRIPS (#5932) * Add FUNDING.json --- FUNDING.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 FUNDING.json diff --git a/FUNDING.json b/FUNDING.json new file mode 100644 index 0000000000..5001999927 --- /dev/null +++ b/FUNDING.json @@ -0,0 +1,7 @@ +{ + "drips": { + "ethereum": { + "ownedBy": "0x25c4a76E7d118705e7Ea2e9b7d8C59930d8aCD3b" + } + } +} \ No newline at end of file From 21f3a191c598dc598e9dde298acfdaacda83b0be Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 18 Jun 2024 01:05:21 +1000 Subject: [PATCH 09/53] Remove `extern crate` (#5922) * Remove extern crate --- beacon_node/client/src/lib.rs | 2 -- beacon_node/eth1/src/lib.rs | 3 --- beacon_node/eth1/src/metrics.rs | 2 ++ beacon_node/execution_layer/src/versioned_hashes.rs | 2 -- beacon_node/lighthouse_network/src/lib.rs | 3 --- beacon_node/lighthouse_network/src/metrics.rs | 2 ++ .../lighthouse_network/src/peer_manager/peerdb/score.rs | 1 + beacon_node/lighthouse_network/src/rpc/protocol.rs | 1 + beacon_node/network/src/lib.rs | 3 --- beacon_node/network/src/metrics.rs | 1 + beacon_node/src/lib.rs | 2 -- beacon_node/store/src/lib.rs | 3 --- beacon_node/store/src/metrics.rs | 1 + common/compare_fields_derive/src/lib.rs | 2 -- common/eth2_interop_keypairs/src/lib.rs | 4 +--- common/lighthouse_metrics/src/lib.rs | 3 +-- common/logging/src/lib.rs | 4 +--- common/logging/src/tracing_metrics_layer.rs | 1 + common/slot_clock/src/lib.rs | 3 --- common/slot_clock/src/metrics.rs | 1 + common/test_random_derive/src/lib.rs | 4 +--- consensus/types/src/beacon_state/committee_cache/tests.rs | 1 + consensus/types/src/beacon_state/tests.rs | 1 + consensus/types/src/lib.rs | 2 -- consensus/types/src/subnet_id.rs | 1 + consensus/types/src/sync_subnet_id.rs | 1 + lcli/src/block_root.rs | 1 + lcli/src/main.rs | 2 -- lcli/src/parse_ssz.rs | 1 + lcli/src/skip_slots.rs | 1 + lcli/src/state_root.rs | 1 + lcli/src/transition_blocks.rs | 1 + testing/simulator/src/main.rs | 3 --- 33 files changed, 23 insertions(+), 41 deletions(-) diff --git a/beacon_node/client/src/lib.rs b/beacon_node/client/src/lib.rs index fd92c28255..e6042103e1 100644 --- a/beacon_node/client/src/lib.rs +++ b/beacon_node/client/src/lib.rs @@ -1,5 +1,3 @@ -extern crate slog; - mod compute_light_client_updates; pub mod config; mod metrics; diff --git a/beacon_node/eth1/src/lib.rs b/beacon_node/eth1/src/lib.rs index 3b288de490..9d6cb7c847 100644 --- a/beacon_node/eth1/src/lib.rs +++ b/beacon_node/eth1/src/lib.rs @@ -1,6 +1,3 @@ -#[macro_use] -extern crate lazy_static; - mod block_cache; mod deposit_cache; mod inner; diff --git a/beacon_node/eth1/src/metrics.rs b/beacon_node/eth1/src/metrics.rs index 5441b40d7e..ad94d42ecb 100644 --- a/beacon_node/eth1/src/metrics.rs +++ b/beacon_node/eth1/src/metrics.rs @@ -1,5 +1,7 @@ pub use lighthouse_metrics::*; +use lazy_static::lazy_static; + lazy_static! { /* * Eth1 blocks diff --git a/beacon_node/execution_layer/src/versioned_hashes.rs b/beacon_node/execution_layer/src/versioned_hashes.rs index 37bd35646d..9bf87596b4 100644 --- a/beacon_node/execution_layer/src/versioned_hashes.rs +++ b/beacon_node/execution_layer/src/versioned_hashes.rs @@ -1,5 +1,3 @@ -extern crate alloy_consensus; -extern crate alloy_rlp; use alloy_consensus::TxEnvelope; use alloy_rlp::Decodable; use types::{EthSpec, ExecutionPayloadRef, Hash256, Unsigned, VersionedHash}; diff --git a/beacon_node/lighthouse_network/src/lib.rs b/beacon_node/lighthouse_network/src/lib.rs index 264795844a..0b827164fc 100644 --- a/beacon_node/lighthouse_network/src/lib.rs +++ b/beacon_node/lighthouse_network/src/lib.rs @@ -2,9 +2,6 @@ /// all required libp2p functionality. /// /// This crate builds and manages the libp2p services required by the beacon node. -#[macro_use] -extern crate lazy_static; - mod config; pub mod service; diff --git a/beacon_node/lighthouse_network/src/metrics.rs b/beacon_node/lighthouse_network/src/metrics.rs index fc441f2533..8efed44eb4 100644 --- a/beacon_node/lighthouse_network/src/metrics.rs +++ b/beacon_node/lighthouse_network/src/metrics.rs @@ -1,5 +1,7 @@ pub use lighthouse_metrics::*; +use lazy_static::lazy_static; + lazy_static! { pub static ref NAT_OPEN: Result = try_create_int_gauge_vec( "nat_open", diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs index ba9bd31472..8187dc4ba4 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs @@ -6,6 +6,7 @@ //! //! The scoring algorithms are currently experimental. use crate::service::gossipsub_scoring_parameters::GREYLIST_THRESHOLD as GOSSIPSUB_GREYLIST_THRESHOLD; +use lazy_static::lazy_static; use serde::Serialize; use std::time::Instant; use strum::AsRefStr; diff --git a/beacon_node/lighthouse_network/src/rpc/protocol.rs b/beacon_node/lighthouse_network/src/rpc/protocol.rs index 12a7f09338..bfaaef9b3b 100644 --- a/beacon_node/lighthouse_network/src/rpc/protocol.rs +++ b/beacon_node/lighthouse_network/src/rpc/protocol.rs @@ -3,6 +3,7 @@ use crate::rpc::codec::{base::BaseInboundCodec, ssz_snappy::SSZSnappyInboundCode use futures::future::BoxFuture; use futures::prelude::{AsyncRead, AsyncWrite}; use futures::{FutureExt, StreamExt}; +use lazy_static::lazy_static; use libp2p::core::{InboundUpgrade, UpgradeInfo}; use ssz::Encode; use ssz_types::VariableList; diff --git a/beacon_node/network/src/lib.rs b/beacon_node/network/src/lib.rs index da64368b16..1149e6e6e3 100644 --- a/beacon_node/network/src/lib.rs +++ b/beacon_node/network/src/lib.rs @@ -1,6 +1,3 @@ -#[macro_use] -extern crate lazy_static; - /// This crate provides the network server for Lighthouse. pub mod error; #[allow(clippy::mutable_key_type)] // PeerId in hashmaps are no longer permitted by clippy diff --git a/beacon_node/network/src/metrics.rs b/beacon_node/network/src/metrics.rs index bf4cbd09ab..32e57da8ae 100644 --- a/beacon_node/network/src/metrics.rs +++ b/beacon_node/network/src/metrics.rs @@ -5,6 +5,7 @@ use beacon_chain::{ sync_committee_verification::Error as SyncCommitteeError, }; use fnv::FnvHashMap; +use lazy_static::lazy_static; pub use lighthouse_metrics::*; use lighthouse_network::{ peer_manager::peerdb::client::ClientKind, types::GossipKind, GossipTopic, Gossipsub, diff --git a/beacon_node/src/lib.rs b/beacon_node/src/lib.rs index 4ca084c316..ab400d2e73 100644 --- a/beacon_node/src/lib.rs +++ b/beacon_node/src/lib.rs @@ -1,5 +1,3 @@ -extern crate clap; - mod cli; mod config; diff --git a/beacon_node/store/src/lib.rs b/beacon_node/store/src/lib.rs index 66032d89c5..0247bea554 100644 --- a/beacon_node/store/src/lib.rs +++ b/beacon_node/store/src/lib.rs @@ -7,9 +7,6 @@ //! //! Provides a simple API for storing/retrieving all types that sometimes needs type-hints. See //! tests for implementation examples. -#[macro_use] -extern crate lazy_static; - mod chunk_writer; pub mod chunked_iter; pub mod chunked_vector; diff --git a/beacon_node/store/src/metrics.rs b/beacon_node/store/src/metrics.rs index 2d901fdd93..1e614036ea 100644 --- a/beacon_node/store/src/metrics.rs +++ b/beacon_node/store/src/metrics.rs @@ -1,6 +1,7 @@ pub use lighthouse_metrics::{set_gauge, try_create_int_gauge, *}; use directory::size_of_dir; +use lazy_static::lazy_static; use std::path::Path; lazy_static! { diff --git a/common/compare_fields_derive/src/lib.rs b/common/compare_fields_derive/src/lib.rs index 01c5a8f6ef..1a89ccf4fd 100644 --- a/common/compare_fields_derive/src/lib.rs +++ b/common/compare_fields_derive/src/lib.rs @@ -1,5 +1,3 @@ -extern crate proc_macro; - use proc_macro::TokenStream; use quote::quote; use syn::{parse_macro_input, DeriveInput}; diff --git a/common/eth2_interop_keypairs/src/lib.rs b/common/eth2_interop_keypairs/src/lib.rs index 3031e1c4dc..34c3d6f87c 100644 --- a/common/eth2_interop_keypairs/src/lib.rs +++ b/common/eth2_interop_keypairs/src/lib.rs @@ -16,11 +16,9 @@ //! //! This implementation passes the [reference implementation //! tests](https://github.com/ethereum/eth2.0-pm/blob/6e41fcf383ebeb5125938850d8e9b4e9888389b4/interop/mocked_start/keygen_test_vector.yaml). -#[macro_use] -extern crate lazy_static; - use bls::{Keypair, PublicKey, SecretKey}; use ethereum_hashing::hash; +use lazy_static::lazy_static; use num_bigint::BigUint; use serde::{Deserialize, Serialize}; use std::fs::File; diff --git a/common/lighthouse_metrics/src/lib.rs b/common/lighthouse_metrics/src/lib.rs index 5d25bb313f..4a76184b8a 100644 --- a/common/lighthouse_metrics/src/lib.rs +++ b/common/lighthouse_metrics/src/lib.rs @@ -20,8 +20,7 @@ //! ## Example //! //! ```rust -//! #[macro_use] -//! extern crate lazy_static; +//! use lazy_static::lazy_static; //! use lighthouse_metrics::*; //! //! // These metrics are "magically" linked to the global registry defined in `lighthouse_metrics`. diff --git a/common/logging/src/lib.rs b/common/logging/src/lib.rs index b0e1da00e9..50d04fc088 100644 --- a/common/logging/src/lib.rs +++ b/common/logging/src/lib.rs @@ -1,6 +1,4 @@ -#[macro_use] -extern crate lazy_static; - +use lazy_static::lazy_static; use lighthouse_metrics::{ inc_counter, try_create_int_counter, IntCounter, Result as MetricsResult, }; diff --git a/common/logging/src/tracing_metrics_layer.rs b/common/logging/src/tracing_metrics_layer.rs index 08c323ee89..b9dde584b4 100644 --- a/common/logging/src/tracing_metrics_layer.rs +++ b/common/logging/src/tracing_metrics_layer.rs @@ -1,5 +1,6 @@ //! Exposes [`MetricsLayer`]: A tracing layer that registers metrics of logging events. +use lazy_static::lazy_static; use lighthouse_metrics as metrics; use tracing_log::NormalizeEvent; diff --git a/common/slot_clock/src/lib.rs b/common/slot_clock/src/lib.rs index 4f54b2ee76..a742e29457 100644 --- a/common/slot_clock/src/lib.rs +++ b/common/slot_clock/src/lib.rs @@ -1,6 +1,3 @@ -#[macro_use] -extern crate lazy_static; - mod manual_slot_clock; mod metrics; mod system_time_slot_clock; diff --git a/common/slot_clock/src/metrics.rs b/common/slot_clock/src/metrics.rs index 23a793b203..ae3a9b599f 100644 --- a/common/slot_clock/src/metrics.rs +++ b/common/slot_clock/src/metrics.rs @@ -1,4 +1,5 @@ use crate::SlotClock; +use lazy_static::lazy_static; pub use lighthouse_metrics::*; use types::{EthSpec, Slot}; diff --git a/common/test_random_derive/src/lib.rs b/common/test_random_derive/src/lib.rs index 648c20121a..8c4b1ef7c3 100644 --- a/common/test_random_derive/src/lib.rs +++ b/common/test_random_derive/src/lib.rs @@ -1,6 +1,4 @@ -extern crate proc_macro; - -use crate::proc_macro::TokenStream; +use proc_macro::TokenStream; use quote::quote; use syn::{parse_macro_input, DeriveInput}; diff --git a/consensus/types/src/beacon_state/committee_cache/tests.rs b/consensus/types/src/beacon_state/committee_cache/tests.rs index a227476569..4dc06feab3 100644 --- a/consensus/types/src/beacon_state/committee_cache/tests.rs +++ b/consensus/types/src/beacon_state/committee_cache/tests.rs @@ -2,6 +2,7 @@ use crate::test_utils::*; use beacon_chain::test_utils::{BeaconChainHarness, EphemeralHarnessType}; use beacon_chain::types::*; +use lazy_static::lazy_static; use swap_or_not_shuffle::shuffle_list; pub const VALIDATOR_COUNT: usize = 16; diff --git a/consensus/types/src/beacon_state/tests.rs b/consensus/types/src/beacon_state/tests.rs index 38a76e44c5..16c7ff152f 100644 --- a/consensus/types/src/beacon_state/tests.rs +++ b/consensus/types/src/beacon_state/tests.rs @@ -6,6 +6,7 @@ use beacon_chain::types::{ ChainSpec, Domain, Epoch, EthSpec, Hash256, Keypair, MainnetEthSpec, MinimalEthSpec, RelativeEpoch, Slot, Vector, }; +use lazy_static::lazy_static; use ssz::Encode; use std::ops::Mul; use swap_or_not_shuffle::compute_shuffled_index; diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index c170b6b70d..db429095d8 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -9,8 +9,6 @@ ) )] -#[macro_use] -extern crate lazy_static; #[macro_use] pub mod test_utils; diff --git a/consensus/types/src/subnet_id.rs b/consensus/types/src/subnet_id.rs index 9b6a2e6a19..7f6ed45ad8 100644 --- a/consensus/types/src/subnet_id.rs +++ b/consensus/types/src/subnet_id.rs @@ -1,5 +1,6 @@ //! Identifies each shard by an integer identifier. use crate::{AttestationData, ChainSpec, CommitteeIndex, Epoch, EthSpec, Slot}; +use lazy_static::lazy_static; use safe_arith::{ArithError, SafeArith}; use serde::{Deserialize, Serialize}; use std::ops::{Deref, DerefMut}; diff --git a/consensus/types/src/sync_subnet_id.rs b/consensus/types/src/sync_subnet_id.rs index dd0807f21c..00fb910bda 100644 --- a/consensus/types/src/sync_subnet_id.rs +++ b/consensus/types/src/sync_subnet_id.rs @@ -1,6 +1,7 @@ //! Identifies each sync committee subnet by an integer identifier. use crate::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; use crate::EthSpec; +use lazy_static::lazy_static; use safe_arith::{ArithError, SafeArith}; use serde::{Deserialize, Serialize}; use ssz_types::typenum::Unsigned; diff --git a/lcli/src/block_root.rs b/lcli/src/block_root.rs index 0ee304c8a5..a90a4843d8 100644 --- a/lcli/src/block_root.rs +++ b/lcli/src/block_root.rs @@ -32,6 +32,7 @@ use clap_utils::{parse_optional, parse_required}; use environment::Environment; use eth2::{types::BlockId, BeaconNodeHttpClient, SensitiveUrl, Timeouts}; use eth2_network_config::Eth2NetworkConfig; +use log::info; use std::path::PathBuf; use std::time::{Duration, Instant}; use types::{EthSpec, FullPayload, SignedBeaconBlock}; diff --git a/lcli/src/main.rs b/lcli/src/main.rs index 911e9bdcac..85898b60ee 100644 --- a/lcli/src/main.rs +++ b/lcli/src/main.rs @@ -1,5 +1,3 @@ -#[macro_use] -extern crate log; mod block_root; mod check_deposit_data; mod generate_bootnode_enr; diff --git a/lcli/src/parse_ssz.rs b/lcli/src/parse_ssz.rs index 3aa77e5700..dd13f6847b 100644 --- a/lcli/src/parse_ssz.rs +++ b/lcli/src/parse_ssz.rs @@ -1,6 +1,7 @@ use clap::ArgMatches; use clap_utils::parse_required; use eth2_network_config::Eth2NetworkConfig; +use log::info; use serde::Serialize; use snap::raw::Decoder; use ssz::Decode; diff --git a/lcli/src/skip_slots.rs b/lcli/src/skip_slots.rs index a2173f10df..2ad79051ea 100644 --- a/lcli/src/skip_slots.rs +++ b/lcli/src/skip_slots.rs @@ -50,6 +50,7 @@ use clap_utils::{parse_optional, parse_required}; use environment::Environment; use eth2::{types::StateId, BeaconNodeHttpClient, SensitiveUrl, Timeouts}; use eth2_network_config::Eth2NetworkConfig; +use log::info; use ssz::Encode; use state_processing::state_advance::{complete_state_advance, partial_state_advance}; use state_processing::AllCaches; diff --git a/lcli/src/state_root.rs b/lcli/src/state_root.rs index 06293b79b3..17a947b2f0 100644 --- a/lcli/src/state_root.rs +++ b/lcli/src/state_root.rs @@ -4,6 +4,7 @@ use clap_utils::{parse_optional, parse_required}; use environment::Environment; use eth2::{types::StateId, BeaconNodeHttpClient, SensitiveUrl, Timeouts}; use eth2_network_config::Eth2NetworkConfig; +use log::info; use std::path::PathBuf; use std::time::{Duration, Instant}; use types::{BeaconState, EthSpec}; diff --git a/lcli/src/transition_blocks.rs b/lcli/src/transition_blocks.rs index ba0c2efa51..5a450fed77 100644 --- a/lcli/src/transition_blocks.rs +++ b/lcli/src/transition_blocks.rs @@ -72,6 +72,7 @@ use eth2::{ BeaconNodeHttpClient, SensitiveUrl, Timeouts, }; use eth2_network_config::Eth2NetworkConfig; +use log::{debug, info}; use ssz::Encode; use state_processing::state_advance::complete_state_advance; use state_processing::{ diff --git a/testing/simulator/src/main.rs b/testing/simulator/src/main.rs index 03ee902c77..a259ac1133 100644 --- a/testing/simulator/src/main.rs +++ b/testing/simulator/src/main.rs @@ -10,9 +10,6 @@ //! simulation uses `println` to communicate some info. It might be nice if the nodes logged to //! easy-to-find files and stdout only contained info from the simulation. //! - -extern crate clap; - mod basic_sim; mod checks; mod cli; From 474c1b44863927c588dd05ab2ac0f934298398e1 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 17:05:24 +0200 Subject: [PATCH 10/53] Verify inclusion proof should not be fallible (#5787) * Verify inclusion proof should not be fallible * Add blob sidecar inclusion test (#33) * Add blob sidecar inclusion test. * Fix lint --- .../beacon_chain/src/blob_verification.rs | 13 +-- .../gossip_methods.rs | 1 - .../src/sync/network_context/requests.rs | 2 +- consensus/types/src/blob_sidecar.rs | 31 +++--- consensus/types/src/eth_spec.rs | 39 ++++++++ .../generate_random_block_and_blobs.rs | 96 +++++++++++++++++++ consensus/types/src/test_utils/mod.rs | 2 + 7 files changed, 153 insertions(+), 31 deletions(-) create mode 100644 consensus/types/src/test_utils/generate_random_block_and_blobs.rs diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index fdf8ee2b97..2b62a83194 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -10,7 +10,6 @@ use crate::block_verification::{ use crate::kzg_utils::{validate_blob, validate_blobs}; use crate::{metrics, BeaconChainError}; use kzg::{Error as KzgError, Kzg, KzgCommitment}; -use merkle_proof::MerkleTreeError; use slog::debug; use ssz_derive::{Decode, Encode}; use ssz_types::VariableList; @@ -128,13 +127,6 @@ pub enum GossipBlobError { /// The blob sidecar is invalid and the peer is faulty. KzgError(kzg::Error), - /// The kzg commitment inclusion proof failed. - /// - /// ## Peer scoring - /// - /// The blob sidecar is invalid - InclusionProof(MerkleTreeError), - /// The pubkey cache timed out. /// /// ## Peer scoring @@ -459,10 +451,7 @@ pub fn validate_blob_sidecar_for_gossip( // Verify the inclusion proof in the sidecar let _timer = metrics::start_timer(&metrics::BLOB_SIDECAR_INCLUSION_PROOF_VERIFICATION); - if !blob_sidecar - .verify_blob_sidecar_inclusion_proof() - .map_err(GossipBlobError::InclusionProof)? - { + if !blob_sidecar.verify_blob_sidecar_inclusion_proof() { return Err(GossipBlobError::InvalidInclusionProof); } drop(_timer); diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 374dca2a5a..99622bfa30 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -690,7 +690,6 @@ impl NetworkBeaconProcessor { | GossipBlobError::InvalidSubnet { .. } | GossipBlobError::InvalidInclusionProof | GossipBlobError::KzgError(_) - | GossipBlobError::InclusionProof(_) | GossipBlobError::NotFinalizedDescendant { .. } => { warn!( self.log, diff --git a/beacon_node/network/src/sync/network_context/requests.rs b/beacon_node/network/src/sync/network_context/requests.rs index cd73b4beba..6e4683701b 100644 --- a/beacon_node/network/src/sync/network_context/requests.rs +++ b/beacon_node/network/src/sync/network_context/requests.rs @@ -120,7 +120,7 @@ impl ActiveBlobsByRootRequest { if self.request.block_root != block_root { return Err(LookupVerifyError::UnrequestedBlockRoot(block_root)); } - if !blob.verify_blob_sidecar_inclusion_proof().unwrap_or(false) { + if !blob.verify_blob_sidecar_inclusion_proof() { return Err(LookupVerifyError::InvalidInclusionProof); } if !self.request.indices.contains(&blob.index) { diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index e54bc2f4f9..50530543a5 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -12,7 +12,7 @@ use kzg::{ }; use merkle_proof::{merkle_root_from_branch, verify_merkle_proof, MerkleTreeError}; use rand::Rng; -use safe_arith::{ArithError, SafeArith}; +use safe_arith::ArithError; use serde::{Deserialize, Serialize}; use ssz::Encode; use ssz_derive::{Decode, Encode}; @@ -190,33 +190,30 @@ impl BlobSidecar { } /// Verifies the kzg commitment inclusion merkle proof. - pub fn verify_blob_sidecar_inclusion_proof(&self) -> Result { - // Depth of the subtree rooted at `blob_kzg_commitments` in the `BeaconBlockBody` - // is equal to depth of the ssz List max size + 1 for the length mixin - let kzg_commitments_tree_depth = (E::max_blob_commitments_per_block() - .next_power_of_two() - .ilog2() - .safe_add(1))? as usize; + pub fn verify_blob_sidecar_inclusion_proof(&self) -> bool { + let kzg_commitments_tree_depth = E::kzg_commitments_tree_depth(); + + // EthSpec asserts that kzg_commitments_tree_depth is less than KzgCommitmentInclusionProofDepth + let (kzg_commitment_subtree_proof, kzg_commitments_proof) = self + .kzg_commitment_inclusion_proof + .split_at(kzg_commitments_tree_depth); + // Compute the `tree_hash_root` of the `blob_kzg_commitments` subtree using the // inclusion proof branches let blob_kzg_commitments_root = merkle_root_from_branch( self.kzg_commitment.tree_hash_root(), - self.kzg_commitment_inclusion_proof - .get(0..kzg_commitments_tree_depth) - .ok_or(MerkleTreeError::PleaseNotifyTheDevs)?, + kzg_commitment_subtree_proof, kzg_commitments_tree_depth, self.index as usize, ); // The remaining inclusion proof branches are for the top level `BeaconBlockBody` tree - Ok(verify_merkle_proof( + verify_merkle_proof( blob_kzg_commitments_root, - self.kzg_commitment_inclusion_proof - .get(kzg_commitments_tree_depth..E::kzg_proof_inclusion_proof_depth()) - .ok_or(MerkleTreeError::PleaseNotifyTheDevs)?, - E::kzg_proof_inclusion_proof_depth().safe_sub(kzg_commitments_tree_depth)?, + kzg_commitments_proof, + E::block_body_tree_depth(), BLOB_KZG_COMMITMENTS_INDEX, self.signed_block_header.message.body_root, - )) + ) } pub fn random_valid(rng: &mut R, kzg: &Kzg) -> Result { diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index 62f7f1b869..38a048d469 100644 --- a/consensus/types/src/eth_spec.rs +++ b/consensus/types/src/eth_spec.rs @@ -292,6 +292,22 @@ pub trait EthSpec: Self::KzgCommitmentInclusionProofDepth::to_usize() } + fn kzg_commitments_tree_depth() -> usize { + // Depth of the subtree rooted at `blob_kzg_commitments` in the `BeaconBlockBody` + // is equal to depth of the ssz List max size + 1 for the length mixin + Self::max_blob_commitments_per_block() + .next_power_of_two() + .ilog2() + .safe_add(1) + .expect("The log of max_blob_commitments_per_block can not overflow") as usize + } + + fn block_body_tree_depth() -> usize { + Self::kzg_proof_inclusion_proof_depth() + .safe_sub(Self::kzg_commitments_tree_depth()) + .expect("Preset values are not configurable and never result in non-positive block body depth") + } + /// Returns the `PENDING_BALANCE_DEPOSITS_LIMIT` constant for this specification. fn pending_balance_deposits_limit() -> usize { Self::PendingBalanceDepositsLimit::to_usize() @@ -517,3 +533,26 @@ impl EthSpec for GnosisEthSpec { EthSpecId::Gnosis } } + +#[cfg(test)] +mod test { + use crate::{EthSpec, GnosisEthSpec, MainnetEthSpec, MinimalEthSpec}; + + fn assert_valid_spec() { + E::kzg_commitments_tree_depth(); + E::block_body_tree_depth(); + } + + #[test] + fn mainnet_spec() { + assert_valid_spec::(); + } + #[test] + fn minimal_spec() { + assert_valid_spec::(); + } + #[test] + fn gnosis_spec() { + assert_valid_spec::(); + } +} diff --git a/consensus/types/src/test_utils/generate_random_block_and_blobs.rs b/consensus/types/src/test_utils/generate_random_block_and_blobs.rs new file mode 100644 index 0000000000..ab7ded0409 --- /dev/null +++ b/consensus/types/src/test_utils/generate_random_block_and_blobs.rs @@ -0,0 +1,96 @@ +use rand::Rng; + +use kzg::{KzgCommitment, KzgProof}; + +use crate::beacon_block_body::KzgCommitments; +use crate::*; + +use super::*; + +type BlobsBundle = (KzgCommitments, KzgProofs, BlobsList); + +pub fn generate_rand_block_and_blobs( + fork_name: ForkName, + num_blobs: usize, + rng: &mut impl Rng, +) -> (SignedBeaconBlock>, Vec>) { + let inner = map_fork_name!(fork_name, BeaconBlock, <_>::random_for_test(rng)); + let mut block = SignedBeaconBlock::from_block(inner, Signature::random_for_test(rng)); + let mut blob_sidecars = vec![]; + + if block.fork_name_unchecked() < ForkName::Deneb { + return (block, blob_sidecars); + } + + let (commitments, proofs, blobs) = generate_blobs::(num_blobs).unwrap(); + *block + .message_mut() + .body_mut() + .blob_kzg_commitments_mut() + .expect("kzg commitment expected from Deneb") = commitments.clone(); + + for (index, ((blob, kzg_commitment), kzg_proof)) in blobs + .into_iter() + .zip(commitments.into_iter()) + .zip(proofs.into_iter()) + .enumerate() + { + blob_sidecars.push(BlobSidecar { + index: index as u64, + blob: blob.clone(), + kzg_commitment, + kzg_proof, + signed_block_header: block.signed_block_header(), + kzg_commitment_inclusion_proof: block + .message() + .body() + .kzg_commitment_merkle_proof(index) + .unwrap(), + }); + } + (block, blob_sidecars) +} + +pub fn generate_blobs(n_blobs: usize) -> Result, String> { + let (mut commitments, mut proofs, mut blobs) = BlobsBundle::::default(); + + for blob_index in 0..n_blobs { + blobs + .push(Blob::::default()) + .map_err(|_| format!("blobs are full, blob index: {:?}", blob_index))?; + commitments + .push(KzgCommitment::empty_for_testing()) + .map_err(|_| format!("blobs are full, blob index: {:?}", blob_index))?; + proofs + .push(KzgProof::empty()) + .map_err(|_| format!("blobs are full, blob index: {:?}", blob_index))?; + } + + Ok((commitments, proofs, blobs)) +} + +#[cfg(test)] +mod test { + use super::*; + use rand::thread_rng; + + #[test] + fn test_verify_blob_inclusion_proof() { + let (_block, blobs) = + generate_rand_block_and_blobs::(ForkName::Deneb, 6, &mut thread_rng()); + for blob in blobs { + assert!(blob.verify_blob_sidecar_inclusion_proof()); + } + } + + #[test] + fn test_verify_blob_inclusion_proof_invalid() { + let (_block, blobs) = + generate_rand_block_and_blobs::(ForkName::Deneb, 6, &mut thread_rng()); + + for mut blob in blobs { + blob.kzg_commitment_inclusion_proof = FixedVector::random_for_test(&mut thread_rng()); + assert!(!blob.verify_blob_sidecar_inclusion_proof()); + } + } +} diff --git a/consensus/types/src/test_utils/mod.rs b/consensus/types/src/test_utils/mod.rs index d172342ee6..9599bcd364 100644 --- a/consensus/types/src/test_utils/mod.rs +++ b/consensus/types/src/test_utils/mod.rs @@ -15,6 +15,8 @@ use tree_hash::TreeHash; #[macro_use] mod macros; mod generate_deterministic_keypairs; +#[cfg(test)] +mod generate_random_block_and_blobs; mod test_random; pub fn test_ssz_tree_hash_pair(v1: &T, v2: &U) From c2838ba2bd45d28e3d4c8cf17c8bf7b23f622fcf Mon Sep 17 00:00:00 2001 From: Akihito Nakano Date: Tue, 18 Jun 2024 13:49:45 +0900 Subject: [PATCH 11/53] Fix SelfRateLimiter breaks the sequence of delayed_requests. (#5903) * Add test for next_peer_request_ready * Use push_front to requeue --- Cargo.lock | 1 + beacon_node/lighthouse_network/Cargo.toml | 1 + .../src/rpc/self_limiter.rs | 71 ++++++++++++++++++- 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 2e87f8b7f0..d932b60972 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4964,6 +4964,7 @@ dependencies = [ "libp2p-mplex", "lighthouse_metrics", "lighthouse_version", + "logging", "lru", "lru_cache", "parking_lot 0.12.3", diff --git a/beacon_node/lighthouse_network/Cargo.toml b/beacon_node/lighthouse_network/Cargo.toml index e850bced16..56a8fe99c7 100644 --- a/beacon_node/lighthouse_network/Cargo.toml +++ b/beacon_node/lighthouse_network/Cargo.toml @@ -60,6 +60,7 @@ tempfile = { workspace = true } quickcheck = { workspace = true } quickcheck_macros = { workspace = true } async-channel = { workspace = true } +logging = { workspace = true } [features] libp2p-websocket = [] diff --git a/beacon_node/lighthouse_network/src/rpc/self_limiter.rs b/beacon_node/lighthouse_network/src/rpc/self_limiter.rs index be4c572308..115ae45a98 100644 --- a/beacon_node/lighthouse_network/src/rpc/self_limiter.rs +++ b/beacon_node/lighthouse_network/src/rpc/self_limiter.rs @@ -147,7 +147,7 @@ impl SelfRateLimiter { Err((rate_limited_req, wait_time)) => { let key = (peer_id, protocol); self.next_peer_request.insert(key, wait_time); - queued_requests.push_back(rate_limited_req); + queued_requests.push_front(rate_limited_req); // If one fails just wait for the next window that allows sending requests. return; } @@ -205,3 +205,72 @@ impl SelfRateLimiter { Poll::Pending } } + +#[cfg(test)] +mod tests { + use crate::rpc::config::{OutboundRateLimiterConfig, RateLimiterConfig}; + use crate::rpc::rate_limiter::Quota; + use crate::rpc::self_limiter::SelfRateLimiter; + use crate::rpc::{OutboundRequest, Ping, Protocol}; + use crate::service::api_types::RequestId; + use libp2p::PeerId; + use std::time::Duration; + use types::MainnetEthSpec; + + /// Test that `next_peer_request_ready` correctly maintains the queue. + #[tokio::test] + async fn test_next_peer_request_ready() { + let log = logging::test_logger(); + let config = OutboundRateLimiterConfig(RateLimiterConfig { + ping_quota: Quota::n_every(1, 2), + ..Default::default() + }); + let mut limiter: SelfRateLimiter, MainnetEthSpec> = + SelfRateLimiter::new(config, log).unwrap(); + let peer_id = PeerId::random(); + + for i in 1..=5 { + let _ = limiter.allows( + peer_id, + RequestId::Application(i), + OutboundRequest::Ping(Ping { data: i }), + ); + } + + { + let queue = limiter + .delayed_requests + .get(&(peer_id, Protocol::Ping)) + .unwrap(); + assert_eq!(4, queue.len()); + + // Check that requests in the queue are ordered in the sequence 2, 3, 4, 5. + let mut iter = queue.iter(); + for i in 2..=5 { + assert_eq!(iter.next().unwrap().request_id, RequestId::Application(i)); + } + + assert_eq!(limiter.ready_requests.len(), 0); + } + + // Wait until the tokens have been regenerated, then run `next_peer_request_ready`. + tokio::time::sleep(Duration::from_secs(3)).await; + limiter.next_peer_request_ready(peer_id, Protocol::Ping); + + { + let queue = limiter + .delayed_requests + .get(&(peer_id, Protocol::Ping)) + .unwrap(); + assert_eq!(3, queue.len()); + + // Check that requests in the queue are ordered in the sequence 3, 4, 5. + let mut iter = queue.iter(); + for i in 3..=5 { + assert_eq!(iter.next().unwrap().request_id, RequestId::Application(i)); + } + + assert_eq!(limiter.ready_requests.len(), 1); + } + } +} From ab7db7ca9c54dbf970539e116bd0428652a38571 Mon Sep 17 00:00:00 2001 From: chonghe <44791194+chong-he@users.noreply.github.com> Date: Tue, 18 Jun 2024 13:53:09 +0800 Subject: [PATCH 12/53] Update local testnet documentation (#5896) * Update local testnet doc * Update doc * Minor revision * Fix directory path * Update scripts/local_testnet/README.md Co-authored-by: Pawan Dhananjay * Add kurtosis web * Add save logs command * Log of a service * Update scripts/local_testnet/README.md Co-authored-by: Jimmy Chen * Update scripts/local_testnet/README.md Co-authored-by: Jimmy Chen * Minor revision * Merge branch 'local-testnet-doc' of https://github.com/chong-he/lighthouse into local-testnet-doc --- scripts/local_testnet/README.md | 224 ++++++++------------------------ 1 file changed, 54 insertions(+), 170 deletions(-) diff --git a/scripts/local_testnet/README.md b/scripts/local_testnet/README.md index 77c9d62c1c..0275cb217f 100644 --- a/scripts/local_testnet/README.md +++ b/scripts/local_testnet/README.md @@ -1,201 +1,85 @@ # Simple Local Testnet -These scripts allow for running a small local testnet with multiple beacon nodes and validator clients and a geth execution client. +These scripts allow for running a small local testnet with a default of 4 beacon nodes, 4 validator clients and 4 geth execution clients using Kurtosis. This setup can be useful for testing and development. -## Requirements +## Installation -The scripts require `lcli`, `lighthouse`, `geth`, `bootnode` to be installed on `PATH` (run `echo $PATH` to view all `PATH` directories). +1. Install [Docker](https://docs.docker.com/get-docker/). Verify that Docker has been successfully installed by running `sudo docker run hello-world`. +1. Install [Kurtosis](https://docs.kurtosis.com/install/). Verify that Kurtosis has been successfully installed by running `kurtosis version` which should display the version. -MacOS users need to install GNU `sed` and GNU `grep`, and add them both to `PATH` as well. - -The first step is to install Rust and dependencies. Refer to the [Lighthouse Book](https://lighthouse-book.sigmaprime.io/installation-source.html#dependencies) for installation. We will also need [jq](https://jqlang.github.io/jq/), which can be installed with `sudo apt install jq`. - -Then, we clone the Lighthouse repository: -```bash -cd ~ -git clone https://github.com/sigp/lighthouse.git -cd lighthouse -``` -We are now ready to build Lighthouse. Run the command: - -```bash -make -make install-lcli -``` - -This will build `lighthouse` and `lcli`. For `geth` and `bootnode`, go to [geth website](https://geth.ethereum.org/downloads) and download the `Geth & Tools`. For example, to download and extract `Geth & Tools 1.13.1`: - -```bash -cd ~ -curl -LO https://gethstore.blob.core.windows.net/builds/geth-alltools-linux-amd64-1.13.1-3f40e65c.tar.gz -tar xvf geth-alltools-linux-amd64-1.13.1-3f40e65c.tar.gz -``` - -After extraction, copy `geth` and `bootnode` to the `PATH`. A typical directory is `/usr/local/bin`. - -```bash -cd geth-alltools-linux-amd64-1.13.1-3f40e65c -sudo cp geth bootnode /usr/local/bin -``` - -After that We can remove the downloaded files: - -```bash -cd ~ -rm -r geth-alltools-linux-amd64-1.13.1-3f40e65c geth-alltools-linux-amd64-1.13.1-3f40e65c.tar.gz -``` - -We are now ready to start a local testnet. +1. Install [yq](https://github.com/mikefarah/yq). If you are on Ubuntu, you can install `yq` by running `sudo apt install yq -y`. ## Starting the testnet -To start a testnet using the predetermined settings: +To start a testnet, from the Lighthouse root repository: ```bash -cd ~ -cd ./lighthouse/scripts/local_testnet -./start_local_testnet.sh genesis.json +cd ./scripts/local_testnet +./start_local_testnet.sh ``` -This will execute the script and if the testnet setup is successful, you will see "Started!" at the end. +It will build a Lighthouse docker image from the root of the directory and will take an approximately 12 minutes to complete. Once built, the testing will be started automatically. You will see a list of services running and "Started!" at the end. +You can also select your own Lighthouse docker image to use by specifying it in `network_params.yml` under the `cl_image` key. +Full configuration reference for kurtosis is specified [here](https://github.com/ethpandaops/ethereum-package?tab=readme-ov-file#configuration). -The testnet starts with a post-merge genesis state. -The testnet starts a consensus layer and execution layer boot node along with `BN_COUNT` -(the number of beacon nodes) each connected to a geth execution client and `VC_COUNT` (the number of validator clients). By default, `BN_COUNT=4`, `VC_COUNT=4`. - -The `start_local_testnet.sh` script takes four options `-v VC_COUNT`, `-d DEBUG_LEVEL`, `-p` to enable builder proposals and `-h` for help. It also takes a mandatory `GENESIS_FILE` for initialising geth's state. -A sample `genesis.json` is provided in this directory. - -The options may be in any order or absent in which case they take the default value specified. -- VC_COUNT: the number of validator clients to create, default: `BN_COUNT` -- DEBUG_LEVEL: one of { error, warn, info, debug, trace }, default: `info` - -The `ETH1_BLOCK_HASH` environment variable is set to the block_hash of the genesis execution layer block which depends on the contents of `genesis.json`. Users of these scripts need to ensure that the `ETH1_BLOCK_HASH` variable is updated if genesis file is modified. - -To view the beacon, validator client and geth logs: +To view all running services: ```bash -tail -f ~/.lighthouse/local-testnet/testnet/beacon_node_1.log -tail -f ~/.lighthouse/local-testnet/testnet/validator_node_1.log -tail -f ~/.lighthouse/local-testnet/testnet/geth_1.log +kurtosis enclave inspect local-testnet ``` -where `beacon_node_1` can be changed to `beacon_node_2`, `beacon_node_3` or `beacon_node_4` to view logs for different beacon nodes. The same applies to validator clients and geth nodes. +To view the logs: + +```bash +kurtosis service logs local-testnet $SERVICE_NAME +``` + +where `$SERVICE_NAME` is obtained by inspecting the running services above. For example, to view the logs of the first beacon node, validator client and geth: + +```bash +kurtosis service logs local-testnet -f cl-1-lighthouse-geth +kurtosis service logs local-testnet -f vc-1-geth-lighthouse +kurtosis service logs local-testnet -f el-1-geth-lighthouse +``` + +If you would like to save the logs, use the command: + +```bash +kurtosis dump $OUTPUT_DIRECTORY +``` + +This will create a folder named `$OUTPUT_DIRECTORY` in the present working directory that contains all logs and other information. If you want the logs for a particular service and saved to a file named `logs.txt`: + +```bash +kurtosis service logs local-testnet $SERVICE_NAME -a > logs.txt +``` +where `$SERVICE_NAME` can be viewed by running `kurtosis enclave inspect local-testnet`. + +Kurtosis comes with a Dora explorer which can be opened with: + +```bash +open $(kurtosis port print local-testnet dora http) +``` + +Some testnet parameters can be varied by modifying the `network_params.yaml` file. Kurtosis also comes with a web UI which can be open with `kurtosis web`. ## Stopping the testnet -To stop the testnet, navigate to the directory `cd ~/lighthouse/scripts/local_testnet`, then run the command: +To stop the testnet, from the Lighthouse root repository: ```bash +cd ./scripts/local_testnet ./stop_local_testnet.sh ``` -Once a testnet is stopped, it cannot be continued from where it left off. When the start local testnet command is run, it will start a new local testnet. +You will see "Local testnet stopped." at the end. -## Manual creation of local testnet +## CLI options -In [Starting the testnet](./README.md#starting-the-testnet), the testnet is started automatically with predetermined parameters (database directory, ports used etc). This section describes some modifications of the local testnet settings, e.g., changing the database directory, or changing the ports used. - - -The testnet also contains parameters that are specified in `vars.env`, such as the slot time `SECONDS_PER_SLOT=3` (instead of 12 seconds on mainnet). You may change these parameters to suit your testing purposes. After that, in the `local_testnet` directory, run the following command to create genesis state with embedded validators and validator keys, and also to update the time in `genesis.json`: +The script comes with some CLI options, which can be viewed with `./start_local_testnet.sh --help`. One of the CLI options is to avoid rebuilding Lighthouse each time the testnet starts, which can be configured with the command: ```bash -./setup.sh -./setup_time.sh genesis.json -``` - -Note: The generated genesis validators are embedded into the genesis state as genesis validators and hence do not require manual deposits to activate. - -Generate bootnode enr and start an EL and CL bootnode so that multiple nodes can find each other -```bash -./bootnode.sh -./el_bootnode.sh -``` - -Start a geth node: -```bash -./geth.sh -``` -e.g. -```bash -./geth.sh $HOME/.lighthouse/local-testnet/geth_1 7001 6001 5001 genesis.json -``` - -Start a beacon node: - -```bash -./beacon_node.sh -``` -e.g. -```bash -./beacon_node.sh $HOME/.lighthouse/local-testnet/node_1 9001 9101 8001 http://localhost:5001 ~/.lighthouse/local-testnet/geth_1/geth/jwtsecret -``` - -In a new terminal, start the validator client which will attach to the first -beacon node: - -```bash -./validator_client.sh -``` -e.g. to attach to the above created beacon node -```bash -./validator_client.sh $HOME/.lighthouse/local-testnet/node_1 http://localhost:8001 -``` - -You can create additional geth, beacon node and validator client instances by changing the ports, e.g., for a second geth, beacon node and validator client: - -```bash -./geth.sh $HOME/.lighthouse/local-testnet/geth_2 7002 6002 5002 genesis.json -./beacon_node.sh $HOME/.lighthouse/local-testnet/node_2 9002 9102 8002 http://localhost:5002 ~/.lighthouse/local-testnet/geth_2/geth/jwtsecret -./validator_client.sh $HOME/.lighthouse/local-testnet/node_2 http://localhost:8002 -``` - -## Additional Info - -### Adjusting number and distribution of validators -The `VALIDATOR_COUNT` parameter is used to specify the number of insecure validator keystores to generate and make deposits for. -The `BN_COUNT` parameter is used to adjust the division of these generated keys among separate validator client instances. -For e.g. for `VALIDATOR_COUNT=80` and `BN_COUNT=4`, the validator keys are distributed over 4 datadirs with 20 keystores per datadir. The datadirs are located in `$DATADIR/node_{i}` which can be passed to separate validator client -instances using the `--datadir` parameter. - -### Starting fresh - -You can delete the current testnet and all related files using the following command. Alternatively, if you wish to start another testnet, doing the steps [Starting the testnet](./README.md#starting-the-testnet) will automatically delete the files and start a fresh local testnet. - -```bash -./clean.sh -``` - -### Updating the genesis time of the beacon state - -If it's been a while since you ran `./setup` then the genesis time of the -genesis state will be far in the future, causing lots of skip slots. - -Update the genesis time to now using: - -```bash -./reset_genesis_time.sh -``` - -> Note: you probably want to just rerun `./start_local_testnet.sh` to start over -> but this is another option. - -### Testing builder flow - -1. Add builder URL to `BN_ARGS` in `./vars.env`, e.g. `--builder http://localhost:8650`. Some mock builder server options: - - [`mock-relay`](https://github.com/realbigsean/mock-relay) - - [`dummy-builder`](https://github.com/michaelsproul/dummy_builder) -2. The above mock builders do not support non-mainnet presets as of now, and will require setting `SECONDS_PER_SLOT` and `SECONDS_PER_ETH1_BLOCK` to `12` in `./vars.env`. -3. Start the testnet with the following command (the `-p` flag enables the validator client `--builder-proposals` flag): - ```bash - ./start_local_testnet.sh -p genesis.json - ``` -4. Block production using builder flow will start at epoch 4. - -### Testing sending a transaction - -Some addresses in the local testnet are seeded with testnet ETH, allowing users to carry out transactions. To send a transaction, we first add the address to a wallet, such as [Metamask](https://metamask.io/). The private keys for the addresses are listed [here](https://github.com/sigp/lighthouse/blob/441fc1691b69f9edc4bbdc6665f3efab16265c9b/testing/execution_engine_integration/src/execution_engine.rs#L13-L14). - -Next, we add the local testnet to Metamask, a brief guide can be found [here](https://support.metamask.io/hc/en-us/articles/360043227612-How-to-add-a-custom-network-RPC). If you start the local testnet with default settings, the network RPC is: http://localhost:6001 and the `Chain ID` is `4242`, as defined in [`vars.env`](https://github.com/sigp/lighthouse/blob/441fc1691b69f9edc4bbdc6665f3efab16265c9b/scripts/local_testnet/vars.env#L42). Once the network and account are added, you should see that the account contains testnet ETH which allow us to carry out transactions. +./start_local_testnet.sh -b false +``` \ No newline at end of file From d06e3894de0625c7fd81c6b7cfb01b4b6a3cda83 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 19 Jun 2024 15:02:22 +1000 Subject: [PATCH 13/53] Fix cargo audit: update curve25519-dalek (#5959) * Patch curve25519-dalek. --- Cargo.lock | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d932b60972..a1865289b0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1716,16 +1716,15 @@ dependencies = [ [[package]] name = "curve25519-dalek" -version = "4.1.2" +version = "4.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0a677b8922c94e01bdbb12126b0bc852f00447528dee1782229af9c720c3f348" +checksum = "97fb8b7c4503de7d6ae7b42ab72a5a59857b4c937ec27a3d4539dba95b5ab2be" dependencies = [ "cfg-if", "cpufeatures", "curve25519-dalek-derive", "digest 0.10.7", "fiat-crypto", - "platforms 3.4.0", "rustc_version 0.4.0", "subtle", "zeroize", @@ -6144,12 +6143,6 @@ version = "2.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e8d0eef3571242013a0d5dc84861c3ae4a652e56e12adf8bdc26ff5f8cb34c94" -[[package]] -name = "platforms" -version = "3.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "db23d408679286588f4d4644f965003d056e3dd5abcaaa938116871d7ce2fee7" - [[package]] name = "plotters" version = "0.3.6" @@ -6456,7 +6449,7 @@ dependencies = [ "nix 0.24.3", "num_cpus", "once_cell", - "platforms 2.0.0", + "platforms", "thiserror", "unescape", ] From 9f8aa963b1184547468e9f6b780a575ead6d20f0 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Wed, 19 Jun 2024 15:02:24 +1000 Subject: [PATCH 14/53] Update CI nethermind and LLVM versions (#5933) * Update nethermind and llvm versions. * Fix nethermind binary path --- .github/workflows/release.yml | 2 +- .github/workflows/test-suite.yml | 5 ----- testing/execution_engine_integration/src/nethermind.rs | 9 ++++----- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 75063ee2e0..416179445f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -64,7 +64,7 @@ jobs: - uses: KyleMayes/install-llvm-action@v1 if: env.SELF_HOSTED_RUNNERS == 'false' && startsWith(matrix.arch, 'x86_64-windows') with: - version: "16.0" + version: "17.0" directory: ${{ runner.temp }}/llvm - name: Set LIBCLANG_PATH if: startsWith(matrix.arch, 'x86_64-windows') diff --git a/.github/workflows/test-suite.yml b/.github/workflows/test-suite.yml index 0840651bbc..769b889de4 100644 --- a/.github/workflows/test-suite.yml +++ b/.github/workflows/test-suite.yml @@ -112,11 +112,6 @@ jobs: - name: Install make if: env.SELF_HOSTED_RUNNERS == 'false' run: choco install -y make -# - uses: KyleMayes/install-llvm-action@v1 -# if: env.SELF_HOSTED_RUNNERS == 'false' -# with: -# version: "16.0" -# directory: ${{ runner.temp }}/llvm - name: Set LIBCLANG_PATH run: echo "LIBCLANG_PATH=$((gcm clang).source -replace "clang.exe")" >> $env:GITHUB_ENV - name: Run tests in release diff --git a/testing/execution_engine_integration/src/nethermind.rs b/testing/execution_engine_integration/src/nethermind.rs index aad37c32bd..c3b8651789 100644 --- a/testing/execution_engine_integration/src/nethermind.rs +++ b/testing/execution_engine_integration/src/nethermind.rs @@ -11,7 +11,7 @@ use unused_port::unused_tcp4_port; /// We've pinned the Nethermind version since our method of using the `master` branch to /// find the latest tag isn't working. It appears Nethermind don't always tag on `master`. /// We should fix this so we always pull the latest version of Nethermind. -const NETHERMIND_BRANCH: &str = "release/1.21.0"; +const NETHERMIND_BRANCH: &str = "release/1.27.0"; const NETHERMIND_REPO_URL: &str = "https://github.com/NethermindEth/nethermind"; fn build_result(repo_dir: &Path) -> Output { @@ -70,11 +70,10 @@ impl NethermindEngine { .join("nethermind") .join("src") .join("Nethermind") - .join("Nethermind.Runner") + .join("artifacts") .join("bin") - .join("Release") - .join("net7.0") - .join("linux-x64") + .join("Nethermind.Runner") + .join("release") .join("nethermind") } } From a87f19d801a57b1d6ff101750840294c210ff956 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Wed, 19 Jun 2024 06:02:26 +0100 Subject: [PATCH 15/53] chore: change `impl Into for U` to `impl From for T` (#5948) * chore: Change Into trait impl for KzgProof to From trait impl * chore: change `impl Into for U` to `impl From for T` * chore: remove `from-over-into` clippy lint exception --- Makefile | 1 - .../src/beacon_fork_choice_store.rs | 44 +++++++++---------- .../beacon_chain/src/persisted_fork_choice.rs | 16 +++---- .../execution_layer/src/engine_api/http.rs | 6 +-- common/eth2/src/types.rs | 10 ++--- consensus/proto_array/src/proto_array.rs | 34 +++++++------- consensus/proto_array/src/ssz_container.rs | 20 ++++----- consensus/types/src/graffiti.rs | 12 ++--- consensus/types/src/selection_proof.rs | 6 +-- consensus/types/src/slot_epoch_macros.rs | 12 ++--- consensus/types/src/subnet_id.rs | 12 ++--- consensus/types/src/sync_selection_proof.rs | 6 +-- consensus/types/src/sync_subnet_id.rs | 12 ++--- .../src/json_keystore/checksum_module.rs | 10 ++--- .../src/json_keystore/cipher_module.rs | 6 +-- .../src/json_keystore/hex_bytes.rs | 6 +-- .../src/json_keystore/kdf_module.rs | 10 ++--- crypto/eth2_wallet/src/json_wallet/mod.rs | 6 +-- crypto/kzg/src/kzg_proof.rs | 6 +-- .../slashing_protection/src/lib.rs | 6 +-- 20 files changed, 120 insertions(+), 121 deletions(-) diff --git a/Makefile b/Makefile index 3e6934e6b5..c3c8d13238 100644 --- a/Makefile +++ b/Makefile @@ -229,7 +229,6 @@ lint: -D clippy::manual_let_else \ -D warnings \ -A clippy::derive_partial_eq_without_eq \ - -A clippy::from-over-into \ -A clippy::upper-case-acronyms \ -A clippy::vec-init-then-push \ -A clippy::question-mark \ diff --git a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs index 2a42b49b42..f7f389c543 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -389,35 +389,35 @@ pub struct PersistedForkChoiceStore { pub equivocating_indices: BTreeSet, } -impl Into for PersistedForkChoiceStoreV11 { - fn into(self) -> PersistedForkChoiceStore { +impl From for PersistedForkChoiceStore { + fn from(from: PersistedForkChoiceStoreV11) -> PersistedForkChoiceStore { PersistedForkChoiceStore { - balances_cache: self.balances_cache, - time: self.time, - finalized_checkpoint: self.finalized_checkpoint, - justified_checkpoint: self.justified_checkpoint, - justified_balances: self.justified_balances, - unrealized_justified_checkpoint: self.unrealized_justified_checkpoint, - unrealized_finalized_checkpoint: self.unrealized_finalized_checkpoint, - proposer_boost_root: self.proposer_boost_root, - equivocating_indices: self.equivocating_indices, + balances_cache: from.balances_cache, + time: from.time, + finalized_checkpoint: from.finalized_checkpoint, + justified_checkpoint: from.justified_checkpoint, + justified_balances: from.justified_balances, + unrealized_justified_checkpoint: from.unrealized_justified_checkpoint, + unrealized_finalized_checkpoint: from.unrealized_finalized_checkpoint, + proposer_boost_root: from.proposer_boost_root, + equivocating_indices: from.equivocating_indices, } } } -impl Into for PersistedForkChoiceStore { - fn into(self) -> PersistedForkChoiceStoreV11 { +impl From for PersistedForkChoiceStoreV11 { + fn from(from: PersistedForkChoiceStore) -> PersistedForkChoiceStoreV11 { PersistedForkChoiceStoreV11 { - balances_cache: self.balances_cache, - time: self.time, - finalized_checkpoint: self.finalized_checkpoint, - justified_checkpoint: self.justified_checkpoint, - justified_balances: self.justified_balances, + balances_cache: from.balances_cache, + time: from.time, + finalized_checkpoint: from.finalized_checkpoint, + justified_checkpoint: from.justified_checkpoint, + justified_balances: from.justified_balances, best_justified_checkpoint: JUNK_BEST_JUSTIFIED_CHECKPOINT, - unrealized_justified_checkpoint: self.unrealized_justified_checkpoint, - unrealized_finalized_checkpoint: self.unrealized_finalized_checkpoint, - proposer_boost_root: self.proposer_boost_root, - equivocating_indices: self.equivocating_indices, + unrealized_justified_checkpoint: from.unrealized_justified_checkpoint, + unrealized_finalized_checkpoint: from.unrealized_finalized_checkpoint, + proposer_boost_root: from.proposer_boost_root, + equivocating_indices: from.equivocating_indices, } } } diff --git a/beacon_node/beacon_chain/src/persisted_fork_choice.rs b/beacon_node/beacon_chain/src/persisted_fork_choice.rs index 8297ea9345..5e50c3433a 100644 --- a/beacon_node/beacon_chain/src/persisted_fork_choice.rs +++ b/beacon_node/beacon_chain/src/persisted_fork_choice.rs @@ -20,20 +20,20 @@ pub struct PersistedForkChoice { pub fork_choice_store: PersistedForkChoiceStoreV17, } -impl Into for PersistedForkChoiceV11 { - fn into(self) -> PersistedForkChoice { +impl From for PersistedForkChoice { + fn from(from: PersistedForkChoiceV11) -> PersistedForkChoice { PersistedForkChoice { - fork_choice: self.fork_choice, - fork_choice_store: self.fork_choice_store.into(), + fork_choice: from.fork_choice, + fork_choice_store: from.fork_choice_store.into(), } } } -impl Into for PersistedForkChoice { - fn into(self) -> PersistedForkChoiceV11 { +impl From for PersistedForkChoiceV11 { + fn from(from: PersistedForkChoice) -> PersistedForkChoiceV11 { PersistedForkChoiceV11 { - fork_choice: self.fork_choice, - fork_choice_store: self.fork_choice_store.into(), + fork_choice: from.fork_choice, + fork_choice_store: from.fork_choice_store.into(), } } } diff --git a/beacon_node/execution_layer/src/engine_api/http.rs b/beacon_node/execution_layer/src/engine_api/http.rs index ef65119414..48095870ff 100644 --- a/beacon_node/execution_layer/src/engine_api/http.rs +++ b/beacon_node/execution_layer/src/engine_api/http.rs @@ -257,9 +257,9 @@ pub mod deposit_methods { Latest, } - impl Into for Eth1Id { - fn into(self) -> u64 { - match self { + impl From for u64 { + fn from(from: Eth1Id) -> u64 { + match from { Eth1Id::Mainnet => 1, Eth1Id::Custom(id) => id, } diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index 2bb749af9f..c5a6ea5d87 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -1700,11 +1700,11 @@ impl ForkVersionDeserialize for FullBlockContents { } } -impl Into> for FullBlockContents { - fn into(self) -> BeaconBlock { - match self { - Self::BlockContents(block_and_sidecars) => block_and_sidecars.block, - Self::Block(block) => block, +impl From> for BeaconBlock { + fn from(from: FullBlockContents) -> BeaconBlock { + match from { + FullBlockContents::::BlockContents(block_and_sidecars) => block_and_sidecars.block, + FullBlockContents::::Block(block) => block, } } } diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 7c2ecfe26a..d50153bfe8 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -145,24 +145,24 @@ impl TryInto for ProtoNodeV16 { } } -impl Into for ProtoNode { - fn into(self) -> ProtoNodeV16 { +impl From for ProtoNodeV16 { + fn from(from: ProtoNode) -> ProtoNodeV16 { ProtoNodeV16 { - slot: self.slot, - state_root: self.state_root, - target_root: self.target_root, - current_epoch_shuffling_id: self.current_epoch_shuffling_id, - next_epoch_shuffling_id: self.next_epoch_shuffling_id, - root: self.root, - parent: self.parent, - justified_checkpoint: Some(self.justified_checkpoint), - finalized_checkpoint: Some(self.finalized_checkpoint), - weight: self.weight, - best_child: self.best_child, - best_descendant: self.best_descendant, - execution_status: self.execution_status, - unrealized_justified_checkpoint: self.unrealized_justified_checkpoint, - unrealized_finalized_checkpoint: self.unrealized_finalized_checkpoint, + slot: from.slot, + state_root: from.state_root, + target_root: from.target_root, + current_epoch_shuffling_id: from.current_epoch_shuffling_id, + next_epoch_shuffling_id: from.next_epoch_shuffling_id, + root: from.root, + parent: from.parent, + justified_checkpoint: Some(from.justified_checkpoint), + finalized_checkpoint: Some(from.finalized_checkpoint), + weight: from.weight, + best_child: from.best_child, + best_descendant: from.best_descendant, + execution_status: from.execution_status, + unrealized_justified_checkpoint: from.unrealized_justified_checkpoint, + unrealized_finalized_checkpoint: from.unrealized_finalized_checkpoint, } } } diff --git a/consensus/proto_array/src/ssz_container.rs b/consensus/proto_array/src/ssz_container.rs index 3208584dc4..a6d585758b 100644 --- a/consensus/proto_array/src/ssz_container.rs +++ b/consensus/proto_array/src/ssz_container.rs @@ -55,19 +55,19 @@ impl TryInto for SszContainerV16 { } } -impl Into for SszContainer { - fn into(self) -> SszContainerV16 { - let nodes = self.nodes.into_iter().map(Into::into).collect(); +impl From for SszContainerV16 { + fn from(from: SszContainer) -> SszContainerV16 { + let nodes = from.nodes.into_iter().map(Into::into).collect(); SszContainerV16 { - votes: self.votes, - balances: self.balances, - prune_threshold: self.prune_threshold, - justified_checkpoint: self.justified_checkpoint, - finalized_checkpoint: self.finalized_checkpoint, + votes: from.votes, + balances: from.balances, + prune_threshold: from.prune_threshold, + justified_checkpoint: from.justified_checkpoint, + finalized_checkpoint: from.finalized_checkpoint, nodes, - indices: self.indices, - previous_proposer_boost: self.previous_proposer_boost, + indices: from.indices, + previous_proposer_boost: from.previous_proposer_boost, } } } diff --git a/consensus/types/src/graffiti.rs b/consensus/types/src/graffiti.rs index c7a0081c9f..3d8f411caf 100644 --- a/consensus/types/src/graffiti.rs +++ b/consensus/types/src/graffiti.rs @@ -37,9 +37,9 @@ impl From<[u8; GRAFFITI_BYTES_LEN]> for Graffiti { } } -impl Into<[u8; GRAFFITI_BYTES_LEN]> for Graffiti { - fn into(self) -> [u8; GRAFFITI_BYTES_LEN] { - self.0 +impl From for [u8; GRAFFITI_BYTES_LEN] { + fn from(from: Graffiti) -> [u8; GRAFFITI_BYTES_LEN] { + from.0 } } @@ -77,9 +77,9 @@ impl<'de> Deserialize<'de> for GraffitiString { } } -impl Into for GraffitiString { - fn into(self) -> Graffiti { - let graffiti_bytes = self.0.as_bytes(); +impl From for Graffiti { + fn from(from: GraffitiString) -> Graffiti { + let graffiti_bytes = from.0.as_bytes(); let mut graffiti = [0; GRAFFITI_BYTES_LEN]; let graffiti_len = std::cmp::min(graffiti_bytes.len(), GRAFFITI_BYTES_LEN); diff --git a/consensus/types/src/selection_proof.rs b/consensus/types/src/selection_proof.rs index cd9f9c06d0..c80a00c3d1 100644 --- a/consensus/types/src/selection_proof.rs +++ b/consensus/types/src/selection_proof.rs @@ -77,9 +77,9 @@ impl SelectionProof { } } -impl Into for SelectionProof { - fn into(self) -> Signature { - self.0 +impl From for Signature { + fn from(from: SelectionProof) -> Signature { + from.0 } } diff --git a/consensus/types/src/slot_epoch_macros.rs b/consensus/types/src/slot_epoch_macros.rs index 00e1af3918..42e7a0f2ee 100644 --- a/consensus/types/src/slot_epoch_macros.rs +++ b/consensus/types/src/slot_epoch_macros.rs @@ -6,9 +6,9 @@ macro_rules! impl_from_into_u64 { } } - impl Into for $main { - fn into(self) -> u64 { - self.0 + impl From<$main> for u64 { + fn from(from: $main) -> u64 { + from.0 } } @@ -28,9 +28,9 @@ macro_rules! impl_from_into_usize { } } - impl Into for $main { - fn into(self) -> usize { - self.0 as usize + impl From<$main> for usize { + fn from(from: $main) -> usize { + from.0 as usize } } diff --git a/consensus/types/src/subnet_id.rs b/consensus/types/src/subnet_id.rs index 7f6ed45ad8..1fff2518e8 100644 --- a/consensus/types/src/subnet_id.rs +++ b/consensus/types/src/subnet_id.rs @@ -150,15 +150,15 @@ impl From for SubnetId { } } -impl Into for SubnetId { - fn into(self) -> u64 { - self.0 +impl From for u64 { + fn from(from: SubnetId) -> u64 { + from.0 } } -impl Into for &SubnetId { - fn into(self) -> u64 { - self.0 +impl From<&SubnetId> for u64 { + fn from(from: &SubnetId) -> u64 { + from.0 } } diff --git a/consensus/types/src/sync_selection_proof.rs b/consensus/types/src/sync_selection_proof.rs index da7370a0c1..0b32c6981b 100644 --- a/consensus/types/src/sync_selection_proof.rs +++ b/consensus/types/src/sync_selection_proof.rs @@ -90,9 +90,9 @@ impl SyncSelectionProof { } } -impl Into for SyncSelectionProof { - fn into(self) -> Signature { - self.0 +impl From for Signature { + fn from(from: SyncSelectionProof) -> Signature { + from.0 } } diff --git a/consensus/types/src/sync_subnet_id.rs b/consensus/types/src/sync_subnet_id.rs index 00fb910bda..7806aecfca 100644 --- a/consensus/types/src/sync_subnet_id.rs +++ b/consensus/types/src/sync_subnet_id.rs @@ -78,15 +78,15 @@ impl From for SyncSubnetId { } } -impl Into for SyncSubnetId { - fn into(self) -> u64 { - self.0 +impl From for u64 { + fn from(from: SyncSubnetId) -> u64 { + from.0 } } -impl Into for &SyncSubnetId { - fn into(self) -> u64 { - self.0 +impl From<&SyncSubnetId> for u64 { + fn from(from: &SyncSubnetId) -> u64 { + from.0 } } diff --git a/crypto/eth2_keystore/src/json_keystore/checksum_module.rs b/crypto/eth2_keystore/src/json_keystore/checksum_module.rs index dbb21e4de1..834c5eda5f 100644 --- a/crypto/eth2_keystore/src/json_keystore/checksum_module.rs +++ b/crypto/eth2_keystore/src/json_keystore/checksum_module.rs @@ -14,9 +14,9 @@ pub enum ChecksumFunction { Sha256, } -impl Into for ChecksumFunction { - fn into(self) -> String { - match self { +impl From for String { + fn from(from: ChecksumFunction) -> String { + match from { ChecksumFunction::Sha256 => "sha256".into(), } } @@ -38,8 +38,8 @@ impl TryFrom for ChecksumFunction { #[serde(try_from = "Value", into = "Value")] pub struct EmptyMap; -impl Into for EmptyMap { - fn into(self) -> Value { +impl From for Value { + fn from(_from: EmptyMap) -> Value { Value::Object(Map::default()) } } diff --git a/crypto/eth2_keystore/src/json_keystore/cipher_module.rs b/crypto/eth2_keystore/src/json_keystore/cipher_module.rs index 03a9d305a2..c801a40923 100644 --- a/crypto/eth2_keystore/src/json_keystore/cipher_module.rs +++ b/crypto/eth2_keystore/src/json_keystore/cipher_module.rs @@ -13,9 +13,9 @@ pub enum CipherFunction { Aes128Ctr, } -impl Into for CipherFunction { - fn into(self) -> String { - match self { +impl From for String { + fn from(from: CipherFunction) -> String { + match from { CipherFunction::Aes128Ctr => "aes-128-ctr".into(), } } diff --git a/crypto/eth2_keystore/src/json_keystore/hex_bytes.rs b/crypto/eth2_keystore/src/json_keystore/hex_bytes.rs index cc61f13d97..e891693040 100644 --- a/crypto/eth2_keystore/src/json_keystore/hex_bytes.rs +++ b/crypto/eth2_keystore/src/json_keystore/hex_bytes.rs @@ -25,9 +25,9 @@ impl From> for HexBytes { } } -impl Into for HexBytes { - fn into(self) -> String { - hex::encode(self.0) +impl From for String { + fn from(from: HexBytes) -> String { + hex::encode(from.0) } } diff --git a/crypto/eth2_keystore/src/json_keystore/kdf_module.rs b/crypto/eth2_keystore/src/json_keystore/kdf_module.rs index a29b895c95..2086ac7b21 100644 --- a/crypto/eth2_keystore/src/json_keystore/kdf_module.rs +++ b/crypto/eth2_keystore/src/json_keystore/kdf_module.rs @@ -23,8 +23,8 @@ pub struct KdfModule { #[serde(try_from = "String", into = "String")] pub struct EmptyString; -impl Into for EmptyString { - fn into(self) -> String { +impl From for String { + fn from(_from: EmptyString) -> String { "".into() } } @@ -91,9 +91,9 @@ pub enum KdfFunction { Pbkdf2, } -impl Into for KdfFunction { - fn into(self) -> String { - match self { +impl From for String { + fn from(from: KdfFunction) -> String { + match from { KdfFunction::Scrypt => "scrypt".into(), KdfFunction::Pbkdf2 => "pbkdf2".into(), } diff --git a/crypto/eth2_wallet/src/json_wallet/mod.rs b/crypto/eth2_wallet/src/json_wallet/mod.rs index d2092508a2..f9a4105911 100644 --- a/crypto/eth2_wallet/src/json_wallet/mod.rs +++ b/crypto/eth2_wallet/src/json_wallet/mod.rs @@ -39,9 +39,9 @@ pub enum TypeField { Hd, } -impl Into for TypeField { - fn into(self) -> String { - match self { +impl From for String { + fn from(from: TypeField) -> String { + match from { TypeField::Hd => "hierarchical deterministic".into(), } } diff --git a/crypto/kzg/src/kzg_proof.rs b/crypto/kzg/src/kzg_proof.rs index c9a138a31c..5a83466d0c 100644 --- a/crypto/kzg/src/kzg_proof.rs +++ b/crypto/kzg/src/kzg_proof.rs @@ -38,9 +38,9 @@ impl From<[u8; BYTES_PER_PROOF]> for KzgProof { } } -impl Into<[u8; BYTES_PER_PROOF]> for KzgProof { - fn into(self) -> [u8; BYTES_PER_PROOF] { - self.0 +impl From for [u8; BYTES_PER_PROOF] { + fn from(from: KzgProof) -> [u8; BYTES_PER_PROOF] { + from.0 } } diff --git a/validator_client/slashing_protection/src/lib.rs b/validator_client/slashing_protection/src/lib.rs index c4fa32b611..e5606d4042 100644 --- a/validator_client/slashing_protection/src/lib.rs +++ b/validator_client/slashing_protection/src/lib.rs @@ -67,9 +67,9 @@ impl From for SigningRoot { } } -impl Into for SigningRoot { - fn into(self) -> Hash256 { - self.0 +impl From for Hash256 { + fn from(from: SigningRoot) -> Hash256 { + from.0 } } From 806c3ce9e912b51002c9d4a9c514a9b0f4912277 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Wed, 19 Jun 2024 06:02:29 +0100 Subject: [PATCH 16/53] chore: simplify method to generate a random valid blob (#5946) * chore: simplify method to generate a random blob * chore: remove now unused import --- consensus/types/src/blob_sidecar.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index 50530543a5..1f60f429db 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -6,10 +6,7 @@ use crate::{ use crate::{KzgProofs, SignedBeaconBlock}; use bls::Signature; use derivative::Derivative; -use kzg::{ - Blob as KzgBlob, Kzg, KzgCommitment, KzgProof, BYTES_PER_BLOB, BYTES_PER_FIELD_ELEMENT, - FIELD_ELEMENTS_PER_BLOB, -}; +use kzg::{Blob as KzgBlob, Kzg, KzgCommitment, KzgProof, BYTES_PER_BLOB, BYTES_PER_FIELD_ELEMENT}; use merkle_proof::{merkle_root_from_branch, verify_merkle_proof, MerkleTreeError}; use rand::Rng; use safe_arith::ArithError; @@ -221,13 +218,7 @@ impl BlobSidecar { rng.fill_bytes(&mut blob_bytes); // Ensure that the blob is canonical by ensuring that // each field element contained in the blob is < BLS_MODULUS - for i in 0..FIELD_ELEMENTS_PER_BLOB { - let Some(byte) = blob_bytes.get_mut( - i.checked_mul(BYTES_PER_FIELD_ELEMENT) - .ok_or("overflow".to_string())?, - ) else { - return Err(format!("blob byte index out of bounds: {:?}", i)); - }; + for byte in blob_bytes.iter_mut().step_by(BYTES_PER_FIELD_ELEMENT) { *byte = 0; } From adb3f865ef78f3b6169925da1ed5322a3cf8cafb Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 19 Jun 2024 15:02:31 +1000 Subject: [PATCH 17/53] Lower tolerance of stale blobs on gossip (#5935) * Lower tolerance of stale blobs on gossip * Drop to debug --- .../gossip_methods.rs | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 99622bfa30..f178fa56c3 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -700,7 +700,7 @@ impl NetworkBeaconProcessor { "index" => %index, "commitment" => %commitment, ); - // Prevent recurring behaviour by penalizing the peer slightly. + // Prevent recurring behaviour by penalizing the peer. self.gossip_penalize_peer( peer_id, PeerAction::LowToleranceError, @@ -712,10 +712,8 @@ impl NetworkBeaconProcessor { MessageAcceptance::Reject, ); } - GossipBlobError::FutureSlot { .. } - | GossipBlobError::RepeatBlob { .. } - | GossipBlobError::PastFinalizedSlot { .. } => { - warn!( + GossipBlobError::FutureSlot { .. } | GossipBlobError::RepeatBlob { .. } => { + debug!( self.log, "Could not verify blob sidecar for gossip. Ignoring the blob sidecar"; "error" => ?err, @@ -736,6 +734,30 @@ impl NetworkBeaconProcessor { MessageAcceptance::Ignore, ); } + GossipBlobError::PastFinalizedSlot { .. } => { + debug!( + self.log, + "Could not verify blob sidecar for gossip. Ignoring the blob sidecar"; + "error" => ?err, + "slot" => %slot, + "root" => %root, + "index" => %index, + "commitment" => %commitment, + ); + // Prevent recurring behaviour by penalizing the peer. A low-tolerance + // error is fine because there's no reason for peers to be propagating old + // blobs on gossip, even if their view of finality is lagging. + self.gossip_penalize_peer( + peer_id, + PeerAction::LowToleranceError, + "gossip_blob_low", + ); + self.propagate_validation_result( + message_id, + peer_id, + MessageAcceptance::Ignore, + ); + } } } } From 1503f7d652d63483236c35e47ac8a3a28a73aad0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Oliveira?= Date: Wed, 19 Jun 2024 06:02:34 +0100 Subject: [PATCH 18/53] report failed requests from `RPC` after being clear from rate limiter but not yet sent. (#5942) * report failed rpc requests from RPC --- beacon_node/lighthouse_network/src/rpc/mod.rs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/beacon_node/lighthouse_network/src/rpc/mod.rs b/beacon_node/lighthouse_network/src/rpc/mod.rs index 1fa6862351..027af89edf 100644 --- a/beacon_node/lighthouse_network/src/rpc/mod.rs +++ b/beacon_node/lighthouse_network/src/rpc/mod.rs @@ -316,6 +316,27 @@ where self.events.push(error_msg); } } + + // Replace the pending Requests to the disconnected peer + // with reports of failed requests. + self.events.iter_mut().for_each(|event| match &event { + ToSwarm::NotifyHandler { + peer_id: p, + event: RPCSend::Request(request_id, req), + .. + } if *p == peer_id => { + *event = ToSwarm::GenerateEvent(RPCMessage { + peer_id, + conn_id: connection_id, + event: HandlerEvent::Err(HandlerErr::Outbound { + id: *request_id, + proto: req.versioned_protocol().protocol(), + error: RPCError::Disconnected, + }), + }); + } + _ => {} + }); } } From f69ccc3b7089fc02630ddc7b15552fcc5d98f522 Mon Sep 17 00:00:00 2001 From: Age Manning Date: Wed, 19 Jun 2024 15:02:37 +1000 Subject: [PATCH 19/53] Ensure all handler events are covered (#5945) * Ensure handler events are covered --- beacon_node/lighthouse_network/src/rpc/handler.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/beacon_node/lighthouse_network/src/rpc/handler.rs b/beacon_node/lighthouse_network/src/rpc/handler.rs index 48f69c64c5..b7166efc37 100644 --- a/beacon_node/lighthouse_network/src/rpc/handler.rs +++ b/beacon_node/lighthouse_network/src/rpc/handler.rs @@ -374,6 +374,12 @@ where id: outbound_info.req_id, }))); } + + // Also handle any events that are awaiting to be sent to the behaviour + if !self.events_out.is_empty() { + return Poll::Ready(Some(self.events_out.remove(0))); + } + Poll::Ready(None) } From d87541c0457a9cf2b89a83a3750b49a651ed2489 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 15:44:32 +0200 Subject: [PATCH 20/53] De-dup attestation constructor logic --- consensus/types/src/aggregate_and_proof.rs | 46 ++++++++++++------- .../types/src/signed_aggregate_and_proof.rs | 14 +++--- validator_client/src/validator_store.rs | 44 +++++------------- 3 files changed, 48 insertions(+), 56 deletions(-) diff --git a/consensus/types/src/aggregate_and_proof.rs b/consensus/types/src/aggregate_and_proof.rs index e8fa01c143..ae80369130 100644 --- a/consensus/types/src/aggregate_and_proof.rs +++ b/consensus/types/src/aggregate_and_proof.rs @@ -4,6 +4,7 @@ use super::{ SignedRoot, }; use crate::test_utils::TestRandom; +use crate::Attestation; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; use superstruct::superstruct; @@ -88,28 +89,39 @@ impl AggregateAndProof { genesis_validators_root: Hash256, spec: &ChainSpec, ) -> Self { - let selection_proof = selection_proof - .unwrap_or_else(|| { - SelectionProof::new::( - aggregate.data().slot, - secret_key, - fork, - genesis_validators_root, - spec, - ) - }) - .into(); + let selection_proof = selection_proof.unwrap_or_else(|| { + SelectionProof::new::( + aggregate.data().slot, + secret_key, + fork, + genesis_validators_root, + spec, + ) + }); + Self::from_attestation( + aggregator_index, + aggregate.clone_as_attestation(), + selection_proof, + ) + } + + /// Produces a new `AggregateAndProof` given a `selection_proof` + pub fn from_attestation( + aggregator_index: u64, + aggregate: Attestation, + selection_proof: SelectionProof, + ) -> Self { match aggregate { - AttestationRef::Base(attestation) => Self::Base(AggregateAndProofBase { + Attestation::Base(aggregate) => Self::Base(AggregateAndProofBase { aggregator_index, - aggregate: attestation.clone(), - selection_proof, + aggregate, + selection_proof: selection_proof.into(), }), - AttestationRef::Electra(attestation) => Self::Electra(AggregateAndProofElectra { + Attestation::Electra(aggregate) => Self::Electra(AggregateAndProofElectra { aggregator_index, - aggregate: attestation.clone(), - selection_proof, + aggregate, + selection_proof: selection_proof.into(), }), } } diff --git a/consensus/types/src/signed_aggregate_and_proof.rs b/consensus/types/src/signed_aggregate_and_proof.rs index ddf1dedb04..94624f02fe 100644 --- a/consensus/types/src/signed_aggregate_and_proof.rs +++ b/consensus/types/src/signed_aggregate_and_proof.rs @@ -83,17 +83,19 @@ impl SignedAggregateAndProof { ); let signing_message = message.signing_root(domain); - match message { + Self::from_aggregate_and_proof(message, secret_key.sign(signing_message)) + } + + /// Produces a new `SignedAggregateAndProof` given a `signature` of `aggregate` + pub fn from_aggregate_and_proof(aggregate: AggregateAndProof, signature: Signature) -> Self { + match aggregate { AggregateAndProof::Base(message) => { - SignedAggregateAndProof::Base(SignedAggregateAndProofBase { - message, - signature: secret_key.sign(signing_message), - }) + SignedAggregateAndProof::Base(SignedAggregateAndProofBase { message, signature }) } AggregateAndProof::Electra(message) => { SignedAggregateAndProof::Electra(SignedAggregateAndProofElectra { message, - signature: secret_key.sign(signing_message), + signature, }) } } diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index a173e5da12..e6e19b6e06 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -18,16 +18,12 @@ use std::sync::Arc; use task_executor::TaskExecutor; use types::{ attestation::Error as AttestationError, graffiti::GraffitiString, AbstractExecPayload, Address, - Attestation, BeaconBlock, BlindedPayload, ChainSpec, ContributionAndProof, Domain, Epoch, - EthSpec, Fork, ForkName, Graffiti, Hash256, PublicKeyBytes, SelectionProof, Signature, - SignedBeaconBlock, SignedContributionAndProof, SignedRoot, SignedValidatorRegistrationData, - SignedVoluntaryExit, Slot, SyncAggregatorSelectionData, SyncCommitteeContribution, - SyncCommitteeMessage, SyncSelectionProof, SyncSubnetId, ValidatorRegistrationData, - VoluntaryExit, -}; -use types::{ - AggregateAndProof, AggregateAndProofBase, AggregateAndProofElectra, SignedAggregateAndProof, - SignedAggregateAndProofBase, SignedAggregateAndProofElectra, + AggregateAndProof, Attestation, BeaconBlock, BlindedPayload, ChainSpec, ContributionAndProof, + Domain, Epoch, EthSpec, Fork, ForkName, Graffiti, Hash256, PublicKeyBytes, SelectionProof, + Signature, SignedAggregateAndProof, SignedBeaconBlock, SignedContributionAndProof, SignedRoot, + SignedValidatorRegistrationData, SignedVoluntaryExit, Slot, SyncAggregatorSelectionData, + SyncCommitteeContribution, SyncCommitteeMessage, SyncSelectionProof, SyncSubnetId, + ValidatorRegistrationData, VoluntaryExit, }; pub use crate::doppelganger_service::DoppelgangerStatus; @@ -805,18 +801,8 @@ impl ValidatorStore { let signing_epoch = aggregate.data().target.epoch; let signing_context = self.signing_context(Domain::AggregateAndProof, signing_epoch); - let message = match aggregate { - Attestation::Base(att) => AggregateAndProof::Base(AggregateAndProofBase { - aggregator_index, - aggregate: att, - selection_proof: selection_proof.into(), - }), - Attestation::Electra(att) => AggregateAndProof::Electra(AggregateAndProofElectra { - aggregator_index, - aggregate: att, - selection_proof: selection_proof.into(), - }), - }; + let message = + AggregateAndProof::from_attestation(aggregator_index, aggregate, selection_proof); let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?; let signature = signing_method @@ -830,17 +816,9 @@ impl ValidatorStore { metrics::inc_counter_vec(&metrics::SIGNED_AGGREGATES_TOTAL, &[metrics::SUCCESS]); - match message { - AggregateAndProof::Base(message) => { - Ok(SignedAggregateAndProof::Base(SignedAggregateAndProofBase { - message, - signature, - })) - } - AggregateAndProof::Electra(message) => Ok(SignedAggregateAndProof::Electra( - SignedAggregateAndProofElectra { message, signature }, - )), - } + Ok(SignedAggregateAndProof::from_aggregate_and_proof( + message, signature, + )) } /// Produces a `SelectionProof` for the `slot`, signed by with corresponding secret key to From dd0d5e2d9332c39d998b670bd26789e68d3e119a Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 16:17:04 +0200 Subject: [PATCH 21/53] Remove unwraps in Attestation construction --- consensus/types/src/attestation.rs | 31 ++++++++++++++++++ validator_client/src/attestation_service.rs | 36 ++++++++++----------- 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 3df14feade..f4d674fd78 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -1,4 +1,5 @@ use crate::slot_data::SlotData; +use crate::ForkName; use crate::{test_utils::TestRandom, Hash256, Slot}; use derivative::Derivative; use rand::RngCore; @@ -22,6 +23,8 @@ pub enum Error { AlreadySigned(usize), SubnetCountIsZero(ArithError), IncorrectStateVariant, + InvalidCommitteeLength, + InvalidCommitteeIndex, } #[superstruct( @@ -104,6 +107,34 @@ impl Hash for Attestation { } impl Attestation { + pub fn empty_for_signing( + committee_index: usize, + committee_length: usize, + data: AttestationData, + spec: &ChainSpec, + ) -> Result { + if spec.fork_name_at_slot::(data.slot) >= ForkName::Electra { + let mut committee_bits: BitVector = BitVector::default(); + committee_bits + .set(committee_index, true) + .map_err(|_| Error::InvalidCommitteeIndex)?; + Ok(Attestation::Electra(AttestationElectra { + aggregation_bits: BitList::with_capacity(committee_length) + .map_err(|_| Error::InvalidCommitteeLength)?, + data, + committee_bits, + signature: AggregateSignature::infinity(), + })) + } else { + Ok(Attestation::Base(AttestationBase { + aggregation_bits: BitList::with_capacity(committee_length) + .map_err(|_| Error::InvalidCommitteeLength)?, + data, + signature: AggregateSignature::infinity(), + })) + } + } + /// Aggregate another Attestation into this one. /// /// The aggregation bitfields must be disjoint, and the data must be the same. diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 404fe41249..a3ab7a4c72 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -386,25 +386,23 @@ impl AttestationService { return None; } - let mut attestation = if fork_name >= ForkName::Electra { - let mut committee_bits: BitVector = BitVector::default(); - committee_bits - .set(duty.committee_index as usize, true) - .unwrap(); - Attestation::Electra(AttestationElectra { - aggregation_bits: BitList::with_capacity(duty.committee_length as usize) - .unwrap(), - data: attestation_data.clone(), - committee_bits, - signature: AggregateSignature::infinity(), - }) - } else { - Attestation::Base(AttestationBase { - aggregation_bits: BitList::with_capacity(duty.committee_length as usize) - .unwrap(), - data: attestation_data.clone(), - signature: AggregateSignature::infinity(), - }) + let mut attestation = match Attestation::::empty_for_signing( + duty.committee_index as usize, + duty.committee_length as usize, + attestation_data.clone(), + &self.context.eth2_config.spec, + ) { + Ok(attestation) => attestation, + Err(err) => { + crit!( + log, + "Invalid validator duties during signing"; + "validator" => ?duty.pubkey, + "duty" => ?duty, + "err" => ?err, + ); + return None; + } }; match self From 3ec21a2435ae60bfbea28010507d0a74554c6b13 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 16:26:47 +0200 Subject: [PATCH 22/53] Dedup match_attestation_data --- common/eth2/src/types.rs | 15 +++++++++++ validator_client/src/attestation_service.rs | 30 +++------------------ 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index 2bb749af9f..c20e723ddd 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -716,6 +716,21 @@ pub struct AttesterData { pub slot: Slot, } +impl AttesterData { + pub fn match_attestation_data( + &self, + attestation_data: &AttestationData, + spec: &ChainSpec, + ) -> bool { + if spec.fork_name_at_slot::(attestation_data.slot) < ForkName::Electra { + self.slot == attestation_data.slot && self.committee_index == attestation_data.index + } else { + // After electra `attestation_data.index` is set to 0 and does not match the duties + self.slot == attestation_data.index + } + } +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct ProposerData { pub pubkey: PublicKeyBytes, diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index a3ab7a4c72..d08d7567af 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -14,11 +14,7 @@ use std::ops::Deref; use std::sync::Arc; use tokio::time::{sleep, sleep_until, Duration, Instant}; use tree_hash::TreeHash; -use types::ForkName; -use types::{ - attestation::AttestationBase, AggregateSignature, Attestation, AttestationData, - AttestationElectra, BitList, BitVector, ChainSpec, CommitteeIndex, EthSpec, Slot, -}; +use types::{Attestation, AttestationData, ChainSpec, CommitteeIndex, EthSpec, Slot}; /// Builds an `AttestationService`. pub struct AttestationServiceBuilder { @@ -363,17 +359,8 @@ impl AttestationService { let duty = &duty_and_proof.duty; let attestation_data = attestation_data_ref; - let fork_name = self - .context - .eth2_config - .spec - .fork_name_at_slot::(attestation_data.slot); - // Ensure that the attestation matches the duties. - #[allow(clippy::suspicious_operation_groupings)] - if duty.slot != attestation_data.slot - || (fork_name < ForkName::Electra && duty.committee_index != attestation_data.index) - { + if !duty.match_attestation_data::(attestation_data, &self.context.eth2_config.spec) { crit!( log, "Inconsistent validator duties during signing"; @@ -552,23 +539,12 @@ impl AttestationService { .await .map_err(|e| e.to_string())?; - let fork_name = self - .context - .eth2_config - .spec - .fork_name_at_slot::(attestation_data.slot); - // Create futures to produce the signed aggregated attestations. let signing_futures = validator_duties.iter().map(|duty_and_proof| async move { let duty = &duty_and_proof.duty; let selection_proof = duty_and_proof.selection_proof.as_ref()?; - let slot = attestation_data.slot; - let committee_index = attestation_data.index; - - if duty.slot != slot - || (fork_name < ForkName::Electra && duty.committee_index != committee_index) - { + if !duty.match_attestation_data::(attestation_data, &self.context.eth2_config.spec) { crit!(log, "Inconsistent validator duties during signing"); return None; } From 795eff9bf49fd58d2a707c10a07a9e1622c5df0d Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 17:58:26 +0200 Subject: [PATCH 23/53] Remove outdated TODO --- testing/ef_tests/src/cases/fork_choice.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index b3b20d6efe..fa8c13d07e 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -131,7 +131,6 @@ pub struct ForkChoiceTest { pub anchor_state: BeaconState, pub anchor_block: BeaconBlock, #[allow(clippy::type_complexity)] - // TODO(electra): these tests will need to be updated to use new types pub steps: Vec< Step, BlobsList, Attestation, AttesterSlashing, PowBlock>, >, From 960f8c5c48c708df1a123e045cf4fc17398fb2d3 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 17:58:38 +0200 Subject: [PATCH 24/53] Use ForkName Ord in fork-choice tests --- testing/ef_tests/src/cases/fork_choice.rs | 54 ++++++++++------------- 1 file changed, 23 insertions(+), 31 deletions(-) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index fa8c13d07e..d6c8b6302d 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -179,42 +179,34 @@ impl LoadCase for ForkChoiceTest { valid, }) } - Step::Attestation { attestation } => match fork_name { - ForkName::Base - | ForkName::Altair - | ForkName::Bellatrix - | ForkName::Capella - | ForkName::Deneb => ssz_decode_file( - &path.join(format!("{}.ssz_snappy", attestation)), - ) - .map(|attestation| Step::Attestation { - attestation: Attestation::Base(attestation), - }), - ForkName::Electra => ssz_decode_file( - &path.join(format!("{}.ssz_snappy", attestation)), - ) - .map(|attestation| Step::Attestation { - attestation: Attestation::Electra(attestation), - }), - }, - Step::AttesterSlashing { attester_slashing } => match fork_name { - ForkName::Base - | ForkName::Altair - | ForkName::Bellatrix - | ForkName::Capella - | ForkName::Deneb => { + Step::Attestation { attestation } => { + if fork_name >= ForkName::Electra { + ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))).map( + |attestation| Step::Attestation { + attestation: Attestation::Electra(attestation), + }, + ) + } else { + ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))).map( + |attestation| Step::Attestation { + attestation: Attestation::Base(attestation), + }, + ) + } + } + Step::AttesterSlashing { attester_slashing } => { + if fork_name >= ForkName::Electra { + ssz_decode_file(&path.join(format!("{}.ssz_snappy", attester_slashing))) + .map(|attester_slashing| Step::AttesterSlashing { + attester_slashing: AttesterSlashing::Electra(attester_slashing), + }) + } else { ssz_decode_file(&path.join(format!("{}.ssz_snappy", attester_slashing))) .map(|attester_slashing| Step::AttesterSlashing { attester_slashing: AttesterSlashing::Base(attester_slashing), }) } - ForkName::Electra => { - ssz_decode_file(&path.join(format!("{}.ssz_snappy", attester_slashing))) - .map(|attester_slashing| Step::AttesterSlashing { - attester_slashing: AttesterSlashing::Electra(attester_slashing), - }) - } - }, + } Step::PowBlock { pow_block } => { ssz_decode_file(&path.join(format!("{}.ssz_snappy", pow_block))) .map(|pow_block| Step::PowBlock { pow_block }) From 1d0e3f4d30ae31253f3f9b45822b18731ac9032a Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 19:18:54 +0200 Subject: [PATCH 25/53] Use ForkName Ord in BeaconBlockBody --- consensus/types/src/beacon_block_body.rs | 27 ++++++++++++------------ 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/consensus/types/src/beacon_block_body.rs b/consensus/types/src/beacon_block_body.rs index 76a2fc1872..9cf66184f8 100644 --- a/consensus/types/src/beacon_block_body.rs +++ b/consensus/types/src/beacon_block_body.rs @@ -820,6 +820,11 @@ impl From>> } impl BeaconBlockBody { + /// Returns the name of the fork pertaining to `self`. + pub fn fork_name(&self) -> ForkName { + self.to_ref().fork_name() + } + pub fn block_body_merkle_proof(&self, generalized_index: usize) -> Result, Error> { let field_index = match generalized_index { light_client_update::EXECUTION_PAYLOAD_INDEX => { @@ -834,22 +839,16 @@ impl BeaconBlockBody { _ => return Err(Error::IndexNotSupported(generalized_index)), }; - let attestations_root = match self { - BeaconBlockBody::Base(_) - | BeaconBlockBody::Altair(_) - | BeaconBlockBody::Bellatrix(_) - | BeaconBlockBody::Capella(_) - | BeaconBlockBody::Deneb(_) => self.attestations_base()?.tree_hash_root(), - BeaconBlockBody::Electra(_) => self.attestations_electra()?.tree_hash_root(), + let attestations_root = if self.fork_name() > ForkName::Electra { + self.attestations_electra()?.tree_hash_root() + } else { + self.attestations_base()?.tree_hash_root() }; - let attester_slashings_root = match self { - BeaconBlockBody::Base(_) - | BeaconBlockBody::Altair(_) - | BeaconBlockBody::Bellatrix(_) - | BeaconBlockBody::Capella(_) - | BeaconBlockBody::Deneb(_) => self.attester_slashings_base()?.tree_hash_root(), - BeaconBlockBody::Electra(_) => self.attester_slashings_electra()?.tree_hash_root(), + let attester_slashings_root = if self.fork_name() > ForkName::Electra { + self.attester_slashings_electra()?.tree_hash_root() + } else { + self.attester_slashings_base()?.tree_hash_root() }; let mut leaves = vec![ From 5acc0523dfb61ece807814ffe2260a8b5eabf760 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 18:18:44 +0200 Subject: [PATCH 26/53] Make to_electra not fallible --- consensus/types/src/indexed_attestation.rs | 10 ++++---- slasher/src/array.rs | 20 ++-------------- slasher/src/lib.rs | 26 ++++++-------------- slasher/src/slasher.rs | 28 +++++++--------------- slasher/src/test_utils.rs | 4 ++-- 5 files changed, 25 insertions(+), 63 deletions(-) diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index d282e2f259..ecddcd2179 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -116,11 +116,13 @@ impl IndexedAttestation { } } - pub fn to_electra(self) -> Result, ssz_types::Error> { - Ok(match self { + pub fn to_electra(self) -> IndexedAttestationElectra { + match self { Self::Base(att) => { let extended_attesting_indices: VariableList = - VariableList::new(att.attesting_indices.to_vec())?; + VariableList::new(att.attesting_indices.to_vec()) + .expect("MaxValidatorsPerSlot must be >= MaxValidatorsPerCommittee"); + // TODO: Add test after unstable rebase https://github.com/sigp/lighthouse/blob/474c1b44863927c588dd05ab2ac0f934298398e1/consensus/types/src/eth_spec.rs#L541 IndexedAttestationElectra { attesting_indices: extended_attesting_indices, @@ -129,7 +131,7 @@ impl IndexedAttestation { } } Self::Electra(att) => att, - }) + } } } diff --git a/slasher/src/array.rs b/slasher/src/array.rs index 714ccf4e77..77ddceb85f 100644 --- a/slasher/src/array.rs +++ b/slasher/src/array.rs @@ -5,7 +5,6 @@ use crate::{ }; use flate2::bufread::{ZlibDecoder, ZlibEncoder}; use serde::{Deserialize, Serialize}; -use slog::Logger; use std::borrow::Borrow; use std::collections::{btree_map::Entry, BTreeMap, HashSet}; use std::io::Read; @@ -486,7 +485,6 @@ pub fn update( batch: Vec>>, current_epoch: Epoch, config: &Config, - log: &Logger, ) -> Result>, Error> { // Split the batch up into horizontal segments. // Map chunk indexes in the range `0..self.config.chunk_size` to attestations @@ -506,7 +504,6 @@ pub fn update( &chunk_attestations, current_epoch, config, - log, )?; slashings.extend(update_array::<_, MaxTargetChunk>( db, @@ -515,7 +512,6 @@ pub fn update( &chunk_attestations, current_epoch, config, - log, )?); // Update all current epochs. @@ -574,7 +570,6 @@ pub fn update_array( chunk_attestations: &BTreeMap>>>, current_epoch: Epoch, config: &Config, - log: &Logger, ) -> Result>, Error> { let mut slashings = HashSet::new(); // Map from chunk index to updated chunk at that index. @@ -608,19 +603,8 @@ pub fn update_array( current_epoch, config, )?; - match slashing_status.into_slashing(&attestation.indexed) { - Ok(Some(slashing)) => { - slashings.insert(slashing); - } - Err(e) => { - slog::error!( - log, - "Invalid slashing conversion"; - "validator_index" => validator_index, - "error" => e - ); - } - Ok(None) => {} + if let Some(slashing) = slashing_status.into_slashing(&attestation.indexed) { + slashings.insert(slashing); } } } diff --git a/slasher/src/lib.rs b/slasher/src/lib.rs index 0c097ac77f..4d58fa7702 100644 --- a/slasher/src/lib.rs +++ b/slasher/src/lib.rs @@ -53,11 +53,11 @@ impl AttesterSlashingStatus { pub fn into_slashing( self, new_attestation: &IndexedAttestation, - ) -> Result>, String> { + ) -> Option> { use AttesterSlashingStatus::*; // The surrounding attestation must be in `attestation_1` to be valid. - Ok(match self { + match self { NotSlashable => None, AlreadyDoubleVoted => None, DoubleVote(existing) | SurroundedByExisting(existing) => { @@ -70,14 +70,8 @@ impl AttesterSlashingStatus { } // A slashing involving an electra attestation type must return an `AttesterSlashingElectra` type (_, _) => Some(AttesterSlashing::Electra(AttesterSlashingElectra { - attestation_1: existing - .clone() - .to_electra() - .map_err(|e| format!("{e:?}"))?, - attestation_2: new_attestation - .clone() - .to_electra() - .map_err(|e| format!("{e:?}"))?, + attestation_1: existing.clone().to_electra(), + attestation_2: new_attestation.clone().to_electra(), })), } } @@ -90,16 +84,10 @@ impl AttesterSlashingStatus { } // A slashing involving an electra attestation type must return an `AttesterSlashingElectra` type (_, _) => Some(AttesterSlashing::Electra(AttesterSlashingElectra { - attestation_1: new_attestation - .clone() - .to_electra() - .map_err(|e| format!("{e:?}"))?, - attestation_2: existing - .clone() - .to_electra() - .map_err(|e| format!("{e:?}"))?, + attestation_1: new_attestation.clone().to_electra(), + attestation_2: existing.clone().to_electra(), })), }, - }) + } } } diff --git a/slasher/src/slasher.rs b/slasher/src/slasher.rs index ef6dcce9b1..fc8e8453c8 100644 --- a/slasher/src/slasher.rs +++ b/slasher/src/slasher.rs @@ -247,7 +247,6 @@ impl Slasher { batch, current_epoch, &self.config, - &self.log, ) { Ok(slashings) => { if !slashings.is_empty() { @@ -295,25 +294,14 @@ impl Slasher { indexed_attestation_id, )?; - match slashing_status.into_slashing(attestation) { - Ok(Some(slashing)) => { - debug!( - self.log, - "Found double-vote slashing"; - "validator_index" => validator_index, - "epoch" => slashing.attestation_1().data().target.epoch, - ); - slashings.insert(slashing); - } - Err(e) => { - error!( - self.log, - "Invalid slashing conversion"; - "validator_index" => validator_index, - "error" => e - ); - } - Ok(None) => {} + if let Some(slashing) = slashing_status.into_slashing(attestation) { + debug!( + self.log, + "Found double-vote slashing"; + "validator_index" => validator_index, + "epoch" => slashing.attestation_1().data().target.epoch, + ); + slashings.insert(slashing); } } diff --git a/slasher/src/test_utils.rs b/slasher/src/test_utils.rs index ef623911bf..7019a8aed7 100644 --- a/slasher/src/test_utils.rs +++ b/slasher/src/test_utils.rs @@ -71,8 +71,8 @@ pub fn att_slashing( } // A slashing involving an electra attestation type must return an electra AttesterSlashing type (_, _) => AttesterSlashing::Electra(AttesterSlashingElectra { - attestation_1: attestation_1.clone().to_electra().unwrap(), - attestation_2: attestation_2.clone().to_electra().unwrap(), + attestation_1: attestation_1.clone().to_electra(), + attestation_2: attestation_2.clone().to_electra(), }), } } From 4f08f6e0da46aefc00a5be47465323395f45b08f Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 18:34:54 +0200 Subject: [PATCH 27/53] Remove TestRandom impl for IndexedAttestation --- consensus/types/src/indexed_attestation.rs | 30 ++++++++-------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index ecddcd2179..1af2512da8 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -1,7 +1,6 @@ use crate::{test_utils::TestRandom, AggregateSignature, AttestationData, EthSpec, VariableList}; use core::slice::Iter; use derivative::Derivative; -use rand::RngCore; use serde::{Deserialize, Serialize}; use ssz::Decode; use ssz::Encode; @@ -208,23 +207,6 @@ impl Decode for IndexedAttestation { } } -// TODO(electra): think about how to handle fork variants here -impl TestRandom for IndexedAttestation { - fn random_for_test(rng: &mut impl RngCore) -> Self { - let attesting_indices = VariableList::random_for_test(rng); - // let committee_bits: BitList = BitList::random_for_test(rng); - let data = AttestationData::random_for_test(rng); - let signature = AggregateSignature::random_for_test(rng); - - Self::Base(IndexedAttestationBase { - attesting_indices, - // committee_bits, - data, - signature, - }) - } -} - /// Implementation of non-crypto-secure `Hash`, for use with `HashMap` and `HashSet`. /// /// Guarantees `att1 == att2 -> hash(att1) == hash(att2)`. @@ -333,14 +315,22 @@ mod tests { assert!(!indexed_vote_first.is_surround_vote(&indexed_vote_second)); } - ssz_and_tree_hash_tests!(IndexedAttestation); + mod base { + use super::*; + ssz_and_tree_hash_tests!(IndexedAttestationBase); + } + mod electra { + use super::*; + ssz_and_tree_hash_tests!(IndexedAttestationElectra); + } fn create_indexed_attestation( target_epoch: u64, source_epoch: u64, ) -> IndexedAttestation { let mut rng = XorShiftRng::from_seed([42; 16]); - let mut indexed_vote = IndexedAttestation::random_for_test(&mut rng); + let mut indexed_vote = + IndexedAttestation::Base(IndexedAttestationBase::random_for_test(&mut rng)); indexed_vote.data_mut().source.epoch = Epoch::new(source_epoch); indexed_vote.data_mut().target.epoch = Epoch::new(target_epoch); From f0492852f3a4cb3ce468275760b7c25935ac1aa9 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 19:01:06 +0200 Subject: [PATCH 28/53] Remove IndexedAttestation faulty Decode impl --- consensus/types/src/indexed_attestation.rs | 21 --------------------- slasher/src/database.rs | 9 +++++++-- 2 files changed, 7 insertions(+), 23 deletions(-) diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index 1af2512da8..00cac09abf 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -2,7 +2,6 @@ use crate::{test_utils::TestRandom, AggregateSignature, AttestationData, EthSpec use core::slice::Iter; use derivative::Derivative; use serde::{Deserialize, Serialize}; -use ssz::Decode; use ssz::Encode; use ssz_derive::{Decode, Encode}; use std::hash::{Hash, Hasher}; @@ -187,26 +186,6 @@ impl<'a, E: EthSpec> IndexedAttestationRef<'a, E> { } } -impl Decode for IndexedAttestation { - fn is_ssz_fixed_len() -> bool { - false - } - - fn from_ssz_bytes(bytes: &[u8]) -> Result { - if let Ok(result) = IndexedAttestationBase::from_ssz_bytes(bytes) { - return Ok(IndexedAttestation::Base(result)); - } - - if let Ok(result) = IndexedAttestationElectra::from_ssz_bytes(bytes) { - return Ok(IndexedAttestation::Electra(result)); - } - - Err(ssz::DecodeError::BytesInvalid(String::from( - "bytes not valid for any fork variant", - ))) - } -} - /// Implementation of non-crypto-secure `Hash`, for use with `HashMap` and `HashSet`. /// /// Guarantees `att1 == att2 -> hash(att1) == hash(att2)`. diff --git a/slasher/src/database.rs b/slasher/src/database.rs index d80112c553..16a67cd684 100644 --- a/slasher/src/database.rs +++ b/slasher/src/database.rs @@ -18,7 +18,8 @@ use std::marker::PhantomData; use std::sync::Arc; use tree_hash::TreeHash; use types::{ - Epoch, EthSpec, Hash256, IndexedAttestation, ProposerSlashing, SignedBeaconBlockHeader, Slot, + Epoch, EthSpec, Hash256, IndexedAttestation, IndexedAttestationBase, ProposerSlashing, + SignedBeaconBlockHeader, Slot, }; /// Current database schema version, to check compatibility of on-disk DB with software. @@ -481,7 +482,11 @@ impl SlasherDB { .ok_or(Error::MissingIndexedAttestation { id: indexed_attestation_id.as_u64(), })?; - ssz_decode(bytes) + // TODO(electra): make slasher fork-aware and return correct variant of attestation. Both + // Base and Electra variant can the same SSZ encoding, however the smaller list maximum of + // Base can error with an Electra attestation with heavy participation. + let indexed_attestation: IndexedAttestationBase = ssz_decode(bytes)?; + Ok(IndexedAttestation::Base(indexed_attestation)) } fn get_attestation_data_root( From 5070ab254dd70b6de19a43669691529df6a68915 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 19:56:12 +0200 Subject: [PATCH 29/53] Drop TestRandom impl --- consensus/types/src/attestation.rs | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index f4d674fd78..21e0a7ccd8 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -2,7 +2,6 @@ use crate::slot_data::SlotData; use crate::ForkName; use crate::{test_utils::TestRandom, Hash256, Slot}; use derivative::Derivative; -use rand::RngCore; use safe_arith::ArithError; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; @@ -77,23 +76,6 @@ pub struct Attestation { pub signature: AggregateSignature, } -// TODO(electra): think about how to handle fork variants here -impl TestRandom for Attestation { - fn random_for_test(rng: &mut impl RngCore) -> Self { - let aggregation_bits = BitList::random_for_test(rng); - let data = AttestationData::random_for_test(rng); - let signature = AggregateSignature::random_for_test(rng); - let committee_bits = BitVector::random_for_test(rng); - - Self::Electra(AttestationElectra { - aggregation_bits, - committee_bits, - data, - signature, - }) - } -} - impl Hash for Attestation { fn hash(&self, state: &mut H) where From 45d007a71f80cac6fd393d1de9b3cc2bd4e1fe04 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 20:01:19 +0200 Subject: [PATCH 30/53] Add PendingAttestationInElectra --- .../src/per_block_processing/errors.rs | 1 + .../process_operations.rs | 49 +++++++++---------- 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/consensus/state_processing/src/per_block_processing/errors.rs b/consensus/state_processing/src/per_block_processing/errors.rs index 336895514f..71a284dea4 100644 --- a/consensus/state_processing/src/per_block_processing/errors.rs +++ b/consensus/state_processing/src/per_block_processing/errors.rs @@ -89,6 +89,7 @@ pub enum BlockProcessingError { found: Hash256, }, WithdrawalCredentialsInvalid, + PendingAttestationInElectra, } impl From for BlockProcessingError { diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index 9b4e9d7139..bd354901a8 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -76,33 +76,30 @@ pub mod base { ) .map_err(|e| e.into_with_index(i))?; - match attestation { - AttestationRef::Base(att) => { - let pending_attestation = PendingAttestation { - aggregation_bits: att.aggregation_bits.clone(), - data: att.data.clone(), - inclusion_delay: state.slot().safe_sub(att.data.slot)?.as_u64(), - proposer_index, - }; - - if attestation.data().target.epoch == state.current_epoch() { - state - .as_base_mut()? - .current_epoch_attestations - .push(pending_attestation)?; - } else { - state - .as_base_mut()? - .previous_epoch_attestations - .push(pending_attestation)?; - } - } - AttestationRef::Electra(_) => { - // TODO(electra) pending attestations are only phase 0 - // so we should just raise a relevant error here - todo!() - } + let AttestationRef::Base(attestation) = attestation else { + // Pending attestations have been deprecated in a altair, this branch should + // never happen + return Err(BlockProcessingError::PendingAttestationInElectra); }; + + let pending_attestation = PendingAttestation { + aggregation_bits: attestation.aggregation_bits.clone(), + data: attestation.data.clone(), + inclusion_delay: state.slot().safe_sub(attestation.data.slot)?.as_u64(), + proposer_index, + }; + + if attestation.data.target.epoch == state.current_epoch() { + state + .as_base_mut()? + .current_epoch_attestations + .push(pending_attestation)?; + } else { + state + .as_base_mut()? + .previous_epoch_attestations + .push(pending_attestation)?; + } } Ok(()) From 9e84779522633f544b314aadbcffe3af5f38c399 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Tue, 18 Jun 2024 11:10:32 -0400 Subject: [PATCH 31/53] Indexed att on disk (#35) * indexed att on disk * fix lints * Update slasher/src/migrate.rs Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> --------- Co-authored-by: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> --- consensus/types/src/indexed_attestation.rs | 32 ++++++++++++++++++++++ consensus/types/src/lib.rs | 3 +- slasher/src/database.rs | 17 ++++++------ slasher/src/migrate.rs | 4 +++ 4 files changed, 46 insertions(+), 10 deletions(-) diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index 00cac09abf..429d65898a 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -239,6 +239,38 @@ mod quoted_variable_list_u64 { } } +#[derive(Debug, Clone, Encode, Decode, PartialEq)] +#[ssz(enum_behaviour = "union")] +pub enum IndexedAttestationOnDisk { + Base(IndexedAttestationBase), + Electra(IndexedAttestationElectra), +} + +#[derive(Debug, Clone, Encode, PartialEq)] +#[ssz(enum_behaviour = "union")] +pub enum IndexedAttestationRefOnDisk<'a, E: EthSpec> { + Base(&'a IndexedAttestationBase), + Electra(&'a IndexedAttestationElectra), +} + +impl<'a, E: EthSpec> From<&'a IndexedAttestation> for IndexedAttestationRefOnDisk<'a, E> { + fn from(attestation: &'a IndexedAttestation) -> Self { + match attestation { + IndexedAttestation::Base(attestation) => Self::Base(attestation), + IndexedAttestation::Electra(attestation) => Self::Electra(attestation), + } + } +} + +impl From> for IndexedAttestation { + fn from(attestation: IndexedAttestationOnDisk) -> Self { + match attestation { + IndexedAttestationOnDisk::Base(attestation) => Self::Base(attestation), + IndexedAttestationOnDisk::Electra(attestation) => Self::Electra(attestation), + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 75456446bf..2a73ff0f37 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -176,7 +176,8 @@ pub use crate::fork_versioned_response::{ForkVersionDeserialize, ForkVersionedRe pub use crate::graffiti::{Graffiti, GRAFFITI_BYTES_LEN}; pub use crate::historical_batch::HistoricalBatch; pub use crate::indexed_attestation::{ - IndexedAttestation, IndexedAttestationBase, IndexedAttestationElectra, IndexedAttestationRef, + IndexedAttestation, IndexedAttestationBase, IndexedAttestationElectra, + IndexedAttestationOnDisk, IndexedAttestationRef, IndexedAttestationRefOnDisk, }; pub use crate::light_client_bootstrap::{ LightClientBootstrap, LightClientBootstrapAltair, LightClientBootstrapCapella, diff --git a/slasher/src/database.rs b/slasher/src/database.rs index 16a67cd684..16089706a0 100644 --- a/slasher/src/database.rs +++ b/slasher/src/database.rs @@ -18,12 +18,12 @@ use std::marker::PhantomData; use std::sync::Arc; use tree_hash::TreeHash; use types::{ - Epoch, EthSpec, Hash256, IndexedAttestation, IndexedAttestationBase, ProposerSlashing, - SignedBeaconBlockHeader, Slot, + Epoch, EthSpec, Hash256, IndexedAttestation, IndexedAttestationOnDisk, + IndexedAttestationRefOnDisk, ProposerSlashing, SignedBeaconBlockHeader, Slot, }; /// Current database schema version, to check compatibility of on-disk DB with software. -pub const CURRENT_SCHEMA_VERSION: u64 = 3; +pub const CURRENT_SCHEMA_VERSION: u64 = 4; /// Metadata about the slashing database itself. const METADATA_DB: &str = "metadata"; @@ -458,7 +458,9 @@ impl SlasherDB { }; let attestation_key = IndexedAttestationId::new(indexed_att_id); - let data = indexed_attestation.as_ssz_bytes(); + let indexed_attestation_on_disk: IndexedAttestationRefOnDisk = + indexed_attestation.into(); + let data = indexed_attestation_on_disk.as_ssz_bytes(); cursor.put(attestation_key.as_ref(), &data)?; drop(cursor); @@ -482,11 +484,8 @@ impl SlasherDB { .ok_or(Error::MissingIndexedAttestation { id: indexed_attestation_id.as_u64(), })?; - // TODO(electra): make slasher fork-aware and return correct variant of attestation. Both - // Base and Electra variant can the same SSZ encoding, however the smaller list maximum of - // Base can error with an Electra attestation with heavy participation. - let indexed_attestation: IndexedAttestationBase = ssz_decode(bytes)?; - Ok(IndexedAttestation::Base(indexed_attestation)) + let indexed_attestation: IndexedAttestationOnDisk = ssz_decode(bytes)?; + Ok(indexed_attestation.into()) } fn get_attestation_data_root( diff --git a/slasher/src/migrate.rs b/slasher/src/migrate.rs index 674ab9c132..ce298caaf2 100644 --- a/slasher/src/migrate.rs +++ b/slasher/src/migrate.rs @@ -17,6 +17,10 @@ impl SlasherDB { software_schema_version: CURRENT_SCHEMA_VERSION, }), (x, y) if x == y => Ok(self), + (3, 4) => { + // TODO(electra): db migration due to `IndexedAttestationOnDisk` + Ok(self) + } (_, _) => Err(Error::IncompatibleSchemaVersion { database_schema_version: schema_version, software_schema_version: CURRENT_SCHEMA_VERSION, From 7af3f2eb35d09aa12424f6f0bb641322eeba01b5 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 18 Jun 2024 22:50:07 +0200 Subject: [PATCH 32/53] add electra fork enabled fn to ForkName impl (#36) * add electra fork enabled fn to ForkName impl * remove inadvertent file --- .../beacon_chain/src/attestation_verification.rs | 5 ++++- beacon_node/beacon_chain/src/beacon_chain.rs | 6 +++++- beacon_node/beacon_chain/src/early_attester_cache.rs | 2 +- beacon_node/beacon_chain/src/test_utils.rs | 12 ++++++------ beacon_node/beacon_chain/tests/block_verification.rs | 2 +- beacon_node/beacon_chain/tests/store_tests.rs | 2 +- beacon_node/client/src/notifier.rs | 2 +- beacon_node/operation_pool/src/lib.rs | 2 +- consensus/types/src/attestation.rs | 3 +-- consensus/types/src/chain_spec.rs | 2 +- consensus/types/src/fork_name.rs | 4 ++++ consensus/types/src/validator.rs | 6 +++--- testing/ef_tests/src/cases/fork_choice.rs | 4 ++-- 13 files changed, 31 insertions(+), 21 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index f2e3565ae6..6eb0c6ecf9 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -1341,7 +1341,10 @@ pub fn verify_committee_index( attestation: AttestationRef, spec: &ChainSpec, ) -> Result<(), Error> { - if spec.fork_name_at_slot::(attestation.data().slot) >= ForkName::Electra { + if spec + .fork_name_at_slot::(attestation.data().slot) + .electra_enabled() + { // Check to ensure that the attestation is for a single committee. let num_committee_bits = get_committee_indices::( attestation diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index abe14796ce..eeb1457954 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1994,7 +1994,11 @@ impl BeaconChain { }; drop(cache_timer); - if self.spec.fork_name_at_slot::(request_slot) >= ForkName::Electra { + if self + .spec + .fork_name_at_slot::(request_slot) + .electra_enabled() + { let mut committee_bits = BitVector::default(); if committee_len > 0 { committee_bits.set(request_index as usize, true)?; diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 936f4de3ee..41a225d9f2 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -123,7 +123,7 @@ impl EarlyAttesterCache { item.committee_lengths .get_committee_length::(request_slot, request_index, spec)?; - let attestation = if spec.fork_name_at_slot::(request_slot) >= ForkName::Electra { + let attestation = if spec.fork_name_at_slot::(request_slot).electra_enabled() { let mut committee_bits = BitVector::default(); if committee_len > 0 { committee_bits diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 34ad6d4324..7d443db380 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1033,7 +1033,7 @@ where *state.get_block_root(target_slot)? }; - if self.spec.fork_name_at_slot::(slot) >= ForkName::Electra { + if self.spec.fork_name_at_slot::(slot).electra_enabled() { let mut committee_bits = BitVector::default(); committee_bits.set(index as usize, true)?; Ok(Attestation::Electra(AttestationElectra { @@ -1376,7 +1376,7 @@ where let fork_name = self.spec.fork_name_at_slot::(slot); - let aggregate = if fork_name >= ForkName::Electra { + let aggregate = if fork_name.electra_enabled() { self.chain .get_aggregated_attestation_electra( slot, @@ -1541,7 +1541,7 @@ where let fork_name = self.spec.fork_name_at_slot::(Slot::new(0)); - let mut attestation_1 = if fork_name >= ForkName::Electra { + let mut attestation_1 = if fork_name.electra_enabled() { IndexedAttestation::Electra(IndexedAttestationElectra { attesting_indices: VariableList::new(validator_indices).unwrap(), data: AttestationData { @@ -1623,7 +1623,7 @@ where } } - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { AttesterSlashing::Electra(AttesterSlashingElectra { attestation_1: attestation_1.as_electra().unwrap().clone(), attestation_2: attestation_2.as_electra().unwrap().clone(), @@ -1657,7 +1657,7 @@ where }, }; - let (mut attestation_1, mut attestation_2) = if fork_name >= ForkName::Electra { + let (mut attestation_1, mut attestation_2) = if fork_name.electra_enabled() { let attestation_1 = IndexedAttestationElectra { attesting_indices: VariableList::new(validator_indices_1).unwrap(), data: data.clone(), @@ -1735,7 +1735,7 @@ where } } - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { AttesterSlashing::Electra(AttesterSlashingElectra { attestation_1: attestation_1.as_electra().unwrap().clone(), attestation_2: attestation_2.as_electra().unwrap().clone(), diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 0fe406a017..55b2c14a3c 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -668,7 +668,7 @@ async fn invalid_signature_attester_slashing() { let mut snapshots = chain_segment.clone(); let fork_name = harness.chain.spec.fork_name_at_slot::(Slot::new(0)); - let attester_slashing = if fork_name >= ForkName::Electra { + let attester_slashing = if fork_name.electra_enabled() { let indexed_attestation = IndexedAttestationElectra { attesting_indices: vec![0].into(), data: AttestationData { diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index ef87390930..c2468102ee 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -1025,7 +1025,7 @@ async fn multiple_attestations_per_block() { let slot = snapshot.beacon_block.slot(); let fork_name = harness.chain.spec.fork_name_at_slot::(slot); - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { assert_eq!( snapshot .beacon_block diff --git a/beacon_node/client/src/notifier.rs b/beacon_node/client/src/notifier.rs index 7c224671ff..632188014e 100644 --- a/beacon_node/client/src/notifier.rs +++ b/beacon_node/client/src/notifier.rs @@ -551,7 +551,7 @@ async fn electra_readiness_logging( .snapshot .beacon_state .fork_name_unchecked() - >= ForkName::Electra; + .electra_enabled(); let has_execution_layer = beacon_chain.execution_layer.is_some(); diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index f9021bb258..49fbed5686 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -286,7 +286,7 @@ impl OperationPool { // TODO(electra): Work out how to do this more elegantly. This is a bit of a hack. let mut all_attestations = self.attestations.write(); - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { all_attestations.aggregate_across_committees(prev_epoch_key); all_attestations.aggregate_across_committees(curr_epoch_key); } diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 21e0a7ccd8..35bd80fc33 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -1,5 +1,4 @@ use crate::slot_data::SlotData; -use crate::ForkName; use crate::{test_utils::TestRandom, Hash256, Slot}; use derivative::Derivative; use safe_arith::ArithError; @@ -95,7 +94,7 @@ impl Attestation { data: AttestationData, spec: &ChainSpec, ) -> Result { - if spec.fork_name_at_slot::(data.slot) >= ForkName::Electra { + if spec.fork_name_at_slot::(data.slot).electra_enabled() { let mut committee_bits: BitVector = BitVector::default(); committee_bits .set(committee_index, true) diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index b0346a14ef..7609e36035 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -376,7 +376,7 @@ impl ChainSpec { state: &BeaconState, ) -> u64 { let fork_name = state.fork_name_unchecked(); - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { self.min_slashing_penalty_quotient_electra } else if fork_name >= ForkName::Bellatrix { self.min_slashing_penalty_quotient_bellatrix diff --git a/consensus/types/src/fork_name.rs b/consensus/types/src/fork_name.rs index 5cc6621473..f0810e2bdb 100644 --- a/consensus/types/src/fork_name.rs +++ b/consensus/types/src/fork_name.rs @@ -119,6 +119,10 @@ impl ForkName { ForkName::Electra => None, } } + + pub fn electra_enabled(self) -> bool { + self >= ForkName::Electra + } } /// Map a fork name into a fork-versioned superstruct type like `BeaconBlock`. diff --git a/consensus/types/src/validator.rs b/consensus/types/src/validator.rs index ba99a0a68d..f2b36ee153 100644 --- a/consensus/types/src/validator.rs +++ b/consensus/types/src/validator.rs @@ -63,7 +63,7 @@ impl Validator { spec: &ChainSpec, current_fork: ForkName, ) -> bool { - if current_fork >= ForkName::Electra { + if current_fork.electra_enabled() { self.is_eligible_for_activation_queue_electra(spec) } else { self.is_eligible_for_activation_queue_base(spec) @@ -162,7 +162,7 @@ impl Validator { spec: &ChainSpec, current_fork: ForkName, ) -> bool { - if current_fork >= ForkName::Electra { + if current_fork.electra_enabled() { self.is_fully_withdrawable_at_electra(balance, epoch, spec) } else { self.is_fully_withdrawable_at_capella(balance, epoch, spec) @@ -202,7 +202,7 @@ impl Validator { spec: &ChainSpec, current_fork: ForkName, ) -> bool { - if current_fork >= ForkName::Electra { + if current_fork.electra_enabled() { self.is_partially_withdrawable_validator_electra(balance, spec) } else { self.is_partially_withdrawable_validator_capella(balance, spec) diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index d6c8b6302d..2a2cc067e5 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -180,7 +180,7 @@ impl LoadCase for ForkChoiceTest { }) } Step::Attestation { attestation } => { - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))).map( |attestation| Step::Attestation { attestation: Attestation::Electra(attestation), @@ -195,7 +195,7 @@ impl LoadCase for ForkChoiceTest { } } Step::AttesterSlashing { attester_slashing } => { - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { ssz_decode_file(&path.join(format!("{}.ssz_snappy", attester_slashing))) .map(|attester_slashing| Step::AttesterSlashing { attester_slashing: AttesterSlashing::Electra(attester_slashing), From 2634a1f1a6c45d4e75a89e884c762be4534858d9 Mon Sep 17 00:00:00 2001 From: Lion - dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 22:50:34 +0200 Subject: [PATCH 33/53] Update common/eth2/src/types.rs Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> --- common/eth2/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index c20e723ddd..c945e09619 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -726,7 +726,7 @@ impl AttesterData { self.slot == attestation_data.slot && self.committee_index == attestation_data.index } else { // After electra `attestation_data.index` is set to 0 and does not match the duties - self.slot == attestation_data.index + self.slot == attestation_data.slot } } } From dec7cff9c76ddaac4f0ac0613cafafa3625bcff3 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Mon, 17 Jun 2024 22:17:48 +0200 Subject: [PATCH 34/53] Dedup attestation constructor logic in attester cache --- .../beacon_chain/src/attester_cache.rs | 2 + .../beacon_chain/src/early_attester_cache.rs | 45 +++++-------------- consensus/types/src/attestation.rs | 28 +++++++++--- validator_client/src/attestation_service.rs | 7 ++- 4 files changed, 39 insertions(+), 43 deletions(-) diff --git a/beacon_node/beacon_chain/src/attester_cache.rs b/beacon_node/beacon_chain/src/attester_cache.rs index 2e07cd32ed..b5012e8e4e 100644 --- a/beacon_node/beacon_chain/src/attester_cache.rs +++ b/beacon_node/beacon_chain/src/attester_cache.rs @@ -15,6 +15,7 @@ use state_processing::state_advance::{partial_state_advance, Error as StateAdvan use std::collections::HashMap; use std::ops::Range; use types::{ + attestation::Error as AttestationError, beacon_state::{ compute_committee_index_in_epoch, compute_committee_range_in_epoch, epoch_committee_count, }, @@ -59,6 +60,7 @@ pub enum Error { InverseRange { range: Range, }, + AttestationError(AttestationError), } impl From for Error { diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 41a225d9f2..dda699cc6c 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -6,7 +6,6 @@ use crate::{ use parking_lot::RwLock; use proto_array::Block as ProtoBlock; use std::sync::Arc; -use types::attestation::AttestationBase; use types::*; pub struct CacheItem { @@ -123,40 +122,16 @@ impl EarlyAttesterCache { item.committee_lengths .get_committee_length::(request_slot, request_index, spec)?; - let attestation = if spec.fork_name_at_slot::(request_slot).electra_enabled() { - let mut committee_bits = BitVector::default(); - if committee_len > 0 { - committee_bits - .set(request_index as usize, true) - .map_err(BeaconStateError::from)?; - } - Attestation::Electra(AttestationElectra { - aggregation_bits: BitList::with_capacity(committee_len) - .map_err(BeaconStateError::from)?, - committee_bits, - data: AttestationData { - slot: request_slot, - index: 0u64, - beacon_block_root: item.beacon_block_root, - source: item.source, - target: item.target, - }, - signature: AggregateSignature::empty(), - }) - } else { - Attestation::Base(AttestationBase { - aggregation_bits: BitList::with_capacity(committee_len) - .map_err(BeaconStateError::from)?, - data: AttestationData { - slot: request_slot, - index: request_index, - beacon_block_root: item.beacon_block_root, - source: item.source, - target: item.target, - }, - signature: AggregateSignature::empty(), - }) - }; + let attestation = Attestation::empty_for_signing( + request_index, + committee_len, + request_slot, + item.beacon_block_root, + item.source, + item.target, + spec, + ) + .map_err(Error::AttestationError)?; metrics::inc_counter(&metrics::BEACON_EARLY_ATTESTER_CACHE_HITS); diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 35bd80fc33..552c0faf6f 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -1,5 +1,6 @@ use crate::slot_data::SlotData; use crate::{test_utils::TestRandom, Hash256, Slot}; +use crate::{Checkpoint, ForkName}; use derivative::Derivative; use safe_arith::ArithError; use serde::{Deserialize, Serialize}; @@ -89,20 +90,29 @@ impl Hash for Attestation { impl Attestation { pub fn empty_for_signing( - committee_index: usize, + committee_index: u64, committee_length: usize, - data: AttestationData, + slot: Slot, + beacon_block_root: Hash256, + source: Checkpoint, + target: Checkpoint, spec: &ChainSpec, ) -> Result { - if spec.fork_name_at_slot::(data.slot).electra_enabled() { + if spec.fork_name_at_slot::(slot) >= ForkName::Electra { let mut committee_bits: BitVector = BitVector::default(); committee_bits - .set(committee_index, true) + .set(committee_index as usize, true) .map_err(|_| Error::InvalidCommitteeIndex)?; Ok(Attestation::Electra(AttestationElectra { aggregation_bits: BitList::with_capacity(committee_length) .map_err(|_| Error::InvalidCommitteeLength)?, - data, + data: AttestationData { + slot, + index: 0u64, + beacon_block_root, + source, + target, + }, committee_bits, signature: AggregateSignature::infinity(), })) @@ -110,7 +120,13 @@ impl Attestation { Ok(Attestation::Base(AttestationBase { aggregation_bits: BitList::with_capacity(committee_length) .map_err(|_| Error::InvalidCommitteeLength)?, - data, + data: AttestationData { + slot, + index: committee_index, + beacon_block_root, + source, + target, + }, signature: AggregateSignature::infinity(), })) } diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index d08d7567af..0cff39546d 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -374,9 +374,12 @@ impl AttestationService { } let mut attestation = match Attestation::::empty_for_signing( - duty.committee_index as usize, + duty.committee_index, duty.committee_length as usize, - attestation_data.clone(), + attestation_data.slot, + attestation_data.beacon_block_root, + attestation_data.source, + attestation_data.target, &self.context.eth2_config.spec, ) { Ok(attestation) => attestation, From 6a4d842376eaf3839bcee126b24b996bfa877814 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 18:00:58 +0200 Subject: [PATCH 35/53] Use if let Ok for committee_bits --- .../src/attestation_verification.rs | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 6eb0c6ecf9..221d9114b1 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -519,7 +519,7 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { .tree_hash_root(); // [New in Electra:EIP7549] - verify_committee_index(attestation, &chain.spec)?; + verify_committee_index(attestation)?; if chain .observed_attestations @@ -837,7 +837,7 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> { } // [New in Electra:EIP7549] - verify_committee_index(attestation, &chain.spec)?; + verify_committee_index(attestation)?; // Attestations must be for a known block. If the block is unknown, we simply drop the // attestation and do not delay consideration for later. @@ -1337,20 +1337,10 @@ pub fn verify_signed_aggregate_signatures( /// Verify that the `attestation` committee index is properly set for the attestation's fork. /// This function will only apply verification post-Electra. -pub fn verify_committee_index( - attestation: AttestationRef, - spec: &ChainSpec, -) -> Result<(), Error> { - if spec - .fork_name_at_slot::(attestation.data().slot) - .electra_enabled() - { +pub fn verify_committee_index(attestation: AttestationRef) -> Result<(), Error> { + if let Ok(committee_bits) = attestation.committee_bits() { // Check to ensure that the attestation is for a single committee. - let num_committee_bits = get_committee_indices::( - attestation - .committee_bits() - .map_err(|e| Error::BeaconChainError(e.into()))?, - ); + let num_committee_bits = get_committee_indices::(committee_bits); if num_committee_bits.len() != 1 { return Err(Error::NotExactlyOneCommitteeBitSet( num_committee_bits.len(), From 6f0b78426a9a4c3804159f3ac8998224645ce456 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 19:00:02 +0200 Subject: [PATCH 36/53] Dedup Attestation constructor code --- beacon_node/beacon_chain/src/beacon_chain.rs | 44 ++++-------------- beacon_node/beacon_chain/src/test_utils.rs | 47 +++++--------------- consensus/types/src/attestation.rs | 1 + 3 files changed, 22 insertions(+), 70 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index eeb1457954..f1d9ce791e 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -122,7 +122,6 @@ use store::{ use task_executor::{ShutdownReason, TaskExecutor}; use tokio_stream::Stream; use tree_hash::TreeHash; -use types::attestation::AttestationBase; use types::blob_sidecar::FixedBlobSidecarList; use types::payload::BlockProductionVersion; use types::*; @@ -1994,40 +1993,15 @@ impl BeaconChain { }; drop(cache_timer); - if self - .spec - .fork_name_at_slot::(request_slot) - .electra_enabled() - { - let mut committee_bits = BitVector::default(); - if committee_len > 0 { - committee_bits.set(request_index as usize, true)?; - } - Ok(Attestation::Electra(AttestationElectra { - aggregation_bits: BitList::with_capacity(committee_len)?, - data: AttestationData { - slot: request_slot, - index: 0u64, - beacon_block_root, - source: justified_checkpoint, - target, - }, - committee_bits, - signature: AggregateSignature::empty(), - })) - } else { - Ok(Attestation::Base(AttestationBase { - aggregation_bits: BitList::with_capacity(committee_len)?, - data: AttestationData { - slot: request_slot, - index: request_index, - beacon_block_root, - source: justified_checkpoint, - target, - }, - signature: AggregateSignature::empty(), - })) - } + Ok(Attestation::::empty_for_signing( + request_index, + committee_len, + request_slot, + beacon_block_root, + justified_checkpoint, + target, + &self.spec, + )?) } /// Performs the same validation as `Self::verify_unaggregated_attestation_for_gossip`, but for diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 7d443db380..5a37c5e6ce 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -58,7 +58,6 @@ use store::{config::StoreConfig, HotColdDB, ItemStore, LevelDB, MemoryStore}; use task_executor::TaskExecutor; use task_executor::{test_utils::TestRuntime, ShutdownReason}; use tree_hash::TreeHash; -use types::attestation::AttestationBase; use types::indexed_attestation::IndexedAttestationBase; use types::payload::BlockProductionVersion; pub use types::test_utils::generate_deterministic_keypairs; @@ -1033,40 +1032,18 @@ where *state.get_block_root(target_slot)? }; - if self.spec.fork_name_at_slot::(slot).electra_enabled() { - let mut committee_bits = BitVector::default(); - committee_bits.set(index as usize, true)?; - Ok(Attestation::Electra(AttestationElectra { - aggregation_bits: BitList::with_capacity(committee_len)?, - committee_bits, - data: AttestationData { - slot, - index: 0u64, - beacon_block_root, - source: state.current_justified_checkpoint(), - target: Checkpoint { - epoch, - root: target_root, - }, - }, - signature: AggregateSignature::empty(), - })) - } else { - Ok(Attestation::Base(AttestationBase { - aggregation_bits: BitList::with_capacity(committee_len)?, - data: AttestationData { - slot, - index, - beacon_block_root, - source: state.current_justified_checkpoint(), - target: Checkpoint { - epoch, - root: target_root, - }, - }, - signature: AggregateSignature::empty(), - })) - } + Ok(Attestation::empty_for_signing( + index, + committee_len, + slot, + beacon_block_root, + state.current_justified_checkpoint(), + Checkpoint { + epoch, + root: target_root, + }, + &self.spec, + )?) } /// A list of attestations for each committee for the given slot. diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 552c0faf6f..c8d1c3fb9b 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -89,6 +89,7 @@ impl Hash for Attestation { } impl Attestation { + /// Produces an attestation with empty signature. pub fn empty_for_signing( committee_index: u64, committee_length: usize, From 444cd625ef3aa9ca732334568ef8d54b7024c2e6 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 19:11:09 +0200 Subject: [PATCH 37/53] Diff reduction in tests --- beacon_node/beacon_chain/src/test_utils.rs | 84 +++++++------------ .../tests/attestation_production.rs | 27 ++---- 2 files changed, 38 insertions(+), 73 deletions(-) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 5a37c5e6ce..bd98f19af6 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1354,36 +1354,25 @@ where let fork_name = self.spec.fork_name_at_slot::(slot); let aggregate = if fork_name.electra_enabled() { - self.chain - .get_aggregated_attestation_electra( - slot, - &attestation.data().tree_hash_root(), - bc.index, - ) - .unwrap() - .unwrap_or_else(|| { - committee_attestations.iter().skip(1).fold( - attestation.clone(), - |mut agg, (att, _)| { - agg.aggregate(att.to_ref()); - agg - }, - ) - }) + self.chain.get_aggregated_attestation_electra( + slot, + &attestation.data().tree_hash_root(), + bc.index, + ) } else { self.chain .get_aggregated_attestation_base(attestation.data()) - .unwrap() - .unwrap_or_else(|| { - committee_attestations.iter().skip(1).fold( - attestation.clone(), - |mut agg, (att, _)| { - agg.aggregate(att.to_ref()); - agg - }, - ) - }) - }; + } + .unwrap() + .unwrap_or_else(|| { + committee_attestations.iter().skip(1).fold( + attestation.clone(), + |mut agg, (att, _)| { + agg.aggregate(att.to_ref()); + agg + }, + ) + }); // If the chain is able to produce an aggregate, use that. Otherwise, build an // aggregate locally. @@ -1518,40 +1507,29 @@ where let fork_name = self.spec.fork_name_at_slot::(Slot::new(0)); + let data = AttestationData { + slot: Slot::new(0), + index: 0, + beacon_block_root: Hash256::zero(), + target: Checkpoint { + root: Hash256::zero(), + epoch: target1.unwrap_or(fork.epoch), + }, + source: Checkpoint { + root: Hash256::zero(), + epoch: source1.unwrap_or(Epoch::new(0)), + }, + }; let mut attestation_1 = if fork_name.electra_enabled() { IndexedAttestation::Electra(IndexedAttestationElectra { attesting_indices: VariableList::new(validator_indices).unwrap(), - data: AttestationData { - slot: Slot::new(0), - index: 0, - beacon_block_root: Hash256::zero(), - target: Checkpoint { - root: Hash256::zero(), - epoch: target1.unwrap_or(fork.epoch), - }, - source: Checkpoint { - root: Hash256::zero(), - epoch: source1.unwrap_or(Epoch::new(0)), - }, - }, + data, signature: AggregateSignature::infinity(), }) } else { IndexedAttestation::Base(IndexedAttestationBase { attesting_indices: VariableList::new(validator_indices).unwrap(), - data: AttestationData { - slot: Slot::new(0), - index: 0, - beacon_block_root: Hash256::zero(), - target: Checkpoint { - root: Hash256::zero(), - epoch: target1.unwrap_or(fork.epoch), - }, - source: Checkpoint { - root: Hash256::zero(), - epoch: source1.unwrap_or(Epoch::new(0)), - }, - }, + data, signature: AggregateSignature::infinity(), }) }; diff --git a/beacon_node/beacon_chain/tests/attestation_production.rs b/beacon_node/beacon_chain/tests/attestation_production.rs index a58cfd9f2d..1716cd262f 100644 --- a/beacon_node/beacon_chain/tests/attestation_production.rs +++ b/beacon_node/beacon_chain/tests/attestation_production.rs @@ -190,30 +190,17 @@ async fn produces_attestations() { .produce_unaggregated_attestation(slot, index) .expect("should produce attestation"); - match &attestation { + let (aggregation_bits_len, aggregation_bits_zero) = match &attestation { Attestation::Base(att) => { - assert_eq!( - att.aggregation_bits.len(), - committee_len, - "bad committee len" - ); - assert!( - att.aggregation_bits.is_zero(), - "some committee bits are set" - ); + (att.aggregation_bits.len(), att.aggregation_bits.is_zero()) } Attestation::Electra(att) => { - assert_eq!( - att.aggregation_bits.len(), - committee_len, - "bad committee len" - ); - assert!( - att.aggregation_bits.is_zero(), - "some committee bits are set" - ); + (att.aggregation_bits.len(), att.aggregation_bits.is_zero()) } - } + }; + assert_eq!(aggregation_bits_len, committee_len, "bad committee len"); + assert!(aggregation_bits_zero, "some committee bits are set"); + let data = attestation.data(); assert_eq!( From d26473621a4392b1e4efc354c44e9828b9c2d664 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 19:38:25 +0200 Subject: [PATCH 38/53] Fix beacon_chain tests --- beacon_node/beacon_chain/src/observed_aggregates.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/beacon_node/beacon_chain/src/observed_aggregates.rs b/beacon_node/beacon_chain/src/observed_aggregates.rs index b02bff96a2..00476bfe7a 100644 --- a/beacon_node/beacon_chain/src/observed_aggregates.rs +++ b/beacon_node/beacon_chain/src/observed_aggregates.rs @@ -473,12 +473,13 @@ where #[cfg(not(debug_assertions))] mod tests { use super::*; - use types::{test_utils::test_random_instance, Hash256}; + use types::{test_utils::test_random_instance, AttestationBase, Hash256}; type E = types::MainnetEthSpec; fn get_attestation(slot: Slot, beacon_block_root: u64) -> Attestation { - let mut a: Attestation = test_random_instance(); + let a: AttestationBase = test_random_instance(); + let mut a = Attestation::Base(a); a.data_mut().slot = slot; a.data_mut().beacon_block_root = Hash256::from_low_u64_be(beacon_block_root); a From 7521f97ca5fcfd63b283d6cba0b0d0ffd548a5e9 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 19:38:35 +0200 Subject: [PATCH 39/53] Diff reduction --- .../beacon_chain/tests/block_verification.rs | 80 ++++++------------- 1 file changed, 26 insertions(+), 54 deletions(-) diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 55b2c14a3c..4c7a499697 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -1308,62 +1308,34 @@ async fn verify_block_for_gossip_doppelganger_detection() { } }; - match indexed_attestation { - IndexedAttestation::Base(indexed_attestation) => { - for &index in &indexed_attestation.attesting_indices { - let index = index as usize; + for index in match indexed_attestation { + IndexedAttestation::Base(att) => att.attesting_indices.into_iter(), + IndexedAttestation::Electra(att) => att.attesting_indices.into_iter(), + } { + let index = index as usize; - assert!(harness.chain.validator_seen_at_epoch(index, epoch)); + assert!(harness.chain.validator_seen_at_epoch(index, epoch)); - // Check the correct beacon cache is populated - assert!(harness - .chain - .observed_block_attesters - .read() - .validator_has_been_observed(epoch, index) - .expect("should check if block attester was observed")); - assert!(!harness - .chain - .observed_gossip_attesters - .read() - .validator_has_been_observed(epoch, index) - .expect("should check if gossip attester was observed")); - assert!(!harness - .chain - .observed_aggregators - .read() - .validator_has_been_observed(epoch, index) - .expect("should check if gossip aggregator was observed")); - } - } - IndexedAttestation::Electra(indexed_attestation) => { - for &index in &indexed_attestation.attesting_indices { - let index = index as usize; - - assert!(harness.chain.validator_seen_at_epoch(index, epoch)); - - // Check the correct beacon cache is populated - assert!(harness - .chain - .observed_block_attesters - .read() - .validator_has_been_observed(epoch, index) - .expect("should check if block attester was observed")); - assert!(!harness - .chain - .observed_gossip_attesters - .read() - .validator_has_been_observed(epoch, index) - .expect("should check if gossip attester was observed")); - assert!(!harness - .chain - .observed_aggregators - .read() - .validator_has_been_observed(epoch, index) - .expect("should check if gossip aggregator was observed")); - } - } - }; + // Check the correct beacon cache is populated + assert!(harness + .chain + .observed_block_attesters + .read() + .validator_has_been_observed(epoch, index) + .expect("should check if block attester was observed")); + assert!(!harness + .chain + .observed_gossip_attesters + .read() + .validator_has_been_observed(epoch, index) + .expect("should check if gossip attester was observed")); + assert!(!harness + .chain + .observed_aggregators + .read() + .validator_has_been_observed(epoch, index) + .expect("should check if gossip aggregator was observed")); + } } } From 4d3edfeaedb2e43314d1953dbfc7b82ef4133f73 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 19:46:12 +0200 Subject: [PATCH 40/53] Use Ord for ForkName in pubsub --- .../lighthouse_network/src/types/pubsub.rs | 75 ++++++++++--------- 1 file changed, 39 insertions(+), 36 deletions(-) diff --git a/beacon_node/lighthouse_network/src/types/pubsub.rs b/beacon_node/lighthouse_network/src/types/pubsub.rs index 7be68f879f..c7b9ee0ec9 100644 --- a/beacon_node/lighthouse_network/src/types/pubsub.rs +++ b/beacon_node/lighthouse_network/src/types/pubsub.rs @@ -158,18 +158,19 @@ impl PubsubMessage { GossipKind::BeaconAggregateAndProof => { let signed_aggregate_and_proof = match fork_context.from_context_bytes(gossip_topic.fork_digest) { - Some(ForkName::Base) - | Some(ForkName::Altair) - | Some(ForkName::Bellatrix) - | Some(ForkName::Capella) - | Some(ForkName::Deneb) => SignedAggregateAndProof::Base( - SignedAggregateAndProofBase::from_ssz_bytes(data) - .map_err(|e| format!("{:?}", e))?, - ), - Some(ForkName::Electra) => SignedAggregateAndProof::Electra( - SignedAggregateAndProofElectra::from_ssz_bytes(data) - .map_err(|e| format!("{:?}", e))?, - ), + Some(&fork_name) => { + if fork_name >= ForkName::Electra { + SignedAggregateAndProof::Electra( + SignedAggregateAndProofElectra::from_ssz_bytes(data) + .map_err(|e| format!("{:?}", e))?, + ) + } else { + SignedAggregateAndProof::Base( + SignedAggregateAndProofBase::from_ssz_bytes(data) + .map_err(|e| format!("{:?}", e))?, + ) + } + } None => { return Err(format!( "Unknown gossipsub fork digest: {:?}", @@ -184,18 +185,19 @@ impl PubsubMessage { GossipKind::Attestation(subnet_id) => { let attestation = match fork_context.from_context_bytes(gossip_topic.fork_digest) { - Some(ForkName::Base) - | Some(ForkName::Altair) - | Some(ForkName::Bellatrix) - | Some(ForkName::Capella) - | Some(ForkName::Deneb) => Attestation::Base( - AttestationBase::from_ssz_bytes(data) - .map_err(|e| format!("{:?}", e))?, - ), - Some(ForkName::Electra) => Attestation::Electra( - AttestationElectra::from_ssz_bytes(data) - .map_err(|e| format!("{:?}", e))?, - ), + Some(&fork_name) => { + if fork_name >= ForkName::Electra { + Attestation::Electra( + AttestationElectra::from_ssz_bytes(data) + .map_err(|e| format!("{:?}", e))?, + ) + } else { + Attestation::Base( + AttestationBase::from_ssz_bytes(data) + .map_err(|e| format!("{:?}", e))?, + ) + } + } None => { return Err(format!( "Unknown gossipsub fork digest: {:?}", @@ -281,18 +283,19 @@ impl PubsubMessage { GossipKind::AttesterSlashing => { let attester_slashing = match fork_context.from_context_bytes(gossip_topic.fork_digest) { - Some(ForkName::Base) - | Some(ForkName::Altair) - | Some(ForkName::Bellatrix) - | Some(ForkName::Capella) - | Some(ForkName::Deneb) => AttesterSlashing::Base( - AttesterSlashingBase::from_ssz_bytes(data) - .map_err(|e| format!("{:?}", e))?, - ), - Some(ForkName::Electra) => AttesterSlashing::Electra( - AttesterSlashingElectra::from_ssz_bytes(data) - .map_err(|e| format!("{:?}", e))?, - ), + Some(&fork_name) => { + if fork_name >= ForkName::Electra { + AttesterSlashing::Electra( + AttesterSlashingElectra::from_ssz_bytes(data) + .map_err(|e| format!("{:?}", e))?, + ) + } else { + AttesterSlashing::Base( + AttesterSlashingBase::from_ssz_bytes(data) + .map_err(|e| format!("{:?}", e))?, + ) + } + } None => { return Err(format!( "Unknown gossipsub fork digest: {:?}", From 7fce143300a6310677b974ef729a482a467f8e27 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 22:16:37 +0200 Subject: [PATCH 41/53] Resolve into_attestation_and_indices todo --- .../src/network_beacon_processor/gossip_methods.rs | 7 +------ consensus/types/src/signed_aggregate_and_proof.rs | 8 ++++++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index 34eb8b26b1..8cbc778218 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -105,12 +105,7 @@ impl VerifiedAttestation for VerifiedAggregate { /// Efficient clone-free implementation that moves out of the `Box`. fn into_attestation_and_indices(self) -> (Attestation, Vec) { - // TODO(electra): technically we shouldn't have to clone.. - let attestation = self - .signed_aggregate - .message() - .aggregate() - .clone_as_attestation(); + let attestation = self.signed_aggregate.into_attestation(); let attesting_indices = self.indexed_attestation.attesting_indices_to_vec(); (attestation, attesting_indices) } diff --git a/consensus/types/src/signed_aggregate_and_proof.rs b/consensus/types/src/signed_aggregate_and_proof.rs index 94624f02fe..d339cecaae 100644 --- a/consensus/types/src/signed_aggregate_and_proof.rs +++ b/consensus/types/src/signed_aggregate_and_proof.rs @@ -6,6 +6,7 @@ use super::{ Signature, SignedRoot, }; use crate::test_utils::TestRandom; +use crate::Attestation; use serde::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; use superstruct::superstruct; @@ -109,4 +110,11 @@ impl SignedAggregateAndProof { } } } + + pub fn into_attestation(self) -> Attestation { + match self { + Self::Base(att) => Attestation::Base(att.message.aggregate), + Self::Electra(att) => Attestation::Electra(att.message.aggregate), + } + } } From 4d4c268e1eff67a09fa1d4a6c29f6c55a670adbd Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 22:29:50 +0200 Subject: [PATCH 42/53] Remove stale TODO --- beacon_node/operation_pool/src/attestation_storage.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 43b1c3abbb..ad4ffe6d3c 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -217,7 +217,6 @@ impl CompactIndexedAttestationElectra { } pub fn aggregate_same_committee(&mut self, other: &Self) -> Option<()> { - // TODO(electra): remove assert in favour of Result if self.committee_bits != other.committee_bits { return None; } From 370d5112232d58b5eb793d2c4553d66f82f7c6fc Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Tue, 18 Jun 2024 22:52:40 +0200 Subject: [PATCH 43/53] Fix beacon_chain tests --- beacon_node/beacon_chain/tests/attestation_production.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/beacon_chain/tests/attestation_production.rs b/beacon_node/beacon_chain/tests/attestation_production.rs index 1716cd262f..697e449dc6 100644 --- a/beacon_node/beacon_chain/tests/attestation_production.rs +++ b/beacon_node/beacon_chain/tests/attestation_production.rs @@ -205,7 +205,7 @@ async fn produces_attestations() { assert_eq!( attestation.signature(), - &AggregateSignature::empty(), + &AggregateSignature::infinity(), "bad signature" ); assert_eq!(data.index, index, "bad index"); From cbb7c5d8f41c10d99aa84653c665ae5cc7a5f843 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 19 Jun 2024 11:39:45 +0200 Subject: [PATCH 44/53] Test spec invariant --- consensus/types/src/eth_spec.rs | 2 ++ consensus/types/src/indexed_attestation.rs | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index cdbab2d540..1c379f5de4 100644 --- a/consensus/types/src/eth_spec.rs +++ b/consensus/types/src/eth_spec.rs @@ -545,10 +545,12 @@ impl EthSpec for GnosisEthSpec { #[cfg(test)] mod test { use crate::{EthSpec, GnosisEthSpec, MainnetEthSpec, MinimalEthSpec}; + use ssz_types::typenum::Unsigned; fn assert_valid_spec() { E::kzg_commitments_tree_depth(); E::block_body_tree_depth(); + assert!(E::MaxValidatorsPerSlot::to_i32() >= E::MaxValidatorsPerCommittee::to_i32()); } #[test] diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index 429d65898a..900c905938 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -120,7 +120,8 @@ impl IndexedAttestation { let extended_attesting_indices: VariableList = VariableList::new(att.attesting_indices.to_vec()) .expect("MaxValidatorsPerSlot must be >= MaxValidatorsPerCommittee"); - // TODO: Add test after unstable rebase https://github.com/sigp/lighthouse/blob/474c1b44863927c588dd05ab2ac0f934298398e1/consensus/types/src/eth_spec.rs#L541 + // Note a unit test in consensus/types/src/eth_spec.rs asserts this invariant for + // all known specs IndexedAttestationElectra { attesting_indices: extended_attesting_indices, From 70a2d4de1055496edd4d1d4267f9b01cfd0c0cd8 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 19 Jun 2024 11:43:41 +0200 Subject: [PATCH 45/53] Use electra_enabled in pubsub --- beacon_node/lighthouse_network/src/types/pubsub.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/beacon_node/lighthouse_network/src/types/pubsub.rs b/beacon_node/lighthouse_network/src/types/pubsub.rs index c7b9ee0ec9..b443ecd1b9 100644 --- a/beacon_node/lighthouse_network/src/types/pubsub.rs +++ b/beacon_node/lighthouse_network/src/types/pubsub.rs @@ -159,7 +159,7 @@ impl PubsubMessage { let signed_aggregate_and_proof = match fork_context.from_context_bytes(gossip_topic.fork_digest) { Some(&fork_name) => { - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { SignedAggregateAndProof::Electra( SignedAggregateAndProofElectra::from_ssz_bytes(data) .map_err(|e| format!("{:?}", e))?, @@ -186,7 +186,7 @@ impl PubsubMessage { let attestation = match fork_context.from_context_bytes(gossip_topic.fork_digest) { Some(&fork_name) => { - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { Attestation::Electra( AttestationElectra::from_ssz_bytes(data) .map_err(|e| format!("{:?}", e))?, @@ -284,7 +284,7 @@ impl PubsubMessage { let attester_slashing = match fork_context.from_context_bytes(gossip_topic.fork_digest) { Some(&fork_name) => { - if fork_name >= ForkName::Electra { + if fork_name.electra_enabled() { AttesterSlashing::Electra( AttesterSlashingElectra::from_ssz_bytes(data) .map_err(|e| format!("{:?}", e))?, From 9e6e76fb898ad9349a2bd383fbf8543ffc915299 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 19 Jun 2024 12:47:38 +0200 Subject: [PATCH 46/53] Remove get_indexed_attestation_from_signed_aggregate --- .../src/attestation_verification.rs | 103 +++++++++--------- .../src/common/get_attesting_indices.rs | 103 ++---------------- 2 files changed, 62 insertions(+), 144 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 221d9114b1..a5635a3392 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -61,9 +61,10 @@ use std::borrow::Cow; use strum::AsRefStr; use tree_hash::TreeHash; use types::{ - Attestation, AttestationRef, BeaconCommittee, BeaconStateError, - BeaconStateError::NoCommitteeFound, ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName, - Hash256, IndexedAttestation, SelectionProof, SignedAggregateAndProof, Slot, SubnetId, + Attestation, AttestationRef, BeaconCommittee, + BeaconStateError::{self, NoCommitteeFound}, + ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, + SelectionProof, SignedAggregateAndProof, Slot, SubnetId, }; pub use batch::{batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations}; @@ -598,57 +599,61 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { }; let get_indexed_attestation_with_committee = |(committees, _): (Vec, CommitteesPerSlot)| { + let (index, aggregator_index, selection_proof, data) = match signed_aggregate { + SignedAggregateAndProof::Base(signed_aggregate) => ( + signed_aggregate.message.aggregate.data.index, + signed_aggregate.message.aggregator_index, + // Note: this clones the signature which is known to be a relatively slow operation. + // Future optimizations should remove this clone. + signed_aggregate.message.selection_proof.clone(), + signed_aggregate.message.aggregate.data.clone(), + ), + SignedAggregateAndProof::Electra(signed_aggregate) => ( + signed_aggregate + .message + .aggregate + .committee_index() + .ok_or(Error::NotExactlyOneCommitteeBitSet(0))?, + signed_aggregate.message.aggregator_index, + signed_aggregate.message.selection_proof.clone(), + signed_aggregate.message.aggregate.data.clone(), + ), + }; + let slot = data.slot; + + let committee = committees + .iter() + .filter(|&committee| committee.index == index) + .at_most_one() + .map_err(|_| Error::NoCommitteeForSlotAndIndex { slot, index })? + .ok_or(Error::NoCommitteeForSlotAndIndex { slot, index })?; + + if !SelectionProof::from(selection_proof) + .is_aggregator(committee.committee.len(), &chain.spec) + .map_err(|e| Error::BeaconChainError(e.into()))? + { + return Err(Error::InvalidSelectionProof { aggregator_index }); + } + + // Ensure the aggregator is a member of the committee for which it is aggregating. + if !committee.committee.contains(&(aggregator_index as usize)) { + return Err(Error::AggregatorNotInCommittee { aggregator_index }); + } + + // p2p aggregates have a single committee, we can assert that aggregation_bits is always + // less then MaxValidatorsPerCommittee match signed_aggregate { SignedAggregateAndProof::Base(signed_aggregate) => { - let att = &signed_aggregate.message.aggregate; - let aggregator_index = signed_aggregate.message.aggregator_index; - let committee = committees - .iter() - .filter(|&committee| committee.index == att.data.index) - .at_most_one() - .map_err(|_| Error::NoCommitteeForSlotAndIndex { - slot: att.data.slot, - index: att.data.index, - })?; - - // TODO(electra): - // Note: this clones the signature which is known to be a relatively slow operation. - // - // Future optimizations should remove this clone. - if let Some(committee) = committee { - let selection_proof = SelectionProof::from( - signed_aggregate.message.selection_proof.clone(), - ); - - if !selection_proof - .is_aggregator(committee.committee.len(), &chain.spec) - .map_err(|e| Error::BeaconChainError(e.into()))? - { - return Err(Error::InvalidSelectionProof { aggregator_index }); - } - - // Ensure the aggregator is a member of the committee for which it is aggregating. - if !committee.committee.contains(&(aggregator_index as usize)) { - return Err(Error::AggregatorNotInCommittee { aggregator_index }); - } - - attesting_indices_base::get_indexed_attestation( - committee.committee, - att, - ) - .map_err(|e| BeaconChainError::from(e).into()) - } else { - Err(Error::NoCommitteeForSlotAndIndex { - slot: att.data.slot, - index: att.data.index, - }) - } + attesting_indices_base::get_indexed_attestation( + committee.committee, + &signed_aggregate.message.aggregate, + ) + .map_err(|e| BeaconChainError::from(e).into()) } SignedAggregateAndProof::Electra(signed_aggregate) => { - attesting_indices_electra::get_indexed_attestation_from_signed_aggregate( + attesting_indices_electra::get_indexed_attestation_from_committees( &committees, - signed_aggregate, - &chain.spec, + &signed_aggregate.message.aggregate, ) .map_err(|e| BeaconChainError::from(e).into()) } diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index a79604003e..f24b003665 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -51,100 +51,6 @@ pub mod attesting_indices_electra { use safe_arith::SafeArith; use types::*; - // TODO(electra) remove duplicate code - // get_indexed_attestation is almost an exact duplicate - // the only differences are the invalid selection proof - // and aggregator not in committee checks - pub fn get_indexed_attestation_from_signed_aggregate( - committees: &[BeaconCommittee], - signed_aggregate: &SignedAggregateAndProofElectra, - spec: &ChainSpec, - ) -> Result, BeaconStateError> { - let mut output: HashSet = HashSet::new(); - - let committee_bits = &signed_aggregate.message.aggregate.committee_bits; - let aggregation_bits = &signed_aggregate.message.aggregate.aggregation_bits; - let aggregator_index = signed_aggregate.message.aggregator_index; - let attestation = &signed_aggregate.message.aggregate; - - let committee_indices = get_committee_indices::(committee_bits); - - let mut committee_offset = 0; - - let committees_map: HashMap = committees - .iter() - .map(|committee| (committee.index, committee)) - .collect(); - - let committee_count_per_slot = committees.len() as u64; - let mut participant_count = 0; - - // TODO(electra): - // Note: this clones the signature which is known to be a relatively slow operation. - // - // Future optimizations should remove this clone. - let selection_proof = - SelectionProof::from(signed_aggregate.message.selection_proof.clone()); - - for index in committee_indices { - if let Some(&beacon_committee) = committees_map.get(&index) { - if !selection_proof - .is_aggregator(beacon_committee.committee.len(), spec) - .map_err(BeaconStateError::ArithError)? - { - return Err(BeaconStateError::InvalidSelectionProof { aggregator_index }); - } - - if !beacon_committee - .committee - .contains(&(aggregator_index as usize)) - { - return Err(BeaconStateError::AggregatorNotInCommittee { aggregator_index }); - } - - // This check is new to the spec's `process_attestation` in Electra. - if index >= committee_count_per_slot { - return Err(BeaconStateError::InvalidCommitteeIndex(index)); - } - - participant_count.safe_add_assign(beacon_committee.committee.len() as u64)?; - let committee_attesters = beacon_committee - .committee - .iter() - .enumerate() - .filter_map(|(i, &index)| { - if let Ok(aggregation_bit_index) = committee_offset.safe_add(i) { - if aggregation_bits.get(aggregation_bit_index).unwrap_or(false) { - return Some(index as u64); - } - } - None - }) - .collect::>(); - - output.extend(committee_attesters); - - committee_offset.safe_add_assign(beacon_committee.committee.len())?; - } else { - return Err(Error::NoCommitteeFound(index)); - } - } - - // This check is new to the spec's `process_attestation` in Electra. - if participant_count as usize != aggregation_bits.len() { - return Err(Error::InvalidBitfield); - } - - let mut indices = output.into_iter().collect_vec(); - indices.sort_unstable(); - - Ok(IndexedAttestation::Electra(IndexedAttestationElectra { - attesting_indices: VariableList::new(indices)?, - data: attestation.data.clone(), - signature: attestation.signature.clone(), - })) - } - pub fn get_indexed_attestation( committees: &[BeaconCommittee], attestation: &AttestationElectra, @@ -167,8 +73,15 @@ pub mod attesting_indices_electra { attestation: &AttestationElectra, ) -> Result, BlockOperationError> { let committees = beacon_state.get_beacon_committees_at_slot(attestation.data.slot)?; + get_indexed_attestation_from_committees(&committees, attestation) + } + + pub fn get_indexed_attestation_from_committees( + committees: &[BeaconCommittee], + attestation: &AttestationElectra, + ) -> Result, BlockOperationError> { let attesting_indices = get_attesting_indices::( - &committees, + committees, &attestation.aggregation_bits, &attestation.committee_bits, )?; From a8d8989c0507ed8246cdf9f657d1adb141772d79 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 19 Jun 2024 12:50:41 +0200 Subject: [PATCH 47/53] Use ok_or instead of if let else --- .../src/common/get_attesting_indices.rs | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index f24b003665..9fb88407d3 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -122,32 +122,32 @@ pub mod attesting_indices_electra { let committee_count_per_slot = committees.len() as u64; let mut participant_count = 0; for index in committee_indices { - if let Some(&beacon_committee) = committees_map.get(&index) { - // This check is new to the spec's `process_attestation` in Electra. - if index >= committee_count_per_slot { - return Err(BeaconStateError::InvalidCommitteeIndex(index)); - } - participant_count.safe_add_assign(beacon_committee.committee.len() as u64)?; - let committee_attesters = beacon_committee - .committee - .iter() - .enumerate() - .filter_map(|(i, &index)| { - if let Ok(aggregation_bit_index) = committee_offset.safe_add(i) { - if aggregation_bits.get(aggregation_bit_index).unwrap_or(false) { - return Some(index as u64); - } - } - None - }) - .collect::>(); + let beacon_committee = committees_map + .get(&index) + .ok_or(Error::NoCommitteeFound(index))?; - output.extend(committee_attesters); - - committee_offset.safe_add_assign(beacon_committee.committee.len())?; - } else { - return Err(Error::NoCommitteeFound(index)); + // This check is new to the spec's `process_attestation` in Electra. + if index >= committee_count_per_slot { + return Err(BeaconStateError::InvalidCommitteeIndex(index)); } + participant_count.safe_add_assign(beacon_committee.committee.len() as u64)?; + let committee_attesters = beacon_committee + .committee + .iter() + .enumerate() + .filter_map(|(i, &index)| { + if let Ok(aggregation_bit_index) = committee_offset.safe_add(i) { + if aggregation_bits.get(aggregation_bit_index).unwrap_or(false) { + return Some(index as u64); + } + } + None + }) + .collect::>(); + + output.extend(committee_attesters); + + committee_offset.safe_add_assign(beacon_committee.committee.len())?; } // This check is new to the spec's `process_attestation` in Electra. From d67270f8992569c59223fc26141b5a2e575c74b9 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 19 Jun 2024 12:59:27 +0200 Subject: [PATCH 48/53] committees are sorted --- .../beacon_chain/src/attestation_verification.rs | 9 +++++---- .../src/common/get_attesting_indices.rs | 16 ++++++++-------- .../types/src/beacon_state/committee_cache.rs | 2 ++ 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index a5635a3392..51b5b11b09 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -597,6 +597,8 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { )) } }; + + // Committees must be sorted by ascending index order 0..committees_per_slot let get_indexed_attestation_with_committee = |(committees, _): (Vec, CommitteesPerSlot)| { let (index, aggregator_index, selection_proof, data) = match signed_aggregate { @@ -622,10 +624,7 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { let slot = data.slot; let committee = committees - .iter() - .filter(|&committee| committee.index == index) - .at_most_one() - .map_err(|_| Error::NoCommitteeForSlotAndIndex { slot, index })? + .get(index as usize) .ok_or(Error::NoCommitteeForSlotAndIndex { slot, index })?; if !SelectionProof::from(selection_proof) @@ -1422,6 +1421,8 @@ pub fn obtain_indexed_attestation_and_committees_per_slot( /// /// If the committees for an `attestation`'s slot isn't found in the `shuffling_cache`, we will read a state /// from disk and then update the `shuffling_cache`. +/// +/// Committees are sorted by ascending index order 0..committees_per_slot fn map_attestation_committees( chain: &BeaconChain, attestation: AttestationRef, diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 9fb88407d3..0657100999 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -44,13 +44,16 @@ pub mod attesting_indices_base { } pub mod attesting_indices_electra { - use std::collections::{HashMap, HashSet}; + use std::collections::HashSet; use crate::per_block_processing::errors::{AttestationInvalid as Invalid, BlockOperationError}; use itertools::Itertools; use safe_arith::SafeArith; use types::*; + /// Compute an Electra IndexedAttestation given a list of committees. + /// + /// Committees must be sorted by ascending order 0..committees_per_slot pub fn get_indexed_attestation( committees: &[BeaconCommittee], attestation: &AttestationElectra, @@ -103,6 +106,8 @@ pub mod attesting_indices_electra { } /// Returns validator indices which participated in the attestation, sorted by increasing index. + /// + /// Committees must be sorted by ascending order 0..committees_per_slot pub fn get_attesting_indices( committees: &[BeaconCommittee], aggregation_bits: &BitList, @@ -114,16 +119,11 @@ pub mod attesting_indices_electra { let mut committee_offset = 0; - let committees_map: HashMap = committees - .iter() - .map(|committee| (committee.index, committee)) - .collect(); - let committee_count_per_slot = committees.len() as u64; let mut participant_count = 0; for index in committee_indices { - let beacon_committee = committees_map - .get(&index) + let beacon_committee = committees + .get(index as usize) .ok_or(Error::NoCommitteeFound(index))?; // This check is new to the spec's `process_attestation` in Electra. diff --git a/consensus/types/src/beacon_state/committee_cache.rs b/consensus/types/src/beacon_state/committee_cache.rs index 209659ea88..161f854157 100644 --- a/consensus/types/src/beacon_state/committee_cache.rs +++ b/consensus/types/src/beacon_state/committee_cache.rs @@ -183,6 +183,8 @@ impl CommitteeCache { } /// Get all the Beacon committees at a given `slot`. + /// + /// Committees are sorted by ascending index order 0..committees_per_slot pub fn get_beacon_committees_at_slot(&self, slot: Slot) -> Result, Error> { if self.initialized_epoch.is_none() { return Err(Error::CommitteeCacheUninitialized(None)); From 3977b92c49047087d8b93a46dd8d3229d2214233 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 19 Jun 2024 13:45:47 -0400 Subject: [PATCH 49/53] remove dup method `get_indexed_attestation_from_committees` --- .../src/attestation_verification.rs | 2 +- .../src/common/get_attesting_indices.rs | 19 +------------------ 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 51b5b11b09..6044cafd6e 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -650,7 +650,7 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { .map_err(|e| BeaconChainError::from(e).into()) } SignedAggregateAndProof::Electra(signed_aggregate) => { - attesting_indices_electra::get_indexed_attestation_from_committees( + attesting_indices_electra::get_indexed_attestation( &committees, &signed_aggregate.message.aggregate, ) diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 0657100999..457a526104 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -76,24 +76,7 @@ pub mod attesting_indices_electra { attestation: &AttestationElectra, ) -> Result, BlockOperationError> { let committees = beacon_state.get_beacon_committees_at_slot(attestation.data.slot)?; - get_indexed_attestation_from_committees(&committees, attestation) - } - - pub fn get_indexed_attestation_from_committees( - committees: &[BeaconCommittee], - attestation: &AttestationElectra, - ) -> Result, BlockOperationError> { - let attesting_indices = get_attesting_indices::( - committees, - &attestation.aggregation_bits, - &attestation.committee_bits, - )?; - - Ok(IndexedAttestation::Electra(IndexedAttestationElectra { - attesting_indices: VariableList::new(attesting_indices)?, - data: attestation.data.clone(), - signature: attestation.signature.clone(), - })) + get_indexed_attestation(&committees, attestation) } /// Shortcut for getting the attesting indices while fetching the committee from the state's cache. From afb9122cc1c9b243542dddaaac611493c01f7af6 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 19 Jun 2024 15:00:33 -0400 Subject: [PATCH 50/53] update default persisted op pool deserialization --- beacon_node/operation_pool/src/persistence.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_node/operation_pool/src/persistence.rs b/beacon_node/operation_pool/src/persistence.rs index 99cb1aafbc..79509e5f6c 100644 --- a/beacon_node/operation_pool/src/persistence.rs +++ b/beacon_node/operation_pool/src/persistence.rs @@ -258,8 +258,8 @@ impl StoreItem for PersistedOperationPool { fn from_store_bytes(bytes: &[u8]) -> Result { // Default deserialization to the latest variant. - PersistedOperationPoolV15::from_ssz_bytes(bytes) - .map(Self::V15) + PersistedOperationPoolV20::from_ssz_bytes(bytes) + .map(Self::V20) .map_err(Into::into) } } From 381bbaba94df0ba23a267f92038f4637942fd2d3 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 19 Jun 2024 17:04:47 -0400 Subject: [PATCH 51/53] ensure aggregate and proof uses serde untagged on ref --- consensus/types/src/aggregate_and_proof.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/types/src/aggregate_and_proof.rs b/consensus/types/src/aggregate_and_proof.rs index ae80369130..6ae0df4ad7 100644 --- a/consensus/types/src/aggregate_and_proof.rs +++ b/consensus/types/src/aggregate_and_proof.rs @@ -31,7 +31,7 @@ use tree_hash_derive::TreeHash; ), ref_attributes( derive(Debug, PartialEq, TreeHash, Serialize,), - serde(bound = "E: EthSpec"), + serde(untagged, bound = "E: EthSpec"), tree_hash(enum_behaviour = "transparent") ) )] From 0e2add2daad556a771a9d66130bf277a8c86968b Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Thu, 20 Jun 2024 09:58:53 +0200 Subject: [PATCH 52/53] Fork aware ssz static attestation tests --- testing/ef_tests/tests/tests.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index fb8bdfcae7..9e8b375b2c 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -219,7 +219,6 @@ mod ssz_static { use types::historical_summary::HistoricalSummary; use types::{AttesterSlashingBase, AttesterSlashingElectra, LightClientBootstrapAltair, *}; - ssz_static_test!(attestation, Attestation<_>); ssz_static_test!(attestation_data, AttestationData); ssz_static_test!(beacon_block, SszStaticWithSpecHandler, BeaconBlock<_>); ssz_static_test!(beacon_block_header, BeaconBlockHeader); @@ -247,6 +246,16 @@ mod ssz_static { ssz_static_test!(validator, Validator); ssz_static_test!(voluntary_exit, VoluntaryExit); + #[test] + fn attestation() { + SszStaticHandler::, MinimalEthSpec>::pre_electra().run(); + SszStaticHandler::, MainnetEthSpec>::pre_electra().run(); + SszStaticHandler::, MinimalEthSpec>::electra_only() + .run(); + SszStaticHandler::, MainnetEthSpec>::electra_only() + .run(); + } + #[test] fn attester_slashing() { SszStaticHandler::, MinimalEthSpec>::pre_electra() From f85a1243623f27f228525277f8fb4d9f04276a52 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 20 Jun 2024 15:36:43 +0200 Subject: [PATCH 53/53] Electra attestation changes from Lions review (#5971) * dedup/cleanup and remove unneeded hashset use * remove irrelevant TODOs --- .../src/attestation_verification.rs | 3 +-- .../src/naive_aggregation_pool.rs | 24 +++++-------------- .../tests/attestation_verification.rs | 1 - .../src/common/get_attesting_indices.rs | 11 ++++----- .../per_block_processing/signature_sets.rs | 1 - 5 files changed, 11 insertions(+), 29 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 6044cafd6e..32c33f3e61 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -1397,8 +1397,7 @@ pub fn obtain_indexed_attestation_and_committees_per_slot( attesting_indices_electra::get_indexed_attestation(&committees, att) .map(|attestation| (attestation, committees_per_slot)) .map_err(|e| { - let index = att.committee_index().unwrap_or(0); - if e == BlockOperationError::BeaconStateError(NoCommitteeFound(index)) { + if let BlockOperationError::BeaconStateError(NoCommitteeFound(index)) = e { Error::NoCommitteeForSlotAndIndex { slot: att.data.slot, index, diff --git a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs index b16ced789f..211aecfe63 100644 --- a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs +++ b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs @@ -241,24 +241,12 @@ impl AggregateMap for AggregatedAttestationMap { fn insert(&mut self, a: AttestationRef) -> Result { let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_CORE_INSERT); - let aggregation_bit = match a { - AttestationRef::Base(att) => att - .aggregation_bits - .iter() - .enumerate() - .filter_map(|(i, bit)| if bit { Some(i) } else { None }) - .at_most_one() - .map_err(|iter| Error::MoreThanOneAggregationBitSet(iter.count()))? - .ok_or(Error::NoAggregationBitsSet)?, - AttestationRef::Electra(att) => att - .aggregation_bits - .iter() - .enumerate() - .filter_map(|(i, bit)| if bit { Some(i) } else { None }) - .at_most_one() - .map_err(|iter| Error::MoreThanOneAggregationBitSet(iter.count()))? - .ok_or(Error::NoAggregationBitsSet)?, - }; + let aggregation_bit = *a + .set_aggregation_bits() + .iter() + .at_most_one() + .map_err(|iter| Error::MoreThanOneAggregationBitSet(iter.count()))? + .ok_or(Error::NoAggregationBitsSet)?; let attestation_key = AttestationKey::from_attestation_ref(a)?; let attestation_key_root = attestation_key.tree_hash_root(); diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 3e35e58296..19efe10c6d 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -233,7 +233,6 @@ fn get_non_aggregator( let state = &head.beacon_state; let current_slot = chain.slot().expect("should get slot"); - // TODO(electra) make fork-agnostic let committee = state .get_beacon_committee( current_slot, diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 457a526104..b131f7679a 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -47,7 +47,6 @@ pub mod attesting_indices_electra { use std::collections::HashSet; use crate::per_block_processing::errors::{AttestationInvalid as Invalid, BlockOperationError}; - use itertools::Itertools; use safe_arith::SafeArith; use types::*; @@ -96,7 +95,7 @@ pub mod attesting_indices_electra { aggregation_bits: &BitList, committee_bits: &BitVector, ) -> Result, BeaconStateError> { - let mut output: HashSet = HashSet::new(); + let mut attesting_indices = vec![]; let committee_indices = get_committee_indices::(committee_bits); @@ -128,8 +127,7 @@ pub mod attesting_indices_electra { }) .collect::>(); - output.extend(committee_attesters); - + attesting_indices.extend(committee_attesters); committee_offset.safe_add_assign(beacon_committee.committee.len())?; } @@ -138,10 +136,9 @@ pub mod attesting_indices_electra { return Err(BeaconStateError::InvalidBitfield); } - let mut indices = output.into_iter().collect_vec(); - indices.sort_unstable(); + attesting_indices.sort_unstable(); - Ok(indices) + Ok(attesting_indices) } pub fn get_committee_indices( diff --git a/consensus/state_processing/src/per_block_processing/signature_sets.rs b/consensus/state_processing/src/per_block_processing/signature_sets.rs index 8cf39abf43..2e00ee0341 100644 --- a/consensus/state_processing/src/per_block_processing/signature_sets.rs +++ b/consensus/state_processing/src/per_block_processing/signature_sets.rs @@ -326,7 +326,6 @@ where genesis_validators_root, ); - // TODO(electra), signing root isnt unique in the case of electra let message = indexed_attestation.data().signing_root(domain); Ok(SignatureSet::multiple_pubkeys(signature, pubkeys, message))