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(op-isthmus): l1tol2-msg-passer storage root validation #13311

Closed
wants to merge 1 commit into from
Closed
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
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions crates/consensus/consensus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,11 @@ pub enum ConsensusError {
/// The block's timestamp.
timestamp: u64,
},
/// Custom error
// todo: remove in favour of AT Consensus::Error, so OpConsensusError can wrap ConsensusError
// in a variant instead <https://github.com/paradigmxyz/reth/issues/13237>
#[display("custom l2 error (search for it in debug logs)")]
Other,
Comment on lines +457 to +458
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, can we use box core::Error and remove the clone from this error type?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to remove the clone from the error type but it required too many changes and grew the scope of the pr significantly

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, I believe this is due to some other error type Clone derives

we can keep this but perhaps add a string value?

Copy link
Member Author

Choose a reason for hiding this comment

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

this failed for another reason I recall, think it was requirement to be Send + Sync, in that case wrapped in an Arc

}

impl ConsensusError {
Expand Down
4 changes: 4 additions & 0 deletions crates/optimism/consensus/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ reth-consensus-common.workspace = true
reth-consensus.workspace = true
reth-primitives.workspace = true
reth-trie-common.workspace = true
reth-storage-api.workspace = true
reth-storage-errors.workspace = true

# op-reth
reth-optimism-forks.workspace = true
Expand All @@ -31,6 +33,8 @@ alloy-primitives.workspace = true
alloy-consensus.workspace = true
alloy-trie.workspace = true

# misc
derive_more = { workspace = true, features = ["display", "error"] }
tracing.workspace = true

[dev-dependencies]
Expand Down
33 changes: 33 additions & 0 deletions crates/optimism/consensus/src/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//! Optimism consensus errors

use alloy_primitives::B256;
use derive_more::{Display, Error, From};
use reth_storage_errors::provider::ProviderError;

/// Optimism consensus error.
#[derive(Debug, PartialEq, Eq, Clone, Display, Error, From)]
pub enum OpConsensusError {
/// Block body has non-empty withdrawals list.
#[display("non-empty withdrawals list")]
Comment on lines +8 to +11
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can now use thiserror again and should stick to that for new error types

WithdrawalsNonEmpty,
/// Failed to load storage root of
/// [`L2toL1MessagePasser`](reth_optimism_primitives::ADDRESS_L2_TO_L1_MESSAGE_PASSER).
#[display("failed to load storage root of L2toL1MessagePasser pre-deploy: {_0}")]
#[from]
LoadStorageRootFailed(ProviderError),
/// Storage root of
/// [`L2toL1MessagePasser`](reth_optimism_primitives::ADDRESS_L2_TO_L1_MESSAGE_PASSER) missing
/// in block (withdrawals root field).
#[display("storage root of L2toL1MessagePasser missing (withdrawals root field empty)")]
StorageRootMissing,
/// Storage root of
/// [`L2toL1MessagePasser`](reth_optimism_primitives::ADDRESS_L2_TO_L1_MESSAGE_PASSER)
/// in block (withdrawals field), doesn't match local storage root.
#[display("L2toL1MessagePasser storage root mismatch, got: {}, expected {expected}", got.map(|hash| hash.to_string()).unwrap_or_else(|| "null".to_string()))]
StorageRootMismatch {
/// Storage root of pre-deploy in block.
got: Option<B256>,
/// Storage root of pre-deploy loaded from local state.
expected: B256,
},
}
5 changes: 4 additions & 1 deletion crates/optimism/consensus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,14 @@ use reth_optimism_primitives::OpPrimitives;
use reth_primitives::{BlockBody, BlockWithSenders, GotExpected, SealedBlock, SealedHeader};
use std::{sync::Arc, time::SystemTime};

pub mod error;
pub use error::OpConsensusError;

mod proof;
pub use proof::calculate_receipt_root_no_memo_optimism;

mod validation;
pub use validation::validate_block_post_execution;
pub use validation::{isthmus, validate_block_post_execution};

/// Optimism consensus implementation.
///
Expand Down
52 changes: 52 additions & 0 deletions crates/optimism/consensus/src/validation/isthmus.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
//! Block validation w.r.t. consensus rules new in isthmus hardfork.

use reth_consensus::ConsensusError;
use reth_optimism_primitives::predeploys::ADDRESS_L2_TO_L1_MESSAGE_PASSER;
use reth_primitives::SealedHeader;
use reth_storage_api::{StateProviderFactory, StorageRootProvider};
use tracing::{debug, trace};

use crate::OpConsensusError;

/// Validates block header field `withdrawals_root` against storage root of
/// `L2-to-L1-message-passer` predeploy.
pub fn validate_l2_to_l1_msg_passer(
provider: impl StateProviderFactory,
header: &SealedHeader,
) -> Result<(), ConsensusError> {
Comment on lines +13 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we split this into two, one fn that validates given the storage root, and one that computes the storage root and then calls this other fn (which we could move to op-alloy as well) because I assume kona could maybe also use this

let state = provider.latest().map_err(|err| {
debug!(target: "op::consensus",
block_number=header.number,
err=%OpConsensusError::LoadStorageRootFailed(err),
"failed to load latest state",
);

ConsensusError::Other
})?;

let storage_root_msg_passer =
state.storage_root(ADDRESS_L2_TO_L1_MESSAGE_PASSER, Default::default()).map_err(|err| {
debug!(target: "op::consensus",
block_number=header.number,
err=%OpConsensusError::LoadStorageRootFailed(err),
"failed to load storage root for l2tol1-msg-passer predeploy",
);

ConsensusError::Other
})?;

if header.withdrawals_root.is_none_or(|root| root != storage_root_msg_passer) {
trace!(target: "op::consensus",
block_number=header.number,
err=%OpConsensusError::StorageRootMismatch {
got: header.withdrawals_root,
expected: storage_root_msg_passer
},
"block failed validation",
);

return Err(ConsensusError::Other)
}

Ok(())
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
//! Validation of blocks w.r.t. Optimism hardforks.

pub mod isthmus;

use crate::proof::calculate_receipt_root_optimism;
use alloy_consensus::TxReceipt;
use alloy_primitives::{Bloom, B256};
Expand Down
2 changes: 2 additions & 0 deletions crates/optimism/primitives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@
extern crate alloc;

pub mod bedrock;
pub mod predeploys;
pub mod transaction;

pub use predeploys::ADDRESS_L2_TO_L1_MESSAGE_PASSER;
pub use transaction::{signed::OpTransactionSigned, tx_type::OpTxType, OpTransaction};

/// Optimism primitive types.
Expand Down
8 changes: 8 additions & 0 deletions crates/optimism/primitives/src/predeploys.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
//! Addresses of OP pre-deploys.
// todo: move to op-alloy
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah let's do that


use alloy_primitives::{address, Address};

/// The L2 contract `L2ToL1MessagePasser`, stores commitments to withdrawal transactions.
pub const ADDRESS_L2_TO_L1_MESSAGE_PASSER: Address =
address!("4200000000000000000000000000000000000016");
Loading