ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2025-11-27 22:45:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix underflow rounding issue:

Browse Source 
Very small payment could fail when STAmount::mulRound underflowed
and returned zero, when it should have rounded up to the smallest
representable value.
This commit is contained in:
seelabs
2015-11-04 16:18:43 -05:00
committed by Nik Bougalis
parent 4626992474
commit 999701e384
15 changed files with 200 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
src/ripple/app/paths/cursor/DeliverNodeForward.cpp
View File

@@ -43,6 +43,9 @@ TER PathCursor::deliverNodeForward (
// Zeroed in reverse pass.
node().directory.restart(multiQuality_);
STAmountCalcSwitchovers amountCalcSwitchovers (
rippleCalc_.view.info ().parentCloseTime);
saInAct.clear (saInReq);
saInFees.clear (saInReq);
@@ -107,11 +110,11 @@ TER PathCursor::deliverNodeForward (
saOutPassFunded,
node().saOfrRate,
node().saTakerPays.issue (),
true);
true, amountCalcSwitchovers);
// Offer maximum in with fees.
auto saInTotal = mulRound (saInFunded, saInFeeRate,
saInFunded.issue (), true);
saInFunded.issue (), true, amountCalcSwitchovers);
auto saInRemaining = saInReq - saInAct - saInFees;
if (saInRemaining < zero)
@@ -123,11 +126,13 @@ TER PathCursor::deliverNodeForward (
// In without fees.
auto saInPassAct = std::min (
node().saTakerPays, divRound (
saInSum, saInFeeRate, saInSum.issue (), true));
saInSum, saInFeeRate, saInSum.issue (), true,
amountCalcSwitchovers));
// Out limited by in remaining.
auto outPass = divRound (
saInPassAct, node().saOfrRate, node().saTakerGets.issue (), true);
saInPassAct, node().saOfrRate, node().saTakerGets.issue (), true,
amountCalcSwitchovers);
STAmount saOutPassMax = std::min (saOutPassFunded, outPass);
STAmount saInPassFeesMax = saInSum - saInPassAct;
@@ -243,10 +248,12 @@ TER PathCursor::deliverNodeForward (
assert (saOutPassAct < saOutPassMax);
auto inPassAct = mulRound (
saOutPassAct, node().saOfrRate, saInReq.issue (), true);
saOutPassAct, node().saOfrRate, saInReq.issue (), true,
amountCalcSwitchovers);
saInPassAct = std::min (node().saTakerPays, inPassAct);
auto inPassFees = mulRound (
saInPassAct, saInFeeRate, saInPassAct.issue (), true);
saInPassAct, saInFeeRate, saInPassAct.issue (), true,
amountCalcSwitchovers);
saInPassFees = std::min (saInPassFeesMax, inPassFees);
}

27
src/ripple/app/paths/cursor/DeliverNodeReverse.cpp
View File

