Skip to content

Conversation

@0x00101010
Copy link

@0x00101010 0x00101010 commented Nov 20, 2025

📝 Summary

💡 Motivation and Context

Update to use op-alloy flashblock types. For consistency across repos.


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@0x00101010 0x00101010 marked this pull request as ready for review November 20, 2025 05:27
Copilot AI review requested due to automatic review settings November 20, 2025 05:27
Copilot finished reviewing on behalf of 0x00101010 November 20, 2025 05:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the codebase to use op-alloy flashblock types from the op-alloy-rpc-types-engine crate instead of the custom types from the rollup-boost crate. This change aligns with similar updates in related repositories (rollup-boost PR #441 and reth PR #19608) for consistency across the ecosystem.

Key changes:

  • Replaced rollup_boost::FlashblocksPayloadV1 with op_alloy_rpc_types_engine::OpFlashblockPayload
  • Introduced a temporary convert_receipt function to bridge between reth's OpReceipt and op-alloy's OpReceipt types
  • Changed from HashMap to BTreeMap for storing receipts and account balances in metadata

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
crates/op-rbuilder/src/builders/flashblocks/wspub.rs Updated WebSocket publisher to use OpFlashblockPayload type and updated documentation
crates/op-rbuilder/src/builders/flashblocks/payload_handler.rs Updated execute_flashblock function signature to return OpFlashblockPayload
crates/op-rbuilder/src/builders/flashblocks/payload.rs Major refactoring: added receipt conversion function, replaced custom FlashblocksMetadata struct with OpFlashblockPayloadMetadata, updated payload construction to use new op-alloy types, improved base_fee_per_gas conversion

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +62 to +76
fn convert_receipt(receipt: &OpReceipt) -> op_alloy_consensus::OpReceipt {
match receipt {
OpReceipt::Legacy(r) => op_alloy_consensus::OpReceipt::Legacy(r.clone()),
OpReceipt::Eip2930(r) => op_alloy_consensus::OpReceipt::Eip2930(r.clone()),
OpReceipt::Eip1559(r) => op_alloy_consensus::OpReceipt::Eip1559(r.clone()),
OpReceipt::Eip7702(r) => op_alloy_consensus::OpReceipt::Eip7702(r.clone()),
OpReceipt::Deposit(r) => {
op_alloy_consensus::OpReceipt::Deposit(op_alloy_consensus::OpDepositReceipt {
inner: r.inner.clone(),
deposit_nonce: r.deposit_nonce,
deposit_receipt_version: r.deposit_receipt_version,
})
}
}
}
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The convert_receipt function performs a deep clone of each receipt variant. This could impact performance in scenarios with many transactions. Consider whether a reference-based approach or shared ownership (Arc) could be used instead, especially since receipts are being collected into a BTreeMap at line 1131.

Copilot uses AI. Check for mistakes.
Comment on lines +1130 to +1137
.map(|(tx, receipt)| (tx.tx_hash(), convert_receipt(receipt)))
.collect::<BTreeMap<B256, op_alloy_consensus::OpReceipt>>();
let new_account_balances = state
.bundle_state
.state
.iter()
.filter_map(|(address, account)| account.info.as_ref().map(|info| (*address, info.balance)))
.collect::<HashMap<Address, U256>>();
.collect::<BTreeMap<Address, U256>>();
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

[nitpick] The change from HashMap to BTreeMap for receipts and account balances introduces a performance trade-off. While BTreeMap ensures deterministic ordering (which is likely desirable for serialization), it has O(log n) lookup time compared to HashMap's O(1) average case. If the new OpFlashblockPayloadMetadata type requires ordered maps for compatibility with the op-alloy types, this is correct. Otherwise, consider whether the ordering guarantee is worth the performance cost for large transaction sets.

Copilot uses AI. Check for mistakes.
@SozinM
Copy link
Collaborator

SozinM commented Nov 20, 2025

Not keep on this change, alloy type is heavily intertwined with reth, and they must by in sync -> if you update flashblock structure you will need to wait until reth carves out new release until you could use this, so IMO there is zero justification to place it there
I would rather place it in https://github.com/flashbots/flashblocks/ as separate crate with primitives

.map(|(tx, receipt)| (tx.tx_hash(), receipt.clone()))
.collect::<HashMap<B256, OpReceipt>>();
.map(|(tx, receipt)| (tx.tx_hash(), convert_receipt(receipt)))
.collect::<BTreeMap<B256, op_alloy_consensus::OpReceipt>>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would we use BTreeMap with B256?

Copy link
Author

@0x00101010 0x00101010 Nov 20, 2025

Choose a reason for hiding this comment

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

To help with cases where you need ordered traversal of receipts, perf wise should be ok since this list is not super big

@0x00101010
Copy link
Author

0x00101010 commented Nov 20, 2025

Not keep on this change, alloy type is heavily intertwined with reth, and they must by in sync -> if you update flashblock structure you will need to wait until reth carves out new release until you could use this, so IMO there is zero justification to place it there I would rather place it in https://github.com/flashbots/flashblocks/ as separate crate with primitives

I completely get the concern about tying this to op-alloy releases. But I think there's a good case for unification:

  1. The struct already exists in 4+ separate repos
    Right now it’s duplicated across:
  • base/node-reth, each copy drifts — node-reth still doesn’t have blob_gas_used.
  • flashbots/rollup-boost
  • flashbots/op-rbuilder
  • paradigmxyz/reth
  • and more

The alternative of importing from rollup-boost may create cyclic dependency problems since so much of rollup-boost depends on alloy & reth types already.

  1. All of these consumers already depend on alloy as the standard, we're not introducing a brand new crate

  2. The structure is stable and changes infrequently now, and reth team has been very responsive in updating it (happens within a day for reviewing & cutting releases)

@SozinM
Copy link
Collaborator

SozinM commented Nov 21, 2025

@0x00101010 There is websocket proxy that work with flashblock but do not import anything from reth/alloy
https://github.com/flashbots/rollup-boost/blob/main/crates/websocket-proxy/Cargo.toml

Any infra tool that would need flashblock shouldn't import alloys

@SozinM
Copy link
Collaborator

SozinM commented Nov 21, 2025

I'm up for unification, but i prefer it to be separate crate, without other deps, and esp without heavy deps

@0x00101010
Copy link
Author

0x00101010 commented Nov 21, 2025

@0x00101010 There is websocket proxy that work with flashblock but do not import anything from reth/alloy https://github.com/flashbots/rollup-boost/blob/main/crates/websocket-proxy/Cargo.toml

Any infra tool that would need flashblock shouldn't import alloys

  1. websocket-proxy only uses raw data, compress it and pass around, it doesn't need any of the detailed data structures?
  2. re in another crate, still think importing op-alloy is the cleaner choice:
    a. FlashBlockMetadata contains reference to OpReceipt which is defined in op-alloy, > @0x00101010 There is websocket proxy that work with flashblock but do not import anything from reth/alloy https://github.com/flashbots/rollup-boost/blob/main/crates/websocket-proxy/Cargo.toml

Any infra tool that would need flashblock shouldn't import alloys

  1. websocket-proxy only uses raw data, compress it and pass around, it doesn't need any of the detailed data structures?
  2. re in another crate, still think importing op-alloy is preferable due to:
    a. FlashBlockMetadata contains reference to OpReceipt which is defined in op-alloy, > @0x00101010 There is websocket proxy that work with flashblock but do not import anything from reth/alloy https://github.com/flashbots/rollup-boost/blob/main/crates/websocket-proxy/Cargo.toml

Any infra tool that would need flashblock shouldn't import alloys

  1. websocket-proxy only uses raw data, compress it and pass around, it doesn't need any of the detailed data structures?
  2. re in another crate, still think importing op-alloy is preferable due to:
    a. FlashBlockMetadata contains reference to OpReceipt which is defined in op-alloy. If we create a new standalone crate, we either depend on op-alloy anyway or duplicate those types, which is unnecessary and error-prone.
    c. Having the primitives live in op-alloy means there’s a single canonical representation that other OP tooling can reuse. A separate primitives crate under Flashbots effectively creates a parallel dialect that third parties have to adapt to.

I totally understand the concern around slowing down iteration by putting this in op-alloy. In practice though, we have a safety valve: if we ever need a hot patch, we can fork the repo, make the change, and temporarily depend on that fork via git until an official release is cut. That lets us keep the types canonical in op-alloy without sacrificing our ability to move fast in edge cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants