-
Notifications
You must be signed in to change notification settings - Fork 607
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
refactor(osmomath): limit pow iterations in osmomath #6627
Conversation
Important Notice This PR modifies an in-repo Go module. It is one of:
The dependent Go modules, especially the root one, will have to be Please follow the instructions below:
Please let us know if you need any help. |
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.
looks good to me!
osmomath/math.go
Outdated
@@ -8,6 +8,8 @@ import ( | |||
// TODO: Analyze choice here. | |||
var powPrecision, _ = NewDecFromStr("0.00000001") | |||
|
|||
const powIterationLimit = 800_000 |
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.
asking to learn: is there a specific reason for this number?
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 actually changed this to a lower value of 150_000
It has shown a 15ms worst case under benchmarks and was tested to be infrequent to occur under normal operations when backporting this to a v19.x release line
d7a3a19
to
b489a40
Compare
name: "11single asset - (almost 1 == tokenIn / liquidity ratio), token in weight is smaller than the other token, with zero spread factor", | ||
spreadFactor: osmomath.MustNewDecFromStr("0"), | ||
poolAssets: []balancer.PoolAsset{ | ||
{ | ||
Token: sdk.NewInt64Coin("uosmo", 500_000), | ||
Weight: osmomath.NewInt(100), | ||
}, | ||
{ | ||
Token: sdk.NewInt64Coin("uatom", 1e12), | ||
Weight: osmomath.NewInt(1000), | ||
}, | ||
}, | ||
// Increasing this by 1 would cause a panic due to pow iteration limit reached | ||
tokensIn: sdk.NewCoins(sdk.NewInt64Coin("uosmo", 499_990)), | ||
expectShares: osmomath.NewIntFromUint64(6_504_012_121_638_943_579), |
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.
Need a little more background here to completely understand. So, now with this change, if a pool gets into this state, the amount of tokens in that can be swapped is reduced due to iteration limits?
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.
The original tokenIn value now causes an iteration limit. The test covering that behavior is kept.
The new token in value of 499990
is the closest one to the old that does not panic
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.
The change lgtm, we didn't include this in a release yet right?
I feel like this change should definitely be state breaking thus should be included in the next major version upgrade
@@ -8,6 +8,8 @@ import ( | |||
// TODO: Analyze choice here. | |||
var powPrecision, _ = NewDecFromStr("0.00000001") | |||
|
|||
const powIterationLimit = 150_000 |
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.
Curious, how was this chosen? 👀
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 is v20. |
Closes: #XXX
What is the purpose of the change
Limits the number of Pow iterations:
To make sure that we don't hit the loop iteration bound too often
under normal conditions, this change was backported to v19.x (mainnet at the time of writing)
and tested for no app-hash over 10000 blocks.
Testing and Verifying
Documentation and Release Note
Unreleased
section ofCHANGELOG.md
?Where is the change documented?
x/{module}/README.md
)