From a1915bd89986e34a0ecf1fbb05b6437531e3c3e2 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Tue, 11 Dec 2012 14:03:32 -0800 Subject: [PATCH 1/3] Omit default paths from pathfinder results. --- src/cpp/ripple/Pathfinder.cpp | 36 ++++++++++++++++++++++++----------- src/cpp/ripple/RippleCalc.cpp | 7 +++++++ 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/cpp/ripple/Pathfinder.cpp b/src/cpp/ripple/Pathfinder.cpp index 7dcc5ef1b..c8f0b66ba 100644 --- a/src/cpp/ripple/Pathfinder.cpp +++ b/src/cpp/ripple/Pathfinder.cpp @@ -77,7 +77,6 @@ PathOption::PathOption(PathOption::pointer other) // Return true, if path is a default path with an element. // A path is a default path if it is implied via src, dst, send, and sendmax. -// XXX Could be determined via STAmount bool Pathfinder::bDefaultPath(const STPath& spPath) { if (2 == spPath.mPath.size()) { @@ -87,6 +86,15 @@ bool Pathfinder::bDefaultPath(const STPath& spPath) return true; } + if (!mPsDefault) + { + // No default path. + // There might not be a direct credit line or there may be no implied nodes + // in send and sendmax. + + return false; // Didn't generate a default path. So can't match. + } + PathState::pointer pspCurrent = boost::make_shared(mDstAmount, mSrcAmount, mLedger); if (pspCurrent) @@ -98,6 +106,8 @@ bool Pathfinder::bDefaultPath(const STPath& spPath) bDefault = pspCurrent->vpnNodes == mPsDefault->vpnNodes; + cLog(lsDEBUG) << "findPaths: expanded path: " << pspCurrent->getJson(); + // Path is a default (implied). Don't need to add it to return set. cLog(lsDEBUG) << "findPaths: default path: indirect: " << spPath.getJson(0); @@ -119,13 +129,23 @@ Pathfinder::Pathfinder(const RippleAddress& uSrcAccountID, const RippleAddress& // Construct the default path for later comparison. - mPsDefault = boost::make_shared(mDstAmount, mSrcAmount, mLedger); + PathState::pointer psDefault = boost::make_shared(mDstAmount, mSrcAmount, mLedger); - if (mPsDefault) + if (psDefault) { LedgerEntrySet lesActive(mLedger); - mPsDefault->setExpanded(lesActive, STPath(), mDstAccountID, mSrcAccountID); + psDefault->setExpanded(lesActive, STPath(), mDstAccountID, mSrcAccountID); + + if (tesSUCCESS == psDefault->terStatus) + { + cLog(lsDEBUG) << "Pathfinder: reference path: " << psDefault->getJson(); + mPsDefault = psDefault; + } + else + { + cLog(lsDEBUG) << "Pathfinder: reference path: NONE: " << transToken(psDefault->terStatus); + } } } @@ -150,13 +170,7 @@ bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet % RippleAddress::createHumanAccountID(mSrcIssuerID) ); - if (!mPsDefault) - { - cLog(lsDEBUG) << boost::str(boost::format("findPaths: failed to generate default path.")); - - return false; - } - else if (mLedger) + if (mLedger) { std::queue pqueue; STPathElement ele(mSrcAccountID, diff --git a/src/cpp/ripple/RippleCalc.cpp b/src/cpp/ripple/RippleCalc.cpp index 8919a848e..5440eb34e 100644 --- a/src/cpp/ripple/RippleCalc.cpp +++ b/src/cpp/ripple/RippleCalc.cpp @@ -62,6 +62,7 @@ bool PathState::lessPriority(PathState& lhs, PathState& rhs) // - A node names it's output. // - A ripple nodes output issuer must be the node's account or the next node's account. // - Offers can only go directly to another offer if the currency and issuer are an exact match. +// - Real issuers must be specified for non-XRP. TER PathState::pushImply( const uint160& uAccountID, // --> Delivering to this account. const uint160& uCurrencyID, // --> Delivering this currency. @@ -186,6 +187,8 @@ TER PathState::pushNode( else { // Add required intermediate nodes to deliver to current account. + cLog(lsDEBUG) << "pushNode: imply for account."; + terResult = pushImply( pnCur.uAccountID, // Current account. pnCur.uCurrencyID, // Wanted currency. @@ -262,6 +265,7 @@ TER PathState::pushNode( else if (!!pnPrv.uAccountID) { // Previous is an account. + cLog(lsDEBUG) << "pushNode: imply for offer."; // Insert intermediary issuer account if needed. terResult = pushImply( @@ -364,7 +368,10 @@ cLog(lsDEBUG) << boost::str(boost::format("PathState: sender implied: account=%s BOOST_FOREACH(const STPathElement& speElement, spSourcePath) { if (tesSUCCESS == terStatus) + { +cLog(lsDEBUG) << boost::str(boost::format("PathState: element in path:")); terStatus = pushNode(speElement.getNodeType(), speElement.getAccountID(), speElement.getCurrency(), speElement.getIssuerID()); + } } const PaymentNode& pnPrv = vpnNodes.back(); From 154f4bc9805c635c74136b4f3ef09bcc8b36f4c0 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Tue, 11 Dec 2012 14:03:53 -0800 Subject: [PATCH 2/3] UT: Update tests for path finding. --- test/path-test.js | 159 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 158 insertions(+), 1 deletion(-) diff --git a/test/path-test.js b/test/path-test.js index 3b2e7c332..9886eedb6 100644 --- a/test/path-test.js +++ b/test/path-test.js @@ -13,9 +13,161 @@ require("../src/js/remote.js").config = require("./config.js"); buster.testRunner.timeout = 5000; buster.testCase("Path finding", { + // 'setUp' : testutils.build_setup({ verbose: true, no_server: true }), + // 'setUp' : testutils.build_setup({ verbose: true }), 'setUp' : testutils.build_setup(), 'tearDown' : testutils.build_teardown(), + "no direct path, no intermediary -> no alternatives" : + function (done) { + var self = this; + + async.waterfall([ + function (callback) { + self.what = "Create accounts."; + + testutils.create_accounts(self.remote, "root", "10000", ["alice", "bob"], callback); + }, + function (callback) { + self.what = "Find path from alice to bob"; + + self.remote.request_ripple_path_find("alice", "bob", "5/USD/bob", + [ { 'currency' : "USD" } ]) + .on('success', function (m) { + // console.log("proposed: %s", JSON.stringify(m)); + + callback(m.alternatives.length); + }) + .request(); + }, + ], function (error) { + buster.refute(error, self.what); + done(); + }); + }, + + "direct path, no intermediary" : + function (done) { + var self = this; + + async.waterfall([ + function (callback) { + self.what = "Create accounts."; + + testutils.create_accounts(self.remote, "root", "10000", ["alice", "bob"], callback); + }, + function (callback) { + self.what = "Set credit limits."; + + testutils.credit_limits(self.remote, + { + "bob" : "700/USD/alice", + }, + callback); + }, +// function (callback) { +// self.what = "Display ledger"; +// +// self.remote.request_ledger('current', true) +// .on('success', function (m) { +// console.log("Ledger: %s", JSON.stringify(m, undefined, 2)); +// +// callback(); +// }) +// .request(); +// }, +// function (callback) { +// self.what = "Display available lines from alice"; +// +// self.remote.request_account_lines("alice", undefined, 'CURRENT') +// .on('success', function (m) { +// console.log("LINES: %s", JSON.stringify(m, undefined, 2)); +// +// callback(); +// }) +// .request(); +// }, + function (callback) { + self.what = "Find path from alice to bob"; + + self.remote.request_ripple_path_find("alice", "bob", "5/USD/bob", + [ { 'currency' : "USD" } ]) + .on('success', function (m) { + // console.log("proposed: %s", JSON.stringify(m)); + + // 1 alternative. + buster.assert.equals(1, m.alternatives.length) + // Path is empty. + buster.assert.equals(0, m.alternatives[0].paths_canonical.length) + + callback(); + }) + .request(); + }, + ], function (error) { + buster.refute(error, self.what); + done(); + }); + }, + + "payment auto path find (using build_path)" : + function (done) { + var self = this; + + async.waterfall([ + function (callback) { + self.what = "Create accounts."; + + testutils.create_accounts(self.remote, "root", "10000", ["alice", "bob", "mtgox"], callback); + }, + function (callback) { + self.what = "Set credit limits."; + + testutils.credit_limits(self.remote, + { + "alice" : "600/USD/mtgox", + "bob" : "700/USD/mtgox", + }, + callback); + }, + function (callback) { + self.what = "Distribute funds."; + + testutils.payments(self.remote, + { + "mtgox" : [ "70/USD/alice" ], + }, + callback); + }, + function (callback) { + self.what = "Payment with auto path"; + + self.remote.transaction() + .payment('alice', 'bob', "24/USD/bob") + .build_path(true) + .once('proposed', function (m) { + // console.log("proposed: %s", JSON.stringify(m)); + callback(m.result !== 'tesSUCCESS'); + }) + .submit(); + }, + function (callback) { + self.what = "Verify balances."; + + testutils.verify_balances(self.remote, + { + "alice" : "46/USD/mtgox", + "mtgox" : [ "-46/USD/alice", "-24/USD/bob" ], + "bob" : "24/USD/mtgox", + }, + callback); + }, + ], function (error) { + buster.refute(error, self.what); + done(); + }); + }, + "path find" : function (done) { var self = this; @@ -51,7 +203,12 @@ buster.testCase("Path finding", { self.remote.request_ripple_path_find("alice", "bob", "5/USD/mtgox", [ { 'currency' : "USD" } ]) .on('success', function (m) { - console.log("proposed: m", JSON.stringify(m)); + // console.log("proposed: %s", JSON.stringify(m)); + + // 1 alternative. + buster.assert.equals(1, m.alternatives.length) + // Path is empty. + buster.assert.equals(0, m.alternatives[0].paths_canonical.length) callback(); }) From eac6fe88c1194b2ebd6ef6018410ae3c8a9091c0 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Tue, 11 Dec 2012 15:31:55 -0800 Subject: [PATCH 3/3] Add support for multiple path finding and path length limiting. --- src/cpp/ripple/Pathfinder.cpp | 163 ++++++++++++++++++++++------------ src/cpp/ripple/Pathfinder.h | 2 +- src/cpp/ripple/RPCHandler.cpp | 4 +- 3 files changed, 108 insertions(+), 61 deletions(-) diff --git a/src/cpp/ripple/Pathfinder.cpp b/src/cpp/ripple/Pathfinder.cpp index c8f0b66ba..6b8c71ba1 100644 --- a/src/cpp/ripple/Pathfinder.cpp +++ b/src/cpp/ripple/Pathfinder.cpp @@ -75,6 +75,11 @@ PathOption::PathOption(PathOption::pointer other) } #endif +static bool bQualityCmp(std::pair a, std::pair b) +{ + return a.first < b.first; +} + // Return true, if path is a default path with an element. // A path is a default path if it is implied via src, dst, send, and sendmax. bool Pathfinder::bDefaultPath(const STPath& spPath) @@ -150,17 +155,17 @@ Pathfinder::Pathfinder(const RippleAddress& uSrcAccountID, const RippleAddress& } // If possible, returns a single path. -// --> maxSearchSteps: unused XXX -// --> maxPay: unused XXX +// --> iMaxSteps: Maximum nodes in paths to return. +// --> iMaxPaths: Maximum number of paths to return. // <-- retPathSet: founds paths not including default paths. // Returns true if found paths. // // When generating a path set blindly, don't allow the empty path, it is implied by default. // When generating a path set for estimates, allow an empty path instead of no paths to indicate a path exists. The caller will // need to strip the empty path when submitting the transaction. -bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet) +bool Pathfinder::findPaths(const unsigned int iMaxSteps, const unsigned int iMaxPaths, STPathSet& spsDst) { - bool bFound = false; + bool bFound = false; // True, iff found a path. cLog(lsDEBUG) << boost::str(boost::format("findPaths> mSrcAccountID=%s mDstAccountID=%s mDstAmount=%s mSrcCurrencyID=%s mSrcIssuerID=%s") % RippleAddress::createHumanAccountID(mSrcAccountID) @@ -172,72 +177,85 @@ bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet if (mLedger) { - std::queue pqueue; - STPathElement ele(mSrcAccountID, - mSrcCurrencyID, - uint160()); // XXX Might add source issuer. + std::vector vspResults; + std::queue qspExplore; + + STPathElement speEnd(mSrcAccountID, + mSrcCurrencyID, + uint160()); // XXX Might add source issuer. STPath path; - path.addElement(ele); // Add the source. + path.addElement(speEnd); // Add the source. - pqueue.push(path); + qspExplore.push(path); - while (pqueue.size()) { - STPath path = pqueue.front(); + while (qspExplore.size()) { + STPath spPath = qspExplore.front(); - pqueue.pop(); // Pop the first path from the queue. + qspExplore.pop(); // Pop the first path from the queue. - ele = path.mPath.back(); // Get the last node from the path. + speEnd = spPath.mPath.back(); // Get the last node from the path. // Done, if dest wants XRP and last element produces XRP. - if (!ele.mCurrencyID // Tail output is XRP. + if (!speEnd.mCurrencyID // Tail output is XRP. && !mDstAmount.getCurrency()) { // Which is dst currency. // Remove implied first. - path.mPath.erase(path.mPath.begin()); + spPath.mPath.erase(spPath.mPath.begin()); - if (path.size()) + if (spPath.size()) { // There is an actual path element. - retPathSet.addPath(path); // Return the path. + vspResults.push_back(spPath); // Potential result. - cLog(lsDEBUG) << "findPaths: adding: " << path.getJson(0); + cLog(lsDEBUG) << "findPaths: adding: " << spPath.getJson(0); } else { cLog(lsDEBUG) << "findPaths: empty path: XRP->XRP"; } - return true; + continue; } // Done, if dest wants non-XRP and last element is dest. // YYY Allows going through self. Is this wanted? - if (ele.mAccountID == mDstAccountID // Tail is destination account. - && ele.mCurrencyID == mDstAmount.getCurrency()) { // With correct output currency. + if (speEnd.mAccountID == mDstAccountID // Tail is destination account. + && speEnd.mCurrencyID == mDstAmount.getCurrency()) { // With correct output currency. // Found a path to the destination. - if (bDefaultPath(path)) { - cLog(lsDEBUG) << "findPaths: default path: dropping: " << path.getJson(0); - return true; + if (bDefaultPath(spPath)) { + cLog(lsDEBUG) << "findPaths: default path: dropping: " << spPath.getJson(0); + + bFound = true; + } + else + { + + // Remove implied first and last nodes. + spPath.mPath.erase(spPath.mPath.begin()); + spPath.mPath.erase(spPath.mPath.begin() + spPath.mPath.size()-1); + + vspResults.push_back(spPath); // Potential result. + + cLog(lsDEBUG) << "findPaths: adding: " << spPath.getJson(0); } - // Remove implied first and last nodes. - path.mPath.erase(path.mPath.begin()); - path.mPath.erase(path.mPath.begin() + path.mPath.size()-1); - - // Return the path. - retPathSet.addPath(path); - - cLog(lsDEBUG) << "findPaths: adding: " << path.getJson(0); - - return true; + continue; } bool bContinued = false; - if (!ele.mCurrencyID) { + if (spPath.mPath.size() == iMaxSteps) + { + // Path is at maximum size. Don't want to add more. + + cLog(lsDEBUG) + << boost::str(boost::format("findPaths: path would exceed max steps: dropping")); + } + else if (!speEnd.mCurrencyID) + { // Last element is for XRP continue with qualifying books. BOOST_FOREACH(OrderBook::pointer book, mOrderBook.getXRPInBooks()) { @@ -245,7 +263,7 @@ bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet //if (!path.hasSeen(line->getAccountIDPeer().getAccountID())) { - STPath new_path(path); + STPath new_path(spPath); STPathElement new_ele(uint160(), book->getCurrencyOut(), book->getIssuerOut()); new_path.mPath.push_back(new_ele); @@ -257,7 +275,7 @@ bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet % STAmount::createHumanCurrency(new_path.mCurrencyID) % RippleAddress::createHumanAccountID(new_path.mCurrentAccount)); - pqueue.push(new_path); + qspExplore.push(new_path); bContinued = true; } @@ -265,33 +283,34 @@ bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet tLog(!bContinued, lsDEBUG) << boost::str(boost::format("findPaths: XRP input - dead end")); - - } else { + } + else + { // Last element is for non-XRP continue by adding ripple lines and order books. // Create new paths for each outbound account not already in the path. - AccountItems rippleLines(ele.mAccountID, mLedger, AccountItem::pointer(new RippleState())); + AccountItems rippleLines(speEnd.mAccountID, mLedger, AccountItem::pointer(new RippleState())); BOOST_FOREACH(AccountItem::pointer item, rippleLines.getItems()) { RippleState* line=(RippleState*)item.get(); - if (!path.hasSeen(line->getAccountIDPeer().getAccountID())) + if (!spPath.hasSeen(line->getAccountIDPeer().getAccountID())) { - STPath new_path(path); + STPath new_path(spPath); STPathElement new_ele(line->getAccountIDPeer().getAccountID(), - ele.mCurrencyID, + speEnd.mCurrencyID, uint160()); cLog(lsDEBUG) << boost::str(boost::format("findPaths: %s/%s --> %s/%s") - % RippleAddress::createHumanAccountID(ele.mAccountID) - % STAmount::createHumanCurrency(ele.mCurrencyID) + % RippleAddress::createHumanAccountID(speEnd.mAccountID) + % STAmount::createHumanCurrency(speEnd.mCurrencyID) % RippleAddress::createHumanAccountID(line->getAccountIDPeer().getAccountID()) - % STAmount::createHumanCurrency(ele.mCurrencyID)); + % STAmount::createHumanCurrency(speEnd.mCurrencyID)); new_path.mPath.push_back(new_ele); - pqueue.push(new_path); + qspExplore.push(new_path); bContinued = true; } @@ -299,27 +318,27 @@ bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet { cLog(lsDEBUG) << boost::str(boost::format("findPaths: SEEN: %s/%s --> %s/%s") - % RippleAddress::createHumanAccountID(ele.mAccountID) - % STAmount::createHumanCurrency(ele.mCurrencyID) + % RippleAddress::createHumanAccountID(speEnd.mAccountID) + % STAmount::createHumanCurrency(speEnd.mCurrencyID) % RippleAddress::createHumanAccountID(line->getAccountIDPeer().getAccountID()) - % STAmount::createHumanCurrency(ele.mCurrencyID)); + % STAmount::createHumanCurrency(speEnd.mCurrencyID)); } } // Every book that wants the source currency. std::vector books; - mOrderBook.getBooks(path.mCurrentAccount, path.mCurrencyID, books); + mOrderBook.getBooks(spPath.mCurrentAccount, spPath.mCurrencyID, books); BOOST_FOREACH(OrderBook::pointer book,books) { - STPath new_path(path); + STPath new_path(spPath); STPathElement new_ele(uint160(), book->getCurrencyOut(), book->getIssuerOut()); cLog(lsDEBUG) << boost::str(boost::format("findPaths: %s/%s :: %s/%s") - % STAmount::createHumanCurrency(ele.mCurrencyID) - % RippleAddress::createHumanAccountID(ele.mAccountID) + % STAmount::createHumanCurrency(speEnd.mCurrencyID) + % RippleAddress::createHumanAccountID(speEnd.mAccountID) % STAmount::createHumanCurrency(book->getCurrencyOut()) % RippleAddress::createHumanAccountID(book->getIssuerOut())); @@ -327,14 +346,42 @@ bool Pathfinder::findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet new_path.mCurrentAccount=book->getIssuerOut(); new_path.mCurrencyID=book->getCurrencyOut(); - pqueue.push(new_path); + qspExplore.push(new_path); bContinued = true; } + + tLog(!bContinued, lsDEBUG) + << boost::str(boost::format("findPaths: non-XRP input - dead end")); + } + } + + unsigned int iLimit = std::min(iMaxPaths, (unsigned int) vspResults.size()); + + if (iLimit) + { + std::vector< std::pair > vMap; + + // Build map of quality to entry. + for (int i = vspResults.size(); i--;) + { + uint32 uQuality = 1; + + if (uQuality) + vMap.push_back(std::make_pair(uQuality, i)); } - tLog(!bContinued, lsDEBUG) - << boost::str(boost::format("findPaths: non-XRP input - dead end")); + std::sort(vMap.begin(), vMap.end(), bQualityCmp); + + // Output best quality entries. + for (int i = 0; i != iLimit; ++i) + { + spsDst.addPath(vspResults[vMap[i].second]); + } + + bFound = true; + + cLog(lsWARNING) << boost::str(boost::format("findPaths: RESULTS: %s") % spsDst.getJson(0)); } } else diff --git a/src/cpp/ripple/Pathfinder.h b/src/cpp/ripple/Pathfinder.h index f890abbaf..27f8d4480 100644 --- a/src/cpp/ripple/Pathfinder.h +++ b/src/cpp/ripple/Pathfinder.h @@ -58,7 +58,7 @@ class Pathfinder public: Pathfinder(const RippleAddress& srcAccountID, const RippleAddress& dstAccountID, const uint160& srcCurrencyID, const uint160& srcIssuerID, const STAmount& dstAmount); - bool findPaths(int maxSearchSteps, int maxPay, STPathSet& retPathSet); + bool findPaths(const unsigned int iMaxSteps, const unsigned int iMaxPaths, STPathSet& spsDst); bool bDefaultPath(const STPath& spPath); }; diff --git a/src/cpp/ripple/RPCHandler.cpp b/src/cpp/ripple/RPCHandler.cpp index c2fa4648d..33f80df17 100644 --- a/src/cpp/ripple/RPCHandler.cpp +++ b/src/cpp/ripple/RPCHandler.cpp @@ -765,7 +765,7 @@ Json::Value RPCHandler::doRipplePathFind(Json::Value jvRequest) STPathSet spsComputed; Pathfinder pf(raSrc, raDst, uSrcCurrencyID, uSrcIssuerID, saDstAmount); - if (!pf.findPaths(5, 1, spsComputed)) + if (!pf.findPaths(5, 3, spsComputed)) { cLog(lsDEBUG) << "ripple_path_find: No paths found."; } @@ -958,7 +958,7 @@ Json::Value RPCHandler::doSubmit(Json::Value jvRequest) Pathfinder pf(raSrcAddressID, dstAccountID, saSendMax.getCurrency(), saSendMax.getIssuer(), saSend); - if (!pf.findPaths(5, 1, spsPaths)) + if (!pf.findPaths(5, 3, spsPaths)) { cLog(lsDEBUG) << "payment: build_path: No paths found.";