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

fix(op): skip tx root validation for filtered out dup txns #8316

Merged
merged 2 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
58 changes: 3 additions & 55 deletions bin/reth/src/commands/import_op.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use reth_downloaders::file_client::{

use reth_node_core::init::init_genesis;

use reth_primitives::{hex, stage::StageId, PruneModes, TxHash};
use reth_primitives::{op_mainnet::is_dup_tx, stage::StageId, PruneModes};
use reth_provider::{ProviderFactory, StageCheckpointReader, StaticFileProviderFactory};
use reth_static_file::StaticFileProducer;
use std::{path::PathBuf, sync::Arc};
Expand Down Expand Up @@ -124,8 +124,8 @@ impl ImportOpCommand {
total_decoded_txns += file_client.bodies_len();

for (block_number, body) in file_client.bodies_iter_mut() {
body.transactions.retain(|tx| {
if is_duplicate(tx.hash, *block_number) {
Comment on lines -127 to -128
Copy link
Member

Choose a reason for hiding this comment

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

hmm, are the dup transactions the only transactions in their blocks? Or do the headers with duplicate transactions always contain an empty transactions root? it would seem like the previous check is more correct

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah they are the only transactions in their blocks, I verified. agreed that checking tx hash feels more intuitive, but then again, this is a specific solution to a specific problem, so loss of generality is fine for less LOC.

body.transactions.retain(|_| {
if is_dup_tx(block_number) {
total_filtered_out_dup_txns += 1;
return false
}
Expand Down Expand Up @@ -196,55 +196,3 @@ impl ImportOpCommand {
Ok(())
}
}

/// A transaction that has been replayed in chain below Bedrock.
#[derive(Debug)]
pub struct ReplayedTx {
tx_hash: TxHash,
original_block: u64,
}

impl ReplayedTx {
/// Returns a new instance.
pub const fn new(tx_hash: TxHash, original_block: u64) -> Self {
Self { tx_hash, original_block }
}
}

/// Transaction 0x9ed8..9cb9, first seen in block 985.
pub const TX_BLOCK_985: ReplayedTx = ReplayedTx::new(
TxHash::new(hex!("9ed8f713b2cc6439657db52dcd2fdb9cc944915428f3c6e2a7703e242b259cb9")),
985,
);

/// Transaction 0x86f8..76e5, first seen in block 123 322.
pub const TX_BLOCK_123_322: ReplayedTx = ReplayedTx::new(
TxHash::new(hex!("c033250c5a45f9d104fc28640071a776d146d48403cf5e95ed0015c712e26cb6")),
123_322,
);

/// Transaction 0x86f8..76e5, first seen in block 1 133 328.
pub const TX_BLOCK_1_133_328: ReplayedTx = ReplayedTx::new(
TxHash::new(hex!("86f8c77cfa2b439e9b4e92a10f6c17b99fce1220edf4001e4158b57f41c576e5")),
1_133_328,
);

/// Transaction 0x3cc2..cd4e, first seen in block 1 244 152.
pub const TX_BLOCK_1_244_152: ReplayedTx = ReplayedTx::new(
TxHash::new(hex!("3cc27e7cc8b7a9380b2b2f6c224ea5ef06ade62a6af564a9dd0bcca92131cd4e")),
1_244_152,
);

/// List of original occurrences of all duplicate transactions below Bedrock.
pub const TX_DUP_ORIGINALS: [ReplayedTx; 4] =
[TX_BLOCK_985, TX_BLOCK_123_322, TX_BLOCK_1_133_328, TX_BLOCK_1_244_152];

/// Returns `true` if transaction is the second or third appearance of the transaction.
pub fn is_duplicate(tx_hash: TxHash, block_number: u64) -> bool {
for ReplayedTx { tx_hash: dup_tx_hash, original_block } in TX_DUP_ORIGINALS {
if tx_hash == dup_tx_hash && block_number != original_block {
return true
}
}
false
}
7 changes: 5 additions & 2 deletions crates/consensus/common/src/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use reth_primitives::{
eip4844::{DATA_GAS_PER_BLOB, MAX_DATA_GAS_PER_BLOCK},
MAXIMUM_EXTRA_DATA_SIZE,
},
op_mainnet::is_dup_tx,
ChainSpec, GotExpected, Hardfork, Header, SealedBlock, SealedHeader,
};

Expand Down Expand Up @@ -73,8 +74,10 @@ pub fn validate_block_standalone(
}

// Check transaction root
if let Err(error) = block.ensure_transaction_root_valid() {
return Err(ConsensusError::BodyTransactionRootDiff(error.into()))
if !chain_spec.is_optimism_mainnet() || !is_dup_tx(block.number) {
if let Err(error) = block.ensure_transaction_root_valid() {
return Err(ConsensusError::BodyTransactionRootDiff(error.into()))
}
}

// EIP-4895: Beacon chain push withdrawals as operations
Expand Down
8 changes: 5 additions & 3 deletions crates/net/downloaders/src/file_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,12 @@ impl FileClient {
}

/// Returns a mutable iterator over bodies in the client.
pub fn bodies_iter_mut(&mut self) -> impl Iterator<Item = (&u64, &mut BlockBody)> {
///
/// Panics, if file client headers and bodies are not mapping 1-1.
pub fn bodies_iter_mut(&mut self) -> impl Iterator<Item = (u64, &mut BlockBody)> {
let bodies = &mut self.bodies;
let headers = &self.headers;
headers.keys().zip(bodies.values_mut())
let numbers = &self.hash_to_number;
bodies.iter_mut().map(|(hash, body)| (numbers[hash], body))
}

/// Returns the current number of transactions in the client.
Expand Down
1 change: 1 addition & 0 deletions crates/primitives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ mod header;
mod integer_list;
mod log;
mod net;
pub mod op_mainnet;
pub mod proofs;
mod prune;
mod receipt;
Expand Down
54 changes: 54 additions & 0 deletions crates/primitives/src/op_mainnet.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
//! Helpers for working with Optimism.

/* --------------------- REPLAYED TRANSACTIONS IN BLOCKS BELOW BEDROCK --------------------- */

/// Transaction 0x9ed8f713b2cc6439657db52dcd2fdb9cc944915428f3c6e2a7703e242b259cb9 in block 985,
/// replayed in blocks:
///
/// 19 022
/// 45 036
pub const TX_BLOCK_985: [u64; 2] = [19_022, 45_036];

/// Transaction 0xc033250c5a45f9d104fc28640071a776d146d48403cf5e95ed0015c712e26cb6 in block
/// 123 322, replayed in block:
///
/// 123 542
pub const TX_BLOCK_123_322: u64 = 123_542;

/// Transaction 0x86f8c77cfa2b439e9b4e92a10f6c17b99fce1220edf4001e4158b57f41c576e5 in block
/// 1 133 328, replayed in blocks:
///
/// 1 135 391
/// 1 144 468
pub const TX_BLOCK_1_133_328: [u64; 2] = [1_135_391, 1_144_468];

/// Transaction 0x3cc27e7cc8b7a9380b2b2f6c224ea5ef06ade62a6af564a9dd0bcca92131cd4e in block
/// 1 244 152, replayed in block:
///
/// 1 272 994
pub const TX_BLOCK_1_244_152: u64 = 1_272_994;

/// The six blocks with replayed transactions.
pub const BLOCK_NUMS_REPLAYED_TX: [u64; 6] = [
TX_BLOCK_985[0],
TX_BLOCK_985[1],
TX_BLOCK_123_322,
TX_BLOCK_1_133_328[0],
TX_BLOCK_1_133_328[1],
TX_BLOCK_1_244_152,
];

/// Returns `true` if transaction is the second or third appearance of the transaction. The blocks
/// with replayed transaction happen to only contain the single transaction.
pub fn is_dup_tx(block_number: u64) -> bool {
if block_number > BLOCK_NUMS_REPLAYED_TX[5] {
return false
}

// these blocks just have one transaction!
if BLOCK_NUMS_REPLAYED_TX.contains(&block_number) {
return true
}

false
}
Loading