Check if gas limit is in line with the value registered by the proposer #18
Conversation
src/rpc/utils.rs
Outdated
| // Compute the gas limit of the next block after parent. It aims | ||
| // to keep the baseline gas close to the provided target, and increase it towards | ||
| // the target if the baseline gas is lower. | ||
| // Reference: https://github.com/flashbots/builder/blob/03ee71cf0a344397204f65ff6d3a917ee8e06724/core/utils/gas_limit.go#L8 |
There was a problem hiding this comment.
Same here. Flashbots doesn't really have any authority here. It'd be good to link the 1559 spec where this calculation is defined.
I'm a bit surprised at the - 1 now that I read the spec. Do you happen to grok where it comes from? No big deal if not, just curious if this is cancelling out a minor bug somewhere else. Spec doesn't seem to say anything about it.
... oooh I might know. Where the spec says >= they probably have a >, sloppy but acceptable. Or a spec bug. Or a way to deal with builders which are off-by-one actually, probably with the same root cause. Hm :-/ would've been good to have a comment there. Can we add a comment: // Not in the spec, presumably added to allow an off-by-one error on the builder side. Which imo, totally not okay, builders should just implement the spec correctly, but let's start by understanding where we are.
There was a problem hiding this comment.
The reference here wasn't exactly meant as "authoritative source of truth" but rather the reference implementation that I just ported to rust tbh. Good point thought to invest the time to really understand how the specs map to this implementation and what differences where introduced / for what reasons. I will add the comment and then also added an issue to review / understand this in more depth.
There was a problem hiding this comment.
Your understanding of the "-1" is correct I think. I removed that and used ">=" instead to be closer to the specification.
Could this have been a performance optimization though ?🤔
There was a problem hiding this comment.
Actually I think the logic is actually different from just using >=
In the case where: desired_limit == parent_gas_limit + parent_gas_limit / GAS_LIMIT_BOUND_DIVISOR the max_acceptable_limit (as defined below) will be one less and therefore also the result.
I will keep the logic consistent with the flashbots implementation for now and create an issue to review / understand this apparent difference to the eip reference implementation.
There was a problem hiding this comment.
I also included references both to the flashbots code as well as the eip.
There was a problem hiding this comment.
Created issue for further investigation here: #19
src/rpc/mod.rs
Outdated
| // limit as calculated with a desired limit of 0, and builders which fall | ||
| // back to calculating with the default 30_000_000. | ||
| // TODO: Review if we still need this | ||
| if registered_gas_limit != 0 |
There was a problem hiding this comment.
How about we have two early return OK cases and err becomes the default last return with Err?
For context: here we still nest an if which feels more complex to me. Second, the first if is looking for a positive match but written as a negative.
There was a problem hiding this comment.
Makes sense.
Adjusted accordingly in: 76a7f23
Note: This introduces some inefficiency because we are getting the parent block and doing some gas check on it twice.
See here