From b303d2fb7eadf8fa471ad546f4b2fd45d09f6e2c Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 15 Mar 2023 15:32:22 -0400 Subject: [PATCH 01/16] lints --- beacon_node/beacon_chain/src/beacon_chain.rs | 2 +- beacon_node/beacon_chain/src/blob_verification.rs | 10 +++++----- beacon_node/http_api/src/build_block_contents.rs | 4 ++-- .../src/beacon_processor/worker/gossip_methods.rs | 4 ++-- .../src/beacon_processor/worker/rpc_methods.rs | 4 ++-- .../network/src/sync/block_sidecar_coupling.rs | 2 +- beacon_node/network/src/sync/manager.rs | 6 +++--- consensus/types/src/signed_block_contents.rs | 12 ++++++------ validator_client/src/block_service.rs | 3 +-- 9 files changed, 23 insertions(+), 24 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 8f39b3758d..79a3e418e8 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4857,7 +4857,7 @@ impl BeaconChain { )), )?; - kzg_utils::compute_blob_kzg_proof::(kzg, blob, kzg_commitment.clone()) + kzg_utils::compute_blob_kzg_proof::(kzg, blob, *kzg_commitment) .map_err(BlockProductionError::KzgError) }) .collect::, BlockProductionError>>() diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 7a4493c973..e2288dbb80 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -3,7 +3,7 @@ use slot_clock::SlotClock; use std::sync::Arc; use crate::beacon_chain::{BeaconChain, BeaconChainTypes, MAXIMUM_GOSSIP_CLOCK_DISPARITY}; -use crate::{kzg_utils, BeaconChainError}; +use crate::BeaconChainError; use state_processing::per_block_processing::eip4844::eip4844::verify_kzg_commitments_against_transactions; use types::signed_beacon_block::BlobReconstructionError; use types::{ @@ -116,11 +116,11 @@ pub fn validate_blob_for_gossip( } fn verify_data_availability( - blob_sidecar: &BlobsSidecar, + _blob_sidecar: &BlobsSidecar, kzg_commitments: &[KzgCommitment], transactions: &Transactions, - block_slot: Slot, - block_root: Hash256, + _block_slot: Slot, + _block_root: Hash256, chain: &BeaconChain, ) -> Result<(), BlobError> { if verify_kzg_commitments_against_transactions::(transactions, kzg_commitments) @@ -130,7 +130,7 @@ fn verify_data_availability( } // Validatate that the kzg proof is valid against the commitments and blobs - let kzg = chain + let _kzg = chain .kzg .as_ref() .ok_or(BlobError::TrustedSetupNotInitialized)?; diff --git a/beacon_node/http_api/src/build_block_contents.rs b/beacon_node/http_api/src/build_block_contents.rs index 1908c03ea1..9fbde0ce06 100644 --- a/beacon_node/http_api/src/build_block_contents.rs +++ b/beacon_node/http_api/src/build_block_contents.rs @@ -24,9 +24,9 @@ pub fn build_block_contents Worker { #[allow(clippy::too_many_arguments)] pub async fn process_gossip_blob( self, - message_id: MessageId, + _message_id: MessageId, peer_id: PeerId, peer_client: Client, blob_index: u64, signed_blob: Arc>, - seen_duration: Duration, + _seen_duration: Duration, ) { // TODO: gossip verification crit!(self.log, "UNIMPLEMENTED gossip blob verification"; diff --git a/beacon_node/network/src/beacon_processor/worker/rpc_methods.rs b/beacon_node/network/src/beacon_processor/worker/rpc_methods.rs index 4480f37130..78b9de303f 100644 --- a/beacon_node/network/src/beacon_processor/worker/rpc_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/rpc_methods.rs @@ -252,7 +252,7 @@ impl Worker { block_parent_root: block.parent_root, proposer_index: block.proposer_index, blob, - kzg_commitment: block.body.blob_kzg_commitments[known_index].clone(), // TODO: needs to be stored in a more logical way so that this won't panic. + kzg_commitment: block.body.blob_kzg_commitments[known_index], // TODO: needs to be stored in a more logical way so that this won't panic. kzg_proof: kzg_aggregated_proof // TODO: yeah }; self.send_response( @@ -843,7 +843,7 @@ impl Worker { beacon_block_root, beacon_block_slot, blobs: blob_bundle, - kzg_aggregated_proof, + kzg_aggregated_proof: _, }: types::BlobsSidecar<_> = blobs; for (blob_index, blob) in blob_bundle.into_iter().enumerate() { diff --git a/beacon_node/network/src/sync/block_sidecar_coupling.rs b/beacon_node/network/src/sync/block_sidecar_coupling.rs index 438317d1cd..67db9a7a32 100644 --- a/beacon_node/network/src/sync/block_sidecar_coupling.rs +++ b/beacon_node/network/src/sync/block_sidecar_coupling.rs @@ -32,7 +32,7 @@ impl BlocksAndBlobsRequestInfo { pub fn into_responses(self) -> Result>, &'static str> { let BlocksAndBlobsRequestInfo { accumulated_blocks, - mut accumulated_sidecars, + accumulated_sidecars, .. } = self; diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 353d3e896e..768b95273e 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -875,8 +875,8 @@ impl SyncManager { fn rpc_blobs_received( &mut self, request_id: RequestId, - peer_id: PeerId, - maybe_blob: Option::EthSpec>>>, + _peer_id: PeerId, + _maybe_blob: Option::EthSpec>>>, _seen_timestamp: Duration, ) { match request_id { @@ -892,7 +892,7 @@ impl SyncManager { RequestId::RangeBlocks { .. } => { unreachable!("Only-blocks range requests don't receive sidecars") } - RequestId::RangeBlobs { id } => { + RequestId::RangeBlobs { id: _ } => { unimplemented!("Adjust range"); } } diff --git a/consensus/types/src/signed_block_contents.rs b/consensus/types/src/signed_block_contents.rs index bce6233338..7f547c86fa 100644 --- a/consensus/types/src/signed_block_contents.rs +++ b/consensus/types/src/signed_block_contents.rs @@ -4,6 +4,11 @@ use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz_types::VariableList; +pub type BlockContentsTuple = ( + SignedBeaconBlock, + Option, ::MaxBlobsPerBlock>>, +); + /// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobSidecars`]. #[derive(Clone, Debug, Derivative, Serialize, Deserialize)] #[derivative(PartialEq, Hash(bound = "T: EthSpec"))] @@ -24,12 +29,7 @@ impl> SignedBlockContents ( - SignedBeaconBlock, - Option, ::MaxBlobsPerBlock>>, - ) { + pub fn deconstruct(self) -> BlockContentsTuple { match self { SignedBlockContents::BlockAndBlobSidecars(block_and_sidecars) => ( block_and_sidecars.signed_block, diff --git a/validator_client/src/block_service.rs b/validator_client/src/block_service.rs index eb40dee9b3..48d6460168 100644 --- a/validator_client/src/block_service.rs +++ b/validator_client/src/block_service.rs @@ -388,7 +388,6 @@ impl BlockService { )) })? .data - .into() } }; @@ -455,7 +454,7 @@ impl BlockService { ); beacon_node // TODO: need to be adjusted for blobs - .post_beacon_blinded_blocks(&signed_block_contents.signed_block()) + .post_beacon_blinded_blocks(signed_block_contents.signed_block()) .await .map_err(|e| { BlockError::Irrecoverable(format!( From fb7d729d92d500c04243038af8c6a073afdd280f Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 15 Mar 2023 16:03:36 -0400 Subject: [PATCH 02/16] migrate types to API crate --- beacon_node/http_api/src/lib.rs | 5 +- beacon_node/http_api/src/publish_blocks.rs | 3 +- .../http_api/tests/interactive_tests.rs | 7 ++- beacon_node/http_api/tests/tests.rs | 4 +- common/eth2/src/types.rs | 51 +++++++++++++++++++ consensus/types/src/lib.rs | 5 +- consensus/types/src/signed_block_and_blobs.rs | 14 +---- consensus/types/src/signed_block_contents.rs | 49 ------------------ validator_client/src/block_service.rs | 3 +- 9 files changed, 68 insertions(+), 73 deletions(-) delete mode 100644 consensus/types/src/signed_block_contents.rs diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 7ac23a9d90..e48f8d7ad1 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -31,7 +31,8 @@ use beacon_chain::{ pub use block_id::BlockId; use directory::DEFAULT_ROOT_DIR; use eth2::types::{ - self as api_types, EndpointVersion, SkipRandaoVerification, ValidatorId, ValidatorStatus, + self as api_types, EndpointVersion, SignedBlockContents, SkipRandaoVerification, ValidatorId, + ValidatorStatus, }; use lighthouse_network::{types::SyncState, EnrExt, NetworkGlobals, PeerId, PubsubMessage}; use lighthouse_version::version_with_platform; @@ -57,7 +58,7 @@ use types::{ Attestation, AttestationData, AttesterSlashing, BeaconStateError, BlindedPayload, CommitteeCache, ConfigAndPreset, Epoch, EthSpec, ForkName, FullPayload, ProposerPreparationData, ProposerSlashing, RelativeEpoch, SignedAggregateAndProof, - SignedBeaconBlock, SignedBlindedBeaconBlock, SignedBlockContents, SignedBlsToExecutionChange, + SignedBeaconBlock, SignedBlindedBeaconBlock, SignedBlsToExecutionChange, SignedContributionAndProof, SignedValidatorRegistrationData, SignedVoluntaryExit, Slot, SyncCommitteeMessage, SyncContributionData, }; diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index 49d655785b..dc8bb020ac 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -3,6 +3,7 @@ use beacon_chain::blob_verification::{AsBlock, BlockWrapper, IntoAvailableBlock} use beacon_chain::validator_monitor::{get_block_delay_ms, timestamp_now}; use beacon_chain::NotifyExecutionLayer; use beacon_chain::{BeaconChain, BeaconChainTypes, BlockError, CountUnrealized}; +use eth2::types::SignedBlockContents; use lighthouse_network::PubsubMessage; use network::NetworkMessage; use slog::{debug, error, info, warn, Logger}; @@ -12,7 +13,7 @@ use tokio::sync::mpsc::UnboundedSender; use tree_hash::TreeHash; use types::{ AbstractExecPayload, BlindedPayload, EthSpec, ExecPayload, ExecutionBlockHash, FullPayload, - Hash256, SignedBeaconBlock, SignedBlockContents, + Hash256, SignedBeaconBlock, }; use warp::Rejection; diff --git a/beacon_node/http_api/tests/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index 7db1b22d67..00fa7faff0 100644 --- a/beacon_node/http_api/tests/interactive_tests.rs +++ b/beacon_node/http_api/tests/interactive_tests.rs @@ -513,12 +513,13 @@ pub async fn proposer_boost_re_org_test( let randao_reveal = harness .sign_randao_reveal(&state_b, proposer_index, slot_c) .into(); - let unsigned_block_c = tester + let unsigned_block_contents_c = tester .client .get_validator_blocks(slot_c, &randao_reveal, None) .await .unwrap() .data; + let unsigned_block_c = unsigned_block_contents_c.deconstruct().0; let block_c = harness.sign_beacon_block(unsigned_block_c, &state_b); if should_re_org { @@ -700,7 +701,9 @@ pub async fn fork_choice_before_proposal() { .get_validator_blocks::>(slot_d, &randao_reveal, None) .await .unwrap() - .data; + .data + .deconstruct() + .0; // Head is now B. assert_eq!( diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 977c737fd0..423e2d4de5 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -2065,7 +2065,9 @@ impl ApiTester { .get_validator_blocks::>(slot, &randao_reveal, None) .await .unwrap() - .data; + .data + .deconstruct() + .0; let signed_block = block.sign(&sk, &fork, genesis_validators_root, &self.chain.spec); diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index d328639120..ffd7a60e4d 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -5,6 +5,7 @@ use crate::Error as ServerError; use lighthouse_network::{ConnectionDirection, Enr, Multiaddr, PeerConnectionStatus}; use mime::{Mime, APPLICATION, JSON, OCTET_STREAM, STAR}; use serde::{Deserialize, Serialize}; +use ssz_derive::Encode; use std::cmp::Reverse; use std::convert::TryFrom; use std::fmt; @@ -1322,3 +1323,53 @@ impl> Into> } } } + +pub type BlockContentsTuple = ( + SignedBeaconBlock, + Option, ::MaxBlobsPerBlock>>, +); + +/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobSidecars`]. +#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(untagged)] +#[serde(bound = "T: EthSpec")] +pub enum SignedBlockContents = FullPayload> { + BlockAndBlobSidecars(SignedBeaconBlockAndBlobSidecars), + Block(SignedBeaconBlock), +} + +impl> SignedBlockContents { + pub fn signed_block(&self) -> &SignedBeaconBlock { + match self { + SignedBlockContents::BlockAndBlobSidecars(block_and_sidecars) => { + &block_and_sidecars.signed_block + } + SignedBlockContents::Block(block) => block, + } + } + + pub fn deconstruct(self) -> BlockContentsTuple { + match self { + SignedBlockContents::BlockAndBlobSidecars(block_and_sidecars) => ( + block_and_sidecars.signed_block, + Some(block_and_sidecars.signed_blob_sidecars), + ), + SignedBlockContents::Block(block) => (block, None), + } + } +} + +impl> From> + for SignedBlockContents +{ + fn from(block: SignedBeaconBlock) -> Self { + SignedBlockContents::Block(block) + } +} + +#[derive(Debug, Clone, Serialize, Deserialize, Encode)] +#[serde(bound = "T: EthSpec")] +pub struct SignedBeaconBlockAndBlobSidecars> { + pub signed_block: SignedBeaconBlock, + pub signed_blob_sidecars: VariableList, ::MaxBlobsPerBlock>, +} diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 14f06bb51d..6a86e773a1 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -102,7 +102,6 @@ pub mod blob_sidecar; pub mod blobs_sidecar; pub mod signed_blob; pub mod signed_block_and_blobs; -pub mod signed_block_contents; pub mod transaction; use ethereum_types::{H160, H256}; @@ -185,10 +184,8 @@ pub use crate::signed_beacon_block::{ pub use crate::signed_beacon_block_header::SignedBeaconBlockHeader; pub use crate::signed_blob::*; pub use crate::signed_block_and_blobs::{ - SignedBeaconBlockAndBlobSidecars, SignedBeaconBlockAndBlobsSidecar, - SignedBeaconBlockAndBlobsSidecarDecode, + SignedBeaconBlockAndBlobsSidecar, SignedBeaconBlockAndBlobsSidecarDecode, }; -pub use crate::signed_block_contents::SignedBlockContents; pub use crate::signed_bls_to_execution_change::SignedBlsToExecutionChange; pub use crate::signed_contribution_and_proof::SignedContributionAndProof; pub use crate::signed_voluntary_exit::SignedVoluntaryExit; diff --git a/consensus/types/src/signed_block_and_blobs.rs b/consensus/types/src/signed_block_and_blobs.rs index c6d154ef0f..a3bd34475d 100644 --- a/consensus/types/src/signed_block_and_blobs.rs +++ b/consensus/types/src/signed_block_and_blobs.rs @@ -1,12 +1,8 @@ -use crate::{ - AbstractExecPayload, BlobsSidecar, EthSpec, SignedBeaconBlock, SignedBeaconBlockEip4844, - SignedBlobSidecar, -}; +use crate::{BlobsSidecar, EthSpec, SignedBeaconBlock, SignedBeaconBlockEip4844}; use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz::{Decode, DecodeError}; use ssz_derive::{Decode, Encode}; -use ssz_types::VariableList; use std::sync::Arc; use tree_hash_derive::TreeHash; @@ -37,11 +33,3 @@ impl SignedBeaconBlockAndBlobsSidecar { }) } } - -#[derive(Debug, Clone, Serialize, Deserialize, Encode, TreeHash, Derivative)] -#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] -#[serde(bound = "T: EthSpec")] -pub struct SignedBeaconBlockAndBlobSidecars> { - pub signed_block: SignedBeaconBlock, - pub signed_blob_sidecars: VariableList, ::MaxBlobsPerBlock>, -} diff --git a/consensus/types/src/signed_block_contents.rs b/consensus/types/src/signed_block_contents.rs deleted file mode 100644 index 7f547c86fa..0000000000 --- a/consensus/types/src/signed_block_contents.rs +++ /dev/null @@ -1,49 +0,0 @@ -use crate::signed_block_and_blobs::SignedBeaconBlockAndBlobSidecars; -use crate::{AbstractExecPayload, EthSpec, FullPayload, SignedBeaconBlock, SignedBlobSidecar}; -use derivative::Derivative; -use serde_derive::{Deserialize, Serialize}; -use ssz_types::VariableList; - -pub type BlockContentsTuple = ( - SignedBeaconBlock, - Option, ::MaxBlobsPerBlock>>, -); - -/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobSidecars`]. -#[derive(Clone, Debug, Derivative, Serialize, Deserialize)] -#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] -#[serde(untagged)] -#[serde(bound = "T: EthSpec")] -pub enum SignedBlockContents = FullPayload> { - BlockAndBlobSidecars(SignedBeaconBlockAndBlobSidecars), - Block(SignedBeaconBlock), -} - -impl> SignedBlockContents { - pub fn signed_block(&self) -> &SignedBeaconBlock { - match self { - SignedBlockContents::BlockAndBlobSidecars(block_and_sidecars) => { - &block_and_sidecars.signed_block - } - SignedBlockContents::Block(block) => block, - } - } - - pub fn deconstruct(self) -> BlockContentsTuple { - match self { - SignedBlockContents::BlockAndBlobSidecars(block_and_sidecars) => ( - block_and_sidecars.signed_block, - Some(block_and_sidecars.signed_blob_sidecars), - ), - SignedBlockContents::Block(block) => (block, None), - } - } -} - -impl> From> - for SignedBlockContents -{ - fn from(block: SignedBeaconBlock) -> Self { - SignedBlockContents::Block(block) - } -} diff --git a/validator_client/src/block_service.rs b/validator_client/src/block_service.rs index 48d6460168..0eb9a07c39 100644 --- a/validator_client/src/block_service.rs +++ b/validator_client/src/block_service.rs @@ -7,6 +7,7 @@ use crate::{ }; use crate::{http_metrics::metrics, validator_store::ValidatorStore}; use environment::RuntimeContext; +use eth2::types::SignedBlockContents; use slog::{crit, debug, error, info, trace, warn}; use slot_clock::SlotClock; use std::ops::Deref; @@ -16,7 +17,7 @@ use tokio::sync::mpsc; use tokio::time::sleep; use types::{ AbstractExecPayload, BeaconBlock, BlindedPayload, BlockType, EthSpec, FullPayload, Graffiti, - PublicKeyBytes, SignedBlockContents, Slot, + PublicKeyBytes, Slot, }; #[derive(Debug)] From cf4285e1d4321f76ae0142fed2df0feeead5605a Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 15 Mar 2023 16:34:00 -0400 Subject: [PATCH 03/16] compile tests --- beacon_node/http_api/tests/tests.rs | 58 +++++++++++++++++++---------- common/eth2/src/types.rs | 9 ++++- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 423e2d4de5..dae17006bc 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -61,8 +61,8 @@ struct ApiTester { harness: Arc>>, chain: Arc>>, client: BeaconNodeHttpClient, - next_block: SignedBeaconBlock, - reorg_block: SignedBeaconBlock, + next_block: SignedBlockContents, + reorg_block: SignedBlockContents, attestations: Vec>, contribution_and_proofs: Vec>, attester_slashing: AttesterSlashing, @@ -154,11 +154,13 @@ impl ApiTester { let (next_block, _next_state) = harness .make_block(head.beacon_state.clone(), harness.chain.slot().unwrap()) .await; + let next_block = SignedBlockContents::from(next_block); // `make_block` adds random graffiti, so this will produce an alternate block let (reorg_block, _reorg_state) = harness .make_block(head.beacon_state.clone(), harness.chain.slot().unwrap()) .await; + let reorg_block = SignedBlockContents::from(reorg_block); let head_state_root = head.beacon_state_root(); let attestations = harness @@ -288,11 +290,13 @@ impl ApiTester { let (next_block, _next_state) = harness .make_block(head.beacon_state.clone(), harness.chain.slot().unwrap()) .await; + let next_block = SignedBlockContents::from(next_block); // `make_block` adds random graffiti, so this will produce an alternate block let (reorg_block, _reorg_state) = harness .make_block(head.beacon_state.clone(), harness.chain.slot().unwrap()) .await; + let reorg_block = SignedBlockContents::from(reorg_block); let head_state_root = head.beacon_state_root(); let attestations = harness @@ -975,9 +979,9 @@ impl ApiTester { } pub async fn test_post_beacon_blocks_valid(mut self) -> Self { - let next_block = &self.next_block; + let next_block = self.next_block.clone(); - self.client.post_beacon_blocks(next_block).await.unwrap(); + self.client.post_beacon_blocks(&next_block).await.unwrap(); assert!( self.network_rx.network_recv.recv().await.is_some(), @@ -988,10 +992,14 @@ impl ApiTester { } pub async fn test_post_beacon_blocks_invalid(mut self) -> Self { - let mut next_block = self.next_block.clone(); + let mut next_block = self.next_block.clone().deconstruct().0; *next_block.message_mut().proposer_index_mut() += 1; - assert!(self.client.post_beacon_blocks(&next_block).await.is_err()); + assert!(self + .client + .post_beacon_blocks(&SignedBlockContents::from(next_block)) + .await + .is_err()); assert!( self.network_rx.network_recv.recv().await.is_some(), @@ -2070,8 +2078,12 @@ impl ApiTester { .0; let signed_block = block.sign(&sk, &fork, genesis_validators_root, &self.chain.spec); + let signed_block_contents = SignedBlockContents::from(signed_block.clone()); - self.client.post_beacon_blocks(&signed_block).await.unwrap(); + self.client + .post_beacon_blocks(&signed_block_contents) + .await + .unwrap(); assert_eq!(self.chain.head_beacon_block().as_ref(), &signed_block); @@ -2095,7 +2107,9 @@ impl ApiTester { ) .await .unwrap() - .data; + .data + .deconstruct() + .0; assert_eq!(block.slot(), slot); self.chain.slot_clock.set_slot(slot.as_u64() + 1); } @@ -3762,12 +3776,12 @@ impl ApiTester { // Submit the next block, which is on an epoch boundary, so this will produce a finalized // checkpoint event, head event, and block event - let block_root = self.next_block.canonical_root(); + let block_root = self.next_block.signed_block().canonical_root(); // current_duty_dependent_root = block root because this is the first slot of the epoch let current_duty_dependent_root = self.chain.head_beacon_block_root(); let current_slot = self.chain.slot().unwrap(); - let next_slot = self.next_block.slot(); + let next_slot = self.next_block.signed_block().slot(); let finalization_distance = E::slots_per_epoch() * 2; let expected_block = EventKind::Block(SseBlock { @@ -3779,7 +3793,7 @@ impl ApiTester { let expected_head = EventKind::Head(SseHead { block: block_root, slot: next_slot, - state: self.next_block.state_root(), + state: self.next_block.signed_block().state_root(), current_duty_dependent_root, previous_duty_dependent_root: self .chain @@ -3828,13 +3842,17 @@ impl ApiTester { .unwrap(); let expected_reorg = EventKind::ChainReorg(SseChainReorg { - slot: self.next_block.slot(), + slot: self.next_block.signed_block().slot(), depth: 1, - old_head_block: self.next_block.canonical_root(), - old_head_state: self.next_block.state_root(), - new_head_block: self.reorg_block.canonical_root(), - new_head_state: self.reorg_block.state_root(), - epoch: self.next_block.slot().epoch(E::slots_per_epoch()), + old_head_block: self.next_block.signed_block().canonical_root(), + old_head_state: self.next_block.signed_block().state_root(), + new_head_block: self.reorg_block.signed_block().canonical_root(), + new_head_state: self.reorg_block.signed_block().state_root(), + epoch: self + .next_block + .signed_block() + .slot() + .epoch(E::slots_per_epoch()), execution_optimistic: false, }); @@ -3896,8 +3914,8 @@ impl ApiTester { .await .unwrap(); - let block_root = self.next_block.canonical_root(); - let next_slot = self.next_block.slot(); + let block_root = self.next_block.signed_block().canonical_root(); + let next_slot = self.next_block.signed_block().slot(); let expected_block = EventKind::Block(SseBlock { block: block_root, @@ -3908,7 +3926,7 @@ impl ApiTester { let expected_head = EventKind::Head(SseHead { block: block_root, slot: next_slot, - state: self.next_block.state_root(), + state: self.next_block.signed_block().state_root(), current_duty_dependent_root: self.chain.genesis_block_root, previous_duty_dependent_root: self.chain.genesis_block_root, epoch_transition: false, diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index ffd7a60e4d..db64d74c2a 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -1363,7 +1363,14 @@ impl> From { fn from(block: SignedBeaconBlock) -> Self { - SignedBlockContents::Block(block) + match block { + SignedBeaconBlock::Base(_) + | SignedBeaconBlock::Altair(_) + | SignedBeaconBlock::Merge(_) + | SignedBeaconBlock::Capella(_) => SignedBlockContents::Block(block), + //TODO: error handling, this should be try from + SignedBeaconBlock::Eip4844(_block) => todo!(), + } } } From 73ab4d85d789cb2229b999ff81c73c199b901d3a Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 15 Mar 2023 17:21:18 -0400 Subject: [PATCH 04/16] cache cleanup --- beacon_node/beacon_chain/src/beacon_chain.rs | 4 +- .../beacon_chain/src/gossip_blob_cache.rs | 130 ++++++++---------- beacon_node/execution_layer/src/lib.rs | 5 +- consensus/types/src/beacon_block_body.rs | 2 +- consensus/types/src/blob_sidecar.rs | 11 +- consensus/types/src/lib.rs | 2 +- consensus/types/src/signed_beacon_block.rs | 2 +- 7 files changed, 75 insertions(+), 81 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index d403cbc597..2b90fcb63a 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -6216,9 +6216,7 @@ impl BeaconChain { return Err(BlobError::TransactionCommitmentMismatch); } - self.blob_cache - - // Validatate that the kzg proof is valid against the commitments and blobs + // Validate that the kzg proof is valid against the commitments and blobs let kzg = self .kzg .as_ref() diff --git a/beacon_node/beacon_chain/src/gossip_blob_cache.rs b/beacon_node/beacon_chain/src/gossip_blob_cache.rs index 2904a2bb85..c76d122a78 100644 --- a/beacon_node/beacon_chain/src/gossip_blob_cache.rs +++ b/beacon_node/beacon_chain/src/gossip_blob_cache.rs @@ -1,45 +1,42 @@ use std::collections::{BTreeMap, HashMap, HashSet}; +use std::future::Future; use std::sync::Arc; use parking_lot::{Mutex, RwLock}; -use kzg::KzgCommitment; +use eth2::reqwest::header::Entry; +use kzg::{Kzg, KzgCommitment}; use ssz_types::VariableList; use types::blob_sidecar::{BlobIdentifier, BlobSidecar}; use types::{EthSpec, Hash256}; use crate::blob_verification::{AvailabilityPendingBlock, verify_data_availability}; -use crate::block_verification::IntoExecutionPendingBlock; +use crate::block_verification::{ExecutedBlock, IntoExecutionPendingBlock}; +use crate::{BeaconChainError, BlockError}; +use crate::kzg_utils::validate_blob; +pub enum BlobCacheError { + DuplicateBlob(Hash256) +} /// This cache contains /// - blobs that have been gossip verified /// - commitments for blocks that have been gossip verified, but the commitments themselves /// have not been verified against blobs /// - blocks that have been fully verified and only require a data availability check pub struct GossipBlobCache { - blob_cache: Mutex> + rpc_blob_cache: RwLock>>>, + gossip_blob_cache: Mutex>>, + kzg: Kzg, } struct GossipBlobCacheInner { - // used when all blobs are not yet present and when the block is not yet present - - //TODO(sean) do we want two versions of this cache, one meant to serve RPC? - unverified_blobs: BTreeMap>>, - // used when the block was fully processed before we received all blobs - availability_pending_blocks: HashMap>, - // used to cache kzg commitments from gossip verified blocks in case we receive all blobs during block processing - unverified_commitments: HashMap>, - // used when block + blob kzg verification completes prior before block processing - verified_commitments: HashSet, + verified_blobs: Vec>>, + executed_block: Option>, } impl GossipBlobCache { - pub fn new() -> Self { - + pub fn new(kzg: Kzg) -> Self { Self { - blob_cache: Mutex::new(GossipBlobCacheInner { - unverified_blobs: BTreeMap::new(), - availability_pending_blocks: HashMap::new(), - unverified_commitments: HashMap::new(), - verified_commitments: HashSet::new(), - }) + rpc_blob_cache: RwLock::new(HashMap::new()), + gossip_blob_cache: Mutex::new(HashMap::new()), + kzg, } } @@ -49,59 +46,52 @@ impl GossipBlobCache { /// cached, verify the block and import it. /// /// This should only accept gossip verified blobs, so we should not have to worry about dupes. - pub fn put_blob(&self, blob: Arc>) { - let blob_id = blob.id(); - let blob_cache = self.blob_cache.lock(); + pub fn put_blob(&self, blob: Arc>) -> Result<(),BlobCacheError> { - if let Some(dup) = blob_cache.unverified_blobs.insert(blob_id, blob) { - // return error relating to gossip validation failure + // TODO(remove clones) + let verified = validate_blob(&self.kzg, blob.blob.clone(), blob.kzg_commitment.clone(), blob.kzg_proof)?; + + if verified { + let mut blob_cache = self.gossip_blob_cache.lock(); + + // Gossip cache. + blob_cache.entry(blob.block_root) + .and_modify(|mut inner| { + // All blobs reaching this cache should be gossip verified and gossip verification + // should filter duplicates, as well as validate indices. + inner.verified_blobs.insert(blob.index as usize, blob.clone()); + + if let Some (executed_block) = inner.executed_block.as_ref() { + // trigger reprocessing ? + } + }) + .or_insert(GossipBlobCacheInner { + verified_blobs: vec![blob.clone()], + executed_block: None + }); + + drop(blob_cache); + + // RPC cache. + self.rpc_blob_cache.write().insert(blob.id(), blob.clone()); } - if let Some(availability_pending_block) = blob_cache.availability_pending_blocks.get(&blob.block_root) { - let num_blobs = availability_pending_block.kzg_commitments().len(); - let mut blobs : Vec> = blob_cache.unverified_blobs.range(BlobIdentifier::new(blob.block_root, 0) - ..BlobIdentifier::new(blob.block_root, num_blobs as u64)).collect(); - - if blobs.len() == num_blobs { - // verify - // import - } - } else if let Some(commitments) = blob_cache.unverified_commitments.get(&blob.block_root) { - let num_blobs = commitments.len(); - let mut blobs : Vec> = blob_cache.unverified_blobs.range(BlobIdentifier::new(blob.block_root, 0) - ..BlobIdentifier::new(blob.block_root, num_blobs as u64)).collect(); - - if blobs.len() == num_blobs { - // verify - // cache - } - } + Ok(()) } - - pub fn put_commitments(&self, block_root: Hash256, kzg_commitments: VariableList) { - let blob_cache = self.blob_cache.lock(); - if let Some(dup) = blob_cache.unverified_commitments.insert(block_root, kzg_commitments) { - // return error relating to gossip validation failure - } - - let num_blobs = commitments.len(); - let mut blobs : Vec> = blob_cache.unverified_blobs.range(BlobIdentifier::new(blob.block_root, 0) - ..BlobIdentifier::new(blob.block_root, num_blobs as u64)).collect(); - - if blobs.len() == num_blobs { - // verify - // cache - } - } - - pub fn check_availability_and_import(&self, block_root: Hash256, block: AvailabilityPendingBlock) -> bool { - let blob_cache = self.blob_cache.lock(); - if blob_cache.verified_commitments.contains(&block_root) { - true - } else { - // cache the block - false - } + pub fn put_block(&self, block: ExecutedBlock) -> () { + let mut guard = self.gossip_blob_cache.lock(); + guard.entry(block.block_root).and_modify(|cache|{ + if cache.verified_blobs == block.block.message_eip4844().blob_kzg_commitments() { + // send to reprocessing queue ? + } else if let Some(dup) = cache.executed_block.insert(block) { + // return error + } else { + // log that we cached it + } + }).or_insert(GossipBlobCacheInner { + verified_blobs: vec![], + executed_block: Some(block) + }); } } diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index adfdcd9f6f..a7a8bdfe26 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -44,12 +44,11 @@ use tree_hash::TreeHash; use types::consts::eip4844::BLOB_TX_TYPE; use types::transaction::{AccessTuple, BlobTransaction, EcdsaSignature, SignedBlobTransaction}; use types::Withdrawals; +use types::{AbstractExecPayload, BeaconStateError, ExecPayload, VersionedHash}; use types::{ - blobs_sidecar::{Blobs, KzgCommitments}, - BlindedPayload, BlockType, ChainSpec, Epoch, ExecutionBlockHash, ExecutionPayload, + BlindedPayload, Blobs, BlockType, ChainSpec, Epoch, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, ExecutionPayloadMerge, ForkName, }; -use types::{AbstractExecPayload, BeaconStateError, ExecPayload, VersionedHash}; use types::{ ProposerPreparationData, PublicKeyBytes, Signature, SignedBeaconBlock, Slot, Transaction, Uint256, diff --git a/consensus/types/src/beacon_block_body.rs b/consensus/types/src/beacon_block_body.rs index c717396522..ace5e0f081 100644 --- a/consensus/types/src/beacon_block_body.rs +++ b/consensus/types/src/beacon_block_body.rs @@ -1,5 +1,5 @@ +use crate::test_utils::TestRandom; use crate::*; -use crate::{blobs_sidecar::KzgCommitments, test_utils::TestRandom}; use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index 27523d588d..3484b5cdba 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -1,5 +1,6 @@ use crate::test_utils::TestRandom; use crate::{Blob, EthSpec, Hash256, SignedRoot, Slot}; +use bls::Signature; use derivative::Derivative; use kzg::{KzgCommitment, KzgProof}; use serde_derive::{Deserialize, Serialize}; @@ -34,7 +35,6 @@ pub struct BlobIdentifier { #[derivative(PartialEq, Hash(bound = "T: EthSpec"))] pub struct BlobSidecar { pub block_root: Hash256, - // TODO: fix the type, should fit in u8 as well #[serde(with = "eth2_serde_utils::quoted_u64")] pub index: u64, pub slot: Slot, @@ -52,6 +52,13 @@ pub type BlobSidecarList = VariableList, ::MaxBl impl SignedRoot for BlobSidecar {} impl BlobSidecar { + pub fn id(&self) -> BlobIdentifier { + BlobIdentifier { + block_root: self.block_root, + index: self.index, + } + } + pub fn empty() -> Self { Self::default() } @@ -61,4 +68,4 @@ impl BlobSidecar { // Fixed part Self::empty().as_ssz_bytes().len() } -} \ No newline at end of file +} diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index e0db5419bb..14f06bb51d 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -228,4 +228,4 @@ pub use bls::{ pub use kzg::{KzgCommitment, KzgProof}; pub use ssz_types::{typenum, typenum::Unsigned, BitList, BitVector, FixedVector, VariableList}; -pub use superstruct::superstruct; \ No newline at end of file +pub use superstruct::superstruct; diff --git a/consensus/types/src/signed_beacon_block.rs b/consensus/types/src/signed_beacon_block.rs index 00aad61ff4..ae59690bf2 100644 --- a/consensus/types/src/signed_beacon_block.rs +++ b/consensus/types/src/signed_beacon_block.rs @@ -581,4 +581,4 @@ mod test { assert_eq!(reconstructed, block); } } -} \ No newline at end of file +} From c26ce824a061bfb9377600927f9d39615522b78c Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 15 Mar 2023 17:48:22 -0400 Subject: [PATCH 05/16] general chaos --- .../src/engine_api/json_structures.rs | 4 +- beacon_node/execution_layer/src/lib.rs | 4 +- .../lighthouse_network/src/rpc/protocol.rs | 13 +++---- beacon_node/store/src/hot_cold_store.rs | 16 ++++---- .../store/src/impls/execution_payload.rs | 6 +-- beacon_node/store/src/lib.rs | 2 +- common/eth2/src/lib.rs | 2 +- consensus/types/src/blob_sidecar.rs | 1 + consensus/types/src/lib.rs | 4 -- consensus/types/src/signed_block_and_blobs.rs | 39 ------------------- 10 files changed, 26 insertions(+), 65 deletions(-) delete mode 100644 consensus/types/src/signed_block_and_blobs.rs diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index 658022e119..b90d314737 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -3,10 +3,12 @@ use serde::{Deserialize, Serialize}; use strum::EnumString; use superstruct::superstruct; use types::{ - Blobs, EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadCapella, + EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, ExecutionPayloadMerge, FixedVector, Transaction, Unsigned, VariableList, Withdrawal, }; +use types::beacon_block_body::KzgCommitments; +use types::blob_sidecar::Blobs; #[derive(Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index a7a8bdfe26..5e2039ebad 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -46,13 +46,15 @@ use types::transaction::{AccessTuple, BlobTransaction, EcdsaSignature, SignedBlo use types::Withdrawals; use types::{AbstractExecPayload, BeaconStateError, ExecPayload, VersionedHash}; use types::{ - BlindedPayload, Blobs, BlockType, ChainSpec, Epoch, ExecutionBlockHash, ExecutionPayload, + BlindedPayload, BlockType, ChainSpec, Epoch, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, ExecutionPayloadMerge, ForkName, }; use types::{ ProposerPreparationData, PublicKeyBytes, Signature, SignedBeaconBlock, Slot, Transaction, Uint256, }; +use types::beacon_block_body::KzgCommitments; +use types::blob_sidecar::Blobs; mod block_hash; mod engine_api; diff --git a/beacon_node/lighthouse_network/src/rpc/protocol.rs b/beacon_node/lighthouse_network/src/rpc/protocol.rs index bf7d9ca2a8..a2acc7976a 100644 --- a/beacon_node/lighthouse_network/src/rpc/protocol.rs +++ b/beacon_node/lighthouse_network/src/rpc/protocol.rs @@ -20,11 +20,10 @@ use tokio_util::{ codec::Framed, compat::{Compat, FuturesAsyncReadCompatExt}, }; -use types::BlobsSidecar; use types::{ BeaconBlock, BeaconBlockAltair, BeaconBlockBase, BeaconBlockCapella, BeaconBlockMerge, EmptyBlock, EthSpec, ForkContext, ForkName, Hash256, MainnetEthSpec, Signature, - SignedBeaconBlock, + SignedBeaconBlock, BlobSidecar }; lazy_static! { @@ -115,12 +114,12 @@ lazy_static! { .as_ssz_bytes() .len(); - pub static ref BLOBS_SIDECAR_MIN: usize = BlobsSidecar::::empty().as_ssz_bytes().len(); - pub static ref BLOBS_SIDECAR_MAX: usize = BlobsSidecar::::max_size(); + pub static ref BLOB_SIDECAR_MIN: usize = BlobSidecar::::empty().as_ssz_bytes().len(); + pub static ref BLOB_SIDECAR_MAX: usize = BlobSidecar::::max_size(); //FIXME(sean) these are underestimates - pub static ref SIGNED_BLOCK_AND_BLOBS_MIN: usize = *BLOBS_SIDECAR_MIN + *SIGNED_BEACON_BLOCK_BASE_MIN; - pub static ref SIGNED_BLOCK_AND_BLOBS_MAX: usize =*BLOBS_SIDECAR_MAX + *SIGNED_BEACON_BLOCK_EIP4844_MAX; + pub static ref SIGNED_BLOCK_AND_BLOBS_MIN: usize = *BLOB_SIDECAR_MIN + *SIGNED_BEACON_BLOCK_BASE_MIN; + pub static ref SIGNED_BLOCK_AND_BLOBS_MAX: usize =*BLOB_SIDECAR_MAX + *SIGNED_BEACON_BLOCK_EIP4844_MAX; } /// The maximum bytes that can be sent across the RPC pre-merge. @@ -385,7 +384,7 @@ impl ProtocolId { Protocol::Goodbye => RpcLimits::new(0, 0), // Goodbye request has no response Protocol::BlocksByRange => rpc_block_limits_by_fork(fork_context.current_fork()), Protocol::BlocksByRoot => rpc_block_limits_by_fork(fork_context.current_fork()), - Protocol::BlobsByRange => RpcLimits::new(*BLOBS_SIDECAR_MIN, *BLOBS_SIDECAR_MAX), + Protocol::BlobsByRange => RpcLimits::new(*BLOB_SIDECAR_MIN, *BLOB_SIDECAR_MAX), Protocol::BlobsByRoot => { // TODO: wrong too RpcLimits::new(*SIGNED_BLOCK_AND_BLOBS_MIN, *SIGNED_BLOCK_AND_BLOBS_MAX) diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 60e2f77595..718ff7f722 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -66,7 +66,7 @@ pub struct HotColdDB, Cold: ItemStore> { /// The hot database also contains all blocks. pub hot_db: Hot, /// LRU cache of deserialized blobs. Updated whenever a blob is loaded. - blob_cache: Mutex>>, + blob_cache: Mutex>>, /// LRU cache of deserialized blocks. Updated whenever a block is loaded. block_cache: Mutex>>, /// Chain spec. @@ -547,7 +547,7 @@ impl, Cold: ItemStore> HotColdDB /// Check if the blobs sidecar for a block exists on disk. pub fn blobs_sidecar_exists(&self, block_root: &Hash256) -> Result { - self.get_item::>(block_root) + self.get_item::>(block_root) .map(|blobs| blobs.is_some()) } @@ -568,7 +568,7 @@ impl, Cold: ItemStore> HotColdDB blobs_db.key_delete(DBColumn::BeaconBlob.into(), block_root.as_bytes()) } - pub fn put_blobs(&self, block_root: &Hash256, blobs: BlobsSidecar) -> Result<(), Error> { + pub fn put_blobs(&self, block_root: &Hash256, blobs: BlobSidecarList) -> Result<(), Error> { let blobs_db = self.blobs_db.as_ref().unwrap_or(&self.cold_db); blobs_db.put_bytes( DBColumn::BeaconBlob.into(), @@ -582,7 +582,7 @@ impl, Cold: ItemStore> HotColdDB pub fn blobs_as_kv_store_ops( &self, key: &Hash256, - blobs: &BlobsSidecar, + blobs: &BlobSidecarList, ops: &mut Vec, ) { let db_key = get_key_for_col(DBColumn::BeaconBlob.into(), key.as_bytes()); @@ -925,8 +925,8 @@ impl, Cold: ItemStore> HotColdDB for op in blob_cache_ops.iter_mut() { let reverse_op = match op { StoreOp::PutBlobs(block_root, _) => StoreOp::DeleteBlobs(*block_root), - StoreOp::DeleteBlobs(_) => match blobs_to_delete.pop() { - Some(blobs) => StoreOp::PutBlobs(blobs.beacon_block_root, Arc::new(blobs)), + StoreOp::DeleteBlobs(block_root) => match blobs_to_delete.pop() { + Some(blobs) => StoreOp::PutBlobs(*block_root, Arc::new(blobs)), None => return Err(HotColdDBError::Rollback.into()), }, _ => return Err(HotColdDBError::Rollback.into()), @@ -1320,12 +1320,12 @@ impl, Cold: ItemStore> HotColdDB } /// Fetch a blobs sidecar from the store. - pub fn get_blobs(&self, block_root: &Hash256) -> Result>, Error> { + pub fn get_blobs(&self, block_root: &Hash256) -> Result>, Error> { let blobs_db = self.blobs_db.as_ref().unwrap_or(&self.cold_db); match blobs_db.get_bytes(DBColumn::BeaconBlob.into(), block_root.as_bytes())? { Some(ref blobs_bytes) => { - let blobs = BlobsSidecar::from_ssz_bytes(blobs_bytes)?; + let blobs = BlobSidecarList::from_ssz_bytes(blobs_bytes)?; // FIXME(sean) I was attempting to use a blob cache here but was getting deadlocks, // may want to attempt to use one again self.blob_cache.lock().put(*block_root, blobs.clone()); diff --git a/beacon_node/store/src/impls/execution_payload.rs b/beacon_node/store/src/impls/execution_payload.rs index 01a2dba0b0..1f5c9debc0 100644 --- a/beacon_node/store/src/impls/execution_payload.rs +++ b/beacon_node/store/src/impls/execution_payload.rs @@ -1,8 +1,8 @@ use crate::{DBColumn, Error, StoreItem}; use ssz::{Decode, Encode}; use types::{ - BlobsSidecar, EthSpec, ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, - ExecutionPayloadMerge, + EthSpec, ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, + ExecutionPayloadMerge,BlobSidecarList }; macro_rules! impl_store_item { @@ -25,7 +25,7 @@ macro_rules! impl_store_item { impl_store_item!(ExecutionPayloadMerge); impl_store_item!(ExecutionPayloadCapella); impl_store_item!(ExecutionPayloadEip4844); -impl_store_item!(BlobsSidecar); +impl_store_item!(BlobSidecarList); /// This fork-agnostic implementation should be only used for writing. /// diff --git a/beacon_node/store/src/lib.rs b/beacon_node/store/src/lib.rs index 3056c29292..ea382a934e 100644 --- a/beacon_node/store/src/lib.rs +++ b/beacon_node/store/src/lib.rs @@ -159,7 +159,7 @@ pub trait ItemStore: KeyValueStore + Sync + Send + Sized + 'stati pub enum StoreOp<'a, E: EthSpec> { PutBlock(Hash256, Arc>), PutState(Hash256, &'a BeaconState), - PutBlobs(Hash256, Arc>), + PutBlobs(Hash256, Arc>), PutOrphanedBlobsKey(Hash256), PutStateSummary(Hash256, HotStateSummary), PutStateTemporaryFlag(Hash256), diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index 2a27d31da9..fec41b8bca 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -704,7 +704,7 @@ impl BeaconNodeHttpClient { pub async fn get_blobs_sidecar( &self, block_id: BlockId, - ) -> Result>>, Error> { + ) -> Result>>, Error> { let path = self.get_blobs_sidecar_path(block_id)?; let response = match self.get_response(path, |b| b).await.optional()? { Some(res) => res, diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index 3484b5cdba..ad6509cb0f 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -48,6 +48,7 @@ pub struct BlobSidecar { } pub type BlobSidecarList = VariableList, ::MaxBlobsPerBlock>; +pub type Blobs = VariableList, ::MaxExtraDataBytes>; impl SignedRoot for BlobSidecar {} diff --git a/consensus/types/src/lib.rs b/consensus/types/src/lib.rs index 592b6cc758..c866442605 100644 --- a/consensus/types/src/lib.rs +++ b/consensus/types/src/lib.rs @@ -100,7 +100,6 @@ pub mod sqlite; pub mod beacon_block_and_blob_sidecars; pub mod blob_sidecar; pub mod signed_blob; -pub mod signed_block_and_blobs; pub mod transaction; use ethereum_types::{H160, H256}; @@ -181,9 +180,6 @@ pub use crate::signed_beacon_block::{ }; pub use crate::signed_beacon_block_header::SignedBeaconBlockHeader; pub use crate::signed_blob::*; -pub use crate::signed_block_and_blobs::{ - SignedBeaconBlockAndBlobsSidecar, SignedBeaconBlockAndBlobsSidecarDecode, -}; pub use crate::signed_bls_to_execution_change::SignedBlsToExecutionChange; pub use crate::signed_contribution_and_proof::SignedContributionAndProof; pub use crate::signed_voluntary_exit::SignedVoluntaryExit; diff --git a/consensus/types/src/signed_block_and_blobs.rs b/consensus/types/src/signed_block_and_blobs.rs deleted file mode 100644 index 9bfdfc4576..0000000000 --- a/consensus/types/src/signed_block_and_blobs.rs +++ /dev/null @@ -1,39 +0,0 @@ -use crate::{ - AbstractExecPayload, EthSpec, SignedBeaconBlock, SignedBeaconBlockEip4844, - SignedBlobSidecar, -}; -use crate::{BlobsSidecar, EthSpec, SignedBeaconBlock, SignedBeaconBlockEip4844}; -use derivative::Derivative; -use serde_derive::{Deserialize, Serialize}; -use ssz::{Decode, DecodeError}; -use ssz_derive::{Decode, Encode}; -use std::sync::Arc; -use tree_hash_derive::TreeHash; - -#[derive(Debug, Clone, Serialize, Deserialize, Encode, Decode, TreeHash, PartialEq)] -#[serde(bound = "T: EthSpec")] -pub struct SignedBeaconBlockAndBlobsSidecarDecode { - pub beacon_block: SignedBeaconBlockEip4844, - pub blobs_sidecar: BlobsSidecar, -} - -// TODO: will be removed once we decouple blobs in Gossip -#[derive(Debug, Clone, Serialize, Deserialize, Encode, TreeHash, Derivative)] -#[derivative(PartialEq, Hash(bound = "T: EthSpec"))] -pub struct SignedBeaconBlockAndBlobsSidecar { - pub beacon_block: Arc>, - pub blobs_sidecar: Arc>, -} - -impl SignedBeaconBlockAndBlobsSidecar { - pub fn from_ssz_bytes(bytes: &[u8]) -> Result { - let SignedBeaconBlockAndBlobsSidecarDecode { - beacon_block, - blobs_sidecar, - } = SignedBeaconBlockAndBlobsSidecarDecode::from_ssz_bytes(bytes)?; - Ok(SignedBeaconBlockAndBlobsSidecar { - beacon_block: Arc::new(SignedBeaconBlock::Eip4844(beacon_block)), - blobs_sidecar: Arc::new(blobs_sidecar), - }) - } -} From 4edca9117d29abb288924ba138d08ffe0fef1bd0 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 15 Mar 2023 20:24:47 -0400 Subject: [PATCH 06/16] extended chaos --- beacon_node/beacon_chain/src/beacon_chain.rs | 35 +-- .../beacon_chain/src/blob_verification.rs | 269 +++--------------- .../beacon_chain/src/block_verification.rs | 44 +-- .../beacon_chain/src/early_attester_cache.rs | 6 +- .../beacon_chain/src/gossip_blob_cache.rs | 45 +-- consensus/types/src/blob_sidecar.rs | 2 +- 6 files changed, 91 insertions(+), 310 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 8be199d719..0c37e70021 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -8,7 +8,7 @@ use crate::beacon_proposer_cache::compute_proposer_duties_from_head; use crate::beacon_proposer_cache::BeaconProposerCache; use crate::blob_cache::BlobCache; use crate::blob_verification::{ - AsBlock, AvailabilityPendingBlock, AvailableBlock, BlobError, Blobs, BlockWrapper, + AsBlock, AvailableBlock, BlobError, Blobs, BlockWrapper, IntoAvailableBlock, }; use crate::block_times_cache::BlockTimesCache; @@ -442,8 +442,7 @@ pub struct BeaconChain { /// Provides monitoring of a set of explicitly defined validators. pub validator_monitor: RwLock>, pub blob_cache: BlobCache, - pub blob_cache: BlobCache, - pub kzg: Option>, + pub kzg: Option>, } type BeaconBlockAndState = (BeaconBlock, BeaconState); @@ -1084,34 +1083,8 @@ impl BeaconChain { pub fn get_blobs( &self, block_root: &Hash256, - data_availability_boundary: Epoch, - ) -> Result>, Error> { - match self.store.get_blobs(block_root)? { - Some(blobs) => Ok(Some(blobs)), - None => { - // Check for the corresponding block to understand whether we *should* have blobs. - self.get_blinded_block(block_root)? - .map(|block| { - // If there are no KZG commitments in the block, we know the sidecar should - // be empty. - let expected_kzg_commitments = - match block.message().body().blob_kzg_commitments() { - Ok(kzg_commitments) => kzg_commitments, - Err(_) => return Err(Error::NoKzgCommitmentsFieldOnBlock), - }; - if expected_kzg_commitments.is_empty() { - Ok(BlobsSidecar::empty_from_parts(*block_root, block.slot())) - } else if data_availability_boundary <= block.epoch() { - // We should have blobs for all blocks younger than the boundary. - Err(Error::BlobsUnavailable) - } else { - // We shouldn't have blobs for blocks older than the boundary. - Err(Error::BlobsOlderThanDataAvailabilityBoundary(block.epoch())) - } - }) - .transpose() - } - } + ) -> Result>, Error> { + self.store.get_blobs(block_root) } pub fn get_blinded_block( diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 50c58391e9..04b5c1a4bb 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -12,8 +12,8 @@ use crate::BeaconChainError; use state_processing::per_block_processing::eip4844::eip4844::verify_kzg_commitments_against_transactions; use types::blob_sidecar::BlobSidecar; use types::{ - BeaconBlockRef, BeaconStateError, BlobsSidecar, EthSpec, Hash256, KzgCommitment, - SignedBeaconBlock, SignedBeaconBlockAndBlobsSidecar, SignedBeaconBlockHeader, + BeaconBlockRef, BeaconStateError, EthSpec, Hash256, KzgCommitment, + SignedBeaconBlock, SignedBeaconBlockHeader, SignedBlobSidecar, Slot, Transactions, }; use types::{Epoch, ExecPayload}; @@ -289,23 +289,6 @@ pub enum DataAvailabilityCheckRequired { No, } -impl BlockWrapper { - fn into_availablilty_pending_block( - self, - block_root: Hash256, - chain: &BeaconChain, - ) -> Result, BlobError> { - match self { - BlockWrapper::Block(block) => { - AvailabilityPendingBlock::new(block, block_root, da_check_required) - } - BlockWrapper::BlockAndBlobs(block, blobs_sidecar) => { - AvailabilityPendingBlock::new_with_blobs(block, blobs_sidecar, da_check_required) - } - } - } -} - pub trait IntoAvailableBlock { fn into_available_block( self, @@ -314,39 +297,32 @@ pub trait IntoAvailableBlock { ) -> Result, BlobError>; } -/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. An -/// `AvailableBlock` has passed any required data availability checks and should be used in -/// consensus. -#[derive(Clone, Debug, Derivative)] -#[derivative(PartialEq, Hash(bound = "T: BeaconChainTypes"))] -pub struct AvailabilityPendingBlock { - block: Arc>, - data_availability_handle: DataAvailabilityHandle, +impl IntoAvailableBlock for BlockWrapper { + fn into_available_block(self, block_root: Hash256, chain: &BeaconChain) -> Result, BlobError> { + todo!() + } } -/// Used to await the result of data availability check. -type DataAvailabilityHandle = JoinHandle>, BlobError>>; - #[derive(Clone, Debug, Derivative)] #[derivative(PartialEq, Hash(bound = "T: BeaconChainTypes"))] -pub struct AvailableBlock { - block: Arc>, - blobs: Blobs, +pub struct AvailableBlock { + block: Arc>, + blobs: Blobs, } -impl AvailableBlock { +impl AvailableBlock { pub fn blobs(&self) -> Option>> { match &self.blobs { Blobs::NotRequired | Blobs::None => None, - Blobs::Available(block_sidecar) => Some(block_sidecar.clone()), + Blobs::Available(blob_sidecar) => Some(blob_sidecar.clone()), } } pub fn deconstruct( self, ) -> ( - Arc>, - Option>>, + Arc>, + Option>>, ) { match self.blobs { Blobs::NotRequired | Blobs::None => (self.block, None), @@ -365,119 +341,6 @@ pub enum Blobs { None, } -//TODO(sean) add block root to the availability pending block? -impl AvailabilityPendingBlock { - pub fn new( - beacon_block: Arc>, - block_root: Hash256, - chain: &BeaconChain, - ) -> Result { - let data_availability_boundary = chain.data_availability_boundary(); - let da_check_required = - data_availability_boundary.map_or(DataAvailabilityCheckRequired::No, |boundary| { - if chain.epoch()? >= boundary { - DataAvailabilityCheckRequired::Yes - } else { - DataAvailabilityCheckRequired::No - } - }); - - match beacon_block.as_ref() { - // No data availability check required prior to Eip4844. - SignedBeaconBlock::Base(_) - | SignedBeaconBlock::Altair(_) - | SignedBeaconBlock::Capella(_) - | SignedBeaconBlock::Merge(_) => Ok(AvailabilityPendingBlock { - block: beacon_block, - data_availability_handle: async { Ok(Some(Blobs::NotRequired)) }, - }), - SignedBeaconBlock::Eip4844(_) => { - match da_check_required { - DataAvailabilityCheckRequired::Yes => { - // Attempt to reconstruct empty blobs here. - let blobs_sidecar = beacon_block - .reconstruct_empty_blobs(Some(block_root)) - .map(Arc::new)?; - Ok(AvailableBlock(AvailableBlockInner::BlockAndBlob( - SignedBeaconBlockAndBlobsSidecar { - beacon_block, - blobs_sidecar, - }, - ))) - } - DataAvailabilityCheckRequired::No => AvailabilityPendingBlock { - block: beacon_block, - data_availability_handle: async { Ok(Some(Blobs::NotRequired)) }, - }, - } - } - } - } - - /// This function is private because an `AvailableBlock` should be - /// constructed via the `into_available_block` method. - //TODO(sean) do we want this to optionally cricumvent the beacon cache? - fn new_with_blobs( - beacon_block: Arc>, - blobs_sidecar: Arc>, - chain: &BeaconChain, - ) -> Result { - let data_availability_boundary = chain.data_availability_boundary(); - let da_check_required = - data_availability_boundary.map_or(DataAvailabilityCheckRequired::No, |boundary| { - if chain.epoch()? >= boundary { - DataAvailabilityCheckRequired::Yes - } else { - DataAvailabilityCheckRequired::No - } - }); - match beacon_block.as_ref() { - // This method shouldn't be called with a pre-Eip4844 block. - SignedBeaconBlock::Base(_) - | SignedBeaconBlock::Altair(_) - | SignedBeaconBlock::Capella(_) - | SignedBeaconBlock::Merge(_) => Err(BlobError::InconsistentFork), - SignedBeaconBlock::Eip4844(_) => { - match da_check_required { - DataAvailabilityCheckRequired::Yes => Ok(AvailableBlock { - block: beacon_block, - blobs: Blobs::Available(blobs_sidecar), - }), - DataAvailabilityCheckRequired::No => { - // Blobs were not verified so we drop them, we'll instead just pass around - // an available `Eip4844` block without blobs. - Ok(AvailableBlock { - block: beacon_block, - blobs: Blobs::NotRequired, - }) - } - } - } - } - } -} - -pub trait IntoBlockWrapper: AsBlock { - fn into_block_wrapper(self) -> BlockWrapper; -} - -impl IntoBlockWrapper for BlockWrapper { - fn into_block_wrapper(self) -> BlockWrapper { - self - } -} - -impl IntoBlockWrapper for AvailabilityPendingBlock { - fn into_block_wrapper(self) -> BlockWrapper { - let (block, blobs) = self.deconstruct(); - if let Some(blobs) = blobs { - BlockWrapper::BlockAndBlobs(block, blobs) - } else { - BlockWrapper::Block(block) - } - } -} - pub trait AsBlock { fn slot(&self) -> Slot; fn epoch(&self) -> Epoch; @@ -542,7 +405,7 @@ impl AsBlock for BlockWrapper { fn canonical_root(&self) -> Hash256 { match &self { BlockWrapper::Block(block) => block.canonical_root(), - BlockWrapper::BlockAndBlob(block, _) => block.canonical_root(), + BlockWrapper::BlockAndBlobs(block, _) => block.canonical_root(), } } } @@ -599,82 +462,42 @@ impl AsBlock for &BlockWrapper { fn canonical_root(&self) -> Hash256 { match &self { BlockWrapper::Block(block) => block.canonical_root(), - BlockWrapper::BlockAndBlob(block, _) => block.canonical_root(), + BlockWrapper::BlockAndBlobs(block, _) => block.canonical_root(), } } } -impl AsBlock for AvailabilityPendingBlock { - fn slot(&self) -> Slot { - match &self.0 { - AvailableBlockInner::Block(block) => block.slot(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.slot() - } - } - } - fn epoch(&self) -> Epoch { - match &self.0 { - AvailableBlockInner::Block(block) => block.epoch(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.epoch() - } - } - } - fn parent_root(&self) -> Hash256 { - match &self.0 { - AvailableBlockInner::Block(block) => block.parent_root(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.parent_root() - } - } - } - fn state_root(&self) -> Hash256 { - match &self.0 { - AvailableBlockInner::Block(block) => block.state_root(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.state_root() - } - } - } - fn signed_block_header(&self) -> SignedBeaconBlockHeader { - match &self.0 { - AvailableBlockInner::Block(block) => block.signed_block_header(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.signed_block_header() - } - } - } - fn message(&self) -> BeaconBlockRef { - match &self.0 { - AvailableBlockInner::Block(block) => block.message(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.message() - } - } - } - fn as_block(&self) -> &SignedBeaconBlock { - match &self.0 { - AvailableBlockInner::Block(block) => block, - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - &block_sidecar_pair.beacon_block - } - } - } - fn block_cloned(&self) -> Arc> { - match &self.0 { - AvailableBlockInner::Block(block) => block.clone(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.clone() - } - } - } - fn canonical_root(&self) -> Hash256 { - match &self.0 { - AvailableBlockInner::Block(block) => block.canonical_root(), - AvailableBlockInner::BlockAndBlob(block_sidecar_pair) => { - block_sidecar_pair.beacon_block.canonical_root() - } +/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. This makes no +/// claims about data availability and should not be used in consensus. This struct is useful in +/// networking when we want to send blocks around without consensus checks. +#[derive(Clone, Debug, Derivative)] +#[derivative(PartialEq, Hash(bound = "E: EthSpec"))] +pub enum BlockWrapper { + Block(Arc>), + BlockAndBlobs(Arc>, Arc>), +} + +impl BlockWrapper { + pub fn new( + block: Arc>, + blobs_sidecar: Option>>, + ) -> Self { + if let Some(blobs_sidecar) = blobs_sidecar { + BlockWrapper::BlockAndBlobs(block, blobs_sidecar) + } else { + BlockWrapper::Block(block) } } } + +impl From> for BlockWrapper { + fn from(block: SignedBeaconBlock) -> Self { + BlockWrapper::Block(Arc::new(block)) + } +} + +impl From>> for BlockWrapper { + fn from(block: Arc>) -> Self { + BlockWrapper::Block(block) + } +} diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 1e420f8a4b..00603727ad 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -49,7 +49,7 @@ #![allow(clippy::result_large_err)] use crate::blob_verification::{ - validate_blob_for_gossip, AsBlock, AvailabilityPendingBlock, AvailableBlock, BlobError, + AsBlock, AvailableBlock, BlobError, BlockWrapper, IntoAvailableBlock, IntoBlockWrapper, }; use crate::eth1_finalization_cache::Eth1FinalizationData; @@ -627,7 +627,7 @@ pub fn signature_verify_chain_segment( #[derive(Derivative)] #[derivative(Debug(bound = "T: BeaconChainTypes"))] pub struct GossipVerifiedBlock { - pub block: AvailabilityPendingBlock, + pub block: Arc>, pub block_root: Hash256, parent: Option>, consensus_context: ConsensusContext, @@ -636,7 +636,7 @@ pub struct GossipVerifiedBlock { /// A wrapper around a `SignedBeaconBlock` that indicates that all signatures (except the deposit /// signatures) have been verified. pub struct SignatureVerifiedBlock { - block: AvailabilityPendingBlock, + block: Arc>, block_root: Hash256, parent: Option>, consensus_context: ConsensusContext, @@ -689,7 +689,7 @@ pub trait IntoExecutionPendingBlock: Sized { block_root: Hash256, chain: &Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockError> { + ) -> Result, BlockError> { self.into_execution_pending_block_slashable(block_root, chain, notify_execution_layer) .map(|execution_pending| { // Supply valid block to slasher. @@ -707,7 +707,7 @@ pub trait IntoExecutionPendingBlock: Sized { block_root: Hash256, chain: &Arc>, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockSlashInfo>>; + ) -> Result, BlockSlashInfo>>; fn block(&self) -> &SignedBeaconBlock; } @@ -930,8 +930,6 @@ impl GossipVerifiedBlock { // Validate the block's execution_payload (if any). validate_execution_payload_for_gossip(&parent_block, block.message(), chain)?; - let available_block = validate_blob_for_gossip(block, block_root, chain)?; - // Having checked the proposer index and the block root we can cache them. let consensus_context = ConsensusContext::new(available_block.slot()) .set_current_block_root(block_root) @@ -969,7 +967,7 @@ impl IntoExecutionPendingBlock for GossipVerifiedBlock &SignedBeaconBlock { - self.block.as_block() + self.block.as_ref() } } @@ -979,7 +977,7 @@ impl SignatureVerifiedBlock { /// /// Returns an error if the block is invalid, or if the block was unable to be verified. pub fn new( - block: AvailabilityPendingBlock, + block: Arc>, block_root: Hash256, chain: &BeaconChain, ) -> Result> { @@ -1029,7 +1027,7 @@ impl SignatureVerifiedBlock { /// As for `new` above but producing `BlockSlashInfo`. pub fn check_slashable( - block: AvailabilityPendingBlock, + block: Arc>, block_root: Hash256, chain: &BeaconChain, ) -> Result>> { @@ -1121,7 +1119,7 @@ impl IntoExecutionPendingBlock for SignatureVerifiedBloc } fn block(&self) -> &SignedBeaconBlock { - self.block.as_block() + self.block.as_ref() } } @@ -1152,28 +1150,6 @@ impl IntoExecutionPendingBlock for Arc IntoExecutionPendingBlock for AvailabilityPendingBlock { - /// Verifies the `SignedBeaconBlock` by first transforming it into a `SignatureVerifiedBlock` - /// and then using that implementation of `IntoExecutionPendingBlock` to complete verification. - fn into_execution_pending_block_slashable( - self, - block_root: Hash256, - chain: &Arc>, - notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockSlashInfo>> { - // Perform an early check to prevent wasting time on irrelevant blocks. - let block_root = check_block_relevancy(self.as_block(), block_root, chain) - .map_err(|e| BlockSlashInfo::SignatureNotChecked(self.signed_block_header(), e))?; - - SignatureVerifiedBlock::check_slashable(self, block_root, chain)? - .into_execution_pending_block_slashable(block_root, chain, notify_execution_layer) - } - - fn block(&self) -> &SignedBeaconBlock { - self.as_block() - } -} - impl ExecutionPendingBlock { /// Instantiates `Self`, a wrapper that indicates that the given `block` is fully valid. See /// the struct-level documentation for more information. @@ -1183,7 +1159,7 @@ impl ExecutionPendingBlock { /// /// Returns an error if the block is invalid, or if the block was unable to be verified. pub fn from_signature_verified_components( - block: AvailabilityPendingBlock, + block: Arc>, block_root: Hash256, parent: PreProcessingSnapshot, mut consensus_context: ConsensusContext, diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 2df1a4f05b..a0785f98e8 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -1,4 +1,4 @@ -use crate::blob_verification::{AvailabilityPendingBlock, AvailableBlock}; +use crate::blob_verification::{AvailableBlock}; use crate::{ attester_cache::{CommitteeLengths, Error}, metrics, @@ -21,7 +21,7 @@ pub struct CacheItem { * Values used to make the block available. */ block: Arc>, - blobs: Option>>, + blobs: Option>>, proto_block: ProtoBlock, } @@ -160,7 +160,7 @@ impl EarlyAttesterCache { } /// Returns the blobs, if `block_root` matches the cached item. - pub fn get_blobs(&self, block_root: Hash256) -> Option>> { + pub fn get_blobs(&self, block_root: Hash256) -> Option>> { self.item .read() .as_ref() diff --git a/beacon_node/beacon_chain/src/gossip_blob_cache.rs b/beacon_node/beacon_chain/src/gossip_blob_cache.rs index fa615a9c2a..a932fc2022 100644 --- a/beacon_node/beacon_chain/src/gossip_blob_cache.rs +++ b/beacon_node/beacon_chain/src/gossip_blob_cache.rs @@ -1,7 +1,7 @@ -use crate::blob_verification::{verify_data_availability, AvailabilityPendingBlock}; +use crate::blob_verification::{verify_data_availability}; use crate::block_verification::{ExecutedBlock, IntoExecutionPendingBlock}; use crate::kzg_utils::validate_blob; -use crate::{BeaconChainError, BlockError}; +use crate::{BeaconChainError, BeaconChainTypes, BlockError}; use eth2::reqwest::header::Entry; use kzg::{Kzg, KzgCommitment}; use parking_lot::{Mutex, RwLock}; @@ -11,27 +11,29 @@ use std::future::Future; use std::sync::Arc; use types::blob_sidecar::{BlobIdentifier, BlobSidecar}; use types::{EthSpec, Hash256}; +use kzg::Error as KzgError; pub enum BlobCacheError { DuplicateBlob(Hash256), + Kzg(KzgError), } /// This cache contains /// - blobs that have been gossip verified /// - commitments for blocks that have been gossip verified, but the commitments themselves /// have not been verified against blobs /// - blocks that have been fully verified and only require a data availability check -pub struct GossipBlobCache { - rpc_blob_cache: RwLock>>>, +pub struct GossipBlobCache { + rpc_blob_cache: RwLock>>>, gossip_blob_cache: Mutex>>, kzg: Kzg, } -struct GossipBlobCacheInner { - verified_blobs: Vec>>, +struct GossipBlobCacheInner { + verified_blobs: Vec>>, executed_block: Option>, } -impl GossipBlobCache { +impl GossipBlobCache { pub fn new(kzg: Kzg) -> Self { Self { rpc_blob_cache: RwLock::new(HashMap::new()), @@ -45,14 +47,14 @@ impl GossipBlobCache { /// cached, verify the block and import it. /// /// This should only accept gossip verified blobs, so we should not have to worry about dupes. - pub fn put_blob(&self, blob: Arc>) -> Result<(), BlobCacheError> { + pub fn put_blob(&self, blob: Arc>) -> Result<(), BlobCacheError> { // TODO(remove clones) - let verified = validate_blob( + let verified = validate_blob::( &self.kzg, blob.blob.clone(), blob.kzg_commitment.clone(), blob.kzg_proof, - )?; + ).map_err(|e|BlobCacheError::Kzg(e))?; if verified { let mut blob_cache = self.gossip_blob_cache.lock(); @@ -85,22 +87,29 @@ impl GossipBlobCache { Ok(()) } - pub fn put_block(&self, block: ExecutedBlock) -> () { + pub fn put_block(&self, executed_block: ExecutedBlock) -> Result<(), BlobCacheError> { let mut guard = self.gossip_blob_cache.lock(); guard - .entry(block.block_root) + .entry(executed_block.block_root) .and_modify(|cache| { - if cache.verified_blobs == block.block.message_eip4844().blob_kzg_commitments() { - // send to reprocessing queue ? - } else if let Some(dup) = cache.executed_block.insert(block) { - // return error + if let Ok(block) = executed_block.block.message_eip4844() { + let verified_commitments_vec: Vec<_> = cache.verified_blobs.iter().map(|blob|blob.kzg_commitment).collect(); + let verified_commitments = VariableList::from(verified_commitments_vec); + if verified_commitments == block.body.blob_kzg_commitments { + // send to reprocessing queue ? + } else { + let _ = cache.executed_block.insert(executed_block); + // log that we cached + } } else { - // log that we cached it + // log error } }) .or_insert(GossipBlobCacheInner { verified_blobs: vec![], - executed_block: Some(block), + executed_block: Some(executed_block), }); + + Ok(()) } } diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index ad6509cb0f..502e453aad 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -11,7 +11,7 @@ use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; /// Container of the data that identifies an individual blob. -#[derive(Encode, Decode, Clone, Debug, PartialEq)] +#[derive(Encode, Decode, Clone, Debug, PartialEq, Eq, Hash)] pub struct BlobIdentifier { pub block_root: Hash256, pub index: u64, From 8c79358d3595c257fa24f09cb1b142b16fa4365f Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 15 Mar 2023 22:18:39 -0400 Subject: [PATCH 07/16] cargo fmt --- beacon_node/beacon_chain/src/beacon_chain.rs | 3 +-- .../beacon_chain/src/blob_verification.rs | 20 +++++++++---------- .../beacon_chain/src/block_verification.rs | 5 ++--- .../beacon_chain/src/early_attester_cache.rs | 2 +- .../beacon_chain/src/gossip_blob_cache.rs | 13 ++++++++---- .../src/engine_api/json_structures.rs | 4 ++-- beacon_node/execution_layer/src/lib.rs | 4 ++-- .../lighthouse_network/src/rpc/protocol.rs | 4 ++-- .../store/src/impls/execution_payload.rs | 4 ++-- 9 files changed, 30 insertions(+), 29 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 0c37e70021..6d21187b09 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -8,8 +8,7 @@ use crate::beacon_proposer_cache::compute_proposer_duties_from_head; use crate::beacon_proposer_cache::BeaconProposerCache; use crate::blob_cache::BlobCache; use crate::blob_verification::{ - AsBlock, AvailableBlock, BlobError, Blobs, BlockWrapper, - IntoAvailableBlock, + AsBlock, AvailableBlock, BlobError, Blobs, BlockWrapper, IntoAvailableBlock, }; use crate::block_times_cache::BlockTimesCache; use crate::block_verification::{ diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 04b5c1a4bb..ee51d47a21 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -12,9 +12,8 @@ use crate::BeaconChainError; use state_processing::per_block_processing::eip4844::eip4844::verify_kzg_commitments_against_transactions; use types::blob_sidecar::BlobSidecar; use types::{ - BeaconBlockRef, BeaconStateError, EthSpec, Hash256, KzgCommitment, - SignedBeaconBlock, SignedBeaconBlockHeader, - SignedBlobSidecar, Slot, Transactions, + BeaconBlockRef, BeaconStateError, EthSpec, Hash256, KzgCommitment, SignedBeaconBlock, + SignedBeaconBlockHeader, SignedBlobSidecar, Slot, Transactions, }; use types::{Epoch, ExecPayload}; @@ -297,8 +296,12 @@ pub trait IntoAvailableBlock { ) -> Result, BlobError>; } -impl IntoAvailableBlock for BlockWrapper { - fn into_available_block(self, block_root: Hash256, chain: &BeaconChain) -> Result, BlobError> { +impl IntoAvailableBlock for BlockWrapper { + fn into_available_block( + self, + block_root: Hash256, + chain: &BeaconChain, + ) -> Result, BlobError> { todo!() } } @@ -318,12 +321,7 @@ impl AvailableBlock { } } - pub fn deconstruct( - self, - ) -> ( - Arc>, - Option>>, - ) { + pub fn deconstruct(self) -> (Arc>, Option>>) { match self.blobs { Blobs::NotRequired | Blobs::None => (self.block, None), Blobs::Available(blob_sidecars) => (self.block, Some(blob_sidecars)), diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 00603727ad..a9e4727ada 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -49,8 +49,7 @@ #![allow(clippy::result_large_err)] use crate::blob_verification::{ - AsBlock, AvailableBlock, BlobError, - BlockWrapper, IntoAvailableBlock, IntoBlockWrapper, + AsBlock, AvailableBlock, BlobError, BlockWrapper, IntoAvailableBlock, IntoBlockWrapper, }; use crate::eth1_finalization_cache::Eth1FinalizationData; use crate::execution_payload::{ @@ -933,7 +932,7 @@ impl GossipVerifiedBlock { // Having checked the proposer index and the block root we can cache them. let consensus_context = ConsensusContext::new(available_block.slot()) .set_current_block_root(block_root) - .set_proposer_index(available_block.as_block().message().proposer_index()) + .set_proposer_index(block.as_block().message().proposer_index()) .set_kzg_commitments_consistent(true); Ok(Self { diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index a0785f98e8..11eeeadedc 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -1,4 +1,4 @@ -use crate::blob_verification::{AvailableBlock}; +use crate::blob_verification::AvailableBlock; use crate::{ attester_cache::{CommitteeLengths, Error}, metrics, diff --git a/beacon_node/beacon_chain/src/gossip_blob_cache.rs b/beacon_node/beacon_chain/src/gossip_blob_cache.rs index a932fc2022..f4f2a77f0b 100644 --- a/beacon_node/beacon_chain/src/gossip_blob_cache.rs +++ b/beacon_node/beacon_chain/src/gossip_blob_cache.rs @@ -1,8 +1,9 @@ -use crate::blob_verification::{verify_data_availability}; +use crate::blob_verification::verify_data_availability; use crate::block_verification::{ExecutedBlock, IntoExecutionPendingBlock}; use crate::kzg_utils::validate_blob; use crate::{BeaconChainError, BeaconChainTypes, BlockError}; use eth2::reqwest::header::Entry; +use kzg::Error as KzgError; use kzg::{Kzg, KzgCommitment}; use parking_lot::{Mutex, RwLock}; use ssz_types::VariableList; @@ -11,7 +12,6 @@ use std::future::Future; use std::sync::Arc; use types::blob_sidecar::{BlobIdentifier, BlobSidecar}; use types::{EthSpec, Hash256}; -use kzg::Error as KzgError; pub enum BlobCacheError { DuplicateBlob(Hash256), @@ -54,7 +54,8 @@ impl GossipBlobCache { blob.blob.clone(), blob.kzg_commitment.clone(), blob.kzg_proof, - ).map_err(|e|BlobCacheError::Kzg(e))?; + ) + .map_err(|e| BlobCacheError::Kzg(e))?; if verified { let mut blob_cache = self.gossip_blob_cache.lock(); @@ -93,7 +94,11 @@ impl GossipBlobCache { .entry(executed_block.block_root) .and_modify(|cache| { if let Ok(block) = executed_block.block.message_eip4844() { - let verified_commitments_vec: Vec<_> = cache.verified_blobs.iter().map(|blob|blob.kzg_commitment).collect(); + let verified_commitments_vec: Vec<_> = cache + .verified_blobs + .iter() + .map(|blob| blob.kzg_commitment) + .collect(); let verified_commitments = VariableList::from(verified_commitments_vec); if verified_commitments == block.body.blob_kzg_commitments { // send to reprocessing queue ? diff --git a/beacon_node/execution_layer/src/engine_api/json_structures.rs b/beacon_node/execution_layer/src/engine_api/json_structures.rs index b90d314737..1cb6235a41 100644 --- a/beacon_node/execution_layer/src/engine_api/json_structures.rs +++ b/beacon_node/execution_layer/src/engine_api/json_structures.rs @@ -2,13 +2,13 @@ use super::*; use serde::{Deserialize, Serialize}; use strum::EnumString; use superstruct::superstruct; +use types::beacon_block_body::KzgCommitments; +use types::blob_sidecar::Blobs; use types::{ EthSpec, ExecutionBlockHash, ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, ExecutionPayloadMerge, FixedVector, Transaction, Unsigned, VariableList, Withdrawal, }; -use types::beacon_block_body::KzgCommitments; -use types::blob_sidecar::Blobs; #[derive(Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] diff --git a/beacon_node/execution_layer/src/lib.rs b/beacon_node/execution_layer/src/lib.rs index 5e2039ebad..fd9b794175 100644 --- a/beacon_node/execution_layer/src/lib.rs +++ b/beacon_node/execution_layer/src/lib.rs @@ -41,6 +41,8 @@ use tokio::{ }; use tokio_stream::wrappers::WatchStream; use tree_hash::TreeHash; +use types::beacon_block_body::KzgCommitments; +use types::blob_sidecar::Blobs; use types::consts::eip4844::BLOB_TX_TYPE; use types::transaction::{AccessTuple, BlobTransaction, EcdsaSignature, SignedBlobTransaction}; use types::Withdrawals; @@ -53,8 +55,6 @@ use types::{ ProposerPreparationData, PublicKeyBytes, Signature, SignedBeaconBlock, Slot, Transaction, Uint256, }; -use types::beacon_block_body::KzgCommitments; -use types::blob_sidecar::Blobs; mod block_hash; mod engine_api; diff --git a/beacon_node/lighthouse_network/src/rpc/protocol.rs b/beacon_node/lighthouse_network/src/rpc/protocol.rs index a2acc7976a..8125ea9303 100644 --- a/beacon_node/lighthouse_network/src/rpc/protocol.rs +++ b/beacon_node/lighthouse_network/src/rpc/protocol.rs @@ -22,8 +22,8 @@ use tokio_util::{ }; use types::{ BeaconBlock, BeaconBlockAltair, BeaconBlockBase, BeaconBlockCapella, BeaconBlockMerge, - EmptyBlock, EthSpec, ForkContext, ForkName, Hash256, MainnetEthSpec, Signature, - SignedBeaconBlock, BlobSidecar + BlobSidecar, EmptyBlock, EthSpec, ForkContext, ForkName, Hash256, MainnetEthSpec, Signature, + SignedBeaconBlock, }; lazy_static! { diff --git a/beacon_node/store/src/impls/execution_payload.rs b/beacon_node/store/src/impls/execution_payload.rs index 1f5c9debc0..f4e6c1ea66 100644 --- a/beacon_node/store/src/impls/execution_payload.rs +++ b/beacon_node/store/src/impls/execution_payload.rs @@ -1,8 +1,8 @@ use crate::{DBColumn, Error, StoreItem}; use ssz::{Decode, Encode}; use types::{ - EthSpec, ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, - ExecutionPayloadMerge,BlobSidecarList + BlobSidecarList, EthSpec, ExecutionPayload, ExecutionPayloadCapella, ExecutionPayloadEip4844, + ExecutionPayloadMerge, }; macro_rules! impl_store_item { From 9df968c99212962248ac85f9da06de1f2a2ce6cb Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 16 Mar 2023 20:55:21 +0530 Subject: [PATCH 08/16] more progress --- beacon_node/beacon_chain/src/beacon_chain.rs | 38 ++-- .../beacon_chain/src/blob_verification.rs | 165 +++++++++--------- .../beacon_chain/src/block_verification.rs | 92 ++++++---- consensus/types/src/signed_blob.rs | 21 ++- 4 files changed, 181 insertions(+), 135 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 6d21187b09..482de67043 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2682,6 +2682,7 @@ impl BeaconChain { metrics::inc_counter(&metrics::BLOCK_PROCESSING_REQUESTS); let slot = unverified_block.block().slot(); + let chain = self.clone(); let execution_pending = unverified_block.into_execution_pending_block( block_root, @@ -2694,8 +2695,6 @@ impl BeaconChain { .into_executed_block(execution_pending, count_unrealized) .await?; - let chain = self.clone(); - // Check if the executed block has all it's blobs available to qualify as a fully // available block let import_block = if let Ok(blobs) = self @@ -2840,12 +2839,12 @@ impl BeaconChain { payload_verification_outcome, parent_eth1_finalization_data, consensus_context, - } = execution_pending_block; + } = executed_block; let chain = self.clone(); let available_block = AvailableBlock { - block: block, + block: block.block_cloned(), blobs: blobs, }; @@ -2857,7 +2856,7 @@ impl BeaconChain { block_root, state, confirmed_state_roots, - payload_verification_status, + payload_verification_outcome.payload_verification_status, count_unrealized, parent_block, parent_eth1_finalization_data, @@ -2871,7 +2870,7 @@ impl BeaconChain { Ok(block_hash) } - /// Accepts a fully-verified block and imports it into the chain without performing any + /// Accepts a fully-verified and available block and imports it into the chain without performing any /// additional verification. /// /// An error is returned if the block was unable to be imported. It may be partially imported @@ -2896,7 +2895,7 @@ impl BeaconChain { // ----------------------------------------------------------------------------------------- let current_slot = self.slot()?; let current_epoch = current_slot.epoch(T::EthSpec::slots_per_epoch()); - let block = signed_block.message(); + let block = signed_block.block.message(); let post_exec_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_POST_EXEC_PROCESSING); // Check against weak subjectivity checkpoint. @@ -2933,9 +2932,14 @@ impl BeaconChain { let mut fork_choice = self.canonical_head.fork_choice_write_lock(); // Do not import a block that doesn't descend from the finalized root. - let signed_block = - check_block_is_finalized_checkpoint_or_descendant(self, &fork_choice, signed_block)?; - let block = signed_block.message(); + let signed_block = check_block_is_finalized_checkpoint_or_descendant( + self, + &fork_choice, + BlockWrapper::from(signed_block), + )?; + // TODO(pawan): fix this atrocity + let signed_block = signed_block.into_available_block().unwrap(); + let block = signed_block.block.message(); // Register the new block with the fork choice service. { @@ -3065,14 +3069,12 @@ impl BeaconChain { // margin, or younger (of higher epoch number). if block_epoch >= import_boundary { if let Some(blobs) = blobs { - if !blobs.blobs.is_empty() { - //FIXME(sean) using this for debugging for now - info!( - self.log, "Writing blobs to store"; - "block_root" => ?block_root - ); - ops.push(StoreOp::PutBlobs(block_root, blobs)); - } + //FIXME(sean) using this for debugging for now + info!( + self.log, "Writing blobs to store"; + "block_root" => ?block_root + ); + ops.push(StoreOp::PutBlobs(block_root, blobs)); } } } diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index ee51d47a21..35fb4c1429 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -1,8 +1,5 @@ -use derivative::Derivative; use slot_clock::SlotClock; -use ssz_types::VariableList; use std::sync::Arc; -use tokio::task::JoinHandle; use crate::beacon_chain::{ BeaconChain, BeaconChainTypes, MAXIMUM_GOSSIP_CLOCK_DISPARITY, @@ -10,12 +7,10 @@ use crate::beacon_chain::{ }; use crate::BeaconChainError; use state_processing::per_block_processing::eip4844::eip4844::verify_kzg_commitments_against_transactions; -use types::blob_sidecar::BlobSidecar; use types::{ - BeaconBlockRef, BeaconStateError, EthSpec, Hash256, KzgCommitment, SignedBeaconBlock, - SignedBeaconBlockHeader, SignedBlobSidecar, Slot, Transactions, + BeaconBlockRef, BeaconStateError, BlobSidecarList, Epoch, EthSpec, Hash256, KzgCommitment, + SignedBeaconBlock, SignedBeaconBlockHeader, SignedBlobSidecar, Slot, Transactions, }; -use types::{Epoch, ExecPayload}; #[derive(Debug)] pub enum BlobError { @@ -190,9 +185,9 @@ pub fn validate_blob_sidecar_for_gossip( }; let blob_proposer_index = blob_sidecar.message.proposer_index; - if proposer_index != blob_proposer_index { + if proposer_index != blob_proposer_index as usize { return Err(BlobError::ProposerIndexMismatch { - sidecar: blob_proposer_index, + sidecar: blob_proposer_index as usize, local: proposer_index, }); } @@ -248,7 +243,7 @@ pub fn validate_blob_sidecar_for_gossip( } pub fn verify_data_availability( - blob_sidecar: &BlobsSidecar, + blob_sidecar: &BlobSidecarList, kzg_commitments: &[KzgCommitment], transactions: &Transactions, _block_slot: Slot, @@ -306,36 +301,38 @@ impl IntoAvailableBlock for BlockWrapper { } } -#[derive(Clone, Debug, Derivative)] -#[derivative(PartialEq, Hash(bound = "T: BeaconChainTypes"))] +#[derive(Clone, Debug, PartialEq)] pub struct AvailableBlock { - block: Arc>, - blobs: Blobs, + pub block: Arc>, + pub blobs: Blobs, } impl AvailableBlock { - pub fn blobs(&self) -> Option>> { + pub fn blobs(&self) -> Option>> { match &self.blobs { Blobs::NotRequired | Blobs::None => None, - Blobs::Available(blob_sidecar) => Some(blob_sidecar.clone()), + Blobs::Available(blobs) => Some(blobs.clone()), } } - pub fn deconstruct(self) -> (Arc>, Option>>) { + pub fn deconstruct(self) -> (Arc>, Option>>) { match self.blobs { Blobs::NotRequired | Blobs::None => (self.block, None), - Blobs::Available(blob_sidecars) => (self.block, Some(blob_sidecars)), + Blobs::Available(blobs) => (self.block, Some(blobs)), } } } +#[derive(Clone, Debug, PartialEq)] pub enum Blobs { /// These blobs are available. - Available(VariableList>, E::MaxBlobsPerBlock>), - /// This block is from outside the data availability boundary or the block is from prior - /// to the eip4844 fork. + Available(Arc>), + /// This block is from outside the data availability boundary so doesn't require + /// a data availability check. NotRequired, - /// The block doesn't have any blob transactions. + /// The block's `kzg_commitments` field is empty so it does not contain any blobs. + EmptyBlobs, + /// This is a block prior to the 4844 fork, so doesn't require any blobs None, } @@ -351,59 +348,78 @@ pub trait AsBlock { fn canonical_root(&self) -> Hash256; } +#[derive(Debug, Clone)] +pub enum BlockWrapper { + /// This variant is fully available. + /// i.e. for pre-4844 blocks, it contains a (`SignedBeaconBlock`, `Blobs::None`) and for + /// post-4844 blocks, it contains a `SignedBeaconBlock` and a Blobs variant other than `Blobs::None`. + Available(AvailableBlock), + /// This variant is not fully available and requires blobs to become fully available. + AvailabilityPending(Arc>), +} + +impl BlockWrapper { + pub fn into_available_block(self) -> Option> { + match self { + BlockWrapper::AvailabilityPending(_) => None, + BlockWrapper::Available(block) => Some(block), + } + } +} + impl AsBlock for BlockWrapper { fn slot(&self) -> Slot { match self { - BlockWrapper::Block(block) => block.slot(), - BlockWrapper::BlockAndBlobs(block, _) => block.slot(), + BlockWrapper::Available(block) => block.block.slot(), + BlockWrapper::AvailabilityPending(block) => block.slot(), } } fn epoch(&self) -> Epoch { match self { - BlockWrapper::Block(block) => block.epoch(), - BlockWrapper::BlockAndBlobs(block, _) => block.epoch(), + BlockWrapper::Available(block) => block.block.epoch(), + BlockWrapper::AvailabilityPending(block) => block.epoch(), } } fn parent_root(&self) -> Hash256 { match self { - BlockWrapper::Block(block) => block.parent_root(), - BlockWrapper::BlockAndBlobs(block, _) => block.parent_root(), + BlockWrapper::Available(block) => block.block.parent_root(), + BlockWrapper::AvailabilityPending(block) => block.parent_root(), } } fn state_root(&self) -> Hash256 { match self { - BlockWrapper::Block(block) => block.state_root(), - BlockWrapper::BlockAndBlobs(block, _) => block.state_root(), + BlockWrapper::Available(block) => block.block.state_root(), + BlockWrapper::AvailabilityPending(block) => block.state_root(), } } fn signed_block_header(&self) -> SignedBeaconBlockHeader { match &self { - BlockWrapper::Block(block) => block.signed_block_header(), - BlockWrapper::BlockAndBlobs(block, _) => block.signed_block_header(), + BlockWrapper::Available(block) => block.block.signed_block_header(), + BlockWrapper::AvailabilityPending(block) => block.signed_block_header(), } } fn message(&self) -> BeaconBlockRef { match &self { - BlockWrapper::Block(block) => block.message(), - BlockWrapper::BlockAndBlobs(block, _) => block.message(), + BlockWrapper::Available(block) => block.block.message(), + BlockWrapper::AvailabilityPending(block) => block.message(), } } fn as_block(&self) -> &SignedBeaconBlock { match &self { - BlockWrapper::Block(block) => &block, - BlockWrapper::BlockAndBlobs(block, _) => &block, + BlockWrapper::Available(block) => &block.block, + BlockWrapper::AvailabilityPending(block) => &block, } } fn block_cloned(&self) -> Arc> { match &self { - BlockWrapper::Block(block) => block.clone(), - BlockWrapper::BlockAndBlobs(block, _) => block.clone(), + BlockWrapper::Available(block) => block.block.clone(), + BlockWrapper::AvailabilityPending(block) => block.clone(), } } fn canonical_root(&self) -> Hash256 { match &self { - BlockWrapper::Block(block) => block.canonical_root(), - BlockWrapper::BlockAndBlobs(block, _) => block.canonical_root(), + BlockWrapper::Available(block) => block.block.canonical_root(), + BlockWrapper::AvailabilityPending(block) => block.canonical_root(), } } } @@ -411,91 +427,74 @@ impl AsBlock for BlockWrapper { impl AsBlock for &BlockWrapper { fn slot(&self) -> Slot { match self { - BlockWrapper::Block(block) => block.slot(), - BlockWrapper::BlockAndBlobs(block, _) => block.slot(), + BlockWrapper::Available(block) => block.block.slot(), + BlockWrapper::AvailabilityPending(block) => block.slot(), } } fn epoch(&self) -> Epoch { match self { - BlockWrapper::Block(block) => block.epoch(), - BlockWrapper::BlockAndBlobs(block, _) => block.epoch(), + BlockWrapper::Available(block) => block.block.epoch(), + BlockWrapper::AvailabilityPending(block) => block.epoch(), } } fn parent_root(&self) -> Hash256 { match self { - BlockWrapper::Block(block) => block.parent_root(), - BlockWrapper::BlockAndBlobs(block, _) => block.parent_root(), + BlockWrapper::Available(block) => block.block.parent_root(), + BlockWrapper::AvailabilityPending(block) => block.parent_root(), } } fn state_root(&self) -> Hash256 { match self { - BlockWrapper::Block(block) => block.state_root(), - BlockWrapper::BlockAndBlobs(block, _) => block.state_root(), + BlockWrapper::Available(block) => block.block.state_root(), + BlockWrapper::AvailabilityPending(block) => block.state_root(), } } fn signed_block_header(&self) -> SignedBeaconBlockHeader { match &self { - BlockWrapper::Block(block) => block.signed_block_header(), - BlockWrapper::BlockAndBlobs(block, _) => block.signed_block_header(), + BlockWrapper::Available(block) => block.block.signed_block_header(), + BlockWrapper::AvailabilityPending(block) => block.signed_block_header(), } } fn message(&self) -> BeaconBlockRef { match &self { - BlockWrapper::Block(block) => block.message(), - BlockWrapper::BlockAndBlobs(block, _) => block.message(), + BlockWrapper::Available(block) => block.block.message(), + BlockWrapper::AvailabilityPending(block) => block.message(), } } fn as_block(&self) -> &SignedBeaconBlock { match &self { - BlockWrapper::Block(block) => &block, - BlockWrapper::BlockAndBlobs(block, _) => &block, + BlockWrapper::Available(block) => &block.block, + BlockWrapper::AvailabilityPending(block) => &block, } } fn block_cloned(&self) -> Arc> { match &self { - BlockWrapper::Block(block) => block.clone(), - BlockWrapper::BlockAndBlobs(block, _) => block.clone(), + BlockWrapper::Available(block) => block.block.clone(), + BlockWrapper::AvailabilityPending(block) => block.clone(), } } fn canonical_root(&self) -> Hash256 { match &self { - BlockWrapper::Block(block) => block.canonical_root(), - BlockWrapper::BlockAndBlobs(block, _) => block.canonical_root(), - } - } -} - -/// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobsSidecar`]. This makes no -/// claims about data availability and should not be used in consensus. This struct is useful in -/// networking when we want to send blocks around without consensus checks. -#[derive(Clone, Debug, Derivative)] -#[derivative(PartialEq, Hash(bound = "E: EthSpec"))] -pub enum BlockWrapper { - Block(Arc>), - BlockAndBlobs(Arc>, Arc>), -} - -impl BlockWrapper { - pub fn new( - block: Arc>, - blobs_sidecar: Option>>, - ) -> Self { - if let Some(blobs_sidecar) = blobs_sidecar { - BlockWrapper::BlockAndBlobs(block, blobs_sidecar) - } else { - BlockWrapper::Block(block) + BlockWrapper::Available(block) => block.block.canonical_root(), + BlockWrapper::AvailabilityPending(block) => block.canonical_root(), } } } impl From> for BlockWrapper { fn from(block: SignedBeaconBlock) -> Self { - BlockWrapper::Block(Arc::new(block)) + BlockWrapper::AvailabilityPending(Arc::new(block)) } } impl From>> for BlockWrapper { fn from(block: Arc>) -> Self { - BlockWrapper::Block(block) + BlockWrapper::AvailabilityPending(block) + } +} + +impl From> for BlockWrapper { + fn from(block: AvailableBlock) -> Self { + BlockWrapper::Available(block) } } diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index a9e4727ada..544f484663 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -48,9 +48,7 @@ // returned alongside. #![allow(clippy::result_large_err)] -use crate::blob_verification::{ - AsBlock, AvailableBlock, BlobError, BlockWrapper, IntoAvailableBlock, IntoBlockWrapper, -}; +use crate::blob_verification::{AsBlock, AvailableBlock, BlobError, BlockWrapper}; use crate::eth1_finalization_cache::Eth1FinalizationData; use crate::execution_payload::{ is_optimistic_candidate_block, validate_execution_payload_for_gossip, validate_merge_block, @@ -66,7 +64,6 @@ use crate::{ }, metrics, BeaconChain, BeaconChainError, BeaconChainTypes, }; -use derivative::Derivative; use eth2::types::EventKind; use execution_layer::PayloadStatus; use fork_choice::{AttestationFromBlock, PayloadVerificationStatus}; @@ -595,13 +592,12 @@ pub fn signature_verify_chain_segment( signature_verifier.include_all_signatures(block.as_block(), &mut consensus_context)?; //FIXME(sean) batch kzg verification - let available_block = block.clone().into_available_block(*block_root, chain)?; consensus_context = consensus_context.set_kzg_commitments_consistent(true); // Save the block and its consensus context. The context will have had its proposer index // and attesting indices filled in, which can be used to accelerate later block processing. signature_verified_blocks.push(SignatureVerifiedBlock { - block: available_block, + block: block.clone(), block_root: *block_root, parent: None, consensus_context, @@ -623,10 +619,9 @@ pub fn signature_verify_chain_segment( /// A wrapper around a `SignedBeaconBlock` that indicates it has been approved for re-gossiping on /// the p2p network. -#[derive(Derivative)] -#[derivative(Debug(bound = "T: BeaconChainTypes"))] +#[derive(Debug)] pub struct GossipVerifiedBlock { - pub block: Arc>, + pub block: BlockWrapper, pub block_root: Hash256, parent: Option>, consensus_context: ConsensusContext, @@ -635,7 +630,7 @@ pub struct GossipVerifiedBlock { /// A wrapper around a `SignedBeaconBlock` that indicates that all signatures (except the deposit /// signatures) have been verified. pub struct SignatureVerifiedBlock { - block: Arc>, + block: BlockWrapper, block_root: Hash256, parent: Option>, consensus_context: ConsensusContext, @@ -658,7 +653,7 @@ type PayloadVerificationHandle = /// due to finality or some other event. A `ExecutionPendingBlock` should be imported into the /// `BeaconChain` immediately after it is instantiated. pub struct ExecutionPendingBlock { - pub block: Arc>, + pub block: BlockWrapper, pub block_root: Hash256, pub state: BeaconState, pub parent_block: SignedBeaconBlock>, @@ -669,7 +664,7 @@ pub struct ExecutionPendingBlock { } pub struct ExecutedBlock { - pub block: Arc>, + pub block: BlockWrapper, pub block_root: Hash256, pub state: BeaconState, pub parent_block: SignedBeaconBlock>, @@ -930,13 +925,13 @@ impl GossipVerifiedBlock { validate_execution_payload_for_gossip(&parent_block, block.message(), chain)?; // Having checked the proposer index and the block root we can cache them. - let consensus_context = ConsensusContext::new(available_block.slot()) + let consensus_context = ConsensusContext::new(block.slot()) .set_current_block_root(block_root) .set_proposer_index(block.as_block().message().proposer_index()) .set_kzg_commitments_consistent(true); Ok(Self { - block: available_block, + block: block, block_root, parent, consensus_context, @@ -966,7 +961,7 @@ impl IntoExecutionPendingBlock for GossipVerifiedBlock &SignedBeaconBlock { - self.block.as_ref() + self.block.as_block() } } @@ -980,6 +975,7 @@ impl SignatureVerifiedBlock { block_root: Hash256, chain: &BeaconChain, ) -> Result> { + let block = BlockWrapper::from(block); // Ensure the block is the correct structure for the fork at `block.slot()`. block .as_block() @@ -1118,7 +1114,7 @@ impl IntoExecutionPendingBlock for SignatureVerifiedBloc } fn block(&self) -> &SignedBeaconBlock { - self.block.as_ref() + self.block.as_block() } } @@ -1136,11 +1132,8 @@ impl IntoExecutionPendingBlock for Arc IntoExecutionPendingBlock for Arc IntoExecutionPendingBlock for BlockWrapper { + fn into_execution_pending_block_slashable( + self, + block_root: Hash256, + chain: &Arc>, + notify_execution_layer: NotifyExecutionLayer, + ) -> Result< + ExecutionPendingBlock, + BlockSlashInfo::EthSpec>>, + > { + match self { + BlockWrapper::AvailabilityPending(block) => block + .into_execution_pending_block_slashable(block_root, chain, notify_execution_layer), + BlockWrapper::Available(AvailableBlock { block, blobs }) => { + let execution_pending_block = block.into_execution_pending_block_slashable( + block_root, + chain, + notify_execution_layer, + )?; + let block = execution_pending_block.block.block_cloned(); + let available_execution_pending_block = + BlockWrapper::Available(AvailableBlock { block, blobs }); + std::mem::replace( + &mut execution_pending_block.block, + available_execution_pending_block, + ); + Ok(execution_pending_block) + } + } + } + + fn block(&self) -> &SignedBeaconBlock<::EthSpec> { + self.as_block() + } +} + impl ExecutionPendingBlock { /// Instantiates `Self`, a wrapper that indicates that the given `block` is fully valid. See /// the struct-level documentation for more information. @@ -1158,7 +1187,7 @@ impl ExecutionPendingBlock { /// /// Returns an error if the block is invalid, or if the block was unable to be verified. pub fn from_signature_verified_components( - block: Arc>, + block: BlockWrapper, block_root: Hash256, parent: PreProcessingSnapshot, mut consensus_context: ConsensusContext, @@ -1188,7 +1217,7 @@ impl ExecutionPendingBlock { // because it will revert finalization. Note that the finalized block is stored in fork // choice, so we will not reject any child of the finalized block (this is relevant during // genesis). - return Err(BlockError::ParentUnknown(block.into_block_wrapper())); + return Err(BlockError::ParentUnknown(block)); } // Reject any block that exceeds our limit on skipped slots. @@ -1623,14 +1652,11 @@ fn check_block_against_finalized_slot( /// ## Warning /// /// Taking a lock on the `chain.canonical_head.fork_choice` might cause a deadlock here. -pub fn check_block_is_finalized_checkpoint_or_descendant< - T: BeaconChainTypes, - B: IntoBlockWrapper, ->( +pub fn check_block_is_finalized_checkpoint_or_descendant( chain: &BeaconChain, fork_choice: &BeaconForkChoice, - block: B, -) -> Result> { + block: BlockWrapper, +) -> Result, BlockError> { if fork_choice.is_finalized_checkpoint_or_descendant(block.parent_root()) { Ok(block) } else { @@ -1651,7 +1677,7 @@ pub fn check_block_is_finalized_checkpoint_or_descendant< block_parent_root: block.parent_root(), }) } else { - Err(BlockError::ParentUnknown(block.into_block_wrapper())) + Err(BlockError::ParentUnknown(block)) } } } @@ -1741,11 +1767,11 @@ fn verify_parent_block_is_known( /// Returns `Err(BlockError::ParentUnknown)` if the parent is not found, or if an error occurs /// whilst attempting the operation. #[allow(clippy::type_complexity)] -fn load_parent>( +fn load_parent( block_root: Hash256, - block: B, + block: BlockWrapper, chain: &BeaconChain, -) -> Result<(PreProcessingSnapshot, B), BlockError> { +) -> Result<(PreProcessingSnapshot, BlockWrapper), BlockError> { let spec = &chain.spec; // Reject any block if its parent is not known to fork choice. @@ -1763,7 +1789,7 @@ fn load_parent>( .fork_choice_read_lock() .contains_block(&block.parent_root()) { - return Err(BlockError::ParentUnknown(block.into_block_wrapper())); + return Err(BlockError::ParentUnknown(block)); } let block_delay = chain diff --git a/consensus/types/src/signed_blob.rs b/consensus/types/src/signed_blob.rs index 4121b8b7f2..c295f6c6c1 100644 --- a/consensus/types/src/signed_blob.rs +++ b/consensus/types/src/signed_blob.rs @@ -1,4 +1,7 @@ -use crate::{test_utils::TestRandom, BlobSidecar, EthSpec, Signature}; +use crate::{ + test_utils::TestRandom, BlobSidecar, ChainSpec, EthSpec, Fork, Hash256, PublicKey, Signature, + SignedRoot, +}; use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; @@ -25,3 +28,19 @@ pub struct SignedBlobSidecar { pub message: BlobSidecar, pub signature: Signature, } + +impl SignedRoot for SignedBlobSidecar {} + +impl SignedBlobSidecar { + pub fn verify_signature( + &self, + _object_root_opt: Option, + _pubkey: &PublicKey, + _fork: &Fork, + _genesis_validators_root: Hash256, + _spec: &ChainSpec, + ) -> bool { + // TODO (pawan): fill up logic + unimplemented!() + } +} From 3c18e1a3a4c0a4895dcd08e79e34b6c59420edd9 Mon Sep 17 00:00:00 2001 From: Divma <26765164+divagant-martian@users.noreply.github.com> Date: Thu, 16 Mar 2023 19:20:39 -0500 Subject: [PATCH 09/16] thread blocks and blobs to sync (#4100) * thread blocks and blobs to sync * satisfy dead code analysis --- beacon_node/network/src/router/processor.rs | 4 +- beacon_node/network/src/sync/manager.rs | 126 ++++++++++-------- .../network/src/sync/network_context.rs | 24 ++-- 3 files changed, 83 insertions(+), 71 deletions(-) diff --git a/beacon_node/network/src/router/processor.rs b/beacon_node/network/src/router/processor.rs index 56a5f24586..76962b373f 100644 --- a/beacon_node/network/src/router/processor.rs +++ b/beacon_node/network/src/router/processor.rs @@ -258,7 +258,7 @@ impl Processor { ); if let RequestId::Sync(id) = request_id { - self.send_to_sync(SyncMessage::RpcBlobs { + self.send_to_sync(SyncMessage::RpcBlob { peer_id, request_id: id, blob_sidecar, @@ -330,7 +330,7 @@ impl Processor { "Received BlobsByRoot Response"; "peer" => %peer_id, ); - self.send_to_sync(SyncMessage::RpcBlobs { + self.send_to_sync(SyncMessage::RpcBlob { request_id, peer_id, blob_sidecar, diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 768b95273e..43921b585a 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -35,7 +35,7 @@ use super::backfill_sync::{BackFillSync, ProcessResult, SyncStart}; use super::block_lookups::BlockLookups; -use super::network_context::{BlockOrBlobs, SyncNetworkContext}; +use super::network_context::{BlockOrBlob, SyncNetworkContext}; use super::peer_sync_info::{remote_sync_type, PeerSyncType}; use super::range_sync::{RangeSync, RangeSyncType, EPOCHS_PER_BATCH}; use crate::beacon_processor::{ChainSegmentProcessId, WorkEvent as BeaconWorkEvent}; @@ -86,6 +86,10 @@ pub enum RequestId { RangeBlobs { id: Id }, } +// TODO(diva) I'm updating functions what at a time, but this should be revisited because I think +// some code paths that are split for blobs and blocks can be made just one after sync as a whole +// is updated. + #[derive(Debug)] /// A message that can be sent to the sync manager thread. pub enum SyncMessage { @@ -101,7 +105,7 @@ pub enum SyncMessage { }, /// A blob has been received from the RPC. - RpcBlobs { + RpcBlob { request_id: RequestId, peer_id: PeerId, blob_sidecar: Option>>, @@ -554,7 +558,12 @@ impl SyncManager { beacon_block, seen_timestamp, } => { - self.rpc_block_received(request_id, peer_id, beacon_block, seen_timestamp); + self.rpc_block_or_blob_received( + request_id, + peer_id, + beacon_block.into(), + seen_timestamp, + ); } SyncMessage::UnknownBlock(peer_id, block, block_root) => { // If we are not synced or within SLOT_IMPORT_TOLERANCE of the block, ignore @@ -638,12 +647,17 @@ impl SyncManager { .block_lookups .parent_chain_processed(chain_hash, result, &mut self.network), }, - SyncMessage::RpcBlobs { + SyncMessage::RpcBlob { request_id, peer_id, blob_sidecar, seen_timestamp, - } => self.rpc_blobs_received(request_id, peer_id, blob_sidecar, seen_timestamp), + } => self.rpc_block_or_blob_received( + request_id, + peer_id, + blob_sidecar.into(), + seen_timestamp, + ), } } @@ -702,30 +716,50 @@ impl SyncManager { } } - fn rpc_block_received( + fn rpc_block_or_blob_received( &mut self, request_id: RequestId, peer_id: PeerId, - beacon_block: Option>>, + block_or_blob: BlockOrBlob, seen_timestamp: Duration, ) { match request_id { - RequestId::SingleBlock { id } => self.block_lookups.single_block_lookup_response( - id, - peer_id, - beacon_block.map(|block| block.into()), - seen_timestamp, - &mut self.network, - ), - RequestId::ParentLookup { id } => self.block_lookups.parent_lookup_response( - id, - peer_id, - beacon_block.map(|block| block.into()), - seen_timestamp, - &mut self.network, - ), + RequestId::SingleBlock { id } => { + // TODO(diva) adjust when dealing with by root requests. This code is here to + // satisfy dead code analysis + match block_or_blob { + BlockOrBlob::Block(maybe_block) => { + self.block_lookups.single_block_lookup_response( + id, + peer_id, + maybe_block.map(BlockWrapper::Block), + seen_timestamp, + &mut self.network, + ) + } + BlockOrBlob::Sidecar(_) => unimplemented!("Mimatch between BlockWrapper and what the network receives needs to be handled first."), + } + } + RequestId::ParentLookup { id } => { + // TODO(diva) adjust when dealing with by root requests. This code is here to + // satisfy dead code analysis + match block_or_blob { + BlockOrBlob::Block(maybe_block) => self.block_lookups.parent_lookup_response( + id, + peer_id, + maybe_block.map(BlockWrapper::Block), + seen_timestamp, + &mut self.network, + ), + BlockOrBlob::Sidecar(_) => unimplemented!("Mimatch between BlockWrapper and what the network receives needs to be handled first."), + } + } RequestId::BackFillBlocks { id } => { - let is_stream_terminator = beacon_block.is_none(); + let maybe_block = match block_or_blob { + BlockOrBlob::Block(maybe_block) => maybe_block, + BlockOrBlob::Sidecar(_) => todo!("I think this is unreachable"), + }; + let is_stream_terminator = maybe_block.is_none(); if let Some(batch_id) = self .network .backfill_sync_only_blocks_response(id, is_stream_terminator) @@ -735,7 +769,7 @@ impl SyncManager { batch_id, &peer_id, id, - beacon_block.map(|block| block.into()), + maybe_block.map(|block| block.into()), ) { Ok(ProcessResult::SyncCompleted) => self.update_sync_state(), Ok(ProcessResult::Successful) => {} @@ -748,7 +782,11 @@ impl SyncManager { } } RequestId::RangeBlocks { id } => { - let is_stream_terminator = beacon_block.is_none(); + let maybe_block = match block_or_blob { + BlockOrBlob::Block(maybe_block) => maybe_block, + BlockOrBlob::Sidecar(_) => todo!("I think this should be unreachable, since this is a range only-blocks request, and the network should not accept this chunk at all. Needs better handling"), + }; + let is_stream_terminator = maybe_block.is_none(); if let Some((chain_id, batch_id)) = self .network .range_sync_block_response(id, is_stream_terminator) @@ -759,28 +797,28 @@ impl SyncManager { chain_id, batch_id, id, - beacon_block.map(|block| block.into()), + maybe_block.map(|block| block.into()), ); self.update_sync_state(); } } RequestId::BackFillBlobs { id } => { - self.blobs_backfill_response(id, peer_id, beacon_block.into()) + self.backfill_block_and_blobs_response(id, peer_id, block_or_blob) } RequestId::RangeBlobs { id } => { - self.blobs_range_response(id, peer_id, beacon_block.into()) + self.range_block_and_blobs_response(id, peer_id, block_or_blob) } } } /// Handles receiving a response for a range sync request that should have both blocks and /// blobs. - fn blobs_range_response( + fn range_block_and_blobs_response( &mut self, id: Id, peer_id: PeerId, - block_or_blob: BlockOrBlobs, + block_or_blob: BlockOrBlob, ) { if let Some((chain_id, resp)) = self .network @@ -822,11 +860,11 @@ impl SyncManager { /// Handles receiving a response for a Backfill sync request that should have both blocks and /// blobs. - fn blobs_backfill_response( + fn backfill_block_and_blobs_response( &mut self, id: Id, peer_id: PeerId, - block_or_blob: BlockOrBlobs, + block_or_blob: BlockOrBlob, ) { if let Some(resp) = self .network @@ -871,32 +909,6 @@ impl SyncManager { } } } - - fn rpc_blobs_received( - &mut self, - request_id: RequestId, - _peer_id: PeerId, - _maybe_blob: Option::EthSpec>>>, - _seen_timestamp: Duration, - ) { - match request_id { - RequestId::SingleBlock { .. } | RequestId::ParentLookup { .. } => { - unreachable!("There is no such thing as a singular 'by root' glob request that is not accompanied by the block") - } - RequestId::BackFillBlocks { .. } => { - unreachable!("An only blocks request does not receive sidecars") - } - RequestId::BackFillBlobs { .. } => { - unimplemented!("Adjust backfill sync"); - } - RequestId::RangeBlocks { .. } => { - unreachable!("Only-blocks range requests don't receive sidecars") - } - RequestId::RangeBlobs { id: _ } => { - unimplemented!("Adjust range"); - } - } - } } impl From>> for BlockProcessResult { diff --git a/beacon_node/network/src/sync/network_context.rs b/beacon_node/network/src/sync/network_context.rs index 10f7f32955..974d8dbd8c 100644 --- a/beacon_node/network/src/sync/network_context.rs +++ b/beacon_node/network/src/sync/network_context.rs @@ -75,20 +75,20 @@ pub struct SyncNetworkContext { } /// Small enumeration to make dealing with block and blob requests easier. -pub enum BlockOrBlobs { +pub enum BlockOrBlob { Block(Option>>), - Blobs(Option>>), + Sidecar(Option>>), } -impl From>>> for BlockOrBlobs { +impl From>>> for BlockOrBlob { fn from(block: Option>>) -> Self { - BlockOrBlobs::Block(block) + BlockOrBlob::Block(block) } } -impl From>>> for BlockOrBlobs { +impl From>>> for BlockOrBlob { fn from(blob: Option>>) -> Self { - BlockOrBlobs::Blobs(blob) + BlockOrBlob::Sidecar(blob) } } @@ -311,15 +311,15 @@ impl SyncNetworkContext { pub fn range_sync_block_and_blob_response( &mut self, request_id: Id, - block_or_blob: BlockOrBlobs, + block_or_blob: BlockOrBlob, ) -> Option<(ChainId, BlocksAndBlobsByRangeResponse)> { match self.range_blocks_and_blobs_requests.entry(request_id) { Entry::Occupied(mut entry) => { let req = entry.get_mut(); let info = &mut req.block_blob_info; match block_or_blob { - BlockOrBlobs::Block(maybe_block) => info.add_block_response(maybe_block), - BlockOrBlobs::Blobs(maybe_sidecar) => info.add_sidecar_response(maybe_sidecar), + BlockOrBlob::Block(maybe_block) => info.add_block_response(maybe_block), + BlockOrBlob::Sidecar(maybe_sidecar) => info.add_sidecar_response(maybe_sidecar), } if info.is_finished() { // If the request is finished, dequeue everything @@ -402,14 +402,14 @@ impl SyncNetworkContext { pub fn backfill_sync_block_and_blob_response( &mut self, request_id: Id, - block_or_blob: BlockOrBlobs, + block_or_blob: BlockOrBlob, ) -> Option> { match self.backfill_blocks_and_blobs_requests.entry(request_id) { Entry::Occupied(mut entry) => { let (_, info) = entry.get_mut(); match block_or_blob { - BlockOrBlobs::Block(maybe_block) => info.add_block_response(maybe_block), - BlockOrBlobs::Blobs(maybe_sidecar) => info.add_sidecar_response(maybe_sidecar), + BlockOrBlob::Block(maybe_block) => info.add_block_response(maybe_block), + BlockOrBlob::Sidecar(maybe_sidecar) => info.add_sidecar_response(maybe_sidecar), } if info.is_finished() { // If the request is finished, dequeue everything From be4d70eeffc3e7e4542480445a10480558836d34 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Thu, 16 Mar 2023 22:25:04 -0400 Subject: [PATCH 10/16] tons of changes, just tryna compile --- beacon_node/beacon_chain/src/beacon_chain.rs | 170 ++++++++---------- .../beacon_chain/src/blob_verification.rs | 28 +-- .../beacon_chain/src/block_verification.rs | 19 +- beacon_node/beacon_chain/src/builder.rs | 28 ++- .../beacon_chain/src/execution_payload.rs | 4 +- .../beacon_chain/src/gossip_blob_cache.rs | 77 +++++--- beacon_node/beacon_chain/src/lib.rs | 2 +- beacon_node/client/src/builder.rs | 9 +- beacon_node/http_api/src/block_id.rs | 8 +- .../http_api/src/build_block_contents.rs | 2 +- beacon_node/http_api/src/publish_blocks.rs | 28 +-- .../beacon_processor/worker/gossip_methods.rs | 10 +- .../beacon_processor/worker/rpc_methods.rs | 118 ++++++------ .../beacon_processor/worker/sync_methods.rs | 29 +-- .../state_processing/src/consensus_context.rs | 2 +- lcli/src/parse_ssz.rs | 2 +- testing/ef_tests/src/type_name.rs | 6 +- 17 files changed, 292 insertions(+), 250 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 482de67043..5039724649 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -8,7 +8,7 @@ use crate::beacon_proposer_cache::compute_proposer_duties_from_head; use crate::beacon_proposer_cache::BeaconProposerCache; use crate::blob_cache::BlobCache; use crate::blob_verification::{ - AsBlock, AvailableBlock, BlobError, Blobs, BlockWrapper, IntoAvailableBlock, + AsBlock, AvailableBlock, BlobError, BlockWrapper, IntoAvailableBlock, VerifiedBlobs, }; use crate::block_times_cache::BlockTimesCache; use crate::block_verification::{ @@ -25,6 +25,7 @@ use crate::eth1_finalization_cache::{Eth1FinalizationCache, Eth1FinalizationData use crate::events::ServerSentEventHandler; use crate::execution_payload::{get_execution_payload, NotifyExecutionLayer, PreparePayloadHandle}; use crate::fork_choice_signal::{ForkChoiceSignalRx, ForkChoiceSignalTx, ForkChoiceWaitResult}; +use crate::gossip_blob_cache::DataAvailabilityChecker; use crate::head_tracker::HeadTracker; use crate::historical_blocks::HistoricalBlockError; use crate::kzg_utils; @@ -76,6 +77,7 @@ use futures::channel::mpsc::Sender; use itertools::process_results; use itertools::Itertools; use kzg::Kzg; +use oneshot_broadcast::Receiver; use operation_pool::{AttestationRef, OperationPool, PersistedOperationPool, ReceivedPreCapella}; use parking_lot::{Mutex, RwLock}; use proto_array::{CountUnrealizedFull, DoNotReOrg, ProposerHeadError}; @@ -112,7 +114,9 @@ use store::{ use task_executor::{ShutdownReason, TaskExecutor}; use tokio::task::JoinHandle; use tree_hash::TreeHash; +use types::beacon_block_body::KzgCommitments; use types::beacon_state::CloneConfig; +use types::blob_sidecar::Blobs; use types::consts::eip4844::MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS; use types::consts::merge::INTERVALS_PER_SLOT; use types::*; @@ -276,7 +280,7 @@ pub enum StateSkipConfig { pub enum BlockProcessingResult { Verified(Hash256), - AvailabilityPending(ExecutedBlock), + AvailabilityPending(ExecutedBlock), } pub trait BeaconChainTypes: Send + Sync + 'static { @@ -440,7 +444,8 @@ pub struct BeaconChain { pub slasher: Option>>, /// Provides monitoring of a set of explicitly defined validators. pub validator_monitor: RwLock>, - pub blob_cache: BlobCache, + pub proposal_blob_cache: BlobCache, + pub data_availability_checker: Option>, pub kzg: Option>, } @@ -971,35 +976,9 @@ impl BeaconChain { pub async fn get_block_and_blobs_checking_early_attester_cache( &self, block_root: &Hash256, - ) -> Result>, Error> { - // If there is no data availability boundary, the Eip4844 fork is disabled. - if let Some(finalized_data_availability_boundary) = - self.finalized_data_availability_boundary() - { - // Only use the attester cache if we can find both the block and blob - if let (Some(block), Some(blobs)) = ( - self.early_attester_cache.get_block(*block_root), - self.early_attester_cache.get_blobs(*block_root), - ) { - Ok(Some(SignedBeaconBlockAndBlobsSidecar { - beacon_block: block, - blobs_sidecar: blobs, - })) - // Attempt to get the block and blobs from the database - } else if let Some(block) = self.get_block(block_root).await?.map(Arc::new) { - let blobs = self - .get_blobs(block_root, finalized_data_availability_boundary)? - .map(Arc::new); - Ok(blobs.map(|blobs| SignedBeaconBlockAndBlobsSidecar { - beacon_block: block, - blobs_sidecar: blobs, - })) - } else { - Ok(None) - } - } else { - Ok(None) - } + ) -> Result, Error> { + //TODO(sean) use the rpc blobs cache and revert this to the current block cache logic + Ok(Some(())) } /// Returns the block at the given root, if any. @@ -1083,7 +1062,7 @@ impl BeaconChain { &self, block_root: &Hash256, ) -> Result>, Error> { - self.store.get_blobs(block_root) + Ok(self.store.get_blobs(block_root)?) } pub fn get_blinded_block( @@ -2674,7 +2653,7 @@ impl BeaconChain { unverified_block: B, count_unrealized: CountUnrealized, notify_execution_layer: NotifyExecutionLayer, - ) -> Result, BlockError> { + ) -> Result> { // Start the Prometheus timer. let _full_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_TIMES); @@ -2692,19 +2671,21 @@ impl BeaconChain { // TODO(log required errors) let executed_block = self + .clone() .into_executed_block(execution_pending, count_unrealized) .await?; // Check if the executed block has all it's blobs available to qualify as a fully // available block - let import_block = if let Ok(blobs) = self - .gossip_blob_cache - .lock() - .blobs(executed_block.block_root) - { - self.import_available_block(executed_block, blobs, count_unrealized) + let import_block = if let Some(da_checker) = self.data_availability_checker.as_ref() { + da_checker.put_block(executed_block); //TODO(sean) errors + return Err(BlockError::AvailabilityPending(block_root)); } else { - return Ok(BlockProcessingResult::AvailabilityPending(executed_block)); + self.clone().import_available_block( + executed_block, + VerifiedBlobs::PreEip4844, + count_unrealized, + ) }; // Verify and import the block. @@ -2721,7 +2702,7 @@ impl BeaconChain { // Increment the Prometheus counter for block processing successes. metrics::inc_counter(&metrics::BLOCK_PROCESSING_SUCCESSES); - Ok(BlockProcessingResult::Verified(block_root)) + Ok(block_root) } Err(e @ BlockError::BeaconChainError(BeaconChainError::TokioJoin(_))) => { debug!( @@ -2761,7 +2742,7 @@ impl BeaconChain { self: Arc, execution_pending_block: ExecutionPendingBlock, count_unrealized: CountUnrealized, - ) -> Result, BlockError> { + ) -> Result, BlockError> { let ExecutionPendingBlock { block, block_root, @@ -2826,8 +2807,8 @@ impl BeaconChain { /// (i.e., this function is not atomic). async fn import_available_block( self: Arc, - executed_block: ExecutedBlock, - blobs: Blobs, + executed_block: ExecutedBlock, + blobs: VerifiedBlobs, count_unrealized: CountUnrealized, ) -> Result> { let ExecutedBlock { @@ -4802,12 +4783,11 @@ impl BeaconChain { .as_ref() .ok_or(BlockProductionError::TrustedSetupNotInitialized)?; let beacon_block_root = block.canonical_root(); - let expected_kzg_commitments: &KzgCommitments = - block.body().blob_kzg_commitments().map_err(|_| { - BlockProductionError::InvalidBlockVariant( - "EIP4844 block does not contain kzg commitments".to_string(), - ) - })?; + let expected_kzg_commitments = block.body().blob_kzg_commitments().map_err(|_| { + BlockProductionError::InvalidBlockVariant( + "EIP4844 block does not contain kzg commitments".to_string(), + ) + })?; if expected_kzg_commitments.len() != blobs.len() { return Err(BlockProductionError::MissingKzgCommitment(format!( @@ -4856,7 +4836,8 @@ impl BeaconChain { .collect::>, BlockProductionError>>()?, ); - self.blob_cache.put(beacon_block_root, blob_sidecars); + self.proposal_blob_cache + .put(beacon_block_root, blob_sidecars); } metrics::inc_counter(&metrics::BLOCK_PRODUCTION_SUCCESSES); @@ -4874,8 +4855,8 @@ impl BeaconChain { fn compute_blob_kzg_proofs( kzg: &Arc, - blobs: &Blobs<::EthSpec>, - expected_kzg_commitments: &KzgCommitments<::EthSpec>, + blobs: &Blobs, + expected_kzg_commitments: &KzgCommitments, slot: Slot, ) -> Result, BlockProductionError> { blobs @@ -6174,47 +6155,50 @@ impl BeaconChain { .unwrap_or(false)) } - pub async fn check_data_availability( - &self, - block: Arc>, - ) -> Result, Error> { - let kzg_commitments = block - .message() - .body() - .blob_kzg_commitments() - .map_err(|_| BlobError::KzgCommitmentMissing)?; - let transactions = block - .message() - .body() - .execution_payload_eip4844() - .map(|payload| payload.transactions()) - .map_err(|_| BlobError::TransactionsMissing)? - .ok_or(BlobError::TransactionsMissing)?; + pub fn start_block_importer( + self: &Arc, + mut rx: tokio::sync::mpsc::Receiver>, + ) { + let chain = self.clone(); + self.task_executor.spawn( + async move { + while let Some(block) = rx.recv().await { + let ExecutedBlock { + block, + block_root, + state, + parent_block, + parent_eth1_finalization_data, + confirmed_state_roots, + consensus_context, + payload_verification_outcome, + } = block; - if verify_kzg_commitments_against_transactions::(transactions, kzg_commitments) - .is_err() - { - return Err(BlobError::TransactionCommitmentMismatch); - } + let available_block = block.into_available_block().unwrap(); //TODO(sean) remove unwrap - // Validate that the kzg proof is valid against the commitments and blobs - let kzg = self - .kzg - .as_ref() - .ok_or(BlobError::TrustedSetupNotInitialized)?; - - if !kzg_utils::validate_blobs_sidecar( - kzg, - block_slot, - block_root, - kzg_commitments, - blob_sidecar, - ) - .map_err(BlobError::KzgError)? - { - return Err(BlobError::InvalidKzgProof); - } - Ok(()) + let chain_inner = chain.clone(); + let block_hash = chain + .spawn_blocking_handle( + move || { + chain_inner.import_block( + available_block, + block_root, + state, + confirmed_state_roots, + payload_verification_outcome.payload_verification_status, + CountUnrealized::True, //TODO(sean) + parent_block, + parent_eth1_finalization_data, + consensus_context, + ) + }, + "block_importer", + ) + .await; + } + }, + "block_importer_listener", + ); } } diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 35fb4c1429..013fae1fd5 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -1,3 +1,4 @@ +use derivative::Derivative; use slot_clock::SlotClock; use std::sync::Arc; @@ -301,30 +302,36 @@ impl IntoAvailableBlock for BlockWrapper { } } -#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq, Derivative)] +#[derivative(Hash(bound = "T: EthSpec"))] pub struct AvailableBlock { pub block: Arc>, - pub blobs: Blobs, + pub blobs: VerifiedBlobs, } impl AvailableBlock { pub fn blobs(&self) -> Option>> { match &self.blobs { - Blobs::NotRequired | Blobs::None => None, - Blobs::Available(blobs) => Some(blobs.clone()), + VerifiedBlobs::EmptyBlobs | VerifiedBlobs::NotRequired | VerifiedBlobs::PreEip4844 => { + None + } + VerifiedBlobs::Available(blobs) => Some(blobs.clone()), } } pub fn deconstruct(self) -> (Arc>, Option>>) { match self.blobs { - Blobs::NotRequired | Blobs::None => (self.block, None), - Blobs::Available(blobs) => (self.block, Some(blobs)), + VerifiedBlobs::EmptyBlobs | VerifiedBlobs::NotRequired | VerifiedBlobs::PreEip4844 => { + (self.block, None) + } + VerifiedBlobs::Available(blobs) => (self.block, Some(blobs)), } } } -#[derive(Clone, Debug, PartialEq)] -pub enum Blobs { +#[derive(Clone, Debug, PartialEq, Derivative)] +#[derivative(Hash(bound = "E: EthSpec"))] +pub enum VerifiedBlobs { /// These blobs are available. Available(Arc>), /// This block is from outside the data availability boundary so doesn't require @@ -333,7 +340,7 @@ pub enum Blobs { /// The block's `kzg_commitments` field is empty so it does not contain any blobs. EmptyBlobs, /// This is a block prior to the 4844 fork, so doesn't require any blobs - None, + PreEip4844, } pub trait AsBlock { @@ -348,7 +355,8 @@ pub trait AsBlock { fn canonical_root(&self) -> Hash256; } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Derivative)] +#[derivative(Hash(bound = "E: EthSpec"))] pub enum BlockWrapper { /// This variant is fully available. /// i.e. for pre-4844 blocks, it contains a (`SignedBeaconBlock`, `Blobs::None`) and for diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 544f484663..4ee15663f1 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -64,6 +64,7 @@ use crate::{ }, metrics, BeaconChain, BeaconChainError, BeaconChainTypes, }; +use derivative::Derivative; use eth2::types::EventKind; use execution_layer::PayloadStatus; use fork_choice::{AttestationFromBlock, PayloadVerificationStatus}; @@ -306,6 +307,7 @@ pub enum BlockError { parent_root: Hash256, }, BlobValidation(BlobError), + AvailabilityPending(Hash256), } impl From for BlockError { @@ -487,6 +489,7 @@ impl From for BlockError { } /// Stores information about verifying a payload against an execution engine. +#[derive(Clone)] pub struct PayloadVerificationOutcome { pub payload_verification_status: PayloadVerificationStatus, pub is_valid_merge_transition_block: bool, @@ -619,7 +622,8 @@ pub fn signature_verify_chain_segment( /// A wrapper around a `SignedBeaconBlock` that indicates it has been approved for re-gossiping on /// the p2p network. -#[derive(Debug)] +#[derive(Derivative)] +#[derivative(Debug(bound = "T: BeaconChainTypes"))] pub struct GossipVerifiedBlock { pub block: BlockWrapper, pub block_root: Hash256, @@ -663,14 +667,15 @@ pub struct ExecutionPendingBlock { pub payload_verification_handle: PayloadVerificationHandle, } -pub struct ExecutedBlock { - pub block: BlockWrapper, +#[derive(Clone)] +pub struct ExecutedBlock { + pub block: BlockWrapper, pub block_root: Hash256, - pub state: BeaconState, - pub parent_block: SignedBeaconBlock>, + pub state: BeaconState, + pub parent_block: SignedBeaconBlock>, pub parent_eth1_finalization_data: Eth1FinalizationData, pub confirmed_state_roots: Vec, - pub consensus_context: ConsensusContext, + pub consensus_context: ConsensusContext, pub payload_verification_outcome: PayloadVerificationOutcome, } @@ -1156,7 +1161,7 @@ impl IntoExecutionPendingBlock for BlockWrapper block .into_execution_pending_block_slashable(block_root, chain, notify_execution_layer), BlockWrapper::Available(AvailableBlock { block, blobs }) => { - let execution_pending_block = block.into_execution_pending_block_slashable( + let mut execution_pending_block = block.into_execution_pending_block_slashable( block_root, chain, notify_execution_layer, diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 2d92ad6cb1..36a6a33796 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -1,9 +1,11 @@ use crate::beacon_chain::{CanonicalHead, BEACON_CHAIN_DB_KEY, ETH1_CACHE_DB_KEY, OP_POOL_DB_KEY}; use crate::blob_cache::BlobCache; +use crate::block_verification::ExecutedBlock; use crate::eth1_chain::{CachingEth1Backend, SszEth1}; use crate::eth1_finalization_cache::Eth1FinalizationCache; use crate::fork_choice_signal::ForkChoiceSignalTx; use crate::fork_revert::{reset_fork_choice_to_finalization, revert_to_fork_boundary}; +use crate::gossip_blob_cache::DataAvailabilityChecker; use crate::head_tracker::HeadTracker; use crate::migrate::{BackgroundMigrator, MigratorConfig}; use crate::persisted_beacon_chain::PersistedBeaconChain; @@ -85,6 +87,7 @@ pub struct BeaconChainBuilder { event_handler: Option>, slot_clock: Option, shutdown_sender: Option>, + block_importer_sender: Option>>, head_tracker: Option, validator_pubkey_cache: Option>, spec: ChainSpec, @@ -127,6 +130,7 @@ where event_handler: None, slot_clock: None, shutdown_sender: None, + block_importer_sender: None, head_tracker: None, validator_pubkey_cache: None, spec: TEthSpec::default_spec(), @@ -558,6 +562,14 @@ where self } + pub fn block_importer_sender( + mut self, + sender: tokio::sync::mpsc::Sender>, + ) -> Self { + self.block_importer_sender = Some(sender); + self + } + /// Creates a new, empty operation pool. fn empty_op_pool(mut self) -> Self { self.op_pool = Some(OperationPool::new()); @@ -641,12 +653,18 @@ where slot_clock.now().ok_or("Unable to read slot")? }; - let kzg = if let Some(trusted_setup) = self.trusted_setup { + let (kzg, data_availability_checker) = if let (Some(tx), Some(trusted_setup)) = + (self.block_importer_sender, self.trusted_setup) + { let kzg = Kzg::new_from_trusted_setup(trusted_setup) .map_err(|e| format!("Failed to load trusted setup: {:?}", e))?; - Some(Arc::new(kzg)) + let kzg_arc = Arc::new(kzg); + ( + Some(kzg_arc.clone()), + Some(DataAvailabilityChecker::new(kzg_arc, tx)), + ) } else { - None + (None, None) }; let initial_head_block_root = fork_choice @@ -850,7 +868,9 @@ where graffiti: self.graffiti, slasher: self.slasher.clone(), validator_monitor: RwLock::new(validator_monitor), - blob_cache: BlobCache::default(), + //TODO(sean) should we move kzg solely to the da checker? + data_availability_checker, + proposal_blob_cache: BlobCache::default(), kzg, }; diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index bb2d3c5b44..b561b6ea1e 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -134,9 +134,9 @@ impl PayloadNotifier { /// contains a few extra checks by running `partially_verify_execution_payload` first: /// /// https://github.com/ethereum/consensus-specs/blob/v1.1.9/specs/bellatrix/beacon-chain.md#notify_new_payload -async fn notify_new_payload( +async fn notify_new_payload<'a, T: BeaconChainTypes>( chain: &Arc>, - block: BeaconBlockRef, + block: BeaconBlockRef<'a, T::EthSpec>, ) -> Result> { let execution_payload = block.execution_payload()?; diff --git a/beacon_node/beacon_chain/src/gossip_blob_cache.rs b/beacon_node/beacon_chain/src/gossip_blob_cache.rs index f4f2a77f0b..ed12970ec6 100644 --- a/beacon_node/beacon_chain/src/gossip_blob_cache.rs +++ b/beacon_node/beacon_chain/src/gossip_blob_cache.rs @@ -1,7 +1,7 @@ -use crate::blob_verification::verify_data_availability; +use crate::blob_verification::{verify_data_availability, AsBlock}; use crate::block_verification::{ExecutedBlock, IntoExecutionPendingBlock}; use crate::kzg_utils::validate_blob; -use crate::{BeaconChainError, BeaconChainTypes, BlockError}; +use crate::{BeaconChain, BeaconChainError, BeaconChainTypes, BlockError}; use eth2::reqwest::header::Entry; use kzg::Error as KzgError; use kzg::{Kzg, KzgCommitment}; @@ -9,9 +9,10 @@ use parking_lot::{Mutex, RwLock}; use ssz_types::VariableList; use std::collections::{BTreeMap, HashMap, HashSet}; use std::future::Future; -use std::sync::Arc; +use std::sync::{mpsc, Arc}; +use tokio::sync::mpsc::Sender; use types::blob_sidecar::{BlobIdentifier, BlobSidecar}; -use types::{EthSpec, Hash256}; +use types::{EthSpec, Hash256, SignedBeaconBlock}; pub enum BlobCacheError { DuplicateBlob(Hash256), @@ -22,23 +23,25 @@ pub enum BlobCacheError { /// - commitments for blocks that have been gossip verified, but the commitments themselves /// have not been verified against blobs /// - blocks that have been fully verified and only require a data availability check -pub struct GossipBlobCache { - rpc_blob_cache: RwLock>>>, - gossip_blob_cache: Mutex>>, - kzg: Kzg, +pub struct DataAvailabilityChecker { + rpc_blob_cache: RwLock>>>, + gossip_blob_cache: Mutex>>, + kzg: Arc, + tx: Sender>, } -struct GossipBlobCacheInner { - verified_blobs: Vec>>, +struct GossipBlobCache { + verified_blobs: Vec>>, executed_block: Option>, } -impl GossipBlobCache { - pub fn new(kzg: Kzg) -> Self { +impl DataAvailabilityChecker { + pub fn new(kzg: Arc, tx: Sender>) -> Self { Self { - rpc_blob_cache: RwLock::new(HashMap::new()), - gossip_blob_cache: Mutex::new(HashMap::new()), + rpc_blob_cache: <_>::default(), + gossip_blob_cache: <_>::default(), kzg, + tx, } } @@ -47,9 +50,9 @@ impl GossipBlobCache { /// cached, verify the block and import it. /// /// This should only accept gossip verified blobs, so we should not have to worry about dupes. - pub fn put_blob(&self, blob: Arc>) -> Result<(), BlobCacheError> { + pub fn put_blob(&self, blob: Arc>) -> Result<(), BlobCacheError> { // TODO(remove clones) - let verified = validate_blob::( + let verified = validate_blob::( &self.kzg, blob.blob.clone(), blob.kzg_commitment.clone(), @@ -70,11 +73,33 @@ impl GossipBlobCache { .verified_blobs .insert(blob.index as usize, blob.clone()); - if let Some(executed_block) = inner.executed_block.as_ref() { - // trigger reprocessing ? + if let Some(executed_block) = inner.executed_block.take() { + let verified_commitments: Vec<_> = inner + .verified_blobs + .iter() + .map(|blob| blob.kzg_commitment) + .collect(); + if verified_commitments + == executed_block + .block + .as_block() + .message_eip4844() + .unwrap() //TODO(sean) errors + .body + .blob_kzg_commitments + .clone() + .to_vec() + { + // send to reprocessing queue ? + //TODO(sean) try_send? + //TODO(sean) errors + self.tx.try_send(executed_block); + } else { + let _ = inner.executed_block.insert(executed_block); + } } }) - .or_insert(GossipBlobCacheInner { + .or_insert(GossipBlobCache { verified_blobs: vec![blob.clone()], executed_block: None, }); @@ -93,24 +118,26 @@ impl GossipBlobCache { guard .entry(executed_block.block_root) .and_modify(|cache| { - if let Ok(block) = executed_block.block.message_eip4844() { - let verified_commitments_vec: Vec<_> = cache + let block: &SignedBeaconBlock = executed_block.block.as_block(); + if let Ok(block) = block.message_eip4844() { + let verified_commitments: Vec<_> = cache .verified_blobs .iter() .map(|blob| blob.kzg_commitment) .collect(); - let verified_commitments = VariableList::from(verified_commitments_vec); - if verified_commitments == block.body.blob_kzg_commitments { + if verified_commitments == block.body.blob_kzg_commitments.clone().to_vec() { // send to reprocessing queue ? + //TODO(sean) errors + self.tx.try_send(executed_block.clone()); } else { - let _ = cache.executed_block.insert(executed_block); + let _ = cache.executed_block.insert(executed_block.clone()); // log that we cached } } else { // log error } }) - .or_insert(GossipBlobCacheInner { + .or_insert(GossipBlobCache { verified_blobs: vec![], executed_block: Some(executed_block), }); diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index 865539a7b6..f1779b45fb 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -66,7 +66,7 @@ pub use self::historical_blocks::HistoricalBlockError; pub use attestation_verification::Error as AttestationError; pub use beacon_fork_choice_store::{BeaconForkChoiceStore, Error as ForkChoiceStoreError}; pub use block_verification::{ - get_block_root, BlockError, ExecutionPayloadError, GossipVerifiedBlock, + get_block_root, BlockError, ExecutedBlock, ExecutionPayloadError, GossipVerifiedBlock, }; pub use canonical_head::{CachedHead, CanonicalHead, CanonicalHeadRwLock}; pub use eth1_chain::{Eth1Chain, Eth1ChainBackend}; diff --git a/beacon_node/client/src/builder.rs b/beacon_node/client/src/builder.rs index e80b6fd18c..5d283eae61 100644 --- a/beacon_node/client/src/builder.rs +++ b/beacon_node/client/src/builder.rs @@ -862,6 +862,9 @@ where .ok_or("beacon_chain requires a runtime context")? .clone(); + let (tx, mut rx) = + tokio::sync::mpsc::channel::>(1000); //TODO(sean) capacity + let chain = self .beacon_chain_builder .ok_or("beacon_chain requires a beacon_chain_builder")? @@ -871,10 +874,14 @@ where .ok_or("beacon_chain requires a slot clock")?, ) .shutdown_sender(context.executor.shutdown_sender()) + .block_importer_sender(tx) .build() .map_err(|e| format!("Failed to build beacon chain: {}", e))?; - self.beacon_chain = Some(Arc::new(chain)); + let arc_chain = Arc::new(chain); + arc_chain.start_block_importer(rx); + + self.beacon_chain = Some(arc_chain); self.beacon_chain_builder = None; // a beacon chain requires a timer diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index b484f4079a..d53bd793e8 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -4,7 +4,7 @@ use eth2::types::BlockId as CoreBlockId; use std::fmt; use std::str::FromStr; use std::sync::Arc; -use types::{BlobsSidecar, Hash256, SignedBeaconBlock, SignedBlindedBeaconBlock, Slot}; +use types::{BlobSidecar, Hash256, SignedBeaconBlock, SignedBlindedBeaconBlock, Slot}; /// Wraps `eth2::types::BlockId` and provides a simple way to obtain a block or root for a given /// `BlockId`. @@ -216,13 +216,13 @@ impl BlockId { pub async fn blobs_sidecar( &self, chain: &BeaconChain, - ) -> Result>, warp::Rejection> { + ) -> Result>, warp::Rejection> { let root = self.root(chain)?.0; let Some(data_availability_boundary) = chain.data_availability_boundary() else { return Err(warp_utils::reject::custom_not_found("Eip4844 fork disabled".into())); }; - match chain.get_blobs(&root, data_availability_boundary) { - Ok(Some(blob)) => Ok(Arc::new(blob)), + match chain.get_blobs(&root) { + Ok(Some(blob)) => todo!(), // Jimmy's PR will fix this, Ok(None) => Err(warp_utils::reject::custom_not_found(format!( "Blob with block root {} is not in the store", root diff --git a/beacon_node/http_api/src/build_block_contents.rs b/beacon_node/http_api/src/build_block_contents.rs index 9fbde0ce06..6512197ef6 100644 --- a/beacon_node/http_api/src/build_block_contents.rs +++ b/beacon_node/http_api/src/build_block_contents.rs @@ -16,7 +16,7 @@ pub fn build_block_contents { let block_root = &block.canonical_root(); - if let Some(blob_sidecars) = chain.blob_cache.pop(block_root) { + if let Some(blob_sidecars) = chain.proposal_blob_cache.pop(block_root) { let block_and_blobs = BeaconBlockAndBlobSidecars { block, blob_sidecars, diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index dc8bb020ac..b9db262dd2 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -67,23 +67,23 @@ pub async fn publish_block( let delay = get_block_delay_ms(seen_timestamp, block.message(), &chain.slot_clock); metrics::observe_duration(&metrics::HTTP_API_BLOCK_BROADCAST_DELAY_TIMES, delay); - let available_block = match wrapped_block.into_available_block(block_root, &chain) { - Ok(available_block) => available_block, - Err(e) => { - let msg = format!("{:?}", e); + let available_block = match wrapped_block.clone().into_available_block() { + Some(available_block) => available_block, + None => { error!( log, - "Invalid block provided to HTTP API"; - "reason" => &msg + "Invalid block provided to HTTP API unavailable block"; //TODO(sean) probably want a real error here ); - return Err(warp_utils::reject::broadcast_without_import(msg)); + return Err(warp_utils::reject::broadcast_without_import( + "unavailable block".to_string(), + )); } }; match chain .process_block( block_root, - available_block.clone(), + wrapped_block, CountUnrealized::True, NotifyExecutionLayer::Yes, ) @@ -95,14 +95,14 @@ pub async fn publish_block( "Valid block from HTTP API"; "block_delay" => ?delay, "root" => format!("{}", root), - "proposer_index" => available_block.message().proposer_index(), - "slot" => available_block.slot(), + "proposer_index" => available_block.block.message().proposer_index(), + "slot" => available_block.block.slot(), ); // Notify the validator monitor. chain.validator_monitor.read().register_api_block( seen_timestamp, - available_block.message(), + available_block.block.message(), root, &chain.slot_clock, ); @@ -124,7 +124,7 @@ pub async fn publish_block( "Block was broadcast too late"; "msg" => "system may be overloaded, block likely to be orphaned", "delay_ms" => delay.as_millis(), - "slot" => available_block.slot(), + "slot" => available_block.block.slot(), "root" => ?root, ) } else if delay >= delayed_threshold { @@ -133,7 +133,7 @@ pub async fn publish_block( "Block broadcast was delayed"; "msg" => "system may be overloaded, block may be orphaned", "delay_ms" => delay.as_millis(), - "slot" => available_block.slot(), + "slot" => available_block.block.slot(), "root" => ?root, ) } @@ -145,7 +145,7 @@ pub async fn publish_block( log, "Block from HTTP API already known"; "block" => ?block_root, - "slot" => available_block.slot(), + "slot" => available_block.block.slot(), ); Ok(()) } diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index 0b64725544..3d56f53b02 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -802,6 +802,10 @@ impl Worker { verified_block } + Err(BlockError::AvailabilityPending(_)) => { + //TODO(sean) think about what to do hereA + todo!() + } Err(BlockError::ParentUnknown(block)) => { debug!( self.log, @@ -985,7 +989,7 @@ impl Worker { ) .await { - Ok(BlockProcessingResult::Verified(block_root)) => { + Ok(block_root) => { metrics::inc_counter(&metrics::BEACON_PROCESSOR_GOSSIP_BLOCK_IMPORTED_TOTAL); if reprocess_tx @@ -1012,8 +1016,8 @@ impl Worker { self.chain.recompute_head_at_current_slot().await; } - Ok(BlockProcessingResult::AvailabilityPending(executed_block)) => { - // cache in blob cache and make rpc request for blob + Err(BlockError::AvailabilityPending(block_root)) => { + // make rpc request for blob } Err(BlockError::ParentUnknown(block)) => { // Inform the sync manager to find parents for this block diff --git a/beacon_node/network/src/beacon_processor/worker/rpc_methods.rs b/beacon_node/network/src/beacon_processor/worker/rpc_methods.rs index 78b9de303f..a42b56794a 100644 --- a/beacon_node/network/src/beacon_processor/worker/rpc_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/rpc_methods.rs @@ -233,36 +233,37 @@ impl Worker { .get_block_and_blobs_checking_early_attester_cache(&root) .await { - Ok(Some(block_and_blobs)) => { - // - // TODO: HORRIBLE NSFW CODE AHEAD - // - let types::SignedBeaconBlockAndBlobsSidecar {beacon_block, blobs_sidecar} = block_and_blobs; - let types::BlobsSidecar{ beacon_block_root, beacon_block_slot, blobs: blob_bundle, kzg_aggregated_proof }: types::BlobsSidecar<_> = blobs_sidecar.as_ref().clone(); - // TODO: this should be unreachable after this is addressed seriously, - // so for now let's be ok with a panic in the expect. - let block = beacon_block.message_eip4844().expect("We fucked up the block blob stuff"); - // Intentionally not accessing the list directly - for (known_index, blob) in blob_bundle.into_iter().enumerate() { - if (known_index as u64) == index { - let blob_sidecar = types::BlobSidecar{ - block_root: beacon_block_root, - index, - slot: beacon_block_slot, - block_parent_root: block.parent_root, - proposer_index: block.proposer_index, - blob, - kzg_commitment: block.body.blob_kzg_commitments[known_index], // TODO: needs to be stored in a more logical way so that this won't panic. - kzg_proof: kzg_aggregated_proof // TODO: yeah - }; - self.send_response( - peer_id, - Response::BlobsByRoot(Some(Arc::new(blob_sidecar))), - request_id, - ); - send_block_count += 1; - } - } + Ok(Some(())) => { + todo!(); + // // + // // TODO: HORRIBLE NSFW CODE AHEAD + // // + // let types::SignedBeaconBlockAndBlobsSidecar {beacon_block, blobs_sidecar} = block_and_blobs; + // let types::BlobsSidecar{ beacon_block_root, beacon_block_slot, blobs: blob_bundle, kzg_aggregated_proof }: types::BlobsSidecar<_> = blobs_sidecar.as_ref().clone(); + // // TODO: this should be unreachable after this is addressed seriously, + // // so for now let's be ok with a panic in the expect. + // let block = beacon_block.message_eip4844().expect("We fucked up the block blob stuff"); + // // Intentionally not accessing the list directly + // for (known_index, blob) in blob_bundle.into_iter().enumerate() { + // if (known_index as u64) == index { + // let blob_sidecar = types::BlobSidecar{ + // block_root: beacon_block_root, + // index, + // slot: beacon_block_slot, + // block_parent_root: block.parent_root, + // proposer_index: block.proposer_index, + // blob, + // kzg_commitment: block.body.blob_kzg_commitments[known_index], // TODO: needs to be stored in a more logical way so that this won't panic. + // kzg_proof: kzg_aggregated_proof // TODO: yeah + // }; + // self.send_response( + // peer_id, + // Response::BlobsByRoot(Some(Arc::new(blob_sidecar))), + // request_id, + // ); + // send_block_count += 1; + // } + // } } Ok(None) => { debug!( @@ -836,35 +837,36 @@ impl Worker { let mut send_response = true; for root in block_roots { - match self.chain.get_blobs(&root, data_availability_boundary) { + match self.chain.get_blobs(&root) { Ok(Some(blobs)) => { - // TODO: more GROSS code ahead. Reader beware - let types::BlobsSidecar { - beacon_block_root, - beacon_block_slot, - blobs: blob_bundle, - kzg_aggregated_proof: _, - }: types::BlobsSidecar<_> = blobs; - - for (blob_index, blob) in blob_bundle.into_iter().enumerate() { - let blob_sidecar = types::BlobSidecar { - block_root: beacon_block_root, - index: blob_index as u64, - slot: beacon_block_slot, - block_parent_root: Hash256::zero(), - proposer_index: 0, - blob, - kzg_commitment: types::KzgCommitment::default(), - kzg_proof: types::KzgProof::default(), - }; - - blobs_sent += 1; - self.send_network_message(NetworkMessage::SendResponse { - peer_id, - response: Response::BlobsByRange(Some(Arc::new(blob_sidecar))), - id: request_id, - }); - } + todo!(); + // // TODO: more GROSS code ahead. Reader beware + // let types::BlobsSidecar { + // beacon_block_root, + // beacon_block_slot, + // blobs: blob_bundle, + // kzg_aggregated_proof: _, + // }: types::BlobsSidecar<_> = blobs; + // + // for (blob_index, blob) in blob_bundle.into_iter().enumerate() { + // let blob_sidecar = types::BlobSidecar { + // block_root: beacon_block_root, + // index: blob_index as u64, + // slot: beacon_block_slot, + // block_parent_root: Hash256::zero(), + // proposer_index: 0, + // blob, + // kzg_commitment: types::KzgCommitment::default(), + // kzg_proof: types::KzgProof::default(), + // }; + // + // blobs_sent += 1; + // self.send_network_message(NetworkMessage::SendResponse { + // peer_id, + // response: Response::BlobsByRange(Some(Arc::new(blob_sidecar))), + // id: request_id, + // }); + // } } Ok(None) => { error!( diff --git a/beacon_node/network/src/beacon_processor/worker/sync_methods.rs b/beacon_node/network/src/beacon_processor/worker/sync_methods.rs index 093e69729b..5bbcb4e1f3 100644 --- a/beacon_node/network/src/beacon_processor/worker/sync_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/sync_methods.rs @@ -86,27 +86,16 @@ impl Worker { }; let slot = block.slot(); let parent_root = block.message().parent_root(); - let available_block = block - .into_available_block(block_root, &self.chain) - .map_err(BlockError::BlobValidation); - let result = match available_block { - Ok(BlockProcessingResult::Verified(block)) => { - self.chain - .process_block( - block_root, - block, - CountUnrealized::True, - NotifyExecutionLayer::Yes, - ) - .await - } - Ok(BlockProcessingResult::AvailabilityPending(executed_block)) => { - // Shouldn't happen as sync should only send blocks for processing - // after sending blocks into the availability cache. - } - Err(e) => Err(e), - }; + let result = self + .chain + .process_block( + block_root, + block, + CountUnrealized::True, + NotifyExecutionLayer::Yes, + ) + .await; metrics::inc_counter(&metrics::BEACON_PROCESSOR_RPC_BLOCK_IMPORTED_TOTAL); diff --git a/consensus/state_processing/src/consensus_context.rs b/consensus/state_processing/src/consensus_context.rs index 4c8966f92c..37bd5fe446 100644 --- a/consensus/state_processing/src/consensus_context.rs +++ b/consensus/state_processing/src/consensus_context.rs @@ -7,7 +7,7 @@ use types::{ ChainSpec, Epoch, EthSpec, Hash256, IndexedAttestation, SignedBeaconBlock, Slot, }; -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ConsensusContext { /// Slot to act as an identifier/safeguard slot: Slot, diff --git a/lcli/src/parse_ssz.rs b/lcli/src/parse_ssz.rs index 0e87e330b1..70f34e09e3 100644 --- a/lcli/src/parse_ssz.rs +++ b/lcli/src/parse_ssz.rs @@ -63,7 +63,7 @@ pub fn run_parse_ssz(matches: &ArgMatches) -> Result<(), String> { "state_merge" => decode_and_print::>(&bytes, format)?, "state_capella" => decode_and_print::>(&bytes, format)?, "state_eip4844" => decode_and_print::>(&bytes, format)?, - "blobs_sidecar" => decode_and_print::>(&bytes, format)?, + "blob_sidecar" => decode_and_print::>(&bytes, format)?, other => return Err(format!("Unknown type: {}", other)), }; diff --git a/testing/ef_tests/src/type_name.rs b/testing/ef_tests/src/type_name.rs index 19b3535fbf..d94dfef485 100644 --- a/testing/ef_tests/src/type_name.rs +++ b/testing/ef_tests/src/type_name.rs @@ -50,7 +50,7 @@ type_name_generic!(BeaconBlockBodyCapella, "BeaconBlockBody"); type_name_generic!(BeaconBlockBodyEip4844, "BeaconBlockBody"); type_name!(BeaconBlockHeader); type_name_generic!(BeaconState); -type_name_generic!(BlobsSidecar); +type_name_generic!(BlobSidecar); type_name!(Checkpoint); type_name_generic!(ContributionAndProof); type_name!(Deposit); @@ -88,9 +88,5 @@ type_name!(Validator); type_name!(VoluntaryExit); type_name!(Withdrawal); type_name!(BlsToExecutionChange, "BLSToExecutionChange"); -type_name_generic!( - SignedBeaconBlockAndBlobsSidecarDecode, - "SignedBeaconBlockAndBlobsSidecar" -); type_name!(SignedBlsToExecutionChange, "SignedBLSToExecutionChange"); type_name!(HistoricalSummary); From 1301c62436d261ea646a86125584d227a618254c Mon Sep 17 00:00:00 2001 From: Jimmy Chen Date: Sat, 18 Mar 2023 00:29:25 +1100 Subject: [PATCH 11/16] Validator blob signing for the unblinded flow (#4096) * Implement validator blob signing (full block and full blob) * Fix compilation error and remove redundant slot check * Fix clippy error --- common/eth2/src/types.rs | 20 +- consensus/types/src/chain_spec.rs | 14 +- consensus/types/src/config_and_preset.rs | 2 +- consensus/types/src/signed_blob.rs | 4 + validator_client/src/block_service.rs | 241 +++++++++++++---------- validator_client/src/signing_method.rs | 6 + validator_client/src/validator_store.rs | 45 ++++- 7 files changed, 218 insertions(+), 114 deletions(-) diff --git a/common/eth2/src/types.rs b/common/eth2/src/types.rs index db64d74c2a..bc27ddb474 100644 --- a/common/eth2/src/types.rs +++ b/common/eth2/src/types.rs @@ -1326,7 +1326,7 @@ impl> Into> pub type BlockContentsTuple = ( SignedBeaconBlock, - Option, ::MaxBlobsPerBlock>>, + Option>, ); /// A wrapper over a [`SignedBeaconBlock`] or a [`SignedBeaconBlockAndBlobSidecars`]. @@ -1374,9 +1374,25 @@ impl> From> From> + for SignedBlockContents +{ + fn from(block_contents_tuple: BlockContentsTuple) -> Self { + match block_contents_tuple { + (signed_block, None) => SignedBlockContents::Block(signed_block), + (signed_block, Some(signed_blob_sidecars)) => { + SignedBlockContents::BlockAndBlobSidecars(SignedBeaconBlockAndBlobSidecars { + signed_block, + signed_blob_sidecars, + }) + } + } + } +} + #[derive(Debug, Clone, Serialize, Deserialize, Encode)] #[serde(bound = "T: EthSpec")] pub struct SignedBeaconBlockAndBlobSidecars> { pub signed_block: SignedBeaconBlock, - pub signed_blob_sidecars: VariableList, ::MaxBlobsPerBlock>, + pub signed_blob_sidecars: SignedBlobSidecarList, } diff --git a/consensus/types/src/chain_spec.rs b/consensus/types/src/chain_spec.rs index 1f947c9e7b..c107f790c5 100644 --- a/consensus/types/src/chain_spec.rs +++ b/consensus/types/src/chain_spec.rs @@ -14,7 +14,7 @@ pub enum Domain { BlsToExecutionChange, BeaconProposer, BeaconAttester, - BlobsSideCar, + BlobSidecar, Randao, Deposit, VoluntaryExit, @@ -100,7 +100,7 @@ pub struct ChainSpec { */ pub(crate) domain_beacon_proposer: u32, pub(crate) domain_beacon_attester: u32, - pub(crate) domain_blobs_sidecar: u32, + pub(crate) domain_blob_sidecar: u32, pub(crate) domain_randao: u32, pub(crate) domain_deposit: u32, pub(crate) domain_voluntary_exit: u32, @@ -366,7 +366,7 @@ impl ChainSpec { match domain { Domain::BeaconProposer => self.domain_beacon_proposer, Domain::BeaconAttester => self.domain_beacon_attester, - Domain::BlobsSideCar => self.domain_blobs_sidecar, + Domain::BlobSidecar => self.domain_blob_sidecar, Domain::Randao => self.domain_randao, Domain::Deposit => self.domain_deposit, Domain::VoluntaryExit => self.domain_voluntary_exit, @@ -574,7 +574,7 @@ impl ChainSpec { domain_voluntary_exit: 4, domain_selection_proof: 5, domain_aggregate_and_proof: 6, - domain_blobs_sidecar: 10, // 0x0a000000 + domain_blob_sidecar: 11, // 0x0B000000 /* * Fork choice @@ -809,7 +809,7 @@ impl ChainSpec { domain_voluntary_exit: 4, domain_selection_proof: 5, domain_aggregate_and_proof: 6, - domain_blobs_sidecar: 10, + domain_blob_sidecar: 11, /* * Fork choice @@ -1285,7 +1285,7 @@ mod tests { test_domain(Domain::BeaconProposer, spec.domain_beacon_proposer, &spec); test_domain(Domain::BeaconAttester, spec.domain_beacon_attester, &spec); - test_domain(Domain::BlobsSideCar, spec.domain_blobs_sidecar, &spec); + test_domain(Domain::BlobSidecar, spec.domain_blob_sidecar, &spec); test_domain(Domain::Randao, spec.domain_randao, &spec); test_domain(Domain::Deposit, spec.domain_deposit, &spec); test_domain(Domain::VoluntaryExit, spec.domain_voluntary_exit, &spec); @@ -1311,7 +1311,7 @@ mod tests { &spec, ); - test_domain(Domain::BlobsSideCar, spec.domain_blobs_sidecar, &spec); + test_domain(Domain::BlobSidecar, spec.domain_blob_sidecar, &spec); } fn apply_bit_mask(domain_bytes: [u8; 4], spec: &ChainSpec) -> u32 { diff --git a/consensus/types/src/config_and_preset.rs b/consensus/types/src/config_and_preset.rs index ac93818b9c..957376c3d6 100644 --- a/consensus/types/src/config_and_preset.rs +++ b/consensus/types/src/config_and_preset.rs @@ -78,7 +78,7 @@ pub fn get_extra_fields(spec: &ChainSpec) -> HashMap { "bls_withdrawal_prefix".to_uppercase() => u8_hex(spec.bls_withdrawal_prefix_byte), "domain_beacon_proposer".to_uppercase() => u32_hex(spec.domain_beacon_proposer), "domain_beacon_attester".to_uppercase() => u32_hex(spec.domain_beacon_attester), - "domain_blobs_sidecar".to_uppercase() => u32_hex(spec.domain_blobs_sidecar), + "domain_blob_sidecar".to_uppercase() => u32_hex(spec.domain_blob_sidecar), "domain_randao".to_uppercase()=> u32_hex(spec.domain_randao), "domain_deposit".to_uppercase()=> u32_hex(spec.domain_deposit), "domain_voluntary_exit".to_uppercase() => u32_hex(spec.domain_voluntary_exit), diff --git a/consensus/types/src/signed_blob.rs b/consensus/types/src/signed_blob.rs index 4121b8b7f2..f9ae481247 100644 --- a/consensus/types/src/signed_blob.rs +++ b/consensus/types/src/signed_blob.rs @@ -2,6 +2,7 @@ use crate::{test_utils::TestRandom, BlobSidecar, EthSpec, Signature}; use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; +use ssz_types::VariableList; use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; @@ -25,3 +26,6 @@ pub struct SignedBlobSidecar { pub message: BlobSidecar, pub signature: Signature, } + +pub type SignedBlobSidecarList = + VariableList, ::MaxBlobsPerBlock>; diff --git a/validator_client/src/block_service.rs b/validator_client/src/block_service.rs index 0eb9a07c39..5fa32d3f42 100644 --- a/validator_client/src/block_service.rs +++ b/validator_client/src/block_service.rs @@ -6,9 +6,11 @@ use crate::{ OfflineOnFailure, }; use crate::{http_metrics::metrics, validator_store::ValidatorStore}; +use bls::SignatureBytes; use environment::RuntimeContext; -use eth2::types::SignedBlockContents; -use slog::{crit, debug, error, info, trace, warn}; +use eth2::types::{BlockContents, SignedBlockContents}; +use eth2::BeaconNodeHttpClient; +use slog::{crit, debug, error, info, trace, warn, Logger}; use slot_clock::SlotClock; use std::ops::Deref; use std::sync::Arc; @@ -16,8 +18,8 @@ use std::time::Duration; use tokio::sync::mpsc; use tokio::time::sleep; use types::{ - AbstractExecPayload, BeaconBlock, BlindedPayload, BlockType, EthSpec, FullPayload, Graffiti, - PublicKeyBytes, Slot, + AbstractExecPayload, BlindedPayload, BlockType, EthSpec, FullPayload, Graffiti, PublicKeyBytes, + Slot, }; #[derive(Debug)] @@ -342,80 +344,46 @@ impl BlockService { "slot" => slot.as_u64(), ); // Request block from first responsive beacon node. - let block = self + let block_contents = self .beacon_nodes .first_success( RequireSynced::No, OfflineOnFailure::Yes, - |beacon_node| async move { - let block: BeaconBlock = match Payload::block_type() { - BlockType::Full => { - let _get_timer = metrics::start_timer_vec( - &metrics::BLOCK_SERVICE_TIMES, - &[metrics::BEACON_BLOCK_HTTP_GET], - ); - beacon_node - .get_validator_blocks::( - slot, - randao_reveal_ref, - graffiti.as_ref(), - ) - .await - .map_err(|e| { - BlockError::Recoverable(format!( - "Error from beacon node when producing block: {:?}", - e - )) - })? - .data - .into() - } - BlockType::Blinded => { - let _get_timer = metrics::start_timer_vec( - &metrics::BLOCK_SERVICE_TIMES, - &[metrics::BLINDED_BEACON_BLOCK_HTTP_GET], - ); - beacon_node - .get_validator_blinded_blocks::( - slot, - randao_reveal_ref, - graffiti.as_ref(), - ) - .await - .map_err(|e| { - BlockError::Recoverable(format!( - "Error from beacon node when producing block: {:?}", - e - )) - })? - .data - } - }; - - info!( + move |beacon_node| { + Self::get_validator_block( + beacon_node, + slot, + randao_reveal_ref, + graffiti, + proposer_index, log, - "Received unsigned block"; - "slot" => slot.as_u64(), - ); - if proposer_index != Some(block.proposer_index()) { - return Err(BlockError::Recoverable( - "Proposer index does not match block proposer. Beacon chain re-orged" - .to_string(), - )); - } - - Ok::<_, BlockError>(block) + ) }, ) .await?; + let (block, maybe_blob_sidecars) = block_contents.deconstruct(); let signing_timer = metrics::start_timer(&metrics::BLOCK_SIGNING_TIMES); - let signed_block_contents: SignedBlockContents = self_ref + + let signed_block = self_ref .validator_store .sign_block::(*validator_pubkey_ref, block, current_slot) .await - .map_err(|e| BlockError::Recoverable(format!("Unable to sign block: {:?}", e)))? - .into(); + .map_err(|e| BlockError::Recoverable(format!("Unable to sign block: {:?}", e)))?; + + let maybe_signed_blobs = match maybe_blob_sidecars { + Some(blob_sidecars) => Some( + self_ref + .validator_store + .sign_blobs(*validator_pubkey_ref, blob_sidecars) + .await + .map_err(|e| { + BlockError::Recoverable(format!("Unable to sign blob: {:?}", e)) + })?, + ), + None => None, + }; + let signing_time_ms = Duration::from_secs_f64(signing_timer.map_or(0.0, |t| t.stop_and_record())).as_millis(); @@ -426,46 +394,19 @@ impl BlockService { "signing_time_ms" => signing_time_ms, ); + let signed_block_contents = SignedBlockContents::from((signed_block, maybe_signed_blobs)); + // Publish block with first available beacon node. self.beacon_nodes .first_success( RequireSynced::No, OfflineOnFailure::Yes, |beacon_node| async { - match Payload::block_type() { - BlockType::Full => { - let _post_timer = metrics::start_timer_vec( - &metrics::BLOCK_SERVICE_TIMES, - &[metrics::BEACON_BLOCK_HTTP_POST], - ); - beacon_node - .post_beacon_blocks(&signed_block_contents) - .await - .map_err(|e| { - BlockError::Irrecoverable(format!( - "Error from beacon node when publishing block: {:?}", - e - )) - })? - } - BlockType::Blinded => { - let _post_timer = metrics::start_timer_vec( - &metrics::BLOCK_SERVICE_TIMES, - &[metrics::BLINDED_BEACON_BLOCK_HTTP_POST], - ); - beacon_node - // TODO: need to be adjusted for blobs - .post_beacon_blinded_blocks(signed_block_contents.signed_block()) - .await - .map_err(|e| { - BlockError::Irrecoverable(format!( - "Error from beacon node when publishing block: {:?}", - e - )) - })? - } - } - Ok::<_, BlockError>(()) + Self::publish_signed_block_contents::( + &signed_block_contents, + beacon_node, + ) + .await }, ) .await?; @@ -482,4 +423,106 @@ impl BlockService { Ok(()) } + + async fn publish_signed_block_contents>( + signed_block_contents: &SignedBlockContents, + beacon_node: &BeaconNodeHttpClient, + ) -> Result<(), BlockError> { + match Payload::block_type() { + BlockType::Full => { + let _post_timer = metrics::start_timer_vec( + &metrics::BLOCK_SERVICE_TIMES, + &[metrics::BEACON_BLOCK_HTTP_POST], + ); + beacon_node + .post_beacon_blocks(signed_block_contents) + .await + .map_err(|e| { + BlockError::Irrecoverable(format!( + "Error from beacon node when publishing block: {:?}", + e + )) + })? + } + BlockType::Blinded => { + let _post_timer = metrics::start_timer_vec( + &metrics::BLOCK_SERVICE_TIMES, + &[metrics::BLINDED_BEACON_BLOCK_HTTP_POST], + ); + todo!("need to be adjusted for blobs"); + // beacon_node + // .post_beacon_blinded_blocks(signed_block_contents.signed_block()) + // .await + // .map_err(|e| { + // BlockError::Irrecoverable(format!( + // "Error from beacon node when publishing block: {:?}", + // e + // )) + // })? + } + } + Ok::<_, BlockError>(()) + } + + async fn get_validator_block>( + beacon_node: &BeaconNodeHttpClient, + slot: Slot, + randao_reveal_ref: &SignatureBytes, + graffiti: Option, + proposer_index: Option, + log: &Logger, + ) -> Result, BlockError> { + let block_contents: BlockContents = match Payload::block_type() { + BlockType::Full => { + let _get_timer = metrics::start_timer_vec( + &metrics::BLOCK_SERVICE_TIMES, + &[metrics::BEACON_BLOCK_HTTP_GET], + ); + beacon_node + .get_validator_blocks::(slot, randao_reveal_ref, graffiti.as_ref()) + .await + .map_err(|e| { + BlockError::Recoverable(format!( + "Error from beacon node when producing block: {:?}", + e + )) + })? + .data + } + BlockType::Blinded => { + let _get_timer = metrics::start_timer_vec( + &metrics::BLOCK_SERVICE_TIMES, + &[metrics::BLINDED_BEACON_BLOCK_HTTP_GET], + ); + todo!("implement blinded flow for blobs"); + // beacon_node + // .get_validator_blinded_blocks::( + // slot, + // randao_reveal_ref, + // graffiti.as_ref(), + // ) + // .await + // .map_err(|e| { + // BlockError::Recoverable(format!( + // "Error from beacon node when producing block: {:?}", + // e + // )) + // })? + // .data + } + }; + + info!( + log, + "Received unsigned block"; + "slot" => slot.as_u64(), + ); + if proposer_index != Some(block_contents.block().proposer_index()) { + return Err(BlockError::Recoverable( + "Proposer index does not match block proposer. Beacon chain re-orged".to_string(), + )); + } + + Ok::<_, BlockError>(block_contents) + } } diff --git a/validator_client/src/signing_method.rs b/validator_client/src/signing_method.rs index ae9df08096..e428bffcff 100644 --- a/validator_client/src/signing_method.rs +++ b/validator_client/src/signing_method.rs @@ -37,6 +37,7 @@ pub enum Error { pub enum SignableMessage<'a, T: EthSpec, Payload: AbstractExecPayload = FullPayload> { RandaoReveal(Epoch), BeaconBlock(&'a BeaconBlock), + BlobSidecar(&'a BlobSidecar), AttestationData(&'a AttestationData), SignedAggregateAndProof(&'a AggregateAndProof), SelectionProof(Slot), @@ -58,6 +59,7 @@ impl<'a, T: EthSpec, Payload: AbstractExecPayload> SignableMessage<'a, T, Pay match self { SignableMessage::RandaoReveal(epoch) => epoch.signing_root(domain), SignableMessage::BeaconBlock(b) => b.signing_root(domain), + SignableMessage::BlobSidecar(b) => b.signing_root(domain), SignableMessage::AttestationData(a) => a.signing_root(domain), SignableMessage::SignedAggregateAndProof(a) => a.signing_root(domain), SignableMessage::SelectionProof(slot) => slot.signing_root(domain), @@ -180,6 +182,10 @@ impl SigningMethod { Web3SignerObject::RandaoReveal { epoch } } SignableMessage::BeaconBlock(block) => Web3SignerObject::beacon_block(block)?, + SignableMessage::BlobSidecar(_) => { + // https://github.com/ConsenSys/web3signer/issues/726 + unimplemented!("Web3Signer blob signing not implemented.") + } SignableMessage::AttestationData(a) => Web3SignerObject::Attestation(a), SignableMessage::SignedAggregateAndProof(a) => { Web3SignerObject::AggregateAndProof(a) diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index 36a0d05734..294689e3c1 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -6,6 +6,7 @@ use crate::{ Config, }; use account_utils::{validator_definitions::ValidatorDefinition, ZeroizeString}; +use eth2::types::VariableList; use parking_lot::{Mutex, RwLock}; use slashing_protection::{ interchange::Interchange, InterchangeError, NotSafe, Safe, SlashingDatabase, @@ -19,11 +20,12 @@ use std::sync::Arc; use task_executor::TaskExecutor; use types::{ attestation::Error as AttestationError, graffiti::GraffitiString, AbstractExecPayload, Address, - AggregateAndProof, Attestation, BeaconBlock, BlindedPayload, ChainSpec, ContributionAndProof, - Domain, Epoch, EthSpec, Fork, Graffiti, Hash256, Keypair, PublicKeyBytes, SelectionProof, - Signature, SignedAggregateAndProof, SignedBeaconBlock, SignedContributionAndProof, SignedRoot, - SignedValidatorRegistrationData, Slot, SyncAggregatorSelectionData, SyncCommitteeContribution, - SyncCommitteeMessage, SyncSelectionProof, SyncSubnetId, ValidatorRegistrationData, + AggregateAndProof, Attestation, BeaconBlock, BlindedPayload, BlobSidecarList, ChainSpec, + ContributionAndProof, Domain, Epoch, EthSpec, Fork, Graffiti, Hash256, Keypair, PublicKeyBytes, + SelectionProof, Signature, SignedAggregateAndProof, SignedBeaconBlock, SignedBlobSidecar, + SignedBlobSidecarList, SignedContributionAndProof, SignedRoot, SignedValidatorRegistrationData, + Slot, SyncAggregatorSelectionData, SyncCommitteeContribution, SyncCommitteeMessage, + SyncSelectionProof, SyncSubnetId, ValidatorRegistrationData, }; use validator_dir::ValidatorDir; @@ -531,6 +533,39 @@ impl ValidatorStore { } } + pub async fn sign_blobs( + &self, + validator_pubkey: PublicKeyBytes, + blob_sidecars: BlobSidecarList, + ) -> Result, Error> { + let mut signed_blob_sidecars = Vec::new(); + + for blob_sidecar in blob_sidecars.into_iter() { + let slot = blob_sidecar.slot; + let signing_epoch = slot.epoch(E::slots_per_epoch()); + let signing_context = self.signing_context(Domain::BlobSidecar, signing_epoch); + let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?; + + let signature = signing_method + .get_signature::>( + SignableMessage::BlobSidecar(&blob_sidecar), + signing_context, + &self.spec, + &self.task_executor, + ) + .await?; + + metrics::inc_counter_vec(&metrics::SIGNED_BLOBS_TOTAL, &[metrics::SUCCESS]); + + signed_blob_sidecars.push(SignedBlobSidecar { + message: blob_sidecar, + signature, + }); + } + + Ok(VariableList::from(signed_blob_sidecars)) + } + pub async fn sign_attestation( &self, validator_pubkey: PublicKeyBytes, From 05db0d2ba3744267f4a68ae4e4029aafc3730868 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Fri, 17 Mar 2023 16:12:40 -0400 Subject: [PATCH 12/16] everything, everywhere, all at once --- beacon_node/beacon_chain/src/beacon_chain.rs | 295 +++++++++--------- .../beacon_chain/src/blob_verification.rs | 33 +- .../beacon_chain/src/block_verification.rs | 9 +- beacon_node/beacon_chain/src/builder.rs | 23 +- .../beacon_chain/src/early_attester_cache.rs | 6 +- .../beacon_chain/src/gossip_blob_cache.rs | 182 ++++++++--- beacon_node/beacon_chain/src/lib.rs | 7 +- beacon_node/beacon_chain/src/test_utils.rs | 6 +- beacon_node/client/src/builder.rs | 6 +- beacon_node/http_api/src/publish_blocks.rs | 22 +- .../beacon_processor/worker/gossip_methods.rs | 41 ++- .../beacon_processor/worker/sync_methods.rs | 5 +- beacon_node/network/src/sync/manager.rs | 4 +- beacon_node/store/src/hot_cold_store.rs | 19 +- beacon_node/store/src/lib.rs | 3 +- consensus/types/src/blob_sidecar.rs | 4 + consensus/types/src/signed_blob.rs | 41 ++- 17 files changed, 453 insertions(+), 253 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 5039724649..4115b2965b 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -25,7 +25,7 @@ use crate::eth1_finalization_cache::{Eth1FinalizationCache, Eth1FinalizationData use crate::events::ServerSentEventHandler; use crate::execution_payload::{get_execution_payload, NotifyExecutionLayer, PreparePayloadHandle}; use crate::fork_choice_signal::{ForkChoiceSignalRx, ForkChoiceSignalTx, ForkChoiceWaitResult}; -use crate::gossip_blob_cache::DataAvailabilityChecker; +use crate::gossip_blob_cache::{Availability, AvailabilityCheckError, DataAvailabilityChecker}; use crate::head_tracker::HeadTracker; use crate::historical_blocks::HistoricalBlockError; use crate::kzg_utils; @@ -116,7 +116,7 @@ use tokio::task::JoinHandle; use tree_hash::TreeHash; use types::beacon_block_body::KzgCommitments; use types::beacon_state::CloneConfig; -use types::blob_sidecar::Blobs; +use types::blob_sidecar::{BlobIdentifier, BlobSidecarArcList, Blobs}; use types::consts::eip4844::MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS; use types::consts::merge::INTERVALS_PER_SLOT; use types::*; @@ -189,6 +189,25 @@ pub enum WhenSlotSkipped { Prev, } +#[derive(Debug)] +pub enum AvailabilityProcessingStatus { + PendingBlobs(Vec), + PendingBlock(Hash256), + Imported(Hash256), +} + +//TODO(sean) using this in tests for now +impl TryInto for AvailabilityProcessingStatus { + type Error = (); + + fn try_into(self) -> Result { + match self { + AvailabilityProcessingStatus::Imported(hash) => Ok(hash.into()), + _ => Err(()), + } + } +} + /// The result of a chain segment processing. pub enum ChainSegmentResult { /// Processing this chain segment finished successfully. @@ -445,7 +464,7 @@ pub struct BeaconChain { /// Provides monitoring of a set of explicitly defined validators. pub validator_monitor: RwLock>, pub proposal_blob_cache: BlobCache, - pub data_availability_checker: Option>, + pub data_availability_checker: DataAvailabilityChecker, pub kzg: Option>, } @@ -1061,7 +1080,7 @@ impl BeaconChain { pub fn get_blobs( &self, block_root: &Hash256, - ) -> Result>, Error> { + ) -> Result>, Error> { Ok(self.store.get_blobs(block_root)?) } @@ -2635,6 +2654,18 @@ impl BeaconChain { .map_err(BeaconChainError::TokioJoin)? } + pub async fn process_blob( + self: &Arc, + blob: BlobSidecar, + count_unrealized: CountUnrealized, + ) -> Result> { + self.check_availability_and_maybe_import( + |chain| chain.data_availability_checker.put_blob(Arc::new(blob)), + count_unrealized, + ) + .await + } + /// Returns `Ok(block_root)` if the given `unverified_block` was successfully verified and /// imported into the chain. /// @@ -2653,7 +2684,7 @@ impl BeaconChain { unverified_block: B, count_unrealized: CountUnrealized, notify_execution_layer: NotifyExecutionLayer, - ) -> Result> { + ) -> Result> { // Start the Prometheus timer. let _full_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_TIMES); @@ -2673,65 +2704,18 @@ impl BeaconChain { let executed_block = self .clone() .into_executed_block(execution_pending, count_unrealized) - .await?; + .await + .map_err(|e| self.handle_block_error(e))?; - // Check if the executed block has all it's blobs available to qualify as a fully - // available block - let import_block = if let Some(da_checker) = self.data_availability_checker.as_ref() { - da_checker.put_block(executed_block); //TODO(sean) errors - return Err(BlockError::AvailabilityPending(block_root)); - } else { - self.clone().import_available_block( - executed_block, - VerifiedBlobs::PreEip4844, - count_unrealized, - ) - }; - - // Verify and import the block. - match import_block.await { - // The block was successfully verified and imported. Yay. - Ok(block_root) => { - trace!( - self.log, - "Beacon block imported"; - "block_root" => ?block_root, - "block_slot" => slot, - ); - - // Increment the Prometheus counter for block processing successes. - metrics::inc_counter(&metrics::BLOCK_PROCESSING_SUCCESSES); - - Ok(block_root) - } - Err(e @ BlockError::BeaconChainError(BeaconChainError::TokioJoin(_))) => { - debug!( - self.log, - "Beacon block processing cancelled"; - "error" => ?e, - ); - Err(e) - } - // There was an error whilst attempting to verify and import the block. The block might - // be partially verified or partially imported. - Err(BlockError::BeaconChainError(e)) => { - crit!( - self.log, - "Beacon block processing error"; - "error" => ?e, - ); - Err(BlockError::BeaconChainError(e)) - } - // The block failed verification. - Err(other) => { - trace!( - self.log, - "Beacon block rejected"; - "reason" => other.to_string(), - ); - Err(other) - } - } + self.check_availability_and_maybe_import( + |chain| { + chain + .data_availability_checker + .check_block_availability(executed_block) + }, + count_unrealized, + ) + .await } /// Accepts a fully-verified block and awaits on it's payload verification handle to @@ -2800,55 +2784,118 @@ impl BeaconChain { }) } + fn handle_block_error(&self, e: BlockError) -> BlockError { + match e { + e @ BlockError::BeaconChainError(BeaconChainError::TokioJoin(_)) => { + debug!( + self.log, + "Beacon block processing cancelled"; + "error" => ?e, + ); + e + } + BlockError::BeaconChainError(e) => { + crit!( + self.log, + "Beacon block processing error"; + "error" => ?e, + ); + BlockError::BeaconChainError(e) + } + other => { + trace!( + self.log, + "Beacon block rejected"; + "reason" => other.to_string(), + ); + other + } + } + } + /// Accepts a fully-verified, available block and imports it into the chain without performing any /// additional verification. /// /// An error is returned if the block was unable to be imported. It may be partially imported /// (i.e., this function is not atomic). - async fn import_available_block( - self: Arc, - executed_block: ExecutedBlock, - blobs: VerifiedBlobs, + async fn check_availability_and_maybe_import( + self: &Arc, + cache_fn: impl FnOnce(Arc) -> Result, AvailabilityCheckError>, count_unrealized: CountUnrealized, - ) -> Result> { - let ExecutedBlock { - block, - block_root, - state, - parent_block, - confirmed_state_roots, - payload_verification_outcome, - parent_eth1_finalization_data, - consensus_context, - } = executed_block; + ) -> Result> { + let availability = cache_fn(self.clone())?; + match availability { + Availability::Available(block) => { + let ExecutedBlock { + block, + block_root, + state, + parent_block, + parent_eth1_finalization_data, + confirmed_state_roots, + consensus_context, + payload_verification_outcome, + } = block; - let chain = self.clone(); + let available_block = match block { + BlockWrapper::Available(block) => block, + BlockWrapper::AvailabilityPending(_) => { + todo!() // logic error + } + }; - let available_block = AvailableBlock { - block: block.block_cloned(), - blobs: blobs, - }; + let slot = available_block.block.slot(); - let block_hash = self - .spawn_blocking_handle( - move || { - chain.import_block( - available_block, - block_root, - state, - confirmed_state_roots, - payload_verification_outcome.payload_verification_status, - count_unrealized, - parent_block, - parent_eth1_finalization_data, - consensus_context, + // import + let chain = self.clone(); + let result = self + .spawn_blocking_handle( + move || { + chain.import_block( + available_block, + block_root, + state, + confirmed_state_roots, + payload_verification_outcome.payload_verification_status, + count_unrealized, + parent_block, + parent_eth1_finalization_data, + consensus_context, + ) + }, + "payload_verification_handle", ) - }, - "payload_verification_handle", - ) - .await??; + .await + .map_err(|e| { + let b = BlockError::from(e); + self.handle_block_error(b) + })?; - Ok(block_hash) + match result { + // The block was successfully verified and imported. Yay. + Ok(block_root) => { + trace!( + self.log, + "Beacon block imported"; + "block_root" => ?block_root, + "block_slot" => slot, + ); + + // Increment the Prometheus counter for block processing successes. + metrics::inc_counter(&metrics::BLOCK_PROCESSING_SUCCESSES); + + Ok(AvailabilityProcessingStatus::Imported(block_root)) + } + Err(e) => Err(self.handle_block_error(e)), + } + } + Availability::PendingBlock(block_root) => { + Ok(AvailabilityProcessingStatus::PendingBlock(block_root)) + } + Availability::PendingBlobs(blob_ids) => { + Ok(AvailabilityProcessingStatus::PendingBlobs(blob_ids)) + } + } } /// Accepts a fully-verified and available block and imports it into the chain without performing any @@ -6154,52 +6201,6 @@ impl BeaconChain { .map(|fork_epoch| fork_epoch <= current_epoch) .unwrap_or(false)) } - - pub fn start_block_importer( - self: &Arc, - mut rx: tokio::sync::mpsc::Receiver>, - ) { - let chain = self.clone(); - self.task_executor.spawn( - async move { - while let Some(block) = rx.recv().await { - let ExecutedBlock { - block, - block_root, - state, - parent_block, - parent_eth1_finalization_data, - confirmed_state_roots, - consensus_context, - payload_verification_outcome, - } = block; - - let available_block = block.into_available_block().unwrap(); //TODO(sean) remove unwrap - - let chain_inner = chain.clone(); - let block_hash = chain - .spawn_blocking_handle( - move || { - chain_inner.import_block( - available_block, - block_root, - state, - confirmed_state_roots, - payload_verification_outcome.payload_verification_status, - CountUnrealized::True, //TODO(sean) - parent_block, - parent_eth1_finalization_data, - consensus_context, - ) - }, - "block_importer", - ) - .await; - } - }, - "block_importer_listener", - ); - } } impl Drop for BeaconChain { diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 013fae1fd5..d80fe03f79 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -1,16 +1,20 @@ use derivative::Derivative; use slot_clock::SlotClock; +use state_processing::ConsensusContext; use std::sync::Arc; use crate::beacon_chain::{ BeaconChain, BeaconChainTypes, MAXIMUM_GOSSIP_CLOCK_DISPARITY, VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT, }; +use crate::snapshot_cache::PreProcessingSnapshot; use crate::BeaconChainError; use state_processing::per_block_processing::eip4844::eip4844::verify_kzg_commitments_against_transactions; +use types::blob_sidecar::BlobSidecarArcList; use types::{ - BeaconBlockRef, BeaconStateError, BlobSidecarList, Epoch, EthSpec, Hash256, KzgCommitment, - SignedBeaconBlock, SignedBeaconBlockHeader, SignedBlobSidecar, Slot, Transactions, + BeaconBlockRef, BeaconStateError, BlobSidecar, BlobSidecarList, Epoch, EthSpec, Hash256, + KzgCommitment, SignedBeaconBlock, SignedBeaconBlockHeader, SignedBlobSidecar, Slot, + Transactions, }; #[derive(Debug)] @@ -122,11 +126,24 @@ impl From for BlobError { } } +/// A wrapper around a `BlobSidecar` that indicates it has been approved for re-gossiping on +/// the p2p network. +#[derive(Debug)] +pub struct GossipVerifiedBlob { + blob: BlobSidecar, +} + +impl GossipVerifiedBlob { + pub fn to_blob(self) -> BlobSidecar { + self.blob + } +} + pub fn validate_blob_sidecar_for_gossip( blob_sidecar: SignedBlobSidecar, subnet: u64, chain: &BeaconChain, -) -> Result<(), BlobError> { +) -> Result, BlobError> { let blob_slot = blob_sidecar.message.slot; let blob_index = blob_sidecar.message.index; let block_root = blob_sidecar.message.block_root; @@ -240,7 +257,9 @@ pub fn validate_blob_sidecar_for_gossip( }); } - Ok(()) + Ok(GossipVerifiedBlob { + blob: blob_sidecar.message, + }) } pub fn verify_data_availability( @@ -310,7 +329,7 @@ pub struct AvailableBlock { } impl AvailableBlock { - pub fn blobs(&self) -> Option>> { + pub fn blobs(&self) -> Option> { match &self.blobs { VerifiedBlobs::EmptyBlobs | VerifiedBlobs::NotRequired | VerifiedBlobs::PreEip4844 => { None @@ -319,7 +338,7 @@ impl AvailableBlock { } } - pub fn deconstruct(self) -> (Arc>, Option>>) { + pub fn deconstruct(self) -> (Arc>, Option>) { match self.blobs { VerifiedBlobs::EmptyBlobs | VerifiedBlobs::NotRequired | VerifiedBlobs::PreEip4844 => { (self.block, None) @@ -333,7 +352,7 @@ impl AvailableBlock { #[derivative(Hash(bound = "E: EthSpec"))] pub enum VerifiedBlobs { /// These blobs are available. - Available(Arc>), + Available(BlobSidecarArcList), /// This block is from outside the data availability boundary so doesn't require /// a data availability check. NotRequired, diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 4ee15663f1..68c4d1cebc 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -54,6 +54,7 @@ use crate::execution_payload::{ is_optimistic_candidate_block, validate_execution_payload_for_gossip, validate_merge_block, AllowOptimisticImport, NotifyExecutionLayer, PayloadNotifier, }; +use crate::gossip_blob_cache::AvailabilityCheckError; use crate::snapshot_cache::PreProcessingSnapshot; use crate::validator_monitor::HISTORIC_EPOCHS as VALIDATOR_MONITOR_HISTORIC_EPOCHS; use crate::validator_pubkey_cache::ValidatorPubkeyCache; @@ -307,7 +308,7 @@ pub enum BlockError { parent_root: Hash256, }, BlobValidation(BlobError), - AvailabilityPending(Hash256), + AvailabilityCheck(AvailabilityCheckError), } impl From for BlockError { @@ -316,6 +317,12 @@ impl From for BlockError { } } +impl From for BlockError { + fn from(e: AvailabilityCheckError) -> Self { + Self::AvailabilityCheck(e) + } +} + /// Returned when block validation failed due to some issue verifying /// the execution payload. #[derive(Debug)] diff --git a/beacon_node/beacon_chain/src/builder.rs b/beacon_node/beacon_chain/src/builder.rs index 36a6a33796..7da25b207f 100644 --- a/beacon_node/beacon_chain/src/builder.rs +++ b/beacon_node/beacon_chain/src/builder.rs @@ -87,7 +87,6 @@ pub struct BeaconChainBuilder { event_handler: Option>, slot_clock: Option, shutdown_sender: Option>, - block_importer_sender: Option>>, head_tracker: Option, validator_pubkey_cache: Option>, spec: ChainSpec, @@ -130,7 +129,6 @@ where event_handler: None, slot_clock: None, shutdown_sender: None, - block_importer_sender: None, head_tracker: None, validator_pubkey_cache: None, spec: TEthSpec::default_spec(), @@ -562,14 +560,6 @@ where self } - pub fn block_importer_sender( - mut self, - sender: tokio::sync::mpsc::Sender>, - ) -> Self { - self.block_importer_sender = Some(sender); - self - } - /// Creates a new, empty operation pool. fn empty_op_pool(mut self) -> Self { self.op_pool = Some(OperationPool::new()); @@ -653,18 +643,13 @@ where slot_clock.now().ok_or("Unable to read slot")? }; - let (kzg, data_availability_checker) = if let (Some(tx), Some(trusted_setup)) = - (self.block_importer_sender, self.trusted_setup) - { + let kzg = if let Some(trusted_setup) = self.trusted_setup { let kzg = Kzg::new_from_trusted_setup(trusted_setup) .map_err(|e| format!("Failed to load trusted setup: {:?}", e))?; let kzg_arc = Arc::new(kzg); - ( - Some(kzg_arc.clone()), - Some(DataAvailabilityChecker::new(kzg_arc, tx)), - ) + Some(kzg_arc) } else { - (None, None) + None }; let initial_head_block_root = fork_choice @@ -869,7 +854,7 @@ where slasher: self.slasher.clone(), validator_monitor: RwLock::new(validator_monitor), //TODO(sean) should we move kzg solely to the da checker? - data_availability_checker, + data_availability_checker: DataAvailabilityChecker::new(kzg.clone()), proposal_blob_cache: BlobCache::default(), kzg, }; diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 11eeeadedc..2520af41a2 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -6,6 +6,7 @@ use crate::{ use parking_lot::RwLock; use proto_array::Block as ProtoBlock; use std::sync::Arc; +use types::blob_sidecar::BlobSidecarArcList; use types::*; pub struct CacheItem { @@ -21,7 +22,8 @@ pub struct CacheItem { * Values used to make the block available. */ block: Arc>, - blobs: Option>>, + //TODO(sean) remove this and just use the da checker?' + blobs: Option>, proto_block: ProtoBlock, } @@ -160,7 +162,7 @@ impl EarlyAttesterCache { } /// Returns the blobs, if `block_root` matches the cached item. - pub fn get_blobs(&self, block_root: Hash256) -> Option>> { + pub fn get_blobs(&self, block_root: Hash256) -> Option> { self.item .read() .as_ref() diff --git a/beacon_node/beacon_chain/src/gossip_blob_cache.rs b/beacon_node/beacon_chain/src/gossip_blob_cache.rs index ed12970ec6..08fe4a6645 100644 --- a/beacon_node/beacon_chain/src/gossip_blob_cache.rs +++ b/beacon_node/beacon_chain/src/gossip_blob_cache.rs @@ -1,12 +1,14 @@ -use crate::blob_verification::{verify_data_availability, AsBlock}; +use crate::blob_verification::{ + verify_data_availability, AsBlock, AvailableBlock, BlockWrapper, VerifiedBlobs, +}; use crate::block_verification::{ExecutedBlock, IntoExecutionPendingBlock}; use crate::kzg_utils::validate_blob; use crate::{BeaconChain, BeaconChainError, BeaconChainTypes, BlockError}; -use eth2::reqwest::header::Entry; use kzg::Error as KzgError; use kzg::{Kzg, KzgCommitment}; use parking_lot::{Mutex, RwLock}; -use ssz_types::VariableList; +use ssz_types::{Error, VariableList}; +use std::collections::hash_map::Entry; use std::collections::{BTreeMap, HashMap, HashSet}; use std::future::Future; use std::sync::{mpsc, Arc}; @@ -14,10 +16,19 @@ use tokio::sync::mpsc::Sender; use types::blob_sidecar::{BlobIdentifier, BlobSidecar}; use types::{EthSpec, Hash256, SignedBeaconBlock}; -pub enum BlobCacheError { +#[derive(Debug)] +pub enum AvailabilityCheckError { DuplicateBlob(Hash256), Kzg(KzgError), + SszTypes(ssz_types::Error), } + +impl From for AvailabilityCheckError { + fn from(value: Error) -> Self { + Self::SszTypes(value) + } +} + /// This cache contains /// - blobs that have been gossip verified /// - commitments for blocks that have been gossip verified, but the commitments themselves @@ -26,8 +37,13 @@ pub enum BlobCacheError { pub struct DataAvailabilityChecker { rpc_blob_cache: RwLock>>>, gossip_blob_cache: Mutex>>, - kzg: Arc, - tx: Sender>, + kzg: Option>, +} + +pub enum Availability { + PendingBlobs(Vec), + PendingBlock(Hash256), + Available(ExecutedBlock), } struct GossipBlobCache { @@ -36,12 +52,11 @@ struct GossipBlobCache { } impl DataAvailabilityChecker { - pub fn new(kzg: Arc, tx: Sender>) -> Self { + pub fn new(kzg: Option>) -> Self { Self { rpc_blob_cache: <_>::default(), gossip_blob_cache: <_>::default(), kzg, - tx, } } @@ -50,15 +65,25 @@ impl DataAvailabilityChecker { /// cached, verify the block and import it. /// /// This should only accept gossip verified blobs, so we should not have to worry about dupes. - pub fn put_blob(&self, blob: Arc>) -> Result<(), BlobCacheError> { + // return an enum here that may include the full block + pub fn put_blob( + &self, + blob: Arc>, + ) -> Result, AvailabilityCheckError> { + let verified = if let Some(kzg) = self.kzg.as_ref() { + validate_blob::( + kzg, + blob.blob.clone(), + blob.kzg_commitment.clone(), + blob.kzg_proof, + ) + .map_err(|e| AvailabilityCheckError::Kzg(e))? + } else { + false + // error wrong fork + }; + // TODO(remove clones) - let verified = validate_blob::( - &self.kzg, - blob.blob.clone(), - blob.kzg_commitment.clone(), - blob.kzg_proof, - ) - .map_err(|e| BlobCacheError::Kzg(e))?; if verified { let mut blob_cache = self.gossip_blob_cache.lock(); @@ -93,7 +118,6 @@ impl DataAvailabilityChecker { // send to reprocessing queue ? //TODO(sean) try_send? //TODO(sean) errors - self.tx.try_send(executed_block); } else { let _ = inner.executed_block.insert(executed_block); } @@ -110,38 +134,106 @@ impl DataAvailabilityChecker { self.rpc_blob_cache.write().insert(blob.id(), blob.clone()); } - Ok(()) + Ok(Availability::PendingBlobs(vec![])) } - pub fn put_block(&self, executed_block: ExecutedBlock) -> Result<(), BlobCacheError> { - let mut guard = self.gossip_blob_cache.lock(); - guard - .entry(executed_block.block_root) - .and_modify(|cache| { - let block: &SignedBeaconBlock = executed_block.block.as_block(); - if let Ok(block) = block.message_eip4844() { - let verified_commitments: Vec<_> = cache - .verified_blobs - .iter() - .map(|blob| blob.kzg_commitment) - .collect(); - if verified_commitments == block.body.blob_kzg_commitments.clone().to_vec() { - // send to reprocessing queue ? - //TODO(sean) errors - self.tx.try_send(executed_block.clone()); - } else { - let _ = cache.executed_block.insert(executed_block.clone()); - // log that we cached + // return an enum here that may include the full block + pub fn check_block_availability( + &self, + executed_block: ExecutedBlock, + ) -> Result, AvailabilityCheckError> { + let block_clone = executed_block.block.clone(); + + let availability = match block_clone { + BlockWrapper::Available(available_block) => Availability::Available(executed_block), + BlockWrapper::AvailabilityPending(block) => { + if let Ok(kzg_commitments) = block.message().body().blob_kzg_commitments() { + // first check if the blockwrapper contains blobs, if so, use those + + let mut guard = self.gossip_blob_cache.lock(); + let entry = guard.entry(executed_block.block_root); + + match entry { + Entry::Occupied(mut occupied_entry) => { + let cache: &mut GossipBlobCache = occupied_entry.get_mut(); + + let verified_commitments: Vec<_> = cache + .verified_blobs + .iter() + .map(|blob| blob.kzg_commitment) + .collect(); + if verified_commitments == kzg_commitments.clone().to_vec() { + let removed: GossipBlobCache = occupied_entry.remove(); + + let ExecutedBlock { + block: _, + block_root, + state, + parent_block, + parent_eth1_finalization_data, + confirmed_state_roots, + consensus_context, + payload_verification_outcome, + } = executed_block; + + let available_block = BlockWrapper::Available(AvailableBlock { + block, + blobs: VerifiedBlobs::Available(VariableList::new( + removed.verified_blobs, + )?), + }); + + let available_executed = ExecutedBlock { + block: available_block, + block_root, + state, + parent_block, + parent_eth1_finalization_data, + confirmed_state_roots, + consensus_context, + payload_verification_outcome, + }; + Availability::Available(available_executed) + } else { + let mut missing_blobs = Vec::with_capacity(kzg_commitments.len()); + for i in 0..kzg_commitments.len() { + if cache.verified_blobs.get(i).is_none() { + missing_blobs.push(BlobIdentifier { + block_root: executed_block.block_root, + index: i as u64, + }) + } + } + + //TODO(sean) add a check that missing blobs > 0 + + let _ = cache.executed_block.insert(executed_block.clone()); + // log that we cached the block? + Availability::PendingBlobs(missing_blobs) + } + } + Entry::Vacant(vacant_entry) => { + let mut blob_ids = Vec::with_capacity(kzg_commitments.len()); + for i in 0..kzg_commitments.len() { + blob_ids.push(BlobIdentifier { + block_root: executed_block.block_root, + index: i as u64, + }); + } + + vacant_entry.insert(GossipBlobCache { + verified_blobs: vec![], + executed_block: Some(executed_block), + }); + + Availability::PendingBlobs(blob_ids) + } } } else { - // log error + Availability::Available(executed_block) } - }) - .or_insert(GossipBlobCache { - verified_blobs: vec![], - executed_block: Some(executed_block), - }); - - Ok(()) + } + }; + Ok(availability) } } diff --git a/beacon_node/beacon_chain/src/lib.rs b/beacon_node/beacon_chain/src/lib.rs index f1779b45fb..1a0247f99a 100644 --- a/beacon_node/beacon_chain/src/lib.rs +++ b/beacon_node/beacon_chain/src/lib.rs @@ -54,9 +54,10 @@ pub mod validator_monitor; pub mod validator_pubkey_cache; pub use self::beacon_chain::{ - AttestationProcessingOutcome, BeaconChain, BeaconChainTypes, BeaconStore, ChainSegmentResult, - CountUnrealized, ForkChoiceError, OverrideForkchoiceUpdate, ProduceBlockVerification, - StateSkipConfig, WhenSlotSkipped, INVALID_FINALIZED_MERGE_TRANSITION_BLOCK_SHUTDOWN_REASON, + AttestationProcessingOutcome, AvailabilityProcessingStatus, BeaconChain, BeaconChainTypes, + BeaconStore, ChainSegmentResult, CountUnrealized, ForkChoiceError, OverrideForkchoiceUpdate, + ProduceBlockVerification, StateSkipConfig, WhenSlotSkipped, + INVALID_FINALIZED_MERGE_TRANSITION_BLOCK_SHUTDOWN_REASON, INVALID_JUSTIFIED_PAYLOAD_SHUTDOWN_REASON, MAXIMUM_GOSSIP_CLOCK_DISPARITY, }; pub use self::beacon_snapshot::BeaconSnapshot; diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index a784c67411..df2545adf1 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1680,7 +1680,8 @@ where NotifyExecutionLayer::Yes, ) .await? - .into(); + .try_into() + .unwrap(); self.chain.recompute_head_at_current_slot().await; Ok(block_hash) } @@ -1698,7 +1699,8 @@ where NotifyExecutionLayer::Yes, ) .await? - .into(); + .try_into() + .unwrap(); self.chain.recompute_head_at_current_slot().await; Ok(block_hash) } diff --git a/beacon_node/client/src/builder.rs b/beacon_node/client/src/builder.rs index 5d283eae61..41e08bfdca 100644 --- a/beacon_node/client/src/builder.rs +++ b/beacon_node/client/src/builder.rs @@ -874,14 +874,10 @@ where .ok_or("beacon_chain requires a slot clock")?, ) .shutdown_sender(context.executor.shutdown_sender()) - .block_importer_sender(tx) .build() .map_err(|e| format!("Failed to build beacon chain: {}", e))?; - let arc_chain = Arc::new(chain); - arc_chain.start_block_importer(rx); - - self.beacon_chain = Some(arc_chain); + self.beacon_chain = Some(Arc::new(chain)); self.beacon_chain_builder = None; // a beacon chain requires a timer diff --git a/beacon_node/http_api/src/publish_blocks.rs b/beacon_node/http_api/src/publish_blocks.rs index b9db262dd2..7d80649ce8 100644 --- a/beacon_node/http_api/src/publish_blocks.rs +++ b/beacon_node/http_api/src/publish_blocks.rs @@ -1,7 +1,7 @@ use crate::metrics; use beacon_chain::blob_verification::{AsBlock, BlockWrapper, IntoAvailableBlock}; use beacon_chain::validator_monitor::{get_block_delay_ms, timestamp_now}; -use beacon_chain::NotifyExecutionLayer; +use beacon_chain::{AvailabilityProcessingStatus, NotifyExecutionLayer}; use beacon_chain::{BeaconChain, BeaconChainTypes, BlockError, CountUnrealized}; use eth2::types::SignedBlockContents; use lighthouse_network::PubsubMessage; @@ -89,7 +89,7 @@ pub async fn publish_block( ) .await { - Ok(root) => { + Ok(AvailabilityProcessingStatus::Imported(root)) => { info!( log, "Valid block from HTTP API"; @@ -140,6 +140,24 @@ pub async fn publish_block( Ok(()) } + Ok(AvailabilityProcessingStatus::PendingBlock(block_root)) => { + let msg = format!("Missing block with root {:?}", block_root); + error!( + log, + "Invalid block provided to HTTP API"; + "reason" => &msg + ); + Err(warp_utils::reject::broadcast_without_import(msg)) + } + Ok(AvailabilityProcessingStatus::PendingBlobs(blob_ids)) => { + let msg = format!("Missing blobs {:?}", blob_ids); + error!( + log, + "Invalid block provided to HTTP API"; + "reason" => &msg + ); + Err(warp_utils::reject::broadcast_without_import(msg)) + } Err(BlockError::BlockIsAlreadyKnown) => { info!( log, diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index 3d56f53b02..a362efcf3f 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -1,6 +1,6 @@ use crate::{metrics, service::NetworkMessage, sync::SyncMessage}; -use beacon_chain::blob_verification::{AsBlock, BlockWrapper}; +use beacon_chain::blob_verification::{AsBlock, BlockWrapper, GossipVerifiedBlob}; use beacon_chain::store::Error; use beacon_chain::{ attestation_verification::{self, Error as AttnError, VerifiedAttestation}, @@ -9,8 +9,8 @@ use beacon_chain::{ observed_operations::ObservationOutcome, sync_committee_verification::{self, Error as SyncCommitteeError}, validator_monitor::get_block_delay_ms, - BeaconChainError, BeaconChainTypes, BlockError, CountUnrealized, ForkChoiceError, - GossipVerifiedBlock, NotifyExecutionLayer, + AvailabilityProcessingStatus, BeaconChainError, BeaconChainTypes, BlockError, CountUnrealized, + ForkChoiceError, GossipVerifiedBlock, NotifyExecutionLayer, }; use lighthouse_network::{Client, MessageAcceptance, MessageId, PeerAction, PeerId, ReportSource}; use operation_pool::ReceivedPreCapella; @@ -802,8 +802,7 @@ impl Worker { verified_block } - Err(BlockError::AvailabilityPending(_)) => { - //TODO(sean) think about what to do hereA + Err(BlockError::AvailabilityCheck(e)) => { todo!() } Err(BlockError::ParentUnknown(block)) => { @@ -965,6 +964,29 @@ impl Worker { } } + pub async fn process_gossip_verified_blob( + self, + peer_id: PeerId, + verified_blob: GossipVerifiedBlob, + reprocess_tx: mpsc::Sender>, + // This value is not used presently, but it might come in handy for debugging. + _seen_duration: Duration, + ) { + // TODO + match self + .chain + .process_blob(verified_blob.to_blob(), CountUnrealized::True) + .await + { + Ok(hash) => { + // block imported + } + Err(e) => { + // handle errors + } + } + } + /// Process the beacon block that has already passed gossip verification. /// /// Raises a log if there are errors. @@ -989,7 +1011,7 @@ impl Worker { ) .await { - Ok(block_root) => { + Ok(AvailabilityProcessingStatus::Imported(block_root)) => { metrics::inc_counter(&metrics::BEACON_PROCESSOR_GOSSIP_BLOCK_IMPORTED_TOTAL); if reprocess_tx @@ -1016,8 +1038,13 @@ impl Worker { self.chain.recompute_head_at_current_slot().await; } - Err(BlockError::AvailabilityPending(block_root)) => { + Ok(AvailabilityProcessingStatus::PendingBlock(block_root)) => { + // make rpc request for block + todo!() + } + Ok(AvailabilityProcessingStatus::PendingBlobs(blob_ids)) => { // make rpc request for blob + todo!() } Err(BlockError::ParentUnknown(block)) => { // Inform the sync manager to find parents for this block diff --git a/beacon_node/network/src/beacon_processor/worker/sync_methods.rs b/beacon_node/network/src/beacon_processor/worker/sync_methods.rs index 5bbcb4e1f3..08c8200bc8 100644 --- a/beacon_node/network/src/beacon_processor/worker/sync_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/sync_methods.rs @@ -8,7 +8,7 @@ use crate::metrics; use crate::sync::manager::{BlockProcessType, SyncMessage}; use crate::sync::{BatchProcessResult, ChainId}; use beacon_chain::blob_verification::{AsBlock, BlockWrapper, IntoAvailableBlock}; -use beacon_chain::CountUnrealized; +use beacon_chain::{AvailabilityProcessingStatus, CountUnrealized}; use beacon_chain::{ BeaconChainError, BeaconChainTypes, BlockError, ChainSegmentResult, HistoricalBlockError, NotifyExecutionLayer, @@ -100,7 +100,8 @@ impl Worker { metrics::inc_counter(&metrics::BEACON_PROCESSOR_RPC_BLOCK_IMPORTED_TOTAL); // RPC block imported, regardless of process type - if let &Ok(hash) = &result { + //TODO(sean) handle pending availability variants + if let &Ok(AvailabilityProcessingStatus::Imported(hash)) = &result { info!(self.log, "New RPC block received"; "slot" => slot, "hash" => %hash); // Trigger processing for work referencing this block. diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index 43921b585a..b9774ffa81 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -732,7 +732,7 @@ impl SyncManager { self.block_lookups.single_block_lookup_response( id, peer_id, - maybe_block.map(BlockWrapper::Block), + maybe_block.map(BlockWrapper::AvailabilityPending), seen_timestamp, &mut self.network, ) @@ -747,7 +747,7 @@ impl SyncManager { BlockOrBlob::Block(maybe_block) => self.block_lookups.parent_lookup_response( id, peer_id, - maybe_block.map(BlockWrapper::Block), + maybe_block.map(BlockWrapper::AvailabilityPending), seen_timestamp, &mut self.network, ), diff --git a/beacon_node/store/src/hot_cold_store.rs b/beacon_node/store/src/hot_cold_store.rs index 718ff7f722..c833c2d345 100644 --- a/beacon_node/store/src/hot_cold_store.rs +++ b/beacon_node/store/src/hot_cold_store.rs @@ -38,6 +38,7 @@ use std::marker::PhantomData; use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::Duration; +use types::blob_sidecar::BlobSidecarArcList; use types::consts::eip4844::MIN_EPOCHS_FOR_BLOBS_SIDECARS_REQUESTS; use types::*; @@ -66,7 +67,7 @@ pub struct HotColdDB, Cold: ItemStore> { /// The hot database also contains all blocks. pub hot_db: Hot, /// LRU cache of deserialized blobs. Updated whenever a blob is loaded. - blob_cache: Mutex>>, + blob_cache: Mutex>>, /// LRU cache of deserialized blocks. Updated whenever a block is loaded. block_cache: Mutex>>, /// Chain spec. @@ -568,7 +569,11 @@ impl, Cold: ItemStore> HotColdDB blobs_db.key_delete(DBColumn::BeaconBlob.into(), block_root.as_bytes()) } - pub fn put_blobs(&self, block_root: &Hash256, blobs: BlobSidecarList) -> Result<(), Error> { + pub fn put_blobs( + &self, + block_root: &Hash256, + blobs: BlobSidecarArcList, + ) -> Result<(), Error> { let blobs_db = self.blobs_db.as_ref().unwrap_or(&self.cold_db); blobs_db.put_bytes( DBColumn::BeaconBlob.into(), @@ -582,7 +587,7 @@ impl, Cold: ItemStore> HotColdDB pub fn blobs_as_kv_store_ops( &self, key: &Hash256, - blobs: &BlobSidecarList, + blobs: &BlobSidecarArcList, ops: &mut Vec, ) { let db_key = get_key_for_col(DBColumn::BeaconBlob.into(), key.as_bytes()); @@ -926,7 +931,7 @@ impl, Cold: ItemStore> HotColdDB let reverse_op = match op { StoreOp::PutBlobs(block_root, _) => StoreOp::DeleteBlobs(*block_root), StoreOp::DeleteBlobs(block_root) => match blobs_to_delete.pop() { - Some(blobs) => StoreOp::PutBlobs(*block_root, Arc::new(blobs)), + Some(blobs) => StoreOp::PutBlobs(*block_root, blobs), None => return Err(HotColdDBError::Rollback.into()), }, _ => return Err(HotColdDBError::Rollback.into()), @@ -972,7 +977,7 @@ impl, Cold: ItemStore> HotColdDB for op in blob_cache_ops { match op { StoreOp::PutBlobs(block_root, blobs) => { - guard_blob.put(block_root, (*blobs).clone()); + guard_blob.put(block_root, blobs.clone()); } StoreOp::DeleteBlobs(block_root) => { @@ -1320,12 +1325,12 @@ impl, Cold: ItemStore> HotColdDB } /// Fetch a blobs sidecar from the store. - pub fn get_blobs(&self, block_root: &Hash256) -> Result>, Error> { + pub fn get_blobs(&self, block_root: &Hash256) -> Result>, Error> { let blobs_db = self.blobs_db.as_ref().unwrap_or(&self.cold_db); match blobs_db.get_bytes(DBColumn::BeaconBlob.into(), block_root.as_bytes())? { Some(ref blobs_bytes) => { - let blobs = BlobSidecarList::from_ssz_bytes(blobs_bytes)?; + let blobs = BlobSidecarArcList::from_ssz_bytes(blobs_bytes)?; // FIXME(sean) I was attempting to use a blob cache here but was getting deadlocks, // may want to attempt to use one again self.blob_cache.lock().put(*block_root, blobs.clone()); diff --git a/beacon_node/store/src/lib.rs b/beacon_node/store/src/lib.rs index ea382a934e..bcda5b97bf 100644 --- a/beacon_node/store/src/lib.rs +++ b/beacon_node/store/src/lib.rs @@ -43,6 +43,7 @@ pub use metrics::scrape_for_metrics; use parking_lot::MutexGuard; use std::sync::Arc; use strum::{EnumString, IntoStaticStr}; +use types::blob_sidecar::BlobSidecarArcList; pub use types::*; pub type ColumnIter<'a> = Box), Error>> + 'a>; @@ -159,7 +160,7 @@ pub trait ItemStore: KeyValueStore + Sync + Send + Sized + 'stati pub enum StoreOp<'a, E: EthSpec> { PutBlock(Hash256, Arc>), PutState(Hash256, &'a BeaconState), - PutBlobs(Hash256, Arc>), + PutBlobs(Hash256, BlobSidecarArcList), PutOrphanedBlobsKey(Hash256), PutStateSummary(Hash256, HotStateSummary), PutStateTemporaryFlag(Hash256), diff --git a/consensus/types/src/blob_sidecar.rs b/consensus/types/src/blob_sidecar.rs index 502e453aad..fbd0aebb12 100644 --- a/consensus/types/src/blob_sidecar.rs +++ b/consensus/types/src/blob_sidecar.rs @@ -7,6 +7,7 @@ use serde_derive::{Deserialize, Serialize}; use ssz::Encode; use ssz_derive::{Decode, Encode}; use ssz_types::VariableList; +use std::sync::Arc; use test_random_derive::TestRandom; use tree_hash_derive::TreeHash; @@ -48,6 +49,9 @@ pub struct BlobSidecar { } pub type BlobSidecarList = VariableList, ::MaxBlobsPerBlock>; +//TODO(sean) is there any other way around this? need it arc blobs for caching in multiple places +pub type BlobSidecarArcList = + VariableList>, ::MaxBlobsPerBlock>; pub type Blobs = VariableList, ::MaxExtraDataBytes>; impl SignedRoot for BlobSidecar {} diff --git a/consensus/types/src/signed_blob.rs b/consensus/types/src/signed_blob.rs index f9ae481247..6b2279ce89 100644 --- a/consensus/types/src/signed_blob.rs +++ b/consensus/types/src/signed_blob.rs @@ -1,9 +1,14 @@ -use crate::{test_utils::TestRandom, BlobSidecar, EthSpec, Signature}; +use crate::{ + test_utils::TestRandom, BlobSidecar, ChainSpec, Domain, EthSpec, Fork, Hash256, Signature, + SignedRoot, SigningData, +}; +use bls::PublicKey; use derivative::Derivative; use serde_derive::{Deserialize, Serialize}; use ssz_derive::{Decode, Encode}; use ssz_types::VariableList; use test_random_derive::TestRandom; +use tree_hash::TreeHash; use tree_hash_derive::TreeHash; #[derive( @@ -29,3 +34,37 @@ pub struct SignedBlobSidecar { pub type SignedBlobSidecarList = VariableList, ::MaxBlobsPerBlock>; + +impl SignedBlobSidecar { + /// Verify `self.signature`. + /// + /// If the root of `block.message` is already known it can be passed in via `object_root_opt`. + /// Otherwise, it will be computed locally. + pub fn verify_signature( + &self, + object_root_opt: Option, + pubkey: &PublicKey, + fork: &Fork, + genesis_validators_root: Hash256, + spec: &ChainSpec, + ) -> bool { + let domain = spec.get_domain( + self.message.slot.epoch(T::slots_per_epoch()), + Domain::BlobSidecar, + fork, + genesis_validators_root, + ); + + let message = if let Some(object_root) = object_root_opt { + SigningData { + object_root, + domain, + } + .tree_hash_root() + } else { + self.message.signing_root(domain) + }; + + self.signature.verify(pubkey, message) + } +} From acd36ccaa68b95b892e6b5d53cf7642cd6fecc1b Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 17 Mar 2023 21:30:14 +0530 Subject: [PATCH 13/16] Reprocess an ExecutedBlock on unavailable blobs --- Cargo.lock | 3 +- beacon_node/beacon_chain/src/beacon_chain.rs | 12 +- .../beacon_chain/src/block_verification.rs | 6 + beacon_node/network/Cargo.toml | 2 +- .../network/src/beacon_processor/mod.rs | 48 +++++- .../work_reprocessing_queue.rs | 140 +++++++++++++++--- .../beacon_processor/worker/gossip_methods.rs | 131 +++++++++++++++- beacon_node/network/src/sync/manager.rs | 11 ++ 8 files changed, 320 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0e669154f5..e7d51d494d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5094,7 +5094,7 @@ dependencies = [ "task_executor", "tokio", "tokio-stream", - "tokio-util 0.6.10", + "tokio-util 0.7.7", "types", ] @@ -8021,6 +8021,7 @@ dependencies = [ "futures-io", "futures-sink", "pin-project-lite 0.2.9", + "slab", "tokio", "tracing", ] diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 4115b2965b..e92c1ceb26 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -297,11 +297,6 @@ pub enum StateSkipConfig { WithoutStateRoots, } -pub enum BlockProcessingResult { - Verified(Hash256), - AvailabilityPending(ExecutedBlock), -} - pub trait BeaconChainTypes: Send + Sync + 'static { type HotStore: store::ItemStore; type ColdStore: store::ItemStore; @@ -2669,10 +2664,14 @@ impl BeaconChain { /// Returns `Ok(block_root)` if the given `unverified_block` was successfully verified and /// imported into the chain. /// + /// For post deneb blocks, this returns a `BlockError::AvailabilityPending` error + /// if the corresponding blobs are not in the required caches. + /// /// Items that implement `IntoExecutionPendingBlock` include: /// /// - `SignedBeaconBlock` /// - `GossipVerifiedBlock` + /// - `BlockWrapper` /// /// ## Errors /// @@ -2691,7 +2690,6 @@ impl BeaconChain { // Increment the Prometheus counter for block processing requests. metrics::inc_counter(&metrics::BLOCK_PROCESSING_REQUESTS); - let slot = unverified_block.block().slot(); let chain = self.clone(); let execution_pending = unverified_block.into_execution_pending_block( @@ -2818,7 +2816,7 @@ impl BeaconChain { /// /// An error is returned if the block was unable to be imported. It may be partially imported /// (i.e., this function is not atomic). - async fn check_availability_and_maybe_import( + pub async fn check_availability_and_maybe_import( self: &Arc, cache_fn: impl FnOnce(Arc) -> Result, AvailabilityCheckError>, count_unrealized: CountUnrealized, diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 68c4d1cebc..7092576f0b 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -686,6 +686,12 @@ pub struct ExecutedBlock { pub payload_verification_outcome: PayloadVerificationOutcome, } +impl std::fmt::Debug for ExecutedBlock { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{:?}", self.block) + } +} + /// Implemented on types that can be converted into a `ExecutionPendingBlock`. /// /// Used to allow functions to accept blocks at various stages of verification. diff --git a/beacon_node/network/Cargo.toml b/beacon_node/network/Cargo.toml index 95d8a294c1..2b7f17f4b7 100644 --- a/beacon_node/network/Cargo.toml +++ b/beacon_node/network/Cargo.toml @@ -41,7 +41,7 @@ num_cpus = "1.13.0" lru_cache = { path = "../../common/lru_cache" } if-addrs = "0.6.4" strum = "0.24.0" -tokio-util = { version = "0.6.3", features = ["time"] } +tokio-util = { version = "0.7.7", features = ["time"] } derivative = "2.2.0" delay_map = "0.1.1" ethereum-types = { version = "0.14.1", optional = true } diff --git a/beacon_node/network/src/beacon_processor/mod.rs b/beacon_node/network/src/beacon_processor/mod.rs index 410389b119..d725342d30 100644 --- a/beacon_node/network/src/beacon_processor/mod.rs +++ b/beacon_node/network/src/beacon_processor/mod.rs @@ -42,7 +42,9 @@ use crate::sync::manager::BlockProcessType; use crate::{metrics, service::NetworkMessage, sync::SyncMessage}; use beacon_chain::blob_verification::BlockWrapper; use beacon_chain::parking_lot::Mutex; -use beacon_chain::{BeaconChain, BeaconChainTypes, GossipVerifiedBlock, NotifyExecutionLayer}; +use beacon_chain::{ + BeaconChain, BeaconChainTypes, ExecutedBlock, GossipVerifiedBlock, NotifyExecutionLayer, +}; use derivative::Derivative; use futures::stream::{Stream, StreamExt}; use futures::task::Poll; @@ -83,6 +85,8 @@ mod worker; use crate::beacon_processor::work_reprocessing_queue::QueuedGossipBlock; pub use worker::{ChainSegmentProcessId, GossipAggregatePackage, GossipAttestationPackage}; +use self::work_reprocessing_queue::QueuedExecutedBlock; + /// The maximum size of the channel for work events to the `BeaconProcessor`. /// /// Setting this too low will cause consensus messages to be dropped. @@ -219,6 +223,7 @@ pub const GOSSIP_ATTESTATION_BATCH: &str = "gossip_attestation_batch"; pub const GOSSIP_AGGREGATE: &str = "gossip_aggregate"; pub const GOSSIP_AGGREGATE_BATCH: &str = "gossip_aggregate_batch"; pub const GOSSIP_BLOCK: &str = "gossip_block"; +pub const EXECUTED_BLOCK: &str = "executed_block"; pub const GOSSIP_BLOCK_AND_BLOBS_SIDECAR: &str = "gossip_block_and_blobs_sidecar"; pub const DELAYED_IMPORT_BLOCK: &str = "delayed_import_block"; pub const GOSSIP_VOLUNTARY_EXIT: &str = "gossip_voluntary_exit"; @@ -729,7 +734,7 @@ impl WorkEvent { impl std::convert::From> for WorkEvent { fn from(ready_work: ReadyWork) -> Self { match ready_work { - ReadyWork::Block(QueuedGossipBlock { + ReadyWork::GossipBlock(QueuedGossipBlock { peer_id, block, seen_timestamp, @@ -741,6 +746,18 @@ impl std::convert::From> for WorkEvent { seen_timestamp, }, }, + ReadyWork::ExecutedBlock(QueuedExecutedBlock { + peer_id, + block, + seen_timestamp, + }) => Self { + drop_during_sync: false, + work: Work::ExecutedBlock { + peer_id, + block, + seen_timestamp, + }, + }, ReadyWork::RpcBlock(QueuedRpcBlock { block_root, block, @@ -872,6 +889,11 @@ pub enum Work { block: Box>, seen_timestamp: Duration, }, + ExecutedBlock { + peer_id: PeerId, + block: ExecutedBlock, + seen_timestamp: Duration, + }, GossipVoluntaryExit { message_id: MessageId, peer_id: PeerId, @@ -968,6 +990,7 @@ impl Work { Work::GossipAggregate { .. } => GOSSIP_AGGREGATE, Work::GossipAggregateBatch { .. } => GOSSIP_AGGREGATE_BATCH, Work::GossipBlock { .. } => GOSSIP_BLOCK, + Work::ExecutedBlock { .. } => EXECUTED_BLOCK, Work::GossipSignedBlobSidecar { .. } => GOSSIP_BLOCK_AND_BLOBS_SIDECAR, Work::DelayedImportBlock { .. } => DELAYED_IMPORT_BLOCK, Work::GossipVoluntaryExit { .. } => GOSSIP_VOLUNTARY_EXIT, @@ -1127,6 +1150,7 @@ impl BeaconProcessor { FifoQueue::new(MAX_GOSSIP_OPTIMISTIC_UPDATE_REPROCESS_QUEUE_LEN); // Using a FIFO queue since blocks need to be imported sequentially. + let mut executed_block_queue = FifoQueue::new(MAX_RPC_BLOCK_QUEUE_LEN); let mut rpc_block_queue = FifoQueue::new(MAX_RPC_BLOCK_QUEUE_LEN); let mut chain_segment_queue = FifoQueue::new(MAX_CHAIN_SEGMENT_QUEUE_LEN); let mut backfill_chain_segment = FifoQueue::new(MAX_CHAIN_SEGMENT_QUEUE_LEN); @@ -1243,6 +1267,9 @@ impl BeaconProcessor { // on the delayed ones. } else if let Some(item) = delayed_block_queue.pop() { self.spawn_worker(item, toolbox); + // Check availability pending blocks + } else if let Some(item) = executed_block_queue.pop() { + self.spawn_worker(item, toolbox); // Check gossip blocks before gossip attestations, since a block might be // required to verify some attestations. } else if let Some(item) = gossip_block_queue.pop() { @@ -1462,6 +1489,9 @@ impl BeaconProcessor { Work::GossipBlock { .. } => { gossip_block_queue.push(work, work_id, &self.log) } + Work::ExecutedBlock { .. } => { + gossip_block_queue.push(work, work_id, &self.log) + } Work::GossipSignedBlobSidecar { .. } => { gossip_block_and_blobs_sidecar_queue.push(work, work_id, &self.log) } @@ -1742,6 +1772,20 @@ impl BeaconProcessor { ) .await }), + Work::ExecutedBlock { + peer_id, + block, + seen_timestamp, + } => task_spawner.spawn_async(async move { + worker + .process_execution_verified_block( + peer_id, + block, + work_reprocessing_tx, + seen_timestamp, + ) + .await + }), /* * Verification for blobs sidecars received on gossip. */ diff --git a/beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs b/beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs index 4d0bdc0027..82111fa6f1 100644 --- a/beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs +++ b/beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs @@ -14,7 +14,9 @@ use super::MAX_SCHEDULED_WORK_QUEUE_LEN; use crate::metrics; use crate::sync::manager::BlockProcessType; use beacon_chain::blob_verification::{AsBlock, BlockWrapper}; -use beacon_chain::{BeaconChainTypes, GossipVerifiedBlock, MAXIMUM_GOSSIP_CLOCK_DISPARITY}; +use beacon_chain::{ + BeaconChainTypes, ExecutedBlock, GossipVerifiedBlock, MAXIMUM_GOSSIP_CLOCK_DISPARITY, +}; use fnv::FnvHashMap; use futures::task::Poll; use futures::{Stream, StreamExt}; @@ -53,11 +55,19 @@ pub const QUEUED_LIGHT_CLIENT_UPDATE_DELAY: Duration = Duration::from_secs(12); /// For how long to queue rpc blocks before sending them back for reprocessing. pub const QUEUED_RPC_BLOCK_DELAY: Duration = Duration::from_secs(3); +/// For how long to queue executed blocks before sending them back for reprocessing. +pub const QUEUED_EXECUTED_BLOCK_DELAY: Duration = Duration::from_secs(12); + /// Set an arbitrary upper-bound on the number of queued blocks to avoid DoS attacks. The fact that /// we signature-verify blocks before putting them in the queue *should* protect against this, but /// it's nice to have extra protection. const MAXIMUM_QUEUED_BLOCKS: usize = 16; +/// An `ExecutedBlock` contains the entire `BeaconState`, so we shouldn't be storing too many of them +/// to avoid getting DoS'd by the block proposer. +/// TODO(pawan): revise the max blocks +const MAXIMUM_QUEUED_EXECUTED_BLOCKS: usize = 4; + /// How many attestations we keep before new ones get dropped. const MAXIMUM_QUEUED_ATTESTATIONS: usize = 16_384; @@ -77,6 +87,9 @@ pub enum ReprocessQueueMessage { block_root: Hash256, parent_root: Hash256, }, + ExecutedBlock(QueuedExecutedBlock), + /// The blobs corresponding to a `block_root` are now fully available. + BlobsAvailable(Hash256), /// An unaggregated attestation that references an unknown block. UnknownBlockUnaggregate(QueuedUnaggregate), /// An aggregated attestation that references an unknown block. @@ -87,7 +100,8 @@ pub enum ReprocessQueueMessage { /// Events sent by the scheduler once they are ready for re-processing. pub enum ReadyWork { - Block(QueuedGossipBlock), + GossipBlock(QueuedGossipBlock), + ExecutedBlock(QueuedExecutedBlock), RpcBlock(QueuedRpcBlock), Unaggregate(QueuedUnaggregate), Aggregate(QueuedAggregate), @@ -131,6 +145,14 @@ pub struct QueuedGossipBlock { pub seen_timestamp: Duration, } +/// A block that has been fully verified and is pending data availability +/// and import into the beacon chain. +pub struct QueuedExecutedBlock { + pub peer_id: PeerId, + pub block: ExecutedBlock, + pub seen_timestamp: Duration, +} + /// A block that arrived for processing when the same block was being imported over gossip. /// It is queued for later import. pub struct QueuedRpcBlock { @@ -147,6 +169,9 @@ pub struct QueuedRpcBlock { enum InboundEvent { /// A gossip block that was queued for later processing and is ready for import. ReadyGossipBlock(QueuedGossipBlock), + /// An executed block that was queued for blob availability and is now + /// ready for import + ReadyExecutedBlock(QueuedExecutedBlock), /// A rpc block that was queued because the same gossip block was being imported /// will now be retried for import. ReadyRpcBlock(QueuedRpcBlock), @@ -170,6 +195,8 @@ struct ReprocessQueue { /* Queues */ /// Queue to manage scheduled early blocks. gossip_block_delay_queue: DelayQueue>, + /// Queue to manage availability pending blocks. + executed_block_delay_queue: DelayQueue>, /// Queue to manage scheduled early blocks. rpc_block_delay_queue: DelayQueue>, /// Queue to manage scheduled attestations. @@ -180,6 +207,8 @@ struct ReprocessQueue { /* Queued items */ /// Queued blocks. queued_gossip_block_roots: HashSet, + /// Queued availability pending blocks. + queued_executed_block_roots: HashMap, /// Queued aggregated attestations. queued_aggregates: FnvHashMap, DelayKey)>, /// Queued attestations. @@ -233,13 +262,21 @@ impl Stream for ReprocessQueue { // The sequential nature of blockchains means it is generally better to try and import all // existing blocks before new ones. match self.gossip_block_delay_queue.poll_expired(cx) { - Poll::Ready(Some(Ok(queued_block))) => { + Poll::Ready(Some(queued_block)) => { return Poll::Ready(Some(InboundEvent::ReadyGossipBlock( queued_block.into_inner(), ))); } - Poll::Ready(Some(Err(e))) => { - return Poll::Ready(Some(InboundEvent::DelayQueueError(e, "gossip_block_queue"))); + // `Poll::Ready(None)` means that there are no more entries in the delay queue and we + // will continue to get this result until something else is added into the queue. + Poll::Ready(None) | Poll::Pending => (), + } + + match self.executed_block_delay_queue.poll_expired(cx) { + Poll::Ready(Some(queued_block)) => { + return Poll::Ready(Some(InboundEvent::ReadyExecutedBlock( + queued_block.into_inner(), + ))); } // `Poll::Ready(None)` means that there are no more entries in the delay queue and we // will continue to get this result until something else is added into the queue. @@ -247,40 +284,31 @@ impl Stream for ReprocessQueue { } match self.rpc_block_delay_queue.poll_expired(cx) { - Poll::Ready(Some(Ok(queued_block))) => { + Poll::Ready(Some(queued_block)) => { return Poll::Ready(Some(InboundEvent::ReadyRpcBlock(queued_block.into_inner()))); } - Poll::Ready(Some(Err(e))) => { - return Poll::Ready(Some(InboundEvent::DelayQueueError(e, "rpc_block_queue"))); - } // `Poll::Ready(None)` means that there are no more entries in the delay queue and we // will continue to get this result until something else is added into the queue. Poll::Ready(None) | Poll::Pending => (), } match self.attestations_delay_queue.poll_expired(cx) { - Poll::Ready(Some(Ok(attestation_id))) => { + Poll::Ready(Some(attestation_id)) => { return Poll::Ready(Some(InboundEvent::ReadyAttestation( attestation_id.into_inner(), ))); } - Poll::Ready(Some(Err(e))) => { - return Poll::Ready(Some(InboundEvent::DelayQueueError(e, "attestations_queue"))); - } // `Poll::Ready(None)` means that there are no more entries in the delay queue and we // will continue to get this result until something else is added into the queue. Poll::Ready(None) | Poll::Pending => (), } match self.lc_updates_delay_queue.poll_expired(cx) { - Poll::Ready(Some(Ok(lc_id))) => { + Poll::Ready(Some(lc_id)) => { return Poll::Ready(Some(InboundEvent::ReadyLightClientUpdate( lc_id.into_inner(), ))); } - Poll::Ready(Some(Err(e))) => { - return Poll::Ready(Some(InboundEvent::DelayQueueError(e, "lc_updates_queue"))); - } // `Poll::Ready(None)` means that there are no more entries in the delay queue and we // will continue to get this result until something else is added into the queue. Poll::Ready(None) | Poll::Pending => (), @@ -313,10 +341,12 @@ pub fn spawn_reprocess_scheduler( work_reprocessing_rx, ready_work_tx, gossip_block_delay_queue: DelayQueue::new(), + executed_block_delay_queue: DelayQueue::new(), rpc_block_delay_queue: DelayQueue::new(), attestations_delay_queue: DelayQueue::new(), lc_updates_delay_queue: DelayQueue::new(), queued_gossip_block_roots: HashSet::new(), + queued_executed_block_roots: HashMap::new(), queued_lc_updates: FnvHashMap::default(), queued_aggregates: FnvHashMap::default(), queued_unaggregates: FnvHashMap::default(), @@ -400,7 +430,7 @@ impl ReprocessQueue { if block_slot <= now && self .ready_work_tx - .try_send(ReadyWork::Block(early_block)) + .try_send(ReadyWork::GossipBlock(early_block)) .is_err() { error!( @@ -411,6 +441,59 @@ impl ReprocessQueue { } } } + InboundEvent::Msg(ExecutedBlock(executed_block)) => { + if self.executed_block_delay_queue.len() >= MAXIMUM_QUEUED_EXECUTED_BLOCKS { + // TODO(use your own debounce) + if self.rpc_block_debounce.elapsed() { + warn!( + log, + "Executed blocks queue is full"; + "queue_size" => MAXIMUM_QUEUED_EXECUTED_BLOCKS, + "msg" => "check system clock" + ); + } + // TODO(pawan): block would essentially get dropped here + // can the devs do something? + } + // Queue the block for a slot + let block_root = executed_block.block.block_root; + if !self.queued_executed_block_roots.contains_key(&block_root) { + let key = self + .executed_block_delay_queue + .insert(executed_block, QUEUED_EXECUTED_BLOCK_DELAY); + + self.queued_executed_block_roots.insert(block_root, key); + } + } + InboundEvent::Msg(BlobsAvailable(block_root)) => { + match self.queued_executed_block_roots.remove(&block_root) { + None => { + // Log an error to alert that we've made a bad assumption about how this + // program works, but still process the block anyway. + error!( + log, + "Unknown executed block in delay queue"; + "block_root" => ?block_root + ); + } + Some(key) => { + if let Some(executed_block) = + self.executed_block_delay_queue.try_remove(&key) + { + if self + .ready_work_tx + .try_send(ReadyWork::ExecutedBlock(executed_block.into_inner())) + .is_err() + { + error!( + log, + "Failed to pop queued block"; + ); + } + } + } + } + } // A rpc block arrived for processing at the same time when a gossip block // for the same block hash is being imported. We wait for `QUEUED_RPC_BLOCK_DELAY` // and then send the rpc block back for processing assuming the gossip import @@ -664,6 +747,24 @@ impl ReprocessQueue { } } } + InboundEvent::ReadyExecutedBlock(executed_block) => { + let block_root = executed_block.block.block_root; + + if self + .queued_executed_block_roots + .remove(&block_root) + .is_none() + { + // Log an error to alert that we've made a bad assumption about how this + // program works, but still process the block anyway. + error!( + log, + "Unknown block in delay queue"; + "block_root" => ?block_root + ); + } + // TODO(pawan): just dropping the block, rethink what can be done here + } // A block that was queued for later processing is now ready to be processed. InboundEvent::ReadyGossipBlock(ready_block) => { let block_root = ready_block.block.block_root; @@ -680,7 +781,7 @@ impl ReprocessQueue { if self .ready_work_tx - .try_send(ReadyWork::Block(ready_block)) + .try_send(ReadyWork::GossipBlock(ready_block)) .is_err() { error!( @@ -689,6 +790,7 @@ impl ReprocessQueue { ); } } + InboundEvent::DelayQueueError(e, queue_name) => { crit!( log, diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index a362efcf3f..91584ee303 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -2,6 +2,7 @@ use crate::{metrics, service::NetworkMessage, sync::SyncMessage}; use beacon_chain::blob_verification::{AsBlock, BlockWrapper, GossipVerifiedBlob}; use beacon_chain::store::Error; +use beacon_chain::ExecutedBlock; use beacon_chain::{ attestation_verification::{self, Error as AttnError, VerifiedAttestation}, light_client_finality_update_verification::Error as LightClientFinalityUpdateError, @@ -30,8 +31,8 @@ use types::{ use super::{ super::work_reprocessing_queue::{ - QueuedAggregate, QueuedGossipBlock, QueuedLightClientUpdate, QueuedUnaggregate, - ReprocessQueueMessage, + QueuedAggregate, QueuedExecutedBlock, QueuedGossipBlock, QueuedLightClientUpdate, + QueuedUnaggregate, ReprocessQueueMessage, }, Worker, }; @@ -987,6 +988,104 @@ impl Worker { } } + /// Process the beacon block that has already passed gossip verification. + /// + /// Raises a log if there are errors. + pub async fn process_execution_verified_block( + self, + peer_id: PeerId, + executed_block: ExecutedBlock, + reprocess_tx: mpsc::Sender>, + // This value is not used presently, but it might come in handy for debugging. + seen_duration: Duration, + ) { + let block_root = executed_block.block_root; + let block = executed_block.block.block_cloned(); + + match self + .chain + .check_availability_and_maybe_import( + |chain| { + chain + .data_availability_checker + .check_block_availability(executed_block) + }, + CountUnrealized::True, + ) + .await + { + Ok(AvailabilityProcessingStatus::Imported(block_root)) => { + metrics::inc_counter(&metrics::BEACON_PROCESSOR_GOSSIP_BLOCK_IMPORTED_TOTAL); + + if reprocess_tx + .try_send(ReprocessQueueMessage::BlockImported { + block_root, + parent_root: block.message().parent_root(), + }) + .is_err() + { + error!( + self.log, + "Failed to inform block import"; + "source" => "gossip", + "block_root" => ?block_root, + ) + }; + + debug!( + self.log, + "Gossipsub block processed"; + "block" => ?block_root, + "peer_id" => %peer_id + ); + + self.chain.recompute_head_at_current_slot().await; + } + Ok(AvailabilityProcessingStatus::PendingBlobs(_)) + | Ok(AvailabilityProcessingStatus::PendingBlock(_)) + | Err(BlockError::AvailabilityCheck(_)) => { + // TODO(need to do something different if it's unavailble again) + unimplemented!() + } + Err(BlockError::ParentUnknown(block)) => { + // Inform the sync manager to find parents for this block + // This should not occur. It should be checked by `should_forward_block` + error!( + self.log, + "Block with unknown parent attempted to be processed"; + "peer_id" => %peer_id + ); + self.send_sync_message(SyncMessage::UnknownBlock(peer_id, block, block_root)); + } + Err(ref e @ BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => { + debug!( + self.log, + "Failed to verify execution payload"; + "error" => %e + ); + } + other => { + debug!( + self.log, + "Invalid gossip beacon block"; + "outcome" => ?other, + "block root" => ?block_root, + "block slot" => block.slot() + ); + self.gossip_penalize_peer( + peer_id, + PeerAction::MidToleranceError, + "bad_gossip_block_ssz", + ); + trace!( + self.log, + "Invalid gossip beacon block ssz"; + "ssz" => format_args!("0x{}", hex::encode(block.as_ssz_bytes())), + ); + } + }; + } + /// Process the beacon block that has already passed gossip verification. /// /// Raises a log if there are errors. @@ -996,7 +1095,7 @@ impl Worker { verified_block: GossipVerifiedBlock, reprocess_tx: mpsc::Sender>, // This value is not used presently, but it might come in handy for debugging. - _seen_duration: Duration, + seen_duration: Duration, ) { let block = verified_block.block.block_cloned(); let block_root = verified_block.block_root; @@ -1044,6 +1143,32 @@ impl Worker { } Ok(AvailabilityProcessingStatus::PendingBlobs(blob_ids)) => { // make rpc request for blob + // let block_slot = block.block.slot(); + // // Make rpc request for blobs + // self.send_sync_message(SyncMessage::UnknownBlobHash { + // peer_id, + // block_root: block.block_root, + // }); + + // // Send block to reprocessing queue to await blobs + // if reprocess_tx + // .try_send(ReprocessQueueMessage::ExecutedBlock(QueuedExecutedBlock { + // peer_id, + // block, + // seen_timestamp: seen_duration, + // })) + // .is_err() + // { + // error!( + // self.log, + // "Failed to send partially verified block to reprocessing queue"; + // "block_slot" => %block_slot, + // "block_root" => ?block_root, + // "location" => "block gossip" + // ) + // } + } + Err(BlockError::AvailabilityCheck(_)) => { todo!() } Err(BlockError::ParentUnknown(block)) => { diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index b9774ffa81..cc043c6269 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -119,6 +119,14 @@ pub enum SyncMessage { /// manager to attempt to find the block matching the unknown hash. UnknownBlockHash(PeerId, Hash256), + /// A peer has sent us a block that we haven't received all the blobs for. This triggers + /// the manager to attempt to find a blobs for the given block root. + /// TODO: add required blob indices as well. + UnknownBlobHash { + peer_id: PeerId, + block_root: Hash256, + }, + /// A peer has disconnected. Disconnect(PeerId), @@ -598,6 +606,9 @@ impl SyncManager { .search_block(block_hash, peer_id, &mut self.network); } } + SyncMessage::UnknownBlobHash { .. } => { + unimplemented!() + } SyncMessage::Disconnect(peer_id) => { self.peer_disconnect(&peer_id); } From c0445e2536f613bdf8bd7ee64f607dd1d1bac7c7 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Fri, 17 Mar 2023 22:06:45 +0530 Subject: [PATCH 14/16] Add sus gossip verification for blobs --- beacon_node/beacon_chain/src/beacon_chain.rs | 12 +++++- .../beacon_chain/src/blob_verification.rs | 37 +++++++++++++------ .../beacon_chain/src/gossip_blob_cache.rs | 19 +++++++++- .../beacon_processor/worker/gossip_methods.rs | 25 ++++++++++++- 4 files changed, 78 insertions(+), 15 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index e92c1ceb26..088d6d316e 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -8,7 +8,8 @@ use crate::beacon_proposer_cache::compute_proposer_duties_from_head; use crate::beacon_proposer_cache::BeaconProposerCache; use crate::blob_cache::BlobCache; use crate::blob_verification::{ - AsBlock, AvailableBlock, BlobError, BlockWrapper, IntoAvailableBlock, VerifiedBlobs, + self, AsBlock, AvailableBlock, BlobError, BlockWrapper, GossipVerifiedBlobSidecar, + IntoAvailableBlock, VerifiedBlobs, }; use crate::block_times_cache::BlockTimesCache; use crate::block_verification::{ @@ -1898,6 +1899,15 @@ impl BeaconChain { }) } + pub fn verify_blob_sidecar_for_gossip( + self: &Arc, + blob_sidecar: Arc>, + subnet_id: u64, + ) -> Result // TODO(pawan): make a GossipVerifedBlob type + { + blob_verification::validate_blob_sidecar_for_gossip(blob_sidecar, subnet_id, self) + } + /// Accepts some 'LightClientOptimisticUpdate' from the network and attempts to verify it pub fn verify_optimistic_update_for_gossip( self: &Arc, diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index d80fe03f79..7f0512ae67 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -7,6 +7,7 @@ use crate::beacon_chain::{ BeaconChain, BeaconChainTypes, MAXIMUM_GOSSIP_CLOCK_DISPARITY, VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT, }; +use crate::gossip_blob_cache::AvailabilityCheckError; use crate::snapshot_cache::PreProcessingSnapshot; use crate::BeaconChainError; use state_processing::per_block_processing::eip4844::eip4844::verify_kzg_commitments_against_transactions; @@ -112,6 +113,8 @@ pub enum BlobError { /// /// The block is invalid and the peer is faulty. UnknownValidator(u64), + + BlobCacheError(AvailabilityCheckError), } impl From for BlobError { @@ -138,15 +141,23 @@ impl GossipVerifiedBlob { self.blob } } +pub struct GossipVerifiedBlobSidecar { + /// Indicates if all blobs for a given block_root are available + /// in the blob cache. + pub all_blobs_available: bool, + pub block_root: Hash256, + // TODO(pawan): add an Arced blob sidecar which when returned to gossip_methods + // adds the entire thing to the blob cache. +} pub fn validate_blob_sidecar_for_gossip( - blob_sidecar: SignedBlobSidecar, + signed_blob_sidecar: Arc>, subnet: u64, chain: &BeaconChain, -) -> Result, BlobError> { - let blob_slot = blob_sidecar.message.slot; - let blob_index = blob_sidecar.message.index; - let block_root = blob_sidecar.message.block_root; +) -> Result { + let blob_slot = signed_blob_sidecar.message.slot; + let blob_index = signed_blob_sidecar.message.index; + let block_root = signed_blob_sidecar.message.block_root; // Verify that the blob_sidecar was received on the correct subnet. if blob_index != subnet { @@ -185,7 +196,7 @@ pub fn validate_blob_sidecar_for_gossip( // TODO(pawan): should we verify locally that the parent root is correct // or just use whatever the proposer gives us? - let proposer_shuffling_root = blob_sidecar.message.block_parent_root; + let proposer_shuffling_root = signed_blob_sidecar.message.block_parent_root; let (proposer_index, fork) = match chain .beacon_proposer_cache @@ -202,7 +213,7 @@ pub fn validate_blob_sidecar_for_gossip( } }; - let blob_proposer_index = blob_sidecar.message.proposer_index; + let blob_proposer_index = signed_blob_sidecar.message.proposer_index; if proposer_index != blob_proposer_index as usize { return Err(BlobError::ProposerIndexMismatch { sidecar: blob_proposer_index as usize, @@ -221,7 +232,7 @@ pub fn validate_blob_sidecar_for_gossip( .get(proposer_index as usize) .ok_or_else(|| BlobError::UnknownValidator(proposer_index as u64))?; - blob_sidecar.verify_signature( + signed_blob_sidecar.verify_signature( None, pubkey, &fork, @@ -239,7 +250,10 @@ pub fn validate_blob_sidecar_for_gossip( // TODO(pawan): Check if other blobs for the same proposer index and blob index have been // received and drop if required. - // TODO(pawan): potentially add to a seen cache at this point. + let da_checker = &chain.data_availability_checker; + let all_blobs_available = da_checker + .put_blob_temp(signed_blob_sidecar) + .map_err(BlobError::BlobCacheError)?; // Verify if the corresponding block for this blob has been received. // Note: this should be the last gossip check so that we can forward the blob @@ -257,8 +271,9 @@ pub fn validate_blob_sidecar_for_gossip( }); } - Ok(GossipVerifiedBlob { - blob: blob_sidecar.message, + Ok(GossipVerifiedBlobSidecar { + all_blobs_available, + block_root, }) } diff --git a/beacon_node/beacon_chain/src/gossip_blob_cache.rs b/beacon_node/beacon_chain/src/gossip_blob_cache.rs index 08fe4a6645..51e937b3bd 100644 --- a/beacon_node/beacon_chain/src/gossip_blob_cache.rs +++ b/beacon_node/beacon_chain/src/gossip_blob_cache.rs @@ -14,7 +14,7 @@ use std::future::Future; use std::sync::{mpsc, Arc}; use tokio::sync::mpsc::Sender; use types::blob_sidecar::{BlobIdentifier, BlobSidecar}; -use types::{EthSpec, Hash256, SignedBeaconBlock}; +use types::{EthSpec, Hash256, SignedBeaconBlock, SignedBlobSidecar}; #[derive(Debug)] pub enum AvailabilityCheckError { @@ -236,4 +236,21 @@ impl DataAvailabilityChecker { }; Ok(availability) } + + /// Adds the blob to the cache. Returns true if adding the blob completes + /// all the required blob sidecars for a given block root. + /// + /// Note: we can only know this if we know `block.kzg_commitments.len()` + pub fn put_blob_temp( + &self, + blob: Arc>, + ) -> Result { + unimplemented!() + } + + /// Returns all blobs associated with a given block root otherwise returns + /// a UnavailableBlobs error. + pub fn blobs(&self, block_root: Hash256) -> Result, AvailabilityCheckError> { + unimplemented!() + } } diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index 91584ee303..b4c1607e6f 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -658,15 +658,36 @@ impl Worker { peer_client: Client, blob_index: u64, signed_blob: Arc>, + reprocess_tx: mpsc::Sender>, _seen_duration: Duration, ) { + if let Ok(gossip_verified) = self + .chain + .verify_blob_sidecar_for_gossip(signed_blob, blob_index) + { + if gossip_verified.all_blobs_available { + if reprocess_tx + .try_send(ReprocessQueueMessage::BlobsAvailable( + gossip_verified.block_root, + )) + .is_err() + { + { + error!( + self.log, + "Failed to send blob availability message"; + "block_root" => ?gossip_verified.block_root, + "location" => "block gossip" + ) + } + } + } + } // TODO: gossip verification crit!(self.log, "UNIMPLEMENTED gossip blob verification"; "peer_id" => %peer_id, "client" => %peer_client, "blob_topic" => blob_index, - "blob_index" => signed_blob.message.index, - "blob_slot" => signed_blob.message.slot ); } From 3c1687d23ca16e77f498f3919adcb8d9085542d1 Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 20 Mar 2023 21:09:00 +0530 Subject: [PATCH 15/16] Merge stuff --- beacon_node/beacon_chain/src/beacon_chain.rs | 8 +- .../beacon_chain/src/blob_verification.rs | 23 +-- .../network/src/beacon_processor/mod.rs | 5 +- .../beacon_processor/worker/gossip_methods.rs | 133 ++++++++---------- beacon_node/network/src/router/mod.rs | 2 +- beacon_node/network/src/router/processor.rs | 2 +- beacon_node/network/src/sync/manager.rs | 6 +- 7 files changed, 76 insertions(+), 103 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 088d6d316e..5aab4cde9a 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -8,8 +8,8 @@ use crate::beacon_proposer_cache::compute_proposer_duties_from_head; use crate::beacon_proposer_cache::BeaconProposerCache; use crate::blob_cache::BlobCache; use crate::blob_verification::{ - self, AsBlock, AvailableBlock, BlobError, BlockWrapper, GossipVerifiedBlobSidecar, - IntoAvailableBlock, VerifiedBlobs, + self, AsBlock, AvailableBlock, BlobError, BlockWrapper, GossipVerifiedBlob, IntoAvailableBlock, + VerifiedBlobs, }; use crate::block_times_cache::BlockTimesCache; use crate::block_verification::{ @@ -1901,9 +1901,9 @@ impl BeaconChain { pub fn verify_blob_sidecar_for_gossip( self: &Arc, - blob_sidecar: Arc>, + blob_sidecar: SignedBlobSidecar, subnet_id: u64, - ) -> Result // TODO(pawan): make a GossipVerifedBlob type + ) -> Result, BlobError> // TODO(pawan): make a GossipVerifedBlob type { blob_verification::validate_blob_sidecar_for_gossip(blob_sidecar, subnet_id, self) } diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 7f0512ae67..ed082083aa 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/beacon_node/beacon_chain/src/blob_verification.rs @@ -141,20 +141,12 @@ impl GossipVerifiedBlob { self.blob } } -pub struct GossipVerifiedBlobSidecar { - /// Indicates if all blobs for a given block_root are available - /// in the blob cache. - pub all_blobs_available: bool, - pub block_root: Hash256, - // TODO(pawan): add an Arced blob sidecar which when returned to gossip_methods - // adds the entire thing to the blob cache. -} pub fn validate_blob_sidecar_for_gossip( - signed_blob_sidecar: Arc>, + signed_blob_sidecar: SignedBlobSidecar, subnet: u64, chain: &BeaconChain, -) -> Result { +) -> Result, BlobError> { let blob_slot = signed_blob_sidecar.message.slot; let blob_index = signed_blob_sidecar.message.index; let block_root = signed_blob_sidecar.message.block_root; @@ -250,11 +242,6 @@ pub fn validate_blob_sidecar_for_gossip( // TODO(pawan): Check if other blobs for the same proposer index and blob index have been // received and drop if required. - let da_checker = &chain.data_availability_checker; - let all_blobs_available = da_checker - .put_blob_temp(signed_blob_sidecar) - .map_err(BlobError::BlobCacheError)?; - // Verify if the corresponding block for this blob has been received. // Note: this should be the last gossip check so that we can forward the blob // over the gossip network even if we haven't received the corresponding block yet @@ -265,15 +252,15 @@ pub fn validate_blob_sidecar_for_gossip( .get_block(&block_root) .or_else(|| chain.early_attester_cache.get_proto_block(block_root)); // TODO(pawan): should we be checking this cache? + // TODO(pawan): this may be redundant with the new `AvailabilityProcessingStatus::PendingBlock variant` if block_opt.is_none() { return Err(BlobError::UnknownHeadBlock { beacon_block_root: block_root, }); } - Ok(GossipVerifiedBlobSidecar { - all_blobs_available, - block_root, + Ok(GossipVerifiedBlob { + blob: signed_blob_sidecar.message, }) } diff --git a/beacon_node/network/src/beacon_processor/mod.rs b/beacon_node/network/src/beacon_processor/mod.rs index d725342d30..66447845dc 100644 --- a/beacon_node/network/src/beacon_processor/mod.rs +++ b/beacon_node/network/src/beacon_processor/mod.rs @@ -454,7 +454,7 @@ impl WorkEvent { peer_id: PeerId, peer_client: Client, blob_index: u64, - signed_blob: Arc>, + signed_blob: SignedBlobSidecar, seen_timestamp: Duration, ) -> Self { Self { @@ -881,7 +881,7 @@ pub enum Work { peer_id: PeerId, peer_client: Client, blob_index: u64, - signed_blob: Arc>, + signed_blob: SignedBlobSidecar, seen_timestamp: Duration, }, DelayedImportBlock { @@ -1804,6 +1804,7 @@ impl BeaconProcessor { peer_client, blob_index, signed_blob, + work_reprocessing_tx, seen_timestamp, ) .await diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index b4c1607e6f..27b7104d99 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -657,38 +657,62 @@ impl Worker { peer_id: PeerId, peer_client: Client, blob_index: u64, - signed_blob: Arc>, + signed_blob: SignedBlobSidecar, reprocess_tx: mpsc::Sender>, _seen_duration: Duration, ) { - if let Ok(gossip_verified) = self + match self .chain .verify_blob_sidecar_for_gossip(signed_blob, blob_index) { - if gossip_verified.all_blobs_available { - if reprocess_tx - .try_send(ReprocessQueueMessage::BlobsAvailable( - gossip_verified.block_root, - )) - .is_err() - { - { - error!( - self.log, - "Failed to send blob availability message"; - "block_root" => ?gossip_verified.block_root, - "location" => "block gossip" - ) - } - } + Ok(gossip_verified_blob) => { + self.process_gossip_verified_blob( + peer_id, + gossip_verified_blob, + reprocess_tx, + _seen_duration, + ) + .await + } + Err(_) => { + // TODO(pawan): handle all blob errors for peer scoring + todo!() + } + } + } + + pub async fn process_gossip_verified_blob( + self, + peer_id: PeerId, + verified_blob: GossipVerifiedBlob, + reprocess_tx: mpsc::Sender>, + // This value is not used presently, but it might come in handy for debugging. + _seen_duration: Duration, + ) { + // TODO + match self + .chain + .process_blob(verified_blob.to_blob(), CountUnrealized::True) + .await + { + Ok(AvailabilityProcessingStatus::Imported(hash)) => { + todo!() + // add to metrics + // logging + } + Ok(AvailabilityProcessingStatus::PendingBlobs(pending_blobs)) => self + .send_sync_message(SyncMessage::UnknownBlobHash { + peer_id, + pending_blobs, + }), + Ok(AvailabilityProcessingStatus::PendingBlock(block_hash)) => { + self.send_sync_message(SyncMessage::UnknownBlockHash(peer_id, block_hash)); + } + Err(e) => { + // handle errors + todo!() } } - // TODO: gossip verification - crit!(self.log, "UNIMPLEMENTED gossip blob verification"; - "peer_id" => %peer_id, - "client" => %peer_client, - "blob_topic" => blob_index, - ); } /// Process the beacon block received from the gossip network and: @@ -986,29 +1010,6 @@ impl Worker { } } - pub async fn process_gossip_verified_blob( - self, - peer_id: PeerId, - verified_blob: GossipVerifiedBlob, - reprocess_tx: mpsc::Sender>, - // This value is not used presently, but it might come in handy for debugging. - _seen_duration: Duration, - ) { - // TODO - match self - .chain - .process_blob(verified_blob.to_blob(), CountUnrealized::True) - .await - { - Ok(hash) => { - // block imported - } - Err(e) => { - // handle errors - } - } - } - /// Process the beacon block that has already passed gossip verification. /// /// Raises a log if there are errors. @@ -1159,35 +1160,19 @@ impl Worker { self.chain.recompute_head_at_current_slot().await; } Ok(AvailabilityProcessingStatus::PendingBlock(block_root)) => { - // make rpc request for block - todo!() + // This error variant doesn't make any sense in this context + crit!( + self.log, + "Internal error. Cannot get AvailabilityProcessingStatus::PendingBlock on processing block"; + "block_root" => %block_root + ); } - Ok(AvailabilityProcessingStatus::PendingBlobs(blob_ids)) => { + Ok(AvailabilityProcessingStatus::PendingBlobs(pending_blobs)) => { // make rpc request for blob - // let block_slot = block.block.slot(); - // // Make rpc request for blobs - // self.send_sync_message(SyncMessage::UnknownBlobHash { - // peer_id, - // block_root: block.block_root, - // }); - - // // Send block to reprocessing queue to await blobs - // if reprocess_tx - // .try_send(ReprocessQueueMessage::ExecutedBlock(QueuedExecutedBlock { - // peer_id, - // block, - // seen_timestamp: seen_duration, - // })) - // .is_err() - // { - // error!( - // self.log, - // "Failed to send partially verified block to reprocessing queue"; - // "block_slot" => %block_slot, - // "block_root" => ?block_root, - // "location" => "block gossip" - // ) - // } + self.send_sync_message(SyncMessage::UnknownBlobHash { + peer_id, + pending_blobs, + }); } Err(BlockError::AvailabilityCheck(_)) => { todo!() diff --git a/beacon_node/network/src/router/mod.rs b/beacon_node/network/src/router/mod.rs index a18ce4e7c8..067d84044f 100644 --- a/beacon_node/network/src/router/mod.rs +++ b/beacon_node/network/src/router/mod.rs @@ -257,7 +257,7 @@ impl Router { peer_id, self.network_globals.client(&peer_id), blob_index, - Arc::new(signed_blob), + signed_blob, ); } PubsubMessage::VoluntaryExit(exit) => { diff --git a/beacon_node/network/src/router/processor.rs b/beacon_node/network/src/router/processor.rs index 76962b373f..281d0ef616 100644 --- a/beacon_node/network/src/router/processor.rs +++ b/beacon_node/network/src/router/processor.rs @@ -365,7 +365,7 @@ impl Processor { peer_id: PeerId, peer_client: Client, blob_index: u64, // TODO: add a type for the blob index - signed_blob: Arc>, + signed_blob: SignedBlobSidecar, ) { self.send_beacon_processor_work(BeaconWorkEvent::gossip_signed_blob_sidecar( message_id, diff --git a/beacon_node/network/src/sync/manager.rs b/beacon_node/network/src/sync/manager.rs index cc043c6269..ad7cad3216 100644 --- a/beacon_node/network/src/sync/manager.rs +++ b/beacon_node/network/src/sync/manager.rs @@ -56,6 +56,7 @@ use std::ops::Sub; use std::sync::Arc; use std::time::Duration; use tokio::sync::mpsc; +use types::blob_sidecar::BlobIdentifier; use types::{BlobSidecar, EthSpec, Hash256, SignedBeaconBlock, Slot}; /// The number of slots ahead of us that is allowed before requesting a long-range (batch) Sync @@ -120,11 +121,10 @@ pub enum SyncMessage { UnknownBlockHash(PeerId, Hash256), /// A peer has sent us a block that we haven't received all the blobs for. This triggers - /// the manager to attempt to find a blobs for the given block root. - /// TODO: add required blob indices as well. + /// the manager to attempt to find the pending blobs for the given block root. UnknownBlobHash { peer_id: PeerId, - block_root: Hash256, + pending_blobs: Vec, }, /// A peer has disconnected. From 463ce3d1fe2e68df267ff207785386f4568df0eb Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Mon, 20 Mar 2023 21:21:45 +0530 Subject: [PATCH 16/16] Remove reprocessing cache stuff --- .../network/src/beacon_processor/mod.rs | 46 +------ .../work_reprocessing_queue.rs | 128 +----------------- .../beacon_processor/worker/gossip_methods.rs | 5 +- 3 files changed, 5 insertions(+), 174 deletions(-) diff --git a/beacon_node/network/src/beacon_processor/mod.rs b/beacon_node/network/src/beacon_processor/mod.rs index 66447845dc..240bf24285 100644 --- a/beacon_node/network/src/beacon_processor/mod.rs +++ b/beacon_node/network/src/beacon_processor/mod.rs @@ -42,9 +42,7 @@ use crate::sync::manager::BlockProcessType; use crate::{metrics, service::NetworkMessage, sync::SyncMessage}; use beacon_chain::blob_verification::BlockWrapper; use beacon_chain::parking_lot::Mutex; -use beacon_chain::{ - BeaconChain, BeaconChainTypes, ExecutedBlock, GossipVerifiedBlock, NotifyExecutionLayer, -}; +use beacon_chain::{BeaconChain, BeaconChainTypes, GossipVerifiedBlock, NotifyExecutionLayer}; use derivative::Derivative; use futures::stream::{Stream, StreamExt}; use futures::task::Poll; @@ -85,8 +83,6 @@ mod worker; use crate::beacon_processor::work_reprocessing_queue::QueuedGossipBlock; pub use worker::{ChainSegmentProcessId, GossipAggregatePackage, GossipAttestationPackage}; -use self::work_reprocessing_queue::QueuedExecutedBlock; - /// The maximum size of the channel for work events to the `BeaconProcessor`. /// /// Setting this too low will cause consensus messages to be dropped. @@ -223,7 +219,6 @@ pub const GOSSIP_ATTESTATION_BATCH: &str = "gossip_attestation_batch"; pub const GOSSIP_AGGREGATE: &str = "gossip_aggregate"; pub const GOSSIP_AGGREGATE_BATCH: &str = "gossip_aggregate_batch"; pub const GOSSIP_BLOCK: &str = "gossip_block"; -pub const EXECUTED_BLOCK: &str = "executed_block"; pub const GOSSIP_BLOCK_AND_BLOBS_SIDECAR: &str = "gossip_block_and_blobs_sidecar"; pub const DELAYED_IMPORT_BLOCK: &str = "delayed_import_block"; pub const GOSSIP_VOLUNTARY_EXIT: &str = "gossip_voluntary_exit"; @@ -746,18 +741,6 @@ impl std::convert::From> for WorkEvent { seen_timestamp, }, }, - ReadyWork::ExecutedBlock(QueuedExecutedBlock { - peer_id, - block, - seen_timestamp, - }) => Self { - drop_during_sync: false, - work: Work::ExecutedBlock { - peer_id, - block, - seen_timestamp, - }, - }, ReadyWork::RpcBlock(QueuedRpcBlock { block_root, block, @@ -889,11 +872,6 @@ pub enum Work { block: Box>, seen_timestamp: Duration, }, - ExecutedBlock { - peer_id: PeerId, - block: ExecutedBlock, - seen_timestamp: Duration, - }, GossipVoluntaryExit { message_id: MessageId, peer_id: PeerId, @@ -990,7 +968,6 @@ impl Work { Work::GossipAggregate { .. } => GOSSIP_AGGREGATE, Work::GossipAggregateBatch { .. } => GOSSIP_AGGREGATE_BATCH, Work::GossipBlock { .. } => GOSSIP_BLOCK, - Work::ExecutedBlock { .. } => EXECUTED_BLOCK, Work::GossipSignedBlobSidecar { .. } => GOSSIP_BLOCK_AND_BLOBS_SIDECAR, Work::DelayedImportBlock { .. } => DELAYED_IMPORT_BLOCK, Work::GossipVoluntaryExit { .. } => GOSSIP_VOLUNTARY_EXIT, @@ -1150,7 +1127,6 @@ impl BeaconProcessor { FifoQueue::new(MAX_GOSSIP_OPTIMISTIC_UPDATE_REPROCESS_QUEUE_LEN); // Using a FIFO queue since blocks need to be imported sequentially. - let mut executed_block_queue = FifoQueue::new(MAX_RPC_BLOCK_QUEUE_LEN); let mut rpc_block_queue = FifoQueue::new(MAX_RPC_BLOCK_QUEUE_LEN); let mut chain_segment_queue = FifoQueue::new(MAX_CHAIN_SEGMENT_QUEUE_LEN); let mut backfill_chain_segment = FifoQueue::new(MAX_CHAIN_SEGMENT_QUEUE_LEN); @@ -1267,9 +1243,6 @@ impl BeaconProcessor { // on the delayed ones. } else if let Some(item) = delayed_block_queue.pop() { self.spawn_worker(item, toolbox); - // Check availability pending blocks - } else if let Some(item) = executed_block_queue.pop() { - self.spawn_worker(item, toolbox); // Check gossip blocks before gossip attestations, since a block might be // required to verify some attestations. } else if let Some(item) = gossip_block_queue.pop() { @@ -1489,9 +1462,6 @@ impl BeaconProcessor { Work::GossipBlock { .. } => { gossip_block_queue.push(work, work_id, &self.log) } - Work::ExecutedBlock { .. } => { - gossip_block_queue.push(work, work_id, &self.log) - } Work::GossipSignedBlobSidecar { .. } => { gossip_block_and_blobs_sidecar_queue.push(work, work_id, &self.log) } @@ -1772,20 +1742,6 @@ impl BeaconProcessor { ) .await }), - Work::ExecutedBlock { - peer_id, - block, - seen_timestamp, - } => task_spawner.spawn_async(async move { - worker - .process_execution_verified_block( - peer_id, - block, - work_reprocessing_tx, - seen_timestamp, - ) - .await - }), /* * Verification for blobs sidecars received on gossip. */ diff --git a/beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs b/beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs index 82111fa6f1..e344a61132 100644 --- a/beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs +++ b/beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs @@ -14,15 +14,13 @@ use super::MAX_SCHEDULED_WORK_QUEUE_LEN; use crate::metrics; use crate::sync::manager::BlockProcessType; use beacon_chain::blob_verification::{AsBlock, BlockWrapper}; -use beacon_chain::{ - BeaconChainTypes, ExecutedBlock, GossipVerifiedBlock, MAXIMUM_GOSSIP_CLOCK_DISPARITY, -}; +use beacon_chain::{BeaconChainTypes, GossipVerifiedBlock, MAXIMUM_GOSSIP_CLOCK_DISPARITY}; use fnv::FnvHashMap; use futures::task::Poll; use futures::{Stream, StreamExt}; use lighthouse_network::{MessageId, PeerId}; use logging::TimeLatch; -use slog::{crit, debug, error, trace, warn, Logger}; +use slog::{debug, error, trace, warn, Logger}; use slot_clock::SlotClock; use std::collections::{HashMap, HashSet}; use std::pin::Pin; @@ -30,7 +28,6 @@ use std::task::Context; use std::time::Duration; use task_executor::TaskExecutor; use tokio::sync::mpsc::{self, Receiver, Sender}; -use tokio::time::error::Error as TimeError; use tokio_util::time::delay_queue::{DelayQueue, Key as DelayKey}; use types::{ Attestation, EthSpec, Hash256, LightClientOptimisticUpdate, SignedAggregateAndProof, SubnetId, @@ -55,19 +52,11 @@ pub const QUEUED_LIGHT_CLIENT_UPDATE_DELAY: Duration = Duration::from_secs(12); /// For how long to queue rpc blocks before sending them back for reprocessing. pub const QUEUED_RPC_BLOCK_DELAY: Duration = Duration::from_secs(3); -/// For how long to queue executed blocks before sending them back for reprocessing. -pub const QUEUED_EXECUTED_BLOCK_DELAY: Duration = Duration::from_secs(12); - /// Set an arbitrary upper-bound on the number of queued blocks to avoid DoS attacks. The fact that /// we signature-verify blocks before putting them in the queue *should* protect against this, but /// it's nice to have extra protection. const MAXIMUM_QUEUED_BLOCKS: usize = 16; -/// An `ExecutedBlock` contains the entire `BeaconState`, so we shouldn't be storing too many of them -/// to avoid getting DoS'd by the block proposer. -/// TODO(pawan): revise the max blocks -const MAXIMUM_QUEUED_EXECUTED_BLOCKS: usize = 4; - /// How many attestations we keep before new ones get dropped. const MAXIMUM_QUEUED_ATTESTATIONS: usize = 16_384; @@ -87,9 +76,6 @@ pub enum ReprocessQueueMessage { block_root: Hash256, parent_root: Hash256, }, - ExecutedBlock(QueuedExecutedBlock), - /// The blobs corresponding to a `block_root` are now fully available. - BlobsAvailable(Hash256), /// An unaggregated attestation that references an unknown block. UnknownBlockUnaggregate(QueuedUnaggregate), /// An aggregated attestation that references an unknown block. @@ -101,7 +87,6 @@ pub enum ReprocessQueueMessage { /// Events sent by the scheduler once they are ready for re-processing. pub enum ReadyWork { GossipBlock(QueuedGossipBlock), - ExecutedBlock(QueuedExecutedBlock), RpcBlock(QueuedRpcBlock), Unaggregate(QueuedUnaggregate), Aggregate(QueuedAggregate), @@ -145,14 +130,6 @@ pub struct QueuedGossipBlock { pub seen_timestamp: Duration, } -/// A block that has been fully verified and is pending data availability -/// and import into the beacon chain. -pub struct QueuedExecutedBlock { - pub peer_id: PeerId, - pub block: ExecutedBlock, - pub seen_timestamp: Duration, -} - /// A block that arrived for processing when the same block was being imported over gossip. /// It is queued for later import. pub struct QueuedRpcBlock { @@ -169,9 +146,6 @@ pub struct QueuedRpcBlock { enum InboundEvent { /// A gossip block that was queued for later processing and is ready for import. ReadyGossipBlock(QueuedGossipBlock), - /// An executed block that was queued for blob availability and is now - /// ready for import - ReadyExecutedBlock(QueuedExecutedBlock), /// A rpc block that was queued because the same gossip block was being imported /// will now be retried for import. ReadyRpcBlock(QueuedRpcBlock), @@ -179,8 +153,6 @@ enum InboundEvent { ReadyAttestation(QueuedAttestationId), /// A light client update that is ready for re-processing. ReadyLightClientUpdate(QueuedLightClientUpdateId), - /// A `DelayQueue` returned an error. - DelayQueueError(TimeError, &'static str), /// A message sent to the `ReprocessQueue` Msg(ReprocessQueueMessage), } @@ -195,8 +167,6 @@ struct ReprocessQueue { /* Queues */ /// Queue to manage scheduled early blocks. gossip_block_delay_queue: DelayQueue>, - /// Queue to manage availability pending blocks. - executed_block_delay_queue: DelayQueue>, /// Queue to manage scheduled early blocks. rpc_block_delay_queue: DelayQueue>, /// Queue to manage scheduled attestations. @@ -207,8 +177,6 @@ struct ReprocessQueue { /* Queued items */ /// Queued blocks. queued_gossip_block_roots: HashSet, - /// Queued availability pending blocks. - queued_executed_block_roots: HashMap, /// Queued aggregated attestations. queued_aggregates: FnvHashMap, DelayKey)>, /// Queued attestations. @@ -272,17 +240,6 @@ impl Stream for ReprocessQueue { Poll::Ready(None) | Poll::Pending => (), } - match self.executed_block_delay_queue.poll_expired(cx) { - Poll::Ready(Some(queued_block)) => { - return Poll::Ready(Some(InboundEvent::ReadyExecutedBlock( - queued_block.into_inner(), - ))); - } - // `Poll::Ready(None)` means that there are no more entries in the delay queue and we - // will continue to get this result until something else is added into the queue. - Poll::Ready(None) | Poll::Pending => (), - } - match self.rpc_block_delay_queue.poll_expired(cx) { Poll::Ready(Some(queued_block)) => { return Poll::Ready(Some(InboundEvent::ReadyRpcBlock(queued_block.into_inner()))); @@ -341,12 +298,10 @@ pub fn spawn_reprocess_scheduler( work_reprocessing_rx, ready_work_tx, gossip_block_delay_queue: DelayQueue::new(), - executed_block_delay_queue: DelayQueue::new(), rpc_block_delay_queue: DelayQueue::new(), attestations_delay_queue: DelayQueue::new(), lc_updates_delay_queue: DelayQueue::new(), queued_gossip_block_roots: HashSet::new(), - queued_executed_block_roots: HashMap::new(), queued_lc_updates: FnvHashMap::default(), queued_aggregates: FnvHashMap::default(), queued_unaggregates: FnvHashMap::default(), @@ -441,59 +396,6 @@ impl ReprocessQueue { } } } - InboundEvent::Msg(ExecutedBlock(executed_block)) => { - if self.executed_block_delay_queue.len() >= MAXIMUM_QUEUED_EXECUTED_BLOCKS { - // TODO(use your own debounce) - if self.rpc_block_debounce.elapsed() { - warn!( - log, - "Executed blocks queue is full"; - "queue_size" => MAXIMUM_QUEUED_EXECUTED_BLOCKS, - "msg" => "check system clock" - ); - } - // TODO(pawan): block would essentially get dropped here - // can the devs do something? - } - // Queue the block for a slot - let block_root = executed_block.block.block_root; - if !self.queued_executed_block_roots.contains_key(&block_root) { - let key = self - .executed_block_delay_queue - .insert(executed_block, QUEUED_EXECUTED_BLOCK_DELAY); - - self.queued_executed_block_roots.insert(block_root, key); - } - } - InboundEvent::Msg(BlobsAvailable(block_root)) => { - match self.queued_executed_block_roots.remove(&block_root) { - None => { - // Log an error to alert that we've made a bad assumption about how this - // program works, but still process the block anyway. - error!( - log, - "Unknown executed block in delay queue"; - "block_root" => ?block_root - ); - } - Some(key) => { - if let Some(executed_block) = - self.executed_block_delay_queue.try_remove(&key) - { - if self - .ready_work_tx - .try_send(ReadyWork::ExecutedBlock(executed_block.into_inner())) - .is_err() - { - error!( - log, - "Failed to pop queued block"; - ); - } - } - } - } - } // A rpc block arrived for processing at the same time when a gossip block // for the same block hash is being imported. We wait for `QUEUED_RPC_BLOCK_DELAY` // and then send the rpc block back for processing assuming the gossip import @@ -747,24 +649,6 @@ impl ReprocessQueue { } } } - InboundEvent::ReadyExecutedBlock(executed_block) => { - let block_root = executed_block.block.block_root; - - if self - .queued_executed_block_roots - .remove(&block_root) - .is_none() - { - // Log an error to alert that we've made a bad assumption about how this - // program works, but still process the block anyway. - error!( - log, - "Unknown block in delay queue"; - "block_root" => ?block_root - ); - } - // TODO(pawan): just dropping the block, rethink what can be done here - } // A block that was queued for later processing is now ready to be processed. InboundEvent::ReadyGossipBlock(ready_block) => { let block_root = ready_block.block.block_root; @@ -791,14 +675,6 @@ impl ReprocessQueue { } } - InboundEvent::DelayQueueError(e, queue_name) => { - crit!( - log, - "Failed to poll queue"; - "queue" => queue_name, - "e" => ?e - ) - } InboundEvent::ReadyAttestation(queued_id) => { metrics::inc_counter( &metrics::BEACON_PROCESSOR_REPROCESSING_QUEUE_EXPIRED_ATTESTATIONS, diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index 27b7104d99..f60843ab9b 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -18,7 +18,6 @@ use operation_pool::ReceivedPreCapella; use slog::{crit, debug, error, info, trace, warn}; use slot_clock::SlotClock; use ssz::Encode; -use std::sync::Arc; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use store::hot_cold_store::HotColdDBError; use tokio::sync::mpsc; @@ -31,8 +30,8 @@ use types::{ use super::{ super::work_reprocessing_queue::{ - QueuedAggregate, QueuedExecutedBlock, QueuedGossipBlock, QueuedLightClientUpdate, - QueuedUnaggregate, ReprocessQueueMessage, + QueuedAggregate, QueuedGossipBlock, QueuedLightClientUpdate, QueuedUnaggregate, + ReprocessQueueMessage, }, Worker, };