ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2025-12-06 17:27:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor: change the return type of mulDiv to std::optional (#4243)

Browse Source 
- Previously, mulDiv had `std::pair<bool, uint64_t>` as the output type.
  - This is an error-prone interface as it is easy to ignore when
    overflow occurs.
- Using a return type of `std::optional` should decrease the likelihood
  of ignoring overflow.
  - It also allows for the use of optional::value_or() as a way to
    explicitly recover from overflow.
- Include limits.h header file preprocessing directive in order to
  satisfy gcc's numeric_limits incomplete_type requirement.

Fix #3495

---------

Co-authored-by: John Freeman <jfreeman08@gmail.com>
This commit is contained in:
Chenna Keshava B S
2023-07-05 20:11:19 -07:00
committed by GitHub
parent 77dc63b549
commit c6fee28b92
12 changed files with 93 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
src/ripple/app/misc/NetworkOPs.cpp
View File

@@ -2058,7 +2058,7 @@ NetworkOPsImp::pubServer()
f.em->openLedgerFeeLevel,
f.loadBaseServer,
f.em->referenceFeeLevel)
.second);
.value_or(ripple::muldiv_max));
jvObj[jss::load_factor] = trunc32(loadFactor);
jvObj[jss::load_factor_fee_escalation] =
@@ -2506,7 +2506,7 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters)
escalationMetrics.openLedgerFeeLevel,
loadBaseServer,
escalationMetrics.referenceFeeLevel)
.second;
.value_or(ripple::muldiv_max);
auto const loadFactor = std::max(
safe_cast<std::uint64_t>(loadFactorServer),

15
src/ripple/app/misc/TxQ.h
View File

@@ -492,7 +492,7 @@ private:
@param seriesSize Total number of transactions in the series to be
processed.
@return A `std::pair` as returned from @ref `mulDiv` indicating
@return A `std::pair` indicating
whether the calculation result overflows.
*/
static std::pair<bool, FeeLevel64>
@@ -862,20 +862,15 @@ template <class T>
XRPAmount
toDrops(FeeLevel<T> const& level, XRPAmount baseFee)
{
if (auto const drops = mulDiv(level, baseFee, TxQ::baseLevel); drops.first)
return drops.second;
return XRPAmount(STAmount::cMaxNativeN);
return mulDiv(level, baseFee, TxQ::baseLevel)
.value_or(XRPAmount(STAmount::cMaxNativeN));
}
inline FeeLevel64
toFeeLevel(XRPAmount const& drops, XRPAmount const& baseFee)
{
if (auto const feeLevel = mulDiv(drops, TxQ::baseLevel, baseFee);
feeLevel.first)
return feeLevel.second;
return FeeLevel64(std::numeric_limits<std::uint64_t>::max());
return mulDiv(drops, TxQ::baseLevel, baseFee)
.value_or(FeeLevel64(std::numeric_limits<std::uint64_t>::max()));
}
} // namespace ripple

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

@@ -109,9 +109,9 @@ scaleFeeLoad(
auto const result = mulDiv(
fee, feeFactor, safe_cast<std::uint64_t>(feeTrack.getLoadBase()));
if (!result.first)
if (!result)
Throw<std::overflow_error>("scaleFeeLoad");
return result.second;
return *result;
}
} // namespace ripple

32
src/ripple/app/misc/impl/TxQ.cpp
View File

