feat(execution): binary search fee estimation #2566
Conversation
| /// The margin for the binary search for the minimal L2 gas limit. | ||
| const L2_GAS_SEARCH_MARGIN: GasAmount = GasAmount(100); |
There was a problem hiding this comment.
Question: is this OK? It seems to work well on sepolia at least, and doesn't take a very long time to converge. Making this value smaller gives precision in exchange for performance, whereas making it larger gives performance in exchange for precision.
There was a problem hiding this comment.
We should ask Ariel if this is OK.
| // TODO: g = (1 + EPSILON) * g | ||
| // This step is missing. Check what EPSILON is, then implement it. |
There was a problem hiding this comment.
Question: what is this EPSILON value from the issue? Surely it's not f64::EPSILON, that doesn't really make sense.
There was a problem hiding this comment.
Wasn't there some discussion about Starknet adapting the Ethereum algorithm? Specifically the Reth implementation? AFAIK there was some epsilon there, somewhere, but I can't find it now...
There was a problem hiding this comment.
I think this is an "optimization" over the Ethereum algorithm. I think Ariel's idea was that in most cases refunds should be relatively small so he was expecting that most transactions run OK with a budget ~20% larger than the actual consumption after refunds. The ~20% margin was considered small enough so that it's not worth finding the exact value within that range.
Let us confirm the actual value of EPSILON with Ariel.
| // TODO: Is there a type-safe way to check for this error? | ||
| revert_error.to_string().contains("Out of gas") |
There was a problem hiding this comment.
I think the answer to this is "no". At least I haven't been able to do this any better.
There was a problem hiding this comment.
That's another good question. As far as I understand no, there's no better way since the actual error is converted to a string in the error stack so we're losing typed errors here.
| return Err(error); | ||
| } | ||
| Err(TransactionSimulationError::OutOfGas) => { | ||
| unreachable!("Transaction should not run out of gas with MAX gas limit") |
There was a problem hiding this comment.
I wonder if it's possible to construct a transaction so expensive it would run out even of GasAmount::MAX...
There was a problem hiding this comment.
Plus, that seems to be a trivial DoS vector -- I'm sure there's a per-block gas limit, we should probably just use that.
| fn midpoint(a: GasAmount, b: GasAmount) -> GasAmount { | ||
| let GasAmount(a) = a; | ||
| let GasAmount(b) = b; | ||
| GasAmount(a + (b - a) / 2) |
There was a problem hiding this comment.
This does not handle u64 overflows. I know it's rare, but still, we should be careful.
There was a problem hiding this comment.
I disagree:
ais a validu64.bis a validu64.(b - a) / 2can't underflow due to how the surrounding code is written (i.e.bis always greater thana).a + (b - a) / 2is guaranteed to give a number betweenaandbhence can't overflow.
The code is written this way intentionally to avoid overflows. Let me know if I'm missing something.
Closes #2488.