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.

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

Optimise block_root computation in gossip verification #5003

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

pawanjay176
Copy link
Member

Issue Addressed

N/A

Proposed Changes

Before doing gossip verification, we get the header corresponding to a block to pass to a slasher.
The SignedBeaconBlock and its correspondingSignedBeaconBlockHeader have the same canonical root for the message, however, computing the root for the header is way faster since the root for the tree rooted at BeaconBlockBody is already calculated in the header.

In this PR, we reuse the header to compute the block_root instead of using the full block.
The root computation in mainnet seems to take 10ms on average looking at the beacon_block_processing_block_root_seconds metric.
Using the header to compute the root should take micro seconds so this seems like a low hanging fruit optimisation.

@pawanjay176 pawanjay176 added ready-for-review The code is ready for review optimization Something to make Lighthouse run more efficiently. labels Dec 12, 2023
@@ -810,7 +814,7 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
});
}

let block_root = get_block_root(&block);
let block_root = get_block_header_root(block_header);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if we could delete get_block_root but it looks like it's used in RpcBlock, so happy to leave it as-is for now.

Aside: I'm not super familiar with RpcBlock but have we checked that blocks that fail availability checks are passed to the slasher?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocks are added to the slasher on calling BeaconChain::process_block which calls IntoExecutionPendingBlock::into_execution_pending_block here

fn into_execution_pending_block(
self,
block_root: Hash256,
chain: &Arc<BeaconChain<T>>,
notify_execution_layer: NotifyExecutionLayer,
) -> Result<ExecutionPendingBlock<T>, BlockError<T::EthSpec>> {
self.into_execution_pending_block_slashable(block_root, chain, notify_execution_layer)
.map(|execution_pending| {
// Supply valid block to slasher.
if let Some(slasher) = chain.slasher.as_ref() {
slasher.accept_block_header(execution_pending.block.signed_block_header());
}
execution_pending
})
.map_err(|slash_info| {
process_block_slash_info::<_, BlockError<T::EthSpec>>(chain, slash_info)
})
}

RpcBlock also implements IntoExecutionPendingBlock so it's added to the slasher regardless if it passes or fails the availability checks

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Dec 13, 2023
@michaelsproul michaelsproul merged commit a3a3703 into sigp:unstable Dec 13, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Something to make Lighthouse run more efficiently. ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants