From 3cb447a4fe7866a533ed9a577ca392d3898cebe5 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 4 Nov 2025 20:44:45 -0500 Subject: [PATCH 01/23] Add optional enforcement of valid integer range to Number --- include/xrpl/basics/Number.h | 116 ++++++++++++++++++++-- src/libxrpl/basics/Number.cpp | 40 ++++++++ src/test/basics/Number_test.cpp | 168 ++++++++++++++++++++++++++++++++ 3 files changed, 317 insertions(+), 7 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index e34cc61b5b3..59f3a212a6a 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -13,16 +13,47 @@ 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 { +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. + */ + enum EnforceInteger { none, weak, strong }; + +private: using rep = std::int64_t; rep mantissa_{0}; int exponent_{std::numeric_limits::lowest()}; + // 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. + EnforceInteger enforceInteger_ = none; + 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 = minMantissa / 10; // The range for the exponent when normalized constexpr static int minExponent = -32768; @@ -35,15 +66,33 @@ class Number explicit constexpr Number() = default; - Number(rep mantissa); - explicit Number(rep mantissa, int exponent); + Number(rep mantissa, EnforceInteger enforce = none); + explicit Number(rep mantissa, int exponent, EnforceInteger enforce = none); 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; + void + setIntegerEnforcement(EnforceInteger enforce); + + EnforceInteger + integerEnforcement() const noexcept; + + bool + valid() const noexcept; + constexpr Number operator+() const noexcept; constexpr Number @@ -184,6 +233,9 @@ class Number private: static thread_local rounding_mode mode_; + void + checkInteger(char const* what) const; + void normalize(); constexpr bool @@ -197,16 +249,52 @@ 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, EnforceInteger enforce) + : mantissa_{mantissa}, exponent_{exponent}, enforceInteger_(enforce) { normalize(); + + checkInteger("Number::Number integer overflow"); } -inline Number::Number(rep mantissa) : Number{mantissa, 0} +inline Number::Number(rep mantissa, EnforceInteger enforce) + : Number{mantissa, 0, enforce} { } +constexpr Number& +Number::operator=(Number const& other) +{ + if (this != &other) + { + mantissa_ = other.mantissa_; + exponent_ = other.exponent_; + enforceInteger_ = std::max(enforceInteger_, other.enforceInteger_); + + checkInteger("Number::operator= integer overflow"); + } + + 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 (other.enforceInteger_ > enforceInteger_) + enforceInteger_ = std::move(other.enforceInteger_); + + checkInteger("Number::operator= integer overflow"); + } + + return *this; +} + inline constexpr Number::rep Number::mantissa() const noexcept { @@ -219,6 +307,20 @@ Number::exponent() const noexcept return exponent_; } +inline void +Number::setIntegerEnforcement(EnforceInteger enforce) +{ + enforceInteger_ = enforce; + + checkInteger("Number::setIntegerEnforcement integer overflow"); +} + +inline Number::EnforceInteger +Number::integerEnforcement() const noexcept +{ + return enforceInteger_; +} + inline constexpr Number Number::operator+() const noexcept { diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 89f7937e068..e3789de90a6 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -155,6 +155,13 @@ Number::Guard::round() noexcept constexpr Number one{1000000000000000, -15, Number::unchecked{}}; +void +Number::checkInteger(char const* what) const +{ + if (enforceInteger_ == strong && !valid()) + throw std::overflow_error(what); +} + void Number::normalize() { @@ -207,9 +214,27 @@ Number::normalize() mantissa_ = -mantissa_; } +bool +Number::valid() const noexcept +{ + if (enforceInteger_ != none) + { + static Number const max = maxIntValue; + static Number const maxNeg = -maxIntValue; + // Avoid making a copy + if (mantissa_ < 0) + return *this >= maxNeg; + return *this <= max; + } + return true; +} + Number& Number::operator+=(Number const& y) { + // The strictest setting prevails + enforceInteger_ = std::max(enforceInteger_, y.enforceInteger_); + if (y == Number{}) return *this; if (*this == Number{}) @@ -322,6 +347,9 @@ Number::operator+=(Number const& y) } mantissa_ = xm * xn; exponent_ = xe; + + checkInteger("Number::addition integer overflow"); + return *this; } @@ -356,6 +384,9 @@ divu10(uint128_t& u) Number& Number::operator*=(Number const& y) { + // The strictest setting prevails + enforceInteger_ = std::max(enforceInteger_, y.enforceInteger_); + if (*this == Number{}) return *this; if (y == Number{}) @@ -422,12 +453,18 @@ Number::operator*=(Number const& y) XRPL_ASSERT( isnormal() || *this == Number{}, "ripple::Number::operator*=(Number) : result is normal"); + + checkInteger("Number::multiplication integer overflow"); + return *this; } Number& Number::operator/=(Number const& y) { + // The strictest setting prevails + enforceInteger_ = std::max(enforceInteger_, y.enforceInteger_); + if (y == Number{}) throw std::overflow_error("Number: divide by 0"); if (*this == Number{}) @@ -455,6 +492,9 @@ Number::operator/=(Number const& y) exponent_ = ne - de - 17; mantissa_ *= np * dp; normalize(); + + checkInteger("Number::division integer overflow"); + return *this; } diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 06203a4c2aa..78c9b289520 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -725,6 +726,172 @@ 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; + + { + Number a{100}; + BEAST_EXPECT(a.integerEnforcement() == Number::none); + BEAST_EXPECT(a.valid()); + a = Number{1, 30}; + BEAST_EXPECT(a.valid()); + a = -100; + BEAST_EXPECT(a.valid()); + } + { + Number a{100, Number::weak}; + BEAST_EXPECT(a.integerEnforcement() == Number::weak); + BEAST_EXPECT(a.valid()); + a = Number{1, 30, Number::none}; + BEAST_EXPECT(!a.valid()); + a = -100; + BEAST_EXPECT(a.integerEnforcement() == Number::weak); + BEAST_EXPECT(a.valid()); + a = Number{5, Number::strong}; + BEAST_EXPECT(a.integerEnforcement() == Number::strong); + BEAST_EXPECT(a.valid()); + } + { + Number a{100, Number::strong}; + BEAST_EXPECT(a.integerEnforcement() == Number::strong); + BEAST_EXPECT(a.valid()); + try + { + a = Number{1, 30}; + 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.valid()); + a = -100; + BEAST_EXPECT(a.integerEnforcement() == Number::strong); + BEAST_EXPECT(a.valid()); + } + { + Number a{INITIAL_XRP.drops(), Number::weak}; + BEAST_EXPECT(!a.valid()); + a = -a; + BEAST_EXPECT(!a.valid()); + + try + { + a.setIntegerEnforcement(Number::strong); + BEAST_EXPECT(false); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECT( + e.what() == + "Number::setIntegerEnforcement integer overflow"s); + // The throw is internal to the operator before the result is + // assigned to the Number + BEAST_EXPECT(a == -INITIAL_XRP); + BEAST_EXPECT(!a.valid()); + } + try + { + ++a; + BEAST_EXPECT(false); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECT(e.what() == "Number::addition integer overflow"s); + // The throw is internal to the operator before the result is + // assigned to the Number + BEAST_EXPECT(a == -INITIAL_XRP); + BEAST_EXPECT(!a.valid()); + } + a = Number::maxIntValue; + try + { + ++a; + BEAST_EXPECT(false); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECT(e.what() == "Number::addition integer overflow"s); + // This time, the throw is done _after_ the number is updated. + BEAST_EXPECT(a == Number::maxIntValue + 1); + BEAST_EXPECT(!a.valid()); + } + a = -Number::maxIntValue; + try + { + --a; + BEAST_EXPECT(false); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECT(e.what() == "Number::addition integer overflow"s); + // This time, the throw is done _after_ the number is updated. + BEAST_EXPECT(a == -Number::maxIntValue - 1); + BEAST_EXPECT(!a.valid()); + } + a = Number(1, 10); + try + { + a *= Number(1, 10); + BEAST_EXPECT(false); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECT( + e.what() == "Number::multiplication integer overflow"s); + // The throw is done _after_ the number is updated. + BEAST_EXPECT((a == Number{1, 20})); + BEAST_EXPECT(!a.valid()); + } + try + { + a = Number::maxIntValue * 2; + 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{2, 14})); + BEAST_EXPECT(!a.valid()); + } + try + { + a = Number(3, 15, Number::strong); + BEAST_EXPECT(false); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECT(e.what() == "Number::Number integer overflow"s); + // The Number doesn't get updated because the ctor throws + BEAST_EXPECT((a == Number{2, 14})); + BEAST_EXPECT(!a.valid()); + } + a = Number(1, 10); + try + { + a /= Number(1, -10); + BEAST_EXPECT(false); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECT(e.what() == "Number::division integer overflow"s); + // The throw is done _after_ the number is updated. + BEAST_EXPECT((a == Number{1, 20})); + BEAST_EXPECT(!a.valid()); + } + a /= Number(1, 15); + BEAST_EXPECT((a == Number{1, 5})); + BEAST_EXPECT(a.valid()); + } + } + void run() override { @@ -746,6 +913,7 @@ class Number_test : public beast::unit_test::suite test_inc_dec(); test_toSTAmount(); test_truncate(); + testInteger(); } }; From 24f37d73f614dafbd522068781532e06584ce68e Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 4 Nov 2025 21:02:39 -0500 Subject: [PATCH 02/23] Make all STNumber fields "soeDEFAULT" --- include/xrpl/protocol/detail/ledger_entries.macro | 6 +++--- src/test/app/Vault_test.cpp | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) 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/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index ccbf0fed42a..58b929f1a2a 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -4438,7 +4438,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)); From b605a2cdcc2ac8ca27e3b44036b2ea375069ac89 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 4 Nov 2025 21:07:01 -0500 Subject: [PATCH 03/23] Add integer enforcement when converting to XRP/MPTAmount to Number --- include/xrpl/protocol/MPTAmount.h | 2 +- include/xrpl/protocol/XRPAmount.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/xrpl/protocol/MPTAmount.h b/include/xrpl/protocol/MPTAmount.h index af147865017..1c7c7a25e7e 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(), Number::strong}; } /** Return the sign of the amount */ diff --git a/include/xrpl/protocol/XRPAmount.h b/include/xrpl/protocol/XRPAmount.h index e102a3707fa..f26bb7366f8 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(), Number::weak}; } /** Return the sign of the amount */ From cb6df196dc00dad4ce4e48d6623b88b765e9a27c Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 5 Nov 2025 09:21:12 -0500 Subject: [PATCH 04/23] Fix build error - avoid copy --- src/test/app/AMM_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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}}) { From 0175dd70dbc6a962e93c1fd4ec4cbadc1bcfd903 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 5 Nov 2025 18:15:49 -0500 Subject: [PATCH 05/23] Catch up the consequences of Number changes - Change the Number::maxIntValue to all 9's. - Add integral() to Asset (copied from Lending) - Add toNumber() functions to STAmount, MPTAmount, XRPAmount to allow explicit conversions with enforcement options. - Add optional Number::EnforceInteger options to STAmount and STNumber ctors, conversions, etc. IOUs are never checked. - Update Vault transactors, and helper functions, to check restrictions. - Fix and add Vault tests. --- include/xrpl/basics/Number.h | 3 +- include/xrpl/protocol/Asset.h | 6 ++ include/xrpl/protocol/MPTAmount.h | 8 ++- include/xrpl/protocol/STAmount.h | 67 ++++++++++++++++++- include/xrpl/protocol/STNumber.h | 12 ++++ include/xrpl/protocol/SystemParameters.h | 1 + include/xrpl/protocol/XRPAmount.h | 6 ++ src/libxrpl/ledger/View.cpp | 23 +++++-- src/libxrpl/protocol/STAmount.cpp | 19 ++++++ src/libxrpl/protocol/STNumber.cpp | 18 ++++++ src/test/app/Vault_test.cpp | 78 +++++++++++++++++++++-- src/test/basics/Number_test.cpp | 4 +- src/xrpld/app/tx/detail/VaultClawback.cpp | 19 +++++- src/xrpld/app/tx/detail/VaultDeposit.cpp | 23 +++++-- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 11 +++- 15 files changed, 274 insertions(+), 24 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 59f3a212a6a..4f447d5f981 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -53,7 +53,8 @@ class Number constexpr static rep maxMantissa = minMantissa * 10 - 1; static_assert(maxMantissa == 9'999'999'999'999'999LL); - constexpr static rep maxIntValue = minMantissa / 10; + constexpr static rep maxIntValue = maxMantissa / 10; + static_assert(maxIntValue == 999'999'999'999'999LL); // The range for the exponent when normalized constexpr static int minExponent = -32768; diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index d0efe814a92..6765f7cf7d9 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -84,6 +84,12 @@ class Asset return holds() && get().native(); } + bool + integral() const + { + return !holds() || get().native(); + } + friend constexpr bool operator==(Asset const& lhs, Asset const& rhs); diff --git a/include/xrpl/protocol/MPTAmount.h b/include/xrpl/protocol/MPTAmount.h index 1c7c7a25e7e..5f552d839b0 100644 --- a/include/xrpl/protocol/MPTAmount.h +++ b/include/xrpl/protocol/MPTAmount.h @@ -62,11 +62,17 @@ class MPTAmount : private boost::totally_ordered, explicit constexpr operator bool() const noexcept; - operator Number() const noexcept + operator Number() const { return {value(), Number::strong}; } + Number + toNumber(Number::EnforceInteger enforce) const + { + return {value(), enforce}; + } + /** Return the sign of the amount */ constexpr int signum() const noexcept; diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index 83493efcdda..d0092fdbec2 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -40,6 +40,12 @@ class STAmount final : public STBase, public CountedObject exponent_type mOffset; bool mIsNegative; + // The Enforce integer setting is not stored or serialized. If set, it is + // used during automatic conversions to Number. If not set, the default + // behavior is used. It can also be overridden when coverting by using + // toNumber(). + std::optional enforceConversion_; + public: using value_type = STAmount; @@ -135,9 +141,28 @@ class STAmount final : public STBase, public CountedObject STAmount(A const& asset, int mantissa, int exponent = 0); template - STAmount(A const& asset, Number const& number) + STAmount( + A const& asset, + Number const& number, + std::optional enforce = std::nullopt) : STAmount(asset, number.mantissa(), number.exponent()) { + enforceConversion_ = enforce; + if (!enforce) + { + // Use the default conversion behavior + [[maybe_unused]] + Number const n = *this; + } + else if (enforce == Number::strong) + { + // Throw if it's not valid + if (!validNumber()) + { + Throw( + "STAmount::STAmount integer Number lost precision"); + } + } } // Legacy support for new-style amounts @@ -145,6 +170,17 @@ class STAmount final : public STBase, public CountedObject STAmount(XRPAmount const& amount); STAmount(MPTAmount const& amount, MPTIssue const& mptIssue); operator Number() const; + Number + toNumber(Number::EnforceInteger enforce) const; + + void + setIntegerEnforcement(std::optional enforce); + + std::optional + integerEnforcement() const noexcept; + + bool + validNumber() const noexcept; //-------------------------------------------------------------------------- // @@ -155,6 +191,9 @@ class STAmount final : public STBase, public CountedObject int exponent() const noexcept; + bool + integral() const noexcept; + bool native() const noexcept; @@ -435,6 +474,12 @@ STAmount::exponent() const noexcept return mOffset; } +inline bool +STAmount::integral() const noexcept +{ + return mAsset.integral(); +} + inline bool STAmount::native() const noexcept { @@ -510,6 +555,8 @@ inline STAmount::operator bool() const noexcept inline STAmount::operator Number() const { + if (enforceConversion_) + return toNumber(*enforceConversion_); if (native()) return xrp(); if (mAsset.holds()) @@ -517,6 +564,17 @@ inline STAmount::operator Number() const return iou(); } +inline Number +STAmount::toNumber(Number::EnforceInteger enforce) const +{ + if (native()) + return xrp().toNumber(enforce); + if (mAsset.holds()) + return mpt().toNumber(enforce); + // It doesn't make sense to enforce limits on IOUs + return iou(); +} + inline STAmount& STAmount::operator=(beast::Zero) { @@ -538,6 +596,11 @@ STAmount::operator=(Number const& number) mValue = mIsNegative ? -number.mantissa() : number.mantissa(); mOffset = number.exponent(); canonicalize(); + + // Convert it back to a Number to check that it's valid + [[maybe_unused]] + Number n = *this; + return *this; } @@ -553,7 +616,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/STNumber.h b/include/xrpl/protocol/STNumber.h index 2ec3d66fd14..43b96a2b465 100644 --- a/include/xrpl/protocol/STNumber.h +++ b/include/xrpl/protocol/STNumber.h @@ -56,6 +56,18 @@ class STNumber : public STBase, public CountedObject bool isDefault() const override; + /// Sets the flag on the underlying number + void + setIntegerEnforcement(Number::EnforceInteger enforce); + + /// Gets the flag value on the underlying number + Number::EnforceInteger + integerEnforcement() const noexcept; + + /// Checks the underlying number + bool + valid() const noexcept; + operator Number() const { return value_; 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 f26bb7366f8..159174accc0 100644 --- a/include/xrpl/protocol/XRPAmount.h +++ b/include/xrpl/protocol/XRPAmount.h @@ -146,6 +146,12 @@ class XRPAmount : private boost::totally_ordered, return {drops(), Number::weak}; } + Number + toNumber(Number::EnforceInteger enforce) const + { + return {value(), enforce}; + } + /** Return the sign of the amount */ constexpr int signum() const noexcept diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 0175b099eaf..92fd5ccc977 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2878,13 +2878,17 @@ assetsToSharesDeposit( Number const assetTotal = vault->at(sfAssetsTotal); STAmount shares{vault->at(sfShareMPTID)}; + shares.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return STAmount{ shares.asset(), Number(assets.mantissa(), assets.exponent() + vault->at(sfScale)) - .truncate()}; + .truncate(), + Number::weak}; - Number const shareTotal = issuance->at(sfOutstandingAmount); + Number const shareTotal{ + unsafe_cast(issuance->at(sfOutstandingAmount)), + Number::strong}; shares = (shareTotal * (assets / assetTotal)).truncate(); return shares; } @@ -2906,6 +2910,7 @@ sharesToAssetsDeposit( Number const assetTotal = vault->at(sfAssetsTotal); STAmount assets{vault->at(sfAsset)}; + assets.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return STAmount{ assets.asset(), @@ -2913,7 +2918,9 @@ sharesToAssetsDeposit( shares.exponent() - vault->at(sfScale), false}; - Number const shareTotal = issuance->at(sfOutstandingAmount); + Number const shareTotal{ + unsafe_cast(issuance->at(sfOutstandingAmount)), + Number::strong}; assets = assetTotal * (shares / shareTotal); return assets; } @@ -2937,9 +2944,12 @@ assetsToSharesWithdraw( Number assetTotal = vault->at(sfAssetsTotal); assetTotal -= vault->at(sfLossUnrealized); STAmount shares{vault->at(sfShareMPTID)}; + shares.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return shares; - Number const shareTotal = issuance->at(sfOutstandingAmount); + Number const shareTotal{ + unsafe_cast(issuance->at(sfOutstandingAmount)), + Number::strong}; Number result = shareTotal * (assets / assetTotal); if (truncate == TruncateShares::yes) result = result.truncate(); @@ -2965,9 +2975,12 @@ sharesToAssetsWithdraw( Number assetTotal = vault->at(sfAssetsTotal); assetTotal -= vault->at(sfLossUnrealized); STAmount assets{vault->at(sfAsset)}; + assets.setIntegerEnforcement(Number::weak); if (assetTotal == 0) return assets; - Number const shareTotal = issuance->at(sfOutstandingAmount); + Number const shareTotal{ + unsafe_cast(issuance->at(sfOutstandingAmount)), + Number::strong}; assets = assetTotal * (shares / shareTotal); return assets; } diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 4aac5510038..6f6355a145c 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -255,6 +255,25 @@ STAmount::move(std::size_t n, void* buf) return emplace(n, buf, std::move(*this)); } +void +STAmount::setIntegerEnforcement(std::optional enforce) +{ + enforceConversion_ = enforce; +} + +std::optional +STAmount::integerEnforcement() const noexcept +{ + return enforceConversion_; +} + +bool +STAmount::validNumber() const noexcept +{ + Number n = toNumber(Number::EnforceInteger::weak); + return n.valid(); +} + //------------------------------------------------------------------------------ // // Conversion diff --git a/src/libxrpl/protocol/STNumber.cpp b/src/libxrpl/protocol/STNumber.cpp index 889dd9ee684..5486864e955 100644 --- a/src/libxrpl/protocol/STNumber.cpp +++ b/src/libxrpl/protocol/STNumber.cpp @@ -94,6 +94,24 @@ STNumber::isDefault() const return value_ == Number(); } +void +STNumber::setIntegerEnforcement(Number::EnforceInteger enforce) +{ + value_.setIntegerEnforcement(enforce); +} + +Number::EnforceInteger +STNumber::integerEnforcement() const noexcept +{ + return value_.integerEnforcement(); +} + +bool +STNumber::valid() const noexcept +{ + return value_.valid(); +} + std::ostream& operator<<(std::ostream& out, STNumber const& rhs) { diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 58b929f1a2a..5b36ffaf8ef 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3597,7 +3597,32 @@ class Vault_test : public beast::unit_test::suite }); testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow on second deposit"); + testcase("MPT scale deposit overflow"); + // The computed number of shares can not be represented as an MPT + // without truncation + + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(5)}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + }); + + testCase(14, [&, this](Env& env, Data d) { + testcase("MPT scale deposit overflow on first deposit"); + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(10)}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + }); + + testCase(14, [&, this](Env& env, Data d) { + testcase("MPT scale deposit overflow on second deposit"); { auto tx = d.vault.deposit( @@ -3618,8 +3643,8 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow on total shares"); + testCase(14, [&, this](Env& env, Data d) { + testcase("No MPT scale deposit overflow on total shares"); { auto tx = d.vault.deposit( @@ -3635,7 +3660,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); env.close(); } }); @@ -3919,6 +3944,28 @@ class Vault_test : public beast::unit_test::suite testCase(18, [&, this](Env& env, Data d) { testcase("Scale withdraw overflow"); + { + 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.withdraw( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = STAmount(d.asset, Number(10, 0))}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + }); + + testCase(14, [&, this](Env& env, Data d) { + testcase("MPT scale withdraw overflow"); + { auto tx = d.vault.deposit( {.depositor = d.depositor, @@ -4137,6 +4184,29 @@ class Vault_test : public beast::unit_test::suite testCase(18, [&, this](Env& env, Data d) { testcase("Scale clawback overflow"); + { + 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.clawback( + {.issuer = d.issuer, + .id = d.keylet.key, + .holder = d.depositor, + .amount = STAmount(d.asset, Number(10, 0))}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + }); + + testCase(14, [&, this](Env& env, Data d) { + testcase("MPT Scale clawback overflow"); + { auto tx = d.vault.deposit( {.depositor = d.depositor, diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 78c9b289520..a5fba8ef7da 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -858,7 +858,7 @@ class Number_test : public beast::unit_test::suite { BEAST_EXPECT(e.what() == "Number::operator= integer overflow"s); // The throw is done _after_ the number is updated. - BEAST_EXPECT((a == Number{2, 14})); + BEAST_EXPECT((a == Number::maxIntValue * 2)); BEAST_EXPECT(!a.valid()); } try @@ -870,7 +870,7 @@ class Number_test : public beast::unit_test::suite { BEAST_EXPECT(e.what() == "Number::Number integer overflow"s); // The Number doesn't get updated because the ctor throws - BEAST_EXPECT((a == Number{2, 14})); + BEAST_EXPECT((a == Number::maxIntValue * 2)); BEAST_EXPECT(!a.valid()); } a = Number(1, 10); diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index 7c56ca1d600..1c73d36bb07 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -71,9 +71,13 @@ VaultClawback::preclaim(PreclaimContext const& ctx) } Asset const vaultAsset = vault->at(sfAsset); - if (auto const amount = ctx.tx[~sfAmount]; - amount && vaultAsset != amount->asset()) - return tecWRONG_ASSET; + if (auto const amount = ctx.tx[~sfAmount]) + { + if (vaultAsset != amount->asset()) + return tecWRONG_ASSET; + else if (!amount->validNumber()) + return tecPRECISION_LOSS; + } if (vaultAsset.native()) { @@ -157,6 +161,8 @@ VaultClawback::doApply() MPTIssue const share{mptIssuanceID}; STAmount sharesDestroyed = {share}; STAmount assetsRecovered; + assetsRecovered.setIntegerEnforcement(Number::weak); + sharesDestroyed.setIntegerEnforcement(Number::weak); try { if (amount == beast::zero) @@ -169,6 +175,9 @@ VaultClawback::doApply() AuthHandling::ahIGNORE_AUTH, j_); + if (!sharesDestroyed.validNumber()) + return tecPRECISION_LOSS; + auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, sharesDestroyed); if (!maybeAssets) @@ -184,6 +193,8 @@ VaultClawback::doApply() if (!maybeShares) return tecINTERNAL; // LCOV_EXCL_LINE sharesDestroyed = *maybeShares; + if (!sharesDestroyed.validNumber()) + return tecPRECISION_LOSS; } auto const maybeAssets = @@ -192,6 +203,8 @@ VaultClawback::doApply() return tecINTERNAL; // LCOV_EXCL_LINE assetsRecovered = *maybeAssets; } + if (!assetsRecovered.validNumber()) + return tecPRECISION_LOSS; // Clamp to maximum. if (assetsRecovered > *assetsAvailable) diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 3e5ae741e30..66e144312fa 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -42,6 +42,9 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) if (assets.asset() != vaultAsset) return tecWRONG_ASSET; + if (!assets.validNumber()) + return tecPRECISION_LOSS; + if (vaultAsset.native()) ; // No special checks for XRP else if (vaultAsset.holds()) @@ -217,6 +220,7 @@ VaultDeposit::doApply() } STAmount sharesCreated = {vault->at(sfShareMPTID)}, assetsDeposited; + sharesCreated.setIntegerEnforcement(Number::weak); try { // Compute exchange before transferring any amounts. @@ -227,14 +231,14 @@ VaultDeposit::doApply() return tecINTERNAL; // LCOV_EXCL_LINE sharesCreated = *maybeShares; } - if (sharesCreated == beast::zero) + if (sharesCreated == beast::zero || !sharesCreated.validNumber()) return tecPRECISION_LOSS; auto const maybeAssets = sharesToAssetsDeposit(vault, sleIssuance, sharesCreated); if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE - else if (*maybeAssets > amount) + else if (*maybeAssets > amount || !maybeAssets->validNumber()) { // LCOV_EXCL_START JLOG(j_.error()) << "VaultDeposit: would take more than offered."; @@ -260,13 +264,22 @@ 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 (vault->at(sfAsset).value().integral()) + { + assetsTotalProxy.value().setIntegerEnforcement(Number::weak); + assetsAvailableProxy.value().setIntegerEnforcement(Number::weak); + } + assetsTotalProxy += assetsDeposited; + assetsAvailableProxy += assetsDeposited; + if (!assetsTotalProxy->valid() || !assetsAvailableProxy->valid()) + return tecLIMIT_EXCEEDED; 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; // Transfer assets from depositor to vault. diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index 8cd3f5cd97d..8806f7b236a 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -50,6 +50,9 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (assets.asset() != vaultAsset && assets.asset() != vaultShare) return tecWRONG_ASSET; + if (!assets.validNumber()) + return tecPRECISION_LOSS; + if (vaultAsset.native()) ; // No special checks for XRP else if (vaultAsset.holds()) @@ -154,6 +157,8 @@ VaultWithdraw::doApply() MPTIssue const share{mptIssuanceID}; STAmount sharesRedeemed = {share}; STAmount assetsWithdrawn; + assetsWithdrawn.setIntegerEnforcement(Number::weak); + sharesRedeemed.setIntegerEnforcement(Number::weak); try { if (amount.asset() == vaultAsset) @@ -167,13 +172,15 @@ VaultWithdraw::doApply() sharesRedeemed = *maybeShares; } - if (sharesRedeemed == beast::zero) + if (sharesRedeemed == beast::zero || !sharesRedeemed.validNumber()) return tecPRECISION_LOSS; auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, sharesRedeemed); if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE assetsWithdrawn = *maybeAssets; + if (!assetsWithdrawn.validNumber()) + return tecPRECISION_LOSS; } else if (amount.asset() == share) { @@ -184,6 +191,8 @@ VaultWithdraw::doApply() if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE assetsWithdrawn = *maybeAssets; + if (!assetsWithdrawn.validNumber()) + return tecPRECISION_LOSS; } else return tefINTERNAL; // LCOV_EXCL_LINE From 8e56af20ee9e9e35b50c66d9ab10e63ef841955d Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 7 Nov 2025 18:30:09 -0500 Subject: [PATCH 06/23] Add a distinction between a "valid" and a "representable" Number - "valid" means the value is <= Number::maxIntValue, which has been changed to maxMantissa / 100. A valid number could get bigger and be ok - such as when paying late interest on a loan. - "representable" means the value is <= Number::maxMantissa. An unrepresentable number WILL be rounded or truncated. - Adds a fourth level of enforcement: "compatible". It is used for converting XRP to Number (for AMM), and when doing explicit checks. - "weak" will now throw if the number is unrepresentable. --- include/xrpl/basics/Number.h | 31 ++++++--- include/xrpl/protocol/XRPAmount.h | 2 +- src/libxrpl/basics/Number.cpp | 32 ++++++++- src/libxrpl/ledger/View.cpp | 8 +++ src/libxrpl/protocol/STAmount.cpp | 3 +- src/test/app/Vault_test.cpp | 10 +-- src/test/basics/Number_test.cpp | 76 +++++++++++++++++++++- src/xrpld/app/tx/detail/InvariantCheck.cpp | 20 ++++++ src/xrpld/app/tx/detail/VaultClawback.cpp | 5 ++ src/xrpld/app/tx/detail/VaultCreate.cpp | 7 ++ src/xrpld/app/tx/detail/VaultDeposit.cpp | 2 + src/xrpld/app/tx/detail/VaultSet.cpp | 5 ++ src/xrpld/app/tx/detail/VaultWithdraw.cpp | 2 + 13 files changed, 184 insertions(+), 19 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 4f447d5f981..3b124e3166f 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -28,12 +28,17 @@ class Number /** 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 @@ class Number // 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 @@ class Number 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 @@ class Number 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 159174accc0..8f553a6083a 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(), Number::weak}; + return {drops(), Number::compatible}; } Number diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index e3789de90a6..489e27c79fe 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 92fd5ccc977..43fcae04f7f 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 6f6355a145c..1df3d3c18b3 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 5b36ffaf8ef..4f811181476 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 a5fba8ef7da..1efafec65fd 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -737,28 +737,87 @@ class Number_test : public beast::unit_test::suite 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 @@ class Number_test : public beast::unit_test::suite 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 @@ class Number_test : public beast::unit_test::suite // assigned to the Number BEAST_EXPECT(a == -INITIAL_XRP); BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); } try { @@ -808,6 +872,7 @@ class Number_test : public beast::unit_test::suite // 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 @@ class Number_test : public beast::unit_test::suite // 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 @@ class Number_test : public beast::unit_test::suite // 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 @@ class Number_test : public beast::unit_test::suite // 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 @@ class Number_test : public beast::unit_test::suite // 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 @@ class Number_test : public beast::unit_test::suite // 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 @@ class Number_test : public beast::unit_test::suite // 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 c15f2b64a55..0dbbd4f24a5 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 1c73d36bb07..b464c1eb28a 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 8acb40ad419..6c30739369c 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 66e144312fa..609379d4c2d 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 38ab6296ef5..75f1689d7f0 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 8806f7b236a..e28c54676ce 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); From edb9b16583063fea805a2e3a79e24a1236dfa111 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 6 Nov 2025 20:08:11 -0500 Subject: [PATCH 07/23] fix: Use ".value()" instead of "->" when with STObject::Proxy objects - Turns out that "Proxy::operator->" is not a safe substitute for "Proxy::value()." if the field is not required. The implementation is different such that "operator->" will return a null ptr if the field is not present. This includes default fields with a value of zero! --- include/xrpl/protocol/STObject.h | 4 ++++ src/xrpld/app/tx/detail/VaultDeposit.cpp | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) 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/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 609379d4c2d..53b7fd67c05 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -275,7 +275,8 @@ VaultDeposit::doApply() } assetsTotalProxy += assetsDeposited; assetsAvailableProxy += assetsDeposited; - if (!assetsTotalProxy->valid() || !assetsAvailableProxy->valid()) + if (!assetsTotalProxy.value().valid() || + !assetsAvailableProxy.value().valid()) return tecLIMIT_EXCEEDED; view().update(vault); From a45b43e8ea40a654f2dbdfafc33a3c40eb8a8233 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 10 Nov 2025 16:45:50 -0500 Subject: [PATCH 08/23] Update include/xrpl/basics/Number.h Fix typo. Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com> --- include/xrpl/basics/Number.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 3b124e3166f..7327531f6be 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -36,7 +36,7 @@ class Number * 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. + * default for automatic conversions from MPTAmount to Number. */ enum EnforceInteger { none, compatible, weak, strong }; From 8822b53bd5720f3f6a4b9a86bcbb8c6d4209e979 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sun, 16 Nov 2025 21:07:04 -0500 Subject: [PATCH 09/23] Rip out about half the code: levels, enforcement, and STAmount changes - Changed the EnforceInteger enum into a bool. - Removed enforcement by throw. - Essentially got rid of the "weak" and "strong" options. - Removed integer options from STAmount. Since there's no throwing, there's no need to override the default. --- include/xrpl/basics/Number.h | 79 +++---- include/xrpl/protocol/Asset.h | 9 +- include/xrpl/protocol/MPTAmount.h | 8 +- include/xrpl/protocol/STAmount.h | 54 +---- include/xrpl/protocol/STNumber.h | 6 +- include/xrpl/protocol/XRPAmount.h | 8 +- src/libxrpl/basics/Number.cpp | 71 +++---- src/libxrpl/ledger/View.cpp | 31 +-- src/libxrpl/protocol/STAmount.cpp | 20 +- src/libxrpl/protocol/STNumber.cpp | 10 +- src/test/basics/Number_test.cpp | 227 +++------------------ src/xrpld/app/tx/detail/InvariantCheck.cpp | 8 +- src/xrpld/app/tx/detail/VaultClawback.cpp | 14 +- src/xrpld/app/tx/detail/VaultCreate.cpp | 31 ++- src/xrpld/app/tx/detail/VaultDeposit.cpp | 21 +- src/xrpld/app/tx/detail/VaultSet.cpp | 13 +- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 13 +- 17 files changed, 175 insertions(+), 448 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 7327531f6be..6632a456ab4 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -24,31 +24,16 @@ isPowerOfTen(T value) class Number { -public: - /** Describes whether and how to enforce this number as an integer. - * - * - none: No enforcement. The value may vary freely. This is the default. - * - 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 - * default for automatic conversions from MPTAmount to Number. - */ - enum EnforceInteger { none, compatible, weak, strong }; - private: using rep = std::int64_t; rep mantissa_{0}; int exponent_{std::numeric_limits::lowest()}; - // 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. See the enum description for more detail. - EnforceInteger enforceInteger_ = none; + // isInteger_ is not serialized, transmitted, or used in + // calculations in any way. It is used only for internal validation + // of integer types. It is a one-way switch. Once it's on, it stays + // on. + bool isInteger_ = false; public: // The range for the mantissa when normalized @@ -71,8 +56,8 @@ class Number explicit constexpr Number() = default; - Number(rep mantissa, EnforceInteger enforce = none); - explicit Number(rep mantissa, int exponent, EnforceInteger enforce = none); + Number(rep mantissa, bool isInteger = false); + explicit Number(rep mantissa, int exponent, bool isInteger = false); explicit constexpr Number(rep mantissa, int exponent, unchecked) noexcept; constexpr Number(Number const& other) = default; constexpr Number(Number&& other) = default; @@ -90,22 +75,22 @@ class Number exponent() const noexcept; void - setIntegerEnforcement(EnforceInteger enforce); + setIsInteger(bool isInteger); - EnforceInteger - integerEnforcement() const noexcept; + bool + isInteger() const noexcept; bool valid() const noexcept; bool representable() const noexcept; - /// Combines setIntegerEnforcement(EnforceInteger) and valid() + /// Combines setIsInteger(bool) and valid() bool - valid(EnforceInteger enforce); + valid(bool isInteger); /// Because this function is const, it should only be used for one-off /// checks bool - valid(EnforceInteger enforce) const; + valid(bool isInteger) const; constexpr Number operator+() const noexcept; @@ -247,9 +232,6 @@ class Number private: static thread_local rounding_mode mode_; - void - checkInteger(char const* what) const; - void normalize(); constexpr bool @@ -263,16 +245,14 @@ inline constexpr Number::Number(rep mantissa, int exponent, unchecked) noexcept { } -inline Number::Number(rep mantissa, int exponent, EnforceInteger enforce) - : mantissa_{mantissa}, exponent_{exponent}, enforceInteger_(enforce) +inline Number::Number(rep mantissa, int exponent, bool isInteger) + : mantissa_{mantissa}, exponent_{exponent}, isInteger_(isInteger) { normalize(); - - checkInteger("Number::Number integer overflow"); } -inline Number::Number(rep mantissa, EnforceInteger enforce) - : Number{mantissa, 0, enforce} +inline Number::Number(rep mantissa, bool isInteger) + : Number{mantissa, 0, isInteger} { } @@ -283,9 +263,8 @@ Number::operator=(Number const& other) { mantissa_ = other.mantissa_; exponent_ = other.exponent_; - enforceInteger_ = std::max(enforceInteger_, other.enforceInteger_); - - checkInteger("Number::operator= integer overflow"); + if (!isInteger_) + isInteger_ = other.isInteger_; } return *this; @@ -300,10 +279,8 @@ Number::operator=(Number&& other) // this is future-proof in case the types ever change mantissa_ = std::move(other.mantissa_); exponent_ = std::move(other.exponent_); - if (other.enforceInteger_ > enforceInteger_) - enforceInteger_ = std::move(other.enforceInteger_); - - checkInteger("Number::operator= integer overflow"); + if (!isInteger_) + isInteger_ = std::move(other.isInteger_); } return *this; @@ -322,17 +299,17 @@ Number::exponent() const noexcept } inline void -Number::setIntegerEnforcement(EnforceInteger enforce) +Number::setIsInteger(bool isInteger) { - enforceInteger_ = enforce; - - checkInteger("Number::setIntegerEnforcement integer overflow"); + if (isInteger_) + return; + isInteger_ = isInteger; } -inline Number::EnforceInteger -Number::integerEnforcement() const noexcept +inline bool +Number::isInteger() const noexcept { - return enforceInteger_; + return isInteger_; } inline constexpr Number diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index 6765f7cf7d9..0a7f41880a9 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -87,7 +87,14 @@ class Asset bool integral() 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 true; + }, + issue_); } friend constexpr bool diff --git a/include/xrpl/protocol/MPTAmount.h b/include/xrpl/protocol/MPTAmount.h index 5f552d839b0..3ca1718cdcd 100644 --- a/include/xrpl/protocol/MPTAmount.h +++ b/include/xrpl/protocol/MPTAmount.h @@ -64,13 +64,7 @@ class MPTAmount : private boost::totally_ordered, operator Number() const { - return {value(), Number::strong}; - } - - Number - toNumber(Number::EnforceInteger enforce) const - { - return {value(), enforce}; + return {value(), true}; } /** Return the sign of the amount */ diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index d0092fdbec2..85e1fd4a992 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -40,12 +40,6 @@ class STAmount final : public STBase, public CountedObject exponent_type mOffset; bool mIsNegative; - // The Enforce integer setting is not stored or serialized. If set, it is - // used during automatic conversions to Number. If not set, the default - // behavior is used. It can also be overridden when coverting by using - // toNumber(). - std::optional enforceConversion_; - public: using value_type = STAmount; @@ -141,28 +135,9 @@ class STAmount final : public STBase, public CountedObject STAmount(A const& asset, int mantissa, int exponent = 0); template - STAmount( - A const& asset, - Number const& number, - std::optional enforce = std::nullopt) + STAmount(A const& asset, Number const& number) : STAmount(asset, number.mantissa(), number.exponent()) { - enforceConversion_ = enforce; - if (!enforce) - { - // Use the default conversion behavior - [[maybe_unused]] - Number const n = *this; - } - else if (enforce == Number::strong) - { - // Throw if it's not valid - if (!validNumber()) - { - Throw( - "STAmount::STAmount integer Number lost precision"); - } - } } // Legacy support for new-style amounts @@ -170,17 +145,11 @@ class STAmount final : public STBase, public CountedObject STAmount(XRPAmount const& amount); STAmount(MPTAmount const& amount, MPTIssue const& mptIssue); operator Number() const; - Number - toNumber(Number::EnforceInteger enforce) const; - - void - setIntegerEnforcement(std::optional enforce); - - std::optional - integerEnforcement() const noexcept; bool validNumber() const noexcept; + bool + representableNumber() const noexcept; //-------------------------------------------------------------------------- // @@ -555,8 +524,6 @@ inline STAmount::operator bool() const noexcept inline STAmount::operator Number() const { - if (enforceConversion_) - return toNumber(*enforceConversion_); if (native()) return xrp(); if (mAsset.holds()) @@ -564,17 +531,6 @@ inline STAmount::operator Number() const return iou(); } -inline Number -STAmount::toNumber(Number::EnforceInteger enforce) const -{ - if (native()) - return xrp().toNumber(enforce); - if (mAsset.holds()) - return mpt().toNumber(enforce); - // It doesn't make sense to enforce limits on IOUs - return iou(); -} - inline STAmount& STAmount::operator=(beast::Zero) { @@ -597,10 +553,6 @@ STAmount::operator=(Number const& number) mOffset = number.exponent(); canonicalize(); - // Convert it back to a Number to check that it's valid - [[maybe_unused]] - Number n = *this; - return *this; } diff --git a/include/xrpl/protocol/STNumber.h b/include/xrpl/protocol/STNumber.h index 43b96a2b465..2d9be9d91ed 100644 --- a/include/xrpl/protocol/STNumber.h +++ b/include/xrpl/protocol/STNumber.h @@ -58,11 +58,11 @@ class STNumber : public STBase, public CountedObject /// Sets the flag on the underlying number void - setIntegerEnforcement(Number::EnforceInteger enforce); + setIsInteger(bool isInteger); /// Gets the flag value on the underlying number - Number::EnforceInteger - integerEnforcement() const noexcept; + bool + isInteger() const noexcept; /// Checks the underlying number bool diff --git a/include/xrpl/protocol/XRPAmount.h b/include/xrpl/protocol/XRPAmount.h index 8f553a6083a..448f87cb24b 100644 --- a/include/xrpl/protocol/XRPAmount.h +++ b/include/xrpl/protocol/XRPAmount.h @@ -143,13 +143,7 @@ class XRPAmount : private boost::totally_ordered, operator Number() const noexcept { - return {drops(), Number::compatible}; - } - - Number - toNumber(Number::EnforceInteger enforce) const - { - return {value(), enforce}; + return {drops(), true}; } /** Return the sign of the amount */ diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 489e27c79fe..89d89ef601e 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -155,15 +155,6 @@ Number::Guard::round() noexcept constexpr Number one{1000000000000000, -15, Number::unchecked{}}; -void -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 Number::normalize() { @@ -219,51 +210,48 @@ Number::normalize() bool Number::valid() const noexcept { - return valid(enforceInteger_); + return valid(isInteger_); } bool -Number::valid(EnforceInteger enforce) +Number::valid(bool isInteger) { - setIntegerEnforcement(enforce); + setIsInteger(isInteger); return valid(); } bool -Number::valid(EnforceInteger enforce) const +Number::valid(bool isInteger) const { - if (enforce != none) - { - static Number const max = maxIntValue; - static Number const maxNeg = -maxIntValue; - // Avoid making a copy - if (mantissa_ < 0) - return *this >= maxNeg; - return *this <= max; - } - return true; + if (!isInteger) + 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 (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; + if (!isInteger_) + 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 - enforceInteger_ = std::max(enforceInteger_, y.enforceInteger_); + if (!isInteger_) + isInteger_ = y.isInteger_; if (y == Number{}) return *this; @@ -377,9 +365,6 @@ Number::operator+=(Number const& y) } mantissa_ = xm * xn; exponent_ = xe; - - checkInteger("Number::addition integer overflow"); - return *this; } @@ -415,7 +400,8 @@ Number& Number::operator*=(Number const& y) { // The strictest setting prevails - enforceInteger_ = std::max(enforceInteger_, y.enforceInteger_); + if (!isInteger_) + isInteger_ = y.isInteger_; if (*this == Number{}) return *this; @@ -483,9 +469,6 @@ Number::operator*=(Number const& y) XRPL_ASSERT( isnormal() || *this == Number{}, "ripple::Number::operator*=(Number) : result is normal"); - - checkInteger("Number::multiplication integer overflow"); - return *this; } @@ -493,7 +476,8 @@ Number& Number::operator/=(Number const& y) { // The strictest setting prevails - enforceInteger_ = std::max(enforceInteger_, y.enforceInteger_); + if (!isInteger_) + isInteger_ = y.isInteger_; if (y == Number{}) throw std::overflow_error("Number: divide by 0"); @@ -522,9 +506,6 @@ Number::operator/=(Number const& y) exponent_ = ne - de - 17; mantissa_ *= np * dp; normalize(); - - checkInteger("Number::division integer overflow"); - return *this; } diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 103ff37ef0b..069bd3a4d89 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2878,19 +2878,13 @@ 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{ shares.asset(), Number(assets.mantissa(), assets.exponent() + vault->at(sfScale)) - .truncate(), - Number::weak}; + .truncate()}; - Number const shareTotal{ - unsafe_cast(issuance->at(sfOutstandingAmount)), - Number::strong}; + Number const shareTotal = issuance->at(sfOutstandingAmount); shares = ((shareTotal * assets) / assetTotal).truncate(); return shares; } @@ -2912,9 +2906,6 @@ 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{ assets.asset(), @@ -2922,9 +2913,7 @@ sharesToAssetsDeposit( shares.exponent() - vault->at(sfScale), false}; - Number const shareTotal{ - unsafe_cast(issuance->at(sfOutstandingAmount)), - Number::strong}; + Number const shareTotal = issuance->at(sfOutstandingAmount); assets = (assetTotal * shares) / shareTotal; return assets; } @@ -2948,14 +2937,9 @@ 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; - Number const shareTotal{ - unsafe_cast(issuance->at(sfOutstandingAmount)), - Number::strong}; + Number const shareTotal = issuance->at(sfOutstandingAmount); Number result = (shareTotal * assets) / assetTotal; if (truncate == TruncateShares::yes) result = result.truncate(); @@ -2981,14 +2965,9 @@ 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; - Number const shareTotal{ - unsafe_cast(issuance->at(sfOutstandingAmount)), - Number::strong}; + Number const shareTotal = issuance->at(sfOutstandingAmount); assets = (assetTotal * shares) / shareTotal; return assets; } diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 73b539df698..e3af2477941 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -255,24 +255,18 @@ STAmount::move(std::size_t n, void* buf) return emplace(n, buf, std::move(*this)); } -void -STAmount::setIntegerEnforcement(std::optional enforce) -{ - enforceConversion_ = enforce; -} - -std::optional -STAmount::integerEnforcement() const noexcept +bool +STAmount::validNumber() const noexcept { - return enforceConversion_; + Number n = *this; + return n.valid(); } bool -STAmount::validNumber() const noexcept +STAmount::representableNumber() const noexcept { - // compatible will not throw. - Number n = toNumber(Number::EnforceInteger::compatible); - return n.valid(); + Number n = *this; + return n.representable(); } //------------------------------------------------------------------------------ diff --git a/src/libxrpl/protocol/STNumber.cpp b/src/libxrpl/protocol/STNumber.cpp index bab99c161dc..c9aebaee582 100644 --- a/src/libxrpl/protocol/STNumber.cpp +++ b/src/libxrpl/protocol/STNumber.cpp @@ -95,15 +95,15 @@ STNumber::isDefault() const } void -STNumber::setIntegerEnforcement(Number::EnforceInteger enforce) +STNumber::setIsInteger(bool isInteger) { - value_.setIntegerEnforcement(enforce); + value_.setIsInteger(isInteger); } -Number::EnforceInteger -STNumber::integerEnforcement() const noexcept +bool +STNumber::isInteger() const noexcept { - return value_.integerEnforcement(); + return value_.isInteger(); } bool diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 1efafec65fd..bc0bbeab604 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -735,230 +735,71 @@ class Number_test : public beast::unit_test::suite { Number a{100}; - BEAST_EXPECT(a.integerEnforcement() == Number::none); + BEAST_EXPECT(!a.isInteger()); 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.isInteger()); + BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.representable()); + // 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.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); } { - Number a{100, Number::compatible}; - BEAST_EXPECT(a.integerEnforcement() == Number::compatible); + Number a{100, true}; + BEAST_EXPECT(a.isInteger()); 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}; + // The false in the assigned value does not override the + // flag in "a" + a = Number{1, 30, false}; + BEAST_EXPECT(a.isInteger()); 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.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); - a = Number{1, 15}; + a *= Number{1, 16}; + BEAST_EXPECT(a.isInteger()); 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); + // Intermittent value rounding can be lost, but the result + // will be rounded, so that's fine. + a /= Number{1, 5}; + BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); - a = Number{5, Number::strong}; - BEAST_EXPECT(a.integerEnforcement() == Number::strong); + a = Number{1, 13} - 3; + BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); - } - { - Number a{100, Number::strong}; - BEAST_EXPECT(a.integerEnforcement() == Number::strong); + a += 1; + BEAST_EXPECT(a.isInteger()); 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}; - 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.valid()); - BEAST_EXPECT(!a.representable()); - a = -100; - BEAST_EXPECT(a.integerEnforcement() == Number::strong); + ++a; + BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); - } - { - Number a{INITIAL_XRP.drops(), Number::compatible}; - BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(!a.representable()); - a = -a; + a++; + BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(!a.representable()); - - try - { - a.setIntegerEnforcement(Number::strong); - BEAST_EXPECT(false); - } - catch (std::overflow_error const& e) - { - BEAST_EXPECT( - e.what() == - "Number::setIntegerEnforcement integer overflow"s); - // The throw is internal to the operator before the result is - // assigned to the Number - BEAST_EXPECT(a == -INITIAL_XRP); - BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(!a.representable()); - } - try - { - ++a; - BEAST_EXPECT(false); - } - catch (std::overflow_error const& e) - { - BEAST_EXPECT(e.what() == "Number::addition integer overflow"s); - // The throw is internal to the operator before the result is - // assigned to the Number - BEAST_EXPECT(a == -INITIAL_XRP); - BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(!a.representable()); - } - a = Number::maxIntValue; - try - { - ++a; - BEAST_EXPECT(false); - } - catch (std::overflow_error const& e) - { - BEAST_EXPECT(e.what() == "Number::addition integer overflow"s); - // 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 - { - --a; - BEAST_EXPECT(false); - } - catch (std::overflow_error const& e) - { - BEAST_EXPECT(e.what() == "Number::addition integer overflow"s); - // 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 - { - a *= Number(1, 10); - BEAST_EXPECT(false); - } - catch (std::overflow_error const& e) - { - BEAST_EXPECT( - e.what() == "Number::multiplication integer overflow"s); - // The throw is done _after_ the number is updated. - BEAST_EXPECT((a == Number{1, 20})); - BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(!a.representable()); - } - try - { - a = Number::maxIntValue * 2; - 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::maxIntValue * 2)); - BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(a.representable()); - } - try - { - a = Number(3, 15, Number::strong); - BEAST_EXPECT(false); - } - catch (std::overflow_error const& e) - { - BEAST_EXPECT(e.what() == "Number::Number integer overflow"s); - // 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 - { - a /= Number(1, -10); - BEAST_EXPECT(false); - } - catch (std::overflow_error const& e) - { - BEAST_EXPECT(e.what() == "Number::division integer overflow"s); - // 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.representable()); + a = Number{5, true}; + BEAST_EXPECT(a.isInteger()); 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 0dbbd4f24a5..862064940a4 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -2182,10 +2182,10 @@ ValidVault::Vault::make(SLE const& from) 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); + self.assetsTotal.setIsInteger(true); + self.assetsAvailable.setIsInteger(true); + self.assetsMaximum.setIsInteger(true); + self.lossUnrealized.setIsInteger(true); } return self; } diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index b464c1eb28a..3dafa766298 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -163,11 +163,7 @@ VaultClawback::doApply() AccountID holder = tx[sfHolder]; 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); + STAmount assetsRecovered = {vaultAsset}; try { if (amount == beast::zero) @@ -180,9 +176,6 @@ VaultClawback::doApply() AuthHandling::ahIGNORE_AUTH, j_); - if (!sharesDestroyed.validNumber()) - return tecPRECISION_LOSS; - auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, sharesDestroyed); if (!maybeAssets) @@ -198,8 +191,6 @@ VaultClawback::doApply() if (!maybeShares) return tecINTERNAL; // LCOV_EXCL_LINE sharesDestroyed = *maybeShares; - if (!sharesDestroyed.validNumber()) - return tecPRECISION_LOSS; } auto const maybeAssets = @@ -208,7 +199,8 @@ VaultClawback::doApply() return tecINTERNAL; // LCOV_EXCL_LINE assetsRecovered = *maybeAssets; } - if (!assetsRecovered.validNumber()) + if (!sharesDestroyed.representableNumber() || + !assetsRecovered.representableNumber()) return tecPRECISION_LOSS; // Clamp to maximum. diff --git a/src/xrpld/app/tx/detail/VaultCreate.cpp b/src/xrpld/app/tx/detail/VaultCreate.cpp index 400c36f1b00..c177eb98985 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().setIsInteger(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().setIsInteger(true); + } + */ + vault->at(sfShareMPTID) = mptIssuanceID; if (auto value = tx[~sfData]) vault->at(sfData) = *value; @@ -204,13 +226,6 @@ 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 53b7fd67c05..e1d8e34dbdf 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -175,6 +175,7 @@ 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)) { @@ -219,10 +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); + STAmount sharesCreated = {vault->at(sfShareMPTID)}; + STAmount assetsDeposited = {vaultAsset}; try { // Compute exchange before transferring any amounts. @@ -233,14 +232,14 @@ VaultDeposit::doApply() return tecINTERNAL; // LCOV_EXCL_LINE sharesCreated = *maybeShares; } - if (sharesCreated == beast::zero || !sharesCreated.validNumber()) + if (sharesCreated == beast::zero) return tecPRECISION_LOSS; auto const maybeAssets = sharesToAssetsDeposit(vault, sleIssuance, sharesCreated); if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE - else if (*maybeAssets > amount || !maybeAssets->validNumber()) + else if (*maybeAssets > amount) { // LCOV_EXCL_START JLOG(j_.error()) << "VaultDeposit: would take more than offered."; @@ -248,6 +247,10 @@ VaultDeposit::doApply() // LCOV_EXCL_STOP } assetsDeposited = *maybeAssets; + + if (!sharesCreated.representableNumber() || + !assetsDeposited.representableNumber()) + return tecPRECISION_LOSS; } catch (std::overflow_error const&) { @@ -268,10 +271,10 @@ VaultDeposit::doApply() auto assetsTotalProxy = vault->at(sfAssetsTotal); auto assetsAvailableProxy = vault->at(sfAssetsAvailable); - if (vault->at(sfAsset).value().integral()) + if (vaultAsset.value().integral()) { - assetsTotalProxy.value().setIntegerEnforcement(Number::weak); - assetsAvailableProxy.value().setIntegerEnforcement(Number::weak); + assetsTotalProxy.value().setIsInteger(true); + assetsAvailableProxy.value().setIsInteger(true); } assetsTotalProxy += assetsDeposited; assetsAvailableProxy += assetsDeposited; diff --git a/src/xrpld/app/tx/detail/VaultSet.cpp b/src/xrpld/app/tx/detail/VaultSet.cpp index 75f1689d7f0..6823d32a151 100644 --- a/src/xrpld/app/tx/detail/VaultSet.cpp +++ b/src/xrpld/app/tx/detail/VaultSet.cpp @@ -138,16 +138,17 @@ 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()) { - if (!vault->at(sfAssetsMaximum).value().valid(Number::compatible)) - return tecLIMIT_EXCEEDED; + assetsMaximumProxy.value().setIsInteger(true); + if (!assetsMaximumProxy.value().representable()) + return tecPRECISION_LOSS; } } diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index e28c54676ce..2423d7128af 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -156,9 +156,7 @@ VaultWithdraw::doApply() Asset const vaultAsset = vault->at(sfAsset); MPTIssue const share{mptIssuanceID}; STAmount sharesRedeemed = {share}; - STAmount assetsWithdrawn; - assetsWithdrawn.setIntegerEnforcement(Number::weak); - sharesRedeemed.setIntegerEnforcement(Number::weak); + STAmount assetsWithdrawn = {vaultAsset}; try { if (amount.asset() == vaultAsset) @@ -172,15 +170,13 @@ VaultWithdraw::doApply() sharesRedeemed = *maybeShares; } - if (sharesRedeemed == beast::zero || !sharesRedeemed.validNumber()) + if (sharesRedeemed == beast::zero) return tecPRECISION_LOSS; auto const maybeAssets = sharesToAssetsWithdraw(vault, sleIssuance, sharesRedeemed); if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE assetsWithdrawn = *maybeAssets; - if (!assetsWithdrawn.validNumber()) - return tecPRECISION_LOSS; } else if (amount.asset() == share) { @@ -191,11 +187,12 @@ VaultWithdraw::doApply() if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE assetsWithdrawn = *maybeAssets; - if (!assetsWithdrawn.validNumber()) - return tecPRECISION_LOSS; } else return tefINTERNAL; // LCOV_EXCL_LINE + if (!sharesRedeemed.representableNumber() || + !assetsWithdrawn.representableNumber()) + return tecPRECISION_LOSS; } catch (std::overflow_error const&) { From 596365d05d9cda37f525f3b1514c02def6f0101e Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 17 Nov 2025 00:32:21 -0500 Subject: [PATCH 10/23] fixup! Rip out about half the code: levels, enforcement, and STAmount changes --- include/xrpl/basics/Number.h | 1 - include/xrpl/protocol/Asset.h | 9 ++- include/xrpl/protocol/MPTAmount.h | 2 +- include/xrpl/protocol/STAmount.h | 3 +- include/xrpl/protocol/STNumber.h | 12 ---- src/libxrpl/protocol/STNumber.cpp | 18 ------ src/test/app/Vault_test.cpp | 42 +++++++++++--- src/test/basics/Number_test.cpp | 11 ++-- src/xrpld/app/tx/detail/VaultClawback.cpp | 14 ++--- src/xrpld/app/tx/detail/VaultDeposit.cpp | 68 ++++++++++++++++++++--- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 11 ++-- 11 files changed, 123 insertions(+), 68 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 6632a456ab4..314754371a1 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -24,7 +24,6 @@ isPowerOfTen(T value) class Number { -private: using rep = std::int64_t; rep mantissa_{0}; int exponent_{std::numeric_limits::lowest()}; diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index 0a7f41880a9..7ad1e702568 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -81,7 +81,14 @@ 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 diff --git a/include/xrpl/protocol/MPTAmount.h b/include/xrpl/protocol/MPTAmount.h index 3ca1718cdcd..46fa4e98b96 100644 --- a/include/xrpl/protocol/MPTAmount.h +++ b/include/xrpl/protocol/MPTAmount.h @@ -62,7 +62,7 @@ class MPTAmount : private boost::totally_ordered, explicit constexpr operator bool() const noexcept; - operator Number() const + operator Number() const noexcept { return {value(), true}; } diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index 85e1fd4a992..e8eded314f3 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -552,7 +552,6 @@ STAmount::operator=(Number const& number) mValue = mIsNegative ? -number.mantissa() : number.mantissa(); mOffset = number.exponent(); canonicalize(); - return *this; } @@ -568,7 +567,7 @@ STAmount::clear() { // The -100 is used to allow 0 to sort less than a small positive values // which have a negative exponent. - mOffset = integral() ? 0 : -100; + mOffset = native() ? 0 : -100; mValue = 0; mIsNegative = false; } diff --git a/include/xrpl/protocol/STNumber.h b/include/xrpl/protocol/STNumber.h index 2d9be9d91ed..2ec3d66fd14 100644 --- a/include/xrpl/protocol/STNumber.h +++ b/include/xrpl/protocol/STNumber.h @@ -56,18 +56,6 @@ class STNumber : public STBase, public CountedObject bool isDefault() const override; - /// Sets the flag on the underlying number - void - setIsInteger(bool isInteger); - - /// Gets the flag value on the underlying number - bool - isInteger() const noexcept; - - /// Checks the underlying number - bool - valid() const noexcept; - operator Number() const { return value_; diff --git a/src/libxrpl/protocol/STNumber.cpp b/src/libxrpl/protocol/STNumber.cpp index c9aebaee582..c353f6b7956 100644 --- a/src/libxrpl/protocol/STNumber.cpp +++ b/src/libxrpl/protocol/STNumber.cpp @@ -94,24 +94,6 @@ STNumber::isDefault() const return value_ == Number(); } -void -STNumber::setIsInteger(bool isInteger) -{ - value_.setIsInteger(isInteger); -} - -bool -STNumber::isInteger() const noexcept -{ - return value_.isInteger(); -} - -bool -STNumber::valid() const noexcept -{ - return value_.valid(); -} - std::ostream& operator<<(std::ostream& out, STNumber const& rhs) { diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 37ceffe5b25..3cb8991a339 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3684,7 +3684,7 @@ class Vault_test : public beast::unit_test::suite }); testCase(18, [&, this](Env& env, Data d) { - testcase("MPT scale deposit overflow"); + testcase("MPT scale deposit over maxIntValue"); // The computed number of shares can not be represented as an MPT // without truncation @@ -3699,7 +3699,7 @@ class Vault_test : public beast::unit_test::suite }); testCase(13, [&, this](Env& env, Data d) { - testcase("MPT scale deposit overflow on first deposit"); + testcase("MPT scale deposit over maxIntValue on first deposit"); auto tx = d.vault.deposit( {.depositor = d.depositor, .id = d.keylet.key, @@ -3709,7 +3709,7 @@ class Vault_test : public beast::unit_test::suite }); testCase(13, [&, this](Env& env, Data d) { - testcase("MPT scale deposit overflow on second deposit"); + testcase("MPT scale deposit over maxIntValue on second deposit"); { auto tx = d.vault.deposit( @@ -3725,13 +3725,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(13, [&, this](Env& env, Data d) { - testcase("No MPT scale deposit overflow on total shares"); + testcase("MPT scale deposit over maxIntValue on total shares"); { auto tx = d.vault.deposit( @@ -3747,7 +3747,7 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(5)}); - env(tx); + env(tx, ter(tecPRECISION_LOSS)); env.close(); } }); @@ -4051,7 +4051,7 @@ class Vault_test : public beast::unit_test::suite }); testCase(13, [&, this](Env& env, Data d) { - testcase("MPT scale withdraw overflow"); + testcase("MPT scale withdraw over maxIntValue"); { auto tx = d.vault.deposit( @@ -4063,11 +4063,22 @@ 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{tecPATH_DRY}); + 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{tecPRECISION_LOSS}); env.close(); } }); @@ -4304,14 +4315,27 @@ 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(); + } + }); testCase(1, [&, this](Env& env, Data d) { diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index bc0bbeab604..539ac8bf0e3 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -2,7 +2,6 @@ #include #include #include -#include #include #include @@ -772,17 +771,21 @@ class Number_test : public beast::unit_test::suite BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); - a *= Number{1, 16}; + a *= Number{1, 13}; BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(!a.valid()); BEAST_EXPECT(a.representable()); - // Intermittent value rounding can be lost, but the result + a *= Number{1, 3}; + BEAST_EXPECT(a.isInteger()); + BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.representable()); + // Intermittent value precision can be lost, but the result // will be rounded, so that's fine. a /= Number{1, 5}; BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); - a = Number{1, 13} - 3; + a = Number{1, 14} - 3; BEAST_EXPECT(a.isInteger()); BEAST_EXPECT(a.valid()); BEAST_EXPECT(a.representable()); diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index 3dafa766298..4282877d1f0 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; @@ -71,13 +73,8 @@ VaultClawback::preclaim(PreclaimContext const& ctx) } Asset const vaultAsset = vault->at(sfAsset); - if (auto const amount = ctx.tx[~sfAmount]) - { - if (vaultAsset != amount->asset()) - return tecWRONG_ASSET; - else if (!amount->validNumber()) - return tecPRECISION_LOSS; - } + if (auto const amount = ctx.tx[~sfAmount]; vaultAsset != amount->asset()) + return tecWRONG_ASSET; if (vaultAsset.native()) { @@ -199,6 +196,9 @@ 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; diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index e1d8e34dbdf..878e018e950 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.validNumber()) return temBAD_AMOUNT; return tesSUCCESS; @@ -42,9 +45,6 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) if (assets.asset() != vaultAsset) return tecWRONG_ASSET; - if (!assets.validNumber()) - return tecPRECISION_LOSS; - if (vaultAsset.native()) ; // No special checks for XRP else if (vaultAsset.holds()) @@ -165,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 @@ -176,6 +177,7 @@ 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)) { @@ -248,8 +250,10 @@ VaultDeposit::doApply() } assetsDeposited = *maybeAssets; - if (!sharesCreated.representableNumber() || - !assetsDeposited.representableNumber()) + // 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.validNumber() || !assetsDeposited.validNumber()) return tecPRECISION_LOSS; } catch (std::overflow_error const&) @@ -257,7 +261,7 @@ VaultDeposit::doApply() // 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) @@ -280,7 +284,19 @@ VaultDeposit::doApply() assetsAvailableProxy += assetsDeposited; if (!assetsTotalProxy.value().valid() || !assetsAvailableProxy.value().valid()) - return tecLIMIT_EXCEEDED; + { + // 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_.warn()) // + << "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. @@ -288,6 +304,9 @@ VaultDeposit::doApply() 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(), @@ -325,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.validNumber()) + { + 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/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index 2423d7128af..e99a1af20d2 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]; @@ -50,9 +53,6 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) if (assets.asset() != vaultAsset && assets.asset() != vaultShare) return tecWRONG_ASSET; - if (!assets.validNumber()) - return tecPRECISION_LOSS; - if (vaultAsset.native()) ; // No special checks for XRP else if (vaultAsset.holds()) @@ -190,6 +190,9 @@ 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; From 7974545da4d258733e527d0918a77eec16c2d3c1 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 17 Nov 2025 14:31:03 -0500 Subject: [PATCH 11/23] Fixes build errors and test failures - STAmount::clear() needs to use 0 exponent for all integral types. - Change vaultMaximumIOUScale to 13. - Fix an dereferenced unseated optional. --- include/xrpl/protocol/Protocol.h | 6 ++++-- include/xrpl/protocol/STAmount.h | 2 +- src/test/app/Vault_test.cpp | 15 ++++++++++++--- src/xrpld/app/tx/detail/VaultClawback.cpp | 3 ++- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 20b73e90c97..7c632340432 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^14 > Number::maxIntValue +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 = 13; /** 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 e8eded314f3..af0ec1e679f 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -567,7 +567,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/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 3cb8991a339..58d2a0c58e4 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 13 + { + auto [tx, keylet] = + vault.create({.owner = owner, .asset = asset}); + tx[sfScale] = 13; env(tx); env.close(); auto const sleVault = env.le(keylet); - BEAST_EXPECT(sleVault); - BEAST_EXPECT((*sleVault)[sfScale] == 18); + if (BEAST_EXPECT(sleVault)) + ; + BEAST_EXPECT((*sleVault)[sfScale] == 13); } { diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index 4282877d1f0..7dbe5f4c971 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -73,7 +73,8 @@ VaultClawback::preclaim(PreclaimContext const& ctx) } Asset const vaultAsset = vault->at(sfAsset); - if (auto const amount = ctx.tx[~sfAmount]; vaultAsset != amount->asset()) + if (auto const amount = ctx.tx[~sfAmount]; + amount && vaultAsset != amount->asset()) return tecWRONG_ASSET; if (vaultAsset.native()) From bba4b444a69fdd0bf774a9a92e65809263855bdd Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 17 Nov 2025 18:18:52 -0500 Subject: [PATCH 12/23] fix: Number: Do not attempt to take the negative of an unsigned --- src/libxrpl/basics/Number.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 89d89ef601e..59eeb60b907 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -164,9 +164,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; From 2338c55dc8c9dd17de1cf6b04afc69df49c4e308 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 17 Nov 2025 18:20:12 -0500 Subject: [PATCH 13/23] test fix: Remove cases with Vault scale 18 - The limit has been reduced to 13, so 18 is now considered malformed. - Make the test more robust so it doesn't segfault if the vault creation fails. --- src/test/app/Vault_test.cpp | 83 ++++--------------------------------- 1 file changed, 9 insertions(+), 74 deletions(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 58d2a0c58e4..9801bce2e43 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3634,13 +3634,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); @@ -3682,31 +3687,6 @@ class Vault_test : public beast::unit_test::suite .peek = peek}); }; - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow 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.close(); - }); - - testCase(18, [&, this](Env& env, Data d) { - testcase("MPT scale deposit over maxIntValue"); - // The computed number of shares can not be represented as an MPT - // without truncation - - { - auto tx = d.vault.deposit( - {.depositor = d.depositor, - .id = d.keylet.key, - .amount = d.asset(5)}); - env(tx, ter{tecPRECISION_LOSS}); - env.close(); - } - }); - testCase(13, [&, this](Env& env, Data d) { testcase("MPT scale deposit over maxIntValue on first deposit"); auto tx = d.vault.deposit( @@ -4037,28 +4017,6 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale withdraw overflow"); - - { - 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.withdraw( - {.depositor = d.depositor, - .id = d.keylet.key, - .amount = STAmount(d.asset, Number(10, 0))}); - env(tx, ter{tecPRECISION_LOSS}); - env.close(); - } - }); - testCase(13, [&, this](Env& env, Data d) { testcase("MPT scale withdraw over maxIntValue"); @@ -4288,29 +4246,6 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale clawback overflow"); - - { - 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.clawback( - {.issuer = d.issuer, - .id = d.keylet.key, - .holder = d.depositor, - .amount = STAmount(d.asset, Number(10, 0))}); - env(tx, ter{tecPRECISION_LOSS}); - env.close(); - } - }); - testCase(13, [&, this](Env& env, Data d) { testcase("MPT Scale clawback overflow"); From 67796cfa90b83a5b5ea1ad6b3b8c9774d0949a53 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 17 Nov 2025 20:25:11 -0500 Subject: [PATCH 14/23] Change the vaultMaximumIOUScale from 13 to 15 - Anything above 13 is _nearly_ unusable, but there might be some edge use cases where it is. - Added unit tests to show this. --- include/xrpl/protocol/Protocol.h | 4 +- src/libxrpl/ledger/View.cpp | 2 +- src/test/app/Vault_test.cpp | 116 +++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 3 deletions(-) diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 7c632340432..e12006a7941 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -109,10 +109,10 @@ std::uint8_t constexpr vaultStrategyFirstComeFirstServe = 1; 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 and will fit into Number. -10^14 > Number::maxIntValue +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 = 13; +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/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 069bd3a4d89..2b109691e22 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/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 9801bce2e43..fd2aa100ed3 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3687,6 +3687,80 @@ class Vault_test : public beast::unit_test::suite .peek = peek}); }; + // 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 large than Number::maxIntValue are invalid + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(10)}); + env(tx, ter{tecPRECISION_LOSS}); + 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{tecPRECISION_LOSS}); + 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( @@ -4246,6 +4320,48 @@ class Vault_test : public beast::unit_test::suite } }); + // 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{tecPRECISION_LOSS}); + 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"); From 2e02c338af82f607b79bbff06dd0ae2f5c4f21bc Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 17 Nov 2025 20:27:32 -0500 Subject: [PATCH 15/23] Expand the description of Number::isInteger_ --- include/xrpl/basics/Number.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 314754371a1..20f97d005aa 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -28,10 +28,11 @@ class Number rep mantissa_{0}; int exponent_{std::numeric_limits::lowest()}; - // isInteger_ is not serialized, transmitted, or used in - // calculations in any way. It is used only for internal validation - // of integer types. It is a one-way switch. Once it's on, it stays - // on. + // isInteger_ is informational only. It is not serialized, transmitted, or + // used in calculations in any way. It is used only for internal validation + // of integer types, usually in transactions. It is a one-way switch. Once + // it's on, it stays on. It is also transmissible in that any operation + // involving a Number with this flag will have a result with this flag. bool isInteger_ = false; public: From 1e47c76fc4d99598d4b84aea35ab475f8ef9ae2b Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 18 Nov 2025 23:07:22 -0500 Subject: [PATCH 16/23] Rewrite to clarify Number limitations, enforce limits in conversion - Overflow Number -> int64 if it doesn't fit in the mantissa range. - Only enabled if at least one of the SingleAssetVault or LendingProtocol amendments are enabled. - Will throw the overflow error if the value is larger than maxMantissa. Current behavior is to throw if the value is larger than max int64_t value. --- include/xrpl/basics/Number.h | 112 ++++++++++++++++----- include/xrpl/protocol/STAmount.h | 15 ++- src/libxrpl/basics/Number.cpp | 44 +++++--- src/libxrpl/protocol/Rules.cpp | 8 ++ src/libxrpl/protocol/STAmount.cpp | 10 +- src/test/basics/Number_test.cpp | 60 +++++------ src/xrpld/app/tx/detail/InvariantCheck.cpp | 8 +- src/xrpld/app/tx/detail/VaultCreate.cpp | 4 +- src/xrpld/app/tx/detail/VaultDeposit.cpp | 14 +-- src/xrpld/app/tx/detail/VaultSet.cpp | 2 +- 10 files changed, 189 insertions(+), 88 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 20f97d005aa..51f134a99b1 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -28,12 +28,32 @@ class Number rep mantissa_{0}; int exponent_{std::numeric_limits::lowest()}; - // isInteger_ is informational only. It is not serialized, transmitted, or - // used in calculations in any way. It is used only for internal validation - // of integer types, usually in transactions. It is a one-way switch. Once - // it's on, it stays on. It is also transmissible in that any operation - // involving a Number with this flag will have a result with this flag. - bool isInteger_ = false; + // 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 Number is not representable(), 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 @@ -56,8 +76,8 @@ class Number explicit constexpr Number() = default; - Number(rep mantissa, bool isInteger = false); - explicit Number(rep mantissa, int exponent, bool isInteger = false); + 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; @@ -74,23 +94,34 @@ class Number 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 - setIsInteger(bool isInteger); + setLimited(bool limited); + // Gets the current value of the limited_ flag. See the description of + // limited for how it works. bool - isInteger() const noexcept; + getLimited() const noexcept; + // 1. "fits()" indicates whether the Number fits into the safe range of + // -maxIntValue to maxIntValue. bool - valid() const noexcept; + 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 setIsInteger(bool) and valid() + /// Combines setLimited(bool) and fits() bool - valid(bool isInteger); + fits(bool limited); /// Because this function is const, it should only be used for one-off /// checks bool - valid(bool isInteger) const; + fits(bool limited) const; constexpr Number operator+() const noexcept; @@ -126,6 +157,12 @@ 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 Number is not representable(), 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 @@ -229,9 +266,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 @@ -245,14 +304,13 @@ inline constexpr Number::Number(rep mantissa, int exponent, unchecked) noexcept { } -inline Number::Number(rep mantissa, int exponent, bool isInteger) - : mantissa_{mantissa}, exponent_{exponent}, isInteger_(isInteger) +inline Number::Number(rep mantissa, int exponent, bool limited) + : mantissa_{mantissa}, exponent_{exponent}, limited_(limited) { normalize(); } -inline Number::Number(rep mantissa, bool isInteger) - : Number{mantissa, 0, isInteger} +inline Number::Number(rep mantissa, bool limited) : Number{mantissa, 0, limited} { } @@ -263,8 +321,8 @@ Number::operator=(Number const& other) { mantissa_ = other.mantissa_; exponent_ = other.exponent_; - if (!isInteger_) - isInteger_ = other.isInteger_; + if (!limited_) + limited_ = other.limited_; } return *this; @@ -279,8 +337,8 @@ Number::operator=(Number&& other) // this is future-proof in case the types ever change mantissa_ = std::move(other.mantissa_); exponent_ = std::move(other.exponent_); - if (!isInteger_) - isInteger_ = std::move(other.isInteger_); + if (!limited_) + limited_ = std::move(other.limited_); } return *this; @@ -299,17 +357,17 @@ Number::exponent() const noexcept } inline void -Number::setIsInteger(bool isInteger) +Number::setLimited(bool limited) { - if (isInteger_) + if (limited_) return; - isInteger_ = isInteger; + limited_ = limited; } inline bool -Number::isInteger() const noexcept +Number::getLimited() const noexcept { - return isInteger_; + return limited_; } inline constexpr Number diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index af0ec1e679f..254ef368223 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -146,8 +146,21 @@ 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 - validNumber() const noexcept; + 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; diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 59eeb60b907..3fcb6c1e899 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 @@ -207,22 +220,22 @@ Number::normalize() } bool -Number::valid() const noexcept +Number::fits() const noexcept { - return valid(isInteger_); + return fits(limited_); } bool -Number::valid(bool isInteger) +Number::fits(bool limited) { - setIsInteger(isInteger); - return valid(); + setLimited(limited); + return fits(); } bool -Number::valid(bool isInteger) const +Number::fits(bool limited) const { - if (!isInteger) + if (!limited) return true; static Number const max = maxIntValue; static Number const maxNeg = -max; @@ -235,7 +248,7 @@ Number::valid(bool isInteger) const bool Number::representable() const noexcept { - if (!isInteger_) + if (!limited_) return true; static Number const max = maxMantissa; static Number const maxNeg = -max; @@ -249,8 +262,8 @@ Number& Number::operator+=(Number const& y) { // The strictest setting prevails - if (!isInteger_) - isInteger_ = y.isInteger_; + if (!limited_) + limited_ = y.limited_; if (y == Number{}) return *this; @@ -399,8 +412,8 @@ Number& Number::operator*=(Number const& y) { // The strictest setting prevails - if (!isInteger_) - isInteger_ = y.isInteger_; + if (!limited_) + limited_ = y.limited_; if (*this == Number{}) return *this; @@ -475,8 +488,8 @@ Number& Number::operator/=(Number const& y) { // The strictest setting prevails - if (!isInteger_) - isInteger_ = y.isInteger_; + if (!limited_) + limited_ = y.limited_; if (y == Number{}) throw std::overflow_error("Number: divide by 0"); @@ -510,6 +523,9 @@ Number::operator/=(Number const& y) Number::operator rep() const { + if (Number::overflowLargeIntegers_ && !representable()) + throw std::overflow_error( + "Number::operator rep() overflow unrepresentable"); rep drops = mantissa_; int offset = exponent_; Guard g; 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 e3af2477941..6933f4e6d51 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -256,15 +256,21 @@ STAmount::move(std::size_t n, void* buf) } bool -STAmount::validNumber() const noexcept +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.valid(); + 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(); } diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 539ac8bf0e3..e81a96bac81 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -734,76 +734,76 @@ class Number_test : public beast::unit_test::suite { Number a{100}; - BEAST_EXPECT(!a.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(!a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); a = Number{1, 30}; - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); a = -100; - BEAST_EXPECT(!a.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(!a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); // 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.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); } { Number a{100, true}; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); a = Number{1, 15}; - BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(!a.fits()); BEAST_EXPECT(a.representable()); // The false in the assigned value does not override the // flag in "a" a = Number{1, 30, false}; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); BEAST_EXPECT(!a.representable()); a = -100; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); a *= Number{1, 13}; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); BEAST_EXPECT(a.representable()); a *= Number{1, 3}; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); BEAST_EXPECT(!a.representable()); // Intermittent value precision can be lost, but the result // will be rounded, so that's fine. a /= Number{1, 5}; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); a = Number{1, 14} - 3; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); a += 1; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); ++a; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); a++; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(!a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); BEAST_EXPECT(a.representable()); a = Number{5, true}; - BEAST_EXPECT(a.isInteger()); - BEAST_EXPECT(a.valid()); + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); } } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 862064940a4..8c6861bf4e1 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -2182,10 +2182,10 @@ ValidVault::Vault::make(SLE const& from) self.lossUnrealized = from.at(sfLossUnrealized); if (self.asset.integral()) { - self.assetsTotal.setIsInteger(true); - self.assetsAvailable.setIsInteger(true); - self.assetsMaximum.setIsInteger(true); - self.lossUnrealized.setIsInteger(true); + self.assetsTotal.setLimited(true); + self.assetsAvailable.setLimited(true); + self.assetsMaximum.setLimited(true); + self.lossUnrealized.setLimited(true); } return self; } diff --git a/src/xrpld/app/tx/detail/VaultCreate.cpp b/src/xrpld/app/tx/detail/VaultCreate.cpp index c177eb98985..5e8085cea3b 100644 --- a/src/xrpld/app/tx/detail/VaultCreate.cpp +++ b/src/xrpld/app/tx/detail/VaultCreate.cpp @@ -200,7 +200,7 @@ VaultCreate::doApply() { // Only the Maximum can be a non-zero value, so only it needs to be // checked. - assetsMaximumProxy.value().setIsInteger(true); + assetsMaximumProxy.value().setLimited(true); if (!assetsMaximumProxy.value().representable()) return tecPRECISION_LOSS; } @@ -212,7 +212,7 @@ VaultCreate::doApply() { auto assetsMaximumProxy = vault->at(~sfAssetsMaximum); assetsMaximumProxy = STNumber::maxIntValue - assetsMaximumProxy.value().setIsInteger(true); + assetsMaximumProxy.value().setLimited(true); } */ diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 878e018e950..ad9355d43e8 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -26,7 +26,7 @@ VaultDeposit::preflight(PreflightContext const& ctx) auto const amount = ctx.tx[sfAmount]; if (amount <= beast::zero) return temBAD_AMOUNT; - if (!amount.validNumber()) + if (!amount.numberFits()) return temBAD_AMOUNT; return tesSUCCESS; @@ -253,7 +253,7 @@ VaultDeposit::doApply() // 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.validNumber() || !assetsDeposited.validNumber()) + if (!sharesCreated.numberFits() || !assetsDeposited.numberFits()) return tecPRECISION_LOSS; } catch (std::overflow_error const&) @@ -277,13 +277,13 @@ VaultDeposit::doApply() auto assetsAvailableProxy = vault->at(sfAssetsAvailable); if (vaultAsset.value().integral()) { - assetsTotalProxy.value().setIsInteger(true); - assetsAvailableProxy.value().setIsInteger(true); + assetsTotalProxy.value().setLimited(true); + assetsAvailableProxy.value().setLimited(true); } assetsTotalProxy += assetsDeposited; assetsAvailableProxy += assetsDeposited; - if (!assetsTotalProxy.value().valid() || - !assetsAvailableProxy.value().valid()) + 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. @@ -362,7 +362,7 @@ VaultDeposit::doApply() // vice-versa STAmount const shareTotal{ vault->at(sfShareMPTID), sleIssuance->at(sfOutstandingAmount)}; - if (!shareTotal.validNumber()) + if (!shareTotal.numberFits()) { JLOG(j_.warn()) // << "VaultDeposit: integer overflow error in total shares with" diff --git a/src/xrpld/app/tx/detail/VaultSet.cpp b/src/xrpld/app/tx/detail/VaultSet.cpp index 6823d32a151..b832dc05a12 100644 --- a/src/xrpld/app/tx/detail/VaultSet.cpp +++ b/src/xrpld/app/tx/detail/VaultSet.cpp @@ -146,7 +146,7 @@ VaultSet::doApply() assetsMaximumProxy = *value; if (vault->at(sfAsset).value().integral()) { - assetsMaximumProxy.value().setIsInteger(true); + assetsMaximumProxy.value().setLimited(true); if (!assetsMaximumProxy.value().representable()) return tecPRECISION_LOSS; } From 82553c2828fc9217240760c99ab5dbf651c4cb28 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 18 Nov 2025 23:23:24 -0500 Subject: [PATCH 17/23] Avoid some null dereferences in tests --- src/test/app/Vault_test.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index fd2aa100ed3..7d6f6a26d40 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -978,8 +978,8 @@ class Vault_test : public beast::unit_test::suite env(tx); env.close(); auto const sleVault = env.le(keylet); - if (BEAST_EXPECT(sleVault)) - ; + if (!BEAST_EXPECT(sleVault)) + return; BEAST_EXPECT((*sleVault)[sfScale] == 13); } @@ -990,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); } @@ -1000,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); } }); @@ -4395,7 +4397,6 @@ class Vault_test : public beast::unit_test::suite env(tx, ter{tecPRECISION_LOSS}); env.close(); } - }); testCase(1, [&, this](Env& env, Data d) { From dc283cee0b77a907a84177f10d776190f0bf8715 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 18 Nov 2025 23:51:19 -0500 Subject: [PATCH 18/23] Add a couple of high value MPT tests --- src/test/app/MPToken_test.cpp | 9 ++++++++ src/test/basics/Number_test.cpp | 37 +++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index f7c93d3c621..5e7b51c3758 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,8 @@ class MPToken_test : public beast::unit_test::suite mptAlice.pay(alice, bob, maxMPTokenAmount); BEAST_EXPECT( mptAlice.checkMPTokenOutstandingAmount(maxMPTokenAmount)); + env.require(balance( + bob, STAmount{mptAlice.issuanceID(), maxMPTokenAmount})); // payment between the holders mptAlice.pay(bob, carol, maxMPTokenAmount); diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index e81a96bac81..3cb70060ced 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -732,14 +732,30 @@ class Number_test : public beast::unit_test::suite using namespace std::string_literals; + auto toInt = [this](Number const& a, std::string const& ex = {}) + -> std::optional { + try + { + return static_cast(a); + } + catch (std::overflow_error const& e) + { + BEAST_EXPECTS( + ex.empty() || e.what() == ex, + to_string(a) + ": " + e.what()); + return std::nullopt; + } + }; { Number a{100}; BEAST_EXPECT(!a.getLimited()); BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); + BEAST_EXPECT(toInt(a) == 100); a = Number{1, 30}; BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); + BEAST_EXPECT(!toInt(a, "Number::operator rep() overflow")); a = -100; BEAST_EXPECT(!a.getLimited()); BEAST_EXPECT(a.fits()); @@ -805,6 +821,27 @@ class Number_test : public beast::unit_test::suite BEAST_EXPECT(a.getLimited()); BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); + + a = 0x7FFF'FFFF'FFFF'FFFFll; + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(!a.representable()); + BEAST_EXPECT(!toInt(a, "Number::operator rep() overflow")); + Number::setEnforceIntegerOverflow(true); + BEAST_EXPECT( + !toInt(a, "Number::operator rep() overflow unrepresentable")); + Number::setEnforceIntegerOverflow(false); + + a = 0xFFF'FFFF'FFFF'FFFFll; + // 1152921504606846975 + BEAST_EXPECT(a.getLimited()); + BEAST_EXPECT(!a.fits()); + BEAST_EXPECT(!a.representable()); + BEAST_EXPECT(toInt(a) == 1152921504606847000ll); + Number::setEnforceIntegerOverflow(true); + BEAST_EXPECT( + !toInt(a, "Number::operator rep() overflow unrepresentable")); + Number::setEnforceIntegerOverflow(false); } } From 687b9cc3529039936bb6875a6526c7040e55da18 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 19 Nov 2025 15:12:46 -0500 Subject: [PATCH 19/23] Add an RAII NumberIntegerOverflowGuard class, and update tests --- include/xrpl/basics/Number.h | 26 ++++++++++++++++ src/test/basics/Number_test.cpp | 54 ++++++++++++++++++++++----------- 2 files changed, 63 insertions(+), 17 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 51f134a99b1..28f73b5f1da 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -555,6 +555,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/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 3cb70060ced..b08e2b0efc3 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -732,18 +732,19 @@ class Number_test : public beast::unit_test::suite using namespace std::string_literals; - auto toInt = [this](Number const& a, std::string const& ex = {}) - -> std::optional { + auto checkInt = [this]( + Number const& a, + std::int64_t expected, + std::string const& ex = {}) { try { - return static_cast(a); + BEAST_EXPECT(static_cast(a) == expected); } catch (std::overflow_error const& e) { BEAST_EXPECTS( ex.empty() || e.what() == ex, to_string(a) + ": " + e.what()); - return std::nullopt; } }; { @@ -751,11 +752,11 @@ class Number_test : public beast::unit_test::suite BEAST_EXPECT(!a.getLimited()); BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); - BEAST_EXPECT(toInt(a) == 100); + checkInt(a, 100); a = Number{1, 30}; BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); - BEAST_EXPECT(!toInt(a, "Number::operator rep() overflow")); + checkInt(a, 0, "Number::operator rep() overflow"); a = -100; BEAST_EXPECT(!a.getLimited()); BEAST_EXPECT(a.fits()); @@ -822,26 +823,45 @@ class Number_test : public beast::unit_test::suite BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); - a = 0x7FFF'FFFF'FFFF'FFFFll; + auto const maxUint = 0x7FFF'FFFF'FFFF'FFFFll; + a = maxUint; BEAST_EXPECT(a.getLimited()); BEAST_EXPECT(!a.fits()); BEAST_EXPECT(!a.representable()); - BEAST_EXPECT(!toInt(a, "Number::operator rep() overflow")); - Number::setEnforceIntegerOverflow(true); - BEAST_EXPECT( - !toInt(a, "Number::operator rep() overflow unrepresentable")); - Number::setEnforceIntegerOverflow(false); + BEAST_EXPECT(to_string(a) == "9223372036854776e3"); + checkInt(a, 0, "Number::operator rep() overflow"); + { + NumberIntegerOverflowGuard og(true); + checkInt( + a, 0, "Number::operator rep() overflow unrepresentable"); + } a = 0xFFF'FFFF'FFFF'FFFFll; // 1152921504606846975 BEAST_EXPECT(a.getLimited()); BEAST_EXPECT(!a.fits()); BEAST_EXPECT(!a.representable()); - BEAST_EXPECT(toInt(a) == 1152921504606847000ll); - Number::setEnforceIntegerOverflow(true); - BEAST_EXPECT( - !toInt(a, "Number::operator rep() overflow unrepresentable")); - Number::setEnforceIntegerOverflow(false); + checkInt(a, 1152921504606847000ll); + { + NumberIntegerOverflowGuard og(true); + checkInt( + a, 0, "Number::operator rep() overflow unrepresentable"); + } + + // Same number, but using the "unchecked" option + a = Number{maxUint, 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, maxUint); + { + NumberIntegerOverflowGuard og(true); + checkInt(a, maxUint); + } } } From 9fae186af73ce82ce6d6847a9378e9ebc5e3dc17 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 19 Nov 2025 15:33:49 -0500 Subject: [PATCH 20/23] Fix large integer overflow - Document why STAmount to Number conversions work in canonicalize(). - Update some unit tests --- src/libxrpl/basics/Number.cpp | 6 +++--- src/libxrpl/protocol/STAmount.cpp | 6 ++++++ src/test/app/MPToken_test.cpp | 7 +++++-- src/test/basics/Number_test.cpp | 29 +++++++++++++++-------------- 4 files changed, 29 insertions(+), 19 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 3fcb6c1e899..4be125463c1 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -523,9 +523,6 @@ Number::operator/=(Number const& y) Number::operator rep() const { - if (Number::overflowLargeIntegers_ && !representable()) - throw std::overflow_error( - "Number::operator rep() overflow unrepresentable"); rep drops = mantissa_; int offset = exponent_; Guard g; @@ -543,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/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 6933f4e6d51..68c6744bb31 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -871,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/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 5e7b51c3758..73f12625a8d 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -1677,8 +1677,11 @@ class MPToken_test : public beast::unit_test::suite mptAlice.pay(alice, bob, maxMPTokenAmount); BEAST_EXPECT( mptAlice.checkMPTokenOutstandingAmount(maxMPTokenAmount)); - env.require(balance( - bob, STAmount{mptAlice.issuanceID(), 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/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index b08e2b0efc3..14a7010878e 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -823,8 +823,8 @@ class Number_test : public beast::unit_test::suite BEAST_EXPECT(a.fits()); BEAST_EXPECT(a.representable()); - auto const maxUint = 0x7FFF'FFFF'FFFF'FFFFll; - a = maxUint; + auto const maxInt = std::numeric_limits::max(); + a = maxInt; BEAST_EXPECT(a.getLimited()); BEAST_EXPECT(!a.fits()); BEAST_EXPECT(!a.representable()); @@ -836,31 +836,32 @@ class Number_test : public beast::unit_test::suite a, 0, "Number::operator rep() overflow unrepresentable"); } - a = 0xFFF'FFFF'FFFF'FFFFll; + // 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()); - checkInt(a, 1152921504606847000ll); + // 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, 0, "Number::operator rep() overflow unrepresentable"); + checkInt(a, maxInt); } - // Same number, but using the "unchecked" option - a = Number{maxUint, 0, Number::unchecked{}}; - // 1152921504606846975 + // 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()); - // Because "a" is not normalized, "to_string" returns the - // wrong answer. - BEAST_EXPECT(to_string(a) == "9223372036854775.807"); - checkInt(a, maxUint); + checkInt(a, ((maxInt / 1000) + 1) * 100); { NumberIntegerOverflowGuard og(true); - checkInt(a, maxUint); + checkInt( + a, 0, "Number::operator rep() overflow unrepresentable"); } } } From bd9346aea9b778b1e6a51c45fc3428a9e05bc928 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 19 Nov 2025 17:42:47 -0500 Subject: [PATCH 21/23] Fix Vault tests due to Number changes --- src/test/app/Vault_test.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 7d6f6a26d40..8bf3b6e2837 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3702,7 +3702,7 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(10)}); - env(tx, ter{tecPRECISION_LOSS}); + env(tx, ter{tecPATH_DRY}); env.close(); } { @@ -3748,7 +3748,7 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(Number(10, 0))}); - env(tx, ter{tecPRECISION_LOSS}); + env(tx, ter{tecPATH_DRY}); env.close(); } @@ -4121,7 +4121,7 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = STAmount(d.asset, Number(1000, 0))}); - env(tx, ter{tecPRECISION_LOSS}); + env(tx, ter{tecPATH_DRY}); env.close(); } }); @@ -4347,7 +4347,7 @@ class Vault_test : public beast::unit_test::suite .id = d.keylet.key, .holder = d.depositor, .amount = d.asset(10)}); - env(tx, ter{tecPRECISION_LOSS}); + env(tx, ter{tecPATH_DRY}); env.close(); } From 3b421cdb696f79c5121d3d24db8108259b5fbeb5 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 19 Nov 2025 18:24:19 -0500 Subject: [PATCH 22/23] More test and logging updates --- include/xrpl/basics/Number.h | 18 ++++++++++-------- src/test/app/Vault_test.cpp | 9 +++++---- src/test/basics/Number_test.cpp | 14 ++++++++++++++ src/xrpld/app/tx/detail/VaultDeposit.cpp | 2 +- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 28f73b5f1da..b94335224f8 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -45,10 +45,11 @@ class Number // 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 Number is not representable(), 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. + // 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. @@ -159,10 +160,11 @@ class Number * "mixed mode" more convenient, e.g. MPTAmount + Number. 3. In "operator rep()", which explicitly converts the number into a - 64-bit integer, if the Number is not representable(), 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. + 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 diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 8bf3b6e2837..715b8d33887 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -970,17 +970,17 @@ class Vault_test : public beast::unit_test::suite env(tx, ter(temMALFORMED)); } - // accepted range from 0 to 13 + // accepted range from 0 to 15 { auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); - tx[sfScale] = 13; + tx[sfScale] = 15; env(tx); env.close(); auto const sleVault = env.le(keylet); if (!BEAST_EXPECT(sleVault)) return; - BEAST_EXPECT((*sleVault)[sfScale] == 13); + BEAST_EXPECT((*sleVault)[sfScale] == 15); } { @@ -3696,7 +3696,8 @@ class Vault_test : public beast::unit_test::suite testCase(15, [&, this](Env& env, Data d) { testcase("MPT fractional deposits are supported"); - // Deposits large than Number::maxIntValue are invalid + // Deposits that result in share amounts larger than + // Number::maxIntValue are invalid { auto tx = d.vault.deposit( {.depositor = d.depositor, diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 14a7010878e..0c570dc320d 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -761,6 +761,7 @@ class Number_test : public beast::unit_test::suite 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 @@ -769,59 +770,72 @@ class Number_test : public beast::unit_test::suite 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; diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index ad9355d43e8..8789933ea45 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -287,7 +287,7 @@ VaultDeposit::doApply() { // 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_.warn()) // + JLOG(j_.debug()) // << "VaultDeposit: integer overflow error in total assets with" << " scale=" << (int)vault->at(sfScale).value() // << ", assetsTotal=" << vault->at(sfAssetsTotal).value() From f9293e67461b36f696698afe2e160d7e7591db55 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 20 Nov 2025 00:20:07 -0500 Subject: [PATCH 23/23] Set the global rules for every transaction step - Amendment gated on SAV and LP --- src/xrpld/app/tx/detail/applySteps.cpp | 127 +++++++++++++++---------- 1 file changed, 76 insertions(+), 51 deletions(-) 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) {