Merge branch 'develop' into gregtatcam/mpt/xrp-mpt-math

IOUAmount arithmetic already detects overflow through Number, but
XRPAmount and MPTAmount performed raw signed integer arithmetic, which
is undefined behavior when a result leaves the int64 range.

Under featureMPTokensV2, XRPAmount::operator+=, -=, = and operator,
and MPTAmount::operator+= and -=, throw std::overflow_error when the
exact result is not representable. Without the amendment the operators
are unchanged. The check also applies when no transaction rules are
set, so RPC and pathfinding compute the same result as the transaction
they describe. STAmount::operator+ routes its native and MPT branches
through these operators so it inherits the check.

include/xrpl/basics/MathUtilities.h

@@ -39,6 +39,31 @@ checkedSub(std::int64_t a, std::int64_t b) noexcept
     return a - b;
 }
 
+/**
+ * Multiply two signed 64-bit integers, returning std::nullopt when the exact
+ * mathematical product is not representable in std::int64_t.
+ */
+[[nodiscard]] constexpr std::optional<std::int64_t>
+checkedMul(std::int64_t a, std::int64_t b) noexcept
+{
+    using L = std::numeric_limits<std::int64_t>;
+
+    if (a == 0 || b == 0)
+        return 0;
+
+    if (a > 0)
+    {
+        if (b > 0 ? a > L::max() / b : b < L::min() / a)
+            return std::nullopt;
+    }
+    else if (b > 0 ? a < L::min() / b : b < L::max() / a)
+    {
+        return std::nullopt;
+    }
+
+    return a * b;
+}
+
 static_assert(checkedAdd(0, 0) == 0);
 static_assert(checkedAdd(1, -1) == 0);
 static_assert(checkedAdd(-5, 2) == -3);

include/xrpl/protocol/XRPAmount.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <xrpl/basics/MathUtilities.h>
 #include <xrpl/basics/Number.h>
 #include <xrpl/basics/contract.h>
 #include <xrpl/beast/utility/Zero.h>
@@ -69,7 +70,13 @@ public:
     constexpr XRPAmount
     operator*(value_type const& rhs) const
     {
-        return XRPAmount{drops_ * rhs};
+        if (auto const result = checkedMul(drops_, rhs))
+            return XRPAmount{*result};
+        // Not reached in constant evaluation: an overflowing constant
+        // expression fails to compile instead.
+        XRPAmount product{*this};
+        product *= rhs;
+        return product;
     }
 
     friend constexpr XRPAmount
@@ -82,37 +89,23 @@ public:
     XRPAmount&
     operator+=(XRPAmount const& other)
     {
-        drops_ += other.drops();
-        return *this;
+        return *this += other.drops();
     }
 
     XRPAmount&
     operator-=(XRPAmount const& other)
     {
-        drops_ -= other.drops();
-        return *this;
+        return *this -= other.drops();
     }
 
     XRPAmount&
-    operator+=(value_type const& rhs)
-    {
-        drops_ += rhs;
-        return *this;
-    }
+    operator+=(value_type const& rhs);
 
     XRPAmount&
-    operator-=(value_type const& rhs)
-    {
-        drops_ -= rhs;
-        return *this;
-    }
+    operator-=(value_type const& rhs);
 
     XRPAmount&
-    operator*=(value_type const& rhs)
-    {
-        drops_ *= rhs;
-        return *this;
-    }
+    operator*=(value_type const& rhs);
 
     XRPAmount
     operator-() const

src/libxrpl/protocol/MPTAmount.cpp

@@ -1,10 +1,24 @@
 #include <xrpl/protocol/MPTAmount.h>
 
