From f919f82e4f7eb06baa2fa3a41e8ec8951fccdabc Mon Sep 17 00:00:00 2001 From: Age Manning Date: Tue, 5 Mar 2024 06:39:23 +1100 Subject: [PATCH 1/5] Improve logging around peer scoring (#5325) * Improve logging around score states * Improve logs also --- .../src/peer_manager/peerdb.rs | 18 ++++++++++-------- .../src/peer_manager/peerdb/score.rs | 8 ++++---- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs index 8aef0bad420..ebb355fefcf 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs @@ -168,7 +168,7 @@ impl PeerDB { fn score_state_banned_or_disconnected(&self, peer_id: &PeerId) -> bool { if let Some(peer) = self.peers.get(peer_id) { match peer.score_state() { - ScoreState::Banned | ScoreState::Disconnected => true, + ScoreState::Banned | ScoreState::ForcedDisconnect => true, _ => self.ip_is_banned(peer).is_some(), } } else { @@ -1062,12 +1062,12 @@ impl PeerDB { log: &slog::Logger, ) -> ScoreTransitionResult { match (info.score_state(), previous_state) { - (ScoreState::Banned, ScoreState::Healthy | ScoreState::Disconnected) => { + (ScoreState::Banned, ScoreState::Healthy | ScoreState::ForcedDisconnect) => { debug!(log, "Peer has been banned"; "peer_id" => %peer_id, "score" => %info.score()); ScoreTransitionResult::Banned } - (ScoreState::Disconnected, ScoreState::Banned | ScoreState::Healthy) => { - debug!(log, "Peer transitioned to disconnect state"; "peer_id" => %peer_id, "score" => %info.score(), "past_state" => %previous_state); + (ScoreState::ForcedDisconnect, ScoreState::Banned | ScoreState::Healthy) => { + debug!(log, "Peer transitioned to forced disconnect score state"; "peer_id" => %peer_id, "score" => %info.score(), "past_score_state" => %previous_state); // disconnect the peer if it's currently connected or dialing if info.is_connected_or_dialing() { ScoreTransitionResult::Disconnected @@ -1079,18 +1079,20 @@ impl PeerDB { ScoreTransitionResult::NoAction } } - (ScoreState::Healthy, ScoreState::Disconnected) => { - debug!(log, "Peer transitioned to healthy state"; "peer_id" => %peer_id, "score" => %info.score(), "past_state" => %previous_state); + (ScoreState::Healthy, ScoreState::ForcedDisconnect) => { + debug!(log, "Peer transitioned to healthy score state"; "peer_id" => %peer_id, "score" => %info.score(), "past_score_state" => %previous_state); ScoreTransitionResult::NoAction } (ScoreState::Healthy, ScoreState::Banned) => { - debug!(log, "Peer transitioned to healthy state"; "peer_id" => %peer_id, "score" => %info.score(), "past_state" => %previous_state); + debug!(log, "Peer transitioned to healthy score state"; "peer_id" => %peer_id, "score" => %info.score(), "past_score_state" => %previous_state); // unban the peer if it was previously banned. ScoreTransitionResult::Unbanned } // Explicitly ignore states that haven't transitioned. (ScoreState::Healthy, ScoreState::Healthy) => ScoreTransitionResult::NoAction, - (ScoreState::Disconnected, ScoreState::Disconnected) => ScoreTransitionResult::NoAction, + (ScoreState::ForcedDisconnect, ScoreState::ForcedDisconnect) => { + ScoreTransitionResult::NoAction + } (ScoreState::Banned, ScoreState::Banned) => ScoreTransitionResult::NoAction, } 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 877d725812c..ba9bd314722 100644 --- a/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs +++ b/beacon_node/lighthouse_network/src/peer_manager/peerdb/score.rs @@ -104,7 +104,7 @@ pub(crate) enum ScoreState { /// We are content with the peers performance. We permit connections and messages. Healthy, /// The peer should be disconnected. We allow re-connections if the peer is persistent. - Disconnected, + ForcedDisconnect, /// The peer is banned. We disallow new connections until it's score has decayed into a /// tolerable threshold. Banned, @@ -115,7 +115,7 @@ impl std::fmt::Display for ScoreState { match self { ScoreState::Healthy => write!(f, "Healthy"), ScoreState::Banned => write!(f, "Banned"), - ScoreState::Disconnected => write!(f, "Disconnected"), + ScoreState::ForcedDisconnect => write!(f, "Disconnected"), } } } @@ -313,7 +313,7 @@ impl Score { pub(crate) fn state(&self) -> ScoreState { match self.score() { x if x <= MIN_SCORE_BEFORE_BAN => ScoreState::Banned, - x if x <= MIN_SCORE_BEFORE_DISCONNECT => ScoreState::Disconnected, + x if x <= MIN_SCORE_BEFORE_DISCONNECT => ScoreState::ForcedDisconnect, _ => ScoreState::Healthy, } } @@ -407,7 +407,7 @@ mod tests { assert!(score.score() < 0.0); assert_eq!(score.state(), ScoreState::Healthy); score.test_add(-1.0001); - assert_eq!(score.state(), ScoreState::Disconnected); + assert_eq!(score.state(), ScoreState::ForcedDisconnect); } #[test] From cff6258bb14ddac9eb70bb8682e713b4597c7421 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Tue, 5 Mar 2024 10:15:05 +1100 Subject: [PATCH 2/5] Fix duties override bug in VC (#5305) * Fix duties override bug in VC * Use initial request efficiently * Prevent expired subscriptions by construction * Clean up selection proof logic * Add test --- validator_client/src/duties_service.rs | 195 ++++++++++++++++--------- 1 file changed, 127 insertions(+), 68 deletions(-) diff --git a/validator_client/src/duties_service.rs b/validator_client/src/duties_service.rs index 290803e257a..b5b56943c4b 100644 --- a/validator_client/src/duties_service.rs +++ b/validator_client/src/duties_service.rs @@ -122,43 +122,37 @@ pub struct SubscriptionSlots { slots: Vec<(Slot, AtomicBool)>, } -impl DutyAndProof { - /// Instantiate `Self`, computing the selection proof as well. - pub async fn new_with_selection_proof( - duty: AttesterData, - validator_store: &ValidatorStore, - spec: &ChainSpec, - ) -> Result { - let selection_proof = validator_store - .produce_selection_proof(duty.pubkey, duty.slot) - .await - .map_err(Error::FailedToProduceSelectionProof)?; - - let selection_proof = selection_proof - .is_aggregator(duty.committee_length as usize, spec) - .map_err(Error::InvalidModulo) - .map(|is_aggregator| { - if is_aggregator { - Some(selection_proof) - } else { - // Don't bother storing the selection proof if the validator isn't an - // aggregator, we won't need it. - None - } - })?; - - let subscription_slots = SubscriptionSlots::new(duty.slot); - - Ok(Self { - duty, - selection_proof, - subscription_slots, +/// Create a selection proof for `duty`. +/// +/// Return `Ok(None)` if the attesting validator is not an aggregator. +async fn make_selection_proof( + duty: &AttesterData, + validator_store: &ValidatorStore, + spec: &ChainSpec, +) -> Result, Error> { + let selection_proof = validator_store + .produce_selection_proof(duty.pubkey, duty.slot) + .await + .map_err(Error::FailedToProduceSelectionProof)?; + + selection_proof + .is_aggregator(duty.committee_length as usize, spec) + .map_err(Error::InvalidModulo) + .map(|is_aggregator| { + if is_aggregator { + Some(selection_proof) + } else { + // Don't bother storing the selection proof if the validator isn't an + // aggregator, we won't need it. + None + } }) - } +} +impl DutyAndProof { /// Create a new `DutyAndProof` with the selection proof waiting to be filled in. - pub fn new_without_selection_proof(duty: AttesterData) -> Self { - let subscription_slots = SubscriptionSlots::new(duty.slot); + pub fn new_without_selection_proof(duty: AttesterData, current_slot: Slot) -> Self { + let subscription_slots = SubscriptionSlots::new(duty.slot, current_slot); Self { duty, selection_proof: None, @@ -168,10 +162,13 @@ impl DutyAndProof { } impl SubscriptionSlots { - fn new(duty_slot: Slot) -> Arc { + fn new(duty_slot: Slot, current_slot: Slot) -> Arc { let slots = ATTESTATION_SUBSCRIPTION_OFFSETS .into_iter() .filter_map(|offset| duty_slot.safe_sub(offset).ok()) + // Keep only scheduled slots that haven't happened yet. This avoids sending expired + // subscriptions. + .filter(|scheduled_slot| *scheduled_slot > current_slot) .map(|scheduled_slot| (scheduled_slot, AtomicBool::new(false))) .collect(); Arc::new(Self { slots }) @@ -787,14 +784,14 @@ async fn poll_beacon_attesters_for_epoch( // request for extra data unless necessary in order to save on network bandwidth. let uninitialized_validators = get_uninitialized_validators(duties_service, &epoch, local_pubkeys); - let indices_to_request = if !uninitialized_validators.is_empty() { + let initial_indices_to_request = if !uninitialized_validators.is_empty() { uninitialized_validators.as_slice() } else { &local_indices[0..min(INITIAL_DUTIES_QUERY_SIZE, local_indices.len())] }; let response = - post_validator_duties_attester(duties_service, epoch, indices_to_request).await?; + post_validator_duties_attester(duties_service, epoch, initial_indices_to_request).await?; let dependent_root = response.dependent_root; // Find any validators which have conflicting (epoch, dependent_root) values or missing duties for the epoch. @@ -818,24 +815,29 @@ async fn poll_beacon_attesters_for_epoch( return Ok(()); } - // Filter out validators which have already been requested. - let initial_duties = &response.data; + // Make a request for all indices that require updating which we have not already made a request + // for. let indices_to_request = validators_to_update .iter() - .filter(|&&&pubkey| !initial_duties.iter().any(|duty| duty.pubkey == pubkey)) .filter_map(|pubkey| duties_service.validator_store.validator_index(pubkey)) + .filter(|validator_index| !initial_indices_to_request.contains(validator_index)) .collect::>(); - let new_duties = if !indices_to_request.is_empty() { + // Filter the initial duties by their relevance so that we don't hit the warning below about + // overwriting duties. There was previously a bug here. + let new_initial_duties = response + .data + .into_iter() + .filter(|duty| validators_to_update.contains(&&duty.pubkey)); + + let mut new_duties = if !indices_to_request.is_empty() { post_validator_duties_attester(duties_service, epoch, indices_to_request.as_slice()) .await? .data - .into_iter() - .chain(response.data) - .collect::>() } else { - response.data + vec![] }; + new_duties.extend(new_initial_duties); drop(fetch_timer); @@ -854,26 +856,53 @@ async fn poll_beacon_attesters_for_epoch( // Update the duties service with the new `DutyAndProof` messages. let mut attesters = duties_service.attesters.write(); let mut already_warned = Some(()); + let current_slot = duties_service + .slot_clock + .now_or_genesis() + .unwrap_or_default(); for duty in &new_duties { let attester_map = attesters.entry(duty.pubkey).or_default(); // Create initial entries in the map without selection proofs. We'll compute them in the // background later to avoid creating a thundering herd of signing threads whenever new // duties are computed. - let duty_and_proof = DutyAndProof::new_without_selection_proof(duty.clone()); + let duty_and_proof = DutyAndProof::new_without_selection_proof(duty.clone(), current_slot); + + match attester_map.entry(epoch) { + hash_map::Entry::Occupied(mut occupied) => { + let mut_value = occupied.get_mut(); + let (prior_dependent_root, prior_duty_and_proof) = &mut_value; + + // Guard against overwriting an existing value for the same duty. If we did + // overwrite we could lose a selection proof or information from + // `subscription_slots`. Hitting this branch should be prevented by our logic for + // fetching duties only for unknown indices. + if dependent_root == *prior_dependent_root + && prior_duty_and_proof.duty == duty_and_proof.duty + { + warn!( + log, + "Redundant attester duty update"; + "dependent_root" => %dependent_root, + "validator_index" => duty.validator_index, + ); + continue; + } - if let Some((prior_dependent_root, _)) = - attester_map.insert(epoch, (dependent_root, duty_and_proof)) - { - // Using `already_warned` avoids excessive logs. - if dependent_root != prior_dependent_root && already_warned.take().is_some() { - warn!( - log, - "Attester duties re-org"; - "prior_dependent_root" => %prior_dependent_root, - "dependent_root" => %dependent_root, - "msg" => "this may happen from time to time" - ) + // Using `already_warned` avoids excessive logs. + if dependent_root != *prior_dependent_root && already_warned.take().is_some() { + warn!( + log, + "Attester duties re-org"; + "prior_dependent_root" => %prior_dependent_root, + "dependent_root" => %dependent_root, + "msg" => "this may happen from time to time" + ) + } + *mut_value = (dependent_root, duty_and_proof); + } + hash_map::Entry::Vacant(vacant) => { + vacant.insert((dependent_root, duty_and_proof)); } } } @@ -1030,12 +1059,13 @@ async fn fill_in_selection_proofs( // Sign selection proofs (serially). let duty_and_proof_results = stream::iter(relevant_duties.into_values().flatten()) .then(|duty| async { - DutyAndProof::new_with_selection_proof( - duty, + let opt_selection_proof = make_selection_proof( + &duty, &duties_service.validator_store, &duties_service.spec, ) - .await + .await?; + Ok((duty, opt_selection_proof)) }) .collect::>() .await; @@ -1043,7 +1073,7 @@ async fn fill_in_selection_proofs( // Add to attesters store. let mut attesters = duties_service.attesters.write(); for result in duty_and_proof_results { - let duty_and_proof = match result { + let (duty, selection_proof) = match result { Ok(duty_and_proof) => duty_and_proof, Err(Error::FailedToProduceSelectionProof( ValidatorStoreError::UnknownPubkey(pubkey), @@ -1071,12 +1101,12 @@ async fn fill_in_selection_proofs( } }; - let attester_map = attesters.entry(duty_and_proof.duty.pubkey).or_default(); - let epoch = duty_and_proof.duty.slot.epoch(E::slots_per_epoch()); + let attester_map = attesters.entry(duty.pubkey).or_default(); + let epoch = duty.slot.epoch(E::slots_per_epoch()); match attester_map.entry(epoch) { hash_map::Entry::Occupied(mut entry) => { // No need to update duties for which no proof was computed. - let Some(selection_proof) = duty_and_proof.selection_proof else { + let Some(selection_proof) = selection_proof else { continue; }; @@ -1097,6 +1127,14 @@ async fn fill_in_selection_proofs( } } hash_map::Entry::Vacant(entry) => { + // This probably shouldn't happen, but we have enough info to fill in the + // entry so we may as well. + let subscription_slots = SubscriptionSlots::new(duty.slot, current_slot); + let duty_and_proof = DutyAndProof { + duty, + selection_proof, + subscription_slots, + }; entry.insert((dependent_root, duty_and_proof)); } } @@ -1320,13 +1358,15 @@ mod test { #[test] fn subscription_slots_exact() { + // Set current slot in the past so no duties are considered expired. + let current_slot = Slot::new(0); for duty_slot in [ - Slot::new(32), + Slot::new(33), Slot::new(47), Slot::new(99), Slot::new(1002003), ] { - let subscription_slots = SubscriptionSlots::new(duty_slot); + let subscription_slots = SubscriptionSlots::new(duty_slot, current_slot); // Run twice to check idempotence (subscription slots shouldn't be marked as done until // we mark them manually). @@ -1360,8 +1400,9 @@ mod test { #[test] fn subscription_slots_mark_multiple() { for (i, offset) in ATTESTATION_SUBSCRIPTION_OFFSETS.into_iter().enumerate() { + let current_slot = Slot::new(0); let duty_slot = Slot::new(64); - let subscription_slots = SubscriptionSlots::new(duty_slot); + let subscription_slots = SubscriptionSlots::new(duty_slot, current_slot); subscription_slots.record_successful_subscription_at(duty_slot - offset); @@ -1376,4 +1417,22 @@ mod test { } } } + + /// Test the boundary condition where all subscription slots are *just* expired. + #[test] + fn subscription_slots_expired() { + let current_slot = Slot::new(100); + let duty_slot = current_slot + ATTESTATION_SUBSCRIPTION_OFFSETS[0]; + let subscription_slots = SubscriptionSlots::new(duty_slot, current_slot); + for offset in ATTESTATION_SUBSCRIPTION_OFFSETS.into_iter() { + let slot = duty_slot - offset; + assert!(!subscription_slots.should_send_subscription_at(slot)); + } + assert!(subscription_slots.slots.is_empty()); + + // If the duty slot is 1 later, we get a non-empty set of duties. + let subscription_slots = SubscriptionSlots::new(duty_slot + 1, current_slot); + assert_eq!(subscription_slots.slots.len(), 1); + assert!(subscription_slots.should_send_subscription_at(current_slot + 1),); + } } From 6aebb49718a4226264bbad2a10944d442c1e73a8 Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Thu, 7 Mar 2024 04:29:05 +1100 Subject: [PATCH 3/5] Update dependency `whoami` (#5351) * Update dependency --- Cargo.lock | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 31e775ec200..1bdced9c7d7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9373,6 +9373,12 @@ version = "0.11.0+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9c8d87e72b64a3b4db28d11ce29237c246188f4f51057d65a7eab63b7987e423" +[[package]] +name = "wasite" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b8dad83b4f25e74f184f64c43b150b91efe7647395b42289f38e50566d82855b" + [[package]] name = "wasm-bindgen" version = "0.2.91" @@ -9558,11 +9564,12 @@ dependencies = [ [[package]] name = "whoami" -version = "1.4.1" +version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22fc3756b8a9133049b26c7f61ab35416c130e8c09b660f5b3958b446f52cc50" +checksum = "0fec781d48b41f8163426ed18e8fc2864c12937df9ce54c88ede7bd47270893e" dependencies = [ - "wasm-bindgen", + "redox_syscall 0.4.1", + "wasite", "web-sys", ] From 258eeb5f09c8280f9c6676ff7396fac266c59cc5 Mon Sep 17 00:00:00 2001 From: chonghe <44791194+chong-he@users.noreply.github.com> Date: Thu, 7 Mar 2024 07:17:42 +0800 Subject: [PATCH 4/5] Delete milagro library (#5298) * fix lib.rs and tests.rs * update decode.rs * auto-delete in Cargo.lock * delete milagro in cargo.toml * remove milagro from makefile * remove milagro from the name * delete milagro in comment * delete milagro in cargo.toml * delete in /testing/ef_tests/cargo.toml * delete milagro in the logical OR * delete milagro in /lighthouse/src/main.rs * delete milagro in /crypto/bls/tests/tests.rs * delete milagro in comment * delete milagro in /testing//ef_test/src//cases/bls_eth_aggregate_pubkeys.rs * delete milagro * delete more in lib.rs * delete more in lib.rs * delete more in lib.rs * delete milagro in /crypto/bls/src/lib.rs * delete milagro in crypto/bls/src/mod.rs * delete milagro.rs --- .github/workflows/test-suite.yml | 2 +- Cargo.lock | 18 -- Makefile | 2 - consensus/types/src/attestation.rs | 2 +- crypto/bls/Cargo.toml | 2 - crypto/bls/src/impls/milagro.rs | 194 ------------------ crypto/bls/src/impls/mod.rs | 2 - crypto/bls/src/lib.rs | 30 +-- crypto/bls/tests/tests.rs | 5 - lighthouse/Cargo.toml | 2 - lighthouse/src/main.rs | 2 - testing/ef_tests/Cargo.toml | 1 - .../src/cases/bls_eth_aggregate_pubkeys.rs | 4 - testing/ef_tests/src/decode.rs | 4 +- 14 files changed, 6 insertions(+), 264 deletions(-) delete mode 100644 crypto/bls/src/impls/milagro.rs diff --git a/.github/workflows/test-suite.yml b/.github/workflows/test-suite.yml index 50ccb02ad3a..975dc1c8be1 100644 --- a/.github/workflows/test-suite.yml +++ b/.github/workflows/test-suite.yml @@ -211,7 +211,7 @@ jobs: channel: stable cache-target: release bins: cargo-nextest - - name: Run consensus-spec-tests with blst, milagro and fake_crypto + - name: Run consensus-spec-tests with blst and fake_crypto run: make nextest-ef - name: Show cache stats if: env.SELF_HOSTED_RUNNERS == 'true' diff --git a/Cargo.lock b/Cargo.lock index 1bdced9c7d7..135c4efe971 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -268,11 +268,6 @@ dependencies = [ "syn 2.0.52", ] -[[package]] -name = "amcl" -version = "0.3.0" -source = "git+https://github.com/sigp/milagro_bls?tag=v1.5.1#d3fc0a40cfe8b72ccda46ba050ee6786a59ce753" - [[package]] name = "android-tzdata" version = "0.1.1" @@ -1162,7 +1157,6 @@ dependencies = [ "ethereum_serde_utils", "ethereum_ssz", "hex", - "milagro_bls", "rand", "serde", "tree_hash", @@ -5468,18 +5462,6 @@ dependencies = [ "quote", ] -[[package]] -name = "milagro_bls" -version = "1.5.1" -source = "git+https://github.com/sigp/milagro_bls?tag=v1.5.1#d3fc0a40cfe8b72ccda46ba050ee6786a59ce753" -dependencies = [ - "amcl", - "hex", - "lazy_static", - "rand", - "zeroize", -] - [[package]] name = "mime" version = "0.3.17" diff --git a/Makefile b/Makefile index 8392d001705..6b6418cb83d 100644 --- a/Makefile +++ b/Makefile @@ -143,7 +143,6 @@ run-ef-tests: rm -rf $(EF_TESTS)/.accessed_file_log.txt cargo test --release -p ef_tests --features "ef_tests,$(EF_TEST_FEATURES)" cargo test --release -p ef_tests --features "ef_tests,$(EF_TEST_FEATURES),fake_crypto" - cargo test --release -p ef_tests --features "ef_tests,$(EF_TEST_FEATURES),milagro" ./$(EF_TESTS)/check_all_files_accessed.py $(EF_TESTS)/.accessed_file_log.txt $(EF_TESTS)/consensus-spec-tests # Runs EF test vectors with nextest @@ -151,7 +150,6 @@ nextest-run-ef-tests: rm -rf $(EF_TESTS)/.accessed_file_log.txt cargo nextest run --release -p ef_tests --features "ef_tests,$(EF_TEST_FEATURES)" cargo nextest run --release -p ef_tests --features "ef_tests,$(EF_TEST_FEATURES),fake_crypto" - cargo nextest run --release -p ef_tests --features "ef_tests,$(EF_TEST_FEATURES),milagro" ./$(EF_TESTS)/check_all_files_accessed.py $(EF_TESTS)/.accessed_file_log.txt $(EF_TESTS)/consensus-spec-tests # Run the tests in the `beacon_chain` crate for all known forks. diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index ac4a583cbb6..d1d75523ad1 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -125,7 +125,7 @@ mod tests { // Check the in-memory size of an `Attestation`, which is useful for reasoning about memory // and preventing regressions. // - // This test will only pass with `blst`, if we run these tests with Milagro or another + // This test will only pass with `blst`, if we run these tests with another // BLS library in future we will have to make it generic. #[test] fn size_of() { diff --git a/crypto/bls/Cargo.toml b/crypto/bls/Cargo.toml index 1216fc2a986..7aa8e02dcab 100644 --- a/crypto/bls/Cargo.toml +++ b/crypto/bls/Cargo.toml @@ -7,7 +7,6 @@ edition = { workspace = true } [dependencies] ethereum_ssz = { workspace = true } tree_hash = { workspace = true } -milagro_bls = { git = "https://github.com/sigp/milagro_bls", tag = "v1.5.1", optional = true } rand = { workspace = true } serde = { workspace = true } ethereum_serde_utils = { workspace = true } @@ -22,7 +21,6 @@ blst = { version = "0.3.3", optional = true } arbitrary = [] default = ["supranational"] fake_crypto = [] -milagro = ["milagro_bls"] supranational = ["blst"] supranational-portable = ["supranational", "blst/portable"] supranational-force-adx = ["supranational", "blst/force-adx"] diff --git a/crypto/bls/src/impls/milagro.rs b/crypto/bls/src/impls/milagro.rs deleted file mode 100644 index eb4767d3c70..00000000000 --- a/crypto/bls/src/impls/milagro.rs +++ /dev/null @@ -1,194 +0,0 @@ -use crate::{ - generic_aggregate_public_key::TAggregatePublicKey, - generic_aggregate_signature::TAggregateSignature, - generic_public_key::{GenericPublicKey, TPublicKey, PUBLIC_KEY_BYTES_LEN}, - generic_secret_key::{TSecretKey, SECRET_KEY_BYTES_LEN}, - generic_signature::{TSignature, SIGNATURE_BYTES_LEN}, - Error, Hash256, ZeroizeHash, -}; -pub use milagro_bls as milagro; -use rand::thread_rng; -use std::iter::ExactSizeIterator; - -/// Provides the externally-facing, core BLS types. -pub mod types { - pub use super::milagro::AggregatePublicKey; - pub use super::milagro::AggregateSignature; - pub use super::milagro::PublicKey; - pub use super::milagro::SecretKey; - pub use super::milagro::Signature; - pub use super::verify_signature_sets; - pub use super::SignatureSet; -} - -pub type SignatureSet<'a> = crate::generic_signature_set::GenericSignatureSet< - 'a, - milagro::PublicKey, - milagro::AggregatePublicKey, - milagro::Signature, - milagro::AggregateSignature, ->; - -pub fn verify_signature_sets<'a>( - signature_sets: impl ExactSizeIterator>, -) -> bool { - if signature_sets.len() == 0 { - return false; - } - - signature_sets - .map(|signature_set| { - let mut aggregate = milagro::AggregatePublicKey::from_public_key( - signature_set.signing_keys.first().ok_or(())?.point(), - ); - - for signing_key in signature_set.signing_keys.iter().skip(1) { - aggregate.add(signing_key.point()) - } - - if signature_set.signature.point().is_none() { - return Err(()); - } - - Ok(( - signature_set.signature.as_ref(), - aggregate, - signature_set.message, - )) - }) - .collect::, ()>>() - .map(|aggregates| { - milagro::AggregateSignature::verify_multiple_aggregate_signatures( - &mut rand::thread_rng(), - aggregates.iter().map(|(signature, aggregate, message)| { - ( - signature - .point() - .expect("guarded against none by previous check"), - aggregate, - message.as_bytes(), - ) - }), - ) - }) - .unwrap_or(false) -} - -impl TPublicKey for milagro::PublicKey { - fn serialize(&self) -> [u8; PUBLIC_KEY_BYTES_LEN] { - let mut bytes = [0; PUBLIC_KEY_BYTES_LEN]; - bytes[..].copy_from_slice(&self.as_bytes()); - bytes - } - - fn deserialize(bytes: &[u8]) -> Result { - Self::from_bytes(bytes).map_err(Into::into) - } -} - -impl TAggregatePublicKey for milagro::AggregatePublicKey { - fn to_public_key(&self) -> GenericPublicKey { - GenericPublicKey::from_point(milagro::PublicKey { - point: self.point.clone(), - }) - } - - fn aggregate(pubkeys: &[GenericPublicKey]) -> Result { - let pubkey_refs = pubkeys.iter().map(|pk| pk.point()).collect::>(); - Ok(milagro::AggregatePublicKey::aggregate(&pubkey_refs)?) - } -} - -impl TSignature for milagro::Signature { - fn serialize(&self) -> [u8; SIGNATURE_BYTES_LEN] { - let mut bytes = [0; SIGNATURE_BYTES_LEN]; - - bytes[..].copy_from_slice(&self.as_bytes()); - - bytes - } - - fn deserialize(bytes: &[u8]) -> Result { - milagro::Signature::from_bytes(&bytes).map_err(Error::MilagroError) - } - - fn verify(&self, pubkey: &milagro::PublicKey, msg: Hash256) -> bool { - self.verify(msg.as_bytes(), pubkey) - } -} - -impl TAggregateSignature - for milagro::AggregateSignature -{ - fn infinity() -> Self { - milagro::AggregateSignature::new() - } - - fn add_assign(&mut self, other: &milagro::Signature) { - self.add(other) - } - - fn add_assign_aggregate(&mut self, other: &Self) { - self.add_aggregate(other) - } - - fn serialize(&self) -> [u8; SIGNATURE_BYTES_LEN] { - let mut bytes = [0; SIGNATURE_BYTES_LEN]; - - bytes[..].copy_from_slice(&self.as_bytes()); - - bytes - } - - fn deserialize(bytes: &[u8]) -> Result { - milagro::AggregateSignature::from_bytes(&bytes).map_err(Error::MilagroError) - } - - fn fast_aggregate_verify( - &self, - msg: Hash256, - pubkeys: &[&GenericPublicKey], - ) -> bool { - let pubkeys = pubkeys.iter().map(|pk| pk.point()).collect::>(); - self.fast_aggregate_verify(msg.as_bytes(), &pubkeys) - } - - fn aggregate_verify( - &self, - msgs: &[Hash256], - pubkeys: &[&GenericPublicKey], - ) -> bool { - let pubkeys = pubkeys.iter().map(|pk| pk.point()).collect::>(); - let msgs = msgs.iter().map(|hash| hash.as_bytes()).collect::>(); - self.aggregate_verify(&msgs, &pubkeys) - } -} - -impl TSecretKey for milagro::SecretKey { - fn random() -> Self { - Self::random(&mut thread_rng()) - } - - fn public_key(&self) -> milagro::PublicKey { - let point = milagro::PublicKey::from_secret_key(self).point; - milagro::PublicKey { point } - } - - fn sign(&self, msg: Hash256) -> milagro::Signature { - let point = milagro::Signature::new(msg.as_bytes(), self).point; - milagro::Signature { point } - } - - fn serialize(&self) -> ZeroizeHash { - let mut bytes = [0; SECRET_KEY_BYTES_LEN]; - - // Takes the right-hand 32 bytes from the secret key. - bytes[..].copy_from_slice(&self.as_bytes()); - - bytes.into() - } - - fn deserialize(bytes: &[u8]) -> Result { - Self::from_bytes(bytes).map_err(Into::into) - } -} diff --git a/crypto/bls/src/impls/mod.rs b/crypto/bls/src/impls/mod.rs index b3f2da77b12..d87c3b12ba3 100644 --- a/crypto/bls/src/impls/mod.rs +++ b/crypto/bls/src/impls/mod.rs @@ -1,5 +1,3 @@ #[cfg(feature = "supranational")] pub mod blst; pub mod fake_crypto; -#[cfg(feature = "milagro")] -pub mod milagro; diff --git a/crypto/bls/src/lib.rs b/crypto/bls/src/lib.rs index 750e1bd5b80..fef9804b784 100644 --- a/crypto/bls/src/lib.rs +++ b/crypto/bls/src/lib.rs @@ -9,15 +9,13 @@ //! are supported via compile-time flags. There are three backends supported via features: //! //! - `supranational`: the pure-assembly, highly optimized version from the `blst` crate. -//! - `milagro`: the classic pure-Rust `milagro_bls` crate. //! - `fake_crypto`: an always-returns-valid implementation that is only useful for testing //! scenarios which intend to *ignore* real cryptography. //! //! This crate uses traits to reduce code-duplication between the two implementations. For example, //! the `GenericPublicKey` struct exported from this crate is generic across the `TPublicKey` trait //! (i.e., `PublicKey`). `TPublicKey` is implemented by all three backends (see the -//! `impls.rs` module). When compiling with the `milagro` feature, we export -//! `type PublicKey = GenericPublicKey`. +//! `impls.rs` module). #[macro_use] mod macros; @@ -43,16 +41,11 @@ pub use zeroize_hash::ZeroizeHash; #[cfg(feature = "supranational")] use blst::BLST_ERROR as BlstError; -#[cfg(feature = "milagro")] -use milagro_bls::AmclError; pub type Hash256 = ethereum_types::H256; #[derive(Clone, Debug, PartialEq)] pub enum Error { - /// An error was raised from the Milagro BLS library. - #[cfg(feature = "milagro")] - MilagroError(AmclError), /// An error was raised from the Supranational BLST BLS library. #[cfg(feature = "supranational")] BlstError(BlstError), @@ -66,13 +59,6 @@ pub enum Error { InvalidZeroSecretKey, } -#[cfg(feature = "milagro")] -impl From for Error { - fn from(e: AmclError) -> Error { - Error::MilagroError(e) - } -} - #[cfg(feature = "supranational")] impl From for Error { fn from(e: BlstError) -> Error { @@ -94,8 +80,7 @@ pub mod generics { } /// Defines all the fundamental BLS points which should be exported by this crate by making -/// concrete the generic type parameters using the points from some external BLS library (e.g., -/// Milagro, BLST). +/// concrete the generic type parameters using the points from some external BLS library (e.g.,BLST). macro_rules! define_mod { ($name: ident, $mod: path) => { pub mod $name { @@ -139,8 +124,6 @@ macro_rules! define_mod { }; } -#[cfg(feature = "milagro")] -define_mod!(milagro_implementations, crate::impls::milagro::types); #[cfg(feature = "supranational")] define_mod!(blst_implementations, crate::impls::blst::types); #[cfg(feature = "fake_crypto")] @@ -149,14 +132,7 @@ define_mod!( crate::impls::fake_crypto::types ); -#[cfg(all(feature = "milagro", not(feature = "fake_crypto"),))] -pub use milagro_implementations::*; - -#[cfg(all( - feature = "supranational", - not(feature = "fake_crypto"), - not(feature = "milagro") -))] +#[cfg(all(feature = "supranational", not(feature = "fake_crypto"),))] pub use blst_implementations::*; #[cfg(feature = "fake_crypto")] diff --git a/crypto/bls/tests/tests.rs b/crypto/bls/tests/tests.rs index ad498dbfa87..478c1b7dc26 100644 --- a/crypto/bls/tests/tests.rs +++ b/crypto/bls/tests/tests.rs @@ -509,8 +509,3 @@ macro_rules! test_suite { mod blst { test_suite!(blst_implementations); } - -#[cfg(all(feature = "milagro", not(debug_assertions)))] -mod milagro { - test_suite!(milagro_implementations); -} diff --git a/lighthouse/Cargo.toml b/lighthouse/Cargo.toml index ffa4727d7f2..77b05e9d137 100644 --- a/lighthouse/Cargo.toml +++ b/lighthouse/Cargo.toml @@ -14,8 +14,6 @@ write_ssz_files = ["beacon_node/write_ssz_files"] portable = ["bls/supranational-portable"] # Compiles BLST so that it always uses ADX instructions. modern = ["bls/supranational-force-adx"] -# Uses the slower Milagro BLS library, which is written in native Rust. -milagro = ["bls/milagro"] # Support minimal spec (used for testing only). spec-minimal = [] # Support Gnosis spec and Gnosis Beacon Chain. diff --git a/lighthouse/src/main.rs b/lighthouse/src/main.rs index d646b9764cd..932b125dc69 100644 --- a/lighthouse/src/main.rs +++ b/lighthouse/src/main.rs @@ -23,8 +23,6 @@ fn bls_library_name() -> &'static str { "blst-portable" } else if cfg!(feature = "modern") { "blst-modern" - } else if cfg!(feature = "milagro") { - "milagro" } else { "blst" } diff --git a/testing/ef_tests/Cargo.toml b/testing/ef_tests/Cargo.toml index 8bc36d008b1..f3d00fa035c 100644 --- a/testing/ef_tests/Cargo.toml +++ b/testing/ef_tests/Cargo.toml @@ -7,7 +7,6 @@ edition = { workspace = true } [features] # `ef_tests` feature must be enabled to actually run the tests ef_tests = [] -milagro = ["bls/milagro"] fake_crypto = ["bls/fake_crypto"] portable = ["beacon_chain/portable"] diff --git a/testing/ef_tests/src/cases/bls_eth_aggregate_pubkeys.rs b/testing/ef_tests/src/cases/bls_eth_aggregate_pubkeys.rs index 8783aa141e9..2a9a393bfdb 100644 --- a/testing/ef_tests/src/cases/bls_eth_aggregate_pubkeys.rs +++ b/testing/ef_tests/src/cases/bls_eth_aggregate_pubkeys.rs @@ -31,10 +31,6 @@ impl Case for BlsEthAggregatePubkeys { { return Ok(()); } - #[cfg(feature = "milagro")] - Err(bls::Error::MilagroError(_)) if self.output.is_none() => { - return Ok(()); - } Err(e) => return Err(Error::FailedToParseTest(format!("{:?}", e))), }; diff --git a/testing/ef_tests/src/decode.rs b/testing/ef_tests/src/decode.rs index b5c0da53a01..e95bddffac3 100644 --- a/testing/ef_tests/src/decode.rs +++ b/testing/ef_tests/src/decode.rs @@ -71,9 +71,7 @@ where f(&bytes).map_err(|e| { match e { // NOTE: this is a bit hacky, but seemingly better than the alternatives - ssz::DecodeError::BytesInvalid(message) - if message.contains("Blst") || message.contains("Milagro") => - { + ssz::DecodeError::BytesInvalid(message) if message.contains("Blst") => { Error::InvalidBLSInput(message) } e => Error::FailedToParseTest(format!( From bf118a17d4db3bb49233fc7bd83c0b07eca1dda2 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 7 Mar 2024 14:31:06 +1100 Subject: [PATCH 5/5] Fix block v3 header decoding (#5366) * Fix block v3 header decoding --- beacon_node/http_api/tests/tests.rs | 88 ++++++++++++++++++++++------- common/eth2/src/types.rs | 4 +- 2 files changed, 69 insertions(+), 23 deletions(-) diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index a7ba2c1ab86..098f9f10512 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -2721,6 +2721,31 @@ impl ApiTester { self } + /// Check that the metadata from the headers & JSON response body are consistent, and that the + /// consensus block value is non-zero. + fn check_block_v3_metadata( + metadata: &ProduceBlockV3Metadata, + response: &JsonProduceBlockV3Response, + ) { + // Compare fork name to ForkVersionedResponse rather than metadata consensus_version, which + // is deserialized to a dummy value. + assert_eq!(Some(metadata.consensus_version), response.version); + assert_eq!(ForkName::Base, response.metadata.consensus_version); + assert_eq!( + metadata.execution_payload_blinded, + response.metadata.execution_payload_blinded + ); + assert_eq!( + metadata.execution_payload_value, + response.metadata.execution_payload_value + ); + assert_eq!( + metadata.consensus_block_value, + response.metadata.consensus_block_value + ); + assert!(!metadata.consensus_block_value.is_zero()); + } + pub async fn test_block_production_v3_ssz(self) -> Self { let fork = self.chain.canonical_head.cached_head().head_fork(); let genesis_validators_root = self.chain.genesis_validators_root; @@ -3582,11 +3607,12 @@ impl ApiTester { let (proposer_index, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: BlindedPayload = match payload_type.data { ProduceBlockV3Response::Blinded(payload) => { @@ -3608,11 +3634,12 @@ impl ApiTester { let (proposer_index, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, Some(0)) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: FullPayload = match payload_type.data { ProduceBlockV3Response::Full(payload) => { @@ -3634,11 +3661,12 @@ impl ApiTester { let (proposer_index, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, Some(u64::MAX)) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: BlindedPayload = match payload_type.data { ProduceBlockV3Response::Blinded(payload) => { @@ -3738,11 +3766,12 @@ impl ApiTester { let (proposer_index, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: BlindedPayload = match payload_type.data { ProduceBlockV3Response::Blinded(payload) => { @@ -3814,11 +3843,12 @@ impl ApiTester { let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: BlindedPayload = match payload_type.data { ProduceBlockV3Response::Blinded(payload) => { @@ -3904,11 +3934,12 @@ impl ApiTester { let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: FullPayload = match payload_type.data { ProduceBlockV3Response::Full(payload) => { @@ -3990,11 +4021,12 @@ impl ApiTester { .unwrap(); let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: FullPayload = match payload_type.data { ProduceBlockV3Response::Full(payload) => { @@ -4076,11 +4108,12 @@ impl ApiTester { let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: FullPayload = match payload_type.data { ProduceBlockV3Response::Full(payload) => { @@ -4160,11 +4193,12 @@ impl ApiTester { let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: FullPayload = match payload_type.data { ProduceBlockV3Response::Full(payload) => { @@ -4216,11 +4250,12 @@ impl ApiTester { let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Full(_) => (), @@ -4282,11 +4317,12 @@ impl ApiTester { let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Full(_) => (), @@ -4390,11 +4426,12 @@ impl ApiTester { .get_test_randao(next_slot, next_slot.epoch(E::slots_per_epoch())) .await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(next_slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Blinded(_) => (), @@ -4410,11 +4447,12 @@ impl ApiTester { .get_test_randao(next_slot, next_slot.epoch(E::slots_per_epoch())) .await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(next_slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Full(_) => (), @@ -4538,11 +4576,12 @@ impl ApiTester { .get_test_randao(next_slot, next_slot.epoch(E::slots_per_epoch())) .await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(next_slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Full(_) => (), @@ -4568,11 +4607,12 @@ impl ApiTester { .get_test_randao(next_slot, next_slot.epoch(E::slots_per_epoch())) .await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(next_slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Blinded(_) => (), @@ -4648,11 +4688,12 @@ impl ApiTester { let (proposer_index, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let payload: FullPayload = match payload_type.data { ProduceBlockV3Response::Full(payload) => { @@ -4717,11 +4758,12 @@ impl ApiTester { let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Blinded(_) => (), @@ -4781,11 +4823,12 @@ impl ApiTester { let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Full(_) => (), @@ -4845,11 +4888,12 @@ impl ApiTester { let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Full(_) => (), @@ -4907,11 +4951,12 @@ impl ApiTester { let epoch = self.chain.epoch().unwrap(); let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); let _block_contents = match payload_type.data { ProduceBlockV3Response::Blinded(payload) => payload, @@ -4979,11 +5024,12 @@ impl ApiTester { let epoch = self.chain.epoch().unwrap(); let (_, randao_reveal) = self.get_test_randao(slot, epoch).await; - let (payload_type, _) = self + let (payload_type, metadata) = self .client .get_validator_blocks_v3::(slot, &randao_reveal, None, None) .await .unwrap(); + Self::check_block_v3_metadata(&metadata, &payload_type); match payload_type.data { ProduceBlockV3Response::Full(_) => (), diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index a301055f34c..8a1cf2ff37e 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -1710,12 +1710,12 @@ impl TryFrom<&HeaderMap> for ProduceBlockV3Metadata { })?; let execution_payload_value = parse_required_header(headers, EXECUTION_PAYLOAD_VALUE_HEADER, |s| { - s.parse::() + Uint256::from_dec_str(s) .map_err(|e| format!("invalid {EXECUTION_PAYLOAD_VALUE_HEADER}: {e:?}")) })?; let consensus_block_value = parse_required_header(headers, CONSENSUS_BLOCK_VALUE_HEADER, |s| { - s.parse::() + Uint256::from_dec_str(s) .map_err(|e| format!("invalid {CONSENSUS_BLOCK_VALUE_HEADER}: {e:?}")) })?;