refactor: clean up and improve flashblocks build_payload#260
Conversation
|
This refactoring causes logic change |
|
@SozinM I have fixed the timing, let me know how it looks! |
| ctx: &mut OpPayloadBuilderCtx<FlashblocksExtraCtx>, | ||
| info: &mut ExecutionInfo<ExtraExecutionInfo>, | ||
| total_gas_per_batch: &mut u64, | ||
| total_da_per_batch: &mut Option<u64>, | ||
| builder_tx_da_size: u64, | ||
| builder_tx_gas: u64, | ||
| state: &mut State<DB>, | ||
| best_txs: &mut NextBestFlashblocksTxs<Pool>, | ||
| block_cancel: &CancellationToken, | ||
| flashblocks_per_block: u64, | ||
| message: Vec<u8>, | ||
| best_payload: &BlockCell<OpBuiltPayload>, | ||
| gas_per_batch: u64, | ||
| da_per_batch: Option<u64>, | ||
| span: &tracing::Span, |
There was a problem hiding this comment.
the gas / da / config variables can be part of the flashblocks ctx I feel. It would reduce the number of arguments here
| builder_tx_da_size: u64, | ||
| builder_tx_gas: u64, |
There was a problem hiding this comment.
there shouldn't be a need to pass this in, can be recalculated for each flashblock. in the future different builder transactions will be inserted depending on whether its the first / last flashblock so it needs to be dynamically calculated per flashblock
| total_gas_per_batch: u64, | ||
| total_da_per_batch: Option<u64>, | ||
| gas_per_batch: u64, | ||
| da_per_batch: Option<u64>, |
There was a problem hiding this comment.
could you add some comments around what the different fields are here
| if let Some(da_limit) = total_da_per_batch.as_mut() { | ||
| *da_limit = da_limit.saturating_sub(builder_tx_da_size); | ||
| } | ||
| ctx.extra_ctx.total_da_per_batch = total_da_per_batch; |
There was a problem hiding this comment.
why not directly assign ctx.extra_ctx.total_da_per_batch to da_per_batch? Same with the other ctx.extra_ctx assignments, may be worth adding a method for modifying these fields here
| } | ||
| } | ||
|
|
||
| ctx.extra_ctx.total_gas_per_batch = total_gas_per_batch; |
There was a problem hiding this comment.
can simplify with ctx.extra_ctx.total_gas_per_batch += ctx.extra_ctx.gas_per_batch;
| .unwrap_or(0); | ||
|
|
||
| // Continue with flashblock building | ||
| let mut total_gas_per_batch = ctx.extra_ctx.total_gas_per_batch; |
There was a problem hiding this comment.
nit: should be named target_gas_for_batch
avalonche
left a comment
avalonche
left a comment
There was a problem hiding this comment.
some nits, great work!
3 participants
📝 Summary
build_payloadinto separate functions for readability, specifically moved the code executed within the loop to build a flashblock to its ownbuild_next_flashblockfn (didn't change any logic here)spawn_timer_taskin favour of select statements with ticker insidebuild_payloadfor clarity💡 Motivation and Context
dug into #209 (this PR also improves the logging) and saw opportunity for clean up
✅ I have completed the following steps:
make lintmake test