From 15942616e83bcfaff52cd8a84b87bf5a37a5440d Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Thu, 7 Mar 2013 16:40:00 -0800 Subject: [PATCH] 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,