+#include <xrpl/basics/MathUtilities.h>
+#include <xrpl/basics/contract.h>
+#include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/Rules.h>
+
+#include <stdexcept>
+
 namespace xrpl {
 
 MPTAmount&
 MPTAmount::operator+=(MPTAmount const& other)
 {
+    if (auto const result = checkedAdd(value_, other.value()))
+    {
+        value_ = *result;
+        return *this;
+    }
+    if (isFeatureEnabled(fixCleanup3_5_0, /*resultIfNoRules*/ true))
+        Throw<std::overflow_error>("MPTAmount::operator+= overflow");
     value_ += other.value();
     return *this;
 }
@@ -12,6 +26,13 @@ MPTAmount::operator+=(MPTAmount const& other)
 MPTAmount&
 MPTAmount::operator-=(MPTAmount const& other)
 {
+    if (auto const result = checkedSub(value_, other.value()))
+    {
+        value_ = *result;
+        return *this;
+    }
+    if (isFeatureEnabled(fixCleanup3_5_0, /*resultIfNoRules*/ true))
+        Throw<std::overflow_error>("MPTAmount::operator-= overflow");
     value_ -= other.value();
     return *this;
 }

src/libxrpl/protocol/STAmount.cpp

@@ -1,6 +1,7 @@
 #include <xrpl/protocol/STAmount.h>
 
 #include <xrpl/basics/Log.h>
+#include <xrpl/basics/MathUtilities.h>
 #include <xrpl/basics/Number.h>
 #include <xrpl/basics/contract.h>
 #include <xrpl/basics/safe_cast.h>
@@ -384,10 +385,11 @@ operator+(STAmount const& v1, STAmount const& v2)
         return {v1.getFName(), v1.asset(), v2.mantissa(), v2.exponent(), v2.negative()};
     }
 
+    // XRPAmount and MPTAmount check for int64 overflow under fixCleanup3_5_0.
     if (v1.native())
-        return {v1.getFName(), getSNValue(v1) + getSNValue(v2)};
+        return {v1.getFName(), (v1.xrp() + v2.xrp()).drops()};
     if (v1.holds<MPTIssue>())
-        return {v1.asset_, v1.mpt().value() + v2.mpt().value()};
+        return {v1.asset_, (v1.mpt() + v2.mpt()).value()};
 
     auto x = v1;
     x = v1.iou() + v2.iou();
@@ -474,16 +476,7 @@ canAdd(STAmount const& a, STAmount const& b)
 
     // XRP case (overflow & underflow check)
     if (isXRP(a) && isXRP(b))
-    {
-        XRPAmount const aVal = a.xrp();
-        XRPAmount const bVal = b.xrp();
-
-        return !(
-            (bVal > XRPAmount{0} &&
-             aVal > XRPAmount{std::numeric_limits<XRPAmount::value_type>::max()} - bVal) ||
-            (bVal < XRPAmount{0} &&
-             aVal < XRPAmount{std::numeric_limits<XRPAmount::value_type>::min()} - bVal));
-    }
+        return checkedAdd(a.xrp().drops(), b.xrp().drops()).has_value();
 
     // IOU case (precision check)
     auto const ret = std::visit(
@@ -500,15 +493,7 @@ canAdd(STAmount const& a, STAmount const& b)
 
             // MPT (overflow & underflow check)
             if constexpr (kIsMptissueV<TIss1> && kIsMptissueV<TIss2>)
-            {
-                MPTAmount const aVal = a.mpt();
-                MPTAmount const bVal = b.mpt();
-                return !(
-                    (bVal > MPTAmount{0} &&
-                     aVal > MPTAmount{std::numeric_limits<MPTAmount::value_type>::max()} - bVal) ||
-                    (bVal < MPTAmount{0} &&
-                     aVal < MPTAmount{std::numeric_limits<MPTAmount::value_type>::min()} - bVal));
-            }
+                return checkedAdd(a.mpt().value(), b.mpt().value()).has_value();
             return std::nullopt;
         },
         a.asset().value(),
@@ -559,11 +544,7 @@ canSubtract(STAmount const& a, STAmount const& b)
             return false;
 
         // Check for overflow
-        if (bVal < XRPAmount{0} &&
-            aVal > XRPAmount{std::numeric_limits<XRPAmount::value_type>::max()} + bVal)
-            return false;
-
-        return true;
+        return checkedSub(aVal.drops(), bVal.drops()).has_value();
     }
 
     // IOU case (no underflow)
@@ -586,10 +567,7 @@ canSubtract(STAmount const& a, STAmount const& b)
                     return false;
 
                 // Overflow check
-                if (bVal < MPTAmount{0} &&
-                    aVal > MPTAmount{std::numeric_limits<MPTAmount::value_type>::max()} + bVal)
-                    return false;
-                return true;
+                return checkedSub(aVal.value(), bVal.value()).has_value();
             }
             return std::nullopt;
         },

