Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Deduplicate block root computation #3590

Closed
wants to merge 7 commits into from
Closed

[Merged by Bors] - Deduplicate block root computation #3590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2e153c3
Avoid root in load_parent
paulhauner Sep 20, 2022
3a812b0
Avoid roots in gossip and sync methods
paulhauner Sep 20, 2022
f3be7af
Remove misc networking roots
paulhauner Sep 20, 2022
469fbb2
Thread block root through RPC block flow
paulhauner Sep 20, 2022
5039e57
Thread block root into block processing
paulhauner Sep 20, 2022
35c812c
Fix compile errors in tests
paulhauner Sep 20, 2022
a5cce2f
Maintain metrics for RPC blocks
paulhauner Sep 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions beacon_node/beacon_chain/src/beacon_chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2220,7 +2220,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
}
}

match check_block_relevancy(&block, Some(block_root), self) {
match check_block_relevancy(&block, block_root, self) {
// If the block is relevant, add it to the filtered chain segment.
Ok(_) => filtered_chain_segment.push((block_root, block)),
// If the block is already known, simply ignore this block.
Expand Down Expand Up @@ -2344,7 +2344,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
// Import the blocks into the chain.
for signature_verified_block in signature_verified_blocks {
match self
.process_block(signature_verified_block, count_unrealized)
.process_block(
signature_verified_block.block_root(),
signature_verified_block,
count_unrealized,
)
.await
{
Ok(_) => imported_blocks += 1,
Expand Down Expand Up @@ -2429,6 +2433,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
/// verification.
pub async fn process_block<B: IntoExecutionPendingBlock<T>>(
self: &Arc<Self>,
block_root: Hash256,
unverified_block: B,
count_unrealized: CountUnrealized,
) -> Result<Hash256, BlockError<T::EthSpec>> {
Expand All @@ -2444,7 +2449,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
// A small closure to group the verification and import errors.
let chain = self.clone();
let import_block = async move {
let execution_pending = unverified_block.into_execution_pending_block(&chain)?;
let execution_pending =
unverified_block.into_execution_pending_block(block_root, &chain)?;
chain
.import_execution_pending_block(execution_pending, count_unrealized)
.await
Expand Down
45 changes: 24 additions & 21 deletions beacon_node/beacon_chain/src/block_verification.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ pub fn signature_verify_chain_segment<T: BeaconChainTypes>(
}

let (first_root, first_block) = chain_segment.remove(0);
let (mut parent, first_block) = load_parent(first_block, chain)?;
let (mut parent, first_block) = load_parent(first_root, first_block, chain)?;
let slot = first_block.slot();
chain_segment.insert(0, (first_root, first_block));

Expand Down Expand Up @@ -622,9 +622,10 @@ pub struct ExecutionPendingBlock<T: BeaconChainTypes> {
pub trait IntoExecutionPendingBlock<T: BeaconChainTypes>: Sized {
fn into_execution_pending_block(
self,
block_root: Hash256,
chain: &Arc<BeaconChain<T>>,
) -> Result<ExecutionPendingBlock<T>, BlockError<T::EthSpec>> {
self.into_execution_pending_block_slashable(chain)
self.into_execution_pending_block_slashable(block_root, chain)
.map(|execution_pending| {
// Supply valid block to slasher.
if let Some(slasher) = chain.slasher.as_ref() {
Expand All @@ -638,6 +639,7 @@ pub trait IntoExecutionPendingBlock<T: BeaconChainTypes>: Sized {
/// Convert the block to fully-verified form while producing data to aid checking slashability.
fn into_execution_pending_block_slashable(
self,
block_root: Hash256,
chain: &Arc<BeaconChain<T>>,
) -> Result<ExecutionPendingBlock<T>, BlockSlashInfo<BlockError<T::EthSpec>>>;

Expand Down Expand Up @@ -781,7 +783,7 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
} else {
// The proposer index was *not* cached and we must load the parent in order to determine
// the proposer index.
let (mut parent, block) = load_parent(block, chain)?;
let (mut parent, block) = load_parent(block_root, block, chain)?;

debug!(
chain.log,
Expand Down Expand Up @@ -877,11 +879,12 @@ impl<T: BeaconChainTypes> IntoExecutionPendingBlock<T> for GossipVerifiedBlock<T
/// Completes verification of the wrapped `block`.
fn into_execution_pending_block_slashable(
self,
block_root: Hash256,
chain: &Arc<BeaconChain<T>>,
) -> Result<ExecutionPendingBlock<T>, BlockSlashInfo<BlockError<T::EthSpec>>> {
let execution_pending =
SignatureVerifiedBlock::from_gossip_verified_block_check_slashable(self, chain)?;
execution_pending.into_execution_pending_block_slashable(chain)
execution_pending.into_execution_pending_block_slashable(block_root, chain)
}

fn block(&self) -> &SignedBeaconBlock<T::EthSpec> {
Expand All @@ -907,7 +910,7 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
// Check the anchor slot before loading the parent, to avoid spurious lookups.
check_block_against_anchor_slot(block.message(), chain)?;

let (mut parent, block) = load_parent(block, chain)?;
let (mut parent, block) = load_parent(block_root, block, chain)?;

// Reject any block that exceeds our limit on skipped slots.
check_block_skip_slots(chain, parent.beacon_block.slot(), block.message())?;
Expand Down Expand Up @@ -955,7 +958,7 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
let (mut parent, block) = if let Some(parent) = from.parent {
(parent, from.block)
} else {
load_parent(from.block, chain)?
load_parent(from.block_root, from.block, chain)?
};

let state = cheap_state_advance_to_obtain_committees(
Expand Down Expand Up @@ -991,29 +994,29 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
Self::from_gossip_verified_block(from, chain)
.map_err(|e| BlockSlashInfo::from_early_error(header, e))
}

pub fn block_root(&self) -> Hash256 {
self.block_root
}
}

impl<T: BeaconChainTypes> IntoExecutionPendingBlock<T> for SignatureVerifiedBlock<T> {
/// Completes verification of the wrapped `block`.
fn into_execution_pending_block_slashable(
self,
block_root: Hash256,
chain: &Arc<BeaconChain<T>>,
) -> Result<ExecutionPendingBlock<T>, BlockSlashInfo<BlockError<T::EthSpec>>> {
let header = self.block.signed_block_header();
let (parent, block) = if let Some(parent) = self.parent {
(parent, self.block)
} else {
load_parent(self.block, chain)
load_parent(self.block_root, self.block, chain)
.map_err(|e| BlockSlashInfo::SignatureValid(header.clone(), e))?
};

ExecutionPendingBlock::from_signature_verified_components(
block,
self.block_root,
parent,
chain,
)
.map_err(|e| BlockSlashInfo::SignatureValid(header, e))
ExecutionPendingBlock::from_signature_verified_components(block, block_root, parent, chain)
.map_err(|e| BlockSlashInfo::SignatureValid(header, e))
}

fn block(&self) -> &SignedBeaconBlock<T::EthSpec> {
Expand All @@ -1026,14 +1029,15 @@ impl<T: BeaconChainTypes> IntoExecutionPendingBlock<T> for Arc<SignedBeaconBlock
/// and then using that implementation of `IntoExecutionPendingBlock` to complete verification.
fn into_execution_pending_block_slashable(
self,
block_root: Hash256,
chain: &Arc<BeaconChain<T>>,
) -> Result<ExecutionPendingBlock<T>, BlockSlashInfo<BlockError<T::EthSpec>>> {
// Perform an early check to prevent wasting time on irrelevant blocks.
let block_root = check_block_relevancy(&self, None, chain)
let block_root = check_block_relevancy(&self, 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(chain)
.into_execution_pending_block_slashable(block_root, chain)
}

fn block(&self) -> &SignedBeaconBlock<T::EthSpec> {
Expand Down Expand Up @@ -1088,7 +1092,7 @@ impl<T: BeaconChainTypes> ExecutionPendingBlock<T> {
* Perform cursory checks to see if the block is even worth processing.
*/

check_block_relevancy(&block, Some(block_root), chain)?;
check_block_relevancy(&block, block_root, chain)?;

/*
* Advance the given `parent.beacon_state` to the slot of the given `block`.
Expand Down Expand Up @@ -1502,7 +1506,7 @@ pub fn check_block_is_finalized_descendant<T: BeaconChainTypes>(
/// experienced whilst attempting to verify.
pub fn check_block_relevancy<T: BeaconChainTypes>(
signed_block: &SignedBeaconBlock<T::EthSpec>,
block_root: Option<Hash256>,
block_root: Hash256,
chain: &BeaconChain<T>,
) -> Result<Hash256, BlockError<T::EthSpec>> {
let block = signed_block.message();
Expand All @@ -1526,8 +1530,6 @@ pub fn check_block_relevancy<T: BeaconChainTypes>(
return Err(BlockError::BlockSlotLimitReached);
}

let block_root = block_root.unwrap_or_else(|| get_block_root(signed_block));

// Do not process a block from a finalized slot.
check_block_against_finalized_slot(block, block_root, chain)?;

Expand Down Expand Up @@ -1581,6 +1583,7 @@ fn verify_parent_block_is_known<T: BeaconChainTypes>(
/// whilst attempting the operation.
#[allow(clippy::type_complexity)]
fn load_parent<T: BeaconChainTypes>(
block_root: Hash256,
block: Arc<SignedBeaconBlock<T::EthSpec>>,
chain: &BeaconChain<T>,
) -> Result<
Expand Down Expand Up @@ -1614,7 +1617,7 @@ fn load_parent<T: BeaconChainTypes>(
.block_times_cache
.read()
.get_block_delays(
block.canonical_root(),
block_root,
chain
.slot_clock
.start_of(block.slot())
Expand Down
4 changes: 3 additions & 1 deletion beacon_node/beacon_chain/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ pub use self::errors::{BeaconChainError, BlockProductionError};
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::{BlockError, ExecutionPayloadError, GossipVerifiedBlock};
pub use block_verification::{
get_block_root, BlockError, ExecutionPayloadError, GossipVerifiedBlock,
};
pub use canonical_head::{CachedHead, CanonicalHead, CanonicalHeadRwLock};
pub use eth1_chain::{Eth1Chain, Eth1ChainBackend};
pub use events::ServerSentEventHandler;
Expand Down
13 changes: 10 additions & 3 deletions beacon_node/beacon_chain/src/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1453,12 +1453,13 @@ where
pub async fn process_block(
&self,
slot: Slot,
block_root: Hash256,
block: SignedBeaconBlock<E>,
) -> Result<SignedBeaconBlockHash, BlockError<E>> {
self.set_current_slot(slot);
let block_hash: SignedBeaconBlockHash = self
.chain
.process_block(Arc::new(block), CountUnrealized::True)
.process_block(block_root, Arc::new(block), CountUnrealized::True)
.await?
.into();
self.chain.recompute_head_at_current_slot().await;
Expand All @@ -1471,7 +1472,11 @@ where
) -> Result<SignedBeaconBlockHash, BlockError<E>> {
let block_hash: SignedBeaconBlockHash = self
.chain
.process_block(Arc::new(block), CountUnrealized::True)
.process_block(
block.canonical_root(),
Arc::new(block),
CountUnrealized::True,
)
.await?
.into();
self.chain.recompute_head_at_current_slot().await;
Expand Down Expand Up @@ -1536,7 +1541,9 @@ where
) -> Result<(SignedBeaconBlockHash, SignedBeaconBlock<E>, BeaconState<E>), BlockError<E>> {
self.set_current_slot(slot);
let (block, new_state) = self.make_block(state, slot).await;
let block_hash = self.process_block(slot, block.clone()).await?;
let block_hash = self
.process_block(slot, block.canonical_root(), block.clone())
.await?;
Ok((block_hash, block, new_state))
}

Expand Down
35 changes: 29 additions & 6 deletions beacon_node/beacon_chain/tests/block_verification.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ async fn assert_invalid_signature(
let process_res = harness
.chain
.process_block(
snapshots[block_index].beacon_block.canonical_root(),
snapshots[block_index].beacon_block.clone(),
CountUnrealized::True,
)
Expand Down Expand Up @@ -403,12 +404,14 @@ async fn invalid_signature_gossip_block() {
.await
.into_block_error()
.expect("should import all blocks prior to the one being tested");
let signed_block = SignedBeaconBlock::from_block(block, junk_signature());
assert!(
matches!(
harness
.chain
.process_block(
Arc::new(SignedBeaconBlock::from_block(block, junk_signature())),
signed_block.canonical_root(),
Arc::new(signed_block),
CountUnrealized::True
)
.await,
Expand Down Expand Up @@ -718,7 +721,11 @@ async fn block_gossip_verification() {

harness
.chain
.process_block(gossip_verified, CountUnrealized::True)
.process_block(
gossip_verified.block_root,
gossip_verified,
CountUnrealized::True,
)
.await
.expect("should import valid gossip verified block");
}
Expand Down Expand Up @@ -985,7 +992,11 @@ async fn verify_block_for_gossip_slashing_detection() {
.unwrap();
harness
.chain
.process_block(verified_block, CountUnrealized::True)
.process_block(
verified_block.block_root,
verified_block,
CountUnrealized::True,
)
.await
.unwrap();
unwrap_err(
Expand Down Expand Up @@ -1020,7 +1031,11 @@ async fn verify_block_for_gossip_doppelganger_detection() {
let attestations = verified_block.block.message().body().attestations().clone();
harness
.chain
.process_block(verified_block, CountUnrealized::True)
.process_block(
verified_block.block_root,
verified_block,
CountUnrealized::True,
)
.await
.unwrap();

Expand Down Expand Up @@ -1161,7 +1176,11 @@ async fn add_base_block_to_altair_chain() {
assert!(matches!(
harness
.chain
.process_block(Arc::new(base_block.clone()), CountUnrealized::True)
.process_block(
base_block.canonical_root(),
Arc::new(base_block.clone()),
CountUnrealized::True
)
.await
.err()
.expect("should error when processing base block"),
Expand Down Expand Up @@ -1289,7 +1308,11 @@ async fn add_altair_block_to_base_chain() {
assert!(matches!(
harness
.chain
.process_block(Arc::new(altair_block.clone()), CountUnrealized::True)
.process_block(
altair_block.canonical_root(),
Arc::new(altair_block.clone()),
CountUnrealized::True
)
.await
.err()
.expect("should error when processing altair block"),
Expand Down
Loading