Simplify fee handling during transaction submission:

Avoid custom overflow code; simply use 128-bit math to maintain precision and return a saturated 64-bit value as the final result. Disallow use of negative values in the `fee_mult_max` and `fee_div_max` fields. This change could potentially cause submissions with negative values that would have previously succeeded to now fail.
qjawe · Feb 1, 2017 · 8345475 · 8345475
1 parent c7de795
commit 8345475
Show file tree

Hide file tree

Showing 8 changed files with 134 additions and 105 deletions.
diff --git a/src/ripple/app/misc/LoadFeeTrack.h b/src/ripple/app/misc/LoadFeeTrack.h
@@ -138,9 +138,6 @@ class LoadFeeTrack final
 
 //------------------------------------------------------------------------------
 
-// Scale from fee units to millionths of a ripple
-std::uint64_t scaleFeeBase(std::uint64_t fee, Fees const& fees);
-
 // Scale using load as well as base rate
 std::uint64_t scaleFeeLoad(std::uint64_t fee, LoadFeeTrack const& feeTrack,
     Fees const& fees, bool bUnlimited);

diff --git a/src/ripple/app/misc/impl/LoadFeeTrack.cpp b/src/ripple/app/misc/impl/LoadFeeTrack.cpp
@@ -21,11 +21,12 @@
 #include <ripple/app/misc/LoadFeeTrack.h>
 #include <ripple/basics/contract.h>
 #include <ripple/basics/Log.h>
-#include <ripple/basics/mulDiv.h>
 #include <ripple/core/Config.h>
 #include <ripple/ledger/ReadView.h>
 #include <ripple/protocol/STAmount.h>
 #include <ripple/protocol/JsonFields.h>
+#include <cstdint>
+#include <type_traits>
 
 namespace ripple {
 
@@ -80,11 +81,47 @@ LoadFeeTrack::lowerLocalFee ()
 
 //------------------------------------------------------------------------------
 
-// Scale from fee units to millionths of a ripple
-std::uint64_t
-scaleFeeBase(std::uint64_t fee, Fees const& fees)
+// NIKB TODO: Once we get C++17, we can replace lowestTerms
+//            with this:
+//
+// template <class T1, class T2,
+//     class = std::enable_if_t<
+//         std::is_integral_v<T1> &&
+//         std::is_integral_v<T2>>
+// >
+// void lowestTerms(T1& a,  T2& b)
+// {
+//     if (auto const gcd = std::gcd(a, b))
+//     {
+//         a /= gcd;
+//         b /= gcd;
+//     }
+// }
+
+template <class T1, class T2,
+    class = std::enable_if_t <
+        std::is_integral<T1>::value &&
+        std::is_unsigned<T1>::value &&
+        sizeof(T1) <= sizeof(std::uint64_t) >,
+    class = std::enable_if_t <
+        std::is_integral<T2>::value &&
+        std::is_unsigned<T2>::value &&
+        sizeof(T2) <= sizeof(std::uint64_t) >
+>
+void lowestTerms(T1& a,  T2& b)
 {
-    return mulDivThrow (fee, fees.base, fees.units);
+    if (a == 0 && b == 0)
+        return;
+
+    std::uint64_t x = a, y = b;
+    while (y != 0)
+    {
+        auto t = x % y;
+        x = y;
+        y = t;
+    }
+    a /= x;
+    b /= x;
 }
 
 // Scale using load as well as base rate

diff --git a/src/ripple/basics/impl/mulDiv.cpp b/src/ripple/basics/impl/mulDiv.cpp
@@ -20,46 +20,29 @@
 #include <BeastConfig.h>
 #include <ripple/basics/mulDiv.h>
 #include <ripple/basics/contract.h>
+#include <boost/multiprecision/cpp_int.hpp>
 #include <limits>
-#include <stdexcept>
 #include <utility>
 
 namespace ripple
 {
 
-// compute (value)*(mul)/(div) - avoid overflow but keep precision
 std::pair<bool, std::uint64_t>
 mulDiv(std::uint64_t value, std::uint64_t mul, std::uint64_t div)
 {
-    if ((value == 0 || mul == 0) && div != 0)
-        return{ true, 0 };
-    lowestTerms(value, div);
-    lowestTerms(mul, div);
+    using namespace boost::multiprecision;
 
-    if (value < mul)
-        std::swap(value, mul);
-    constexpr std::uint64_t max =
-        std::numeric_limits<std::uint64_t>::max();
-    const auto limit = max / mul;
-    if (value > limit)
-    {
-        value /= div;
-        if (value > limit)
-            return{ false, max };
-        return{ true, value * mul };
-    }
-    return{ true, value * mul / div };
-}
+    uint128_t result;
+    result = multiply(result, value, mul);
 
-// compute (value)*(mul)/(div) - avoid overflow but keep precision
-std::uint64_t
-mulDivThrow(std::uint64_t value, std::uint64_t mul, std::uint64_t div)
-{
-    auto const result = mulDiv(value, mul, div);
-    if(!result.first)
-        Throw<std::overflow_error>("mulDiv");
-    return result.second;
-}
+    result /= div;
 
+    auto const limit = std::numeric_limits<std::uint64_t>::max();
+
+    if (result > limit)
+        return { false, limit };
+
+    return { true, static_cast<std::uint64_t>(result) };
+}
 
 } // ripple