src/libxrpl/protocol/XRPAmount.cpp

@@ -0,0 +1,54 @@
+#include <xrpl/protocol/XRPAmount.h>
+
+#include <xrpl/basics/MathUtilities.h>
+#include <xrpl/basics/contract.h>
+#include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/Rules.h>
+
+#include <stdexcept>
+
+namespace xrpl {
+
+XRPAmount&
+XRPAmount::operator+=(value_type const& rhs)
+{
+    if (auto const result = checkedAdd(drops_, rhs))
+    {
+        drops_ = *result;
+        return *this;
+    }
+    if (isFeatureEnabled(fixCleanup3_5_0, /*resultIfNoRules*/ true))
+        Throw<std::overflow_error>("XRPAmount::operator+= overflow");
+    drops_ += rhs;
+    return *this;
+}
+
+XRPAmount&
+XRPAmount::operator-=(value_type const& rhs)
+{
+    if (auto const result = checkedSub(drops_, rhs))
+    {
+        drops_ = *result;
+        return *this;
+    }
+    if (isFeatureEnabled(fixCleanup3_5_0, /*resultIfNoRules*/ true))
+        Throw<std::overflow_error>("XRPAmount::operator-= overflow");
+    drops_ -= rhs;
+    return *this;
+}
+
+XRPAmount&
+XRPAmount::operator*=(value_type const& rhs)
+{
+    if (auto const result = checkedMul(drops_, rhs))
+    {
+        drops_ = *result;
+        return *this;
+    }
+    if (isFeatureEnabled(fixCleanup3_5_0, /*resultIfNoRules*/ true))
+        Throw<std::overflow_error>("XRPAmount::operator*= overflow");
+    drops_ *= rhs;
+    return *this;
+}
+
+}  // namespace xrpl

src/test/protocol/STAmount_test.cpp

@@ -851,14 +851,17 @@ public:
             STAmount const amt1(std::numeric_limits<XRPAmount::value_type>::max());
             STAmount const amt2(XRPAmount(1));
             BEAST_EXPECT(canAdd(amt1, amt2) == false);
+            BEAST_EXPECT(except<std::overflow_error>([&] { (void)(amt1 + amt2); }));
         }
 
         // Overflow check for min XRP amounts
         {
-            STAmount amt1(std::numeric_limits<XRPAmount::value_type>::max());
-            amt1 += XRPAmount(1);
-            STAmount const amt2(XRPAmount(-1));
+            STAmount const amt1(
+                static_cast<std::uint64_t>(std::numeric_limits<XRPAmount::value_type>::max()),
+                true);
+            STAmount const amt2(XRPAmount(-2));
             BEAST_EXPECT(canAdd(amt1, amt2) == false);
+            BEAST_EXPECT(except<std::overflow_error>([&] { (void)(amt1 + amt2); }));
         }
     }
 
@@ -968,6 +971,7 @@ public:
             STAmount const amt1(mpt, std::numeric_limits<MPTAmount::value_type>::max());
             STAmount const amt2(mpt, 1);
             BEAST_EXPECT(canAdd(amt1, amt2) == false);
+            BEAST_EXPECT(except<std::overflow_error>([&] { (void)(amt1 + amt2); }));
         }
 
         // Overflow check for min MPT amounts
@@ -1116,10 +1120,12 @@ public:
 
         // Underflow check for min XRP amounts
         {
-            STAmount amt1(std::numeric_limits<XRPAmount::value_type>::max());
-            amt1 += XRPAmount(1);
-            STAmount const amt2(XRPAmount(1));
+            STAmount const amt1(
+                static_cast<std::uint64_t>(std::numeric_limits<XRPAmount::value_type>::max()),
+                true);
+            STAmount const amt2(XRPAmount(2));
             BEAST_EXPECT(canSubtract(amt1, amt2) == false);
+            BEAST_EXPECT(except<std::overflow_error>([&] { (void)(amt1 - amt2); }));
         }
 
         // Overflow check for max XRP amounts
