diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index e34cc61b5b3..b94335224f8 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -13,16 +13,58 @@ class Number; std::string to_string(Number const& amount); +template +constexpr bool +isPowerOfTen(T value) +{ + while (value >= 10 && value % 10 == 0) + value /= 10; + return value == 1; +} + class Number { using rep = std::int64_t; rep mantissa_{0}; int exponent_{std::numeric_limits::lowest()}; + // Within Number itself limited_ is informational only. It is not + // serialized, transmitted, or used in calculations in any way. It is used + // only to indicate that a given Number's absolute value _might_ need to be + // less than maxIntValue or maxMantissa. + // + // It is a one-way switch. Once it's on, it stays on. It is also + // transmissible in that any operation (e.g +, /, power, etc.) involving a + // Number with this flag will have a result with this flag. + // + // The flag is checked in the following places: + // 1. "fits()" indicates whether the Number fits into the safe range of + // -maxIntValue to maxIntValue. + // 2. "representable()" indicates whether the Number can accurately + // represent an integer, meaning that it fits withing the allowable range + // of -maxMantissa to maxMantissa. Values larger than this will be + // truncated before the decimal point, rendering the value inaccurate. + // 3. In "operator rep()", which explicitly converts the number into a + // 64-bit integer, if the integer value grows larger than maxMantissa + // while it's being computed, AND one of the SingleAssetVault (or + // LendingProtocol, coming soon) amendments are enabled, the operator + // will throw a "std::overflow_error" as if the number had overflowed the + // limits of the 64-bit integer range. + // + // The Number is usually only going to be checked in transactions, based on + // the specific transaction logic, and is entirely context dependent. + // + bool limited_ = false; + public: // The range for the mantissa when normalized - constexpr static std::int64_t minMantissa = 1'000'000'000'000'000LL; - constexpr static std::int64_t maxMantissa = 9'999'999'999'999'999LL; + constexpr static rep minMantissa = 1'000'000'000'000'000LL; + static_assert(isPowerOfTen(minMantissa)); + constexpr static rep maxMantissa = minMantissa * 10 - 1; + static_assert(maxMantissa == 9'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; @@ -35,15 +77,53 @@ class Number explicit constexpr Number() = default; - Number(rep mantissa); - explicit Number(rep mantissa, int exponent); + Number(rep mantissa, bool limited = false); + explicit Number(rep mantissa, int exponent, bool limited = false); explicit constexpr Number(rep mantissa, int exponent, unchecked) noexcept; + constexpr Number(Number const& other) = default; + constexpr Number(Number&& other) = default; + + ~Number() = default; + + constexpr Number& + operator=(Number const& other); + constexpr Number& + operator=(Number&& other); constexpr rep mantissa() const noexcept; constexpr int exponent() const noexcept; + // Sets the limited_ flag. See the description of limited for how it works. + // Note that the flag can only change from false to true, not from true to + // false. + void + setLimited(bool limited); + + // Gets the current value of the limited_ flag. See the description of + // limited for how it works. + bool + getLimited() const noexcept; + + // 1. "fits()" indicates whether the Number fits into the safe range of + // -maxIntValue to maxIntValue. + bool + fits() const noexcept; + bool + // 2. "representable()" indicates whether the Number can accurately + // represent an integer, meaning that it fits withing the allowable range + // of -maxMantissa to maxMantissa. Values larger than this will be + // truncated before the decimal point, rendering the value inaccurate. + representable() const noexcept; + /// Combines setLimited(bool) and fits() + bool + fits(bool limited); + /// Because this function is const, it should only be used for one-off + /// checks + bool + fits(bool limited) const; + constexpr Number operator+() const noexcept; constexpr Number @@ -78,6 +158,13 @@ class Number * are explicit. This design encourages and facilitates the use of Number * as the preferred type for floating point arithmetic as it makes * "mixed mode" more convenient, e.g. MPTAmount + Number. + + 3. In "operator rep()", which explicitly converts the number into a + 64-bit integer, if the integer value grows larger than maxMantissa + while it's being computed, AND one of the SingleAssetVault (or + LendingProtocol, coming soon) amendments are enabled, the operator + will throw a "std::overflow_error" as if the number had overflowed + the limits of the 64-bit integer range. */ explicit operator rep() const; // round to nearest, even on tie @@ -181,9 +268,31 @@ class Number static rounding_mode setround(rounding_mode mode); + // Thread local integer overflow control. See overflowLargeIntegers_ for + // more info. + static bool + getEnforceIntegerOverflow(); + // Thread local integer overflow control. See overflowLargeIntegers_ for + // more info. + static void + setEnforceIntegerOverflow(bool enforce); + private: static thread_local rounding_mode mode_; + // This flag defaults to false. It is set and cleared by + // "setCurrentTransactionRules" in Rules.cpp. It will be set to true if and + // only if any of the SingleAssetVault or LendingProtocol amendments are + // enabled. + // + // If set, then any explicit conversions from Number to rep (which is + // std::int64_t) of Numbers that are not representable (which means their + // magnitude is larger than maxMantissa, and thus they will lose integer + // precision) will throw a std::overflow_error. Note that this coversion + // will already throw an overflow error if the Number is larger 2^63. + // See also "operator rep()". + static thread_local bool overflowLargeIntegers_; + void normalize(); constexpr bool @@ -197,16 +306,46 @@ inline constexpr Number::Number(rep mantissa, int exponent, unchecked) noexcept { } -inline Number::Number(rep mantissa, int exponent) - : mantissa_{mantissa}, exponent_{exponent} +inline Number::Number(rep mantissa, int exponent, bool limited) + : mantissa_{mantissa}, exponent_{exponent}, limited_(limited) { normalize(); } -inline Number::Number(rep mantissa) : Number{mantissa, 0} +inline Number::Number(rep mantissa, bool limited) : Number{mantissa, 0, limited} { } +constexpr Number& +Number::operator=(Number const& other) +{ + if (this != &other) + { + mantissa_ = other.mantissa_; + exponent_ = other.exponent_; + if (!limited_) + limited_ = other.limited_; + } + + return *this; +} + +constexpr Number& +Number::operator=(Number&& other) +{ + if (this != &other) + { + // std::move doesn't really do anything for these types, but + // this is future-proof in case the types ever change + mantissa_ = std::move(other.mantissa_); + exponent_ = std::move(other.exponent_); + if (!limited_) + limited_ = std::move(other.limited_); + } + + return *this; +} + inline constexpr Number::rep Number::mantissa() const noexcept { @@ -219,6 +358,20 @@ Number::exponent() const noexcept return exponent_; } +inline void +Number::setLimited(bool limited) +{ + if (limited_) + return; + limited_ = limited; +} + +inline bool +Number::getLimited() const noexcept +{ + return limited_; +} + inline constexpr Number Number::operator+() const noexcept { @@ -404,6 +557,32 @@ class NumberRoundModeGuard operator=(NumberRoundModeGuard const&) = delete; }; +// Sets the EnforceIntegerOverflow flag and restores the old value when it +// leaves scope. Since Number doesn't have that facility, we'll build it here. +// +// This class may only end up needed in tests +class NumberIntegerOverflowGuard +{ + bool const saved_; + +public: + explicit NumberIntegerOverflowGuard(bool enforce) noexcept + : saved_{Number::getEnforceIntegerOverflow()} + { + Number::setEnforceIntegerOverflow(enforce); + } + + ~NumberIntegerOverflowGuard() + { + Number::setEnforceIntegerOverflow(saved_); + } + + NumberIntegerOverflowGuard(NumberIntegerOverflowGuard const&) = delete; + + NumberIntegerOverflowGuard& + operator=(NumberIntegerOverflowGuard const&) = delete; +}; + } // namespace ripple #endif // XRPL_BASICS_NUMBER_H_INCLUDED diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index d0efe814a92..7ad1e702568 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -81,7 +81,27 @@ class Asset bool native() const { - return holds() && get().native(); + return std::visit( + [&](TIss const& issue) { + if constexpr (std::is_same_v) + return issue.native(); + if constexpr (std::is_same_v) + return false; + }, + issue_); + } + + bool + integral() const + { + return std::visit( + [&](TIss const& issue) { + if constexpr (std::is_same_v) + return issue.native(); + if constexpr (std::is_same_v) + return true; + }, + issue_); } friend constexpr bool diff --git a/include/xrpl/protocol/MPTAmount.h b/include/xrpl/protocol/MPTAmount.h index af147865017..46fa4e98b96 100644 --- a/include/xrpl/protocol/MPTAmount.h +++ b/include/xrpl/protocol/MPTAmount.h @@ -64,7 +64,7 @@ class MPTAmount : private boost::totally_ordered, operator Number() const noexcept { - return value(); + return {value(), true}; } /** Return the sign of the amount */ diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 20b73e90c97..e12006a7941 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -108,9 +108,11 @@ std::uint8_t constexpr vaultStrategyFirstComeFirstServe = 1; /** Default IOU scale factor for a Vault */ std::uint8_t constexpr vaultDefaultIOUScale = 6; /** Maximum scale factor for a Vault. The number is chosen to ensure that -1 IOU can be always converted to shares. +1 IOU can be always converted to shares and will fit into Number. +10^16 > Number::maxMantissa +In the future, this should be increased to 18. 10^19 > maxMPTokenAmount (2^64-1) > 10^18 */ -std::uint8_t constexpr vaultMaximumIOUScale = 18; +std::uint8_t constexpr vaultMaximumIOUScale = 15; /** Maximum recursion depth for vault shares being put as an asset inside * another vault; counted from 0 */ diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index 83493efcdda..254ef368223 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -146,6 +146,24 @@ class STAmount final : public STBase, public CountedObject STAmount(MPTAmount const& amount, MPTIssue const& mptIssue); operator Number() const; + // Determines if the "Number" representation of the amount can fit within + // the Number's soft range limits. Converts the amount to a Number, using + // the automatic conversion rules defined in "operator Number()", and in + // "XRPAmount::operator Number()", "MPTAmount::operator Number()", and + // "IOUAmount::operator Number()", then returns the result of + // "Number::fits()". See "Number::fits()" for more information. + bool + numberFits() const noexcept; + // Determines if the "Number" representation of the amount can fit within + // the Number's hard range limits. Converts the amount to a Number, using + // the automatic conversion rules defined in "operator Number()", and in + // "XRPAmount::operator Number()", "MPTAmount::operator Number()", and + // "IOUAmount::operator Number()", then returns the result of + // "Number::representable()". See "Number::representable()" for more + // information. + bool + representableNumber() const noexcept; + //-------------------------------------------------------------------------- // // Observers @@ -155,6 +173,9 @@ class STAmount final : public STBase, public CountedObject int exponent() const noexcept; + bool + integral() const noexcept; + bool native() const noexcept; @@ -435,6 +456,12 @@ STAmount::exponent() const noexcept return mOffset; } +inline bool +STAmount::integral() const noexcept +{ + return mAsset.integral(); +} + inline bool STAmount::native() const noexcept { @@ -553,7 +580,7 @@ STAmount::clear() { // The -100 is used to allow 0 to sort less than a small positive values // which have a negative exponent. - mOffset = native() ? 0 : -100; + mOffset = integral() ? 0 : -100; mValue = 0; mIsNegative = false; } diff --git a/include/xrpl/protocol/STObject.h b/include/xrpl/protocol/STObject.h index 9b325f06feb..0e985e9712a 100644 --- a/include/xrpl/protocol/STObject.h +++ b/include/xrpl/protocol/STObject.h @@ -482,6 +482,8 @@ class STObject::Proxy value_type operator*() const; + /// Do not use operator->() unless the field is required, or you've checked + /// that it's set. T const* operator->() const; @@ -718,6 +720,8 @@ STObject::Proxy::operator*() const -> value_type return this->value(); } +/// Do not use operator->() unless the field is required, or you've checked that +/// it's set. template T const* STObject::Proxy::operator->() const diff --git a/include/xrpl/protocol/SystemParameters.h b/include/xrpl/protocol/SystemParameters.h index de78b65265d..7a2c5a7f63f 100644 --- a/include/xrpl/protocol/SystemParameters.h +++ b/include/xrpl/protocol/SystemParameters.h @@ -23,6 +23,7 @@ systemName() /** Number of drops in the genesis account. */ constexpr XRPAmount INITIAL_XRP{100'000'000'000 * DROPS_PER_XRP}; +static_assert(INITIAL_XRP.drops() == 100'000'000'000'000'000); /** Returns true if the amount does not exceed the initial XRP in existence. */ inline bool diff --git a/include/xrpl/protocol/XRPAmount.h b/include/xrpl/protocol/XRPAmount.h index e102a3707fa..448f87cb24b 100644 --- a/include/xrpl/protocol/XRPAmount.h +++ b/include/xrpl/protocol/XRPAmount.h @@ -143,7 +143,7 @@ class XRPAmount : private boost::totally_ordered, operator Number() const noexcept { - return drops(); + return {drops(), true}; } /** Return the sign of the amount */ diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index 53110f09f5c..5aae9a9322c 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/include/xrpl/protocol/detail/ledger_entries.macro @@ -479,10 +479,10 @@ LEDGER_ENTRY(ltVAULT, 0x0084, Vault, vault, ({ {sfAccount, soeREQUIRED}, {sfData, soeOPTIONAL}, {sfAsset, soeREQUIRED}, - {sfAssetsTotal, soeREQUIRED}, - {sfAssetsAvailable, soeREQUIRED}, + {sfAssetsTotal, soeDEFAULT}, + {sfAssetsAvailable, soeDEFAULT}, {sfAssetsMaximum, soeDEFAULT}, - {sfLossUnrealized, soeREQUIRED}, + {sfLossUnrealized, soeDEFAULT}, {sfShareMPTID, soeREQUIRED}, {sfWithdrawalPolicy, soeREQUIRED}, {sfScale, soeDEFAULT}, diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 89f7937e068..4be125463c1 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -23,6 +23,7 @@ using uint128_t = __uint128_t; namespace ripple { thread_local Number::rounding_mode Number::mode_ = Number::to_nearest; +thread_local bool Number::overflowLargeIntegers_ = false; Number::rounding_mode Number::getround() @@ -36,6 +37,18 @@ Number::setround(rounding_mode mode) return std::exchange(mode_, mode); } +bool +Number::getEnforceIntegerOverflow() +{ + return overflowLargeIntegers_; +} + +void +Number::setEnforceIntegerOverflow(bool enforce) +{ + std::exchange(overflowLargeIntegers_, enforce); +} + // Guard // The Guard class is used to tempoarily add extra digits of @@ -164,9 +177,8 @@ Number::normalize() return; } bool const negative = (mantissa_ < 0); - auto m = static_cast>(mantissa_); - if (negative) - m = -m; + auto m = static_cast>( + negative ? -mantissa_ : mantissa_); while ((m < minMantissa) && (exponent_ > minExponent)) { m *= 10; @@ -207,9 +219,52 @@ Number::normalize() mantissa_ = -mantissa_; } +bool +Number::fits() const noexcept +{ + return fits(limited_); +} + +bool +Number::fits(bool limited) +{ + setLimited(limited); + return fits(); +} + +bool +Number::fits(bool limited) const +{ + if (!limited) + return true; + static Number const max = maxIntValue; + static Number const maxNeg = -max; + // Avoid making a copy + if (mantissa_ < 0) + return *this >= maxNeg; + return *this <= max; +} + +bool +Number::representable() const noexcept +{ + if (!limited_) + return true; + static Number const max = maxMantissa; + static Number const maxNeg = -max; + // Avoid making a copy + if (mantissa_ < 0) + return *this >= maxNeg; + return *this <= max; +} + Number& Number::operator+=(Number const& y) { + // The strictest setting prevails + if (!limited_) + limited_ = y.limited_; + if (y == Number{}) return *this; if (*this == Number{}) @@ -356,6 +411,10 @@ divu10(uint128_t& u) Number& Number::operator*=(Number const& y) { + // The strictest setting prevails + if (!limited_) + limited_ = y.limited_; + if (*this == Number{}) return *this; if (y == Number{}) @@ -428,6 +487,10 @@ Number::operator*=(Number const& y) Number& Number::operator/=(Number const& y) { + // The strictest setting prevails + if (!limited_) + limited_ = y.limited_; + if (y == Number{}) throw std::overflow_error("Number: divide by 0"); if (*this == Number{}) @@ -477,6 +540,9 @@ Number::operator rep() const } for (; offset > 0; --offset) { + if (Number::overflowLargeIntegers_ && drops > maxMantissa / 10) + throw std::overflow_error( + "Number::operator rep() overflow unrepresentable"); if (drops > std::numeric_limits::max() / 10) throw std::overflow_error("Number::operator rep() overflow"); drops *= 10; diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 6356dd1131e..e7ef1309385 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2927,7 +2927,7 @@ assetsToSharesWithdraw( { XRPL_ASSERT( !assets.negative(), - "ripple::assetsToSharesDeposit : non-negative assets"); + "ripple::assetsToSharesWithdraw : non-negative assets"); XRPL_ASSERT( assets.asset() == vault->at(sfAsset), "ripple::assetsToSharesWithdraw : assets and vault match"); diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index 7d84c4e7da7..ae8be118646 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -33,6 +33,14 @@ getCurrentTransactionRules() void setCurrentTransactionRules(std::optional r) { + // Make global changes associated with the rules before the value is moved. + // Push the appropriate setting to Number, which has no access to even this + // global rules object. + bool enableIntegerOverflow = r && + (r->enabled(featureSingleAssetVault) /*|| + r->enabled(featureLendingProtocol)*/); + Number::setEnforceIntegerOverflow(enableIntegerOverflow); + *getCurrentTransactionRulesRef() = std::move(r); } diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index d659ee0d05d..68c6744bb31 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -255,6 +255,26 @@ STAmount::move(std::size_t n, void* buf) return emplace(n, buf, std::move(*this)); } +bool +STAmount::numberFits() const noexcept +{ + // Converting this STAmount to a Number will automatically create a number + // with the correct value for `limited_`, because the conversions from + // MPTAmount, XRPAmount, and IOUAmount always set the flag correctly. + Number n = *this; + return n.fits(); +} + +bool +STAmount::representableNumber() const noexcept +{ + // Converting this STAmount to a Number will automatically create a number + // with the correct value for `limited_`, because the conversions from + // MPTAmount, XRPAmount, and IOUAmount always set the flag correctly. + Number n = *this; + return n.representable(); +} + //------------------------------------------------------------------------------ // // Conversion @@ -851,6 +871,12 @@ STAmount::canonicalize() if (getSTNumberSwitchover()) { + // "unchecked" skips the normalization step, so the parameters are + // left unmodified. When converting the `Number` back into an + // integer, if `mOffset` is 0 (which is the default), the value is + // unmodified, so the original integer drops back out, no matter how + // large. Precision loss is only an issue if there is an + // exponent/offset that requires making the value larger. Number num( mIsNegative ? -mValue : mValue, mOffset, Number::unchecked{}); auto set = [&](auto const& val) { diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 5a1816ebae4..66bceec3279 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1384,7 +1384,7 @@ struct AMM_test : public jtx::AMMTest // equal asset deposit: unit test to exercise the rounding-down of // LPTokens in the AMMHelpers.cpp: adjustLPTokens calculations // The LPTokens need to have 16 significant digits and a fractional part - for (Number const deltaLPTokens : + for (Number const& deltaLPTokens : {Number{UINT64_C(100000'0000000009), -10}, Number{UINT64_C(100000'0000000001), -10}}) { diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index f7c93d3c621..73f12625a8d 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -168,6 +168,13 @@ class MPToken_test : public beast::unit_test::suite Json::Value const result = env.rpc("tx", txHash)[jss::result]; BEAST_EXPECT( result[sfMaximumAmount.getJsonName()] == "9223372036854775807"); + + auto const issuanceID = mptAlice.issuanceID(); + if (auto const le = env.le(keylet::mptIssuance(issuanceID)); + BEAST_EXPECT(le)) + { + BEAST_EXPECT(le->at(sfMaximumAmount) == maxMPTokenAmount); + } } if (features[featureSingleAssetVault]) @@ -1670,6 +1677,11 @@ class MPToken_test : public beast::unit_test::suite mptAlice.pay(alice, bob, maxMPTokenAmount); BEAST_EXPECT( mptAlice.checkMPTokenOutstandingAmount(maxMPTokenAmount)); + STAmount const expectedAmount{ + mptAlice.issuanceID(), maxMPTokenAmount}; + // the value was not truncated or rounded + BEAST_EXPECT(expectedAmount.mantissa() == maxMPTokenAmount); + env.require(balance(bob, expectedAmount)); // payment between the holders mptAlice.pay(bob, carol, maxMPTokenAmount); diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 4097b937860..715b8d33887 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -962,16 +962,25 @@ class Vault_test : public beast::unit_test::suite env(tx, ter(temMALFORMED)); } - // accepted range from 0 to 18 + // the prior acceptable upper limit { auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); tx[sfScale] = 18; + env(tx, ter(temMALFORMED)); + } + + // accepted range from 0 to 15 + { + auto [tx, keylet] = + vault.create({.owner = owner, .asset = asset}); + tx[sfScale] = 15; env(tx); env.close(); auto const sleVault = env.le(keylet); - BEAST_EXPECT(sleVault); - BEAST_EXPECT((*sleVault)[sfScale] == 18); + if (!BEAST_EXPECT(sleVault)) + return; + BEAST_EXPECT((*sleVault)[sfScale] == 15); } { @@ -981,7 +990,8 @@ class Vault_test : public beast::unit_test::suite env(tx); env.close(); auto const sleVault = env.le(keylet); - BEAST_EXPECT(sleVault); + if (!BEAST_EXPECT(sleVault)) + return; BEAST_EXPECT((*sleVault)[sfScale] == 0); } @@ -991,7 +1001,8 @@ class Vault_test : public beast::unit_test::suite env(tx); env.close(); auto const sleVault = env.le(keylet); - BEAST_EXPECT(sleVault); + if (!BEAST_EXPECT(sleVault)) + return; BEAST_EXPECT((*sleVault)[sfScale] == 6); } }); @@ -3625,13 +3636,18 @@ class Vault_test : public beast::unit_test::suite tx[sfScale] = scale; env(tx); - auto const [vaultAccount, issuanceId] = - [&env](ripple::Keylet keylet) -> std::tuple { + auto const vaultInfo = [&env](ripple::Keylet keylet) + -> std::optional> { auto const vault = env.le(keylet); - return { + if (!vault) + return std::nullopt; + return std::make_tuple( Account("vault", vault->at(sfAccount)), - vault->at(sfShareMPTID)}; + vault->at(sfShareMPTID)); }(keylet); + if (!BEAST_EXPECT(vaultInfo)) + return; + auto const [vaultAccount, issuanceId] = *vaultInfo; MPTIssue shares(issuanceId); env.memoize(vaultAccount); @@ -3673,18 +3689,93 @@ class Vault_test : public beast::unit_test::suite .peek = peek}); }; - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow on first deposit"); + // The scale can go to 15, which will allow the total assets to + // go that high, but single deposits are not allowed over 10^13. + // There probably aren't too many use cases that will be able to + // use this, but it does work. + testCase(15, [&, this](Env& env, Data d) { + testcase("MPT fractional deposits are supported"); + + // Deposits that result in share amounts larger than + // Number::maxIntValue are invalid + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(10)}); + env(tx, ter{tecPATH_DRY}); + env.close(); + } + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(5)}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(Number{1, -1})}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + + auto const smallDeposit = d.asset(Number{5, -2}); + { + // Individual deposits that fit within + // Number::maxIntValue are valid + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = smallDeposit}); + env(tx); + } + env.close(); + { + // The total shares can not go over Number::maxIntValue + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = smallDeposit}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + + { + auto tx = d.vault.withdraw( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(Number(10, 0))}); + env(tx, ter{tecPATH_DRY}); + env.close(); + } + + { + // A withdraw can take any representable amount, even one + // that can't be deposited + auto tx = d.vault.withdraw( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(Number{10, -2})}); + env(tx, ter{tecINSUFFICIENT_FUNDS}); + } + }); + + testCase(13, [&, this](Env& env, Data d) { + testcase("MPT scale deposit over maxIntValue on first deposit"); auto tx = d.vault.deposit( {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(10)}); - env(tx, ter{tecPATH_DRY}); + env(tx, ter{tecPRECISION_LOSS}); env.close(); }); - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow on second deposit"); + testCase(13, [&, this](Env& env, Data d) { + testcase("MPT scale deposit over maxIntValue on second deposit"); { auto tx = d.vault.deposit( @@ -3700,13 +3791,13 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(10)}); - env(tx, ter{tecPATH_DRY}); + env(tx, ter{tecPRECISION_LOSS}); env.close(); } }); - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow on total shares"); + testCase(13, [&, this](Env& env, Data d) { + testcase("MPT scale deposit over maxIntValue on total shares"); { auto tx = d.vault.deposit( @@ -3722,7 +3813,7 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(5)}); - env(tx, ter{tecPATH_DRY}); + env(tx, ter(tecPRECISION_LOSS)); env.close(); } }); @@ -4003,8 +4094,8 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale withdraw overflow"); + testCase(13, [&, this](Env& env, Data d) { + testcase("MPT scale withdraw over maxIntValue"); { auto tx = d.vault.deposit( @@ -4016,10 +4107,21 @@ class Vault_test : public beast::unit_test::suite } { + // withdraws are allowed to be invalid... auto tx = d.vault.withdraw( {.depositor = d.depositor, .id = d.keylet.key, .amount = STAmount(d.asset, Number(10, 0))}); + env(tx, ter{tecINSUFFICIENT_FUNDS}); + env.close(); + } + + { + // ...but they are not allowed to be unrepresentable + auto tx = d.vault.withdraw( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = STAmount(d.asset, Number(1000, 0))}); env(tx, ter{tecPATH_DRY}); env.close(); } @@ -4221,9 +4323,51 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(18, [&, this](Env& env, Data d) { + // The scale can go to 15, which will allow the total assets to + // go that high, but single deposits are not allowed over 10^13. + // There probably aren't too many use cases that will be able to + // use this, but it does work. + testCase(15, [&, this](Env& env, Data d) { testcase("Scale clawback overflow"); + auto const smallDeposit = d.asset(Number{5, -2}); + { + // Individual deposits that fit within + // Number::maxIntValue are valid + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = smallDeposit}); + env(tx); + } + env.close(); + + { + auto tx = d.vault.clawback( + {.issuer = d.issuer, + .id = d.keylet.key, + .holder = d.depositor, + .amount = d.asset(10)}); + env(tx, ter{tecPATH_DRY}); + env.close(); + } + + { + // A clawback can take any representable amount, even one + // that can't be deposited + auto tx = d.vault.clawback( + {.issuer = d.issuer, + .id = d.keylet.key, + .holder = d.depositor, + .amount = d.asset(Number(10, -2))}); + env(tx); + env.close(); + } + }); + + testCase(13, [&, this](Env& env, Data d) { + testcase("MPT Scale clawback overflow"); + { auto tx = d.vault.deposit( {.depositor = d.depositor, @@ -4234,12 +4378,24 @@ class Vault_test : public beast::unit_test::suite } { + // clawbacks are allowed to be invalid... auto tx = d.vault.clawback( {.issuer = d.issuer, .id = d.keylet.key, .holder = d.depositor, .amount = STAmount(d.asset, Number(10, 0))}); - env(tx, ter{tecPATH_DRY}); + env(tx); + env.close(); + } + + { + // ...but they are not allowed to be unrepresentable + auto tx = d.vault.clawback( + {.issuer = d.issuer, + .id = d.keylet.key, + .holder = d.depositor, + .amount = STAmount(d.asset, Number(1000, 0))}); + env(tx, ter{tecPRECISION_LOSS}); env.close(); } }); @@ -4525,7 +4681,8 @@ class Vault_test : public beast::unit_test::suite BEAST_EXPECT(checkString(vault, sfAssetsAvailable, "50")); BEAST_EXPECT(checkString(vault, sfAssetsMaximum, "1000")); BEAST_EXPECT(checkString(vault, sfAssetsTotal, "50")); - BEAST_EXPECT(checkString(vault, sfLossUnrealized, "0")); + // Since this field is default, it is not returned. + BEAST_EXPECT(!vault.isMember(sfLossUnrealized.getJsonName())); auto const strShareID = strHex(sle->at(sfShareMPTID)); BEAST_EXPECT(checkString(vault, sfShareMPTID, strShareID)); diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 06203a4c2aa..0c570dc320d 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -725,6 +725,161 @@ class Number_test : public beast::unit_test::suite BEAST_EXPECT(Number(-100, -30000).truncate() == Number(0, 0)); } + void + testInteger() + { + testcase("Integer enforcement"); + + using namespace std::string_literals; + + auto checkInt = [this]( + Number const& a, + std::int64_t expected, + std::string const& ex = {}) { + try + { + BEAST_EXPECT(static_cast(a) == expected); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECTS( + ex.empty() || e.what() == ex, + to_string(a) + ": " + e.what()); + } + }; + { + Number a{100}; + BEAST_EXPECT(!a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 100); + a = Number{1, 30}; + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 0, "Number::operator rep() overflow"); + a = -100; + BEAST_EXPECT(!a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, -100); + // If there's any interaction with an integer, the value + // becomes an integer. This is not always what the value is + // being used for, so it's up to the context to check or not + // check whether the number is a _valid_ integer. + a += Number{37, 2, true}; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 3600); + } + { + Number a{100, true}; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 100); + a = Number{1, 15}; + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 1'000'000'000'000'000); + // The false in the assigned value does not override the + // flag in "a" + a = Number{1, 30, false}; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(!a.representable()); + checkInt(a, 0, "Number::operator rep() overflow"); + a = -100; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, -100); + a *= Number{1, 13}; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, -1'000'000'000'000'000); + a *= Number{1, 3}; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(!a.representable()); + checkInt(a, -1'000'000'000'000'000'000); + // Intermittent value precision can be lost, but the result + // will be rounded, so that's fine. + a /= Number{1, 5}; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, -10'000'000'000'000); + a = Number{1, 14} - 3; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 99'999'999'999'997); + a += 1; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 99'999'999'999'998); + ++a; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 99'999'999'999'999); + a++; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 100'000'000'000'000); + a = Number{5, true}; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); + BEAST_EXPECT(a.representable()); + checkInt(a, 5); + + auto const maxInt = std::numeric_limits::max(); + a = maxInt; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(!a.representable()); + BEAST_EXPECT(to_string(a) == "9223372036854776e3"); + checkInt(a, 0, "Number::operator rep() overflow"); + { + NumberIntegerOverflowGuard og(true); + checkInt( + a, 0, "Number::operator rep() overflow unrepresentable"); + } + + // Same number, but using the "unchecked" option + a = Number{maxInt, 0, Number::unchecked{}}; + // 1152921504606846975 + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(!a.representable()); + // Because "a" is not normalized, "to_string" returns the + // wrong answer. + BEAST_EXPECT(to_string(a) == "9223372036854775.807"); + checkInt(a, maxInt); + { + NumberIntegerOverflowGuard og(true); + checkInt(a, maxInt); + } + + // Use a smaller number that doesn't overflow int64 when rounded up + a = maxInt / 10; + // 922337203685477580 + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(!a.representable()); + checkInt(a, ((maxInt / 1000) + 1) * 100); + { + NumberIntegerOverflowGuard og(true); + checkInt( + a, 0, "Number::operator rep() overflow unrepresentable"); + } + } + } + void run() override { @@ -746,6 +901,7 @@ class Number_test : public beast::unit_test::suite test_inc_dec(); test_toSTAmount(); test_truncate(); + testInteger(); } }; diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 80c41f4d2b0..93b7c41657a 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -2175,6 +2175,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.setLimited(true); + self.assetsAvailable.setLimited(true); + self.assetsMaximum.setLimited(true); + self.lossUnrealized.setLimited(true); + } return self; } @@ -2408,6 +2415,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 7c56ca1d600..7dbe5f4c971 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -48,6 +48,8 @@ VaultClawback::preflight(PreflightContext const& ctx) << "VaultClawback: only asset issuer can clawback."; return temMALFORMED; } + else if (!amount->representableNumber()) + return temBAD_AMOUNT; } return tesSUCCESS; @@ -146,8 +148,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), @@ -156,7 +161,7 @@ VaultClawback::doApply() AccountID holder = tx[sfHolder]; MPTIssue const share{mptIssuanceID}; STAmount sharesDestroyed = {share}; - STAmount assetsRecovered; + STAmount assetsRecovered = {vaultAsset}; try { if (amount == beast::zero) @@ -192,6 +197,12 @@ VaultClawback::doApply() return tecINTERNAL; // LCOV_EXCL_LINE assetsRecovered = *maybeAssets; } + // Clawback amounts are allowed to be invalid, but not unrepresentable. + // Amounts over the "soft" limit help bring the numbers back into the + // valid range. + if (!sharesDestroyed.representableNumber() || + !assetsRecovered.representableNumber()) + return tecPRECISION_LOSS; // Clamp to maximum. if (assetsRecovered > *assetsAvailable) diff --git a/src/xrpld/app/tx/detail/VaultCreate.cpp b/src/xrpld/app/tx/detail/VaultCreate.cpp index 393faa35f81..5e8085cea3b 100644 --- a/src/xrpld/app/tx/detail/VaultCreate.cpp +++ b/src/xrpld/app/tx/detail/VaultCreate.cpp @@ -193,7 +193,29 @@ VaultCreate::doApply() vault->at(sfLossUnrealized) = Number(0); // Leave default values for AssetTotal and AssetAvailable, both zero. if (auto value = tx[~sfAssetsMaximum]) - vault->at(sfAssetsMaximum) = *value; + { + auto assetsMaximumProxy = vault->at(sfAssetsMaximum); + assetsMaximumProxy = *value; + if (asset.integral()) + { + // Only the Maximum can be a non-zero value, so only it needs to be + // checked. + assetsMaximumProxy.value().setLimited(true); + if (!assetsMaximumProxy.value().representable()) + return tecPRECISION_LOSS; + } + } + // TODO: Should integral types automatically set a limit to the + // Number::maxMantissa value? Or maxIntValue? + /* + else if (asset.integral()) + { + auto assetsMaximumProxy = vault->at(~sfAssetsMaximum); + assetsMaximumProxy = STNumber::maxIntValue + assetsMaximumProxy.value().setLimited(true); + } + */ + vault->at(sfShareMPTID) = mptIssuanceID; if (auto value = tx[~sfData]) vault->at(sfData) = *value; diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 3e5ae741e30..8789933ea45 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -23,7 +23,10 @@ VaultDeposit::preflight(PreflightContext const& ctx) return temMALFORMED; } - if (ctx.tx[sfAmount] <= beast::zero) + auto const amount = ctx.tx[sfAmount]; + if (amount <= beast::zero) + return temBAD_AMOUNT; + if (!amount.numberFits()) return temBAD_AMOUNT; return tesSUCCESS; @@ -162,7 +165,8 @@ VaultDeposit::doApply() auto const amount = ctx_.tx[sfAmount]; // Make sure the depositor can hold shares. auto const mptIssuanceID = (*vault)[sfShareMPTID]; - auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID)); + SLE::const_pointer sleIssuance = + view().read(keylet::mptIssuance(mptIssuanceID)); if (!sleIssuance) { // LCOV_EXCL_START @@ -172,6 +176,8 @@ VaultDeposit::doApply() } auto const& vaultAccount = vault->at(sfAccount); + auto const& vaultAsset = vault->at(sfAsset); + // Note, vault owner is always authorized if (vault->isFlag(lsfVaultPrivate) && account_ != vault->at(sfOwner)) { @@ -216,7 +222,8 @@ VaultDeposit::doApply() } } - STAmount sharesCreated = {vault->at(sfShareMPTID)}, assetsDeposited; + STAmount sharesCreated = {vault->at(sfShareMPTID)}; + STAmount assetsDeposited = {vaultAsset}; try { // Compute exchange before transferring any amounts. @@ -242,13 +249,19 @@ VaultDeposit::doApply() // LCOV_EXCL_STOP } assetsDeposited = *maybeAssets; + + // Deposit needs to be more strict than the other Vault transactions + // that deal with asset <-> share calculations, because we don't + // want to go over the "soft" limit. + if (!sharesCreated.numberFits() || !assetsDeposited.numberFits()) + return tecPRECISION_LOSS; } catch (std::overflow_error const&) { // It's easy to hit this exception from Number with large enough Scale // so we avoid spamming the log and only use debug here. JLOG(j_.debug()) // - << "VaultDeposit: overflow error with" + << "VaultDeposit: overflow error computing shares with" << " scale=" << (int)vault->at(sfScale).value() // << ", assetsTotal=" << vault->at(sfAssetsTotal).value() << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) @@ -260,15 +273,40 @@ VaultDeposit::doApply() sharesCreated.asset() != assetsDeposited.asset(), "ripple::VaultDeposit::doApply : assets are not shares"); - vault->at(sfAssetsTotal) += assetsDeposited; - vault->at(sfAssetsAvailable) += assetsDeposited; + auto assetsTotalProxy = vault->at(sfAssetsTotal); + auto assetsAvailableProxy = vault->at(sfAssetsAvailable); + if (vaultAsset.value().integral()) + { + assetsTotalProxy.value().setLimited(true); + assetsAvailableProxy.value().setLimited(true); + } + assetsTotalProxy += assetsDeposited; + assetsAvailableProxy += assetsDeposited; + if (!assetsTotalProxy.value().fits() || + !assetsAvailableProxy.value().fits()) + { + // It's easy to hit this exception from Number with large enough + // Scale so we avoid spamming the log and only use debug here. + JLOG(j_.debug()) // + << "VaultDeposit: integer overflow error in total assets with" + << " scale=" << (int)vault->at(sfScale).value() // + << ", assetsTotal=" << vault->at(sfAssetsTotal).value() + << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) + << ", amount=" << amount; + + return tecPRECISION_LOSS; + } + view().update(vault); // A deposit must not push the vault over its limit. auto const maximum = *vault->at(sfAssetsMaximum); - if (maximum != 0 && *vault->at(sfAssetsTotal) > maximum) + if (maximum != 0 && *assetsTotalProxy > maximum) return tecLIMIT_EXCEEDED; + // Reset the sleIssance ptr, since it's about to get invalidated + sleIssuance.reset(); + // Transfer assets from depositor to vault. if (auto const ter = accountSend( view(), @@ -306,6 +344,37 @@ VaultDeposit::doApply() !isTesSuccess(ter)) return ter; + { + // Load the updated issuance + sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID)); + if (!sleIssuance) + { + // LCOV_EXCL_START + JLOG(j_.error()) + << "VaultDeposit: missing issuance of vault shares."; + return tefINTERNAL; + // LCOV_EXCL_STOP + } + + // Check if the deposit pushed the total over the integer Number limit. + // That is not a problem for the MPT itself, which is 64-bit, but for + // any computations that use it, such as converting assets to shares and + // vice-versa + STAmount const shareTotal{ + vault->at(sfShareMPTID), sleIssuance->at(sfOutstandingAmount)}; + if (!shareTotal.numberFits()) + { + JLOG(j_.warn()) // + << "VaultDeposit: integer overflow error in total shares with" + << " scale=" << (int)vault->at(sfScale).value() // + << ", assetsTotal=" << vault->at(sfAssetsTotal).value() + << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) + << ", amount=" << amount; + + return tecPRECISION_LOSS; + } + } + return tesSUCCESS; } diff --git a/src/xrpld/app/tx/detail/VaultSet.cpp b/src/xrpld/app/tx/detail/VaultSet.cpp index 38ab6296ef5..b832dc05a12 100644 --- a/src/xrpld/app/tx/detail/VaultSet.cpp +++ b/src/xrpld/app/tx/detail/VaultSet.cpp @@ -138,12 +138,18 @@ VaultSet::doApply() // Update mutable flags and fields if given. if (tx.isFieldPresent(sfData)) vault->at(sfData) = tx[sfData]; - if (tx.isFieldPresent(sfAssetsMaximum)) + if (auto const value = tx[~sfAssetsMaximum]) { - if (tx[sfAssetsMaximum] != 0 && - tx[sfAssetsMaximum] < *vault->at(sfAssetsTotal)) + if (*value != 0 && *value < *vault->at(sfAssetsTotal)) return tecLIMIT_EXCEEDED; - vault->at(sfAssetsMaximum) = tx[sfAssetsMaximum]; + auto assetsMaximumProxy = vault->at(sfAssetsMaximum); + assetsMaximumProxy = *value; + if (vault->at(sfAsset).value().integral()) + { + assetsMaximumProxy.value().setLimited(true); + if (!assetsMaximumProxy.value().representable()) + return tecPRECISION_LOSS; + } } 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 7777f2257f9..b310909e78b 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -20,7 +20,10 @@ VaultWithdraw::preflight(PreflightContext const& ctx) return temMALFORMED; } - if (ctx.tx[sfAmount] <= beast::zero) + auto const amount = ctx.tx[sfAmount]; + if (amount <= beast::zero) + return temBAD_AMOUNT; + if (!amount.representableNumber()) return temBAD_AMOUNT; if (auto const destination = ctx.tx[~sfDestination]; @@ -155,7 +158,7 @@ VaultWithdraw::doApply() Asset const vaultAsset = vault->at(sfAsset); MPTIssue const share{mptIssuanceID}; STAmount sharesRedeemed = {share}; - STAmount assetsWithdrawn; + STAmount assetsWithdrawn = {vaultAsset}; try { if (amount.asset() == vaultAsset) @@ -189,6 +192,12 @@ VaultWithdraw::doApply() } else return tefINTERNAL; // LCOV_EXCL_LINE + // Withdraw amounts are allowed to be invalid, but not unrepresentable. + // Amounts over the "soft" limit help bring the numbers back into the + // valid range. + if (!sharesRedeemed.representableNumber() || + !assetsWithdrawn.representableNumber()) + return tecPRECISION_LOSS; } catch (std::overflow_error const&) { @@ -215,6 +224,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); diff --git a/src/xrpld/app/tx/detail/applySteps.cpp b/src/xrpld/app/tx/detail/applySteps.cpp index 81a930f5622..39a5c5ee905 100644 --- a/src/xrpld/app/tx/detail/applySteps.cpp +++ b/src/xrpld/app/tx/detail/applySteps.cpp @@ -34,8 +34,31 @@ struct UnknownTxnType : std::exception // throw an "UnknownTxnType" exception on error template auto -with_txn_type(TxType txnType, F&& f) +with_txn_type(Rules const& rules, TxType txnType, F&& f) { + // These global updates really should have been for every Transaction + // step: preflight, preclaim, calculateBaseFee, and doApply. Unfortunately, + // they were only included in doApply (via Transactor::operator()). That may + // have been sufficient when the changes were only related to operations + // that mutated data, but some features will now change how they read data, + // so these need to be more global. + // + // To prevent unintentional side effects on existing checks, they will be + // set for every operation only once SingleAssetVault (or later + // LendingProtocol) are enabled. + // + // See also Transactor::operator(). + // + std::optional stNumberSO; + std::optional rulesGuard; + if (rules.enabled(featureSingleAssetVault) /*|| rules.enabled(featureLendingProtocol)*/) + { + // raii classes for the current ledger rules. + // fixUniversalNumber predate the rulesGuard and should be replaced. + stNumberSO.emplace(rules.enabled(fixUniversalNumber)); + rulesGuard.emplace(rules); + } + switch (txnType) { #pragma push_macro("TRANSACTION") @@ -99,7 +122,7 @@ invoke_preflight(PreflightContext const& ctx) { try { - return with_txn_type(ctx.tx.getTxnType(), [&]() { + return with_txn_type(ctx.rules, ctx.tx.getTxnType(), [&]() { auto const tec = Transactor::invokePreflight(ctx); return std::make_pair( tec, @@ -126,50 +149,51 @@ invoke_preclaim(PreclaimContext const& ctx) { // use name hiding to accomplish compile-time polymorphism of static // class functions for Transactor and derived classes. - return with_txn_type(ctx.tx.getTxnType(), [&]() -> TER { - // preclaim functionality is divided into two sections: - // 1. Up to and including the signature check: returns NotTEC. - // All transaction checks before and including checkSign - // MUST return NotTEC, or something more restrictive. - // Allowing tec results in these steps risks theft or - // destruction of funds, as a fee will be charged before the - // signature is checked. - // 2. After the signature check: returns TER. - - // If the transactor requires a valid account and the - // transaction doesn't list one, preflight will have already - // a flagged a failure. - auto const id = ctx.tx.getAccountID(sfAccount); - - if (id != beast::zero) - { - if (NotTEC const preSigResult = [&]() -> NotTEC { - if (NotTEC const result = - T::checkSeqProxy(ctx.view, ctx.tx, ctx.j)) - return result; - - if (NotTEC const result = - T::checkPriorTxAndLastLedger(ctx)) - return result; - - if (NotTEC const result = - T::checkPermission(ctx.view, ctx.tx)) - return result; - - if (NotTEC const result = T::checkSign(ctx)) - return result; - - return tesSUCCESS; - }()) - return preSigResult; - - if (TER const result = - T::checkFee(ctx, calculateBaseFee(ctx.view, ctx.tx))) - return result; - } - - return T::preclaim(ctx); - }); + return with_txn_type( + ctx.view.rules(), ctx.tx.getTxnType(), [&]() -> TER { + // preclaim functionality is divided into two sections: + // 1. Up to and including the signature check: returns NotTEC. + // All transaction checks before and including checkSign + // MUST return NotTEC, or something more restrictive. + // Allowing tec results in these steps risks theft or + // destruction of funds, as a fee will be charged before the + // signature is checked. + // 2. After the signature check: returns TER. + + // If the transactor requires a valid account and the + // transaction doesn't list one, preflight will have already + // a flagged a failure. + auto const id = ctx.tx.getAccountID(sfAccount); + + if (id != beast::zero) + { + if (NotTEC const preSigResult = [&]() -> NotTEC { + if (NotTEC const result = + T::checkSeqProxy(ctx.view, ctx.tx, ctx.j)) + return result; + + if (NotTEC const result = + T::checkPriorTxAndLastLedger(ctx)) + return result; + + if (NotTEC const result = + T::checkPermission(ctx.view, ctx.tx)) + return result; + + if (NotTEC const result = T::checkSign(ctx)) + return result; + + return tesSUCCESS; + }()) + return preSigResult; + + if (TER const result = T::checkFee( + ctx, calculateBaseFee(ctx.view, ctx.tx))) + return result; + } + + return T::preclaim(ctx); + }); } catch (UnknownTxnType const& e) { @@ -204,7 +228,7 @@ invoke_calculateBaseFee(ReadView const& view, STTx const& tx) { try { - return with_txn_type(tx.getTxnType(), [&]() { + return with_txn_type(view.rules(), tx.getTxnType(), [&]() { return T::calculateBaseFee(view, tx); }); } @@ -264,10 +288,11 @@ invoke_apply(ApplyContext& ctx) { try { - return with_txn_type(ctx.tx.getTxnType(), [&]() { - T p(ctx); - return p(); - }); + return with_txn_type( + ctx.view().rules(), ctx.tx.getTxnType(), [&]() { + T p(ctx); + return p(); + }); } catch (UnknownTxnType const& e) {