@@ -58,12 +58,8 @@ getFeeLevelPaid(ReadView const& view, STTx const& tx)
return FeeLevel64(0);
}
if (std::pair<bool, FeeLevel64> const feeLevelPaid =
mulDiv(effectiveFeePaid, TxQ::baseLevel, baseFee);
feeLevelPaid.first)
return feeLevelPaid.second;
return FeeLevel64(std::numeric_limits<std::uint64_t>::max());
return mulDiv(effectiveFeePaid, TxQ::baseLevel, baseFee)
.value_or(FeeLevel64(std::numeric_limits<std::uint64_t>::max()));
}
static std::optional<LedgerIndex>
@@ -77,7 +73,8 @@ getLastLedgerSequence(STTx const& tx)
static FeeLevel64
increase(FeeLevel64 level, std::uint32_t increasePercent)
{
return mulDiv(level, 100 + increasePercent, 100).second;
return mulDiv(level, 100 + increasePercent, 100)
.value_or(static_cast<FeeLevel64>(ripple::muldiv_max));
}
//////////////////////////////////////////////////////////////////////////
@@ -114,16 +111,19 @@ TxQ::FeeMetrics::update(
// upperLimit must be >= minimumTxnCount_ or std::clamp can give
// unexpected results
auto const upperLimit = std::max<std::uint64_t>(
mulDiv(txnsExpected_, cutPct, 100).second, minimumTxnCount_);
mulDiv(txnsExpected_, cutPct, 100).value_or(ripple::muldiv_max),
minimumTxnCount_);
txnsExpected_ = std::clamp<std::uint64_t>(
mulDiv(size, cutPct, 100).second, minimumTxnCount_, upperLimit);
mulDiv(size, cutPct, 100).value_or(ripple::muldiv_max),
minimumTxnCount_,
upperLimit);
recentTxnCounts_.clear();
}
else if (size > txnsExpected_ || size > targetTxnCount_)
{
recentTxnCounts_.push_back(
mulDiv(size, 100 + setup.normalConsensusIncreasePercent, 100)
.second);
.value_or(ripple::muldiv_max));
auto const iter =
std::max_element(recentTxnCounts_.begin(), recentTxnCounts_.end());
BOOST_ASSERT(iter != recentTxnCounts_.end());
@@ -181,7 +181,8 @@ TxQ::FeeMetrics::scaleFeeLevel(Snapshot const& snapshot, OpenView const& view)
{
// Compute escalated fee level
// Don't care about the overflow flag
return mulDiv(multiplier, current * current, target * target).second;
return mulDiv(multiplier, current * current, target * target)
.value_or(static_cast<FeeLevel64>(ripple::muldiv_max));
}
return baseLevel;
@@ -264,7 +265,7 @@ TxQ::FeeMetrics::escalatedSeriesFeeLevel(
auto const totalFeeLevel = mulDiv(
multiplier, sumNlast.second - sumNcurrent.second, target * target);
return totalFeeLevel;
return {totalFeeLevel.has_value(), *totalFeeLevel};
}
LedgerHash TxQ::MaybeTx::parentHashComp{};
@@ -1782,8 +1783,11 @@ TxQ::getTxRequiredFeeAndSeq(
std::uint32_t const accountSeq = sle ? (*sle)[sfSequence] : 0;
std::uint32_t const availableSeq = nextQueuableSeqImpl(sle, lock).value();
return {mulDiv(fee, baseFee, baseLevel).second, accountSeq, availableSeq};
return {
mulDiv(fee, baseFee, baseLevel)
.value_or(XRPAmount(std::numeric_limits<std::int64_t>::max())),
accountSeq,
availableSeq};
}
std::vector<TxQ::TxDetails>

32
src/ripple/basics/FeeUnits.h
View File

