From 394c7be70443e522ad78e43f5a9cde81e5491861 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Thu, 7 Mar 2013 15:30:30 -0800 Subject: [PATCH 1/2] Fix pathloop detech in RippleCalc. --- src/cpp/ripple/RippleCalc.cpp | 63 ++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/src/cpp/ripple/RippleCalc.cpp b/src/cpp/ripple/RippleCalc.cpp index b214c037c4..641c6808d8 100644 --- a/src/cpp/ripple/RippleCalc.cpp +++ b/src/cpp/ripple/RippleCalc.cpp @@ -427,12 +427,7 @@ void PathState::setExpanded( { const PaymentNode& pnCur = vpnNodes[uNode]; - if (!!pnCur.uAccountID) - { - // Source is a ripple line - nothing(); - } - else if (!umForward.insert(std::make_pair(boost::make_tuple(pnCur.uAccountID, pnCur.uCurrencyID, pnCur.uIssuerID), uNode)).second) + if (!umForward.insert(std::make_pair(boost::make_tuple(pnCur.uAccountID, pnCur.uCurrencyID, pnCur.uIssuerID), uNode)).second) { // Failed to insert. Have a loop. cLog(lsDEBUG) << boost::str(boost::format("PathState: loop detected: %s") @@ -472,7 +467,7 @@ void PathState::setExpanded( // Optimization: // - An XRP output implies an offer node or destination node is next. // - A change in currency implies an offer node. -// - A change in issuer... +// - A change in issuer... void PathState::setCanonical( const PathState& psExpanded ) @@ -830,8 +825,8 @@ TER RippleCalc::calcNodeAdvance( uDirectEnd = Ledger::getQualityNext(uDirectTip); sleDirectDir = lesActive.entryCache(ltDIR_NODE, uDirectTip); - bDirectAdvance = !sleDirectDir; - bDirectDirDirty = true; + bDirectDirDirty = !!sleDirectDir; // Associated vars are dirty, if found it. + bDirectAdvance = !sleDirectDir; // Advance, if didn't find it. Normal not to be unable to lookup firstdirectory. Maybe even skip this lookup. cLog(lsTRACE) << boost::str(boost::format("calcNodeAdvance: Initialize node: uDirectTip=%s uDirectEnd=%s bDirectAdvance=%d") % uDirectTip % uDirectEnd % bDirectAdvance); } @@ -880,6 +875,7 @@ TER RippleCalc::calcNodeAdvance( { if (bFundsDirty) { + // We were called again probably merely to update structure variables. saTakerPays = sleOffer->getFieldAmount(sfTakerPays); saTakerGets = sleOffer->getFieldAmount(sfTakerGets); @@ -898,22 +894,27 @@ TER RippleCalc::calcNodeAdvance( { // Failed to find an entry in directory. - uOfferIndex = 0; - // Do another cur directory iff bMultiQuality if (bMultiQuality) { + // We are allowed to process multiple qualities if this is the only path. cLog(lsTRACE) << boost::str(boost::format("calcNodeAdvance: next quality")); - bDirectAdvance = true; + + bDirectAdvance = true; // Process next quality. } else if (!bReverse) { cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: unreachable: ran out of offers")); assert(false); // Can't run out of offers in forward direction. - terResult = tefEXCEPTION; + terResult = tefEXCEPTION; } else - bEntryAdvance = false; + { + // Ran off end of offers. + + bEntryAdvance = false; // Done. + uOfferIndex = 0; // Report nore more entries. + } } else { @@ -933,17 +934,33 @@ TER RippleCalc::calcNodeAdvance( cLog(lsTRACE) << "calcNodeAdvance: expired offer"; assert(musUnfundedFound.find(uOfferIndex) != musUnfundedFound.end()); // Verify reverse found it too. - bEntryAdvance = true; + // Just skip it. It will be deleted. + // bEntryAdvance = true; // Already set continue; } else if (!saTakerPays.isPositive() || !saTakerGets.isPositive()) { - // Offer is has bad amounts. - cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: NON-POSITIVE: saTakerPays=%s saTakerGets=%s") - % saTakerPays % saTakerGets); + // Offer has bad amounts. Offers should never have a bad amounts. - // assert(musUnfundedFound.find(uOfferIndex) != musUnfundedFound.end()); // Verify reverse found it too. - bEntryAdvance = true; + if (musUnfundedFound.find(uOfferIndex) != musUnfundedFound.end()) + { + // Reverse should have previously put bad offer in list. + // An internal error previously left a bad offer. + cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: REVERSE INTERNAL ERROR: NON-POSITIVE: saTakerPays=%s saTakerGets=%s") + % saTakerPays % saTakerGets); + + // Just skip it. It will be deleted. + // bEntryAdvance = true; // Already set + } + else + { + // An internal error failed to place this in musUnfundedFound. + cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: OFFER INTERNAL ERROR: NON-POSITIVE: saTakerPays=%s saTakerGets=%s") + % saTakerPays % saTakerGets); + + // Don't process at all, things are in an unexpected state for this transactions. + terResult = tefEXCEPTION; + } continue; } @@ -954,6 +971,8 @@ TER RippleCalc::calcNodeAdvance( curIssuerNodeConstIterator itForward = psCur.umForward.find(asLine); const bool bFoundForward = itForward != psCur.umForward.end(); + // Only a allow a source to be used once, in the first node encountered from initial path scan. + // This prevents conflicting uses of the same balance when going reverse vs forward. if (bFoundForward && itForward->second != uNode) { // Temporarily unfunded. Another node uses this source, ignore in this offer. @@ -966,6 +985,8 @@ TER RippleCalc::calcNodeAdvance( curIssuerNodeConstIterator itPast = mumSource.find(asLine); bool bFoundPast = itPast != mumSource.end(); + // Don't allow a source of funding used in previous increments to be reused. + // XXX This seems overly strict. Why??? if (bFoundPast && itPast->second != uNode) { // Temporarily unfunded. Another node uses this source, ignore in this offer. @@ -978,6 +999,8 @@ TER RippleCalc::calcNodeAdvance( curIssuerNodeConstIterator itReverse = psCur.umReverse.find(asLine); bool bFoundReverse = itReverse != psCur.umReverse.end(); + // For this increment, only allow a source to be used once, in the first node encountered from applying offers in + // reverse. if (bFoundReverse && itReverse->second != uNode) { // Temporarily unfunded. Another node uses this source, ignore in this offer. From 15942616e83bcfaff52cd8a84b87bf5a37a5440d Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Thu, 7 Mar 2013 16:40:00 -0800 Subject: [PATCH 2/2] Fix FillOrKill kill handling of found unfunded orders. --- src/cpp/ripple/OfferCreateTransactor.cpp | 36 +++++++------- src/cpp/ripple/OfferCreateTransactor.h | 3 ++ src/cpp/ripple/RippleCalc.cpp | 61 ++++++++++++++---------- src/cpp/ripple/TransactionErr.h | 1 - 4 files changed, 59 insertions(+), 42 deletions(-) diff --git a/src/cpp/ripple/OfferCreateTransactor.cpp b/src/cpp/ripple/OfferCreateTransactor.cpp index 5055e8715a..830d082319 100644 --- a/src/cpp/ripple/OfferCreateTransactor.cpp +++ b/src/cpp/ripple/OfferCreateTransactor.cpp @@ -46,7 +46,6 @@ TER OfferCreateTransactor::takeOffers( const uint160 uTakerGetsAccountID = saTakerGets.getIssuer(); TER terResult = temUNCERTAIN; - boost::unordered_set usOfferUnfundedFound; // Offers found unfunded. boost::unordered_set usOfferUnfundedBecame; // Offers that became unfunded. boost::unordered_set usAccountTouched; // Accounts touched. @@ -153,7 +152,7 @@ TER OfferCreateTransactor::takeOffers( // Would take own offer. Consider old offer expired. Delete it. cLog(lsINFO) << "takeOffers: encountered taker's own old offer"; - usOfferUnfundedFound.insert(uOfferIndex); + usOfferUnfundedBecame.insert(uOfferIndex); } else if (!saOfferGets.isPositive() || !saOfferPays.isPositive()) { @@ -304,20 +303,6 @@ TER OfferCreateTransactor::takeOffers( cLog(lsINFO) << "takeOffers: " << transToken(terResult); - // On storing meta data, delete offers that were found unfunded to prevent encountering them in future. - if (tesSUCCESS == terResult) - { - BOOST_FOREACH(const uint256& uOfferIndex, usOfferUnfundedFound) - { - - cLog(lsINFO) << "takeOffers: found unfunded: " << uOfferIndex.ToString(); - - terResult = mEngine->getNodes().offerDelete(uOfferIndex); - if (tesSUCCESS != terResult) - break; - } - } - if (tesSUCCESS == terResult) { // On success, delete offers that became unfunded. @@ -369,6 +354,9 @@ TER OfferCreateTransactor::doApply() uint64 uOwnerNode; uint64 uBookNode; + LedgerEntrySet& lesActive = mEngine->getNodes(); + LedgerEntrySet lesCheckpoint = lesActive; // Checkpoint with just fees paid. + if (uTxFlags & tfOfferCreateMask) { cLog(lsINFO) << "OfferCreate: Malformed transaction: Invalid flags set."; @@ -500,7 +488,7 @@ TER OfferCreateTransactor::doApply() else if (bFillOrKill && (saTakerPays || saTakerGets)) { // Fill or kill and have leftovers. - terResult = tecKILL; + lesActive.swapWith(lesCheckpoint); // Restore with just fees paid. } else if ( !saTakerPays // Wants nothing more. @@ -599,6 +587,20 @@ TER OfferCreateTransactor::doApply() } } + // On storing meta data, delete offers that were found unfunded to prevent encountering them in future. + if (tesSUCCESS == terResult) + { + BOOST_FOREACH(const uint256& uOfferIndex, usOfferUnfundedFound) + { + + cLog(lsINFO) << "takeOffers: found unfunded: " << uOfferIndex.ToString(); + + terResult = mEngine->getNodes().offerDelete(uOfferIndex); + if (tesSUCCESS != terResult) + break; + } + } + tLog(tesSUCCESS != terResult, lsINFO) << boost::str(boost::format("OfferCreate: final terResult=%s") % transToken(terResult)); if (isTesSuccess(terResult)) diff --git a/src/cpp/ripple/OfferCreateTransactor.h b/src/cpp/ripple/OfferCreateTransactor.h index a5c3a8d3d9..0947c656fd 100644 --- a/src/cpp/ripple/OfferCreateTransactor.h +++ b/src/cpp/ripple/OfferCreateTransactor.h @@ -5,6 +5,7 @@ class OfferCreateTransactor : public Transactor { +protected: TER takeOffers( const bool bOpenLedger, const bool bPassive, @@ -17,6 +18,8 @@ class OfferCreateTransactor : public Transactor STAmount& saTakerGot, bool& bUnfunded); + boost::unordered_set usOfferUnfundedFound; // Offers found unfunded. + public: OfferCreateTransactor(const SerializedTransaction& txn,TransactionEngineParams params, TransactionEngine* engine) : Transactor(txn,params,engine) {} TER doApply(); diff --git a/src/cpp/ripple/RippleCalc.cpp b/src/cpp/ripple/RippleCalc.cpp index 641c6808d8..e297584a4e 100644 --- a/src/cpp/ripple/RippleCalc.cpp +++ b/src/cpp/ripple/RippleCalc.cpp @@ -944,9 +944,8 @@ TER RippleCalc::calcNodeAdvance( if (musUnfundedFound.find(uOfferIndex) != musUnfundedFound.end()) { - // Reverse should have previously put bad offer in list. - // An internal error previously left a bad offer. - cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: REVERSE INTERNAL ERROR: NON-POSITIVE: saTakerPays=%s saTakerGets=%s") + // An internal error, offer was found failed to place this in musUnfundedFound. + cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: PAST INTERNAL ERROR: OFFER NON-POSITIVE: saTakerPays=%s saTakerGets=%s") % saTakerPays % saTakerGets); // Just skip it. It will be deleted. @@ -954,16 +953,19 @@ TER RippleCalc::calcNodeAdvance( } else { - // An internal error failed to place this in musUnfundedFound. - cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: OFFER INTERNAL ERROR: NON-POSITIVE: saTakerPays=%s saTakerGets=%s") + // Reverse should have previously put bad offer in list. + // An internal error previously left a bad offer. + cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: INTERNAL ERROR: OFFER NON-POSITIVE: saTakerPays=%s saTakerGets=%s") % saTakerPays % saTakerGets); - +assert(false); // Don't process at all, things are in an unexpected state for this transactions. terResult = tefEXCEPTION; } continue; } + bEntryAdvance = false; + // Allowed to access source from this node? // XXX This can get called multiple times for same source in a row, caching result would be nice. // XXX Going forward could we fund something with a worse quality which was previously skipped? Might need to check @@ -982,25 +984,12 @@ TER RippleCalc::calcNodeAdvance( continue; } - curIssuerNodeConstIterator itPast = mumSource.find(asLine); - bool bFoundPast = itPast != mumSource.end(); - - // Don't allow a source of funding used in previous increments to be reused. - // XXX This seems overly strict. Why??? - if (bFoundPast && itPast->second != uNode) - { - // Temporarily unfunded. Another node uses this source, ignore in this offer. - cLog(lsTRACE) << "calcNodeAdvance: temporarily unfunded offer (past)"; - - bEntryAdvance = true; - continue; - } - + // This is overly strict. For contributions to past. We should only count source if actually used. curIssuerNodeConstIterator itReverse = psCur.umReverse.find(asLine); bool bFoundReverse = itReverse != psCur.umReverse.end(); - // For this increment, only allow a source to be used once, in the first node encountered from applying offers in - // reverse. + // For this quality increment, only allow a source to be used from a single node, in the first node encountered from applying offers + // in reverse. if (bFoundReverse && itReverse->second != uNode) { // Temporarily unfunded. Another node uses this source, ignore in this offer. @@ -1010,7 +999,23 @@ TER RippleCalc::calcNodeAdvance( continue; } - saOfferFunds = lesActive.accountFunds(uOfrOwnerID, saTakerGets); // Funds left. + curIssuerNodeConstIterator itPast = mumSource.find(asLine); + bool bFoundPast = itPast != mumSource.end(); + + // Determine if used in past. + // XXX Restriction seems like a misunderstanding. + if (bFoundPast && itPast->second != uNode) + { + // Temporarily unfunded. Another node uses this source, ignore in this offer. + cLog(lsTRACE) << "calcNodeAdvance: temporarily unfunded offer (past)"; + + bEntryAdvance = true; + continue; + } + + // Only the current node is allowed to use the source. + + saOfferFunds = lesActive.accountFunds(uOfrOwnerID, saTakerGets); // Funds held. if (!saOfferFunds.isPositive()) { @@ -1019,9 +1024,17 @@ TER RippleCalc::calcNodeAdvance( if (bReverse && !bFoundReverse && !bFoundPast) { - // Never mentioned before: found unfunded. + // Never mentioned before, clearly just: found unfunded. + // That is, even if this offer fails due to fill or kill still do deletions. musUnfundedFound.insert(uOfferIndex); // Mark offer for always deletion. } + else + { + // Moving forward, don't need to check again. + // Or source was used this reverse + // Or source was previously used + // XXX + } // YYY Could verify offer is correct place for unfundeds. bEntryAdvance = true; diff --git a/src/cpp/ripple/TransactionErr.h b/src/cpp/ripple/TransactionErr.h index 05c5c9ad63..a1d9fb6909 100644 --- a/src/cpp/ripple/TransactionErr.h +++ b/src/cpp/ripple/TransactionErr.h @@ -123,7 +123,6 @@ enum TER // aka TransactionEngineResult tecUNFUNDED_OFFER = 103, tecUNFUNDED_PAYMENT = 104, tecFAILED_PROCESSING = 105, - tecKILL = 106, // tesSUCCESS is not retryable. tecDIR_FULL = 121, tecINSUF_RESERVE_LINE = 122, tecINSUF_RESERVE_OFFER = 123,