From 98d5eefc86f8a6cc7f1786538b859ecb858e18d6 Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Tue, 23 Dec 2014 14:39:30 -0800 Subject: [PATCH] Pathfinding bugfixes (RIPD-735): * Fix specifying paths and search level for ripple_path_find * Don't modify the pathfinder, it has issuer-neutral paths. * Handle previous paths correctly * Compare paths correctly --- src/ripple/app/paths/FindPaths.cpp | 16 +- src/ripple/app/paths/FindPaths.h | 12 +- src/ripple/app/paths/Pathfinder.cpp | 232 ++++++++++++--------- src/ripple/app/paths/Pathfinder.h | 11 +- src/ripple/rpc/handlers/RipplePathFind.cpp | 27 +-- 5 files changed, 163 insertions(+), 135 deletions(-) diff --git a/src/ripple/app/paths/FindPaths.cpp b/src/ripple/app/paths/FindPaths.cpp index 3cf7f708c..374d6184b 100644 --- a/src/ripple/app/paths/FindPaths.cpp +++ b/src/ripple/app/paths/FindPaths.cpp @@ -43,15 +43,16 @@ public: bool findPathsForIssue ( Issue const& issue, - STPathSet& pathsOut, + STPathSet& pathsInOut, STPath& fullLiquidityPath) { if (auto& pathfinder = getPathFinder (issue.currency)) { - pathsOut = pathfinder->getBestPaths ( - maxPaths_, fullLiquidityPath, issue.account); + pathsInOut = pathfinder->getBestPaths ( + maxPaths_, fullLiquidityPath, pathsInOut, issue.account); return true; } + assert (false); return false; } @@ -99,10 +100,10 @@ FindPaths::~FindPaths() = default; bool FindPaths::findPathsForIssue ( Issue const& issue, - STPathSet& pathsOut, + STPathSet& pathsInOut, STPath& fullLiquidityPath) { - return impl_->findPathsForIssue (issue, pathsOut, fullLiquidityPath); + return impl_->findPathsForIssue (issue, pathsInOut, fullLiquidityPath); } bool findPathsForOneIssuer ( @@ -113,7 +114,7 @@ bool findPathsForOneIssuer ( STAmount const& dstAmount, int searchLevel, unsigned int const maxPaths, - STPathSet& pathsOut, + STPathSet& pathsInOut, STPath& fullLiquidityPath) { Pathfinder pf ( @@ -127,9 +128,8 @@ bool findPathsForOneIssuer ( if (!pf.findPaths (searchLevel)) return false; - pf.addPathsFromPreviousPathfinding (pathsOut); pf.computePathRanks (maxPaths); - pathsOut = pf.getBestPaths(maxPaths, fullLiquidityPath, srcIssue.account); + pathsInOut = pf.getBestPaths(maxPaths, fullLiquidityPath, pathsInOut, srcIssue.account); return true; } diff --git a/src/ripple/app/paths/FindPaths.h b/src/ripple/app/paths/FindPaths.h index 86f0e1f63..dc7122973 100644 --- a/src/ripple/app/paths/FindPaths.h +++ b/src/ripple/app/paths/FindPaths.h @@ -41,12 +41,12 @@ public: bool findPathsForIssue ( Issue const& issue, - /** On input, pathsOut contains any paths you want to ensure are + /** On input, pathsInOut contains any paths you want to ensure are included if still good. - On output, pathsOut will have any additional paths found. Only + On output, pathsInOut will have any additional paths found. Only non-default paths without source or destination will be added. */ - STPathSet& pathsOut, + STPathSet& pathsInOut, /** On input, fullLiquidityPath must be an empty STPath. @@ -73,12 +73,12 @@ bool findPathsForOneIssuer ( pathsOut. */ unsigned int const maxPaths, - /** On input, pathsOut contains any paths you want to ensure are included if + /** On input, pathsInOut contains any paths you want to ensure are included if still good. - On output, pathsOut will have any additional paths found. Only + On output, pathsInOut will have any additional paths found. Only non-default paths without source or destination will be added. */ - STPathSet& pathsOut, + STPathSet& pathsInOut, /** On input, fullLiquidityPath must be an empty STPath. diff --git a/src/ripple/app/paths/Pathfinder.cpp b/src/ripple/app/paths/Pathfinder.cpp index 66451b496..323458173 100644 --- a/src/ripple/app/paths/Pathfinder.cpp +++ b/src/ripple/app/paths/Pathfinder.cpp @@ -23,47 +23,46 @@ #include #include +/* + +Core Pathfinding Engine + +The pathfinding request is identified by category, XRP to XRP, XRP to +non-XRP, non-XRP to XRP, same currency non-XRP to non-XRP, cross-currency +non-XRP to non-XRP. For each category, there is a table of paths that the +pathfinder searches for. Complete paths are collected. + +Each complete path is then rated and sorted. Paths with no or trivial +liquidity are dropped. Otherwise, paths are sorted based on quality, +liquidity, and path length. + +Path slots are filled in quality (ratio of out to in) order, with the +exception that the last path must have enough liquidity to complete the +payment (assuming no liquidity overlap). In addition, if no selected path +is capable of providing enough liquidity to complete the payment by itself, +an extra "covering" path is returned. + +The selected paths are then tested to determine if they can complete the +payment and, if so, at what cost. If they fail and a covering path was +found, the test is repeated with the covering path. If this succeeds, the +final paths and the estimated cost are returned. + +The engine permits the search depth to be selected and the paths table +includes the depth at which each path type is found. A search depth of zero +causes no searching to be done. Extra paths can also be injected, and this +should be used to preserve previously-found paths across invokations for the +same path request (particularly if the search depth may change). + +*/ + namespace ripple { -/* -we just need to find a succession of the highest quality paths there until we -find enough width - -Don't do branching within each path - -We have a list of paths we are working on but how do we compare the ones that -are terminating in a different currency? - -Loops - -TODO: what is a good way to come up with multiple paths? - Maybe just change the sort criteria? - first a low cost one and then a fat short one? - - -OrderDB: - getXRPOffers(); - - // return list of all orderbooks that want XRP - // return list of all orderbooks that want Issuer - // return list of all orderbooks that want this issuer and currency -*/ - -/* -Test sending to XRP -Test XRP to XRP -Test offer in middle -Test XRP to USD -Test USD to EUR -*/ - namespace { // We sort possible paths by: // cost of path // length of path // width of path -// correct currency at the end. // Compare two PathRanks. A better PathRank is lower, so the best are sorted to // the beginning. @@ -339,35 +338,6 @@ bool Pathfinder::findPaths (int searchLevel) return true; } -void Pathfinder::addPathsFromPreviousPathfinding (STPathSet& pathsOut) -{ - // Add any result paths that aren't in mCompletePaths. - // TODO(tom): this is also quadratic in the size of the paths. - for (auto const& path : pathsOut) - { - // make sure no paths were lost - bool found = false; - if (!path.empty ()) - { - for (auto const& ePath : mCompletePaths) - { - if (ePath == path) - { - found = true; - break; - } - } - - // TODO(tom): write a test that exercises this code path. - if (!found) - mCompletePaths.push_back (path); - } - } - - WriteLog (lsDEBUG, Pathfinder) - << mCompletePaths.size () << " paths to filter"; -} - TER Pathfinder::getPathLiquidity ( STPath const& path, // IN: The path to check. STAmount const& minDstAmount, // IN: The minimum output this path must @@ -441,9 +411,6 @@ STAmount smallestUsefulAmount (STAmount const& amount, int maxPaths) void Pathfinder::computePathRanks (int maxPaths) { - if (mCompletePaths.size () <= maxPaths) - return; - mRemainingAmount = mDstAmount; // Must subtract liquidity in default path from remaining amount. @@ -479,34 +446,7 @@ void Pathfinder::computePathRanks (int maxPaths) WriteLog (lsDEBUG, Pathfinder) << "Default path causes exception"; } - // Ignore paths that move only very small amounts. - auto saMinDstAmount = smallestUsefulAmount (mDstAmount, maxPaths); - - // Get the PathRank for each path. - for (int i = 0; i < mCompletePaths.size (); ++i) - { - auto const& currentPath = mCompletePaths[i]; - STAmount liquidity; - uint64_t uQuality; - auto const resultCode = getPathLiquidity ( - currentPath, saMinDstAmount, liquidity, uQuality); - - if (resultCode != tesSUCCESS) - { - WriteLog (lsDEBUG, Pathfinder) << - "findPaths: dropping: " << transToken (resultCode) << - ": " << currentPath.getJson (0); - } - else - { - WriteLog (lsDEBUG, Pathfinder) << - "findPaths: quality: " << uQuality << - ": " << currentPath.getJson (0); - - mPathRanks.push_back ({uQuality, currentPath.size (), liquidity, i}); - } - } - std::sort (mPathRanks.begin (), mPathRanks.end (), comparePathRank); + rankPaths (maxPaths, mCompletePaths, mPathRanks); } static bool isDefaultPath (STPath const& path) @@ -540,37 +480,133 @@ static STPath removeIssuer (STPath const& path) return ret; } +// For each useful path in the input path set, +// create a ranking entry in the output vector of path ranks +void Pathfinder::rankPaths ( + int maxPaths, + STPathSet const& paths, + std::vector & rankedPaths) +{ + + rankedPaths.clear (); + rankedPaths.reserve (paths.size()); + + // Ignore paths that move only very small amounts. + auto saMinDstAmount = smallestUsefulAmount (mDstAmount, maxPaths); + + for (int i = 0; i < paths.size (); ++i) + { + auto const& currentPath = paths[i]; + STAmount liquidity; + uint64_t uQuality; + + if (currentPath.empty ()) + continue; + + auto const resultCode = getPathLiquidity ( + currentPath, saMinDstAmount, liquidity, uQuality); + + if (resultCode != tesSUCCESS) + { + WriteLog (lsDEBUG, Pathfinder) << + "findPaths: dropping : " << transToken (resultCode) << + ": " << currentPath.getJson (0); + } + else + { + WriteLog (lsDEBUG, Pathfinder) << + "findPaths: quality: " << uQuality << + ": " << currentPath.getJson (0); + + rankedPaths.push_back ({uQuality, currentPath.size (), liquidity, i}); + } + } + std::sort (rankedPaths.begin (), rankedPaths.end (), comparePathRank); +} + STPathSet Pathfinder::getBestPaths ( int maxPaths, STPath& fullLiquidityPath, + STPathSet& extraPaths, Account const& srcIssuer) { + WriteLog (lsDEBUG, Pathfinder) << "findPaths: " << + mCompletePaths.size() << " paths and " << + extraPaths.size () << " extras"; + assert (fullLiquidityPath.empty ()); const bool issuerIsSender = isXRP (mSrcCurrency) || (srcIssuer == mSrcAccount); - if (issuerIsSender && (mCompletePaths.size () <= maxPaths)) + if (issuerIsSender && + (mCompletePaths.size () <= maxPaths) && + (extraPaths.size() == 0)) return mCompletePaths; + std::vector extraPathRanks; + rankPaths (maxPaths, extraPaths, extraPathRanks); + STPathSet bestPaths; // The best PathRanks are now at the start. Pull off enough of them to // fill bestPaths, then look through the rest for the best individual // path that can satisfy the entire liquidity - if one exists. STAmount remaining = mRemainingAmount; - for (auto& pathRank: mPathRanks) + + auto pathsIterator = mPathRanks.begin(); + auto extraPathsIterator = extraPathRanks.begin(); + + while (pathsIterator != mPathRanks.end() || + extraPathsIterator != extraPathRanks.end()) { + bool usePath = false; + bool useExtraPath = false; + + if (pathsIterator == mPathRanks.end()) + useExtraPath = true; + else if (extraPathsIterator == extraPathRanks.end()) + usePath = true; + else if (extraPathsIterator->quality < pathsIterator->quality) + useExtraPath = true; + else if (extraPathsIterator->quality > pathsIterator->quality) + usePath = true; + else if (extraPathsIterator->liquidity > pathsIterator->liquidity) + useExtraPath = true; + else if (extraPathsIterator->liquidity < pathsIterator->liquidity) + usePath = true; + else + { + // Risk is high they have identical liquidity + useExtraPath = true; + usePath = true; + } + + auto& pathRank = usePath ? *pathsIterator : *extraPathsIterator; + + auto& path = usePath ? mCompletePaths[pathRank.index] : + extraPaths[pathRank.index]; + + if (useExtraPath) + ++extraPathsIterator; + + if (usePath) + ++pathsIterator; + auto iPathsLeft = maxPaths - bestPaths.size (); if (!(iPathsLeft > 0 || fullLiquidityPath.empty ())) break; - auto& path = mCompletePaths[pathRank.index]; - assert (!path.empty ()); if (path.empty ()) + { + assert (false); continue; + } bool startsWithIssuer = false; - if (! issuerIsSender) + + if (! issuerIsSender && usePath) { + // Need to make sure path matches issuer constraints + if (path.front ().getAccountID() != srcIssuer) continue; if (isDefaultPath (path)) diff --git a/src/ripple/app/paths/Pathfinder.h b/src/ripple/app/paths/Pathfinder.h index 186c005af..3046a1df1 100644 --- a/src/ripple/app/paths/Pathfinder.h +++ b/src/ripple/app/paths/Pathfinder.h @@ -54,9 +54,6 @@ public: bool findPaths (int searchLevel); - /** Make sure that all the input paths are included in mCompletePaths. */ - void addPathsFromPreviousPathfinding (STPathSet&); - /** Compute the rankings of the paths. */ void computePathRanks (int maxPaths); @@ -68,6 +65,7 @@ public: STPathSet getBestPaths ( int maxPaths, STPath& fullLiquidityPath, + STPathSet& extraPaths, Account const& srcIssuer); enum NodeType @@ -115,8 +113,6 @@ private: isNoRippleOut: isNoRipple - addPathsFromPreviousPathfinding - computePathRanks: rippleCalculate getPathLiquidity: @@ -167,6 +163,11 @@ private: Account const& toAccount, Currency const& currency); + void rankPaths ( + int maxPaths, + STPathSet const& paths, + std::vector & rankedPaths); + Account mSrcAccount; Account mDstAccount; STAmount mDstAmount; diff --git a/src/ripple/rpc/handlers/RipplePathFind.cpp b/src/ripple/rpc/handlers/RipplePathFind.cpp index 80317c434..8e6874a29 100644 --- a/src/ripple/rpc/handlers/RipplePathFind.cpp +++ b/src/ripple/rpc/handlers/RipplePathFind.cpp @@ -156,8 +156,8 @@ Json::Value doRipplePathFind (RPC::Context& context) ++level; } - if (context.params.isMember("depth") - && context.params["depth"].isIntegral()) + if (context.params.isMember("search_depth") + && context.params["search_depth"].isIntegral()) { int rLev = context.params["search_depth"].asInt (); if ((rLev < level) || (context.role == Role::ADMIN)) @@ -206,28 +206,19 @@ Json::Value doRipplePathFind (RPC::Context& context) return rpcError (rpcSRC_ISR_MALFORMED); } - int level = getConfig().PATH_SEARCH_OLD; - if ((getConfig().PATH_SEARCH_MAX > level) - && !getApp().getFeeTrack().isLoadedLocal()) - { - ++level; - } - if (context.params.isMember("depth") - && context.params["depth"].isIntegral()) - { - int rLev = context.params["search_depth"].asInt (); - if ((rLev < level) || (context.role == Role::ADMIN)) - level = rLev; - } - STPathSet spsComputed; if (context.params.isMember("paths")) { - STParsedJSONObject paths ("paths", context.params["paths"]); + Json::Value pathSet = Json::objectValue; + pathSet["Paths"] = context.params["paths"]; + STParsedJSONObject paths ("pathSet", pathSet); if (paths.object.get() == nullptr) return paths.error; else - spsComputed = paths.object.get()->downcast (); + { + spsComputed = paths.object.get()->getFieldPathSet (sfPaths); + WriteLog (lsTRACE, RPCHandler) << "ripple_path_find: Paths: " << spsComputed.getJson (0); + } } STPath fullLiquidityPath;