From 901ccad0cfc7fbb0ca13f7b90482fe6802816afe Mon Sep 17 00:00:00 2001 From: Tom Ritchford Date: Wed, 6 Aug 2014 18:11:37 -0400 Subject: [PATCH] Clarify unfunded offer deletion strategy. --- src/ripple/module/app/paths/RippleCalc.cpp | 52 +++++++++---------- src/ripple/module/app/paths/RippleCalc.h | 9 ++-- src/ripple/module/app/paths/Types.h | 2 + .../module/app/paths/cursor/AdvanceNode.cpp | 20 +++---- 4 files changed, 43 insertions(+), 40 deletions(-) diff --git a/src/ripple/module/app/paths/RippleCalc.cpp b/src/ripple/module/app/paths/RippleCalc.cpp index a0e4ad81c3..50346924b7 100644 --- a/src/ripple/module/app/paths/RippleCalc.cpp +++ b/src/ripple/module/app/paths/RippleCalc.cpp @@ -23,6 +23,21 @@ namespace ripple { namespace path { +namespace { + +TER deleteOffers ( + LedgerEntrySet& activeLedger, OfferSet& offers) +{ + for (auto& o: offers) + { + if (TER r = activeLedger.offerDelete (o)) + return r; + } + return tesSUCCESS; +} + +} // namespace + bool RippleCalc::addPathState(STPath const& path, TER& resultCode) { auto pathState = std::make_shared ( @@ -85,6 +100,8 @@ TER RippleCalc::rippleCalculate () << " saDstAmountReq_:" << saDstAmountReq_; TER resultCode = temUNCERTAIN; + permanentlyUnfundedOffers_.clear (); + mumSource_.clear (); // YYY Might do basic checks on src and dst validity as per doPayment. @@ -131,7 +148,7 @@ TER RippleCalc::rippleCalculate () STAmount::getRate (saDstAmountReq_, saMaxAmountReq_) : 0; // Offers that became unfunded. - std::vector unfundedOffers; + OfferSet unfundedOffersFromBestPaths; int iPass = 0; @@ -261,8 +278,8 @@ TER RippleCalc::rippleCalculate () // Record best pass' offers that became unfunded for deletion on // success. - unfundedOffers.insert ( - unfundedOffers.end (), + + unfundedOffersFromBestPaths.insert ( pathState->unfundedOffers().begin (), pathState->unfundedOffers().end ()); @@ -304,7 +321,7 @@ TER RippleCalc::rippleCalculate () // more. Prepare for next pass. // // Merge best pass' umReverse. - mumSource.insert ( + mumSource_.insert ( pathState->reverse().begin (), pathState->reverse().end ()); } @@ -343,32 +360,11 @@ TER RippleCalc::rippleCalculate () } } - if (deleteUnfundedOffers_) + if (resultCode == tesSUCCESS) { + resultCode = deleteOffers(mActiveLedger, unfundedOffersFromBestPaths); if (resultCode == tesSUCCESS) - { - // Delete offers that became unfunded. - for (auto const& offerIndex: unfundedOffers) - { - if (resultCode == tesSUCCESS) - { - WriteLog (lsDEBUG, RippleCalc) - << "Became unfunded " << to_string (offerIndex); - resultCode = mActiveLedger.offerDelete (offerIndex); - } - } - } - - // Delete found unfunded offers. - for (auto const& offerIndex: unfundedOffers_) - { - if (resultCode == tesSUCCESS) - { - WriteLog (lsDEBUG, RippleCalc) - << "Delete unfunded " << to_string (offerIndex); - resultCode = mActiveLedger.offerDelete (offerIndex); - } - } + resultCode = deleteOffers(mActiveLedger, permanentlyUnfundedOffers_); } // If isOpenLedger, then ledger is not final, can vote no. diff --git a/src/ripple/module/app/paths/RippleCalc.h b/src/ripple/module/app/paths/RippleCalc.h index dedd29b845..8d200683c1 100644 --- a/src/ripple/module/app/paths/RippleCalc.h +++ b/src/ripple/module/app/paths/RippleCalc.h @@ -63,12 +63,15 @@ struct RippleCalc { } - LedgerEntrySet& mActiveLedger; + /** Compute liquidity through these path sets. */ TER rippleCalculate (); /** Add a single PathState. Returns true on success.*/ bool addPathState(STPath const&, TER&); + /** The active ledger. */ + LedgerEntrySet& mActiveLedger; + STAmount const& saDstAmountReq_; STAmount const& saMaxAmountReq_; Account const& uDstAccountID_; @@ -79,13 +82,13 @@ struct RippleCalc // only be used there. // Map of currency, issuer to node index. - AccountIssueToNodeIndex mumSource; + AccountIssueToNodeIndex mumSource_; // If the transaction fails to meet some constraint, still need to delete // unfunded offers. // // Offers that were found unfunded. - hash_set unfundedOffers_; + path::OfferSet permanentlyUnfundedOffers_; // The computed input amount. STAmount actualAmountIn_; diff --git a/src/ripple/module/app/paths/Types.h b/src/ripple/module/app/paths/Types.h index e37ce13aa4..72daf6afc1 100644 --- a/src/ripple/module/app/paths/Types.h +++ b/src/ripple/module/app/paths/Types.h @@ -30,6 +30,8 @@ namespace path { typedef unsigned int NodeIndex; +typedef hash_set OfferSet; + } typedef hash_map AccountIssueToNodeIndex; diff --git a/src/ripple/module/app/paths/cursor/AdvanceNode.cpp b/src/ripple/module/app/paths/cursor/AdvanceNode.cpp index 550a6ef49e..f7290e70fa 100644 --- a/src/ripple/module/app/paths/cursor/AdvanceNode.cpp +++ b/src/ripple/module/app/paths/cursor/AdvanceNode.cpp @@ -216,7 +216,8 @@ TER PathCursor::advanceNode (bool const bReverse) const // Offer is expired. WriteLog (lsTRACE, RippleCalc) << "advanceNode: expired offer"; - rippleCalc_.unfundedOffers_.insert(node().offerIndex_); + rippleCalc_.permanentlyUnfundedOffers_.insert( + node().offerIndex_); continue; } @@ -224,7 +225,7 @@ TER PathCursor::advanceNode (bool const bReverse) const { // Offer has bad amounts. Offers should never have a bad // amounts. - + auto const index = node().offerIndex_; if (bReverse) { // Past internal error, offer had bad amounts. @@ -236,13 +237,14 @@ TER PathCursor::advanceNode (bool const bReverse) const << " node.saTakerGets=%s" << node().saTakerGets; // Mark offer for always deletion. - rippleCalc_.unfundedOffers_.insert (node().offerIndex_); + rippleCalc_.permanentlyUnfundedOffers_.insert ( + node().offerIndex_); } - else if (rippleCalc_.unfundedOffers_.find (node().offerIndex_) - != rippleCalc_.unfundedOffers_.end ()) + else if (rippleCalc_.permanentlyUnfundedOffers_.find (index) + != rippleCalc_.permanentlyUnfundedOffers_.end ()) { // Past internal error, offer was found failed to place - // this in unfundedOffers. + // this in permanentlyUnfundedOffers_. // Just skip it. It will be deleted. WriteLog (lsDEBUG, RippleCalc) << "advanceNode: PAST INTERNAL ERROR " @@ -320,8 +322,8 @@ TER PathCursor::advanceNode (bool const bReverse) const // Determine if used in past. // We only need to know if it might need to be marked unfunded. - auto itPast = rippleCalc_.mumSource.find (accountIssue); - bool bFoundPast = (itPast != rippleCalc_.mumSource.end ()); + auto itPast = rippleCalc_.mumSource_.find (accountIssue); + bool bFoundPast = (itPast != rippleCalc_.mumSource_.end ()); // Only the current node is allowed to use the source. @@ -343,7 +345,7 @@ TER PathCursor::advanceNode (bool const bReverse) const // That is, even if this offer fails due to fill or kill // still do deletions. // Mark offer for always deletion. - rippleCalc_.unfundedOffers_.insert (node().offerIndex_); + rippleCalc_.permanentlyUnfundedOffers_.insert (node().offerIndex_); } else {