@@ -409,7 +409,7 @@ template <
class Source2,
class Dest,
class = enable_muldiv_t<Source1, Source2, Dest>>
std::pair<bool, Dest>
std::optional<Dest>
mulDivU(Source1 value, Dest mul, Source2 div)
{
// Fees can never be negative in any context.
@@ -420,7 +420,7 @@ mulDivU(Source1 value, Dest mul, Source2 div)
assert(value.value() >= 0);
assert(mul.value() >= 0);
assert(div.value() >= 0);
return {false, Dest{0}};
return std::nullopt;
}
using desttype = typename Dest::value_type;
@@ -428,12 +428,12 @@ mulDivU(Source1 value, Dest mul, Source2 div)
// Shortcuts, since these happen a lot in the real world
if (value == div)
return {true, mul};
return mul;
if (mul.value() == div.value())
{
if (value.value() > max)
return {false, Dest{max}};
return {true, Dest{static_cast<desttype>(value.value())}};
return std::nullopt;
return Dest{static_cast<desttype>(value.value())};
}
using namespace boost::multiprecision;
@@ -447,9 +447,9 @@ mulDivU(Source1 value, Dest mul, Source2 div)
auto quotient = product / div.value();
if (quotient > max)
return {false, Dest{max}};
return std::nullopt;
return {true, Dest{static_cast<desttype>(quotient)}};
return Dest{static_cast<desttype>(quotient)};
}
} // namespace feeunit
@@ -464,7 +464,7 @@ template <
class Source2,
class Dest,
class = feeunit::enable_muldiv_t<Source1, Source2, Dest>>
std::pair<bool, Dest>
std::optional<Dest>
mulDiv(Source1 value, Dest mul, Source2 div)
{
return feeunit::mulDivU(value, mul, div);
@@ -475,7 +475,7 @@ template <
class Source2,
class Dest,
class = feeunit::enable_muldiv_commute_t<Source1, Source2, Dest>>
std::pair<bool, Dest>
std::optional<Dest>
mulDiv(Dest value, Source1 mul, Source2 div)
{
// Multiplication is commutative
@@ -483,7 +483,7 @@ mulDiv(Dest value, Source1 mul, Source2 div)
}
template <class Dest, class = feeunit::enable_muldiv_dest_t<Dest>>
std::pair<bool, Dest>
std::optional<Dest>
mulDiv(std::uint64_t value, Dest mul, std::uint64_t div)
{
// Give the scalars a non-tag so the
@@ -492,7 +492,7 @@ mulDiv(std::uint64_t value, Dest mul, std::uint64_t div)
}
template <class Dest, class = feeunit::enable_muldiv_dest_t<Dest>>
std::pair<bool, Dest>
std::optional<Dest>
mulDiv(Dest value, std::uint64_t mul, std::uint64_t div)
{
// Multiplication is commutative
@@ -503,20 +503,24 @@ template <
class Source1,
class Source2,
class = feeunit::enable_muldiv_sources_t<Source1, Source2>>
std::pair<bool, std::uint64_t>
std::optional<std::uint64_t>
mulDiv(Source1 value, std::uint64_t mul, Source2 div)
{
// Give the scalars a dimensionless unit so the
// unit-handling version gets called.
auto unitresult = feeunit::mulDivU(value, feeunit::scalar(mul), div);
return {unitresult.first, unitresult.second.value()};
if (!unitresult)
return std::nullopt;
return unitresult->value();
}
template <
class Source1,
class Source2,
class = feeunit::enable_muldiv_sources_t<Source1, Source2>>
std::pair<bool, std::uint64_t>
std::optional<std::uint64_t>
mulDiv(std::uint64_t value, Source1 mul, Source2 div)
{
// Multiplication is commutative

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

@@ -17,15 +17,13 @@
*/
//==============================================================================
#include <ripple/basics/contract.h>
#include <ripple/basics/mulDiv.h>
#include <boost/multiprecision/cpp_int.hpp>
#include <limits>
#include <utility>
#include <optional>
namespace ripple {
std::pair<bool, std::uint64_t>
std::optional<std::uint64_t>
mulDiv(std::uint64_t value, std::uint64_t mul, std::uint64_t div)
{
using namespace boost::multiprecision;
@@ -35,12 +33,10 @@ mulDiv(std::uint64_t value, std::uint64_t mul, std::uint64_t div)
result /= div;
auto constexpr limit = std::numeric_limits<std::uint64_t>::max();
if (result > ripple::muldiv_max)
return std::nullopt;
if (result > limit)
return {false, limit};
return {true, static_cast<std::uint64_t>(result)};
return static_cast<std::uint64_t>(result);
}
} // namespace ripple

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

@@ -21,9 +21,12 @@
#define RIPPLE_BASICS_MULDIV_H_INCLUDED
#include <cstdint>
#include <limits>
#include <optional>
#include <utility>
namespace ripple {
auto constexpr muldiv_max = std::numeric_limits<std::uint64_t>::max();
/** Return value*mul/div accurately.
Computes the result of the multiplication and division in
@@ -31,14 +34,11 @@ namespace ripple {
Throws:
None
Returns:
`std::pair`:
`first` is `false` if the calculation overflows,
`true` if the calculation is safe.
`second` is the result of the calculation if
`first` is `false`, max value of `uint64_t`
if `true`.
`std::optional`:
`std::nullopt` if the calculation overflows. Otherwise, `value * mul
/ div`.
*/
std::pair<bool, std::uint64_t>
std::optional<std::uint64_t>
mulDiv(std::uint64_t value, std::uint64_t mul, std::uint64_t div);
} // namespace ripple

1
src/ripple/rpc/handlers/Fee1.cpp
View File

@@ -20,7 +20,6 @@
#include <ripple/app/ledger/OpenLedger.h>
#include <ripple/app/main/Application.h>
#include <ripple/app/misc/TxQ.h>
#include <ripple/basics/mulDiv.h>
#include <ripple/protocol/ErrorCodes.h>
#include <ripple/protocol/Feature.h>
#include <ripple/rpc/Context.h>

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

@@ -738,9 +738,9 @@ checkFee(
auto const limit = [&]() {
// Scale fee units to drops:
auto const result = mulDiv(feeDefault, mult, div);
if (!result.first)
if (!result)
Throw<std::overflow_error>("mulDiv");
return result.second;
return *result;
}();
if (fee > limit)

1
src/test/app/TxQ_test.cpp
View File

@@ -22,7 +22,6 @@
#include <ripple/app/misc/TxQ.h>
#include <ripple/app/tx/apply.h>
#include <ripple/basics/Log.h>
#include <ripple/basics/mulDiv.h>
#include <ripple/protocol/ErrorCodes.h>
#include <ripple/protocol/jss.h>
#include <ripple/protocol/st.h>

37
src/test/basics/FeeUnits_test.cpp
View File

@@ -50,12 +50,17 @@ private:
FeeLevel32 f{10};
FeeLevel32 baseFee{100};
auto drops = mulDiv(baseFee, x, f).second;
auto drops = mulDiv(baseFee, x, f);
BEAST_EXPECT(drops);
BEAST_EXPECT(drops.value() == 1000);
BEAST_EXPECT(
(std::is_same_v<decltype(drops)::unit_type, feeunit::dropTag>));
BEAST_EXPECT((std::is_same_v<decltype(drops), XRPAmount>));
BEAST_EXPECT((std::is_same_v<
std::remove_reference_t<decltype(*drops)>::unit_type,
feeunit::dropTag>));
BEAST_EXPECT((std::is_same_v<
std::remove_reference_t<decltype(*drops)>,
XRPAmount>));
}
{
XRPAmount x{100};
@@ -70,12 +75,16 @@ private:
FeeLevel64 f{10};
FeeLevel64 baseFee{100};
auto drops = mulDiv(baseFee, x, f).second;
auto drops = mulDiv(baseFee, x, f);
BEAST_EXPECT(drops);
BEAST_EXPECT(drops.value() == 1000);
BEAST_EXPECT(
(std::is_same_v<decltype(drops)::unit_type, feeunit::dropTag>));
BEAST_EXPECT((std::is_same_v<decltype(drops), XRPAmount>));
BEAST_EXPECT((std::is_same_v<
std::remove_reference_t<decltype(*drops)>::unit_type,
feeunit::dropTag>));
BEAST_EXPECT((std::is_same_v<
std::remove_reference_t<decltype(*drops)>,
XRPAmount>));
}
{
FeeLevel64 x{1024};
@@ -91,12 +100,16 @@ private:
XRPAmount basefee{10};
FeeLevel64 referencefee{256};
auto drops = mulDiv(x, basefee, referencefee).second;
auto drops = mulDiv(x, basefee, referencefee);
BEAST_EXPECT(drops);
BEAST_EXPECT(drops.value() == 40);
BEAST_EXPECT(
(std::is_same_v<decltype(drops)::unit_type, feeunit::dropTag>));
BEAST_EXPECT((std::is_same_v<decltype(drops), XRPAmount>));
BEAST_EXPECT((std::is_same_v<
std::remove_reference_t<decltype(*drops)>::unit_type,
feeunit::dropTag>));
BEAST_EXPECT((std::is_same_v<
std::remove_reference_t<decltype(*drops)>,
XRPAmount>));
}
}

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

@@ -32,27 +32,27 @@ struct mulDiv_test : beast::unit_test::suite
const std::uint64_t max32 = std::numeric_limits<std::uint32_t>::max();
auto result = mulDiv(85, 20, 5);
BEAST_EXPECT(result.first && result.second == 340);
BEAST_EXPECT(result && *result == 340);
result = mulDiv(20, 85, 5);
BEAST_EXPECT(result.first && result.second == 340);
BEAST_EXPECT(result && *result == 340);
result = mulDiv(0, max - 1, max - 3);
BEAST_EXPECT(result.first && result.second == 0);
BEAST_EXPECT(result && *result == 0);
result = mulDiv(max - 1, 0, max - 3);
BEAST_EXPECT(result.first && result.second == 0);
BEAST_EXPECT(result && *result == 0);
result = mulDiv(max, 2, max / 2);
BEAST_EXPECT(result.first && result.second == 4);
BEAST_EXPECT(result && *result == 4);
result = mulDiv(max, 1000, max / 1000);
BEAST_EXPECT(result.first && result.second == 1000000);
BEAST_EXPECT(result && *result == 1000000);
result = mulDiv(max, 1000, max / 1001);
BEAST_EXPECT(result.first && result.second == 1001000);
BEAST_EXPECT(result && *result == 1001000);
result = mulDiv(max32 + 1, max32 + 1, 5);
BEAST_EXPECT(result.first && result.second == 3689348814741910323);
BEAST_EXPECT(result && *result == 3689348814741910323);
// Overflow
result = mulDiv(max - 1, max - 2, 5);
BEAST_EXPECT(!result.first && result.second == max);
BEAST_EXPECT(!result);
}
};