Cosmos-sdk: ApproxRoot Infinite Looping

[migueldingli1997]

Summary of Bug

Infinite loop when supplying small values to the Dec.ApproxRoot() function, most likely because the SmallestDec boundary is never satisfied. Example input: ApproxRoot(NewDecWithPrec(10000000000, 18), 3)

Version

Commit: 6a7cf4442e340b1878b28552778e42d3c6285624 (master as of writing)

Steps to Reproduce

Add the following test case to TestApproxRoot function in types/decimal_test.go:

• {NewDecWithPrec(10000000000, 18), 3, MustNewDecFromStr("0")},

---

For Admin Use

• [ ] Not duplicate issue
• [ ] Appropriate labels applied
• [ ] Appropriate contributors tagged
• [ ] Contributor assigned/self-assigned

[alexanderbez]

/cc @sunnya97 :(

[alessio]

I'd propose to remove the feature in master and reintroduce it when we have a fix.
For what concerns the Launchpad Release Series, we must communicate this ASAP (and maybe even remove it)

[alexanderbez]

Fine by me -- feel free to remove entirely.

[sunnya97]

@karzak Are you using this function?

[karzak]

No - this doesn't have an impact on Kava.

[migueldingli1997]

I understand why it would be removed, but is there the possibility of restricting it to a known range of valid inputs or a larger error tolerance rather than removing it completely? Or maybe an alternative approach can be found? It is currently used by the bonds module. In any case, I'm happy to see the bug get squished.

[alessio]

As discussed with @migueldingli1997, he'll take a stab at this

[migueldingli1997]

I'm thinking that with any implementation with no hard limit, there might always be the possibility that we get an infinite loop. So in my opinion the simplest solution without messing too much with the current code is to add an extra 'maximum number of iterations' argument which puts a hard limit on the looping. From my experimentation, it seems typically the number of iterations required is in the 10's, so maybe a hard limit that could be used would be 100. Will open a PR with this solution.