diff --git a/src/ripple/basics/mulDiv.h b/src/ripple/basics/mulDiv.h
@@ -21,7 +21,6 @@
 #define RIPPLE_BASICS_MULDIV_H_INCLUDED
 
 #include <cstdint>
-#include <type_traits>
 #include <utility>
 
 namespace ripple
@@ -43,38 +42,6 @@ namespace ripple
 std::pair<bool, std::uint64_t>
 mulDiv(std::uint64_t value, std::uint64_t mul, std::uint64_t div);
 
-/** Return value*mul/div accurately.
-    Computes the result of the multiplication and division in
-    a single step, avoiding overflow and retaining precision.
-    Throws:
-        std::overflow_error
-*/
-std::uint64_t
-mulDivThrow(std::uint64_t value, std::uint64_t mul, std::uint64_t div);
-
-template <class T1, class T2,
-    class = std::enable_if_t <
-        std::is_integral<T1>::value &&
-        std::is_unsigned<T1>::value &&
-        sizeof(T1) <= sizeof(std::uint64_t) >,
-    class = std::enable_if_t <
-        std::is_integral<T2>::value &&
-        std::is_unsigned<T2>::value &&
-        sizeof(T2) <= sizeof(std::uint64_t) >
->
-void lowestTerms(T1& a,  T2& b)
-{
-    std::uint64_t x = a, y = b;
-    while (y != 0)
-    {
-        auto t = x % y;
-        x = y;
-        y = t;
-    }
-    a /= x;
-    b /= x;
-}
-
 } // ripple
 
 #endif
diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp
@@ -662,26 +662,33 @@ Json::Value checkFee (
         if (request[jss::fee_mult_max].isInt())
         {
             mult = request[jss::fee_mult_max].asInt();
+            if (mult < 0)
+                return RPC::make_error(rpcINVALID_PARAMS,
+                    RPC::expected_field_message(jss::fee_mult_max,
+                        "a positive integer"));
         }
         else
         {
             return RPC::make_error (rpcHIGH_FEE,
-                RPC::expected_field_message (jss::fee_mult_max, "an integer"));
+                RPC::expected_field_message (jss::fee_mult_max,
+                    "a positive integer"));
         }
     }
     if (request.isMember(jss::fee_div_max))
     {
         if (request[jss::fee_div_max].isInt())
         {
             div = request[jss::fee_div_max].asInt();
-            if (div == 0)
+            if (div <= 0)
                 return RPC::make_error(rpcINVALID_PARAMS,
-                    RPC::expected_field_message(jss::fee_div_max, "non-zero"));
+                    RPC::expected_field_message(jss::fee_div_max,
+                        "a positive integer"));
         }
         else
         {
             return RPC::make_error(rpcHIGH_FEE,
-                RPC::expected_field_message(jss::fee_div_max, "an integer"));
+                RPC::expected_field_message(jss::fee_div_max,
+                    "a positive integer"));
         }
     }
 
@@ -711,8 +718,18 @@ Json::Value checkFee (
         }
     }
 
-    auto const limit = mulDivThrow(scaleFeeBase (
-        feeDefault, ledger->fees()), mult, div);
+    auto const limit = [&]()
+    {
+        // Scale fee units to drops:
+        auto const drops = mulDiv (feeDefault,
+            ledger->fees().base, ledger->fees().units);
+        if (!drops.first)
+            Throw<std::overflow_error>("mulDiv");
+        auto const result = mulDiv (drops.second, mult, div);
+        if (!result.first)
+            Throw<std::overflow_error>("mulDiv");
+        return result.second;
+    }();
 
     if (fee > limit && fee != loadFee &&
         request.isMember("x_queue_okay") &&

diff --git a/src/test/app/LoadFeeTrack_test.cpp b/src/test/app/LoadFeeTrack_test.cpp
@@ -42,16 +42,8 @@ class LoadFeeTrack_test : public beast::unit_test::suite
             return f;
         }();
 
-        BEAST_EXPECT (scaleFeeBase (10000, fees) == 10000);
         BEAST_EXPECT (scaleFeeLoad (10000, l, fees, false) == 10000);
-        BEAST_EXPECT (scaleFeeBase (1, fees) == 1);
         BEAST_EXPECT (scaleFeeLoad (1, l, fees, false) == 1);
-
-        // Check new default fee values give same fees as old defaults
-        BEAST_EXPECT (scaleFeeBase (d.FEE_DEFAULT, fees) == 10);
-        BEAST_EXPECT (scaleFeeBase (d.FEE_ACCOUNT_RESERVE, fees) == 200 * SYSTEM_CURRENCY_PARTS);
-        BEAST_EXPECT (scaleFeeBase (d.FEE_OWNER_RESERVE, fees) == 50 * SYSTEM_CURRENCY_PARTS);
-        BEAST_EXPECT (scaleFeeBase (d.FEE_OFFER, fees) == 10);
     }
 };
 