@@ -45,6 +45,9 @@ TER PathCursor::deliverNodeReverseImpl (
{
TER resultCode = tesSUCCESS;
STAmountCalcSwitchovers amountCalcSwitchovers (
rippleCalc_.view.info ().parentCloseTime);
// Accumulation of what the previous node must deliver.
// Possible optimization: Note this gets zeroed on each increment, ideally
// only on first increment, then it could be a limit on the forward pass.
@@ -157,7 +160,10 @@ TER PathCursor::deliverNodeReverseImpl (
//
// Round down: prefer liquidity rather than microscopic fees.
STAmount saOutPlusFees = mulRound (
saOutPassAct, saOutFeeRate, saOutPassAct.issue (), false);
saOutPassAct, saOutFeeRate, saOutPassAct.issue (), false,
amountCalcSwitchovers);
// Offer out with fees.
JLOG (j_.trace)
@@ -178,7 +184,7 @@ TER PathCursor::deliverNodeReverseImpl (
// Round up: prefer liquidity rather than microscopic fees. But,
// limit by requested.
auto fee = divRound (saOutPlusFees, saOutFeeRate,
saOutPlusFees.issue (), true);
saOutPlusFees.issue (), true, amountCalcSwitchovers);
saOutPassAct = std::min (saOutPassReq, fee);
JLOG (j_.trace)
@@ -190,7 +196,16 @@ TER PathCursor::deliverNodeReverseImpl (
// Compute portion of input needed to cover actual output.
auto outputFee = mulRound (
saOutPassAct, node().saOfrRate, node().saTakerPays.issue (), true);
saOutPassAct, node().saOfrRate, node().saTakerPays.issue (), true,
amountCalcSwitchovers);
if (!amountCalcSwitchovers.enableUnderflowFix () && !outputFee)
{
JLOG (j_.fatal)
<< "underflow computing outputFee "
<< "saOutPassAct: " << saOutPassAct
<< " saOfrRate: " << node ().saOfrRate;
return telFAILED_PROCESSING;
}
STAmount saInPassReq = std::min (node().saTakerPays, outputFee);
STAmount saInPassAct;
@@ -255,10 +270,12 @@ TER PathCursor::deliverNodeReverseImpl (
{
// Adjust output to conform to limited input.
auto outputRequirements = divRound (
saInPassAct, node().saOfrRate, node().saTakerGets.issue (), true);
saInPassAct, node ().saOfrRate, node ().saTakerGets.issue (), true,
amountCalcSwitchovers);
saOutPassAct = std::min (saOutPassReq, outputRequirements);
auto outputFees = mulRound (
saOutPassAct, saOutFeeRate, saOutPassAct.issue (), true);
saOutPassAct, saOutFeeRate, saOutPassAct.issue (), true,
amountCalcSwitchovers);
saOutPlusFees = std::min (node().saOfferFunds, outputFees);
JLOG (j_.trace)

5
src/ripple/app/paths/cursor/ForwardLiquidityForAccount.cpp
View File

@@ -150,6 +150,9 @@ TER PathCursor::forwardLiquidityForAccount () const
<< " previousNode.saFwdRedeem:" << previousNode().saFwdRedeem
<< " previousNode.saFwdIssue:" << previousNode().saFwdIssue;
STAmountCalcSwitchovers amountCalcSwitchovers (
rippleCalc_.view.info ().parentCloseTime);
// Last node. Accept all funds. Calculate amount actually to credit.
auto& saCurReceive = pathState_.outPass();
@@ -159,7 +162,7 @@ TER PathCursor::forwardLiquidityForAccount () const
previousNode().saFwdIssue,
amountFromRate (uQualityIn),
previousNode().saFwdIssue.issue (),
true); // Amount to credit.
true, amountCalcSwitchovers); // Amount to credit.
// Amount to credit. Credit for less than received as a surcharge.
pathState_.setOutPass (previousNode().saFwdRedeem + saIssueCrd);

15
src/ripple/app/paths/cursor/RippleLiquidity.cpp
View File

@@ -135,16 +135,21 @@ void rippleLiquidity (
// If the next rate is at least as good as the current rate, process.
if (!uRateMax || uRate <= uRateMax)
{
STAmountCalcSwitchovers amountCalcSwitchovers (
rippleCalc.view.info ().parentCloseTime);
auto currency = saCur.getCurrency ();
auto uCurIssuerID = saCur.getIssuer ();
// current actual = current request * (quality out / quality in).
auto numerator = mulRound (
saCur, uQualityOut, {currency, uCurIssuerID}, true);
saCur, uQualityOut, {currency, uCurIssuerID}, true,
amountCalcSwitchovers);
// True means "round up" to get best flow.
STAmount saCurIn = divRound (
numerator, uQualityIn, {currency, uCurIssuerID}, true);
numerator, uQualityIn, {currency, uCurIssuerID}, true,
amountCalcSwitchovers);
JLOG (rippleCalc.j_.trace)
<< "rippleLiquidity:"
@@ -173,11 +178,13 @@ void rippleLiquidity (
Issue issue{currency, uCurIssuerID};
auto numerator = mulRound (
saPrv, uQualityIn, issue, true);
saPrv, uQualityIn, issue, true,
amountCalcSwitchovers);
// A part of current. All of previous. (Cur is the driver
// variable.)
STAmount saCurOut = divRound (
numerator, uQualityOut, issue, true);
numerator, uQualityOut, issue, true,
amountCalcSwitchovers);
JLOG (rippleCalc.j_.trace)
<< "rippleLiquidity:4: saCurReq=" << saCurReq;

