refactor(osmomath): limit pow iterations in osmomath #6627

p0mvn · 2023-10-03T13:55:35Z

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.

go test -benchmem -run=^$ -bench ^BenchmarkSlowPowCases$ github.com/osmosis-labs/osmosis/osmomath -count=6
goos: linux
goarch: amd64
pkg: github.com/osmosis-labs/osmosis/osmomath
cpu: 12th Gen Intel(R) Core(TM) i7-1260P
BenchmarkSlowPowCases-16              76          14897602 ns/op         3760805 B/op     168182 allocs/op
BenchmarkSlowPowCases-16              78          14858077 ns/op         3760792 B/op     168181 allocs/op
BenchmarkSlowPowCases-16              79          15163556 ns/op         3760784 B/op     168181 allocs/op
BenchmarkSlowPowCases-16              79          15022864 ns/op         3760810 B/op     168182 allocs/op
BenchmarkSlowPowCases-16              85          15068015 ns/op         3760784 B/op     168182 allocs/op
BenchmarkSlowPowCases-16              68          15159170 ns/op         3760789 B/op     168182 allocs/op
PASS
ok      github.com/osmosis-labs/osmosis/osmomath        7.105s

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.
Was tested by backporting to the v19.x release line

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

github-actions · 2023-10-03T13:55:52Z

Important Notice

This PR modifies an in-repo Go module. It is one of:

osmomath
osmoutils
x/ibc-hooks
x/epochs

The dependent Go modules, especially the root one, will have to be
updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

Open https://github.com/osmosis-labs/osmosis/actions/workflows/go-mod-auto-bump.yml
Provide the current branch name
On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

pysel

looks good to me!

pysel · 2023-10-03T17:50:47Z

osmomath/math.go

@@ -8,6 +8,8 @@ import (
 // TODO: Analyze choice here.
 var powPrecision, _ = NewDecFromStr("0.00000001")

+const powIterationLimit = 800_000


asking to learn: is there a specific reason for this number?

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

czarcas7ic · 2023-10-04T15:05:08Z

x/gamm/pool-models/balancer/pool_suite_test.go

+		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),


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?

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

x/gamm/pool-models/balancer/pool_suite_test.go

mattverse

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

mattverse · 2023-10-05T10:58:18Z

osmomath/math.go

@@ -8,6 +8,8 @@ import (
 // TODO: Analyze choice here.
 var powPrecision, _ = NewDecFromStr("0.00000001")

+const powIterationLimit = 150_000


Curious, how was this chosen? 👀

#6627 (comment)

p0mvn · 2023-10-05T12:50:20Z

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

This is v20.

p0mvn added the V:state/breaking State machine breaking PR label Oct 3, 2023

p0mvn marked this pull request as ready for review October 3, 2023 14:10

pysel approved these changes Oct 3, 2023

View reviewed changes

p0mvn marked this pull request as draft October 4, 2023 09:26

refactor(osmomath): limit pow iterations in osmomath

b489a40

p0mvn force-pushed the roman/limit-num-iterations-pow branch from d7a3a19 to b489a40 Compare October 4, 2023 09:27

refactor(osmomath): limit pow iterations in osmomath

403923d

github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Oct 4, 2023

p0mvn marked this pull request as ready for review October 4, 2023 14:05

p0mvn requested review from mattverse, czarcas7ic and AlpinYukseloglu as code owners October 4, 2023 14:05

p0mvn mentioned this pull request Oct 4, 2023

V20: Scoping Issue #5864

Closed

czarcas7ic reviewed Oct 4, 2023

View reviewed changes

p0mvn commented Oct 4, 2023

View reviewed changes

x/gamm/pool-models/balancer/pool_suite_test.go Outdated Show resolved Hide resolved

Update x/gamm/pool-models/balancer/pool_suite_test.go

fb755ad

czarcas7ic approved these changes Oct 4, 2023

View reviewed changes

mattverse reviewed Oct 5, 2023

View reviewed changes

Merge branch 'main' into roman/limit-num-iterations-pow

d6615cc

p0mvn merged commit a1d76ab into main Oct 6, 2023
1 check passed

p0mvn deleted the roman/limit-num-iterations-pow branch October 6, 2023 15:58

github-actions bot mentioned this pull request Apr 15, 2024

Broken Links Detected #8046

Closed

github-actions bot mentioned this pull request May 1, 2024

Broken Links Detected #8183

Closed

github-actions bot mentioned this pull request May 15, 2024

Broken Links Detected #8269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(osmomath): limit pow iterations in osmomath #6627

refactor(osmomath): limit pow iterations in osmomath #6627

p0mvn commented Oct 3, 2023 •

edited

Loading

github-actions bot commented Oct 3, 2023

pysel left a comment

pysel Oct 3, 2023

p0mvn Oct 4, 2023

czarcas7ic Oct 4, 2023

p0mvn Oct 4, 2023

mattverse left a comment

mattverse Oct 5, 2023

p0mvn Oct 5, 2023

p0mvn commented Oct 5, 2023

refactor(osmomath): limit pow iterations in osmomath #6627

refactor(osmomath): limit pow iterations in osmomath #6627

Conversation

p0mvn commented Oct 3, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

github-actions bot commented Oct 3, 2023

pysel left a comment

Choose a reason for hiding this comment

pysel Oct 3, 2023

Choose a reason for hiding this comment

p0mvn Oct 4, 2023

Choose a reason for hiding this comment

czarcas7ic Oct 4, 2023

Choose a reason for hiding this comment

p0mvn Oct 4, 2023

Choose a reason for hiding this comment

mattverse left a comment

Choose a reason for hiding this comment

mattverse Oct 5, 2023

Choose a reason for hiding this comment

p0mvn Oct 5, 2023

Choose a reason for hiding this comment

p0mvn commented Oct 5, 2023

p0mvn commented Oct 3, 2023 •

edited

Loading