@@ -1127,6 +1133,7 @@ public:
             STAmount const amt1(std::numeric_limits<XRPAmount::value_type>::max());
             STAmount const amt2(XRPAmount(-1));
             BEAST_EXPECT(canSubtract(amt1, amt2) == false);
+            BEAST_EXPECT(except<std::overflow_error>([&] { (void)(amt1 - amt2); }));
         }
     }
 

src/tests/libxrpl/basics/MPTAmount.cpp

@@ -0,0 +1,106 @@
+#include <xrpl/protocol/MPTAmount.h>
+
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/beast/hash/uhash.h>
+#include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/Rules.h>
+
+#include <gtest/gtest.h>
+
+#include <limits>
+#include <stdexcept>
+#include <unordered_set>
+
+namespace xrpl {
+
+namespace {
+
+constexpr auto kMaxMpt = std::numeric_limits<MPTAmount::value_type>::max();
+constexpr auto kMinMpt = std::numeric_limits<MPTAmount::value_type>::min();
+
+Rules
+makeRules(bool withFix)
+{
+    // Rules keeps a reference to its presets, so they must outlive it.
+    static std::unordered_set<uint256, beast::Uhash<>> const kWithFix{fixCleanup3_5_0};
+    static std::unordered_set<uint256, beast::Uhash<>> const kWithoutFix;
+    return Rules{withFix ? kWithFix : kWithoutFix};
+}
+
+void
+expectBoundariesDoNotThrow()
+{
+    MPTAmount const max(kMaxMpt);
+    MPTAmount const min(kMinMpt);
+
+    EXPECT_EQ(MPTAmount(kMaxMpt - 1) + MPTAmount(1), max);
+    EXPECT_EQ(MPTAmount(kMinMpt + 1) - MPTAmount(1), min);
+    EXPECT_EQ(MPTAmount(kMinMpt + 1) + MPTAmount(-1), min);
+    EXPECT_EQ(MPTAmount(kMaxMpt - 1) - MPTAmount(-1), max);
+    EXPECT_EQ(max + min, MPTAmount(-1));
+    EXPECT_EQ(max - max, MPTAmount(0));
+    EXPECT_EQ(min - min, MPTAmount(0));
+    EXPECT_EQ(MPTAmount(kMaxMpt / 2) + MPTAmount(kMaxMpt / 2), MPTAmount(kMaxMpt - 1));
+}
+
+void
+expectOverflowsThrow()
+{
+    // += and -= leave the lhs unchanged after the throw
+    {
+        MPTAmount a(kMaxMpt);
+        EXPECT_THROW(a += MPTAmount(1), std::overflow_error);
+        EXPECT_EQ(a, MPTAmount(kMaxMpt));
+
+        MPTAmount b(kMinMpt);
+        EXPECT_THROW(b += MPTAmount(-1), std::overflow_error);
+        EXPECT_EQ(b, MPTAmount(kMinMpt));
+
+        MPTAmount c(kMinMpt);
+        EXPECT_THROW(c -= MPTAmount(1), std::overflow_error);
+        EXPECT_EQ(c, MPTAmount(kMinMpt));
+
+        MPTAmount d(kMaxMpt);
+        EXPECT_THROW(d -= MPTAmount(-1), std::overflow_error);
+        EXPECT_EQ(d, MPTAmount(kMaxMpt));
+    }
+
+    // Binary + and - are built from the compound operators
+    {
+        // Two amounts that are each a legal MPT balance
+        MPTAmount const half((kMaxMpt / 2) + 1);
+        EXPECT_THROW((void)(half + half), std::overflow_error);
+
+        MPTAmount const max(kMaxMpt);
+        MPTAmount const negMax = MPTAmount(0) - max;
+        EXPECT_THROW((void)(negMax - max), std::overflow_error);
+    }
+}
+
+}  // namespace
+
+TEST(MPTAmountTest, overflow_throws_with_fix)
+{
+    CurrentTransactionRulesGuard const rg(makeRules(true));
+    expectOverflowsThrow();
+    expectBoundariesDoNotThrow();
+}
+
+// With no rules set (RPC, pathfinding) the operators throw as if the
+// amendment were enabled.
+TEST(MPTAmountTest, overflow_throws_without_rules)
+{
+    ASSERT_FALSE(getCurrentTransactionRules());
+    expectOverflowsThrow();
+    expectBoundariesDoNotThrow();
+}
+
+// Without the amendment an overflow keeps the legacy signed arithmetic, which
+// is undefined behavior, so only results that do not overflow are checked.
+TEST(MPTAmountTest, boundaries_without_fix)
+{
+    CurrentTransactionRulesGuard const rg(makeRules(false));
+    expectBoundariesDoNotThrow();
+}
+
+}  // namespace xrpl

