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

feat: make Consensus trait generic over block parts #12451

Merged
merged 5 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
47 changes: 46 additions & 1 deletion crates/consensus/common/src/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ use alloy_consensus::constants::MAXIMUM_EXTRA_DATA_SIZE;
use alloy_eips::eip4844::{DATA_GAS_PER_BLOB, MAX_DATA_GAS_PER_BLOCK};
use reth_chainspec::{EthChainSpec, EthereumHardforks};
use reth_consensus::ConsensusError;
use reth_primitives::{EthereumHardfork, GotExpected, Header, SealedBlock, SealedHeader};
use reth_primitives::{
BlockBody, EthereumHardfork, GotExpected, Header, SealedBlock, SealedHeader,
};
use revm_primitives::calc_excess_blob_gas;

/// Gas used needs to be less than gas limit. Gas used is going to be checked after execution.
Expand Down Expand Up @@ -73,6 +75,49 @@ pub fn validate_cancun_gas(block: &SealedBlock) -> Result<(), ConsensusError> {
Ok(())
}

/// Ensures the block response data matches the header.
///
/// This ensures the body response items match the header's hashes:
/// - ommer hash
/// - transaction root
/// - withdrawals root
pub fn validate_body_against_header(
body: &BlockBody,
header: &SealedHeader,
) -> Result<(), ConsensusError> {
let ommers_hash = body.calculate_ommers_root();
if header.ommers_hash != ommers_hash {
return Err(ConsensusError::BodyOmmersHashDiff(
GotExpected { got: ommers_hash, expected: header.ommers_hash }.into(),
))
}

let tx_root = body.calculate_tx_root();
if header.transactions_root != tx_root {
return Err(ConsensusError::BodyTransactionRootDiff(
GotExpected { got: tx_root, expected: header.transactions_root }.into(),
))
}

match (header.withdrawals_root, &body.withdrawals) {
(Some(header_withdrawals_root), Some(withdrawals)) => {
let withdrawals = withdrawals.as_slice();
let withdrawals_root = reth_primitives::proofs::calculate_withdrawals_root(withdrawals);
if withdrawals_root != header_withdrawals_root {
return Err(ConsensusError::BodyWithdrawalsRootDiff(
GotExpected { got: withdrawals_root, expected: header_withdrawals_root }.into(),
))
}
}
(None, None) => {
// this is ok because we assume the fork is not active in this case
}
_ => return Err(ConsensusError::WithdrawalsRootUnexpected),
}

Ok(())
}

/// Validate a block without regard for state:
///
/// - Compares the ommer hash in the block header to the block body
Expand Down
34 changes: 24 additions & 10 deletions crates/consensus/consensus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ use alloc::{fmt::Debug, vec::Vec};
use alloy_eips::eip7685::Requests;
use alloy_primitives::{BlockHash, BlockNumber, Bloom, B256, U256};
use reth_primitives::{
constants::MINIMUM_GAS_LIMIT, BlockWithSenders, GotExpected, GotExpectedBoxed, Header,
InvalidTransactionError, Receipt, SealedBlock, SealedHeader,
constants::MINIMUM_GAS_LIMIT, BlockBody, BlockWithSenders, GotExpected, GotExpectedBoxed,
Header, InvalidTransactionError, Receipt, SealedBlock, SealedHeader,
};

