perf: remove unnecessary clones in payload building#392
Conversation
0xl3on
requested review from
0x416e746f6e,
SozinM,
avalonche and
karim-agha
as code owners
julio4
left a comment
julio4
left a comment
There was a problem hiding this comment.
Thank you for the contribution, looks solid! If you want to look into logs bloom feel free else we can merge it.
| let receipts_root = calculate_receipt_root_no_memo_optimism( | ||
| &info.receipts, | ||
| &ctx.chain_spec, | ||
| ctx.attributes().timestamp(), | ||
| ); | ||
|
|
||
| let receipts_root = execution_outcome | ||
| .generic_receipts_root_slow(block_number, |receipts| { | ||
| calculate_receipt_root_no_memo_optimism( | ||
| receipts, | ||
| &ctx.chain_spec, | ||
| ctx.attributes().timestamp(), | ||
| ) | ||
| }) | ||
| .ok_or_else(|| { | ||
| PayloadBuilderError::Other( | ||
| eyre::eyre!( | ||
| "receipts and block number not in range, block number {}", | ||
| block_number | ||
| ) | ||
| .into(), | ||
| ) | ||
| })?; | ||
| let logs_bloom = execution_outcome | ||
| .block_logs_bloom(block_number) | ||
| .ok_or_else(|| { | ||
| PayloadBuilderError::Other( | ||
| eyre::eyre!( | ||
| "logs bloom and block number not in range, block number {}", | ||
| block_number | ||
| ) | ||
| .into(), | ||
| ) | ||
| })?; | ||
| let logs_bloom = alloy_primitives::logs_bloom(info.receipts.iter().flat_map(|r| r.logs())); |
There was a problem hiding this comment.
I think we could try to compute logs bloom first with &info.receipts.with_bloom_ref() / ReceiptWithBloom and use calculate_receipt_root_optimism with memoized logs blooms to not compute it twice.
There was a problem hiding this comment.
I think we could try to compute logs bloom first with
&info.receipts.with_bloom_ref()/ ReceiptWithBloom and usecalculate_receipt_root_optimismwith memoized logs blooms to not compute it twice.
Yes, good idea
I'll open a PR there suggesting changing the visibility, because atm its pub(crate) (https://github.com/ethereum-optimism/optimism/blame/2eb7e3c074b6b77cf94956529508ec882b0b5f18/rust/op-reth/crates/consensus/src/proof.rs#L12)
So I think for now we can merge this and once its merged there I update here?
edit: saw you opened the PR already!
There was a problem hiding this comment.
yeah found out the same. Sorry for the frontrun
Co-authored-by: Julio <30329843+julio4@users.noreply.github.com>
2 participants
📝 Summary
Closes #391
Remove some allocations while still reducing LoC
✅ I have completed the following steps:
make lintmake test