Fix ApproxRoot Infinite Looping #7140

migueldingli1997 · 2020-08-23T22:32:19Z

Description

Closes: #7038

Added a maximum number of iterations (100) to ApproxRoot (and ApproxSqrt) to serve as a hard limit on the number of iterations that the algorithm performs. If the answer converges before the maximum iterations, it returns immediately. Otherwise, if the answer never converges enough to satisfy the primary loop condition, the looping stops after a hundred iterations.

A test case showing a scenario where the answer never converges enough was added, with inline commenting.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-08-23T22:36:40Z

Codecov Report

Merging #7140 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #7140   +/-   ##
=======================================
  Coverage   54.78%   54.78%           
=======================================
  Files         563      563           
  Lines       38624    38624           
=======================================
  Hits        21162    21162           
  Misses      15730    15730           
  Partials     1732     1732

alessio · 2020-08-23T23:18:21Z

If this is accepted as-is, for Launchpad I'll propose a specific non-breaking patch + documentation and appropriate section in the release notes. @clevinson @ethanfrey

alexanderbez · 2020-08-24T01:25:15Z

types/decimal.go

-func (d Dec) ApproxRoot(root uint64) (guess Dec, err error) {
+// The maximum number of iterations is required as a backup boundary condition for
+// cases where the answer never converges enough to satisfy the main condition.
+func (d Dec) ApproxRoot(root, maxIters uint64) (guess Dec, err error) {


I personally don't feel comfortable with this code. If this is necessary by the application, why not just define it there? We don't even use this in the SDK or relevant applications (e.g. gaia and simapp).

I would prefer to remove it outright.

I understand. My reasoning is that it's nice to have root calculation functionality built into the SDK alongside the Dec type, but as you said one could implement it on the app side of things if they need it.

I'm curious if it's the solution approach that you don't feel comfortable with or the function/s in general? I could try a different approach if the former is the case.

If we want to remove it outright, I can get that done instead of the above solution.

I think having a root function is pretty important for a Decimal/Number library, as its a pretty common operation. And implementing a solid version in the library itself would be better than having different applications all having their own potentially broken implementation (oops 😅).

ethanfrey · 2020-08-24T12:32:22Z

When this is resolved, one way or the other, I agree with merging it into launchpad (after another review pass).
Having a potential infinite loop seems like a good bug to patch.

sunnya97 · 2020-08-25T04:29:52Z

This seems like a good solution to me. Do we want the end users to provide a MaxIterations, or just hardcode a reasonable default? Requiring them to provide it would need the user to understand the inner workings of how the function calculates roots, which seems unnecessary.

migueldingli1997 · 2020-08-25T12:03:46Z

This seems like a good solution to me. Do we want the end users to provide a MaxIterations, or just hardcode a reasonable default? Requiring them to provide it would need the user to understand the inner workings of how the function calculates roots, which seems unnecessary.

I had these thoughts as well actually but my reasoning is that with any other function the user will always need to understand the inner workings to some extent. That being said, from my experimentation (and test cases included in the PR) it seems typically the number of iterations required is in the 10's (< 100), so maybe a default that could be used would be around 100.

Not sure if there's a way in Go to allow the default to be bypassed for extra iterations without having to implement an extra function or complicating things elsehow.

alessio · 2020-08-25T12:25:00Z

For what concerns Launchpad, I'd love to avoid breaking API and ABI, thus the folllowing:

just hardcode a reasonable default?

might actually be a reasonable fix IMHO. Wdyt? @ethanfrey @clevinson

ethanfrey · 2020-08-25T16:40:18Z

t seems typically the number of iterations required is in the 10's (< 100), so maybe a default that could be used would be around 100.

Sounds good. Same API, just hardcoded max of 100. Avoids an attack vector and I doubt any real case will be too far off reality after 100 steps

… inline doc and CHANGELOG accordingly.

migueldingli1997 · 2020-08-26T08:50:21Z

I've updated the code and doc to instead use a hard-coded maximum number of iterations. Please check it out :)

types/decimal.go

Co-authored-by: Alessio Treglia <[email protected]>

alessio

Looks good to go! Thanks @migueldingli1997!

migueldingli1997 · 2020-08-26T16:33:46Z

Looks good to go! Thanks @migueldingli1997!

No problem. I just fixed 10 to 100

amaury1093

LGTM 👍

alessio · 2020-08-27T19:31:43Z

@alexanderbez is in favor of removing the ApproxRoot function from the SDK altogether, please see here #7140 (comment). His main argument is the SDK does not use it, thus we should remove it [1]

Whilst I acknowledge and partly agree with @alexanderbez's point, I suppose that ApproxRoot could potentially be useful to applications and should be maintained in the SDK as long as the current implementation is fixed. Either way, it's a nice to have and definitely not a big deal.

@aaronc @clevinson I'd like to hear from you guys what you think!

[1] Bez any mischaracterization is purely incidental, so please elaborate further if you believe I misunderstood your message :-)

clevinson · 2020-08-27T20:49:06Z

Looks good to me, I'm in agreement that an ApproxRoot method is useful to have with a decimal library, but its also confusing to me why cosmos needs its own implementaiton of decimals in general... Not trying to dig issue up again here, but that's my perspective.

Given that we have Dec in the sdk, i think having ApproxRoot there as well (with the solution of hardcoding a max iterations in this PR) feels like the right approach.

alessio · 2020-08-28T00:11:15Z

@clevinson dixit:

its also confusing to me why cosmos needs its own implementaiton of decimals in general

I hear you brother, I hear you... (cit.)

But that's a way longer conversation...

aaronc · 2020-08-28T00:41:15Z

This sounds fine for now. We should consider something like https://github.com/cockroachdb/apd long term

alessio · 2020-08-28T06:53:20Z

@alexanderbez Considering #7140 (comment) and #7140 (comment), could we merge this in now then? Is there any other concerns do you reckon we should address before merging this in?

alexanderbez · 2020-08-28T16:08:09Z

No need to rest on my approval here. I still think we don't need half-baked APIs like this, but if there is a strong need for it, merge away 👍

As per #7140 (comment)

Added a maximum number of iterations (100) to ApproxRoot (and ApproxSqrt) to serve as a hard limit on the number of iterations that the algorithm performs. If the answer converges before the maximum iterations, it returns immediately. Otherwise, if the answer never converges enough to satisfy the primary loop condition, the looping stops after a hundred iterations. Closes: #7038

Added a maximum number of iterations (100) to ApproxRoot (and ApproxSqrt) to serve as a hard limit on the number of iterations that the algorithm performs. If the answer converges before the maximum iterations, it returns immediately. Otherwise, if the answer never converges enough to satisfy the primary loop condition, the looping stops after a hundred iterations. Closes: #7038 Co-authored-by: Miguel Dingli <[email protected]>

migueldingli1997 added 2 commits August 24, 2020 00:21

Added maxIters to ApproxRoot and ApproxSqrt. Also added test cases.

77ece2d

Updated CHANGELOG.md

de4ea1d

migueldingli1997 requested review from cwgoes and sunnya97 as code owners August 23, 2020 22:32

migueldingli1997 changed the title ~~Miguel/7038 approx root infinite loop fix~~ Fix ApproxRoot Infinite Looping Aug 23, 2020

alessio added the backport/0.39.x (Launchpad) label Aug 23, 2020

alexanderbez previously requested changes Aug 24, 2020

View reviewed changes

Merge branch 'master' into miguel/7038-approx-root-infinite-loop-fix

6942cd1

migueldingli1997 requested review from aaronc and alessio as code owners August 24, 2020 15:29

migueldingli1997 requested a review from alexanderbez August 24, 2020 15:30

Merge branch 'master' into miguel/7038-approx-root-infinite-loop-fix

0edd9a5

migueldingli1997 added 2 commits August 26, 2020 10:47

Removed maxIters as argument and hard-coded it in ApproxRoot. Changed…

d0ef6a6

… inline doc and CHANGELOG accordingly.

Removed unnecessary uint64 cast.

fbf6be8

migueldingli1997 and others added 3 commits August 26, 2020 10:51

Minor inline doc change.

69366d8

Merge branch 'master' into miguel/7038-approx-root-infinite-loop-fix

e56c129

Merge branch 'master' into miguel/7038-approx-root-infinite-loop-fix

ccb4c1b

alessio reviewed Aug 26, 2020

View reviewed changes

types/decimal.go Outdated Show resolved Hide resolved

alessio reviewed Aug 26, 2020

View reviewed changes

types/decimal.go Outdated Show resolved Hide resolved

migueldingli1997 and others added 3 commits August 26, 2020 18:14

Update types/decimal.go

3825d86

Co-authored-by: Alessio Treglia <[email protected]>

Update types/decimal.go

390efab

Co-authored-by: Alessio Treglia <[email protected]>

Update decimal.go

c21b001

alessio approved these changes Aug 26, 2020

View reviewed changes

alessio requested review from fedekunze, amaury1093 and jgimeno August 26, 2020 16:28

Fixed iterations 10 -> 100

6abf006

Merge branch 'master' into miguel/7038-approx-root-infinite-loop-fix

50a69a9

amaury1093 approved these changes Aug 27, 2020

View reviewed changes

Alessio Treglia added 2 commits August 27, 2020 17:55

Merge branch 'master' into miguel/7038-approx-root-infinite-loop-fix

32c4c58

Merge branch 'master' into miguel/7038-approx-root-infinite-loop-fix

e4f238d

Merge branch 'master' into miguel/7038-approx-root-infinite-loop-fix

8040aec

alessio merged commit 018915b into cosmos:master Aug 29, 2020

alessio mentioned this pull request Aug 29, 2020

cherry-pick #7140: Fix ApproxRoot Infinite Looping #7199

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ApproxRoot Infinite Looping #7140

Fix ApproxRoot Infinite Looping #7140

migueldingli1997 commented Aug 23, 2020 •

edited

Loading

codecov bot commented Aug 23, 2020 •

edited

Loading

alessio commented Aug 23, 2020

alexanderbez Aug 24, 2020 •

edited

Loading

migueldingli1997 Aug 24, 2020 •

edited

Loading

migueldingli1997 Aug 24, 2020

sunnya97 Aug 25, 2020

ethanfrey commented Aug 24, 2020

sunnya97 commented Aug 25, 2020

migueldingli1997 commented Aug 25, 2020 •

edited

Loading

alessio commented Aug 25, 2020

ethanfrey commented Aug 25, 2020

migueldingli1997 commented Aug 26, 2020

alessio left a comment

migueldingli1997 commented Aug 26, 2020

amaury1093 left a comment

alessio commented Aug 27, 2020 •

edited

Loading

clevinson commented Aug 27, 2020

alessio commented Aug 28, 2020

aaronc commented Aug 28, 2020

alessio commented Aug 28, 2020

alexanderbez commented Aug 28, 2020

Fix ApproxRoot Infinite Looping #7140

Fix ApproxRoot Infinite Looping #7140

Conversation

migueldingli1997 commented Aug 23, 2020 • edited Loading

Description

codecov bot commented Aug 23, 2020 • edited Loading

Codecov Report

alessio commented Aug 23, 2020

alexanderbez Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

migueldingli1997 Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

migueldingli1997 Aug 24, 2020

Choose a reason for hiding this comment

sunnya97 Aug 25, 2020

Choose a reason for hiding this comment

ethanfrey commented Aug 24, 2020

sunnya97 commented Aug 25, 2020

migueldingli1997 commented Aug 25, 2020 • edited Loading

alessio commented Aug 25, 2020

ethanfrey commented Aug 25, 2020

migueldingli1997 commented Aug 26, 2020

alessio left a comment

Choose a reason for hiding this comment

migueldingli1997 commented Aug 26, 2020

amaury1093 left a comment

Choose a reason for hiding this comment

alessio commented Aug 27, 2020 • edited Loading

clevinson commented Aug 27, 2020

alessio commented Aug 28, 2020

aaronc commented Aug 28, 2020

alessio commented Aug 28, 2020

alexanderbez commented Aug 28, 2020

migueldingli1997 commented Aug 23, 2020 •

edited

Loading

codecov bot commented Aug 23, 2020 •

edited

Loading

alexanderbez Aug 24, 2020 •

edited

Loading

migueldingli1997 Aug 24, 2020 •

edited

Loading

migueldingli1997 commented Aug 25, 2020 •

edited

Loading

alessio commented Aug 27, 2020 •

edited

Loading