-
Notifications
You must be signed in to change notification settings - Fork 1
rollup-fee for spec=feynman #41
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
Thegaram
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.
lgtm, would be nice to add some tests
frisitano
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.
Looks good, thanks for adding the documentation, that's super insightful and helpful. I added a question inline around the compression_ratio.
src/l1block.rs
Outdated
| // | ||
| // We use the `TX_L1_FEE_PRECISION` to allow fractions. We then divide the overall product | ||
| // by the precision value as well. | ||
| let compression_ratio = |_input: &[u8]| -> U256 { TX_L1_FEE_PRECISION }; |
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.
From my understanding, we will assume the worst case in the prover, i.e. the compression ratio is 1. Is this the case, or will the prover also estimate compression? If the prover logic and execution node logic is different, how will we capture that here? Do we need to feature flag this here, depending on whether we are running as a prover or not?
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.
The prover should not need to estimate compression ratio, specifically because zstd-encoding will add a lot of cycles to our batch prover (which we would like to avoid).
You are probably right, we will eventually need feature flag to either:
- prover: fix upper bound for rollup fee (assuming
compression ratio == 1) - node: calculating the actual rollup fee by estimating the
compression_ratiowith zstd-encoding
And we additionally must then constrain the rollup fee to be at the most the upper bound (in the prover).
Let's keep this PR unmerged until we resolve this conversation.
|
I have some thoughts on how we can address the compression factor for the prover. We introduce a scroll-revm/src/transaction.rs Lines 17 to 24 in 54e4e90
We provide a utility to compute the compression factors for all transactions in the host. These compression factors should be computed by the utility and then added to the witness. The witness is then provided to the guest. Inside the guest, we modify the EVM to enable access to the compression factor for each transaction. Inside of the @roynalnaruto @Thegaram, I'm keen to hear your feedback on this approach. |
|
Superseded by #50. |
Fixes #42.
The PR implements the proposed updates to rollup-fee for
ScrollSpecId::FEYNMAN.Assumption
compression_factor = 1until further notice