From 0d8ad928f618d8a3298e91fa8ac58522942ca3a8 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Mon, 27 Aug 2012 13:20:47 -0700 Subject: [PATCH 1/2] Simplify clean up for takeOffers. --- src/TransactionEngine.cpp | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/src/TransactionEngine.cpp b/src/TransactionEngine.cpp index 7cf1460c99..694892fae0 100644 --- a/src/TransactionEngine.cpp +++ b/src/TransactionEngine.cpp @@ -4184,24 +4184,14 @@ TransactionEngineResult TransactionEngine::takeOffers( } // On storing meta data, delete offers that were found unfunded to prevent encountering them in future. - switch (terResult) + if (tesSUCCESS == terResult) { - case tesSUCCESS: - case tepPATH_DRY: - case tepPATH_PARTIAL: - BOOST_FOREACH(const uint256& uOfferIndex, usOfferUnfundedFound) - { - TransactionEngineResult terDelete = offerDelete(uOfferIndex); - - if (tesSUCCESS != terDelete) - terResult = terDelete; - break; - } - break; - - default: - nothing(); - break; + BOOST_FOREACH(const uint256& uOfferIndex, usOfferUnfundedFound) + { + terResult = offerDelete(uOfferIndex); + if (tesSUCCESS != terResult) + break; + } } if (tesSUCCESS == terResult) @@ -4209,10 +4199,8 @@ TransactionEngineResult TransactionEngine::takeOffers( // On success, delete offers that became unfunded. BOOST_FOREACH(const uint256& uOfferIndex, usOfferUnfundedBecame) { - TransactionEngineResult terDelete = offerDelete(uOfferIndex); - - if (tesSUCCESS != terDelete) - terResult = terDelete; + terResult = offerDelete(uOfferIndex); + if (tesSUCCESS != terResult) break; } } From 354c33f71a42716e2f3b2513cf41ce5d89596489 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Tue, 28 Aug 2012 13:27:54 -0700 Subject: [PATCH 2/2] Work toward ripple loop detection and offer deletion. --- src/TransactionEngine.cpp | 126 ++++++++++++++++++++++++++++---------- src/TransactionEngine.h | 34 ++++++---- 2 files changed, 117 insertions(+), 43 deletions(-) diff --git a/src/TransactionEngine.cpp b/src/TransactionEngine.cpp index 694892fae0..df8a0c7082 100644 --- a/src/TransactionEngine.cpp +++ b/src/TransactionEngine.cpp @@ -24,6 +24,17 @@ static STAmount saZero(CURRENCY_ONE, 0, 0); static STAmount saOne(CURRENCY_ONE, 1, 0); +std::size_t hash_value(const aciSource& asValue) +{ + std::size_t seed = 0; + + asValue.get<0>().hash_combine(seed); + asValue.get<1>().hash_combine(seed); + asValue.get<2>().hash_combine(seed); + + return seed; +} + bool transResultInfo(TransactionEngineResult terCode, std::string& strToken, std::string& strHuman) { static struct { @@ -1887,11 +1898,10 @@ void TransactionEngine::calcOfferBridgeNext( #endif // <-- bSuccess: false= no transfer -// XXX Make sure missing ripple path is addressed cleanly. bool TransactionEngine::calcNodeOfferRev( - unsigned int uIndex, // 0 < uIndex < uLast - PathState::pointer pspCur, - bool bMultiQuality) + const unsigned int uIndex, // 0 < uIndex < uLast + const PathState::pointer& pspCur, + const bool bMultiQuality) { bool bSuccess = false; @@ -1950,9 +1960,9 @@ bool TransactionEngine::calcNodeOfferRev( { // Do a directory. // - Drive on computing saCurDlvAct to derive saPrvDlvAct. - // XXX Behave well, if entry type is wrong (someone beat us to using the hash) + // XXX Behave well, if entry type is not ltDIR_NODE (someone beat us to using the hash) SLE::pointer sleDirectDir = entryCache(ltDIR_NODE, uDirectTip); - STAmount saOfrRate = STAmount::setRate(Ledger::getQuality(uDirectTip)); // For correct ratio + const STAmount saOfrRate = STAmount::setRate(Ledger::getQuality(uDirectTip)); // For correct ratio unsigned int uEntry = 0; uint256 uCurIndex; @@ -1962,28 +1972,55 @@ bool TransactionEngine::calcNodeOfferRev( Log(lsINFO) << boost::str(boost::format("calcNodeOfferRev: uCurIndex=%s") % uCurIndex.ToString()); SLE::pointer sleCurOfr = entryCache(ltOFFER, uCurIndex); - uint160 uCurOfrAccountID = sleCurOfr->getIValueFieldAccount(sfAccount).getAccountID(); + + if (sleCurOfr->getIFieldPresent(sfExpiration) && sleCurOfr->getIFieldU32(sfExpiration) <= mLedger->getParentCloseTimeNC()) + { + // Offer is expired. + Log(lsINFO) << "calcNodeOfferRev: encountered expired offer"; + + musUnfundedFound.insert(uCurIndex); // Mark offer for always deletion. + continue; + } + + const uint160 uCurOfrAccountID = sleCurOfr->getIValueFieldAccount(sfAccount).getAccountID(); + const aciSource asLine = boost::make_tuple(uCurOfrAccountID, uCurCurrencyID, uCurIssuerID); + + // Allowed to access source from this node? + curIssuerNodeConstIterator it = pspCur->umReverse.find(asLine); + + if (it == pspCur->umReverse.end()) + { + // Temporarily unfunded. ignore in this column. + + nothing(); + continue; + } + const STAmount& saCurOfrOutReq = sleCurOfr->getIValueFieldAmount(sfTakerGets); // UNUSED? const STAmount& saCurOfrIn = sleCurOfr->getIValueFieldAmount(sfTakerPays); STAmount saCurOfrFunds = accountFunds(uCurOfrAccountID, saCurOfrOutReq); // Funds left. - // XXX Offer is also unfunded if funding source previously mentioned. if (!saCurOfrFunds) { // Offer is unfunded. Log(lsINFO) << "calcNodeOfferRev: encountered unfunded offer"; - // XXX Mark unfunded. + curIssuerNodeConstIterator itSource = mumSource.find(asLine); + if (itSource == mumSource.end()) + { + // Never mentioned before: found unfunded. + musUnfundedFound.insert(uCurIndex); // Mark offer for always deletion. + } + else + { + // Mentioned before: source became unfunded. + pspCur->vUnfundedBecame.push_back(uCurIndex); // Mark offer for deletion on use. + } + continue; } - else if (sleCurOfr->getIFieldPresent(sfExpiration) && sleCurOfr->getIFieldU32(sfExpiration) <= mLedger->getParentCloseTimeNC()) - { - // Offer is expired. - Log(lsINFO) << "calcNodeOfferRev: encountered expired offer"; - // XXX Mark unfunded. - } - else if (!!uNxtAccountID) + if (!!uNxtAccountID) { // Next is an account. @@ -2094,9 +2131,9 @@ bool TransactionEngine::calcNodeOfferRev( } bool TransactionEngine::calcNodeOfferFwd( - unsigned int uIndex, // 0 < uIndex < uLast - PathState::pointer pspCur, - bool bMultiQuality + const unsigned int uIndex, // 0 < uIndex < uLast + const PathState::pointer& pspCur, + const bool bMultiQuality ) { bool bSuccess = false; @@ -2615,7 +2652,7 @@ Log(lsINFO) << boost::str(boost::format("calcNodeRipple:4: saCurReq=%s") % saCur } // Calculate saPrvRedeemReq, saPrvIssueReq, saPrvDeliver; -bool TransactionEngine::calcNodeAccountRev(unsigned int uIndex, PathState::pointer pspCur, bool bMultiQuality) +bool TransactionEngine::calcNodeAccountRev(const unsigned int uIndex, const PathState::pointer& pspCur, const bool bMultiQuality) { bool bSuccess = true; const unsigned int uLast = pspCur->vpnNodes.size() - 1; @@ -2948,9 +2985,9 @@ bool TransactionEngine::calcNodeAccountRev(unsigned int uIndex, PathState::point // - Output to next node minus fees. // Perform balance adjustment with previous. bool TransactionEngine::calcNodeAccountFwd( - unsigned int uIndex, // 0 <= uIndex <= uLast - PathState::pointer pspCur, - bool bMultiQuality) + const unsigned int uIndex, // 0 <= uIndex <= uLast + const PathState::pointer& pspCur, + const bool bMultiQuality) { bool bSuccess = true; const unsigned int uLast = pspCur->vpnNodes.size() - 1; @@ -3282,7 +3319,8 @@ bool PathState::pushImply(uint160 uAccountID, uint160 uCurrencyID, uint160 uIssu } // Append a node and insert before it any implied nodes. -// <-- bValid: true, if node is valid. false, if node is malformed. +// <-- bValid: true, if node is valid. false, if node is malformed or missing credit link. +// XXX Report missinge credit link distinct from malformed for retry. bool PathState::pushNode(int iType, uint160 uAccountID, uint160 uCurrencyID, uint160 uIssuerID) { Log(lsINFO) << "pushNode> " @@ -3431,7 +3469,6 @@ bool PathState::pushNode(int iType, uint160 uAccountID, uint160 uCurrencyID, uin return bValid; } -// XXX Disallow loops in ripple paths PathState::PathState( const Ledger::pointer& lpLedger, const int iIndex, @@ -3491,6 +3528,33 @@ PathState::PathState( } } + if (bValid) + { + // Look for first mention of source in nodes and detect loops. + // Note: The output is not allowed to be a source. + + const unsigned int uNodes = vpnNodes.size(); + + for (unsigned int uIndex = 0; bValid && uIndex != uNodes; ++uIndex) + { + const paymentNode& pnCur = vpnNodes[uIndex]; + + 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), uIndex)).second) + { + // Failed to insert. Have a loop. + Log(lsINFO) << boost::str(boost::format("PathState: loop detected: %s") + % getJson()); + + bValid = false; + } + } + } + Log(lsINFO) << boost::str(boost::format("PathState: in=%s/%s out=%s/%s %s") % STAmount::createHumanCurrency(uInCurrencyID) % NewcoinAddress::createHumanAccountID(uInIssuerID) @@ -3583,8 +3647,7 @@ Json::Value PathState::getJson() const // --> [all]saWanted.mCurrency // --> [all]saAccount // <-> [0]saWanted.mAmount : --> limit, <-- actual -// XXX Disallow looping. -bool TransactionEngine::calcNode(unsigned int uIndex, PathState::pointer pspCur, bool bMultiQuality) +bool TransactionEngine::calcNode(const unsigned int uIndex, const PathState::pointer& pspCur, const bool bMultiQuality) { const paymentNode& pnCur = pspCur->vpnNodes[uIndex]; const bool bCurAccount = !!(pnCur.uFlags & STPathElement::typeAccount); @@ -3619,7 +3682,7 @@ bool TransactionEngine::calcNode(unsigned int uIndex, PathState::pointer pspCur, // Calculate the next increment of a path. // The increment is what can satisfy a portion or all of the requested output at the best quality. // <-- pspCur->uQuality -void TransactionEngine::pathNext(PathState::pointer pspCur, int iPaths) +void TransactionEngine::pathNext(const PathState::pointer& pspCur, const int iPaths) { // The next state is what is available in preference order. // This is calculated when referenced accounts changed. @@ -3631,7 +3694,7 @@ void TransactionEngine::pathNext(PathState::pointer pspCur, int iPaths) assert(pspCur->vpnNodes.size() >= 2); pspCur->vUnfundedBecame.clear(); - pspCur->umSource.clear(); + pspCur->umReverse.clear(); pspCur->bValid = calcNode(uLast, pspCur, iPaths == 1); @@ -3857,8 +3920,8 @@ TransactionEngineResult TransactionEngine::doPayment(const SerializedTransaction { // Prepare for next pass. - // Merge best pass' umSource. - mumSource.insert(pspBest->umSource.begin(), pspBest->umSource.end()); + // Merge best pass' umReverse. + mumSource.insert(pspBest->umReverse.begin(), pspBest->umReverse.end()); } } // Not done and ran out of paths. @@ -3981,7 +4044,6 @@ TransactionEngineResult TransactionEngine::doInvoice(const SerializedTransaction // <-- saTakerPaid: What taker paid not including fees. To reduce an offer. // <-- saTakerGot: What taker got not including fees. To reduce an offer. // <-- terResult: tesSUCCESS or terNO_ACCOUNT -// Note: All SLE modifications must always occur even on failure. // XXX: Fees should be paid by the source of the currency. TransactionEngineResult TransactionEngine::takeOffers( bool bPassive, diff --git a/src/TransactionEngine.h b/src/TransactionEngine.h index f8b7f34ed1..5a88c1775c 100644 --- a/src/TransactionEngine.h +++ b/src/TransactionEngine.h @@ -1,6 +1,7 @@ #ifndef __TRANSACTIONENGINE__ #define __TRANSACTIONENGINE__ +#include #include #include @@ -97,8 +98,8 @@ enum TransactionEngineParams typedef struct { uint16 uFlags; // --> From path. - uint160 uAccountID; // --> Recieving/sending account. - uint160 uCurrencyID; // --> Accounts: receive and send, Offers: send. + uint160 uAccountID; // --> Accounts: Recieving/sending account. + uint160 uCurrencyID; // --> Accounts: Receive and send, Offers: send. // --- For offer's next has currency out. uint160 uIssuerID; // --> Currency's issuer @@ -115,6 +116,13 @@ typedef struct { STAmount saFwdDeliver; // <-- Amount to deliver to next regardless of fee. } paymentNode; +// account id, currency id, issuer id :: node +typedef boost::tuple aciSource; +typedef boost::unordered_map curIssuerNode; // Map of currency, issuer to node index. +typedef boost::unordered_map::const_iterator curIssuerNodeConstIterator; + +extern std::size_t hash_value(const aciSource& asValue); + // Hold a path state under incremental application. class PathState { @@ -133,8 +141,12 @@ public: // When processing, don't want to complicate directory walking with deletion. std::vector vUnfundedBecame; // Offers that became unfunded. - // First time working in reverse a funding source was mentioned. Source may only be used there. - boost::unordered_map, int> umSource; // Map of currency, issuer to node index. + // First time working foward a funding source was mentioned for accounts. Source may only be used there. + curIssuerNode umForward; // Map of currency, issuer to node index. + + // First time working in reverse a funding source was used. + // Source may only be used there if not mentioned by an account. + curIssuerNode umReverse; // Map of currency, issuer to node index. LedgerEntrySet lesEntries; @@ -221,7 +233,7 @@ protected: SLE::pointer mTxnAccount; // First time working in reverse a funding source was mentioned. Source may only be used there. - boost::unordered_map, int> mumSource; // Map of currency, issuer to node index. + curIssuerNode mumSource; // Map of currency, issuer to node index. // When processing, don't want to complicate directory walking with deletion. std::vector mvUnfundedBecame; // Offers that became unfunded. @@ -254,12 +266,12 @@ protected: STAmount accountFunds(const uint160& uAccountID, const STAmount& saDefault); PathState::pointer pathCreate(const STPath& spPath); - void pathNext(PathState::pointer pspCur, int iPaths); - bool calcNode(unsigned int uIndex, PathState::pointer pspCur, bool bMultiQuality); - bool calcNodeOfferRev(unsigned int uIndex, PathState::pointer pspCur, bool bMultiQuality); - bool calcNodeOfferFwd(unsigned int uIndex, PathState::pointer pspCur, bool bMultiQuality); - bool calcNodeAccountRev(unsigned int uIndex, PathState::pointer pspCur, bool bMultiQuality); - bool calcNodeAccountFwd(unsigned int uIndex, PathState::pointer pspCur, bool bMultiQuality); + void pathNext(const PathState::pointer& pspCur, const int iPaths); + bool calcNode(const unsigned int uIndex, const PathState::pointer& pspCur, const bool bMultiQuality); + bool calcNodeOfferRev(const unsigned int uIndex, const PathState::pointer& pspCur, const bool bMultiQuality); + bool calcNodeOfferFwd(const unsigned int uIndex, const PathState::pointer& pspCur, const bool bMultiQuality); + bool calcNodeAccountRev(const unsigned int uIndex, const PathState::pointer& pspCur, const bool bMultiQuality); + bool calcNodeAccountFwd(const unsigned int uIndex, const PathState::pointer& pspCur, const bool bMultiQuality); void calcNodeRipple(const uint32 uQualityIn, const uint32 uQualityOut, const STAmount& saPrvReq, const STAmount& saCurReq, STAmount& saPrvAct, STAmount& saCurAct);