fix(math): revert to correct version of ApproxRoot from a former state breaking change #17725

p0mvn · 2023-09-13T17:22:25Z

This PR fixes a state break that was introduced in a performanice optimization
PR #16141 from May 2023 (4 months ago)

Upon Osmosis switching to math/1.1.2, we started getting app hashes in the module responsible for swaps.

Debugging has led to realizing that old ApproxRoot and new ApproxRoot produced different results.

For example, before the change in this PR, we would have a sqrt producing:

base : "1.000000011823380862"
output: "1.000000005911690413"
After:

base: "1.000000011823380862"
output: "1.000000005911690414"
The discrepancy stems from this PR: #16141 which was later osmosis-labs#468 due to the state break but not in the upstream (here).

The change in this PR essentially copies: osmosis-labs#468
It also adds a testcase proving that the state-break has been fixed.

This reverts commit 952328a.

This reverts commit 2fd310f.

p0mvn · 2023-09-13T22:16:17Z

Attempted to revert #17497 because suspected it as another state-break while testing. The suspicion proved to be true.

Opened a separate PR to revert it: #17736

As of right now, 2fd310f is state-compatible with the math history

facundomedica · 2023-09-14T12:34:12Z

utACK. This has been in math since math/v1.0.1.
Q for other reviewers: How do we handle this? Do we tag a new release and that's it? Or should we also retract versions in between?

odeke-em · 2023-09-15T16:28:37Z

math/dec.go