/// A consensus implementation that does nothing.
Expand Down Expand Up @@ -44,11 +44,11 @@ impl<'a> PostExecutionInput<'a> {

/// Consensus is a protocol that chooses canonical chain.
#[auto_impl::auto_impl(&, Arc)]
pub trait Consensus: Debug + Send + Sync {
pub trait Consensus<H = Header, B = BlockBody>: Debug + Send + Sync {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub trait Consensus<H = Header, B = BlockBody>: Debug + Send + Sync {
pub trait Consensus<N: NodePrimitives<BlockHeader = Header, BlockBody = BlockBody>>: Debug + Send + Sync {

prefer this, and adding header and block body as ATs parallel to block in NodePrimitives. it's nice to grow the generics list by only a single element everywhere via this task of generalising the data primitives.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

imo ideally this has a single block AT bounded by

Consensus responsibility is to validate blocks so don't think it should know about entire node storage model. this would also be consistent with pool traits which only know the transaction AT

I've done this as 2 generics right now so that we can still keep dyn Consensus everywhere

does it make sense?

Copy link
Member

Choose a reason for hiding this comment

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

why can't you keep the trait object if you add the generic?

NetworkPrimitives has header and body as ATs, and the block, so I'm not sure you're intentions are really clear?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why can't you keep the trait object if you add the generic?

I mean that we can't keep it as is if we'd add AT because we'd need to specify Block = Block everywhere, so went with generics for now

NetworkPrimitives has header and body as ATs, and the block, so I'm not sure you're intentions are really clear?

sorry, not sure I follow, how are NetworkPrimitives related to consensus trait?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah a single AT would be ideal, but for now we can introduce both AT just to move forward

Copy link
Member

Choose a reason for hiding this comment

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

i

why can't you keep the trait object if you add the generic?

I mean that we can't keep it as is if we'd add AT because we'd need to specify Block = Block everywhere, so went with generics for now

I see, seems to be a misunderstanding, I didn't mean adding an AT to the consensus trait

NetworkPrimitives has header and body as ATs, and the block, so I'm not sure your intentions are really clear?

sorry, not sure I follow, how are NetworkPrimitives related to consensus trait?

if you remove the rlp encoding trait bounds from ATs on NetworkPrimitives, and just have auto trait bounds, then the associated types can easily just be the unit struct ().

by passing one type, that aggregates the data primitives as associated types, we make a framework that is extensible without breaking the public api.

in the future, and even towards the end of this sdk integration task, it may well be, that somewhere in consensus, another data primitive type is needed. then we have to change the type definition again to put another generic to the list if we do it the way you impl it now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

by passing one type, that aggregates the data primitives as associated types, we make a framework that is extensible without breaking the public api.

in the future, and even towards the end of this sdk integration task, it may well be, that somewhere in consensus, another data primitive type is needed. then we have to change the type definition again to put another generic to the list if we do it the way you impl it now.

hmm, in scope of sdk the Consensus trait API only needs block and its components right now. If we were to add additional methods requiring something else this would anyway be a breaking change to API I think? and internally trait implementations are free to use any components

Copy link
Member

Choose a reason for hiding this comment

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

just recently op impl of transaction rpc, needed a receipt type, who would have known.

#11164
#11732

keep the code usable for use cases you could not have had time to think of yourself at the time of impl.

/// Validate if header is correct and follows consensus specification.
///
/// This is called on standalone header to check if all hashes are correct.
fn validate_header(&self, header: &SealedHeader) -> Result<(), ConsensusError>;
fn validate_header(&self, header: &SealedHeader<H>) -> Result<(), ConsensusError>;

/// Validate that the header information regarding parent are correct.
/// This checks the block number, timestamp, basefee and gas limit increment.
Expand All @@ -61,8 +61,8 @@ pub trait Consensus: Debug + Send + Sync {
/// Note: Validating header against its parent does not include other Consensus validations.
fn validate_header_against_parent(
&self,
header: &SealedHeader,
parent: &SealedHeader,
header: &SealedHeader<H>,
parent: &SealedHeader<H>,
) -> Result<(), ConsensusError>;

/// Validates the given headers
Expand All @@ -71,7 +71,13 @@ pub trait Consensus: Debug + Send + Sync {
/// on its own and valid against its parent.
///
/// Note: this expects that the headers are in natural order (ascending block number)
fn validate_header_range(&self, headers: &[SealedHeader]) -> Result<(), HeaderConsensusError> {
fn validate_header_range(
&self,
headers: &[SealedHeader<H>],
) -> Result<(), HeaderConsensusError<H>>
where
H: Clone,
{
if let Some((initial_header, remaining_headers)) = headers.split_first() {
self.validate_header(initial_header)
.map_err(|e| HeaderConsensusError(e, initial_header.clone()))?;
Expand All @@ -94,10 +100,17 @@ pub trait Consensus: Debug + Send + Sync {
/// Note: validating headers with TD does not include other Consensus validation.
fn validate_header_with_total_difficulty(
&self,
header: &Header,
header: &H,
total_difficulty: U256,
) -> Result<(), ConsensusError>;

/// Ensures that body field values match the header.
fn validate_body_against_header(
&self,
body: &B,
header: &SealedHeader<H>,
) -> Result<(), ConsensusError>;

/// Validate a block disregarding world state, i.e. things that can be checked before sender
/// recovery and execution.
///
Expand All @@ -107,7 +120,8 @@ pub trait Consensus: Debug + Send + Sync {
/// **This should not be called for the genesis block**.
///
/// Note: validating blocks does not include other validations of the Consensus
fn validate_block_pre_execution(&self, block: &SealedBlock) -> Result<(), ConsensusError>;
fn validate_block_pre_execution(&self, block: &SealedBlock<H, B>)
-> Result<(), ConsensusError>;

/// Validate a block considering world state, i.e. things that can not be checked before
/// execution.
Expand Down Expand Up @@ -407,4 +421,4 @@ impl From<InvalidTransactionError> for ConsensusError {
/// `HeaderConsensusError` combines a `ConsensusError` with the `SealedHeader` it relates to.
#[derive(derive_more::Display, derive_more::Error, Debug)]
#[display("Consensus error: {_0}, Invalid header: {_1:?}")]
pub struct HeaderConsensusError(ConsensusError, SealedHeader);
pub struct HeaderConsensusError<H>(ConsensusError, SealedHeader<H>);
25 changes: 18 additions & 7 deletions crates/consensus/consensus/src/noop.rs
Original file line number Diff line number Diff line change
@@ -1,34 +1,45 @@
use crate::{Consensus, ConsensusError, PostExecutionInput};
use alloy_primitives::U256;
use reth_primitives::{BlockWithSenders, Header, SealedBlock, SealedHeader};
use reth_primitives::{BlockWithSenders, SealedBlock, SealedHeader};

/// A Consensus implementation that does nothing.
#[derive(Debug, Copy, Clone, Default)]
#[non_exhaustive]
pub struct NoopConsensus;

impl Consensus for NoopConsensus {
fn validate_header(&self, _header: &SealedHeader) -> Result<(), ConsensusError> {
impl<H, B> Consensus<H, B> for NoopConsensus {
fn validate_header(&self, _header: &SealedHeader<H>) -> Result<(), ConsensusError> {
Ok(())
}

fn validate_header_against_parent(
&self,
_header: &SealedHeader,
_parent: &SealedHeader,
_header: &SealedHeader<H>,
_parent: &SealedHeader<H>,
) -> Result<(), ConsensusError> {
Ok(())
}

fn validate_header_with_total_difficulty(
&self,
_header: &Header,
_header: &H,
_total_difficulty: U256,
) -> Result<(), ConsensusError> {
Ok(())
}

fn validate_block_pre_execution(&self, _block: &SealedBlock) -> Result<(), ConsensusError> {
fn validate_body_against_header(
&self,
_body: &B,
_header: &SealedHeader<H>,
) -> Result<(), ConsensusError> {
Ok(())
}

fn validate_block_pre_execution(
&self,
_block: &SealedBlock<H, B>,
) -> Result<(), ConsensusError> {
Ok(())
}

Expand Down
51 changes: 42 additions & 9 deletions crates/consensus/consensus/src/test_utils.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,25 @@
use crate::{Consensus, ConsensusError, PostExecutionInput};
use alloy_primitives::U256;
use core::sync::atomic::{AtomicBool, Ordering};
use reth_primitives::{BlockWithSenders, Header, SealedBlock, SealedHeader};
use reth_primitives::{BlockWithSenders, SealedBlock, SealedHeader};

/// Consensus engine implementation for testing
#[derive(Debug)]
pub struct TestConsensus {
/// Flag whether the header validation should purposefully fail
fail_validation: AtomicBool,
/// Separate flag for setting whether `validate_body_against_header` should fail. It is needed
/// for testing networking logic for which the body failing this check is getting completely
/// rejected while more high-level failures are handled by the sync logic.
fail_body_against_header: AtomicBool,
}

impl Default for TestConsensus {
fn default() -> Self {
Self { fail_validation: AtomicBool::new(false) }
Self {
fail_validation: AtomicBool::new(false),
fail_body_against_header: AtomicBool::new(false),
}
}
}

Expand All @@ -24,12 +31,23 @@ impl TestConsensus {

/// Update the validation flag.
pub fn set_fail_validation(&self, val: bool) {
self.fail_validation.store(val, Ordering::SeqCst)
self.fail_validation.store(val, Ordering::SeqCst);
self.fail_body_against_header.store(val, Ordering::SeqCst);
}

/// Returns the body validation flag.
pub fn fail_body_against_header(&self) -> bool {
self.fail_body_against_header.load(Ordering::SeqCst)
}

/// Update the body validation flag.
pub fn set_fail_body_against_header(&self, val: bool) {
self.fail_body_against_header.store(val, Ordering::SeqCst);
}
}

impl Consensus for TestConsensus {
fn validate_header(&self, _header: &SealedHeader) -> Result<(), ConsensusError> {
impl<H, B> Consensus<H, B> for TestConsensus {
fn validate_header(&self, _header: &SealedHeader<H>) -> Result<(), ConsensusError> {
if self.fail_validation() {
Err(ConsensusError::BaseFeeMissing)
} else {
Expand All @@ -39,8 +57,8 @@ impl Consensus for TestConsensus {

fn validate_header_against_parent(
&self,
_header: &SealedHeader,
_parent: &SealedHeader,
_header: &SealedHeader<H>,
_parent: &SealedHeader<H>,
) -> Result<(), ConsensusError> {
if self.fail_validation() {
Err(ConsensusError::BaseFeeMissing)
Expand All @@ -51,7 +69,7 @@ impl Consensus for TestConsensus {

fn validate_header_with_total_difficulty(
&self,
_header: &Header,
_header: &H,
_total_difficulty: U256,
) -> Result<(), ConsensusError> {
if self.fail_validation() {
Expand All @@ -61,7 +79,22 @@ impl Consensus for TestConsensus {
}
}

fn validate_block_pre_execution(&self, _block: &SealedBlock) -> Result<(), ConsensusError> {
fn validate_body_against_header(
&self,
_body: &B,
_header: &SealedHeader<H>,
) -> Result<(), ConsensusError> {
if self.fail_body_against_header() {
Err(ConsensusError::BaseFeeMissing)
} else {
Ok(())
}
}

fn validate_block_pre_execution(
&self,
_block: &SealedBlock<H, B>,
) -> Result<(), ConsensusError> {
if self.fail_validation() {
Err(ConsensusError::BaseFeeMissing)
} else {
Expand Down
14 changes: 11 additions & 3 deletions crates/ethereum/consensus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ use reth_consensus::{Consensus, ConsensusError, PostExecutionInput};
use reth_consensus_common::validation::{
validate_4844_header_standalone, validate_against_parent_4844,
validate_against_parent_eip1559_base_fee, validate_against_parent_hash_number,
validate_against_parent_timestamp, validate_block_pre_execution, validate_header_base_fee,
validate_header_extradata, validate_header_gas,
validate_against_parent_timestamp, validate_block_pre_execution, validate_body_against_header,
validate_header_base_fee, validate_header_extradata, validate_header_gas,
};
use reth_primitives::{
constants::MINIMUM_GAS_LIMIT, BlockWithSenders, Header, SealedBlock, SealedHeader,
constants::MINIMUM_GAS_LIMIT, BlockBody, BlockWithSenders, Header, SealedBlock, SealedHeader,
};
use std::{fmt::Debug, sync::Arc, time::SystemTime};

Expand Down Expand Up @@ -212,6 +212,14 @@ impl<ChainSpec: Send + Sync + EthChainSpec + EthereumHardforks + Debug> Consensu
Ok(())
}

fn validate_body_against_header(
&self,
body: &BlockBody,
header: &SealedHeader,
) -> Result<(), ConsensusError> {
validate_body_against_header(body, header)
}

fn validate_block_pre_execution(&self, block: &SealedBlock) -> Result<(), ConsensusError> {
validate_block_pre_execution(block, &self.chain_spec)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/net/downloaders/src/headers/reverse_headers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1310,7 +1310,7 @@ mod tests {
fn test_head_update() {
let client = Arc::new(TestHeadersClient::default());

let header = SealedHeader::default();
let header: SealedHeader = SealedHeader::default();

let mut downloader = ReverseHeadersDownloaderBuilder::default()
.build(Arc::clone(&client), Arc::new(TestConsensus::default()));
Expand Down
4 changes: 1 addition & 3 deletions crates/net/network/src/fetch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,6 @@ mod tests {
use super::*;
use crate::{peers::PeersManager, PeersConfig};
use alloy_primitives::B512;
use reth_primitives::SealedHeader;
use std::future::poll_fn;

#[tokio::test(flavor = "multi_thread")]
Expand Down Expand Up @@ -590,8 +589,7 @@ mod tests {
},
response: tx,
};
let mut header = SealedHeader::default().unseal();
header.number = 0u64;
let header = Header { number: 0, ..Default::default() };
(req, header)
};

Expand Down
Loading
Loading