From 21c563f83a3a8a1ddefaaa56e284c0e1491a30f7 Mon Sep 17 00:00:00 2001 From: seelabs Date: Sat, 14 May 2016 11:43:39 -0400 Subject: [PATCH] Fix false dry and other payment bugs: The Owner count could decrease while evaluating a strand, causing different behavior in forward passes and reverses passes. The fix treats a decreased owner count like a deferred credit. In some situations, deferred credits could cause an XRP balance to be calculated as negative, triggering some asserts. When XRP is used as a bridge currency, a path could be falsely marked as dry. This happens when the XRP/XXX offer recursively checks the XXX/XRP offer and the XXX/XRP offer could not satisfy the request in a single call. With a single strand and limit quality the old payment code incorrectly computed with multiquailty set to true. This could cause the total quality to go below the requested quality even if there was liquidity available above the requested quality value. --- src/ripple/app/paths/Credit.cpp | 11 - src/ripple/app/paths/Credit.h | 7 - src/ripple/app/paths/RippleCalc.cpp | 9 +- src/ripple/app/paths/cursor/AdvanceNode.cpp | 7 +- .../app/paths/cursor/DeliverNodeForward.cpp | 8 +- .../app/paths/cursor/DeliverNodeReverse.cpp | 21 +- .../app/paths/cursor/ForwardLiquidity.cpp | 3 +- src/ripple/app/paths/cursor/PathCursor.h | 8 +- src/ripple/app/paths/impl/DirectStep.cpp | 6 +- src/ripple/app/tests/Flow_test.cpp | 277 ++++++++++++++++++ src/ripple/ledger/ApplyView.h | 8 + src/ripple/ledger/PaymentSandbox.h | 22 +- src/ripple/ledger/ReadView.h | 20 +- src/ripple/ledger/View.h | 4 + src/ripple/ledger/impl/PaymentSandbox.cpp | 82 +++++- src/ripple/ledger/impl/View.cpp | 133 ++++++--- .../ledger/tests/PaymentSandbox_test.cpp | 42 +++ 17 files changed, 577 insertions(+), 91 deletions(-) diff --git a/src/ripple/app/paths/Credit.cpp b/src/ripple/app/paths/Credit.cpp index ec6539016c..fe7352f559 100644 --- a/src/ripple/app/paths/Credit.cpp +++ b/src/ripple/app/paths/Credit.cpp @@ -83,15 +83,4 @@ STAmount creditBalance ( return result; } -IOUAmount -creditBalance2 ( - ReadView const& v, - AccountID const& acc, - AccountID const& iss, - Currency const& cur) -{ - return toAmount (creditBalance (v, acc, iss, cur)); -} - - } // ripple diff --git a/src/ripple/app/paths/Credit.h b/src/ripple/app/paths/Credit.h index 4c70afebfa..9d1cfc2cfc 100644 --- a/src/ripple/app/paths/Credit.h +++ b/src/ripple/app/paths/Credit.h @@ -60,13 +60,6 @@ STAmount creditBalance ( AccountID const& account, AccountID const& issuer, Currency const& currency); - -IOUAmount -creditBalance2 ( - ReadView const& v, - AccountID const& acc, - AccountID const& iss, - Currency const& cur); /** @} */ } // ripple diff --git a/src/ripple/app/paths/RippleCalc.cpp b/src/ripple/app/paths/RippleCalc.cpp index a433ae9a7a..5db8df17c0 100644 --- a/src/ripple/app/paths/RippleCalc.cpp +++ b/src/ripple/app/paths/RippleCalc.cpp @@ -291,6 +291,7 @@ TER RippleCalc::rippleCalculate () boost::container::flat_set unfundedOffersFromBestPaths; int iPass = 0; + auto const dcSwitch = dcSwitchover(view.info().parentCloseTime); while (resultCode == temUNCERTAIN) { @@ -306,8 +307,12 @@ TER RippleCalc::rippleCalculate () if (pathState->quality()) // Only do active paths. { - // If computing the only non-dry path, compute multi-quality. - multiQuality = ((pathStateList_.size () - iDry) == 1); + // If computing the only non-dry path, and not limiting quality, + // compute multi-quality. + multiQuality = dcSwitch + ? !inputFlags.limitQuality && + ((pathStateList_.size () - iDry) == 1) + : ((pathStateList_.size () - iDry) == 1); // Update to current amount processed. pathState->reset (actualAmountIn_, actualAmountOut_); diff --git a/src/ripple/app/paths/cursor/AdvanceNode.cpp b/src/ripple/app/paths/cursor/AdvanceNode.cpp index 4c46d85f3c..c6f904f9b9 100644 --- a/src/ripple/app/paths/cursor/AdvanceNode.cpp +++ b/src/ripple/app/paths/cursor/AdvanceNode.cpp @@ -20,13 +20,16 @@ #include #include #include +#include namespace ripple { namespace path { -TER PathCursor::advanceNode (STAmount const& amount, bool reverse) const +TER PathCursor::advanceNode (STAmount const& amount, bool reverse, bool callerHasLiquidity) const { - bool multi = multiQuality_ || amount == zero; + bool const multi = dcSwitchover (view ().info ().parentCloseTime) + ? (multiQuality_ || (!callerHasLiquidity && amount == zero)) + : (multiQuality_ || amount == zero); // If the multiQuality_ is unchanged, use the PathCursor we're using now. if (multi == multiQuality_) diff --git a/src/ripple/app/paths/cursor/DeliverNodeForward.cpp b/src/ripple/app/paths/cursor/DeliverNodeForward.cpp index 9976d92868..7e0c16f413 100644 --- a/src/ripple/app/paths/cursor/DeliverNodeForward.cpp +++ b/src/ripple/app/paths/cursor/DeliverNodeForward.cpp @@ -35,7 +35,8 @@ TER PathCursor::deliverNodeForward ( AccountID const& uInAccountID, // --> Input owner's account. STAmount const& saInReq, // --> Amount to deliver. STAmount& saInAct, // <-- Amount delivered, this invocation. - STAmount& saInFees) const // <-- Fees charged, this invocation. + STAmount& saInFees, // <-- Fees charged, this invocation. + bool callerHasLiquidity) const { TER resultCode = tesSUCCESS; @@ -66,7 +67,7 @@ TER PathCursor::deliverNodeForward ( // Determine values for pass to adjust saInAct, saInFees, and // node().saFwdDeliver. - advanceNode (saInAct, false); + advanceNode (saInAct, false, callerHasLiquidity); // If needed, advance to next funded offer. @@ -224,7 +225,8 @@ TER PathCursor::deliverNodeForward ( node().offerOwnerAccount_, // --> Current holder. saOutPassMax, // --> Amount available. saOutPassAct, // <-- Amount delivered. - saOutPassFees); // <-- Fees charged. + saOutPassFees, // <-- Fees charged. + saInAct > zero); if (resultCode != tesSUCCESS) break; diff --git a/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp b/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp index 094693e569..e59aef6bef 100644 --- a/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp +++ b/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp @@ -39,8 +39,9 @@ TER PathCursor::deliverNodeReverseImpl ( AccountID const& uOutAccountID, // --> Output owner's account. STAmount const& saOutReq, // --> Funds requested to be // delivered for an increment. - STAmount& saOutAct // <-- Funds actually delivered for an + STAmount& saOutAct, // <-- Funds actually delivered for an // increment + bool callerHasLiquidity ) const { TER resultCode = tesSUCCESS; @@ -73,7 +74,7 @@ TER PathCursor::deliverNodeReverseImpl ( return telFAILED_PROCESSING; } - resultCode = advanceNode (saOutAct, true); + resultCode = advanceNode (saOutAct, true, callerHasLiquidity); // If needed, advance to next funded offer. if (resultCode != tesSUCCESS || !node().offerIndex_) @@ -251,7 +252,19 @@ TER PathCursor::deliverNodeReverseImpl ( resultCode = increment(-1).deliverNodeReverseImpl( node().offerOwnerAccount_, saInPassReq, - saInPassAct); + saInPassAct, + saOutAct > zero); + + if (dcSwitchover(view().info().parentCloseTime)) + { + // The recursive call is dry this time, but we have liquidity + // from previous calls + if (resultCode == tecPATH_DRY && saOutAct > zero) + { + resultCode = tesSUCCESS; + break; + } + } JLOG (j_.trace()) << "deliverNodeReverse: offer --> OFFER --> ? :" @@ -372,7 +385,7 @@ TER PathCursor::deliverNodeReverse ( for (int i = nodeIndex_; i >= 0 && !node (i).isAccount(); --i) node (i).directory.restart (multiQuality_); - return deliverNodeReverseImpl(uOutAccountID, saOutReq, saOutAct); + return deliverNodeReverseImpl(uOutAccountID, saOutReq, saOutAct, /*callerHasLiquidity*/false); } } // path diff --git a/src/ripple/app/paths/cursor/ForwardLiquidity.cpp b/src/ripple/app/paths/cursor/ForwardLiquidity.cpp index 6ec36e3ae6..be366de3e4 100644 --- a/src/ripple/app/paths/cursor/ForwardLiquidity.cpp +++ b/src/ripple/app/paths/cursor/ForwardLiquidity.cpp @@ -42,7 +42,8 @@ TER PathCursor::forwardLiquidity () const previousNode().account_, previousNode().saFwdDeliver, // Previous is sending this much. saInAct, - saInFees); + saInFees, + false); assert (resultCode != tesSUCCESS || previousNode().saFwdDeliver == saInAct + saInFees); diff --git a/src/ripple/app/paths/cursor/PathCursor.h b/src/ripple/app/paths/cursor/PathCursor.h index 3d133c92c7..2346ce430d 100644 --- a/src/ripple/app/paths/cursor/PathCursor.h +++ b/src/ripple/app/paths/cursor/PathCursor.h @@ -79,7 +79,7 @@ private: <-- uOfferIndex : 0=end of list. */ TER advanceNode (bool reverse) const; - TER advanceNode (STAmount const& amount, bool reverse) const; + TER advanceNode (STAmount const& amount, bool reverse, bool callerHasLiquidity) const; // To deliver from an order book, when computing TER deliverNodeReverse ( @@ -91,13 +91,15 @@ private: TER deliverNodeReverseImpl ( AccountID const& uOutAccountID, STAmount const& saOutReq, - STAmount& saOutAct) const; + STAmount& saOutAct, + bool callerHasLiquidity) const; TER deliverNodeForward ( AccountID const& uInAccountID, STAmount const& saInReq, STAmount& saInAct, - STAmount& saInFees) const; + STAmount& saInFees, + bool callerHasLiquidity) const; // VFALCO TODO Rename this to view() PaymentSandbox& diff --git a/src/ripple/app/paths/impl/DirectStep.cpp b/src/ripple/app/paths/impl/DirectStep.cpp index cf59ae9022..106a8d6da4 100644 --- a/src/ripple/app/paths/impl/DirectStep.cpp +++ b/src/ripple/app/paths/impl/DirectStep.cpp @@ -197,7 +197,8 @@ maxFlow ( AccountID const& dst, Currency const& cur) { - auto const srcOwed = creditBalance2 (sb, dst, src, cur); + auto const srcOwed = toAmount ( + accountHolds (sb, src, cur, dst, fhIGNORE_FREEZE, beast::Journal{})); if (srcOwed.signum () > 0) return {srcOwed, true}; @@ -211,7 +212,8 @@ DirectStepI::redeems (ReadView const& sb, bool fwd) const { if (!fwd) { - auto const srcOwed = creditBalance2 (sb, dst_, src_, currency_); + auto const srcOwed = accountHolds ( + sb, src_, currency_, dst_, fhIGNORE_FREEZE, beast::Journal{}); return srcOwed.signum () > 0; } else diff --git a/src/ripple/app/tests/Flow_test.cpp b/src/ripple/app/tests/Flow_test.cpp index 49887ce7cf..72f32034af 100644 --- a/src/ripple/app/tests/Flow_test.cpp +++ b/src/ripple/app/tests/Flow_test.cpp @@ -901,12 +901,289 @@ struct Flow_test : public beast::unit_test::suite } } + void + testFalseDryHelper (jtx::Env& env) + { + using namespace jtx; + + auto const gw = Account ("gateway"); + auto const USD = gw["USD"]; + auto const EUR = gw["EUR"]; + Account const alice ("alice"); + Account const bob ("bob"); + Account const carol ("carol"); + + auto const closeTime = dcSoTime() + + 100 * env.closed ()->info ().closeTimeResolution; + env.close (closeTime); + + env.fund (XRP(10000), alice, carol, gw); + env.fund (reserve(env, 5), bob); + env.trust (USD (1000), alice, bob, carol); + env.trust (EUR (1000), alice, bob, carol); + + + env (pay (gw, alice, EUR (50))); + env (pay (gw, bob, USD (50))); + + // Bob has _just_ slightly less than 50 xrp available + // If his owner count changes, he will have more liquidity. + // This is one error case to test (when FlowV2 is used). + // Computing the incomming xrp to the XRP/USD offer will require two + // recursive calls to the EUR/XRP offer. The second call will return + // tecPATH_DRY, but the entire path should not be marked as dry. This + // is the second error case to test (when flowV1 is used). + env (offer (bob, EUR (50), XRP (50))); + env (offer (bob, XRP (50), USD (50))); + + env (pay (alice, carol, USD (1000000)), path (~XRP, ~USD), + sendmax (EUR (500)), + txflags (tfNoRippleDirect | tfPartialPayment)); + + auto const carolUSD = env.balance(carol, USD).value(); + expect (carolUSD > USD (0) && carolUSD < USD (50)); + } + + void + testFalseDry () + { + testcase ("falseDryChanges"); + using namespace jtx; + { + Env env (*this, features (featureFlowV2)); + testFalseDryHelper (env); + } + { + Env env (*this); + testFalseDryHelper (env); + } + } + + void + testLimitQuality () + { + // Single path with two offers and limit quality. The quality limit is + // such that the first offer should be taken but the second should not. + // The total amount delivered should be the sum of the two offers and + // sendMax should be more than the first offer. + testcase ("limitQuality"); + using namespace jtx; + + auto const gw = Account ("gateway"); + auto const USD = gw["USD"]; + Account const alice ("alice"); + Account const bob ("bob"); + Account const carol ("carol"); + + auto const timeDelta = Env{*this}.closed ()->info ().closeTimeResolution; + + for(auto const& t :{dcSoTime(), flowV2SoTime()}){ + for(auto const& d: {-timeDelta*100, +timeDelta*100}){ + auto const closeTime = t+d; + Env env (*this); + env.close (closeTime); + + env.fund (XRP(10000), alice, bob, carol, gw); + + env.trust (USD(100), alice, bob, carol); + env (pay (gw, bob, USD (100))); + env (offer (bob, XRP (50), USD (50))); + env (offer (bob, XRP (100), USD (50))); + + auto expectedResult = + closeTime < dcSoTime () ? tecPATH_DRY : tesSUCCESS; + env (pay (alice, carol, USD (100)), path (~USD), sendmax (XRP (100)), + txflags (tfNoRippleDirect | tfPartialPayment | tfLimitQuality), + ter (expectedResult)); + + if (expectedResult == tesSUCCESS) + env.require (balance (carol, USD (50))); + } + } + + } + + // Helper function that returns the reserve on an account based on + // the passed in number of owners. + static XRPAmount reserve(jtx::Env& env, std::uint32_t count) + { + return env.current()->fees().accountReserve (count); + } + + // Helper function that returns the Offers on an account. + static std::vector> + offersOnAccount (jtx::Env& env, jtx::Account account) + { + std::vector> result; + forEachItem (*env.current (), account, + [&env, &result](std::shared_ptr const& sle) + { + if (sle->getType() == ltOFFER) + result.push_back (sle); + }); + return result; + } + + void + testSelfPayment1() + { + testcase ("Self-payment 1"); + + // In this test case the new flow code mis-computes the amount + // of money to move. Fortunately the new code's re-execute + // check catches the problem and throws out the transaction. + // + // The old payment code handles the payment correctly. + using namespace jtx; + + auto const gw1 = Account ("gw1"); + auto const gw2 = Account ("gw2"); + auto const alice = Account ("alice"); + auto const USD = gw1["USD"]; + auto const EUR = gw2["EUR"]; + + Env env (*this, features (featureFlowV2)); + + auto const closeTime = + flowV2SoTime () + 100 * env.closed ()->info ().closeTimeResolution; + env.close (closeTime); + + env.fund (XRP (1000000), gw1, gw2); + env.close (); + + // The fee that's charged for transactions. + auto const f = env.current ()->fees ().base; + + env.fund (reserve (env, 3) + f * 4, alice); + env.close (); + + env (trust (alice, USD (2000))); + env (trust (alice, EUR (2000))); + env.close (); + + env (pay (gw1, alice, USD (1))); + env (pay (gw2, alice, EUR (1000))); + env.close (); + + env (offer (alice, USD (500), EUR (600))); + env.close (); + + env.require (owners (alice, 3)); + env.require (balance (alice, USD (1))); + env.require (balance (alice, EUR (1000))); + + auto aliceOffers = offersOnAccount (env, alice); + expect (aliceOffers.size () == 1); + for (auto const& offerPtr : aliceOffers) + { + auto const offer = *offerPtr; + expect (offer[sfLedgerEntryType] == ltOFFER); + expect (offer[sfTakerGets] == EUR (600)); + expect (offer[sfTakerPays] == USD (500)); + } + + env (pay (alice, alice, EUR (600)), sendmax (USD (500)), + txflags (tfPartialPayment)); + env.close (); + + env.require (owners (alice, 3)); + env.require (balance (alice, USD (1))); + env.require (balance (alice, EUR (1000))); + aliceOffers = offersOnAccount (env, alice); + expect (aliceOffers.size () == 1); + for (auto const& offerPtr : aliceOffers) + { + auto const offer = *offerPtr; + expect (offer[sfLedgerEntryType] == ltOFFER); + expect (offer[sfTakerGets] == EUR (598.8)); + expect (offer[sfTakerPays] == USD (499)); + } + } + + void + testSelfPayment2() + { + testcase ("Self-payment 2"); + + // In this case the difference between the old payment code and + // the new is the values left behind in the offer. Not saying either + // ios ring, they are just different. + using namespace jtx; + + auto const gw1 = Account ("gw1"); + auto const gw2 = Account ("gw2"); + auto const alice = Account ("alice"); + auto const USD = gw1["USD"]; + auto const EUR = gw2["EUR"]; + + Env env (*this, features (featureFlowV2)); + + auto const closeTime = + flowV2SoTime () + 100 * env.closed ()->info ().closeTimeResolution; + env.close (closeTime); + + env.fund (XRP (1000000), gw1, gw2); + env.close (); + + // The fee that's charged for transactions. + auto const f = env.current ()->fees ().base; + + env.fund (reserve (env, 3) + f * 4, alice); + env.close (); + + env (trust (alice, USD (506))); + env (trust (alice, EUR (606))); + env.close (); + + env (pay (gw1, alice, USD (500))); + env (pay (gw2, alice, EUR (600))); + env.close (); + + env (offer (alice, USD (500), EUR (600))); + env.close (); + + env.require (owners (alice, 3)); + env.require (balance (alice, USD (500))); + env.require (balance (alice, EUR (600))); + + auto aliceOffers = offersOnAccount (env, alice); + expect (aliceOffers.size () == 1); + for (auto const& offerPtr : aliceOffers) + { + auto const offer = *offerPtr; + expect (offer[sfLedgerEntryType] == ltOFFER); + expect (offer[sfTakerGets] == EUR (600)); + expect (offer[sfTakerPays] == USD (500)); + } + + env (pay (alice, alice, EUR (60)), sendmax (USD (50)), + txflags (tfPartialPayment)); + env.close (); + + env.require (owners (alice, 3)); + env.require (balance (alice, USD (500))); + env.require (balance (alice, EUR (600))); + aliceOffers = offersOnAccount (env, alice); + expect (aliceOffers.size () == 1); + for (auto const& offerPtr : aliceOffers) + { + auto const offer = *offerPtr; + expect (offer[sfLedgerEntryType] == ltOFFER); + expect (offer[sfTakerGets] == EUR (594)); + expect (offer[sfTakerPays] == USD (495)); + } + } + void run() override { testDirectStep (); testBookStep (); testTransferRate (); testToStrand (); + testFalseDry(); + testLimitQuality(); + testSelfPayment1(); + testSelfPayment2(); } }; diff --git a/src/ripple/ledger/ApplyView.h b/src/ripple/ledger/ApplyView.h index b98b5e98ef..40c83e4b50 100644 --- a/src/ripple/ledger/ApplyView.h +++ b/src/ripple/ledger/ApplyView.h @@ -204,6 +204,14 @@ public: STAmount const& preCreditBalance) { } + + // Called when the owner count changes + // This is required to support PaymentSandbox + virtual + void adjustOwnerCountHook (AccountID const& account, + std::uint32_t cur, std::uint32_t next) + {}; + }; } // ripple diff --git a/src/ripple/ledger/PaymentSandbox.h b/src/ripple/ledger/PaymentSandbox.h index 04781f53f1..04d95e6c24 100644 --- a/src/ripple/ledger/PaymentSandbox.h +++ b/src/ripple/ledger/PaymentSandbox.h @@ -57,6 +57,16 @@ public: STAmount const& amount, STAmount const& preCreditSenderBalance); + void ownerCount (AccountID const& id, + std::uint32_t cur, + std::uint32_t next); + + // Get the adjusted owner count. Since DeferredCredits is meant to be used + // in payments, and payments only decrease owner counts, return the max + // remembered owner count. + boost::optional + ownerCount (AccountID const& id) const; + void apply (DeferredCredits& to); private: // lowAccount, highAccount @@ -75,7 +85,8 @@ private: AccountID const& a2, Currency const& c); - std::map map_; + std::map credits_; + std::map ownerCounts_; }; } // detail @@ -155,6 +166,15 @@ public: STAmount const& amount, STAmount const& preCreditBalance) override; + void + adjustOwnerCountHook (AccountID const& account, + std::uint32_t cur, + std::uint32_t next) override; + + std::uint32_t + ownerCountHook (AccountID const& account, + std::uint32_t count) const override; + /** Apply changes to base view. `to` must contain contents identical to the parent diff --git a/src/ripple/ledger/ReadView.h b/src/ripple/ledger/ReadView.h index e952b6e01a..b313bce1fe 100644 --- a/src/ripple/ledger/ReadView.h +++ b/src/ripple/ledger/ReadView.h @@ -303,8 +303,11 @@ public: std::shared_ptr read (Keylet const& k) const = 0; - // Called to adjust returned balances - // This is required to support PaymentSandbox + // Accounts in a payment are not allowed to use assets acquired during that + // payment. The PaymentSandbox tracks the debits, credits, and owner count + // changes that accounts make during a payment. `balanceHook` adjusts balances + // so newly acquired assets are not counted toward the balance. + // This is required to support PaymentSandbox. virtual STAmount balanceHook (AccountID const& account, @@ -314,6 +317,19 @@ public: return amount; } + // Accounts in a payment are not allowed to use assets acquired during that + // payment. The PaymentSandbox tracks the debits, credits, and owner count + // changes that accounts make during a payment. `ownerCountHook` adjusts the + // ownerCount so it returns the max value of the ownerCount so far. + // This is required to support PaymentSandbox. + virtual + std::uint32_t + ownerCountHook (AccountID const& account, + std::uint32_t count) const + { + return count; + } + // used by the implementation virtual std::unique_ptr diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index 16b45317bd..23b49a5432 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -343,6 +343,10 @@ transferXRP (ApplyView& view, NetClock::time_point const& flowV2SoTime (); bool flowV2Switchover (NetClock::time_point const closeTime); +NetClock::time_point const& dcSoTime (); +bool dcSwitchover (NetClock::time_point const closeTime); + + } // ripple #endif diff --git a/src/ripple/ledger/impl/PaymentSandbox.cpp b/src/ripple/ledger/impl/PaymentSandbox.cpp index 865cbadfa0..fe32a95223 100644 --- a/src/ripple/ledger/impl/PaymentSandbox.cpp +++ b/src/ripple/ledger/impl/PaymentSandbox.cpp @@ -49,8 +49,8 @@ DeferredCredits::credit (AccountID const& sender, assert (!amount.negative()); auto const k = makeKey (sender, receiver, amount.getCurrency ()); - auto i = map_.find (k); - if (i == map_.end ()) + auto i = credits_.find (k); + if (i == credits_.end ()) { Value v; @@ -67,7 +67,7 @@ DeferredCredits::credit (AccountID const& sender, v.lowAcctOrigBalance = -preCreditSenderBalance; } - map_[k] = v; + credits_[k] = v; } else { @@ -80,6 +80,29 @@ DeferredCredits::credit (AccountID const& sender, } } +void +DeferredCredits::ownerCount (AccountID const& id, + std::uint32_t cur, + std::uint32_t next) +{ + auto const v = std::max (cur, next); + auto r = ownerCounts_.emplace (std::make_pair (id, v)); + if (!r.second) + { + auto& mapVal = r.first->second; + mapVal = std::max (v, mapVal); + } +} + +boost::optional +DeferredCredits::ownerCount (AccountID const& id) const +{ + auto i = ownerCounts_.find (id); + if (i != ownerCounts_.end ()) + return i->second; + return boost::none; +} + // Get the adjustments for the balance between main and other. auto DeferredCredits::adjustments (AccountID const& main, @@ -89,8 +112,8 @@ DeferredCredits::adjustments (AccountID const& main, boost::optional result; Key const k = makeKey (main, other, currency); - auto i = map_.find (k); - if (i == map_.end ()) + auto i = credits_.find (k); + if (i == credits_.end ()) return result; auto const& v = i->second; @@ -110,19 +133,29 @@ DeferredCredits::adjustments (AccountID const& main, void DeferredCredits::apply( DeferredCredits& to) { - for (auto& p : map_) + for (auto const& i : credits_) { - auto r = - to.map_.emplace(p); - if (! r.second) + auto r = to.credits_.emplace (i); + if (!r.second) { auto& toVal = r.first->second; - auto const& fromVal = p.second; + auto const& fromVal = i.second; toVal.lowAcctCredits += fromVal.lowAcctCredits; toVal.highAcctCredits += fromVal.highAcctCredits; // Do not update the orig balance, it's already correct } } + + for (auto const& i : ownerCounts_) + { + auto r = to.ownerCounts_.emplace (i); + if (!r.second) + { + auto& toVal = r.first->second; + auto const& fromVal = i.second; + toVal = std::max (toVal, fromVal); + } + } } } // detail @@ -172,10 +205,29 @@ PaymentSandbox::balanceHook (AccountID const& account, } } - assert (!isXRP(issuer) || adjustedAmt >= beast::zero); + if (isXRP(issuer) && adjustedAmt < beast::zero) + // A calculated negative XRP balance is not an error case. Consider a + // payment snippet that credits a large XRP amount and then debits the + // same amount. The credit can't be used but we subtract the debit and + // calculate a negative value. It's not an error case. + adjustedAmt.clear(); + return adjustedAmt; } +std::uint32_t +PaymentSandbox::ownerCountHook (AccountID const& account, + std::uint32_t count) const +{ + std::uint32_t result = count; + for (auto curSB = this; curSB; curSB = curSB->ps_) + { + if (auto adj = curSB->tab_.ownerCount (account)) + result = std::max (result, *adj); + } + return result; +} + void PaymentSandbox::creditHook (AccountID const& from, AccountID const& to, @@ -185,6 +237,14 @@ PaymentSandbox::creditHook (AccountID const& from, tab_.credit (from, to, amount, preCreditBalance); } +void +PaymentSandbox::adjustOwnerCountHook (AccountID const& account, + std::uint32_t cur, + std::uint32_t next) +{ + tab_.ownerCount (account, cur, next); +} + void PaymentSandbox::apply (RawView& to) { diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index 0739a8aa7e..f862334c65 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -34,8 +34,8 @@ namespace ripple { NetClock::time_point const& flowV2SoTime () { using namespace std::chrono_literals; - // Wed May 25, 2016 10:00:00am PDT - static NetClock::time_point const soTime{517510800s}; + // Mon Aug 29, 2016 10:00:00am PDT + static NetClock::time_point const soTime{525805200s}; return soTime; } @@ -44,6 +44,19 @@ bool flowV2Switchover (NetClock::time_point const closeTime) return closeTime > flowV2SoTime(); } +NetClock::time_point const& dcSoTime () +{ + using namespace std::chrono_literals; + // Mon May 23, 2016 10:00:00am PDT + static NetClock::time_point const soTime{517338000s}; + return soTime; +} + +bool dcSwitchover (NetClock::time_point const closeTime) +{ + return closeTime > dcSoTime(); +} + // VFALCO NOTE A copy of the other one for now /** Maximum number of entries in a directory page A change would be protocol-breaking. @@ -121,51 +134,85 @@ accountHolds (ReadView const& view, if (isXRP(currency)) { // XRP: return balance minus reserve - auto const sle = view.read( - keylet::account(account)); - auto const reserve = - view.fees().accountReserve( - sle->getFieldU32(sfOwnerCount)); - auto const balance = - sle->getFieldAmount(sfBalance).xrp (); - if (balance < reserve) - amount.clear (); + if (dcSwitchover (view.info ().parentCloseTime)) + { + auto const sle = view.read( + keylet::account(account)); + auto const ownerCount = + view.ownerCountHook (account, sle->getFieldU32 (sfOwnerCount)); + auto const reserve = + view.fees().accountReserve(ownerCount); + + auto const fullBalance = + sle->getFieldAmount(sfBalance); + + auto const balance = view.balanceHook( + account, issuer, fullBalance).xrp(); + + if (balance < reserve) + amount.clear (); + else + amount = balance - reserve; + + JLOG (j.trace()) << "accountHolds:" << + " account=" << to_string (account) << + " amount=" << amount.getFullText () << + " fullBalance=" << to_string (fullBalance.xrp()) << + " balance=" << to_string (balance) << + " reserve=" << to_string (reserve); + + return amount; + } else - amount = balance - reserve; - JLOG (j.trace()) << "accountHolds:" << - " account=" << to_string (account) << - " amount=" << amount.getFullText () << - " balance=" << to_string (balance) << - " reserve=" << to_string (reserve); + { + // pre-switchover + // XRP: return balance minus reserve + auto const sle = view.read( + keylet::account(account)); + auto const reserve = + view.fees().accountReserve( + sle->getFieldU32(sfOwnerCount)); + auto const balance = + sle->getFieldAmount(sfBalance).xrp (); + if (balance < reserve) + amount.clear (); + else + amount = balance - reserve; + JLOG (j.trace()) << "accountHolds:" << + " account=" << to_string (account) << + " amount=" << amount.getFullText () << + " balance=" << to_string (balance) << + " reserve=" << to_string (reserve); + return view.balanceHook( + account, issuer, amount); + } + } + + // IOU: Return balance on trust line modulo freeze + auto const sle = view.read(keylet::line( + account, issuer, currency)); + if (! sle) + { + amount.clear ({currency, issuer}); + } + else if ((zeroIfFrozen == fhZERO_IF_FROZEN) && + isFrozen(view, account, currency, issuer)) + { + amount.clear (Issue (currency, issuer)); } else { - // IOU: Return balance on trust line modulo freeze - auto const sle = view.read(keylet::line( - account, issuer, currency)); - if (! sle) + amount = sle->getFieldAmount (sfBalance); + if (account > issuer) { - amount.clear ({currency, issuer}); + // Put balance in account terms. + amount.negate(); } - else if ((zeroIfFrozen == fhZERO_IF_FROZEN) && - isFrozen(view, account, currency, issuer)) - { - amount.clear (Issue (currency, issuer)); - } - else - { - amount = sle->getFieldAmount (sfBalance); - if (account > issuer) - { - // Put balance in account terms. - amount.negate(); - } - amount.setIssuer (issuer); - } - JLOG (j.trace()) << "accountHolds:" << - " account=" << to_string (account) << - " amount=" << amount.getFullText (); + amount.setIssuer (issuer); } + JLOG (j.trace()) << "accountHolds:" << + " account=" << to_string (account) << + " amount=" << amount.getFullText (); return view.balanceHook( account, issuer, amount); @@ -609,13 +656,14 @@ adjustOwnerCount (ApplyView& view, auto const current = sle->getFieldU32 (sfOwnerCount); auto adjusted = current + amount; + AccountID const id = (*sle)[sfAccount]; if (amount > 0) { // Overflow is well defined on unsigned if (adjusted < current) { JLOG (j.fatal()) << - "Account " << sle->getAccountID(sfAccount) << + "Account " << id << " owner count exceeds max!"; adjusted = std::numeric_limits::max (); @@ -627,12 +675,13 @@ adjustOwnerCount (ApplyView& view, if (adjusted > current) { JLOG (j.fatal()) << - "Account " << sle->getAccountID (sfAccount) << + "Account " << id << " owner count set below 0!"; adjusted = 0; assert(false); } } + view.adjustOwnerCountHook (id, current, adjusted); sle->setFieldU32 (sfOwnerCount, adjusted); view.update(sle); } diff --git a/src/ripple/ledger/tests/PaymentSandbox_test.cpp b/src/ripple/ledger/tests/PaymentSandbox_test.cpp index 328f305f1a..abbbf4ceb5 100644 --- a/src/ripple/ledger/tests/PaymentSandbox_test.cpp +++ b/src/ripple/ledger/tests/PaymentSandbox_test.cpp @@ -22,6 +22,7 @@ #include #include #include +#include namespace ripple { namespace test { @@ -290,6 +291,46 @@ class PaymentSandbox_test : public beast::unit_test::suite expect (pv.balanceHook (alice, gw, hugeAmt) != tinyAmt); } } + void testReserve() + { + testcase ("Reserve"); + using namespace jtx; + + beast::Journal dj; + + auto accountFundsXRP = [&dj]( + ReadView const& view, AccountID const& id) -> XRPAmount + { + return toAmount (accountHolds ( + view, id, xrpCurrency (), xrpAccount (), fhZERO_IF_FROZEN, dj)); + }; + + auto reserve = [](jtx::Env& env, std::uint32_t count) -> XRPAmount + { + return env.current ()->fees ().accountReserve (count); + }; + + Env env (*this); + + Account const alice ("alice"); + env.fund (reserve(env, 1), alice); + + auto const closeTime = dcSoTime () + + 100 * env.closed ()->info ().closeTimeResolution; + env.close (closeTime); + ApplyViewImpl av (&*env.current (), tapNONE); + PaymentSandbox sb (&av); + { + // Send alice an amount and spend it. The deferredCredits will cause her balance + // to drop below the reserve. Make sure her funds are zero (there was a bug that + // caused her funds to become negative). + + accountSend (sb, xrpAccount (), alice, XRP(100), dj); + accountSend (sb, alice, xrpAccount (), XRP(100), dj); + expect (accountFundsXRP (sb, alice) == beast::zero); + } + + } public: void run () @@ -297,6 +338,7 @@ public: testSelfFunding (); testSubtractCredits (); testTinyBalance (); + testReserve(); } };