-
Notifications
You must be signed in to change notification settings - Fork 266
fix: relax L1GasPriceProvider sample sequencing requirements #3197
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
base: main
Are you sure you want to change the base?
Conversation
|
This looks more like a workaround than a fix imho. Any particular reason why relaxing the consecutive block requirement was chosen as the solution? |
Reorgs (IOW we can't really assume blocks don't repeat), and security - I don't think we should assume invariants on data coming from outside... |
I've been avoiding any sort of arbitrary assumptions. Instead, I've used Apollo as source of truth to make decisions. (Not to mention the inconsistencies that could silently sneak in with the proposed solution) Having said that, I looked at what Apollo does. And it seems like they do indeed enforce consecutiveness. If you try to add a non-consecutive block to the buffer they reject with an error. When that happens, their scrapper fetches the missing data to fill in those gaps. Wrt reorgs, they check for parent hash mismatches on every iteration. If a reorg is detected, they'll just reset the whole thing (scrapper loop + provider) and build it back up again. It's simpler for them to handle these "failure" scenarios because they're just polling in a loop, they don't have an ongoing subscription to L1 data like we do. So our solution probably requires a bit more care and love. But should still be very doable. Happy to help. |
Well, we can't use the Apollo code as is (as you say, they have a different communication framework, for example), and yes, a different implementation will lead to inconsistencies, but I'm afraid that's inevitable - not even different instances of Apollo will be consistent with each other (they won't compute the same price if they have different samples), and I think the problems that might cause will just have to be seen before they're solved...
Hmm, where is that?
I don't think we should do that. |
Hence why I said our solution requires a bit more care and love. Again, happy to help.
I disagree here. This all looks quite deterministic to me if done right. Regardless, they do allow for some wiggle room in the validation later.
Find the relevant check here. Then, the scrapper will fail early here without incrementing the block counter, thus fetching the same block in the next iteration.
Not sure what to say here. Feel free to assign this to me if you're not comfortable with it. Happy to take it while the L2 validation SNIP is being finalized. |
Fixes #3196 , by allowing unordered and repeated gas price samples.