39
src/ripple/app/tests/Offer.test.cpp
View File

@@ -121,10 +121,49 @@ public:
expect (isOffer (env, "alice", XRP (300), USD (100)) &&
isOffer (env, "alice", XRP (400), USD (200)));
}
void testTinyPayment ()
{
// Regression test for tiny payments
using namespace jtx;
auto const alice = Account ("alice");
auto const bob = Account ("bob");
auto const carol = Account ("carol");
auto const gw = Account ("gw");
auto const USD = gw["USD"];
auto const EUR = gw["EUR"];
Env env (*this);
env.fund (XRP (10000), alice, bob, carol, gw);
env.trust (USD (1000), alice, bob, carol);
env.trust (EUR (1000), alice, bob, carol);
env (pay (gw, alice, USD (100)));
env (pay (gw, carol, EUR (100)));
// Create more offers than the loop max count in DeliverNodeReverse
for (int i=0;i<101;++i)
env (offer (carol, USD (1), EUR (2)));
auto const switchoverTime = STAmountCalcSwitchovers::enableUnderflowFixCloseTime ();
for (auto timeDelta : {-10, 10}){
auto const closeTime = switchoverTime + timeDelta;
STAmountCalcSwitchovers switchover (closeTime);
env.close (NetClock::time_point (std::chrono::seconds (closeTime)));
// Will fail without the underflow fix
auto expectedResult = switchover.enableUnderflowFix () ?
tesSUCCESS : tecPATH_PARTIAL;
env (pay ("alice", "bob", EUR (epsilon)), path (~EUR),
sendmax (USD (100)), ter (expectedResult));
}
}
void run ()
{
testCanceledOffer ();
testRmFundedOffer ();
testTinyPayment ();
}
};

3
src/ripple/app/tests/Taker.test.cpp
View File

