-
Notifications
You must be signed in to change notification settings - Fork 131
fix(l1): fix exponential overflow in fake exponential #5093
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
Lines of code reportTotal lines added: Detailed view |
| #[test] | ||
| fn test_fake_exponential_overflow() { | ||
| // With u64 this overflows | ||
| fake_exponential(57532635, 3145728, 3338477); | ||
| } |
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.
Let's add a test with the maximum expected inputs here, for good measure:
fake_exponential(MIN_BASE_FEE_PER_BLOB_GAS, u64::MAX, BLOB_BASE_FEE_UPDATE_FRACTION);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.
This test panics.
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.
Created issue to define a bound #5096
MegaRedHand
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.
I'm pretty sure the u64::MAX isn't reachable in practice, and can be bounded further down. We should check this, but I think it's safe to merge this PR as-is.
| } | ||
| output / denominator | ||
| if (output / denominator) > U256::from(u64::MAX) { | ||
| u64::MAX |
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.
We should panic here, since I'm sure this is unreachable. In a later PR we can update this with an explanation.
| #[test] | ||
| fn test_fake_exponential_overflow() { | ||
| // With u64 this overflows | ||
| fake_exponential(57532635, 3145728, 3338477); | ||
| } |
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.
This test panics.
Motivation
Fake Exponential defined in
block.rscan overflow. As such, we need to use the U256 data type to avoid overflowsDescription