From cb23ee284df6ed3fb5bf193525c139a1868325fe Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 31 Aug 2020 18:37:31 +0200 Subject: [PATCH] Fix ApproxRoot Infinite Looping (#7140) (#7199) 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 --- CHANGELOG.md | 1 + types/decimal.go | 9 +++++++-- types/decimal_test.go | 21 ++++++++++++++------- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81d28c953f87..c033832675ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* (types) [\#7038](https://github.com/cosmos/cosmos-sdk/issues/7038) Fix infinite looping of `ApproxRoot` by including a hard-coded maximum iterations limit of 100. * (types) [\#7084](https://github.com/cosmos/cosmos-sdk/pull/7084) Fix panic when calling `BigInt()` on an uninitialized `Int`. * (client) [\#7048](https://github.com/cosmos/cosmos-sdk/issues/7048) Fix client `keys add` failure when generating mnemonic in interactive mode. diff --git a/types/decimal.go b/types/decimal.go index 03e27fc356d4..7d3c97cf769c 100644 --- a/types/decimal.go +++ b/types/decimal.go @@ -16,13 +16,16 @@ type Dec struct { *big.Int `json:"int"` } -// number of decimal places const ( + // number of decimal places Precision = 18 // bytes required to represent the above precision // Ceiling[Log2[999 999 999 999 999 999]] DecimalPrecisionBits = 60 + + // max number of iterations in ApproxRoot function + maxApproxRootIterations = 100 ) var ( @@ -339,6 +342,8 @@ func (d Dec) QuoInt64(i int64) Dec { // using Newton's method (where n is positive). The algorithm starts with some guess and // computes the sequence of improved guesses until an answer converges to an // approximate answer. It returns `|d|.ApproxRoot() * -1` if input is negative. +// A maximum number of 100 iterations is used a backup boundary condition for +// cases where the answer never converges enough to satisfy the main condition. func (d Dec) ApproxRoot(root uint64) (guess Dec, err error) { defer func() { if r := recover(); r != nil { @@ -366,7 +371,7 @@ func (d Dec) ApproxRoot(root uint64) (guess Dec, err error) { rootInt := NewIntFromUint64(root) guess, delta := OneDec(), OneDec() - for delta.Abs().GT(SmallestDec()) { + for iter := 0; delta.Abs().GT(SmallestDec()) && iter < maxApproxRootIterations; iter++ { prev := guess.Power(root - 1) if prev.IsZero() { prev = SmallestDec() diff --git a/types/decimal_test.go b/types/decimal_test.go index 48c691a100dc..2d01cd8d1bbe 100644 --- a/types/decimal_test.go +++ b/types/decimal_test.go @@ -451,15 +451,22 @@ func TestApproxRoot(t *testing.T) { root uint64 expected Dec }{ - {OneDec(), 10, OneDec()}, // 1.0 ^ (0.1) => 1.0 - {NewDecWithPrec(25, 2), 2, NewDecWithPrec(5, 1)}, // 0.25 ^ (0.5) => 0.5 - {NewDecWithPrec(4, 2), 2, NewDecWithPrec(2, 1)}, // 0.04 => 0.2 - {NewDecFromInt(NewInt(27)), 3, NewDecFromInt(NewInt(3))}, // 27 ^ (1/3) => 3 - {NewDecFromInt(NewInt(-81)), 4, NewDecFromInt(NewInt(-3))}, // -81 ^ (0.25) => -3 - {NewDecFromInt(NewInt(2)), 2, NewDecWithPrec(1414213562373095049, 18)}, // 2 ^ (0.5) => 1.414213562373095049 - {NewDecWithPrec(1005, 3), 31536000, MustNewDecFromStr("1.000000000158153904")}, + {OneDec(), 10, OneDec()}, // 1.0 ^ (0.1) => 1.0 + {NewDecWithPrec(25, 2), 2, NewDecWithPrec(5, 1)}, // 0.25 ^ (0.5) => 0.5 + {NewDecWithPrec(4, 2), 2, NewDecWithPrec(2, 1)}, // 0.04 ^ (0.5) => 0.2 + {NewDecFromInt(NewInt(27)), 3, NewDecFromInt(NewInt(3))}, // 27 ^ (1/3) => 3 + {NewDecFromInt(NewInt(-81)), 4, NewDecFromInt(NewInt(-3))}, // -81 ^ (0.25) => -3 + {NewDecFromInt(NewInt(2)), 2, NewDecWithPrec(1414213562373095049, 18)}, // 2 ^ (0.5) => 1.414213562373095049 + {NewDecWithPrec(1005, 3), 31536000, MustNewDecFromStr("1.000000000158153904")}, // 1.005 ^ (1/31536000) ≈ 1.00000000016 + {SmallestDec(), 2, NewDecWithPrec(1, 9)}, // 1e-18 ^ (0.5) => 1e-9 + {SmallestDec(), 3, MustNewDecFromStr("0.000000999999999997")}, // 1e-18 ^ (1/3) => 1e-6 + {NewDecWithPrec(1, 8), 3, MustNewDecFromStr("0.002154434690031900")}, // 1e-8 ^ (1/3) ≈ 0.00215443469 } + // In the case of 1e-8 ^ (1/3), the result repeats every 5 iterations starting from iteration 24 + // (i.e. 24, 29, 34, ... give the same result) and never converges enough. The maximum number of + // iterations (100) causes the result at iteration 100 to be returned, regardless of convergence. + for i, tc := range testCases { res, err := tc.input.ApproxRoot(tc.root) require.NoError(t, err)