-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add compute_compressed_size for Galileo #355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CodSpeed Performance ReportMerging #355 will not alter performanceComparing Summary
|
roynalnaruto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness wise I don't really see an issue.
I just want to mention however that based on the current approach, we call compute_compression_ratio and compute_compressed_size for each transaction. Do note that compute_compression_ratio itself also calls compute_compressed_size to compute the ratio.
Effectively, compute_compressed_size is being called twice per transaction, meaning that the zstd-encoding is being performed twice per transaction. As such this is not desirable.
Since the inputs to both calls are different -- tx.input vs rlp-encoded tx, I suppose this is unavoidable, right?
Yes, that was my conclusion. Though in practice, we only need There are some code paths where deciding which fork is active for a transaction is not straightforward (txpool, rpc), so after discussion with @frisitano, I suggest that we merge the current version and keep this as a future optimization. This "double compression" has no impact on proving performance, and can be optimized in reth later without a hard fork. |
This PR bumps revm to include the updated Galileo rollup fee (scroll-tech/scroll-revm#65), and adds logic to compute
compressed_size(rlp(tx))used in the formula.The main changes are:
compute_compressed_sizein addition to the existingcompute_compression_ratio. These both use zstd.compression_ratioargument now take an additionalcompressed_sizeargument.WithCompressionRatio,FromTxWithCompressionRatio) are renamed to*CompressionInfo.Important notes:
compression_ratiois computed on the transaction payload only. On the other hand, Galileo'scompressed_sizeis computed on the full rlp-encoded transaction.