Skip to content

fix: ensure DA transaction throttling uses params that are set #8

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: ensure DA transaction throttling uses params that are set #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 6 additions & 2 deletions crates/op-rbuilder/src/integration/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,9 @@ mod tests {
}

// check there's 10 flashblocks log lines (2000ms / 200ms)
op_rbuilder.find_log_line("Building flashblock 9").await?;
op_rbuilder
.find_log_line("Building flashblock idx=9")
.await?;

// Process websocket messages
let timeout_duration = Duration::from_secs(10);
Expand Down Expand Up @@ -675,7 +677,9 @@ mod tests {
}

// check there's no more than 10 flashblocks log lines (2000ms / 200ms)
op_rbuilder.find_log_line("Building flashblock 9").await?;
op_rbuilder
.find_log_line("Building flashblock idx=9")
.await?;
op_rbuilder
.find_log_line("Skipping flashblock reached target=10 idx=10")
.await?;
Expand Down
23 changes: 15 additions & 8 deletions crates/op-rbuilder/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use reth_optimism_node::{node::OpAddOnsBuilder, OpNode};
use payload_builder::CustomOpPayloadBuilder;
#[cfg(not(feature = "flashblocks"))]
use payload_builder_vanilla::CustomOpPayloadBuilder;
use reth_optimism_payload_builder::config::{OpBuilderConfig, OpDAConfig};
use reth_transaction_pool::TransactionPool;

/// CLI argument parsing.
Expand Down Expand Up @@ -36,20 +37,26 @@ fn main() {
let rollup_args = builder_args.rollup_args;

let op_node = OpNode::new(rollup_args.clone());
let builder_config = OpBuilderConfig::new(OpDAConfig::default());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like i forgot to save 1 comment
Let's add this field to OpRbuilderConfig directly so we won't have extra staff in here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I maybe misunderstanding, but I'm not sure we should do that. My understanding (please correct me if I'm wrong) is:

  1. OpRbuilderConfig is only used for integration testing (ref) (and is used to setup the builder binary)
  2. We probably don't want to configure this parameter right now, as it's required in every case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you are correct!
I meant to say that we should use structure OpRbuilderArgs
We could add DAConfig field to it, and set it to default (unless configured).
As i see default DAConfig won't have any effect (because it's fields are zeroes)
Then in 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,
                )))

We will pass daconfig from OpRbuilderArgs too, like this

                .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,
                    builder_args.da_config,
                )))


let handle = builder
.with_types::<OpNode>()
.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,
)))
.with_components(op_node.components().payload(
CustomOpPayloadBuilder::with_builder_config(
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,
builder_config.clone(),
),
))
.with_add_ons(
OpAddOnsBuilder::default()
.with_sequencer(rollup_args.sequencer.clone())
.with_enable_tx_conditional(rollup_args.enable_tx_conditional)
.with_da_config(builder_config.da_config)
.build(),
)
.on_node_started(move |ctx| {
Expand Down
62 changes: 56 additions & 6 deletions crates/op-rbuilder/src/payload_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ use reth_optimism_evm::{OpEvmConfig, OpNextBlockEnvAttributes};
use reth_optimism_forks::OpHardforks;
use reth_optimism_node::OpEngineTypes;
use reth_optimism_payload_builder::{
config::{OpBuilderConfig, OpDAConfig},
error::OpPayloadBuilderError,
payload::{OpBuiltPayload, OpPayloadBuilderAttributes},
};
Expand Down Expand Up @@ -96,6 +97,7 @@ struct FlashblocksMetadata<N: NodePrimitives> {
pub struct CustomOpPayloadBuilder {
#[expect(dead_code)]
builder_signer: Option<Signer>,
builder_config: OpBuilderConfig,
flashblocks_ws_url: String,
chain_block_time: u64,
flashblock_block_time: u64,
Expand All @@ -111,9 +113,30 @@ impl CustomOpPayloadBuilder {
flashblocks_ws_url: String,
chain_block_time: u64,
flashblock_block_time: u64,
) -> Self {
Self::with_builder_config(
builder_signer,
extra_block_deadline,
enable_revert_protection,
flashblocks_ws_url,
chain_block_time,
flashblock_block_time,
Default::default(),
)
}

pub fn with_builder_config(
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,
builder_config: OpBuilderConfig,
) -> Self {
Self {
builder_signer,
builder_config,
flashblocks_ws_url,
chain_block_time,
flashblock_block_time,
Expand Down Expand Up @@ -145,6 +168,7 @@ where
) -> eyre::Result<Self::PayloadBuilder> {
Ok(OpPayloadBuilder::new(
OpEvmConfig::optimism(ctx.chain_spec()),
self.builder_config,
pool,
ctx.provider().clone(),
self.flashblocks_ws_url.clone(),
Expand Down Expand Up @@ -236,6 +260,8 @@ pub struct OpPayloadBuilder<Pool, Client> {
pub pool: Pool,
/// Node client
pub client: Client,
/// Settings for the builder, e.g. DA settings.
pub config: OpBuilderConfig,
/// Channel sender for publishing messages
pub tx: mpsc::UnboundedSender<String>,
/// chain block time
Expand All @@ -252,6 +278,7 @@ impl<Pool, Client> OpPayloadBuilder<Pool, Client> {
/// `OpPayloadBuilder` constructor.
pub fn new(
evm_config: OpEvmConfig,
builder_config: OpBuilderConfig,
pool: Pool,
client: Client,
flashblocks_ws_url: String,
Expand All @@ -269,6 +296,7 @@ impl<Pool, Client> OpPayloadBuilder<Pool, Client> {

Self {
evm_config,
config: builder_config,
pool,
client,
tx,
Expand Down Expand Up @@ -390,6 +418,7 @@ where
let ctx = OpPayloadBuilderCtx {
evm_config: self.evm_config.clone(),
chain_spec: self.client.chain_spec(),
da_config: self.config.da_config.clone(),
config,
evm_env,
block_env_attributes,
Expand Down Expand Up @@ -437,9 +466,15 @@ where
}

let gas_per_batch = ctx.block_gas_limit() / self.flashblocks_per_block;

let mut total_gas_per_batch = gas_per_batch;

let da_per_batch = if let Some(da_limit) = ctx.da_config.max_da_block_size() {
Some(da_limit / self.flashblocks_per_block)
} else {
None
};
let mut total_da_per_batch = da_per_batch;

let mut flashblock_count = 0;
// Create a channel to coordinate flashblock building
let (build_tx, mut build_rx) = mpsc::channel(1);
Expand Down Expand Up @@ -505,12 +540,12 @@ where
continue;
}

// Continue with flashblock building
tracing::info!(
target: "payload_builder",
"Building flashblock {} {}",
"Building flashblock idx={} target_gas={} taget_da={}",
flashblock_count,
total_gas_per_batch,
total_da_per_batch.unwrap_or(0),
);

let flashblock_build_start_time = Instant::now();
Expand All @@ -537,6 +572,7 @@ where
&mut db,
best_txs,
total_gas_per_batch.min(ctx.block_gas_limit()),
total_da_per_batch,
)?;
ctx.metrics
.payload_tx_simulation_duration
Expand Down Expand Up @@ -587,12 +623,23 @@ where
.payload_num_tx
.record(info.executed_transactions.len() as f64);

tracing::info!(
target: "payload_builder",
"Built flashblock idx={} target_gas={} gas_used={} taget_da={} da_used={}", flashblock_count,
total_gas_per_batch,
info.cumulative_gas_used,
total_da_per_batch.unwrap_or(0),
info.cumulative_da_bytes_used
);

best_payload.set(new_payload.clone());
// Update bundle_state for next iteration
bundle_state = new_bundle_state;
total_gas_per_batch += gas_per_batch;
if let Some(da_limit) = da_per_batch {
total_da_per_batch = Some(total_da_per_batch.unwrap() + da_limit);
}
flashblock_count += 1;
tracing::info!(target: "payload_builder", "Flashblock {} built", flashblock_count);
}
}
}
Expand Down Expand Up @@ -883,6 +930,8 @@ impl<T: PoolTransaction> OpPayloadTransactions<T> for () {
pub struct OpPayloadBuilderCtx<ChainSpec> {
/// The type that knows how to perform system calls and configure the evm.
pub evm_config: OpEvmConfig,
/// The DA config for the payload builder
pub da_config: OpDAConfig,
/// The chainspec
pub chain_spec: Arc<ChainSpec>,
/// How to build the payload.
Expand Down Expand Up @@ -1142,6 +1191,7 @@ where
Transaction: PoolTransaction<Consensus = OpTransactionSigned>,
>,
batch_gas_limit: u64,
block_da_limit: Option<u64>,
) -> Result<Option<()>, PayloadBuilderError>
where
DB: Database<Error = ProviderError>,
Expand All @@ -1151,7 +1201,7 @@ where
let mut num_txs_simulated = 0;
let mut num_txs_simulated_success = 0;
let mut num_txs_simulated_fail = 0;

let tx_da_limit = self.da_config.max_da_tx_size();
let mut evm = self.evm_config.evm_with_env(&mut *db, self.evm_env.clone());

while let Some(tx) = best_txs.next(()) {
Expand All @@ -1164,7 +1214,7 @@ where
}

// ensure we still have capacity for this transaction
if info.is_tx_over_limits(tx.inner(), batch_gas_limit, None, None) {
if info.is_tx_over_limits(tx.inner(), batch_gas_limit, tx_da_limit, block_da_limit) {
// we can't fit this transaction into the block, so we need to mark it as
// invalid which also removes all dependent transaction from
// the iterator before we can continue
Expand Down
37 changes: 27 additions & 10 deletions crates/op-rbuilder/src/payload_builder_vanilla.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ pub struct CustomOpPayloadBuilder {
builder_signer: Option<Signer>,
extra_block_deadline: std::time::Duration,
enable_revert_protection: bool,
builder_config: OpBuilderConfig,
#[cfg(feature = "flashblocks")]
flashblocks_ws_url: String,
#[cfg(feature = "flashblocks")]
Expand All @@ -90,31 +91,55 @@ impl CustomOpPayloadBuilder {
#[cfg(feature = "flashblocks")]
pub fn new(
builder_signer: Option<Signer>,
config: OpBuilderConfig,
flashblocks_ws_url: String,
chain_block_time: u64,
flashblock_block_time: u64,
) -> Self {
Self {
builder_signer,
config,
flashblocks_ws_url,
chain_block_time,
flashblock_block_time,
}
}

#[cfg(not(feature = "flashblocks"))]
#[allow(dead_code)]
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::with_builder_config(
builder_signer,
extra_block_deadline,
enable_revert_protection,
flashblocks_ws_url,
chain_block_time,
flashblock_block_time,
Default::default(),
)
}

pub fn with_builder_config(
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,
builder_config: OpBuilderConfig,
) -> Self {
Self {
builder_signer,
extra_block_deadline,
enable_revert_protection,
builder_config,
}
}
}
Expand Down Expand Up @@ -143,6 +168,7 @@ where
Ok(OpPayloadBuilderVanilla::new(
OpEvmConfig::optimism(ctx.chain_spec()),
self.builder_signer,
self.builder_config,
pool,
ctx.provider().clone(),
))
Expand Down Expand Up @@ -249,18 +275,9 @@ impl<Pool, Client> OpPayloadBuilderVanilla<Pool, Client> {
pub fn new(
evm_config: OpEvmConfig,
builder_signer: Option<Signer>,
config: OpBuilderConfig,
pool: Pool,
client: Client,
) -> Self {
Self::with_builder_config(evm_config, builder_signer, pool, client, Default::default())
}

pub fn with_builder_config(
evm_config: OpEvmConfig,
builder_signer: Option<Signer>,
pool: Pool,
client: Client,
config: OpBuilderConfig,
) -> Self {
Self {
pool,
Expand Down
Loading