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(); } };