refactor: builder transactions overhaul#549
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors builder-transaction handling from a flag-driven BuilderTransactions trait to a scheduled, position-based BuilderTxSchedule/BuilderTxProducer model, and updates integration tests to match the new behavior (notably: only one mandatory “claim” builder tx, and no fallback claim when flashblock-number tx simulation fails).
Changes:
- Introduces a new builder-tx module split (
env,producer,sim,schedule,impls/*) with dispatch points (TopOfBlock,TopOfFlashblock,BottomOfBlock) and uniform nonce/state chaining. - Rewires payload building and continuous-mode codepaths to call schedule dispatch methods instead of threading positional booleans.
- Updates tests across smoke/revert/flashblocks/flashtestations/DA/miner gas limit/fork expectations to reflect new builder-tx counts and ordering.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/op-rbuilder/src/tests/smoke.rs | Updates tx-count expectations and adjusts max-uncompressed-size test for single claim builder tx. |
| crates/op-rbuilder/src/tests/revert.rs | Updates flashblock tx-count and builder-tx validation for single builder tx. |
| crates/op-rbuilder/src/tests/miner_gas_limit.rs | Updates expected block fill count/math for one builder tx. |
| crates/op-rbuilder/src/tests/forks.rs | Updates blob gas expectations for one builder tx. |
| crates/op-rbuilder/src/tests/flashtestations.rs | Adjusts ordering/count assertions given claim tx is now the sole mandatory builder tx. |
| crates/op-rbuilder/src/tests/flashblocks.rs | Updates flashblock-number contract test expectations for “no fallback claim on failure” behavior. |
| crates/op-rbuilder/src/tests/data_availability.rs | Updates DA-fit expectations due to reduced builder-tx overhead. |
| crates/op-rbuilder/src/flashtestations/builder_tx.rs | Migrates flashtestations tx production to BuilderTxProducer and shared sim/sign helpers. |
| crates/op-rbuilder/src/builder/service.rs | Builds a startup-time BuilderTxSchedule (claim + optional flashblock-number + optional flashtestations). |
| crates/op-rbuilder/src/builder/payload.rs | Integrates schedule dispatch calls and bottom-of-block estimation for budget reservation. |
| crates/op-rbuilder/src/builder/mod.rs | Re-exports new builder-tx API and removes legacy module wiring. |
| crates/op-rbuilder/src/builder/flashblocks_builder_tx.rs | Removes legacy flag-threaded builder-tx implementations. |
| crates/op-rbuilder/src/builder/continuous/publish.rs | Updates generics/wiring after removing BuilderTransactions parameterization. |
| crates/op-rbuilder/src/builder/continuous/interval.rs | Updates state-provider handling to Arc<dyn StateProvider + Send> and new builder-tx hooks. |
| crates/op-rbuilder/src/builder/continuous/candidate_loop.rs | Uses schedule dispatch points and bottom-of-block estimation in continuous candidate loop. |
| crates/op-rbuilder/src/builder/builder_tx/env.rs | Adds minimal BuilderTxEnv for producers. |
| crates/op-rbuilder/src/builder/builder_tx/producer.rs | Defines BuilderTxProducer, SimulatedBuilderTx, and BuilderTxError. |
| crates/op-rbuilder/src/builder/builder_tx/sim.rs | Adds shared simulation/sign/commit helpers and SimulationState type alias. |
| crates/op-rbuilder/src/builder/builder_tx/schedule.rs | Adds schedule/position dispatch and budget reservation logic. |
| crates/op-rbuilder/src/builder/builder_tx/impls/mod.rs | Defines builder-tx producer impl module structure. |
| crates/op-rbuilder/src/builder/builder_tx/impls/claim.rs | Implements claim builder tx producer. |
| crates/op-rbuilder/src/builder/builder_tx/impls/flashblock_number.rs | Implements flashblock-number contract builder tx producer. |
| crates/op-rbuilder/src/builder/builder_tx/mod.rs | Wires new builder-tx module exports and internal helpers. |
| crates/op-rbuilder/src/builder/builder_tx.rs | Removes legacy monolithic builder-tx implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use alloy_eips::Encodable2718; | ||
| use alloy_evm::Database; | ||
| use alloy_primitives::map::HashSet; | ||
| use reth_evm::{Evm, EvmError, InvalidTxError}; |
| use alloy_evm::{Database, rpc::TryIntoTxEnv}; | ||
| use alloy_op_evm::{OpEvm, OpTx}; | ||
| use alloy_primitives::{Address, B256, Bytes, TxKind, map::HashSet}; | ||
| use alloy_sol_types::{ContractError, Revert, SolCall, SolError, SolInterface}; |
| use op_alloy_consensus::OpTypedTransaction; | ||
| use op_alloy_rpc_types::OpTransactionRequest; | ||
| use op_revm::OpTransactionError; | ||
| use reth_evm::{Evm, EvmError, InvalidTxError, precompiles::PrecompilesMap}; |
| #[derive(Debug, thiserror::Error)] | ||
| pub enum InvalidContractDataError { | ||
| #[error("did not find expected logs expected {0:?} but got {1:?}")] | ||
| InvalidLogs(Vec<B256>, Vec<B256>), | ||
| #[error("could not decode output from contract call")] | ||
| OutputAbiDecodeError, | ||
| } | ||
|
|
||
| /// Possible error variants during construction of builder txs. | ||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum BuilderTxError { | ||
| #[error("failed to load account {0}")] | ||
| AccountLoadFailed(Address), | ||
| #[error("failed to sign transaction: {0}")] | ||
| SigningError(secp256k1::Error), | ||
| #[error("contract {0} may be incorrect, invalid contract data: {1}")] | ||
| InvalidContract(Address, InvalidContractDataError), | ||
| #[error("transaction to {0} halted {1:?}")] | ||
| TransactionHalted(Address, OpHaltReason), | ||
| #[error("transaction to {0} reverted {1}")] | ||
| TransactionReverted(Address, Revert), | ||
| #[error("invalid transaction error {0}")] | ||
| InvalidTransactionError(Box<dyn core::error::Error + Send + Sync>), | ||
| #[error("evm execution error {0}")] | ||
| EvmExecutionError(Box<dyn core::error::Error + Send + Sync>), | ||
| #[error(transparent)] | ||
| Other(Box<dyn core::error::Error + Send + Sync>), | ||
| } |
There was a problem hiding this comment.
Is there not alloy types for these?
| fn estimate_builder_tx_gas(&self, input: &[u8]) -> u64 { | ||
| let (zero_bytes, nonzero_bytes) = input.iter().fold((0, 0), |(zeros, nonzeros), &byte| { | ||
| if byte == 0 { | ||
| (zeros + 1, nonzeros) | ||
| } else { | ||
| (zeros, nonzeros + 1) | ||
| } | ||
| }); | ||
|
|
||
| let zero_cost = zero_bytes * 4; | ||
| let nonzero_cost = nonzero_bytes * 16; | ||
|
|
||
| let tokens_in_calldata = zero_bytes + nonzero_bytes * 4; | ||
| let floor_gas = 21_000 + tokens_in_calldata * TOTAL_COST_FLOOR_PER_TOKEN; | ||
|
|
||
| std::cmp::max(zero_cost + nonzero_cost + 21_000, floor_gas) | ||
| } |
There was a problem hiding this comment.
Seems more like min gas limit than estimate tx gas.
Also consider:
let intrinsic_gas = 21_000
+ input
.iter()
.map(|byte| if *byte == 0 { 4 } else { 16 })
.sum::<u64>();
let floor_gas = eip7623::transaction_floor_cost(eip7623::tokens_in_calldata(input));
intrinsic_gas.max(floor_gas)| let tx = num_tx.inspect_err(|e| { | ||
| warn!( | ||
| target: "builder_tx", | ||
| error = %e, | ||
| "flashblocks number contract tx simulation failed" | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
Consider a flashblock_number_tx_failure_total metric
| // in flashblocks the DA quota is divided by the number of flashblocks | ||
| // so we will include only one tx in the block because not all of them | ||
| // will fit within DA quote / flashblocks count. |
| error!( | ||
| target: "payload_builder", | ||
| error = %e, | ||
| position = ?position, | ||
| "Error simulating builder txs" | ||
| ); | ||
| }) |
| use alloy_eips::Encodable2718; | ||
| use alloy_evm::Database; | ||
| use alloy_primitives::map::HashSet; | ||
| use reth_evm::{Evm, EvmError, InvalidTxError}; |
| use op_alloy_consensus::OpTypedTransaction; | ||
| use op_alloy_rpc_types::OpTransactionRequest; | ||
| use op_revm::OpTransactionError; | ||
| use reth_evm::{Evm, EvmError, InvalidTxError, precompiles::PrecompilesMap}; |
Summary
Major overhaul of builder transaction management.
Motivation and Context
Previously, builder transaction code:
top_of_block,is_first_flashblock,is_last_flashblockbools through theBuilderTransactionstrait, each impl re-derived its behavior from flag combinations, and anis_top_of_block: boolon the produced tx decided at commit time what actually landed.This refactor gets us closer to exposes builder transaction functionality as a library, i.e. if other builders want to have additional builder transactions this should make it easier.
What changed
Behavior changes
Previously if the flashblocks number contract tx failed, we would add another claim tx. Now it falls back to not writing anything at all, since the claim tx should be independent of the flashblocks number contract. This affected a lot of the integration tests.
Positional dispatch
Position is now a type, replacing flag threading, and is declared once at startup time:
BuilderTxPositionenum (TopOfBlock/TopOfFlashblock/BottomOfBlock) on aScheduledBuilderTxentry pairs each producer with its dispatch point.BuilderTxScheduleowns the entries and exposes one method per dispatch point (commit_top_of_block,commit_top_of_flashblock,estimate_bottom_of_block,commit_bottom_of_block). The payload builder is responsible for calling the right method at the right moment; no position flags cross the trait boundary.BottomOfBlockschedule entry rather than a field inside the flashblocks types. Nonce chaining between entries is done uniformly by the schedule (each entry's txs are committed to the shared simulation state before the next entry runs).Module split
builder_tx.rs→builder_tx/with one concept per file:env.rs(BuilderTxEnv),sim.rs(simulation state + helpers),producer.rs(trait, artifact, error), schedule.rs (dispatch),impls/claim.rs,impls/flashblock_number.rs.Naming overhaul
"Builder tx" previously meant three different things (the on-chain tx, the component producing it, and the collection of producers). Now:
BuilderTxProducerproducesSimulatedBuilderTxvalues, dispatched by aBuilderTxSchedule.The block claim tx (the tx from the builder's address with the "Block Number: {}" message) is named for what it is:
ClaimBuilderTx, replacing the misleading "fallback"/"base" naming.Error/type spellings converge on Tx (
BuilderTxError), andctx: &BuilderTxEnvparameters are nowenvthroughout.Fixes along the way:
BuilderTxtype parameter is gone fromOpPayloadBuilder.&(was&mut).Future work
The public API is fairly large (see
crates/op-rbuilder/src/builder/mod.rs). Some work will need to go into redesigning this so it's suitable as an rblib feature.