refactor: clean up flashblocks context in payload builder #297
Conversation
noot
requested review from
0x416e746f6e,
avalonche and
karim-agha
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the flashblocks context handling in the payload builder to improve immutability and reduce state mutation. The main change involves modifying build_payload to construct a new FlashblocksExtraCtx for each flashblock rather than mutating a shared context.
- Refactored
build_next_flashblockto return new context instead of mutating existing one - Extracted common payload builder context creation into a dedicated method
- Cleaned up imports and removed unused variables
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/op-rbuilder/src/tests/flashblocks.rs | Removed unused imports (constants and types) |
| crates/op-rbuilder/src/builders/flashblocks/payload.rs | Main refactor implementing immutable flashblocks context pattern with new helper methods |
| crates/op-rbuilder/src/builders/context.rs | Added utility methods for creating new context instances with updated fields |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| .get_op_payload_builder_ctx( | ||
| config.clone(), | ||
| block_cancel.clone(), | ||
| FlashblocksExtraCtx { |
There was a problem hiding this comment.
we could also add
FlashblocksExtraCtx::from_config to simplify it too
There was a problem hiding this comment.
since Default is implemented on FlashblocksExtraCtx I replaced the empty fields w ..Default::default()
|
|
||
| best_payload.set(payload.clone()); | ||
| self.send_payload_to_engine(payload); | ||
| self.send_payload_to_engine(payload.clone()); |
There was a problem hiding this comment.
Don't see any problem with order change
| }; | ||
| let _entered = fb_span.enter(); | ||
|
|
||
| if ctx.flashblock_index() > ctx.target_flashblock_count() { |
There was a problem hiding this comment.
We need to check that it's okay. AFAIK if future is completed before we get engine_getPayload it won't return result to it
There was a problem hiding this comment.
How to check it:
make it so rollup-boost can't connect to flashblocks enpoint (or disable flashblocks for it)
It will fallback to getPayload
There was a problem hiding this comment.
i tested this by removing --flashblocks.enabled for rollup-boost in builder-playground and running with telemetry, everything appears to be the same as before, what should i be looking for specifically to confirm?
| ctx.extra_ctx.target_da_for_batch = target_da_per_batch; | ||
| let target_gas_for_batch = | ||
| ctx.extra_ctx.target_gas_for_batch + ctx.extra_ctx.gas_per_batch; | ||
| let next_extra_ctx = FlashblocksExtraCtx { |
There was a problem hiding this comment.
Also let's make FlashblocksExtraCtx::produce_next(FlashblocksExtraCtx)?
|
Seems good! |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
3 participants
📝 Summary
build_payloadsuch that the usedFlashblocksExtraCtxisn't mutable, but a new one is constructed per each flashblock✅ I have completed the following steps:
make lintmake test