From 03e2404a68e1460b82060b7386185bcfe7896bd4 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S <ckbs.keshava56@gmail.com> Date: Mon, 18 Jul 2022 17:29:06 -0700 Subject: [PATCH] refactor: change the return type of mulDiv to std::optional (#4243) - 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> --- src/ripple/app/misc/NetworkOPs.cpp | 4 +-- src/ripple/app/misc/TxQ.h | 15 +++------ src/ripple/app/misc/impl/LoadFeeTrack.cpp | 4 +-- src/ripple/app/misc/impl/TxQ.cpp | 32 +++++++++++--------- src/ripple/basics/FeeUnits.h | 32 +++++++++++--------- src/ripple/basics/impl/mulDiv.cpp | 14 +++------ src/ripple/basics/mulDiv.h | 14 ++++----- src/ripple/rpc/handlers/Fee1.cpp | 1 - src/ripple/rpc/impl/TransactionSign.cpp | 4 +-- src/test/app/TxQ_test.cpp | 1 - src/test/basics/FeeUnits_test.cpp | 37 +++++++++++++++-------- src/test/basics/mulDiv_test.cpp | 18 +++++------ 12 files changed, 93 insertions(+), 83 deletions(-) diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 79cd857b785..4d90e0622f8 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -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), diff --git a/src/ripple/app/misc/TxQ.h b/src/ripple/app/misc/TxQ.h index 69b6d264825..c7bd1f1d3d7 100644 --- a/src/ripple/app/misc/TxQ.h +++ b/src/ripple/app/misc/TxQ.h @@ -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> @@ -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 diff --git a/src/ripple/app/misc/impl/LoadFeeTrack.cpp b/src/ripple/app/misc/impl/LoadFeeTrack.cpp index 11679c9a66e..86d145c856c 100644 --- a/src/ripple/app/misc/impl/LoadFeeTrack.cpp +++ b/src/ripple/app/misc/impl/LoadFeeTrack.cpp @@ -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 diff --git a/src/ripple/app/misc/impl/TxQ.cpp b/src/ripple/app/misc/impl/TxQ.cpp index bf278970bb8..faaca0655cf 100644 --- a/src/ripple/app/misc/impl/TxQ.cpp +++ b/src/ripple/app/misc/impl/TxQ.cpp @@ -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> diff --git a/src/ripple/basics/FeeUnits.h b/src/ripple/basics/FeeUnits.h index c74524c7c71..c0f1afbe6c6 100644 --- a/src/ripple/basics/FeeUnits.h +++ b/src/ripple/basics/FeeUnits.h @@ -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 diff --git a/src/ripple/basics/impl/mulDiv.cpp b/src/ripple/basics/impl/mulDiv.cpp index 20e72e0477a..6dd01c71fe7 100644 --- a/src/ripple/basics/impl/mulDiv.cpp +++ b/src/ripple/basics/impl/mulDiv.cpp @@ -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 diff --git a/src/ripple/basics/mulDiv.h b/src/ripple/basics/mulDiv.h index 30579c255ac..e338f87c819 100644 --- a/src/ripple/basics/mulDiv.h +++ b/src/ripple/basics/mulDiv.h @@ -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 diff --git a/src/ripple/rpc/handlers/Fee1.cpp b/src/ripple/rpc/handlers/Fee1.cpp index 554480f10af..89fa9c6ea07 100644 --- a/src/ripple/rpc/handlers/Fee1.cpp +++ b/src/ripple/rpc/handlers/Fee1.cpp @@ -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> diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index c903c26f8e3..7610682fd1a 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -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) diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index 8bf359e101c..4bc0040f867 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -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> diff --git a/src/test/basics/FeeUnits_test.cpp b/src/test/basics/FeeUnits_test.cpp index 85527423c58..3ded5812947 100644 --- a/src/test/basics/FeeUnits_test.cpp +++ b/src/test/basics/FeeUnits_test.cpp @@ -50,12 +50,17 @@ class feeunits_test : public beast::unit_test::suite 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 @@ class feeunits_test : public beast::unit_test::suite 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 @@ class feeunits_test : public beast::unit_test::suite 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>)); } } diff --git a/src/test/basics/mulDiv_test.cpp b/src/test/basics/mulDiv_test.cpp index 2dfc9760608..f51b91fecf4 100644 --- a/src/test/basics/mulDiv_test.cpp +++ b/src/test/basics/mulDiv_test.cpp @@ -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); } };