diff --git a/src/ripple/app/paths/Flow.cpp b/src/ripple/app/paths/Flow.cpp index 15b285699..1c9bd7c4f 100644 --- a/src/ripple/app/paths/Flow.cpp +++ b/src/ripple/app/paths/Flow.cpp @@ -66,9 +66,13 @@ flow ( boost::optional const& sendMax, beast::Journal j) { - - Issue const srcIssue = - sendMax ? sendMax->issue () : Issue (deliver.issue ().currency, src); + Issue const srcIssue = [&] { + if (sendMax) + return sendMax->issue (); + if (!isXRP (deliver.issue ().currency)) + return Issue (deliver.issue ().currency, src); + return xrpIssue (); + }(); Issue const dstIssue = deliver.issue (); diff --git a/src/ripple/app/paths/impl/BookStep.cpp b/src/ripple/app/paths/impl/BookStep.cpp index d8968cb0c..6e4d4c8b0 100644 --- a/src/ripple/app/paths/impl/BookStep.cpp +++ b/src/ripple/app/paths/impl/BookStep.cpp @@ -102,6 +102,12 @@ public: return EitherAmount (cache_->out); } + boost::optional + bookStepBook () const override + { + return book_; + } + std::pair revImp ( PaymentSandbox& sb, @@ -258,7 +264,7 @@ forEachOffer ( break; } - return {offers.toRemove (), counter.count()}; + return {offers.permToRemove (), counter.count()}; } template @@ -546,11 +552,16 @@ BookStep::check(StrandContext const& ctx) const JLOG (j_.debug()) << "Book: currency is inconsistent with issuer." << *this; return temBAD_PATH; } - if (!ctx.seenBooks.insert (book_).second) + + // Do not allow two books to output the same issue. This may cause offers on + // one step to unfund offers in another step. + if (!ctx.seenBookOuts.insert (book_.out).second || + ctx.seenDirectIssues[0].count (book_.out)) { JLOG (j_.debug()) << "BookStep: loop detected: " << *this; return temBAD_PATH_LOOP; } + return tesSUCCESS; } diff --git a/src/ripple/app/paths/impl/DirectStep.cpp b/src/ripple/app/paths/impl/DirectStep.cpp index efb77e208..9a492a34b 100644 --- a/src/ripple/app/paths/impl/DirectStep.cpp +++ b/src/ripple/app/paths/impl/DirectStep.cpp @@ -109,6 +109,12 @@ class DirectStepI : public StepImp return EitherAmount (cache_->out); } + boost::optional + directStepSrcAcct () const override + { + return src_; + } + std::pair revImp ( PaymentSandbox& sb, @@ -565,21 +571,43 @@ TER DirectStepI::check (StrandContext const& ctx) const if (ctx.prevStep) { - if (auto pds = dynamic_cast (ctx.prevStep)) + if (auto prevSrc = ctx.prevStep->directStepSrcAcct ()) { - auto const ter = checkNoRipple ( - ctx.view, pds->src (), src_, dst_, currency_, j_); + auto const ter = + checkNoRipple (ctx.view, *prevSrc, src_, dst_, currency_, j_); if (ter != tesSUCCESS) return ter; } } - if (!ctx.seenDirectIssues[0].insert (Issue{currency_, src_}).second || - !ctx.seenDirectIssues[1].insert (Issue{currency_, dst_}).second) { - JLOG (j_.debug()) << "DirectStepI: loop detected: Index: " - << ctx.strandSize << ' ' << *this; - return temBAD_PATH_LOOP; + Issue const srcIssue{currency_, src_}; + Issue const dstIssue{currency_, dst_}; + + if (ctx.seenBookOuts.count (srcIssue)) + { + if (!ctx.prevStep) + { + assert(0); // prev seen book without a prev step!?! + return temBAD_PATH_LOOP; + } + + // This is OK if the previous step is a book step that outputs this issue + if (auto book = ctx.prevStep->bookStepBook()) + { + if (book->out != srcIssue) + return temBAD_PATH_LOOP; + } + } + + if (!ctx.seenDirectIssues[0].insert (srcIssue).second || + !ctx.seenDirectIssues[1].insert (dstIssue).second) + { + JLOG (j_.debug ()) + << "DirectStepI: loop detected: Index: " << ctx.strandSize + << ' ' << *this; + return temBAD_PATH_LOOP; + } } { diff --git a/src/ripple/app/paths/impl/PaySteps.cpp b/src/ripple/app/paths/impl/PaySteps.cpp index 3f13e4dbd..374dec4ed 100644 --- a/src/ripple/app/paths/impl/PaySteps.cpp +++ b/src/ripple/app/paths/impl/PaySteps.cpp @@ -226,14 +226,14 @@ toStrand ( */ std::array, 2> seenDirectIssues; // A strand may not include the same offer book more than once - boost::container::flat_set seenBooks; + boost::container::flat_set seenBookOuts; seenDirectIssues[0].reserve (pes.size()); seenDirectIssues[1].reserve (pes.size()); - seenBooks.reserve (pes.size()); + seenBookOuts.reserve (pes.size()); auto ctx = [&](bool isLast = false) { return StrandContext{view, result, strandSrc, strandDst, isLast, - seenDirectIssues, seenBooks, j}; + seenDirectIssues, seenBookOuts, j}; }; for (int i = 0; i < pes.size () - 1; ++i) @@ -456,7 +456,7 @@ StrandContext::StrandContext ( AccountID strandDst_, bool isLast_, std::array, 2>& seenDirectIssues_, - boost::container::flat_set& seenBooks_, + boost::container::flat_set& seenBookOuts_, beast::Journal j_) : view (view_) , strandSrc (strandSrc_) @@ -467,7 +467,7 @@ StrandContext::StrandContext ( , prevStep (!strand_.empty () ? strand_.back ().get () : nullptr) , seenDirectIssues(seenDirectIssues_) - , seenBooks(seenBooks_) + , seenBookOuts(seenBookOuts_) , j (j_) { } diff --git a/src/ripple/app/paths/impl/Steps.h b/src/ripple/app/paths/impl/Steps.h index 7de6680d2..27c817598 100644 --- a/src/ripple/app/paths/impl/Steps.h +++ b/src/ripple/app/paths/impl/Steps.h @@ -54,13 +54,14 @@ class ApplyView; when executing `fwd` or `rev`, but all those offers will be from the same quality directory. - A step may not have enough liquidity to transform the entire requested amount. Both - `fwd` and `rev` return a pair of amounts (one for input amount, one for output amount) - that show how much of the requested amount the step was actually able use. + A step may not have enough liquidity to transform the entire requested + amount. Both `fwd` and `rev` return a pair of amounts (one for input amount, + one for output amount) that show how much of the requested amount the step + was actually able to use. */ class Step { - public: +public: virtual ~Step () = default; /** @@ -109,6 +110,25 @@ class Step boost::optional cachedOut () const = 0; + /** + If this step is DirectStepI (IOU->IOU direct step), return the src + account. This is needed for checkNoRipple. + */ + virtual boost::optional + directStepSrcAcct () const + { + return boost::none; + } + + /** + If this step is a BookStep, return the book. + */ + virtual boost::optional + bookStepBook () const + { + return boost::none; + } + /** Check if amount is zero */ @@ -122,6 +142,10 @@ class Step EitherAmount const& lhs, EitherAmount const& rhs) const = 0; + virtual bool equalIn ( + EitherAmount const& lhs, + EitherAmount const& rhs) const = 0; + /** Check that the step can correctly execute in the forward direction @@ -267,18 +291,22 @@ struct StepImp : public Step } bool - equalOut ( - EitherAmount const& lhs, - EitherAmount const& rhs) const override + equalOut (EitherAmount const& lhs, EitherAmount const& rhs) const override { - return get (lhs) == get (rhs); + return get (lhs) == get (rhs); + } + + bool + equalIn (EitherAmount const& lhs, EitherAmount const& rhs) const override + { + return get (lhs) == get (rhs); } }; // Thrown when unexpected errors occur class FlowException : public std::runtime_error { - public: +public: TER ter; std::string msg; @@ -312,14 +340,14 @@ struct StrandContext bool const isLast = false; size_t const strandSize; // The previous step in the strand. Needed to check the no ripple constraint - Step const * const prevStep = nullptr; + Step const* const prevStep = nullptr; // A strand may not include the same account node more than once // in the same currency. In a direct step, an account will show up // at most twice: once as a src and once as a dst (hence the two element array). // The strandSrc and strandDst will only show up once each. std::array, 2>& seenDirectIssues; - // A strand may not include the same offer book more than once - boost::container::flat_set& seenBooks; + // A strand may not include an offer that output the same issue more than once + boost::container::flat_set& seenBookOuts; beast::Journal j; StrandContext (ReadView const& view_, @@ -330,12 +358,11 @@ struct StrandContext AccountID strandDst_, bool isLast_, std::array, 2>& seenDirectIssues_, - boost::container::flat_set& seenBooks_, + boost::container::flat_set& seenBookOuts_, beast::Journal j); }; -namespace test -{ +namespace test { // Needed for testing bool directStepEqual (Step const& step, AccountID const& src, diff --git a/src/ripple/app/paths/impl/StrandFlow.h b/src/ripple/app/paths/impl/StrandFlow.h index 5d1b72d2c..fbdc1a5b6 100644 --- a/src/ripple/app/paths/impl/StrandFlow.h +++ b/src/ripple/app/paths/impl/StrandFlow.h @@ -181,7 +181,19 @@ flow ( { EitherAmount stepIn (limitStepOut); for (auto i = limitingStep + 1; i < s; ++i) - stepIn = strand[i]->fwd (*sb, *afView, ofrsToRm, stepIn).second; + { + auto const r = strand[i]->fwd (*sb, *afView, ofrsToRm, stepIn); + if (strand[i]->dry (r.second) || + !strand[i]->equalIn (r.first, stepIn)) + { + // The limits should already have been found, so executing a strand forward + // from the limiting step should not find a new limit + JLOG (j.fatal()) << "Re-executed forward pass failed"; + assert (0); + return {telFAILED_PROCESSING, std::move (ofrsToRm)}; + } + stepIn = r.second; + } } auto const strandIn = *strand.front ()->cachedIn (); diff --git a/src/ripple/app/tests/Flow_test.cpp b/src/ripple/app/tests/Flow_test.cpp index 045358d15..0da3a81d2 100644 --- a/src/ripple/app/tests/Flow_test.cpp +++ b/src/ripple/app/tests/Flow_test.cpp @@ -274,6 +274,30 @@ struct Flow_test : public beast::unit_test::suite test (env, EUR, USD.issue (), STPath ({IPE (EUR), IPE (USD), IPE (EUR)}), temBAD_PATH_LOOP); } + + { + // cannot have more than one offer with the same output issue + + using namespace jtx; + Env env (*this, features (featureFlowV2)); + + env.fund (XRP (10000), alice, bob, carol, gw); + env.trust (USD (10000), alice, bob, carol); + env.trust (EUR (10000), alice, bob, carol); + + env (pay (gw, bob, USD (100))); + env (pay (gw, bob, EUR (100))); + + env (offer (bob, XRP (100), USD (100))); + env (offer (bob, USD (100), EUR (100)), txflags (tfPassive)); + env (offer (bob, EUR (100), USD (100)), txflags (tfPassive)); + + // payment path: XRP -> XRP/USD -> USD/EUR -> EUR/USD + env (pay (alice, carol, USD (100)), path (~USD, ~EUR, ~USD), + sendmax (XRP (200)), txflags (tfNoRippleDirect), + ter (temBAD_PATH_LOOP)); + } + { Env env (*this, features(featureFlowV2)); env.fund (XRP (10000), alice, bob, noripple (gw)); diff --git a/src/ripple/app/tests/Offer.test.cpp b/src/ripple/app/tests/Offer.test.cpp index 2d89d5265..9bcc96041 100644 --- a/src/ripple/app/tests/Offer.test.cpp +++ b/src/ripple/app/tests/Offer.test.cpp @@ -41,6 +41,15 @@ class Offer_test : public beast::unit_test::suite return env.current()->info().parentCloseTime.time_since_epoch().count(); } + static auto + xrpMinusFee (jtx::Env const& env, std::int64_t xrpAmount) + -> jtx::PrettyAmount + { + using namespace jtx; + auto feeDrops = env.current ()->fees ().base; + return drops (dropsPerXRP::value * xrpAmount - feeDrops); + }; + public: void testRmFundedOffer () { @@ -344,15 +353,6 @@ public: env (pay (alice, carol, USD2 (50)), path (~USD1, bob), sendmax (XRP (50)), txflags (tfNoRippleDirect)); - auto xrpMinusFee = [](jtx::Env const& env, - std::int64_t xrpAmount) -> jtx::PrettyAmount - { - using namespace jtx; - auto feeDrops = env.current ()->fees ().base; - return drops ( - dropsPerXRP::value * xrpAmount - feeDrops); - }; - env.require (balance (alice, xrpMinusFee (env, 10000 - 50))); env.require (balance (bob, USD1 (100))); env.require (balance (bob, USD2 (0))); diff --git a/src/ripple/app/tx/impl/OfferStream.cpp b/src/ripple/app/tx/impl/OfferStream.cpp index 4fd366e1d..59924b765 100644 --- a/src/ripple/app/tx/impl/OfferStream.cpp +++ b/src/ripple/app/tx/impl/OfferStream.cpp @@ -216,7 +216,7 @@ OfferStream::permRmOffer (std::shared_ptr const& sle) template void FlowOfferStream::permRmOffer (std::shared_ptr const& sle) { - toRemove_.insert (sle->key()); + permToRemove_.insert (sle->key()); } template class FlowOfferStream; diff --git a/src/ripple/app/tx/impl/OfferStream.h b/src/ripple/app/tx/impl/OfferStream.h index 5533383a7..2a28cdfdf 100644 --- a/src/ripple/app/tx/impl/OfferStream.h +++ b/src/ripple/app/tx/impl/OfferStream.h @@ -85,6 +85,7 @@ protected: virtual void permRmOffer (std::shared_ptr const& sle) = 0; + public: TOfferStreamBase (ApplyView& view, ApplyView& cancelView, Book const& book, NetClock::time_point when, @@ -149,11 +150,11 @@ public: The `view_' ` `ApplyView` accumulates changes to the ledger. The `cancelView_` is used to determine if an offer is found unfunded or became unfunded. - The `toRemove` collection identifies offers that should be + The `permToRemove` collection identifies offers that should be removed even if the strand associated with this OfferStream is not applied. - Certain invalid offers are added to the `toRemove` collection: + Certain invalid offers are added to the `permToRemove` collection: - Offers with missing ledger entries - Offers that expired - Offers found unfunded: @@ -165,7 +166,7 @@ template class FlowOfferStream : public TOfferStreamBase { private: - boost::container::flat_set toRemove_; + boost::container::flat_set permToRemove_; protected: void permRmOffer (std::shared_ptr const& sle) override; @@ -173,9 +174,9 @@ protected: public: using TOfferStreamBase::TOfferStreamBase; - boost::container::flat_set const& toRemove () const + boost::container::flat_set const& permToRemove () const { - return toRemove_; + return permToRemove_; }; }; } diff --git a/src/ripple/ledger/ApplyView.h b/src/ripple/ledger/ApplyView.h index a22479853..ffb660676 100644 --- a/src/ripple/ledger/ApplyView.h +++ b/src/ripple/ledger/ApplyView.h @@ -201,11 +201,11 @@ public: // Called when a credit is made to an account // This is required to support PaymentSandbox - virtual - void + virtual void creditHook (AccountID const& from, AccountID const& to, - STAmount const& amount) + STAmount const& amount, + STAmount const& preCreditBalance) { } }; diff --git a/src/ripple/ledger/PaymentSandbox.h b/src/ripple/ledger/PaymentSandbox.h index 6753d6410..04781f53f 100644 --- a/src/ripple/ledger/PaymentSandbox.h +++ b/src/ripple/ledger/PaymentSandbox.h @@ -36,31 +36,38 @@ namespace detail { class DeferredCredits { public: - // Get the adjusted balance of main for the - // balance between main and other. - STAmount - adjustedBalance (AccountID const& main, + struct Adjustment + { + Adjustment (STAmount const& d, STAmount const& c, STAmount const& b) + : debits (d), credits (c), origBalance (b) {} + STAmount debits; + STAmount credits; + STAmount origBalance; + }; + + // Get the adjustments for the balance between main and other. + // Returns the debits, credits and the original balance + boost::optional adjustments ( + AccountID const& main, AccountID const& other, - STAmount const& curBalance) const; + Currency const& currency) const; void credit (AccountID const& sender, - AccountID const& receiver, - STAmount const& amount); + AccountID const& receiver, + STAmount const& amount, + STAmount const& preCreditSenderBalance); void apply (DeferredCredits& to); - - // VFALCO Is this needed? - // DEPRECATED - void clear (); - private: // lowAccount, highAccount using Key = std::tuple< AccountID, AccountID, Currency>; - - // lowAccountCredits, highAccountCredits - using Value = std::tuple< - STAmount, STAmount>; + struct Value + { + STAmount lowAcctCredits; + STAmount highAcctCredits; + STAmount lowAcctOrigBalance; + }; static Key @@ -145,7 +152,8 @@ public: void creditHook (AccountID const& from, AccountID const& to, - STAmount const& amount) override; + STAmount const& amount, + STAmount const& preCreditBalance) override; /** Apply changes to base view. @@ -154,8 +162,7 @@ public: behavior will result. */ /** @{ */ - void - apply (RawView& to); + void apply (RawView& to); void apply (PaymentSandbox& to); diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index ef2b5f4e9..53cc4d9c7 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -328,6 +328,7 @@ transferXRP (ApplyView& view, STAmount const& amount, beast::Journal j); +NetClock::time_point const& flowV2SoTime (); bool flowV2Switchover (NetClock::time_point const closeTime); } // ripple diff --git a/src/ripple/ledger/impl/PaymentSandbox.cpp b/src/ripple/ledger/impl/PaymentSandbox.cpp index 7123b87e1..865cbadfa 100644 --- a/src/ripple/ledger/impl/PaymentSandbox.cpp +++ b/src/ripple/ledger/impl/PaymentSandbox.cpp @@ -19,16 +19,19 @@ #include #include +#include + +#include + #include namespace ripple { namespace detail { -auto -DeferredCredits::makeKey (AccountID const& a1, - AccountID const& a2, Currency const& c) -> - Key +auto DeferredCredits::makeKey (AccountID const& a1, + AccountID const& a2, + Currency const& c) -> Key { if (a1 < a2) return std::make_tuple(a1, a2, c); @@ -36,12 +39,12 @@ DeferredCredits::makeKey (AccountID const& a1, return std::make_tuple(a2, a1, c); } -void DeferredCredits::credit (AccountID const& sender, - AccountID const& receiver, - STAmount const& amount) +void +DeferredCredits::credit (AccountID const& sender, + AccountID const& receiver, + STAmount const& amount, + STAmount const& preCreditSenderBalance) { - using std::get; - assert (sender != receiver); assert (!amount.negative()); @@ -53,52 +56,55 @@ void DeferredCredits::credit (AccountID const& sender, if (sender < receiver) { - get<1> (v) = amount; - get<0> (v) = amount.zeroed (); + v.highAcctCredits = amount; + v.lowAcctCredits = amount.zeroed (); + v.lowAcctOrigBalance = preCreditSenderBalance; } else { - get<1> (v) = amount.zeroed (); - get<0> (v) = amount; + v.highAcctCredits = amount.zeroed (); + v.lowAcctCredits = amount; + v.lowAcctOrigBalance = -preCreditSenderBalance; } map_[k] = v; } else { + // only record the balance the first time, do not record it here auto& v = i->second; if (sender < receiver) - get<1> (v) += amount; + v.highAcctCredits += amount; else - get<0> (v) += amount; + v.lowAcctCredits += amount; } } -// Get the adjusted balance of main for the -// balance between main and other. -STAmount DeferredCredits::adjustedBalance (AccountID const& main, - AccountID const& other, - STAmount const& curBalance) const +// Get the adjustments for the balance between main and other. +auto +DeferredCredits::adjustments (AccountID const& main, + AccountID const& other, + Currency const& currency) const -> boost::optional { - using std::get; - STAmount result (curBalance); + boost::optional result; - Key const k = makeKey (main, other, curBalance.getCurrency ()); + Key const k = makeKey (main, other, currency); auto i = map_.find (k); - if (i != map_.end ()) - { - auto const& v = i->second; - if (main < other) - { - result -= get<0> (v); - } - else - { - result -= get<1> (v); - } - } + if (i == map_.end ()) + return result; - return result; + auto const& v = i->second; + + if (main < other) + { + result.emplace (v.highAcctCredits, v.lowAcctCredits, v.lowAcctOrigBalance); + return result; + } + else + { + result.emplace (v.lowAcctCredits, v.highAcctCredits, -v.lowAcctOrigBalance); + return result; + } } void DeferredCredits::apply( @@ -110,18 +116,15 @@ void DeferredCredits::apply( to.map_.emplace(p); if (! r.second) { - using std::get; - get<0>(r.first->second) += get<0>(p.second); - get<1>(r.first->second) += get<1>(p.second); + auto& toVal = r.first->second; + auto const& fromVal = p.second; + toVal.lowAcctCredits += fromVal.lowAcctCredits; + toVal.highAcctCredits += fromVal.highAcctCredits; + // Do not update the orig balance, it's already correct } } } -void DeferredCredits::clear () -{ - map_.clear (); -} - } // detail STAmount @@ -129,19 +132,57 @@ PaymentSandbox::balanceHook (AccountID const& account, AccountID const& issuer, STAmount const& amount) const { - if (ps_) - return tab_.adjustedBalance ( - account, issuer, ps_->balanceHook (account, issuer, amount)); - return tab_.adjustedBalance( - account, issuer, amount); + /* + There are two algorithms here. The pre-switchover algorithm takes the + current amount and subtracts the recorded credits. The post-switchover + algorithm remembers the original balance, and subtracts the debits. The + post-switchover algorithm should be more numerically stable. Consider a + large credit with a small initial balance. The pre-switchover algorithm + computes (B+C)-C (where B+C will the the amount passed in). The + post-switchover algorithm returns B. When B and C differ by large + magnitudes, (B+C)-C may not equal B. + */ + + auto const currency = amount.getCurrency (); + auto const switchover = flowV2Switchover (info ().parentCloseTime); + + auto adjustedAmt = amount; + if (switchover) + { + auto delta = amount.zeroed (); + auto lastBal = amount; + for (auto curSB = this; curSB; curSB = curSB->ps_) + { + if (auto adj = curSB->tab_.adjustments (account, issuer, currency)) + { + delta += adj->debits; + lastBal = adj->origBalance; + } + } + adjustedAmt = std::min(amount, lastBal - delta); + } + else + { + for (auto curSB = this; curSB; curSB = curSB->ps_) + { + if (auto adj = curSB->tab_.adjustments (account, issuer, currency)) + { + adjustedAmt -= adj->credits; + } + } + } + + assert (!isXRP(issuer) || adjustedAmt >= beast::zero); + return adjustedAmt; } void PaymentSandbox::creditHook (AccountID const& from, AccountID const& to, - STAmount const& amount) + STAmount const& amount, + STAmount const& preCreditBalance) { - tab_.credit(from, to, amount); + tab_.credit (from, to, amount, preCreditBalance); } void diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index e39f88d06..c5621d3c0 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -31,12 +31,17 @@ namespace ripple { -bool flowV2Switchover (NetClock::time_point const closeTime) +NetClock::time_point const& flowV2SoTime () { using namespace std::chrono_literals; // Mon March 28, 2016 10:00:00am PST static NetClock::time_point const soTime{512503200s}; - return closeTime > soTime; + return soTime; +} + +bool flowV2Switchover (NetClock::time_point const closeTime) +{ + return closeTime > flowV2SoTime(); } // VFALCO NOTE A copy of the other one for now @@ -1092,7 +1097,7 @@ trustCreate (ApplyView& view, sleRippleState->setFieldAmount (sfBalance, bSetHigh ? -saBalance : saBalance); view.creditHook (uSrcAccountID, - uDstAccountID, saBalance); + uDstAccountID, saBalance, saBalance.zeroed()); } return terResult; @@ -1239,14 +1244,14 @@ rippleCredit (ApplyView& view, } else { - view.creditHook (uSenderID, - uReceiverID, saAmount); - STAmount saBalance = sleRippleState->getFieldAmount (sfBalance); if (bSenderHigh) saBalance.negate (); // Put balance in sender terms. + view.creditHook (uSenderID, + uReceiverID, saAmount, saBalance); + STAmount saBefore = saBalance; saBalance -= saAmount; @@ -1432,8 +1437,12 @@ accountSend (ApplyView& view, return rippleSend (view, uSenderID, uReceiverID, saAmount, saActual, j); } - view.creditHook (uSenderID, - uReceiverID, saAmount); + auto const fv2Switch = flowV2Switchover (view.info ().parentCloseTime); + if (!fv2Switch) + { + auto const dummyBalance = saAmount.zeroed(); + view.creditHook (uSenderID, uReceiverID, saAmount, dummyBalance); + } /* XRP send which does not check reserve and can do pure adjustment. * Note that sender or receiver may be null and this not a mistake; this @@ -1479,9 +1488,12 @@ accountSend (ApplyView& view, } else { + auto const sndBal = sender->getFieldAmount (sfBalance); + if (fv2Switch) + view.creditHook (uSenderID, xrpAccount (), saAmount, sndBal); + // Decrement XRP balance. - sender->setFieldAmount (sfBalance, - sender->getFieldAmount (sfBalance) - saAmount); + sender->setFieldAmount (sfBalance, sndBal - saAmount); view.update (sender); } } @@ -1489,8 +1501,12 @@ accountSend (ApplyView& view, if (tesSUCCESS == terResult && receiver) { // Increment XRP balance. - receiver->setFieldAmount (sfBalance, - receiver->getFieldAmount (sfBalance) + saAmount); + auto const rcvBal = receiver->getFieldAmount (sfBalance); + receiver->setFieldAmount (sfBalance, rcvBal + saAmount); + + if (fv2Switch) + view.creditHook (xrpAccount (), uReceiverID, saAmount, -rcvBal); + view.update (receiver); } @@ -1619,12 +1635,11 @@ issueIOU (ApplyView& view, auto const must_delete = updateTrustLine(view, state, bSenderHigh, issue.account, start_balance, final_balance, j); + view.creditHook (issue.account, account, amount, start_balance); + if (bSenderHigh) final_balance.negate (); - view.creditHook (issue.account, - account, amount); - // Adjust the balance on the trust line if necessary. We do this even if we // are going to delete the line to reflect the correct balance at the time // of deletion. @@ -1687,11 +1702,11 @@ redeemIOU (ApplyView& view, auto const must_delete = updateTrustLine (view, state, bSenderHigh, account, start_balance, final_balance, j); + view.creditHook (account, issue.account, amount, start_balance); + if (bSenderHigh) final_balance.negate (); - view.creditHook (account, - issue.account, amount); // Adjust the balance on the trust line if necessary. We do this even if we // are going to delete the line to reflect the correct balance at the time diff --git a/src/ripple/ledger/tests/PaymentSandbox_test.cpp b/src/ripple/ledger/tests/PaymentSandbox_test.cpp index 1a9b88b24..328f305f1 100644 --- a/src/ripple/ledger/tests/PaymentSandbox_test.cpp +++ b/src/ripple/ledger/tests/PaymentSandbox_test.cpp @@ -21,6 +21,7 @@ #include #include #include +#include namespace ripple { namespace test { @@ -253,11 +254,49 @@ class PaymentSandbox_test : public beast::unit_test::suite } } + void testTinyBalance () + { + testcase ("Tiny balance"); + + // Add and subtract a huge credit from a tiny balance, expect the tiny + // balance back. Numerical stability problems could cause the balance to + // be zero. + + using namespace jtx; + + Env env (*this); + + Account const gw ("gw"); + Account const alice ("alice"); + auto const USD = gw["USD"]; + + auto const issue = USD.issue (); + STAmount tinyAmt (issue, STAmount::cMinValue, STAmount::cMinOffset + 1, + false, false, STAmount::unchecked{}); + STAmount hugeAmt (issue, STAmount::cMaxValue, STAmount::cMaxOffset - 1, + false, false, STAmount::unchecked{}); + + for (auto timeDelta : {-env.closed ()->info ().closeTimeResolution, + env.closed ()->info ().closeTimeResolution}) + { + auto const closeTime = flowV2SoTime () + timeDelta; + env.close (closeTime); + ApplyViewImpl av (&*env.current (), tapNONE); + PaymentSandbox pv (&av); + pv.creditHook (gw, alice, hugeAmt, -tinyAmt); + if (closeTime > flowV2SoTime ()) + expect (pv.balanceHook (alice, gw, hugeAmt) == tinyAmt); + else + expect (pv.balanceHook (alice, gw, hugeAmt) != tinyAmt); + } + } + public: void run () { testSelfFunding (); testSubtractCredits (); + testTinyBalance (); } };