From aad57519ae630eb59d9f861afe8d31779082e381 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Sun, 6 Jan 2013 17:50:09 -0800 Subject: [PATCH 1/4] Improve pathfinding, don't explore obviously dry paths. --- src/cpp/ripple/Pathfinder.cpp | 46 +++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/src/cpp/ripple/Pathfinder.cpp b/src/cpp/ripple/Pathfinder.cpp index acce67ae6..a1d37d192 100644 --- a/src/cpp/ripple/Pathfinder.cpp +++ b/src/cpp/ripple/Pathfinder.cpp @@ -268,7 +268,7 @@ bool Pathfinder::findPaths(const unsigned int iMaxSteps, const unsigned int iMax } else if (!speEnd.mCurrencyID) { - // Last element is for XRP continue with qualifying books. + // Last element is for XRP, continue with qualifying books. BOOST_FOREACH(OrderBook::ref book, mOrderBook.getXRPInBooks()) { // XXX Don't allow looping through same order books. @@ -298,19 +298,42 @@ bool Pathfinder::findPaths(const unsigned int iMaxSteps, const unsigned int iMax } else { - // Last element is for non-XRP continue by adding ripple lines and order books. + // 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(speEnd.mAccountID, mLedger, AccountItem::pointer(new RippleState())); BOOST_FOREACH(AccountItem::ref item, rippleLines.getItems()) { - RippleState* line=(RippleState*)item.get(); + RippleState* rspEntry = (RippleState*) item.get(); - if (!spPath.hasSeen(line->getAccountIDPeer().getAccountID())) + if (spPath.hasSeen(rspEntry->getAccountIDPeer().getAccountID())) { + // Peer is in path already. Ignore it to avoid a loop. + cLog(lsDEBUG) << + boost::str(boost::format("findPaths: SEEN: %s/%s --> %s/%s") + % RippleAddress::createHumanAccountID(speEnd.mAccountID) + % STAmount::createHumanCurrency(speEnd.mCurrencyID) + % RippleAddress::createHumanAccountID(rspEntry->getAccountIDPeer().getAccountID()) + % STAmount::createHumanCurrency(speEnd.mCurrencyID)); + } + else if (!rspEntry->getBalance().isPositive() // Have IOUs to send. + && (!rspEntry->getLimitPeer() // Peer does not extend credit. + || *rspEntry->getBalance().negate() >= rspEntry->getLimitPeer())) // No credit left. + { + // Path has no credit left. Ignore it. + cLog(lsDEBUG) << + boost::str(boost::format("findPaths: No credit: %s/%s --> %s/%s") + % RippleAddress::createHumanAccountID(speEnd.mAccountID) + % STAmount::createHumanCurrency(speEnd.mCurrencyID) + % RippleAddress::createHumanAccountID(rspEntry->getAccountIDPeer().getAccountID()) + % STAmount::createHumanCurrency(speEnd.mCurrencyID)); + } + else + { + // Can transmit IOUs. STPath new_path(spPath); - STPathElement new_ele(line->getAccountIDPeer().getAccountID(), + STPathElement new_ele(rspEntry->getAccountIDPeer().getAccountID(), speEnd.mCurrencyID, uint160()); @@ -318,7 +341,7 @@ bool Pathfinder::findPaths(const unsigned int iMaxSteps, const unsigned int iMax boost::str(boost::format("findPaths: %s/%s --> %s/%s") % RippleAddress::createHumanAccountID(speEnd.mAccountID) % STAmount::createHumanCurrency(speEnd.mCurrencyID) - % RippleAddress::createHumanAccountID(line->getAccountIDPeer().getAccountID()) + % RippleAddress::createHumanAccountID(rspEntry->getAccountIDPeer().getAccountID()) % STAmount::createHumanCurrency(speEnd.mCurrencyID)); new_path.mPath.push_back(new_ele); @@ -326,15 +349,6 @@ bool Pathfinder::findPaths(const unsigned int iMaxSteps, const unsigned int iMax bContinued = true; } - else - { - cLog(lsDEBUG) << - boost::str(boost::format("findPaths: SEEN: %s/%s --> %s/%s") - % RippleAddress::createHumanAccountID(speEnd.mAccountID) - % STAmount::createHumanCurrency(speEnd.mCurrencyID) - % RippleAddress::createHumanAccountID(line->getAccountIDPeer().getAccountID()) - % STAmount::createHumanCurrency(speEnd.mCurrencyID)); - } } // Every book that wants the source currency. @@ -448,6 +462,8 @@ bool Pathfinder::findPaths(const unsigned int iMaxSteps, const unsigned int iMax cLog(lsDEBUG) << boost::str(boost::format("findPaths: no ledger")); } + cLog(lsDEBUG) << boost::str(boost::format("findPaths< bFound=%d") % bFound); + return bFound; } From ec85d58bdf9077cc1d9220b0448dacfd1f9477d2 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Sun, 6 Jan 2013 17:50:39 -0800 Subject: [PATCH 2/4] UT: Add UT for issue #5. --- test/path-test.js | 110 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/test/path-test.js b/test/path-test.js index 70e4ebec3..f2faae6a2 100644 --- a/test/path-test.js +++ b/test/path-test.js @@ -660,4 +660,114 @@ buster.testCase("More Path finding", { // Test alternative paths with qualities. }); + +buster.testCase("Path negatives", { + // 'setUp' : testutils.build_setup({ verbose: true, no_server: true }), + // 'setUp' : testutils.build_setup({ verbose: true }), + 'setUp' : testutils.build_setup(), + 'tearDown' : testutils.build_teardown(), + + "=>Issue #5" : + // alice +- bitstamp -+ bob + // |- carol(fee) -| // To be excluded. + // |- dan(issue) -| + // |- mtgox -| + function (done) { + var self = this; + + async.waterfall([ + function (callback) { + self.what = "Create accounts."; + + testutils.create_accounts(self.remote, "root", "10000.0", ["alice", "bob", "carol", "dan"], callback); + }, + function (callback) { + self.what = "Set credit limits."; + + testutils.credit_limits(self.remote, + { + // 2. acct 4 trusted all the other accts for 100 usd + "dan" : [ "100/USD/alice", "100/USD/bob", "100/USD/carol" ], + // 3. acct 2 acted as a nexus for acct 1 and 3, was trusted by 1 and 3 for 100 usd + "alice" : [ "100/USD/bob" ], + "carol" : [ "100/USD/bob" ], + }, + callback); + }, + function (callback) { + self.what = "Distribute funds."; + + testutils.payments(self.remote, + { + // 4. acct 2 sent acct 3 a 75 iou + "bob" : "25/USD/carol", + }, + callback); + }, + function (callback) { + self.what = "Verify balances."; + + testutils.verify_balances(self.remote, + { + "bob" : [ "-25/USD/carol" ], + "carol" : "25/USD/bob", + }, + 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 = "Find path from alice to bob"; + + // 5. acct 1 sent a 25 usd iou to acct 2 + self.remote.request_ripple_path_find("alice", "bob", "25/USD/bob", + [ { 'currency' : "USD" } ]) + .on('success', function (m) { + // console.log("proposed: %s", JSON.stringify(m)); + + // 0 alternatives. + buster.assert.equals(0, m.alternatives.length) + + callback(); + }) + .request(); + }, + function (callback) { + self.what = "alice fails to send to bob."; + + self.remote.transaction() + .payment('alice', 'bob', "25/USD/alice") + .once('proposed', function (m) { + // console.log("proposed: %s", JSON.stringify(m)); + callback(m.result !== 'tecPATH_DRY'); + }) + .submit(); + }, + function (callback) { + self.what = "Verify balances final."; + + testutils.verify_balances(self.remote, + { + "alice" : [ "0/USD/bob", "0/USD/dan"], + "bob" : [ "0/USD/alice", "-25/USD/carol", "0/USD/dan" ], + "carol" : [ "25/USD/bob", "0/USD/dan" ], + "dan" : [ "0/USD/alice", "0/USD/bob", "0/USD/carol" ], + }, + callback); + }, + ], function (error) { + buster.refute(error, self.what); + done(); + }); + } +}); // vim:sw=2:sts=2:ts=8:et From 69a0d5ac164723e6acebd8d5d487c6c498740ccf Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Sun, 6 Jan 2013 18:20:58 -0800 Subject: [PATCH 3/4] UT: Remove focus from latest test. --- test/path-test.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/path-test.js b/test/path-test.js index f2faae6a2..4b290b2e5 100644 --- a/test/path-test.js +++ b/test/path-test.js @@ -667,11 +667,7 @@ buster.testCase("Path negatives", { 'setUp' : testutils.build_setup(), 'tearDown' : testutils.build_teardown(), - "=>Issue #5" : - // alice +- bitstamp -+ bob - // |- carol(fee) -| // To be excluded. - // |- dan(issue) -| - // |- mtgox -| + "Issue #5" : function (done) { var self = this; From 22a8f576e2b60e006c0858f6e347b5bb61db4129 Mon Sep 17 00:00:00 2001 From: Arthur Britto Date: Sun, 6 Jan 2013 21:21:11 -0800 Subject: [PATCH 4/4] UT: Fix Issue #5 test to use 75/USD not 25/USD. --- test/path-test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/path-test.js b/test/path-test.js index 4b290b2e5..692a3940c 100644 --- a/test/path-test.js +++ b/test/path-test.js @@ -696,7 +696,7 @@ buster.testCase("Path negatives", { testutils.payments(self.remote, { // 4. acct 2 sent acct 3 a 75 iou - "bob" : "25/USD/carol", + "bob" : "75/USD/carol", }, callback); }, @@ -705,8 +705,8 @@ buster.testCase("Path negatives", { testutils.verify_balances(self.remote, { - "bob" : [ "-25/USD/carol" ], - "carol" : "25/USD/bob", + "bob" : [ "-75/USD/carol" ], + "carol" : "75/USD/bob", }, callback); }, @@ -754,8 +754,8 @@ buster.testCase("Path negatives", { testutils.verify_balances(self.remote, { "alice" : [ "0/USD/bob", "0/USD/dan"], - "bob" : [ "0/USD/alice", "-25/USD/carol", "0/USD/dan" ], - "carol" : [ "25/USD/bob", "0/USD/dan" ], + "bob" : [ "0/USD/alice", "-75/USD/carol", "0/USD/dan" ], + "carol" : [ "75/USD/bob", "0/USD/dan" ], "dan" : [ "0/USD/alice", "0/USD/bob", "0/USD/carol" ], }, callback);