@@ -84,7 +84,8 @@ class Taker_test : public beast::unit_test::suite
cross (Amounts offer1, Quality quality1, Amounts offer2, Quality quality2)
{
/* check if composed quality should be rejected */
Quality const quality (composed_quality (quality1, quality2));
Quality const quality (composed_quality (
quality1, quality2, STAmountCalcSwitchovers{false}));
if (reject (quality))
return std::make_pair(

20
src/ripple/app/tx/impl/CreateOffer.cpp
View File

@@ -272,7 +272,8 @@ CreateOffer::dry_offer (ApplyView& view, Offer const& offer)
std::pair<bool, Quality>
CreateOffer::select_path (
bool have_direct, OfferStream const& direct,
bool have_bridge, OfferStream const& leg1, OfferStream const& leg2)
bool have_bridge, OfferStream const& leg1, OfferStream const& leg2,
STAmountCalcSwitchovers const& amountCalcSwitchovers)
{
// If we don't have any viable path, why are we here?!
assert (have_direct || have_bridge);
@@ -282,7 +283,7 @@ CreateOffer::select_path (
return std::make_pair (true, direct.tip ().quality ());
Quality const bridged_quality (composed_quality (
leg1.tip ().quality (), leg2.tip ().quality ()));
leg1.tip ().quality (), leg2.tip ().quality (), amountCalcSwitchovers));
if (have_direct)
{
@@ -349,6 +350,9 @@ CreateOffer::bridged_cross (
auto viewJ = ctx_.app.journal ("View");
STAmountCalcSwitchovers amountCalcSwitchovers (
view.info ().parentCloseTime);
// Modifying the order or logic of the operations in the loop will cause
// a protocol breaking change.
while (have_direct || have_bridge)
@@ -362,7 +366,9 @@ CreateOffer::bridged_cross (
std::tie (use_direct, quality) = select_path (
have_direct, offers_direct,
have_bridge, offers_leg1, offers_leg2);
have_bridge, offers_leg1, offers_leg2,
amountCalcSwitchovers);
// We are always looking at the best quality; we are done with
// crossing as soon as we cross the quality boundary.
@@ -465,7 +471,7 @@ CreateOffer::bridged_cross (
Throw<std::logic_error> ("bridged crossing: nothing was fully consumed.");
}
return std::make_pair(cross_result, taker.remaining_offer ());
return std::make_pair(cross_result, taker.remaining_offer (amountCalcSwitchovers));
}
std::pair<TER, Amounts>
@@ -546,7 +552,8 @@ CreateOffer::direct_cross (
Throw<std::logic_error> ("direct crossing: nothing was fully consumed.");
}
return std::make_pair(cross_result, taker.remaining_offer ());
STAmountCalcSwitchovers amountCalcSwitchovers (view.info ().parentCloseTime);
return std::make_pair(cross_result, taker.remaining_offer (amountCalcSwitchovers));
}
// Step through the stream for as long as possible, skipping any offers
@@ -590,6 +597,7 @@ CreateOffer::cross (
Taker taker (cross_type_, view, account_, taker_amount,
ctx_.tx.getFlags(), beast::Journal (takerSink));
STAmountCalcSwitchovers amountCalcSwitchovers (view.info ().parentCloseTime);
try
{
if (cross_type_ == CrossType::IouToIou)
@@ -600,7 +608,7 @@ CreateOffer::cross (
catch (std::exception const& e)
{
j_.error << "Exception during offer crossing: " << e.what ();
return std::make_pair (tecINTERNAL, taker.remaining_offer ());
return std::make_pair (tecINTERNAL, taker.remaining_offer (amountCalcSwitchovers));
}
}

3
src/ripple/app/tx/impl/CreateOffer.h
View File

@@ -77,7 +77,8 @@ private:
std::pair<bool, Quality>
select_path (
bool have_direct, OfferStream const& direct,
bool have_bridge, OfferStream const& leg1, OfferStream const& leg2);
bool have_bridge, OfferStream const& leg1, OfferStream const& leg2,
STAmountCalcSwitchovers const& amountCalcSwitchovers);
std::pair<TER, Amounts>
bridged_cross (

8
src/ripple/app/tx/impl/Taker.cpp
View File

@@ -140,7 +140,7 @@ BasicTaker::done () const
}
Amounts
BasicTaker::remaining_offer () const
BasicTaker::remaining_offer (STAmountCalcSwitchovers const& amountCalcSwitchovers) const
{
// If the taker is done, then there's no offer to place.
if (done ())
@@ -156,14 +156,16 @@ BasicTaker::remaining_offer () const
// We scale the output based on the remaining input:
return Amounts (remaining_.in, divRound (
remaining_.in, quality_.rate (), issue_out_, true));
remaining_.in, quality_.rate (), issue_out_, true,
amountCalcSwitchovers));
}
assert (remaining_.out > zero);
// We scale the input based on the remaining output:
return Amounts (mulRound (
remaining_.out, quality_.rate (), issue_in_, true), remaining_.out);
remaining_.out, quality_.rate (), issue_in_, true, amountCalcSwitchovers),
remaining_.out);
}
Amounts const&

2
src/ripple/app/tx/impl/Taker.h
View File

@@ -167,7 +167,7 @@ public:
It is always at the original offer quality (quality_)
*/
Amounts
remaining_offer () const;
remaining_offer (STAmountCalcSwitchovers const& amountCalcSwitchovers) const;
/** Returns the amount that the offer was originally placed at. */
Amounts const&

9
src/ripple/protocol/Quality.h
View File

@@ -131,14 +131,16 @@ public:
to prevent money creation.
*/
Amounts
ceil_in (Amounts const& amount, STAmount const& limit) const;
ceil_in (Amounts const& amount, STAmount const& limit,
STAmountCalcSwitchovers const& switchovers) const;
/** Returns the scaled amount with out capped.
Math is avoided if the result is exact. The input is clamped
to prevent money creation.
*/
Amounts
ceil_out (Amounts const& amount, STAmount const& limit) const;
ceil_out (Amounts const& amount, STAmount const& limit,
STAmountCalcSwitchovers const& switchovers) const;
/** Returns `true` if lhs is lower quality than `rhs`.
Lower quality means the taker receives a worse deal.
@@ -179,7 +181,8 @@ public:
@param rhs The second leg of the path: intermediate to output.
*/
Quality
composed_quality (Quality const& lhs, Quality const& rhs);
composed_quality (Quality const& lhs, Quality const& rhs,
STAmountCalcSwitchovers const& switchovers);
}

