Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the return type of mulDiv to std::optional #4243

Merged
merged 1 commit into from
Jul 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/ripple/app/misc/NetworkOPs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2058,7 +2058,7 @@ NetworkOPsImp::pubServer()
f.em->openLedgerFeeLevel,
f.loadBaseServer,
f.em->referenceFeeLevel)
.second);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are cases like this guaranteed to have a value? Is that why the old code didn't check for success? If so, then I think the new code should just use operator*.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cases like this are not guaranteed to have a value. When the old behavior overflowed, it passed back an overflow flag and the value ripple::muldiv_max. The client level code (in this example) didn't check the overflow flag and just took the value. In this rewrite, the client code is forced to be explicit that it is using ripple::muldiv_max on overflow.

In any case the behavior is unchanged.

.value_or(ripple::muldiv_max));

jvObj[jss::load_factor] = trunc32(loadFactor);
jvObj[jss::load_factor_fee_escalation] =
Expand Down Expand Up @@ -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),
Expand Down
15 changes: 5 additions & 10 deletions src/ripple/app/misc/TxQ.h
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ class TxQ
@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>
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/app/misc/impl/LoadFeeTrack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 changes: 18 additions & 14 deletions src/ripple/app/misc/impl/TxQ.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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));
}

//////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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{};
Expand Down Expand Up @@ -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>
Expand Down
32 changes: 18 additions & 14 deletions src/ripple/basics/FeeUnits.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -420,20 +420,20 @@ 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;
constexpr auto max = std::numeric_limits<desttype>::max();

// 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;
Expand All @@ -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
Expand All @@ -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);
Expand All @@ -475,15 +475,15 @@ 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
return feeunit::mulDivU(mul, value, 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
Expand All @@ -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
Expand All @@ -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
Expand Down
14 changes: 5 additions & 9 deletions src/ripple/basics/impl/mulDiv.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 changes: 7 additions & 7 deletions src/ripple/basics/mulDiv.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,24 @@
#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
a single step, avoiding overflow and retaining precision.
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
Expand Down
1 change: 0 additions & 1 deletion src/ripple/rpc/handlers/Fee1.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/rpc/impl/TransactionSign.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion src/test/app/TxQ_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Loading