Skip to content
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
3 changes: 3 additions & 0 deletions crates/op-rbuilder/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,7 @@ pub struct OpRbuilderArgs {
/// How much time extra to wait for the block building job to complete and not get garbage collected
#[arg(long = "builder.extra-block-deadline-secs", default_value = "20")]
pub extra_block_deadline_secs: u64,
/// Whether to enable revert protection by default
#[arg(long = "builder.enable-revert-protection", default_value = "false")]
pub enable_revert_protection: bool,
}
12 changes: 12 additions & 0 deletions crates/op-rbuilder/src/generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ pub struct BlockPayloadJobGenerator<Client, Tasks, Builder> {
last_payload: Arc<Mutex<CancellationToken>>,
/// The extra block deadline in seconds
extra_block_deadline: std::time::Duration,
/// Whether to enable revert protection
enable_revert_protection: bool,
}

// === impl EmptyBlockPayloadJobGenerator ===
Expand All @@ -88,6 +90,7 @@ impl<Client, Tasks, Builder> BlockPayloadJobGenerator<Client, Tasks, Builder> {
builder: Builder,
ensure_only_one_payload: bool,
extra_block_deadline: std::time::Duration,
enable_revert_protection: bool,
) -> Self {
Self {
client,
Expand All @@ -97,6 +100,7 @@ impl<Client, Tasks, Builder> BlockPayloadJobGenerator<Client, Tasks, Builder> {
ensure_only_one_payload,
last_payload: Arc::new(Mutex::new(CancellationToken::new())),
extra_block_deadline,
enable_revert_protection,
}
}
}
Expand Down Expand Up @@ -182,6 +186,7 @@ where
cancel: cancel_token,
deadline,
build_complete: None,
enable_revert_protection: self.enable_revert_protection,
};

job.spawn_build_job();
Expand Down Expand Up @@ -214,6 +219,8 @@ where
pub(crate) cancel: CancellationToken,
pub(crate) deadline: Pin<Box<Sleep>>, // Add deadline
pub(crate) build_complete: Option<oneshot::Receiver<Result<(), PayloadBuilderError>>>,
/// Block building options
pub(crate) enable_revert_protection: bool,
}

impl<Tasks, Builder> PayloadJob for BlockPayloadJob<Tasks, Builder>
Expand Down Expand Up @@ -256,6 +263,8 @@ pub struct BuildArguments<Attributes, Payload: BuiltPayload> {
pub config: PayloadConfig<Attributes, HeaderTy<Payload::Primitives>>,
/// A marker that can be used to cancel the job.
pub cancel: CancellationToken,
/// Whether to enable revert protection
pub enable_revert_protection: bool,
}

/// A [PayloadJob] is a future that's being polled by the `PayloadBuilderService`
Expand All @@ -271,6 +280,7 @@ where
let payload_config = self.config.clone();
let cell = self.cell.clone();
let cancel = self.cancel.clone();
let enable_revert_protection = self.enable_revert_protection;

let (tx, rx) = oneshot::channel();
self.build_complete = Some(rx);
Expand All @@ -280,6 +290,7 @@ where
cached_reads: Default::default(),
config: payload_config,
cancel,
enable_revert_protection,
};

let result = builder.try_build(args, cell);
Expand Down Expand Up @@ -639,6 +650,7 @@ mod tests {
builder.clone(),
false,
std::time::Duration::from_secs(1),
false,
);

// this is not nice but necessary
Expand Down
39 changes: 38 additions & 1 deletion crates/op-rbuilder/src/integration/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,42 @@ mod tests {
Ok(())
}

#[tokio::test]
#[cfg(not(feature = "flashblocks"))]
async fn integration_test_revert_protection_disabled() -> eyre::Result<()> {
let harness = TestHarness::new("integration_test_revert_protection_disabled").await;
let mut generator = harness.block_generator().await?;

let txn1 = harness.send_valid_transaction().await?;
let txn2 = harness.send_revert_transaction().await?;
let pending_txn = vec![txn1, txn2];

let block_hash = generator.generate_block().await?;

// the transactions should be included in the block now
let pending_txn = {
let mut transaction_hashes = Vec::new();
for txn in pending_txn {
let txn_hash = txn.with_timeout(None).watch().await?;
transaction_hashes.push(txn_hash);
}
transaction_hashes
};

// validate that all the transaction hashes are included in the block
let provider = harness.provider()?;
let block = provider
.get_block_by_hash(block_hash)
.await?
.expect("block");

for txn in pending_txn {
assert!(block.transactions.hashes().any(|hash| hash == txn));
}

Ok(())
}

