ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2025-11-20 02:55:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

Simplify fee handling during transaction submission:

Browse Source 
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.
This commit is contained in:
Nik Bougalis
2017-01-18 03:33:46 -08:00
parent c7de7950c4
commit 8345475bc3
8 changed files with 135 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
src/ripple/app/misc/LoadFeeTrack.h
View File

@@ -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);

47
src/ripple/app/misc/impl/LoadFeeTrack.cpp
View File

@@ -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

43
src/ripple/basics/impl/mulDiv.cpp
View File

@@ -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);
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) };
}
// 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;
}
} // ripple

33
src/ripple/basics/mulDiv.h
View File

@@ -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

29
src/ripple/rpc/impl/TransactionSign.cpp
View File

@@ -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<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") &&

8
src/test/app/LoadFeeTrack_test.cpp
View File

@@ -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);
}
};

14
src/test/basics/mulDiv_test.cpp
View File

@@ -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"));
}
}
};

64
src/test/rpc/JSONRPC_test.cpp
View File

@@ -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)]);
}
}
}