-
Notifications
You must be signed in to change notification settings - Fork 679
Add multi dimensional multi constraint pricer algorithm #4012
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: master
Are you sure you want to change the base?
Add multi dimensional multi constraint pricer algorithm #4012
Conversation
a3cb346 to
5dc32f7
Compare
5dc32f7 to
a7acb86
Compare
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
454b599 to
28a7a5f
Compare
fa8781a to
4f2c34e
Compare
8ec77cd to
0611789
Compare
b14c464 to
46e002a
Compare
0953140 to
a138b23
Compare
a138b23 to
9392bb1
Compare
fd0e681 to
752c710
Compare
arbos/l2pricing/l2pricing.go
Outdated
| const GethBlockGasLimit = 1 << 50 | ||
| const gasConstraintsMaxNum = 20 | ||
| const MaxExponentBips = arbmath.Bips(85_000) | ||
| const multiGasConstraintsMaxNum = 10 |
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.
10 might not be enough.
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.
There is a related issue with retryable redeem: if we have more than 17 multi-gas constraints, storage would charge extra 15000 gas units for chunking (assumption). My suggestion is to put 15 for now and after NIT-4152 remove this limit
arbos/l2pricing/l2pricing.go
Outdated
| func (ps *L2PricingState) MultiGasConstraintsMaxNum() int { | ||
| return multiGasConstraintsMaxNum | ||
| } |
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.
You can make the constant public instead of having this method.
arbos/l2pricing/model.go
Outdated
| } | ||
|
|
||
| // GrowBacklog increases the backlog for the active pricing model. | ||
| func (ps *L2PricingState) GrowBacklog(usedGas uint64, usedMultiGas multigas.MultiGas) error { |
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.
You split some functions into separate grow/shrink, but others continue to use the boolean constant, making the code look strange. I have another suggestion:
| func (ps *L2PricingState) GrowBacklog(usedGas uint64, usedMultiGas multigas.MultiGas) error { | |
| enum BacklogOp int | |
| const ( | |
| Shrink BacklopOp = iota | |
| Grow | |
| ) | |
| func (ps *L2PricingState) UpdateBacklog(op BacklogOp, usedGas uint64, usedMultiGas multigas.MultiGas) error { |
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 can probably even make this parameter generic, but I'm not sure if Go can handle this:
| func (ps *L2PricingState) GrowBacklog(usedGas uint64, usedMultiGas multigas.MultiGas) error { | |
| func (ps *L2PricingState) UpdateBacklog[op BacklogOp](usedGas uint64, usedMultiGas multigas.MultiGas) error { ``` |
arbos/l2pricing/model.go
Outdated
| if usedMultiGas.SingleGas() != usedGas { | ||
| log.Warn("usedGas does not match sum of usedMultiGas", "usedGas", usedGas, "usedMultiGas", usedMultiGas.SingleGas()) | ||
| } |
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.
It is so easy to just ignore this warning. I'd rather remove this and think of a better way of testing that these numbers won't diverge.
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.
Also, by the time we see this warning in production, it will be too late to fix it because we will have to support this divergence forever.
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.
Removed for now, probably there is way to have a debug-only assertion
| // IncrementBacklog increments the constraint backlog based on multi-dimensional gas usage | ||
| func (c *MultiGasConstraint) IncrementBacklog(multiGas multigas.MultiGas) error { | ||
| totalBacklog, err := c.backlog.Get() |
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.
To be consistent with the other models, it would be better to receive the BacklogOp and call applyGasDelta
c8f6c5a to
09d57c1
Compare
Fixes NIT-4069
(Temporary includes #3995)
Depends on:
Changes:
AddToGasPoolto accept multigasGasModelToUseto select a model depending on the configuration