Fix FillOrKill kill handling of found unfunded orders.

src/cpp/ripple/OfferCreateTransactor.cpp

@@ -46,7 +46,6 @@ TER OfferCreateTransactor::takeOffers(
 	const uint160			uTakerGetsAccountID	= saTakerGets.getIssuer();
 	TER						terResult			= temUNCERTAIN;
 
-	boost::unordered_set<uint256>	usOfferUnfundedFound;	// Offers found unfunded.
 	boost::unordered_set<uint256>	usOfferUnfundedBecame;	// Offers that became unfunded.
 	boost::unordered_set<uint160>	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))

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<uint256>	usOfferUnfundedFound;	// Offers found unfunded.
+
 public:
 	OfferCreateTransactor(const SerializedTransaction& txn,TransactionEngineParams params, TransactionEngine* engine) : Transactor(txn,params,engine) {}
 	TER doApply();

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, offer was found failed to place this in musUnfundedFound.
-					// An internal error previously left a bad offer.
+					cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: PAST INTERNAL ERROR: OFFER NON-POSITIVE: saTakerPays=%s saTakerGets=%s")
-					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.
@@ -954,16 +953,19 @@ TER RippleCalc::calcNodeAdvance(
 				}
 				else
 				{
-					// An internal error failed to place this in musUnfundedFound.
+					// Reverse should have previously put bad offer in list.
-					cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: OFFER INTERNAL ERROR: NON-POSITIVE: saTakerPays=%s saTakerGets=%s")
+					// 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);
+			// This is overly strict. For contributions to past. We should only count source if actually used.
-			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;
-			}
-
 			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
+			// For this quality increment, only allow a source to be used from a single node, in the first node encountered from applying offers
-			// reverse.
+			// 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;

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,