-		if root != 2 {
-			prev = guess.Power(root - 1)
-		}
+	for iter := 0; delta.Abs().GT(LegacySmallestDec()) && iter < maxApproxRootIterations; iter++ {


We can start by cheaper comparisons firstly of just integers then onto the .Abs().GT so

for iter := 0; iter < maxApproxRootIterations && delta.Abs().GT(legacySmallestDec); iter++ {

and even more LegacySmallestDec() never changes so we can initiate it as a global so

var legacySmallestDec = LegacySmallestDec()

odeke-em · 2023-09-15T20:19:39Z

utACK. This has been in math since math/v1.0.1. Q for other reviewers: How do we handle this? Do we tag a new release and that's it? Or should we also retract versions in between?

The verdict @facundomedica: we can't just retract the v1.0.1 cosmossdk.io/math module because:
There are 1,000+ importers of the v1.0.1 math module
ApproxRoot needs more testing, fuzzing and fixes hence forward rolls would be much better
Survey_on_code_using_cosmossdk.iomath.LegacyDec.approxroot.pdf

julienrbrt

Discussed internally with Orijtech.
We will most probably merge this and contact the one party depending on the reverted behavior (Lava).
Could you make a changelog a bit more descriptive?

p0mvn · 2023-09-16T00:47:06Z

@julienrbrt changelog improved. Could you please also re-ACK the extra commits I added post approval?

They should not affect state compatibility but keep performance benefits

odeke-em

LGTM, thank you @p0mvn!

odeke-em · 2023-09-18T10:39:55Z

@p0mvn could we please update the title to something more descriptive about the fix like

fix(math): revert to correct version of ApproxRoot from a former state breaking change

and then the commit message and PR message to

This PR fixes a state break that was introduced in a performanice optimization
PR #16141 from May 2023 (4 months ago)

Upon Osmosis switching to math/1.1.2, we started getting app hashes in the module responsible for swaps.

Debugging has led to realizing that old ApproxRoot and new ApproxRoot produced different results.

For example, before the change in this PR, we would have a sqrt producing:

base : "1.000000011823380862"
output: "1.000000005911690413"
After:

base: "1.000000011823380862"
output: "1.000000005911690414"
The discrepancy stems from this PR: https://github.com/cosmos/cosmos-sdk/pull/16141 which was later https://github.com/osmosis-labs/cosmos-sdk/pull/468 due to the state break but not in the upstream (here).

The change in this PR essentially copies: https://github.com/osmosis-labs/cosmos-sdk/pull/468
It also adds a testcase proving that the state-break has been fixed.

julienrbrt · 2023-09-18T14:19:25Z

Tagged a rc, so you can already integrate: https://github.com/cosmos/cosmos-sdk/releases/tag/math%2Fv1.1.3-rc.0
Final tag will follow close before v0.50 release once we have decided if we retract versions or not.

…ions This fuzzer performs a roundtrip comparison for square roots comparing the value from: * squaring LegacyDec.ApproxRoot(2) against itself then checking if the value is very close to the original whole value * comparing against the Go standard library's math/big.Float.Sqrt and panicking if we find a deviation of greater than 5% It's great to note that the results from this library so far from fuzzing show a higher precision/accuracy than math/big.Float.Sqrt for example with: LegacyDec.ApproxRoot(100000000000000000.000000000000000000, 2) Stdlib sqrt: 316227766.000000000000000000 cosmossdk.io/math.*Dec.ApproxSqrt: 316227766.016837933199889354 Python3.7.6: math.sqrt(100000000000000000): 316227766.01683795 LegacyDec.ApproxRoot(10000700000000000.000000000000000000, 2) Stdlib sqrt: 100003499.900000000000000000 cosmossdk.io/math.*Dec.ApproxSqrt: 100003499.938752143656215533 Python3.7.6: math.sqrt(10000700000000000): 100003499.93875214 Updates PR #16141 Updates PR #17725

p0mvn added 4 commits September 13, 2023 13:15

fix: state-break in ApproxRoot

ca09655

changelog

dd04717

updates

35f2110

update

800b3d4

julienrbrt changed the title ~~fix!: state-break in ApproxRoot~~ fix(math)!: state-break in ApproxRoot Sep 13, 2023

Revert "perf: optimize math.Int.Size for small values (#17497)"

2fd310f

This reverts commit 952328a.

p0mvn mentioned this pull request Sep 13, 2023

fix(math): Revert "perf: optimize math.Int.Size for small values (#17497)" #17736

Merged

20 tasks

p0mvn added 2 commits September 13, 2023 18:12

Revert "Revert "perf: optimize math.Int.Size for small values (#17497)""

b2f982b

This reverts commit 2fd310f.

updates

26e36e3

p0mvn marked this pull request as ready for review September 13, 2023 22:23

p0mvn requested a review from a team as a code owner September 13, 2023 22:23

github-prbot requested review from a team, kocubinski and samricotta and removed request for a team September 13, 2023 22:23

tac0turtle assigned facundomedica and testinginprod Sep 14, 2023

odeke-em reviewed Sep 15, 2023

View reviewed changes

julienrbrt approved these changes Sep 15, 2023

View reviewed changes

p0mvn added 2 commits September 15, 2023 19:27

avoid redundant reallocation

9a39ad9

better changelog

4e85854

odeke-em mentioned this pull request Sep 15, 2023

test(math): add fuzzer for LegacyDec.ApproxRoot to catch stark deviations #17770

Closed

p0mvn added 2 commits September 15, 2023 20:29

Merge branch 'main' into roman/revert-approx-root-state-break

db87bda

updates

f4dff23

p0mvn requested a review from julienrbrt September 16, 2023 00:47

p0mvn requested a review from odeke-em September 16, 2023 00:47

odeke-em approved these changes Sep 16, 2023

View reviewed changes

p0mvn changed the title ~~fix(math)!: state-break in ApproxRoot~~ fix(math): revert to correct version of ApproxRoot from a former state breaking change Sep 18, 2023

julienrbrt approved these changes Sep 18, 2023

View reviewed changes

julienrbrt added this pull request to the merge queue Sep 18, 2023

Merged via the queue into main with commit 267cd15 Sep 18, 2023
4 checks passed

julienrbrt deleted the roman/revert-approx-root-state-break branch September 18, 2023 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(math): revert to correct version of ApproxRoot from a former state breaking change #17725

fix(math): revert to correct version of ApproxRoot from a former state breaking change #17725

p0mvn commented Sep 13, 2023 •

edited

Loading

p0mvn commented Sep 13, 2023

facundomedica commented Sep 14, 2023

odeke-em Sep 15, 2023

p0mvn Sep 16, 2023

odeke-em commented Sep 15, 2023

julienrbrt left a comment •

edited

Loading

p0mvn commented Sep 16, 2023

odeke-em left a comment

odeke-em commented Sep 18, 2023

julienrbrt commented Sep 18, 2023

fix(math): revert to correct version of ApproxRoot from a former state breaking change #17725

fix(math): revert to correct version of ApproxRoot from a former state breaking change #17725

Conversation

p0mvn commented Sep 13, 2023 • edited Loading

p0mvn commented Sep 13, 2023

facundomedica commented Sep 14, 2023

odeke-em Sep 15, 2023

Choose a reason for hiding this comment

p0mvn Sep 16, 2023

Choose a reason for hiding this comment

odeke-em commented Sep 15, 2023

julienrbrt left a comment • edited Loading

Choose a reason for hiding this comment

p0mvn commented Sep 16, 2023

odeke-em left a comment

Choose a reason for hiding this comment

odeke-em commented Sep 18, 2023

julienrbrt commented Sep 18, 2023

p0mvn commented Sep 13, 2023 •

edited

Loading

julienrbrt left a comment •

edited

Loading