#[tokio::test]
#[cfg(not(feature = "flashblocks"))]
async fn integration_test_revert_protection() -> eyre::Result<()> {
Expand All @@ -115,7 +151,8 @@ mod tests {
.auth_rpc_port(1244)
.network_port(1245)
.http_port(1248)
.with_builder_private_key(BUILDER_PRIVATE_KEY);
.with_builder_private_key(BUILDER_PRIVATE_KEY)
.with_revert_protection(true);

// create the validation reth node
let reth_data_dir = std::env::temp_dir().join(Uuid::new_v4().to_string());
Expand Down
66 changes: 62 additions & 4 deletions crates/op-rbuilder/src/integration/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use alloy_consensus::TxEip1559;
use alloy_eips::{eip1559::MIN_PROTOCOL_BASE_FEE, eip2718::Encodable2718, BlockNumberOrTag};
use alloy_provider::{Identity, Provider, ProviderBuilder};
use alloy_primitives::{hex, TxHash, TxNonce};
use alloy_provider::{
Identity, PendingTransactionBuilder, Provider, ProviderBuilder, RootProvider,
};
use op_alloy_consensus::OpTypedTransaction;
use op_alloy_network::Optimism;
use op_rbuilder::OpRbuilderConfig;
Expand Down Expand Up @@ -269,7 +272,9 @@ impl TestHarness {
}
}

pub async fn send_valid_transaction(&self) -> eyre::Result<()> {
pub async fn send_valid_transaction(
&self,
) -> eyre::Result<PendingTransactionBuilder<Optimism>> {
// Get builder's address
let known_wallet = Signer::try_from_secret(BUILDER_PRIVATE_KEY.parse()?)?;
let builder_address = known_wallet.address;
Expand Down Expand Up @@ -303,11 +308,64 @@ impl TestHarness {
..Default::default()
});
let signed_tx = known_wallet.sign_tx(tx_request)?;
let _ = provider
let pending_tx = provider
.send_raw_transaction(signed_tx.encoded_2718().as_slice())
.await?;

Ok(())
Ok(pending_tx)
}

pub async fn send_revert_transaction(
&self,
) -> eyre::Result<PendingTransactionBuilder<Optimism>> {
// TODO: Merge this with send_valid_transaction
// Get builder's address
let known_wallet = Signer::try_from_secret(BUILDER_PRIVATE_KEY.parse()?)?;
let builder_address = known_wallet.address;

let url = format!("http://localhost:{}", self.builder_http_port);
let provider =
ProviderBuilder::<Identity, Identity, Optimism>::default().on_http(url.parse()?);

// Get current nonce includeing the ones from the txpool
let nonce = provider
.get_transaction_count(builder_address)
.pending()
.await?;

let latest_block = provider
.get_block_by_number(BlockNumberOrTag::Latest)
.await?
.unwrap();

let base_fee = max(
latest_block.header.base_fee_per_gas.unwrap(),
MIN_PROTOCOL_BASE_FEE,
);

// Transaction from builder should succeed
let tx_request = OpTypedTransaction::Eip1559(TxEip1559 {
chain_id: 901,
nonce,
gas_limit: 210000,
max_fee_per_gas: base_fee.into(),
input: hex!("60006000fd").into(), // PUSH1 0x00 PUSH1 0x00 REVERT
..Default::default()
});
let signed_tx = known_wallet.sign_tx(tx_request)?;
let pending_tx = provider
.send_raw_transaction(signed_tx.encoded_2718().as_slice())
.await?;

Ok(pending_tx)
}

pub fn provider(&self) -> eyre::Result<RootProvider<Optimism>> {
let url = format!("http://localhost:{}", self.builder_http_port);
let provider =
ProviderBuilder::<Identity, Identity, Optimism>::default().on_http(url.parse()?);

Ok(provider)
}

pub async fn block_generator(&self) -> eyre::Result<BlockGenerator> {
Expand Down
12 changes: 12 additions & 0 deletions crates/op-rbuilder/src/integration/op_rbuilder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub struct OpRbuilderConfig {
flashblocks_ws_url: Option<String>,
chain_block_time: Option<u64>,
flashbots_block_time: Option<u64>,
with_revert_protection: Option<bool>,
}

impl OpRbuilderConfig {
Expand Down Expand Up @@ -64,6 +65,11 @@ impl OpRbuilderConfig {
self
}

pub fn with_revert_protection(mut self, revert_protection: bool) -> Self {
self.with_revert_protection = Some(revert_protection);
self
}

pub fn with_flashblocks_ws_url(mut self, url: &str) -> Self {
self.flashblocks_ws_url = Some(url.to_string());
self
Expand Down Expand Up @@ -114,6 +120,12 @@ impl Service for OpRbuilderConfig {
.arg(self.network_port.expect("network_port not set").to_string())
.arg("--ipcdisable");

if let Some(revert_protection) = self.with_revert_protection {
if revert_protection {
cmd.arg("--builder.enable-revert-protection");
}
}

if let Some(builder_private_key) = &self.builder_private_key {
cmd.arg("--rollup.builder-secret-key")
.arg(builder_private_key);
Expand Down
1 change: 1 addition & 0 deletions crates/op-rbuilder/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ fn main() {
.with_components(op_node.components().payload(CustomOpPayloadBuilder::new(
builder_args.builder_signer,
std::time::Duration::from_secs(builder_args.extra_block_deadline_secs),
builder_args.enable_revert_protection,
builder_args.flashblocks_ws_url,
builder_args.chain_block_time,
builder_args.flashblock_block_time,
Expand Down
1 change: 1 addition & 0 deletions crates/op-rbuilder/src/payload_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ impl CustomOpPayloadBuilder {
pub fn new(
builder_signer: Option<Signer>,
extra_block_deadline: std::time::Duration,
enable_revert_protection: bool,
flashblocks_ws_url: String,
chain_block_time: u64,
flashblock_block_time: u64,
Expand Down
27 changes: 22 additions & 5 deletions crates/op-rbuilder/src/payload_builder_vanilla.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ const TOTAL_COST_FLOOR_PER_TOKEN: u64 = 10;
pub struct CustomOpPayloadBuilder {
builder_signer: Option<Signer>,
extra_block_deadline: std::time::Duration,
enable_revert_protection: bool,
#[cfg(feature = "flashblocks")]
flashblocks_ws_url: String,
#[cfg(feature = "flashblocks")]
Expand Down Expand Up @@ -105,13 +106,15 @@ impl CustomOpPayloadBuilder {
pub fn new(
builder_signer: Option<Signer>,
extra_block_deadline: std::time::Duration,
enable_revert_protection: bool,
_flashblocks_ws_url: String,
_chain_block_time: u64,
_flashblock_block_time: u64,
) -> Self {
Self {
builder_signer,
extra_block_deadline,
enable_revert_protection,
}
}
}
Expand Down Expand Up @@ -167,6 +170,7 @@ where
) -> eyre::Result<PayloadBuilderHandle<<Node::Types as NodeTypes>::Payload>> {
tracing::info!("Spawning a custom payload builder");
let extra_block_deadline = self.extra_block_deadline;
let enable_revert_protection = self.enable_revert_protection;
let payload_builder = self.build_payload_builder(ctx, pool).await?;
let payload_job_config = BasicPayloadJobGeneratorConfig::default();

Expand All @@ -177,6 +181,7 @@ where
payload_builder,
false,
extra_block_deadline,
enable_revert_protection,
);

let (payload_service, payload_builder) =
Expand Down Expand Up @@ -351,6 +356,7 @@ where
mut cached_reads,
config,
cancel,
enable_revert_protection,
} = args;

let chain_spec = self.client.chain_spec();
Expand Down Expand Up @@ -392,6 +398,7 @@ where
cancel,
builder_signer: self.builder_signer,
metrics: Default::default(),
enable_revert_protection,
};

let builder = OpBuilder::new(best, remove_reverted);
Expand Down Expand Up @@ -771,6 +778,8 @@ pub struct OpPayloadBuilderCtx<ChainSpec, N: NodePrimitives> {
pub builder_signer: Option<Signer>,
/// The metrics for the builder
pub metrics: OpRBuilderMetrics,
/// Whether we enabled revert protection
pub enable_revert_protection: bool,
}

impl<ChainSpec, N> OpPayloadBuilderCtx<ChainSpec, N>
Expand Down Expand Up @@ -1094,6 +1103,12 @@ where
}
};

println!("include reverted txn: {}", result.is_success());
println!(
"self.enable_revert_protection: {}",
self.enable_revert_protection
);

self.metrics
.tx_simulation_duration
.record(tx_simulation_start_time.elapsed());
Expand All @@ -1102,11 +1117,13 @@ where
if result.is_success() {
num_txs_simulated_success += 1;
} else {
num_txs_simulated_fail += 1;
info!(target: "payload_builder", tx_hash = ?tx.tx_hash(), "skipping reverted transaction");
best_txs.mark_invalid(tx.signer(), tx.nonce());
info.invalid_tx_hashes.insert(tx.tx_hash());
continue;
if self.enable_revert_protection {
num_txs_simulated_fail += 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

think we still want this metric here

Copy link
Collaborator

Choose a reason for hiding this comment

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

should have it outside of the flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it resolved now?

info!(target: "payload_builder", tx_hash = ?tx.tx_hash(), "skipping reverted transaction");
best_txs.mark_invalid(tx.signer(), tx.nonce());
info.invalid_tx_hashes.insert(tx.tx_hash());
continue;
}
}

// add gas used by the transaction to cumulative gas used, before creating the
Expand Down
Loading