diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 4f447d5f98..3b124e3166 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -28,12 +28,17 @@ public: /** Describes whether and how to enforce this number as an integer. * * - none: No enforcement. The value may vary freely. This is the default. - * - weak: If the absolute value is greater than maxIntValue, valid() will - * return false. - * - strong: Assignment operations will throw if the absolute value is above - * maxIntValue. + * - compatible: If the absolute value is greater than maxIntValue, valid() + * will return false. Needed for backward compatibility with XRP used in + * AMMs, and available for functions that will do their own checking. This + * is the default for automatic conversions from XRPAmount to Number. + * - weak: Like compatible, plus, if the value is unrepresentable (larger + * than maxMantissa), assignment and other operations will throw. + * - strong: Like weak, plus, if the absolute value is invalid (larger than + * maxIntValue), assignment and other operations will throw. This is the + * defalut for automatic conversions from MPTAmount to Number. */ - enum EnforceInteger { none, weak, strong }; + enum EnforceInteger { none, compatible, weak, strong }; private: using rep = std::int64_t; @@ -42,8 +47,7 @@ private: // The enforcement setting is not serialized, and does not affect the // ledger. If not "none", the value is checked to be within the valid - // integer range. With "strong", the checks will be made as automatic as - // possible. + // integer range. See the enum description for more detail. EnforceInteger enforceInteger_ = none; public: @@ -53,8 +57,8 @@ public: constexpr static rep maxMantissa = minMantissa * 10 - 1; static_assert(maxMantissa == 9'999'999'999'999'999LL); - constexpr static rep maxIntValue = maxMantissa / 10; - static_assert(maxIntValue == 999'999'999'999'999LL); + constexpr static rep maxIntValue = maxMantissa / 100; + static_assert(maxIntValue == 99'999'999'999'999LL); // The range for the exponent when normalized constexpr static int minExponent = -32768; @@ -93,6 +97,15 @@ public: bool valid() const noexcept; + bool + representable() const noexcept; + /// Combines setIntegerEnforcement(EnforceInteger) and valid() + bool + valid(EnforceInteger enforce); + /// Because this function is const, it should only be used for one-off + /// checks + bool + valid(EnforceInteger enforce) const; constexpr Number operator+() const noexcept; diff --git a/include/xrpl/protocol/XRPAmount.h b/include/xrpl/protocol/XRPAmount.h index 159174accc..8f553a6083 100644 --- a/include/xrpl/protocol/XRPAmount.h +++ b/include/xrpl/protocol/XRPAmount.h @@ -143,7 +143,7 @@ public: operator Number() const noexcept { - return {drops(), Number::weak}; + return {drops(), Number::compatible}; } Number diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index e3789de90a..489e27c79f 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -160,6 +160,8 @@ Number::checkInteger(char const* what) const { if (enforceInteger_ == strong && !valid()) throw std::overflow_error(what); + if (enforceInteger_ == weak && !representable()) + throw std::overflow_error(what); } void @@ -217,7 +219,20 @@ Number::normalize() bool Number::valid() const noexcept { - if (enforceInteger_ != none) + return valid(enforceInteger_); +} + +bool +Number::valid(EnforceInteger enforce) +{ + setIntegerEnforcement(enforce); + return valid(); +} + +bool +Number::valid(EnforceInteger enforce) const +{ + if (enforce != none) { static Number const max = maxIntValue; static Number const maxNeg = -maxIntValue; @@ -229,6 +244,21 @@ Number::valid() const noexcept return true; } +bool +Number::representable() const noexcept +{ + if (enforceInteger_ != none) + { + static Number const max = maxMantissa; + static Number const maxNeg = -maxMantissa; + // Avoid making a copy + if (mantissa_ < 0) + return *this >= maxNeg; + return *this <= max; + } + return true; +} + Number& Number::operator+=(Number const& y) { diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 92fd5ccc97..43fcae04f7 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2878,6 +2878,8 @@ assetsToSharesDeposit( Number const assetTotal = vault->at(sfAssetsTotal); STAmount shares{vault->at(sfShareMPTID)}; + // STAmount will ignore enforcement for IOUs, so we can set it regardless of + // type. shares.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return STAmount{ @@ -2910,6 +2912,8 @@ sharesToAssetsDeposit( Number const assetTotal = vault->at(sfAssetsTotal); STAmount assets{vault->at(sfAsset)}; + // STAmount will ignore enforcement for IOUs, so we can set it regardless of + // type. assets.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return STAmount{ @@ -2944,6 +2948,8 @@ assetsToSharesWithdraw( Number assetTotal = vault->at(sfAssetsTotal); assetTotal -= vault->at(sfLossUnrealized); STAmount shares{vault->at(sfShareMPTID)}; + // STAmount will ignore enforcement for IOUs, so we can set it regardless of + // type. shares.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return shares; @@ -2975,6 +2981,8 @@ sharesToAssetsWithdraw( Number assetTotal = vault->at(sfAssetsTotal); assetTotal -= vault->at(sfLossUnrealized); STAmount assets{vault->at(sfAsset)}; + // STAmount will ignore enforcement for IOUs, so we can set it regardless of + // type. assets.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return assets; diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 6f6355a145..1df3d3c18b 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -270,7 +270,8 @@ STAmount::integerEnforcement() const noexcept bool STAmount::validNumber() const noexcept { - Number n = toNumber(Number::EnforceInteger::weak); + // compatible will not throw. + Number n = toNumber(Number::EnforceInteger::compatible); return n.valid(); } diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 5b36ffaf8e..4f81118147 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3611,7 +3611,7 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(14, [&, this](Env& env, Data d) { + testCase(13, [&, this](Env& env, Data d) { testcase("MPT scale deposit overflow on first deposit"); auto tx = d.vault.deposit( {.depositor = d.depositor, @@ -3621,7 +3621,7 @@ class Vault_test : public beast::unit_test::suite env.close(); }); - testCase(14, [&, this](Env& env, Data d) { + testCase(13, [&, this](Env& env, Data d) { testcase("MPT scale deposit overflow on second deposit"); { @@ -3643,7 +3643,7 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(14, [&, this](Env& env, Data d) { + testCase(13, [&, this](Env& env, Data d) { testcase("No MPT scale deposit overflow on total shares"); { @@ -3963,7 +3963,7 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(14, [&, this](Env& env, Data d) { + testCase(13, [&, this](Env& env, Data d) { testcase("MPT scale withdraw overflow"); { @@ -4204,7 +4204,7 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(14, [&, this](Env& env, Data d) { + testCase(13, [&, this](Env& env, Data d) { testcase("MPT Scale clawback overflow"); { diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index a5fba8ef7d..1efafec65f 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -737,28 +737,87 @@ public: Number a{100}; BEAST_EXPECT(a.integerEnforcement() == Number::none); BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); a = Number{1, 30}; BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); a = -100; BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); + } + { + Number a{100, Number::compatible}; + BEAST_EXPECT(a.integerEnforcement() == Number::compatible); + BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); + a = Number{1, 15}; + BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.representable()); + a = Number{1, 30, Number::none}; + BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); + a = -100; + BEAST_EXPECT(a.integerEnforcement() == Number::compatible); + BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); + a = Number{5, Number::weak}; + BEAST_EXPECT(a.integerEnforcement() == Number::weak); + BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); + a = Number{5, Number::strong}; + BEAST_EXPECT(a.integerEnforcement() == Number::strong); + BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); } { Number a{100, Number::weak}; BEAST_EXPECT(a.integerEnforcement() == Number::weak); BEAST_EXPECT(a.valid()); - a = Number{1, 30, Number::none}; + BEAST_EXPECT(a.representable()); + a = Number{1, 15}; BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.representable()); + try + { + a = Number{1, 30, Number::compatible}; + BEAST_EXPECT(false); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECT(e.what() == "Number::operator= integer overflow"s); + // The throw is done _after_ the number is updated. + BEAST_EXPECT((a == Number{1, 30})); + } + BEAST_EXPECT(a.integerEnforcement() == Number::weak); + BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); a = -100; BEAST_EXPECT(a.integerEnforcement() == Number::weak); BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); a = Number{5, Number::strong}; BEAST_EXPECT(a.integerEnforcement() == Number::strong); BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); } { Number a{100, Number::strong}; BEAST_EXPECT(a.integerEnforcement() == Number::strong); BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); + try + { + a = Number{1, 15, Number::compatible}; + BEAST_EXPECT(false); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECT(e.what() == "Number::operator= integer overflow"s); + // The throw is done _after_ the number is updated. + BEAST_EXPECT((a == Number{1, 15})); + } + BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.representable()); try { a = Number{1, 30}; @@ -771,15 +830,19 @@ public: BEAST_EXPECT((a == Number{1, 30})); } BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); a = -100; BEAST_EXPECT(a.integerEnforcement() == Number::strong); BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); } { - Number a{INITIAL_XRP.drops(), Number::weak}; + Number a{INITIAL_XRP.drops(), Number::compatible}; BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); a = -a; BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); try { @@ -795,6 +858,7 @@ public: // assigned to the Number BEAST_EXPECT(a == -INITIAL_XRP); BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); } try { @@ -808,6 +872,7 @@ public: // assigned to the Number BEAST_EXPECT(a == -INITIAL_XRP); BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); } a = Number::maxIntValue; try @@ -821,6 +886,7 @@ public: // This time, the throw is done _after_ the number is updated. BEAST_EXPECT(a == Number::maxIntValue + 1); BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.representable()); } a = -Number::maxIntValue; try @@ -834,6 +900,7 @@ public: // This time, the throw is done _after_ the number is updated. BEAST_EXPECT(a == -Number::maxIntValue - 1); BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.representable()); } a = Number(1, 10); try @@ -848,6 +915,7 @@ public: // The throw is done _after_ the number is updated. BEAST_EXPECT((a == Number{1, 20})); BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); } try { @@ -860,6 +928,7 @@ public: // The throw is done _after_ the number is updated. BEAST_EXPECT((a == Number::maxIntValue * 2)); BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.representable()); } try { @@ -872,6 +941,7 @@ public: // The Number doesn't get updated because the ctor throws BEAST_EXPECT((a == Number::maxIntValue * 2)); BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.representable()); } a = Number(1, 10); try @@ -885,10 +955,12 @@ public: // The throw is done _after_ the number is updated. BEAST_EXPECT((a == Number{1, 20})); BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); } a /= Number(1, 15); BEAST_EXPECT((a == Number{1, 5})); BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); } } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index c15f2b64a5..0dbbd4f24a 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -2180,6 +2180,13 @@ ValidVault::Vault::make(SLE const& from) self.assetsAvailable = from.at(sfAssetsAvailable); self.assetsMaximum = from.at(sfAssetsMaximum); self.lossUnrealized = from.at(sfLossUnrealized); + if (self.asset.integral()) + { + self.assetsTotal.setIntegerEnforcement(Number::compatible); + self.assetsAvailable.setIntegerEnforcement(Number::compatible); + self.assetsMaximum.setIntegerEnforcement(Number::compatible); + self.lossUnrealized.setIntegerEnforcement(Number::compatible); + } return self; } @@ -2413,6 +2420,19 @@ ValidVault::finalize( beforeVault_.empty() || beforeVault_[0].key == afterVault.key, "ripple::ValidVault::finalize : single vault operation"); + if (!afterVault.assetsTotal.representable() || + !afterVault.assetsAvailable.representable() || + !afterVault.assetsMaximum.representable() || + !afterVault.lossUnrealized.representable()) + { + JLOG(j.fatal()) << "Invariant failed: vault overflowed maximum current " + "representable integer value"; + XRPL_ASSERT( + enforce, + "ripple::ValidVault::finalize : vault integer limit invariant"); + return !enforce; // That's all we can do here + } + auto const updatedShares = [&]() -> std::optional { // At this moment we only know that a vault is being updated and there // might be some MPTokenIssuance objects which are also updated in the diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index 1c73d36bb0..b464c1eb28 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -150,8 +150,11 @@ VaultClawback::doApply() amount.asset() == vaultAsset, "ripple::VaultClawback::doApply : matching asset"); + // Both of these values are going to be decreased in this transaction, + // so the limit doesn't really matter. auto assetsAvailable = vault->at(sfAssetsAvailable); auto assetsTotal = vault->at(sfAssetsTotal); + [[maybe_unused]] auto const lossUnrealized = vault->at(sfLossUnrealized); XRPL_ASSERT( lossUnrealized <= (assetsTotal - assetsAvailable), @@ -161,6 +164,8 @@ VaultClawback::doApply() MPTIssue const share{mptIssuanceID}; STAmount sharesDestroyed = {share}; STAmount assetsRecovered; + // STAmount will ignore enforcement for IOUs, so we can set it regardless of + // type. assetsRecovered.setIntegerEnforcement(Number::weak); sharesDestroyed.setIntegerEnforcement(Number::weak); try diff --git a/src/xrpld/app/tx/detail/VaultCreate.cpp b/src/xrpld/app/tx/detail/VaultCreate.cpp index 8acb40ad41..6c30739369 100644 --- a/src/xrpld/app/tx/detail/VaultCreate.cpp +++ b/src/xrpld/app/tx/detail/VaultCreate.cpp @@ -210,6 +210,13 @@ VaultCreate::doApply() vault->at(sfWithdrawalPolicy) = vaultStrategyFirstComeFirstServe; if (scale) vault->at(sfScale) = scale; + if (asset.integral()) + { + // Only the Maximum can be a non-zero value, so only it needs to be + // checked. + if (!vault->at(sfAssetsMaximum).value().valid(Number::compatible)) + return tecLIMIT_EXCEEDED; + } view().insert(vault); // Explicitly create MPToken for the vault owner diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 66e144312f..609379d4c2 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -220,6 +220,8 @@ VaultDeposit::doApply() } STAmount sharesCreated = {vault->at(sfShareMPTID)}, assetsDeposited; + // STAmount will ignore enforcement for IOUs, so we can set it regardless of + // type. sharesCreated.setIntegerEnforcement(Number::weak); try { diff --git a/src/xrpld/app/tx/detail/VaultSet.cpp b/src/xrpld/app/tx/detail/VaultSet.cpp index 38ab6296ef..75f1689d7f 100644 --- a/src/xrpld/app/tx/detail/VaultSet.cpp +++ b/src/xrpld/app/tx/detail/VaultSet.cpp @@ -144,6 +144,11 @@ VaultSet::doApply() tx[sfAssetsMaximum] < *vault->at(sfAssetsTotal)) return tecLIMIT_EXCEEDED; vault->at(sfAssetsMaximum) = tx[sfAssetsMaximum]; + if (vault->at(sfAsset).value().integral()) + { + if (!vault->at(sfAssetsMaximum).value().valid(Number::compatible)) + return tecLIMIT_EXCEEDED; + } } if (auto const domainId = tx[~sfDomainID]; domainId) diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index 8806f7b236..e28c54676c 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -222,6 +222,8 @@ VaultWithdraw::doApply() return tecINSUFFICIENT_FUNDS; } + // These values are only going to decrease, and can't be less than 0, so + // there's no need for integer range enforcement. auto assetsAvailable = vault->at(sfAssetsAvailable); auto assetsTotal = vault->at(sfAssetsTotal); [[maybe_unused]] auto const lossUnrealized = vault->at(sfLossUnrealized);