src/tests/libxrpl/basics/XRPAmount.cpp

@@ -1,11 +1,18 @@
 #include <xrpl/protocol/XRPAmount.h>
 
+#include <xrpl/basics/base_uint.h>
+#include <xrpl/beast/hash/uhash.h>
 #include <xrpl/beast/utility/Zero.h>
+#include <xrpl/protocol/Feature.h>
+#include <xrpl/protocol/Rules.h>
+#include <xrpl/protocol/SystemParameters.h>
 
 #include <gtest/gtest.h>
 
 #include <cstdint>
 #include <limits>
+#include <stdexcept>
+#include <unordered_set>
 
 namespace xrpl {
 
@@ -314,4 +321,150 @@ TEST_F(XRPAmountMulRatioTest, underflow_saturates_at_the_minimum)
     EXPECT_EQ(mulRatio(bigNegative, 2, 1, true), kMinXrp);
 }
 
+namespace {
+
+constexpr auto kMaxDrops = std::numeric_limits<XRPAmount::value_type>::max();
+constexpr auto kMinDrops = std::numeric_limits<XRPAmount::value_type>::min();
+
+Rules
+makeRules(bool withFix)
+{
+    // Rules keeps a reference to its presets, so they must outlive it.
+    static std::unordered_set<uint256, beast::Uhash<>> const kWithFix{fixCleanup3_5_0};
+    static std::unordered_set<uint256, beast::Uhash<>> const kWithoutFix;
+    return Rules{withFix ? kWithFix : kWithoutFix};
+}
+
+void
+expectBoundariesDoNotThrow()
+{
+    XRPAmount const max(kMaxDrops);
+    XRPAmount const min(kMinDrops);
+
+    EXPECT_EQ(XRPAmount(kMaxDrops - 1) + XRPAmount(1), max);
+    EXPECT_EQ(XRPAmount(kMinDrops + 1) - XRPAmount(1), min);
+    EXPECT_EQ(max + min, XRPAmount(-1));
+    EXPECT_EQ(min - min, XRPAmount(0));
+
+    XRPAmount a(kMaxDrops - 1);
+    a += XRPAmount::value_type{1};
+    EXPECT_EQ(a, max);
+    a -= kMaxDrops;
+    EXPECT_EQ(a, XRPAmount(0));
+
+    EXPECT_EQ(XRPAmount(kMaxDrops / 2) * 2, XRPAmount(kMaxDrops - 1));
+    EXPECT_EQ(2 * XRPAmount(kMaxDrops / 2), XRPAmount(kMaxDrops - 1));
+    EXPECT_EQ(min * 1, min);
+    EXPECT_EQ(min * 0, XRPAmount(0));
+    EXPECT_EQ(0 * min, XRPAmount(0));
+    EXPECT_EQ(max * -1, XRPAmount(-kMaxDrops));
+    EXPECT_EQ(XRPAmount(-2) * -3, XRPAmount(6));
+    EXPECT_EQ(XRPAmount(3) * -2, XRPAmount(-6));
+    EXPECT_EQ(XRPAmount(-3) * 2, XRPAmount(-6));
+
+    XRPAmount b(kMinDrops / 2);
+    b *= 2;
+    EXPECT_EQ(b, min);
+
+    // The largest legal XRP amounts cannot overflow a single operation
+    XRPAmount const maxLegal(kInitialXrp);
+    EXPECT_EQ(maxLegal + maxLegal, XRPAmount(2 * kInitialXrp.drops()));
+    EXPECT_EQ(XRPAmount(0) - maxLegal - maxLegal, XRPAmount(-2 * kInitialXrp.drops()));
+}
+
+void
+expectOverflowsThrow()
+{
+    // += and -= with an XRPAmount leave the lhs unchanged after the throw
+    {
+        XRPAmount a(kMaxDrops);
+        EXPECT_THROW(a += XRPAmount(1), std::overflow_error);
+        EXPECT_EQ(a, XRPAmount(kMaxDrops));
+
+        XRPAmount b(kMinDrops);
+        EXPECT_THROW(b += XRPAmount(-1), std::overflow_error);
+        EXPECT_EQ(b, XRPAmount(kMinDrops));
+
+        XRPAmount c(kMinDrops);
+        EXPECT_THROW(c -= XRPAmount(1), std::overflow_error);
+        EXPECT_EQ(c, XRPAmount(kMinDrops));
+
+        XRPAmount d(kMaxDrops);
+        EXPECT_THROW(d -= XRPAmount(-1), std::overflow_error);
+        EXPECT_EQ(d, XRPAmount(kMaxDrops));
+    }
+
+    // += and -= with a scalar
+    {
+        XRPAmount a(kMaxDrops);
+        EXPECT_THROW(a += XRPAmount::value_type{1}, std::overflow_error);
+        EXPECT_EQ(a, XRPAmount(kMaxDrops));
+
+        XRPAmount b(kMinDrops);
+        EXPECT_THROW(b -= XRPAmount::value_type{1}, std::overflow_error);
+        EXPECT_EQ(b, XRPAmount(kMinDrops));
+
+        XRPAmount c(kMinDrops);
+        EXPECT_THROW(c += XRPAmount::value_type{-1}, std::overflow_error);
+        EXPECT_EQ(c, XRPAmount(kMinDrops));
+
+        XRPAmount d(kMaxDrops);
+        EXPECT_THROW(d -= XRPAmount::value_type{-1}, std::overflow_error);
+        EXPECT_EQ(d, XRPAmount(kMaxDrops));
+    }
+
+    // Binary + and - are built from the compound operators
+    {
+        XRPAmount const max(kMaxDrops);
+        EXPECT_THROW((void)(max + XRPAmount(1)), std::overflow_error);
+        EXPECT_THROW((void)((XRPAmount(0) - max) - max), std::overflow_error);
+    }
+
+    // *= and both orders of binary *
+    {
+        XRPAmount a(kMaxDrops);
+        EXPECT_THROW(a *= 2, std::overflow_error);
+        EXPECT_EQ(a, XRPAmount(kMaxDrops));
+
+        XRPAmount b(kMinDrops);
+        EXPECT_THROW(b *= -1, std::overflow_error);
+        EXPECT_EQ(b, XRPAmount(kMinDrops));
+
+        XRPAmount const half((kMaxDrops / 2) + 1);
+        EXPECT_THROW((void)(half * 2), std::overflow_error);
+        EXPECT_THROW((void)(2 * half), std::overflow_error);
+        EXPECT_THROW((void)(XRPAmount(kMinDrops) * -1), std::overflow_error);
+        EXPECT_THROW((void)(-1 * XRPAmount(kMinDrops)), std::overflow_error);
+        // Mixed signs
+        EXPECT_THROW((void)(XRPAmount(kMaxDrops) * -2), std::overflow_error);
+        EXPECT_THROW((void)(XRPAmount(kMinDrops) * 2), std::overflow_error);
+    }
+}
+
+}  // namespace
+
+TEST(XRPAmountTest, overflow_throws_with_fix)
+{
+    CurrentTransactionRulesGuard const rg(makeRules(true));
+    expectOverflowsThrow();
+    expectBoundariesDoNotThrow();
+}
+
+// With no rules set (RPC, pathfinding) the operators throw as if the
+// amendment were enabled.
+TEST(XRPAmountTest, overflow_throws_without_rules)
+{
+    ASSERT_FALSE(getCurrentTransactionRules());
+    expectOverflowsThrow();
+    expectBoundariesDoNotThrow();
+}
+
+// Without the amendment an overflow keeps the legacy signed arithmetic, which
+// is undefined behavior, so only results that do not overflow are checked.
+TEST(XRPAmountTest, boundaries_without_fix)
+{
+    CurrentTransactionRulesGuard const rg(makeRules(false));
+    expectBoundariesDoNotThrow();
+}
+
 }  // namespace xrpl