Merge branch 'master' of github.com:jedmccaleb/NewCoin

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

@@ -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,20 +934,38 @@ 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())
+				{
+					// 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.
+					// bEntryAdvance	= true;		// Already set
+				}
+				else
+				{
+					// 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
@@ -954,6 +973,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.
@@ -963,21 +984,12 @@ TER RippleCalc::calcNodeAdvance(
 				continue;
 			}
 
-			curIssuerNodeConstIterator	itPast			= mumSource.find(asLine);
-			bool						bFoundPast		= itPast != mumSource.end();
-
-			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 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.
@@ -987,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())
 			{
@@ -996,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,