23
src/ripple/protocol/STAmount.h
View File

@@ -377,15 +377,32 @@ divide (STAmount const& v1, STAmount const& v2, Issue const& issue);
STAmount
multiply (STAmount const& v1, STAmount const& v2, Issue const& issue);
// multiply, or divide rounding result in specified direction
/** Control when bugfixes that require switchover dates are enabled */
class STAmountCalcSwitchovers
{
bool enableUnderflowFix_ {false};
public:
STAmountCalcSwitchovers () = delete;
explicit
STAmountCalcSwitchovers (std::uint32_t parentCloseTime);
explicit
STAmountCalcSwitchovers (bool enableAll)
: enableUnderflowFix_ (enableAll) {}
bool enableUnderflowFix () const;
// for tests
static std::uint32_t enableUnderflowFixCloseTime ();
};
// multiply, or divide rounding result in specified direction
STAmount
mulRound (STAmount const& v1, STAmount const& v2,
Issue const& issue, bool roundUp);
Issue const& issue, bool roundUp,
STAmountCalcSwitchovers const& switchovers);
STAmount
divRound (STAmount const& v1, STAmount const& v2,
Issue const& issue, bool roundUp);
Issue const& issue, bool roundUp,
STAmountCalcSwitchovers const& switchovers);
// Someone is offering X for Y, what is the rate?
// Rate: smaller is better, the taker wants the most out: in/out

15
src/ripple/protocol/impl/Quality.cpp
View File

@@ -67,12 +67,13 @@ Quality::operator-- (int)
}
Amounts
Quality::ceil_in (Amounts const& amount, STAmount const& limit) const
Quality::ceil_in (Amounts const& amount, STAmount const& limit,
STAmountCalcSwitchovers const& switchovers) const
{
if (amount.in > limit)
{
Amounts result (limit, divRound (
limit, rate(), amount.out.issue (), true));
limit, rate(), amount.out.issue (), true, switchovers));
// Clamp out
if (result.out > amount.out)
result.out = amount.out;
@@ -84,12 +85,13 @@ Quality::ceil_in (Amounts const& amount, STAmount const& limit) const
}
Amounts
Quality::ceil_out (Amounts const& amount, STAmount const& limit) const
Quality::ceil_out (Amounts const& amount, STAmount const& limit,
STAmountCalcSwitchovers const& switchovers) const
{
if (amount.out > limit)
{
Amounts result (mulRound (
limit, rate(), amount.in.issue (), true), limit);
limit, rate(), amount.in.issue (), true, switchovers), limit);
// Clamp in
if (result.in > amount.in)
result.in = amount.in;
@@ -101,7 +103,8 @@ Quality::ceil_out (Amounts const& amount, STAmount const& limit) const
}
Quality
composed_quality (Quality const& lhs, Quality const& rhs)
composed_quality (Quality const& lhs, Quality const& rhs,
STAmountCalcSwitchovers const& switchovers)
{
STAmount const lhs_rate (lhs.rate ());
assert (lhs_rate != zero);
@@ -110,7 +113,7 @@ composed_quality (Quality const& lhs, Quality const& rhs)
assert (rhs_rate != zero);
STAmount const rate (mulRound (
lhs_rate, rhs_rate, lhs_rate.issue (), true));
lhs_rate, rhs_rate, lhs_rate.issue (), true, switchovers));
std::uint64_t const stored_exponent (rate.exponent () + 100);
std::uint64_t const stored_mantissa (rate.mantissa());

44
src/ripple/protocol/impl/STAmount.cpp
View File

