From 2d6e33f0b6d2d5b44b903ed561e1314ff83bdf85 Mon Sep 17 00:00:00 2001 From: jed Date: Thu, 28 Feb 2013 13:57:06 -0800 Subject: [PATCH 1/4] ? --- src/cpp/ripple/RippleCalc.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cpp/ripple/RippleCalc.cpp b/src/cpp/ripple/RippleCalc.cpp index 5bf438bbb..ddeba3080 100644 --- a/src/cpp/ripple/RippleCalc.cpp +++ b/src/cpp/ripple/RippleCalc.cpp @@ -536,8 +536,7 @@ void PathState::setCanonical( } else { - // Node 1 must be an account - + // Node 1 must be an account } } else From 45a6c5938cffc7eb7261af892c478d6f10623a2f Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Thu, 7 Mar 2013 18:21:23 -0800 Subject: [PATCH 2/4] Fix RippleCalc on offers with bad amounts. --- src/cpp/ripple/RippleCalc.cpp | 51 ++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/src/cpp/ripple/RippleCalc.cpp b/src/cpp/ripple/RippleCalc.cpp index 7fe6d8b4b..eb67a4187 100644 --- a/src/cpp/ripple/RippleCalc.cpp +++ b/src/cpp/ripple/RippleCalc.cpp @@ -942,14 +942,25 @@ TER RippleCalc::calcNodeAdvance( { // Offer has bad amounts. Offers should never have a bad amounts. - if (musUnfundedFound.find(uOfferIndex) != musUnfundedFound.end()) + if (bReverse) { - // An internal error, offer was found failed to place this in musUnfundedFound. + // Past internal error, offer had bad amounts. cLog(lsWARNING) << boost::str(boost::format("calcNodeAdvance: PAST INTERNAL ERROR: OFFER NON-POSITIVE: saTakerPays=%s saTakerGets=%s") % saTakerPays % saTakerGets); + musUnfundedFound.insert(uOfferIndex); // Mark offer for always deletion. + // bEntryAdvance = true; // Already set + continue; + } + else if (musUnfundedFound.find(uOfferIndex) != musUnfundedFound.end()) + { + // Past internal error, offer was found failed to place this in musUnfundedFound. + cLog(lsDEBUG) << boost::str(boost::format("calcNodeAdvance: PAST INTERNAL ERROR: OFFER NON-POSITIVE: saTakerPays=%s saTakerGets=%s") + % saTakerPays % saTakerGets); +assert(false); // Just skip it. It will be deleted. // bEntryAdvance = true; // Already set + continue; } else { @@ -957,15 +968,13 @@ TER RippleCalc::calcNodeAdvance( // 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); +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 @@ -979,8 +988,6 @@ TER RippleCalc::calcNodeAdvance( { // Temporarily unfunded. Another node uses this source, ignore in this offer. cLog(lsTRACE) << "calcNodeAdvance: temporarily unfunded offer (forward)"; - - bEntryAdvance = true; continue; } @@ -994,24 +1001,21 @@ TER RippleCalc::calcNodeAdvance( { // Temporarily unfunded. Another node uses this source, ignore in this offer. cLog(lsTRACE) << "calcNodeAdvance: temporarily unfunded offer (reverse)"; - - bEntryAdvance = true; continue; } + // Determine if used in past. + // We only need to know if it might need to be marked unfunded. 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; - } + // XXX Restricting by node 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)"; + // continue; + // } // Only the current node is allowed to use the source. @@ -1030,20 +1034,17 @@ TER RippleCalc::calcNodeAdvance( } else { - // Moving forward, don't need to check again. - // Or source was used this reverse - // Or source was previously used - // XXX + // Moving forward, don't need to insert again + // Or, already found it. } // YYY Could verify offer is correct place for unfundeds. - bEntryAdvance = true; continue; } if (bReverse // Need to remember reverse mention. && !bFoundPast // Not mentioned in previous passes. - && !bFoundReverse) // Not mentioned for pass. + && !bFoundReverse) // New to pass. { // Consider source mentioned by current path state. cLog(lsTRACE) << boost::str(boost::format("calcNodeAdvance: remember=%s/%s/%s") From 5e6f399b9decda15473e7b1d7b3459ed24293761 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Thu, 7 Mar 2013 19:14:54 -0800 Subject: [PATCH 3/4] Unfund offers with microscopic amounts. --- src/cpp/ripple/RippleCalc.cpp | 86 ++++++++++++++++++++++++++++------- 1 file changed, 69 insertions(+), 17 deletions(-) diff --git a/src/cpp/ripple/RippleCalc.cpp b/src/cpp/ripple/RippleCalc.cpp index eb67a4187..ae9be13c0 100644 --- a/src/cpp/ripple/RippleCalc.cpp +++ b/src/cpp/ripple/RippleCalc.cpp @@ -606,7 +606,7 @@ void PathState::setCanonical( && pnPrv.uCurrencyID != pnNxt.uCurrencyID) { // Offer can be implied by currency change. -// XXX What abount issuer? +// XXX What about issuer? bSkip = true; } @@ -935,7 +935,6 @@ TER RippleCalc::calcNodeAdvance( assert(musUnfundedFound.find(uOfferIndex) != musUnfundedFound.end()); // Verify reverse found it too. // Just skip it. It will be deleted. - // bEntryAdvance = true; // Already set continue; } else if (!saTakerPays.isPositive() || !saTakerGets.isPositive()) @@ -949,7 +948,6 @@ TER RippleCalc::calcNodeAdvance( % saTakerPays % saTakerGets); musUnfundedFound.insert(uOfferIndex); // Mark offer for always deletion. - // bEntryAdvance = true; // Already set continue; } else if (musUnfundedFound.find(uOfferIndex) != musUnfundedFound.end()) @@ -957,9 +955,8 @@ TER RippleCalc::calcNodeAdvance( // Past internal error, offer was found failed to place this in musUnfundedFound. cLog(lsDEBUG) << boost::str(boost::format("calcNodeAdvance: PAST INTERNAL ERROR: OFFER NON-POSITIVE: saTakerPays=%s saTakerGets=%s") % saTakerPays % saTakerGets); -assert(false); + // Just skip it. It will be deleted. - // bEntryAdvance = true; // Already set continue; } else @@ -968,7 +965,7 @@ assert(false); // 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; } @@ -1326,6 +1323,7 @@ TER RippleCalc::calcNodeDeliverFwd( const uint160& uNxtAccountID = pnNxt.uAccountID; const uint160& uCurCurrencyID = pnCur.uCurrencyID; const uint160& uCurIssuerID = pnCur.uIssuerID; + const uint256& uOfferIndex = pnCur.uOfferIndex; const uint160& uPrvCurrencyID = pnPrv.uCurrencyID; const uint160& uPrvIssuerID = pnPrv.uIssuerID; const STAmount& saInTransRate = pnPrv.saTransferRate; @@ -1345,14 +1343,23 @@ TER RippleCalc::calcNodeDeliverFwd( { if (++loopCount > 40) { - cLog(lsWARNING) << "max loops cndf"; + cLog(lsWARNING) << "calcNodeDeliverFwd: max loops cndf"; return mOpenLedger ? telFAILED_PROCESSING : tecFAILED_PROCESSING; } // Determine values for pass to adjust saInAct, saInFees, and saCurDeliverAct terResult = calcNodeAdvance(uNode, psCur, bMultiQuality, false); // If needed, advance to next funded offer. - if (tesSUCCESS == terResult) + if (tesSUCCESS != terResult) + { + nothing(); + } + else if (!uOfferIndex) + { + cLog(lsWARNING) << "calcNodeDeliverFwd: INTERNAL ERROR: Ran out of offers."; + return mOpenLedger ? telFAILED_PROCESSING : tecFAILED_PROCESSING; + } + else if (tesSUCCESS == terResult) { // Doesn't charge input. Input funds are in limbo. bool& bEntryAdvance = pnCur.bEntryAdvance; @@ -1386,9 +1393,11 @@ TER RippleCalc::calcNodeDeliverFwd( STAmount saInPassFees; // Will be determined by adjusted saInPassAct. - cLog(lsINFO) << boost::str(boost::format("calcNodeDeliverFwd: uNode=%d saOutFunded=%s saInReq=%s saInAct=%s saInFees=%s saInFunded=%s saInTotal=%s saInSum=%s saInPassAct=%s saOutPassMax=%s") + cLog(lsINFO) << boost::str(boost::format("calcNodeDeliverFwd: uNode=%d saOutFunded=%s saOfferFunds=%s saTakerGets=%s saInReq=%s saInAct=%s saInFees=%s saInFunded=%s saInTotal=%s saInSum=%s saInPassAct=%s saOutPassMax=%s") % uNode % saOutFunded + % saOfferFunds + % saTakerGets % saInReq % saInAct % saInFees @@ -1398,7 +1407,26 @@ TER RippleCalc::calcNodeDeliverFwd( % saInPassAct % saOutPassMax); - if (!!uNxtAccountID) + if (!saInSum) + { + cLog(lsINFO) << "calcNodeDeliverFwd: Microscopic offer unfunded."; + + // After math offer is effectively unfunded. + psCur.vUnfundedBecame.push_back(uOfferIndex); + bEntryAdvance = true; + continue; + } + else if (!saInFunded) + { + // Previous check should catch this. + cLog(lsWARNING) << "calcNodeDeliverFwd: UNREACHABLE REACHED"; + + // After math offer is effectively unfunded. + psCur.vUnfundedBecame.push_back(uOfferIndex); + bEntryAdvance = true; + continue; + } + else if (!!uNxtAccountID) { // ? --> OFFER --> account // Input fees: vary based upon the consumed offer's owner. @@ -2191,13 +2219,21 @@ TER RippleCalc::calcNodeAccountFwd( STAmount saIssueCrd = uQualityIn >= QUALITY_ONE ? saPrvIssueReq // No fee. - : STAmount::multiply(saPrvIssueReq, uQualityIn, uCurrencyID, saPrvIssueReq.getIssuer()); // Fee. + : STAmount::multiply(saPrvIssueReq, STAmount(CURRENCY_ONE, ACCOUNT_ONE, uQualityIn, -9)); // Amount to credit. - // Amount to credit. + // Amount to credit. Credit for less than received as a surcharge. saCurReceive = saPrvRedeemReq+saIssueCrd; - // Actually receive. - terResult = lesActive.rippleCredit(uPrvAccountID, uCurAccountID, saPrvRedeemReq+saPrvIssueReq, false); + if (saCurReceive) + { + // Actually receive. + terResult = lesActive.rippleCredit(uPrvAccountID, uCurAccountID, saPrvRedeemReq+saPrvIssueReq, false); + } + else + { + // After applying quality, total payment was microscopic. + terResult = tecPATH_DRY; + } } else { @@ -2239,8 +2275,12 @@ TER RippleCalc::calcNodeAccountFwd( calcNodeRipple(uQualityIn, QUALITY_ONE, saPrvIssueReq, saCurIssueReq, saPrvIssueAct, saCurIssueAct, uRateMax); } + STAmount saProvide = saCurRedeemAct + saCurIssueAct; + // Adjust prv --> cur balance : take all inbound - terResult = lesActive.rippleCredit(uPrvAccountID, uCurAccountID, saPrvRedeemReq + saPrvIssueReq, false); + terResult = saProvide + ? lesActive.rippleCredit(uPrvAccountID, uCurAccountID, saPrvRedeemReq + saPrvIssueReq, false) + : tecPATH_DRY; } } else if (bPrvAccount && !bNxtAccount) @@ -2276,7 +2316,9 @@ TER RippleCalc::calcNodeAccountFwd( } // Adjust prv --> cur balance : take all inbound - terResult = lesActive.rippleCredit(uPrvAccountID, uCurAccountID, saPrvRedeemReq + saPrvIssueReq, false); + terResult = saCurDeliverAct + ? lesActive.rippleCredit(uPrvAccountID, uCurAccountID, saPrvRedeemReq + saPrvIssueReq, false) + : tecPATH_DRY; // Didn't actually deliver anything. } else { @@ -2296,7 +2338,11 @@ TER RippleCalc::calcNodeAccountFwd( } saCurSendMaxPass = saCurDeliverAct; // Record amount sent for pass. - if (!!uCurrencyID) + if (!saCurDeliverAct) + { + terResult = tecPATH_DRY; + } + else if (!!uCurrencyID) { // Non-XRP, current node is the issuer. // We could be delivering to multiple accounts, so we don't know which ripple balance will be adjusted. Assume @@ -2357,6 +2403,10 @@ TER RippleCalc::calcNodeAccountFwd( } // No income balance adjustments necessary. The paying side inside the offer paid and the next link will receive. + STAmount saProvide = saCurRedeemAct + saCurIssueAct; + + if (!saProvide) + terResult = tecPATH_DRY; } } else @@ -2375,6 +2425,8 @@ TER RippleCalc::calcNodeAccountFwd( } // No income balance adjustments necessary. The paying side inside the offer paid and the next link will receive. + if (!saCurDeliverAct) + terResult = tecPATH_DRY; } return terResult; From 78b31c15c0a2b09bbe4acd1858864c636a4694f7 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Fri, 8 Mar 2013 10:30:18 -0800 Subject: [PATCH 4/4] Cosmetic. --- src/cpp/ripple/RippleCalc.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/cpp/ripple/RippleCalc.cpp b/src/cpp/ripple/RippleCalc.cpp index ae9be13c0..2f7096cbb 100644 --- a/src/cpp/ripple/RippleCalc.cpp +++ b/src/cpp/ripple/RippleCalc.cpp @@ -1006,14 +1006,6 @@ TER RippleCalc::calcNodeAdvance( curIssuerNodeConstIterator itPast = mumSource.find(asLine); bool bFoundPast = itPast != mumSource.end(); - // XXX Restricting by node 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)"; - // continue; - // } - // Only the current node is allowed to use the source. saOfferFunds = lesActive.accountFunds(uOfrOwnerID, saTakerGets); // Funds held. @@ -1643,7 +1635,6 @@ TER RippleCalc::calcNodeOfferFwd( // - prv is the driver: it calculates current deliver based on previous delivery limits and current wants. // This routine is called one or two times for a node in a pass. If called once, it will work and set a rate. If called again, // the new work must not worsen the previous rate. -// XXX Deal with uQualityIn or uQualityOut = 0 void RippleCalc::calcNodeRipple( const uint32 uQualityIn, const uint32 uQualityOut,