diff --git a/src/test/basics/mulDiv_test.cpp b/src/test/basics/mulDiv_test.cpp
@@ -47,24 +47,12 @@ struct mulDiv_test : beast::unit_test::suite
         BEAST_EXPECT(result.first && result.second == 1000000);
         result = mulDiv(max, 1000, max / 1001);
         BEAST_EXPECT(result.first && result.second == 1001000);
-        // 2^64 / 5 = 3689348814741910323, but we lose some precision
-        // starting in the 10th digit to avoid the overflow.
         result = mulDiv(max32 + 1, max32 + 1, 5);
-        BEAST_EXPECT(result.first && result.second == 3689348813882916864);
+        BEAST_EXPECT(result.first && result.second == 3689348814741910323);
 
         // Overflow
         result = mulDiv(max - 1, max - 2, 5);
         BEAST_EXPECT(!result.first && result.second == max);
-
-        try
-        {
-            mulDivThrow(max - 1, max - 2, 5);
-            fail();
-        }
-        catch (std::overflow_error const& e)
-        {
-            BEAST_EXPECT(e.what() == std::string("mulDiv"));
-        }
     }
 };
 

diff --git a/src/test/rpc/JSONRPC_test.cpp b/src/test/rpc/JSONRPC_test.cpp
@@ -232,8 +232,8 @@ R"({
     }
 })",
 {
-"Invalid field 'fee_mult_max', not an integer.",
-"Invalid field 'fee_mult_max', not an integer.",
+"Invalid field 'fee_mult_max', not a positive integer.",
+"Invalid field 'fee_mult_max', not a positive integer.",
 "Missing field 'tx_json.Fee'.",
 "Missing field 'tx_json.SigningPubKey'."}},
 
@@ -253,8 +253,8 @@ R"({
     }
 })",
 {
-"Invalid field 'fee_div_max', not an integer.",
-"Invalid field 'fee_div_max', not an integer.",
+"Invalid field 'fee_div_max', not a positive integer.",
+"Invalid field 'fee_div_max', not a positive integer.",
 "Missing field 'tx_json.Fee'.",
 "Missing field 'tx_json.SigningPubKey'."}},
 
@@ -315,8 +315,8 @@ R"({
     }
 })",
 {
-"Invalid field 'fee_div_max', not non-zero.",
-"Invalid field 'fee_div_max', not non-zero.",
+"Invalid field 'fee_div_max', not a positive integer.",
+"Invalid field 'fee_div_max', not a positive integer.",
 "Missing field 'tx_json.Fee'.",
 "Missing field 'tx_json.SigningPubKey'."}},
 
@@ -2111,6 +2111,55 @@ class JSONRPC_test : public beast::unit_test::suite
                 req[jss::tx_json][jss::Fee] == 3333);
         }
 
+        {
+            // 9: negative mult
+            Json::Value req;
+            Json::Reader().parse(R"({
+                "fee_mult_max" : -5,
+                "x_queue_okay" : true,
+                "tx_json" : { }
+            })", req);
+            Json::Value result =
+                checkFee(req, Role::ADMIN, true,
+                    env.app().config(), feeTrack,
+                      env.app().getTxQ(), env.current());
+
+            BEAST_EXPECT(RPC::contains_error(result));
+        }
+
+        {
+            // 9: negative div
+            Json::Value req;
+            Json::Reader().parse(R"({
+                "fee_div_max" : -2,
+                "x_queue_okay" : true,
+                "tx_json" : { }
+            })", req);
+            Json::Value result =
+                checkFee(req, Role::ADMIN, true,
+                    env.app().config(), feeTrack,
+                      env.app().getTxQ(), env.current());
+
+            BEAST_EXPECT(RPC::contains_error(result));
+        }
+
+        {
+            // 9: negative mult & div
+            Json::Value req;
+            Json::Reader().parse(R"({
+                "fee_mult_max" : -2,
+                "fee_div_max" : -3,
+                "x_queue_okay" : true,
+                "tx_json" : { }
+            })", req);
+            Json::Value result =
+                checkFee(req, Role::ADMIN, true,
+                    env.app().config(), feeTrack,
+                      env.app().getTxQ(), env.current());
+
+            BEAST_EXPECT(RPC::contains_error(result));
+        }
+
     }
 
     // A function that can be called as though it would process a transaction.
@@ -2219,8 +2268,7 @@ class JSONRPC_test : public beast::unit_test::suite
                     if (RPC::contains_error (result))
                         errStr = result["error_message"].asString ();
 
-                    std::string const expStr (txnTest.expMsg[get<3>(testFunc)]);
-                    BEAST_EXPECT(errStr == expStr);
+                    BEAST_EXPECT(errStr == txnTest.expMsg[get<3>(testFunc)]);
                 }
             }
         }