@@ -1140,7 +1140,8 @@ canonicalizeRound (bool native, std::uint64_t& value, int& offset, bool roundUp)
STAmount
mulRound (STAmount const& v1, STAmount const& v2,
Issue const& issue, bool roundUp)
Issue const& issue, bool roundUp,
STAmountCalcSwitchovers const& switchovers)
{
if (v1 == zero || v2 == zero)
return {issue};
@@ -1203,12 +1204,21 @@ mulRound (STAmount const& v1, STAmount const& v2,
int offset = offset1 + offset2 + 14;
canonicalizeRound (
isXRP (issue), amount, offset, resultNegative != roundUp);
return STAmount (issue, amount, offset, resultNegative);
STAmount result (issue, amount, offset, resultNegative);
if (switchovers.enableUnderflowFix () && roundUp && !resultNegative && !result)
{
// return the smallest value above zero
amount = STAmount::cMinValue;
offset = STAmount::cMinOffset;
return STAmount (issue, amount, offset, resultNegative);
}
return result;
}
STAmount
divRound (STAmount const& num, STAmount const& den,
Issue const& issue, bool roundUp)
Issue const& issue, bool roundUp,
STAmountCalcSwitchovers const& switchovers)
{
if (den == zero)
Throw<std::runtime_error> ("division by zero");
@@ -1254,7 +1264,15 @@ divRound (STAmount const& num, STAmount const& den,
int offset = numOffset - denOffset - 17;
canonicalizeRound (
isXRP (issue), amount, offset, resultNegative != roundUp);
return STAmount (issue, amount, offset, resultNegative);
STAmount result (issue, amount, offset, resultNegative);
if (switchovers.enableUnderflowFix () && roundUp && !resultNegative && !result)
{
// return the smallest value above zero
amount = STAmount::cMinValue;
offset = STAmount::cMinOffset;
return STAmount (issue, amount, offset, resultNegative);
}
return result;
}
// compute (value)*(mul)/(div) - avoid overflow but keep precision
@@ -1291,4 +1309,22 @@ mulDivNoThrow(std::uint64_t value, std::uint64_t mul, std::uint64_t div)
}
}
std::uint32_t
STAmountCalcSwitchovers::enableUnderflowFixCloseTime ()
{
// Mon Dec 28 10:00:00am PST
return 504'640'800;
}
STAmountCalcSwitchovers::STAmountCalcSwitchovers (std::uint32_t parentCloseTime)
{
enableUnderflowFix_ = parentCloseTime > enableUnderflowFixCloseTime();
}
bool STAmountCalcSwitchovers::enableUnderflowFix () const
{
return enableUnderflowFix_;
}
} // ripple

18
src/ripple/protocol/tests/Quality.test.cpp
View File

@@ -70,7 +70,8 @@ public:
In1 in, Out1 out, Int limit, In2 in_expected, Out2 out_expected)
{
auto expect_result (amounts (in_expected, out_expected));
auto actual_result (q.ceil_in (amounts(in, out), amount(limit)));
auto actual_result (q.ceil_in (
amounts (in, out), amount (limit), STAmountCalcSwitchovers{false}));
expect (actual_result == expect_result);
}
@@ -81,7 +82,8 @@ public:
In1 in, Out1 out, Int limit, In2 in_expected, Out2 out_expected)
{
auto const expect_result (amounts (in_expected, out_expected));
auto const actual_result (q.ceil_out (amounts(in, out), amount(limit)));
auto const actual_result (q.ceil_out (
amounts (in, out), amount (limit), STAmountCalcSwitchovers{false}));
expect (actual_result == expect_result);
}
@@ -230,7 +232,8 @@ public:
raw (2755280000000000ull, -15)); // 2.75528
STAmount const limit (
raw (4131113916555555, -16)); // .4131113916555555
Amounts const result (q.ceil_out (value, limit));
Amounts const result (
q.ceil_out (value, limit, STAmountCalcSwitchovers{false}));
expect (result.in != zero);
}
}
@@ -273,10 +276,13 @@ public:
Quality const q21 (Amounts (amount2, amount1));
Quality const q31 (Amounts (amount3, amount1));
expect (composed_quality (q12, q21) == q11);
expect (
composed_quality (q12, q21, STAmountCalcSwitchovers{false}) == q11);
Quality const q13_31 (composed_quality (q13, q31));
Quality const q31_13 (composed_quality (q31, q13));
Quality const q13_31 (
composed_quality (q13, q31, STAmountCalcSwitchovers{false}));
Quality const q31_13 (
composed_quality (q31, q13, STAmountCalcSwitchovers{false}));
expect (q13_31 == q31_13);
expect (q13_31 == q11);