From 8345475bc37a4d6bddf1e47dc06f22ef9396bbd8 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Wed, 18 Jan 2017 03:33:46 -0800 Subject: [PATCH] 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. --- src/ripple/app/misc/LoadFeeTrack.h | 3 -- src/ripple/app/misc/impl/LoadFeeTrack.cpp | 47 +++++++++++++++-- src/ripple/basics/impl/mulDiv.cpp | 43 +++++---------- src/ripple/basics/mulDiv.h | 33 ------------ src/ripple/rpc/impl/TransactionSign.cpp | 29 +++++++--- src/test/app/LoadFeeTrack_test.cpp | 8 --- src/test/basics/mulDiv_test.cpp | 14 +---- src/test/rpc/JSONRPC_test.cpp | 64 ++++++++++++++++++++--- 8 files changed, 135 insertions(+), 106 deletions(-) diff --git a/src/ripple/app/misc/LoadFeeTrack.h b/src/ripple/app/misc/LoadFeeTrack.h index cc4c15caa..621a7ff3c 100644 --- a/src/ripple/app/misc/LoadFeeTrack.h +++ b/src/ripple/app/misc/LoadFeeTrack.h @@ -138,9 +138,6 @@ private: //------------------------------------------------------------------------------ -// 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 index 077e667a3..69c0a4bce 100644 --- a/src/ripple/app/misc/impl/LoadFeeTrack.cpp +++ b/src/ripple/app/misc/impl/LoadFeeTrack.cpp @@ -21,11 +21,12 @@ #include #include #include -#include #include #include #include #include +#include +#include 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 && +// std::is_integral_v> +// > +// void lowestTerms(T1& a, T2& b) +// { +// if (auto const gcd = std::gcd(a, b)) +// { +// a /= gcd; +// b /= gcd; +// } +// } + +template ::value && + std::is_unsigned::value && + sizeof(T1) <= sizeof(std::uint64_t) >, + class = std::enable_if_t < + std::is_integral::value && + std::is_unsigned::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 index 83e768e75..0ba6495fd 100644 --- a/src/ripple/basics/impl/mulDiv.cpp +++ b/src/ripple/basics/impl/mulDiv.cpp @@ -20,46 +20,29 @@ #include #include #include +#include #include -#include #include namespace ripple { -// compute (value)*(mul)/(div) - avoid overflow but keep precision std::pair 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::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); + + result /= div; + + auto const limit = std::numeric_limits::max(); + + if (result > limit) + return { false, limit }; + + return { true, static_cast(result) }; } -// 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("mulDiv"); - return result.second; -} - - } // ripple diff --git a/src/ripple/basics/mulDiv.h b/src/ripple/basics/mulDiv.h index cd7d41529..1ee788387 100644 --- a/src/ripple/basics/mulDiv.h +++ b/src/ripple/basics/mulDiv.h @@ -21,7 +21,6 @@ #define RIPPLE_BASICS_MULDIV_H_INCLUDED #include -#include #include namespace ripple @@ -43,38 +42,6 @@ namespace ripple std::pair 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 ::value && - std::is_unsigned::value && - sizeof(T1) <= sizeof(std::uint64_t) >, - class = std::enable_if_t < - std::is_integral::value && - std::is_unsigned::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 index d81cd7962..a595f9526 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -662,11 +662,16 @@ 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)) @@ -674,14 +679,16 @@ Json::Value checkFee ( 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("mulDiv"); + auto const result = mulDiv (drops.second, mult, div); + if (!result.first) + Throw("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 index 6a78e6da1..76761cedd 100644 --- a/src/test/app/LoadFeeTrack_test.cpp +++ b/src/test/app/LoadFeeTrack_test.cpp @@ -42,16 +42,8 @@ public: 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 index f1626c28a..3a7ca6d6b 100644 --- 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 index a464518b0..8c1da742a 100644 --- 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 @@ public: 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 @@ public: 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)]); } } }