diff --git a/src/ripple/app/paths/AccountCurrencies.cpp b/src/ripple/app/paths/AccountCurrencies.cpp index 2892ff869c9..18452725b67 100644 --- a/src/ripple/app/paths/AccountCurrencies.cpp +++ b/src/ripple/app/paths/AccountCurrencies.cpp @@ -33,18 +33,23 @@ accountSourceCurrencies( if (includeXRP) currencies.insert(xrpCurrency()); - for (auto const& rspEntry : lrCache->getRippleLines(account)) + if (auto const lines = + lrCache->getRippleLines(account, LineDirection::outgoing)) { - auto& saBalance = rspEntry.getBalance(); - - // Filter out non - if (saBalance > beast::zero - // Have IOUs to send. - || (rspEntry.getLimitPeer() - // Peer extends credit. - && ((-saBalance) < rspEntry.getLimitPeer()))) // Credit left. + for (auto const& rspEntry : *lines) { - currencies.insert(saBalance.getCurrency()); + auto& saBalance = rspEntry.getBalance(); + + // Filter out non + if (saBalance > beast::zero + // Have IOUs to send. + || + (rspEntry.getLimitPeer() + // Peer extends credit. + && ((-saBalance) < rspEntry.getLimitPeer()))) // Credit left. + { + currencies.insert(saBalance.getCurrency()); + } } } @@ -64,12 +69,16 @@ accountDestCurrencies( currencies.insert(xrpCurrency()); // Even if account doesn't exist - for (auto const& rspEntry : lrCache->getRippleLines(account)) + if (auto const lines = + lrCache->getRippleLines(account, LineDirection::outgoing)) { - auto& saBalance = rspEntry.getBalance(); + for (auto const& rspEntry : *lines) + { + auto& saBalance = rspEntry.getBalance(); - if (saBalance < rspEntry.getLimit()) // Can take more - currencies.insert(saBalance.getCurrency()); + if (saBalance < rspEntry.getLimit()) // Can take more + currencies.insert(saBalance.getCurrency()); + } } currencies.erase(badCurrency()); diff --git a/src/ripple/app/paths/PathRequest.cpp b/src/ripple/app/paths/PathRequest.cpp index e5b15fd9d01..d1acb3ac1fd 100644 --- a/src/ripple/app/paths/PathRequest.cpp +++ b/src/ripple/app/paths/PathRequest.cpp @@ -748,6 +748,8 @@ PathRequest::doUpdate( jvStatus = newStatus; } + JLOG(m_journal.debug()) + << iIdentifier << " update finished " << (fast ? "fast" : "normal"); return newStatus; } diff --git a/src/ripple/app/paths/Pathfinder.cpp b/src/ripple/app/paths/Pathfinder.cpp index 4e81bebd3c3..71a4afa7563 100644 --- a/src/ripple/app/paths/Pathfinder.cpp +++ b/src/ripple/app/paths/Pathfinder.cpp @@ -708,6 +708,7 @@ int Pathfinder::getPathsOut( Currency const& currency, AccountID const& account, + LineDirection direction, bool isDstCurrency, AccountID const& dstAccount, std::function const& continueCallback) @@ -735,33 +736,37 @@ Pathfinder::getPathsOut( { count = app_.getOrderBookDB().getBookSize(issue); - for (auto const& rspEntry : mRLCache->getRippleLines(account)) + if (auto const lines = mRLCache->getRippleLines(account, direction)) { - if (currency != rspEntry.getLimit().getCurrency()) + for (auto const& rspEntry : *lines) { - } - else if ( - rspEntry.getBalance() <= beast::zero && - (!rspEntry.getLimitPeer() || - -rspEntry.getBalance() >= rspEntry.getLimitPeer() || - (bAuthRequired && !rspEntry.getAuth()))) - { - } - else if (isDstCurrency && dstAccount == rspEntry.getAccountIDPeer()) - { - count += 10000; // count a path to the destination extra - } - else if (rspEntry.getNoRipplePeer()) - { - // This probably isn't a useful path out - } - else if (rspEntry.getFreezePeer()) - { - // Not a useful path out - } - else - { - ++count; + if (currency != rspEntry.getLimit().getCurrency()) + { + } + else if ( + rspEntry.getBalance() <= beast::zero && + (!rspEntry.getLimitPeer() || + -rspEntry.getBalance() >= rspEntry.getLimitPeer() || + (bAuthRequired && !rspEntry.getAuth()))) + { + } + else if ( + isDstCurrency && dstAccount == rspEntry.getAccountIDPeer()) + { + count += 10000; // count a path to the destination extra + } + else if (rspEntry.getNoRipplePeer()) + { + // This probably isn't a useful path out + } + else if (rspEntry.getFreezePeer()) + { + // Not a useful path out + } + else + { + ++count; + } } } } @@ -976,117 +981,128 @@ Pathfinder::addLink( bool const bIsNoRippleOut(isNoRippleOut(currentPath)); bool const bDestOnly(addFlags & afAC_LAST); - auto& rippleLines(mRLCache->getRippleLines(uEndAccount)); - - AccountCandidates candidates; - candidates.reserve(rippleLines.size()); - - for (auto const& rs : rippleLines) + if (auto const lines = mRLCache->getRippleLines( + uEndAccount, + bIsNoRippleOut ? LineDirection::incoming + : LineDirection::outgoing)) { - if (continueCallback && !continueCallback()) - return; - auto const& acct = rs.getAccountIDPeer(); + auto& rippleLines = *lines; - if (hasEffectiveDestination && (acct == mDstAccount)) - { - // We skipped the gateway - continue; - } - - bool bToDestination = acct == mEffectiveDst; + AccountCandidates candidates; + candidates.reserve(rippleLines.size()); - if (bDestOnly && !bToDestination) + for (auto const& rs : rippleLines) { - continue; - } + if (continueCallback && !continueCallback()) + return; + auto const& acct = rs.getAccountIDPeer(); + LineDirection const direction = rs.getDirectionPeer(); - if ((uEndCurrency == rs.getLimit().getCurrency()) && - !currentPath.hasSeen(acct, uEndCurrency, acct)) - { - // path is for correct currency and has not been seen - if (rs.getBalance() <= beast::zero && - (!rs.getLimitPeer() || - -rs.getBalance() >= rs.getLimitPeer() || - (bRequireAuth && !rs.getAuth()))) + if (hasEffectiveDestination && (acct == mDstAccount)) { - // path has no credit + // We skipped the gateway + continue; } - else if (bIsNoRippleOut && rs.getNoRipple()) + + bool bToDestination = acct == mEffectiveDst; + + if (bDestOnly && !bToDestination) { - // Can't leave on this path + continue; } - else if (bToDestination) + + if ((uEndCurrency == rs.getLimit().getCurrency()) && + !currentPath.hasSeen(acct, uEndCurrency, acct)) { - // destination is always worth trying - if (uEndCurrency == mDstAmount.getCurrency()) + // path is for correct currency and has not been + // seen + if (rs.getBalance() <= beast::zero && + (!rs.getLimitPeer() || + -rs.getBalance() >= rs.getLimitPeer() || + (bRequireAuth && !rs.getAuth()))) + { + // path has no credit + } + else if (bIsNoRippleOut && rs.getNoRipple()) + { + // Can't leave on this path + } + else if (bToDestination) { - // this is a complete path - if (!currentPath.empty()) + // destination is always worth trying + if (uEndCurrency == mDstAmount.getCurrency()) { - JLOG(j_.trace()) - << "complete path found ae: " - << currentPath.getJson( - JsonOptions::none); - addUniquePath(mCompletePaths, currentPath); + // this is a complete path + if (!currentPath.empty()) + { + JLOG(j_.trace()) + << "complete path found ae: " + << currentPath.getJson( + JsonOptions::none); + addUniquePath( + mCompletePaths, currentPath); + } + } + else if (!bDestOnly) + { + // this is a high-priority candidate + candidates.push_back( + {AccountCandidate::highPriority, acct}); } } - else if (!bDestOnly) + else if (acct == mSrcAccount) { - // this is a high-priority candidate - candidates.push_back( - {AccountCandidate::highPriority, acct}); + // going back to the source is bad + } + else + { + // save this candidate + int out = getPathsOut( + uEndCurrency, + acct, + direction, + bIsEndCurrency, + mEffectiveDst, + continueCallback); + if (out) + candidates.push_back({out, acct}); } - } - else if (acct == mSrcAccount) - { - // going back to the source is bad - } - else - { - // save this candidate - int out = getPathsOut( - uEndCurrency, - acct, - bIsEndCurrency, - mEffectiveDst, - continueCallback); - if (out) - candidates.push_back({out, acct}); } } - } - if (!candidates.empty()) - { - std::sort( - candidates.begin(), - candidates.end(), - std::bind( - compareAccountCandidate, - mLedger->seq(), - std::placeholders::_1, - std::placeholders::_2)); - - int count = candidates.size(); - // allow more paths from source - if ((count > 10) && (uEndAccount != mSrcAccount)) - count = 10; - else if (count > 50) - count = 50; - - auto it = candidates.begin(); - while (count-- != 0) + if (!candidates.empty()) { - if (continueCallback && !continueCallback()) - return; - // Add accounts to incompletePaths - STPathElement pathElement( - STPathElement::typeAccount, - it->account, - uEndCurrency, - it->account); - incompletePaths.assembleAdd(currentPath, pathElement); - ++it; + std::sort( + candidates.begin(), + candidates.end(), + std::bind( + compareAccountCandidate, + mLedger->seq(), + std::placeholders::_1, + std::placeholders::_2)); + + int count = candidates.size(); + // allow more paths from source + if ((count > 10) && (uEndAccount != mSrcAccount)) + count = 10; + else if (count > 50) + count = 50; + + auto it = candidates.begin(); + while (count-- != 0) + { + if (continueCallback && !continueCallback()) + return; + // Add accounts to incompletePaths + STPathElement pathElement( + STPathElement::typeAccount, + it->account, + uEndCurrency, + it->account); + incompletePaths.assembleAdd( + currentPath, pathElement); + ++it; + } } } } diff --git a/src/ripple/app/paths/Pathfinder.h b/src/ripple/app/paths/Pathfinder.h index 45da9ec1126..375e5e24677 100644 --- a/src/ripple/app/paths/Pathfinder.h +++ b/src/ripple/app/paths/Pathfinder.h @@ -144,6 +144,7 @@ class Pathfinder : public CountedObject getPathsOut( Currency const& currency, AccountID const& account, + LineDirection direction, bool isDestCurrency, AccountID const& dest, std::function const& continueCallback); diff --git a/src/ripple/app/paths/RippleLineCache.cpp b/src/ripple/app/paths/RippleLineCache.cpp index a0b26ba2841..2487924ff0e 100644 --- a/src/ripple/app/paths/RippleLineCache.cpp +++ b/src/ripple/app/paths/RippleLineCache.cpp @@ -26,39 +26,101 @@ namespace ripple { RippleLineCache::RippleLineCache( std::shared_ptr const& ledger, beast::Journal j) - : journal_(j) + : ledger_(ledger), journal_(j) { - mLedger = ledger; - - JLOG(journal_.debug()) << "RippleLineCache created for ledger " - << mLedger->info().seq; + JLOG(journal_.debug()) << "created for ledger " << ledger_->info().seq; } RippleLineCache::~RippleLineCache() { - JLOG(journal_.debug()) << "~RippleLineCache destroyed for ledger " - << mLedger->info().seq << " with " << lines_.size() - << " accounts"; + JLOG(journal_.debug()) << "destroyed for ledger " << ledger_->info().seq + << " with " << lines_.size() << " accounts and " + << totalLineCount_ << " distinct trust lines."; } -std::vector const& -RippleLineCache::getRippleLines(AccountID const& accountID) +std::shared_ptr> +RippleLineCache::getRippleLines( + AccountID const& accountID, + LineDirection direction) { - AccountKey key(accountID, hasher_(accountID)); + auto const hash = hasher_(accountID); + AccountKey key(accountID, direction, hash); + AccountKey otherkey( + accountID, + direction == LineDirection::outgoing ? LineDirection::incoming + : LineDirection::outgoing, + hash); std::lock_guard sl(mLock); - auto [it, inserted] = lines_.emplace(key, std::vector()); + auto [it, inserted] = [&]() { + if (auto otheriter = lines_.find(otherkey); otheriter != lines_.end()) + { + // The whole point of using the direction flag is to reduce the + // number of trust line objects held in memory. Ensure that there is + // only a single set of trustlines in the cache per account. + auto const size = otheriter->second ? otheriter->second->size() : 0; + JLOG(journal_.info()) + << "Request for " + << (direction == LineDirection::outgoing ? "outgoing" + : "incoming") + << " trust lines for account " << accountID << " found " << size + << (direction == LineDirection::outgoing ? " incoming" + : " outgoing") + << " trust lines. " + << (direction == LineDirection::outgoing + ? "Deleting the subset of incoming" + : "Returning the superset of outgoing") + << " trust lines. "; + if (direction == LineDirection::outgoing) + { + // This request is for the outgoing set, but there is already a + // subset of incoming lines in the cache. Erase that subset + // to be replaced by the full set. The full set will be built + // below, and will be returned, if needed, on subsequent calls + // for either value of outgoing. + assert(size <= totalLineCount_); + totalLineCount_ -= size; + lines_.erase(otheriter); + } + else + { + // This request is for the incoming set, but there is + // already a superset of the outgoing trust lines in the cache. + // The path finding engine will disregard the non-rippling trust + // lines, so to prevent them from being stored twice, return the + // outgoing set. + key = otherkey; + return std::pair{otheriter, false}; + } + } + return lines_.emplace(key, nullptr); + }(); if (inserted) - it->second = PathFindTrustLine::getItems(accountID, *mLedger); + { + assert(it->second == nullptr); + auto lines = + PathFindTrustLine::getItems(accountID, *ledger_, direction); + if (lines.size()) + { + it->second = std::make_shared>( + std::move(lines)); + totalLineCount_ += it->second->size(); + } + } - JLOG(journal_.debug()) << "RippleLineCache getRippleLines for ledger " - << mLedger->info().seq << " found " - << it->second.size() << " lines for " - << (inserted ? "new " : "existing ") << accountID - << " out of a total of " << lines_.size() - << " accounts"; + assert(!it->second || (it->second->size() > 0)); + auto const size = it->second ? it->second->size() : 0; + JLOG(journal_.trace()) << "getRippleLines for ledger " + << ledger_->info().seq << " found " << size + << (key.direction_ == LineDirection::outgoing + ? " outgoing" + : " incoming") + << " lines for " << (inserted ? "new " : "existing ") + << accountID << " out of a total of " + << lines_.size() << " accounts and " + << totalLineCount_ << " trust lines"; return it->second; } diff --git a/src/ripple/app/paths/RippleLineCache.h b/src/ripple/app/paths/RippleLineCache.h index e7a7e0f74a3..590c50082f7 100644 --- a/src/ripple/app/paths/RippleLineCache.h +++ b/src/ripple/app/paths/RippleLineCache.h @@ -44,27 +44,43 @@ class RippleLineCache final : public CountedObject std::shared_ptr const& getLedger() const { - return mLedger; + return ledger_; } - std::vector const& - getRippleLines(AccountID const& accountID); + /** Find the trust lines associated with an account. + + @param accountID The account + @param direction Whether the account is an "outgoing" link on the path. + "Outgoing" is defined as the source account, or an account found via a + trustline that has rippling enabled on the @accountID's side. If an + account is "outgoing", all trust lines will be returned. If an account is + not "outgoing", then any trust lines that don't have rippling enabled are + not usable, so only return trust lines that have rippling enabled on + @accountID's side. + @return Returns a vector of the usable trust lines. + */ + std::shared_ptr> + getRippleLines(AccountID const& accountID, LineDirection direction); private: std::mutex mLock; ripple::hardened_hash<> hasher_; - std::shared_ptr mLedger; + std::shared_ptr ledger_; beast::Journal journal_; struct AccountKey final : public CountedObject { AccountID account_; + LineDirection direction_; std::size_t hash_value_; - AccountKey(AccountID const& account, std::size_t hash) - : account_(account), hash_value_(hash) + AccountKey( + AccountID const& account, + LineDirection direction, + std::size_t hash) + : account_(account), direction_(direction), hash_value_(hash) { } @@ -76,7 +92,8 @@ class RippleLineCache final : public CountedObject bool operator==(AccountKey const& lhs) const { - return hash_value_ == lhs.hash_value_ && account_ == lhs.account_; + return hash_value_ == lhs.hash_value_ && account_ == lhs.account_ && + direction_ == lhs.direction_; } std::size_t @@ -97,8 +114,17 @@ class RippleLineCache final : public CountedObject }; }; - hash_map, AccountKey::Hash> + // Use a shared_ptr so entries can be removed from the map safely. + // Even though a shared_ptr to a vector will take more memory just a vector, + // most accounts are not going to have any entries (estimated over 90%), so + // vectors will not need to be created for them. This should lead to far + // less memory usage overall. + hash_map< + AccountKey, + std::shared_ptr>, + AccountKey::Hash> lines_; + std::size_t totalLineCount_ = 0; }; } // namespace ripple diff --git a/src/ripple/app/paths/TrustLine.cpp b/src/ripple/app/paths/TrustLine.cpp index 12020acf714..14a5d6f8823 100644 --- a/src/ripple/app/paths/TrustLine.cpp +++ b/src/ripple/app/paths/TrustLine.cpp @@ -61,26 +61,38 @@ PathFindTrustLine::makeItem( namespace detail { template std::vector -getTrustLineItems(AccountID const& accountID, ReadView const& view) +getTrustLineItems( + AccountID const& accountID, + ReadView const& view, + LineDirection direction = LineDirection::outgoing) { std::vector items; forEachItem( view, accountID, - [&items, &accountID](std::shared_ptr const& sleCur) { + [&items, &accountID, &direction]( + std::shared_ptr const& sleCur) { auto ret = T::makeItem(accountID, sleCur); - if (ret) + if (ret && + (direction == LineDirection::outgoing || !ret->getNoRipple())) items.push_back(std::move(*ret)); }); + // This list may be around for a while, so free up any unneeded + // capacity + items.shrink_to_fit(); return items; } } // namespace detail std::vector -PathFindTrustLine::getItems(AccountID const& accountID, ReadView const& view) +PathFindTrustLine::getItems( + AccountID const& accountID, + ReadView const& view, + LineDirection direction) { - return detail::getTrustLineItems(accountID, view); + return detail::getTrustLineItems( + accountID, view, direction); } RPCTrustLine::RPCTrustLine( diff --git a/src/ripple/app/paths/TrustLine.h b/src/ripple/app/paths/TrustLine.h index 0217f0e750a..6b27dca3669 100644 --- a/src/ripple/app/paths/TrustLine.h +++ b/src/ripple/app/paths/TrustLine.h @@ -31,6 +31,15 @@ namespace ripple { +/** Describes how an account was found in a path, and how to find the next set +of paths. "Outgoing" is defined as the source account, or an account found via a +trustline that has rippling enabled on the account's side. +"Incoming" is defined as an account found via a trustline that has rippling +disabled on the account's side. Any trust lines for an incoming account that +have rippling disabled are unusable in paths. +*/ +enum class LineDirection : bool { incoming = false, outgoing = true }; + /** Wraps a trust line SLE for convenience. The complication of trust lines is that there is a "low" account and a "high" account. This wraps the @@ -109,6 +118,20 @@ class TrustLineBase return mFlags & (!mViewLowest ? lsfLowNoRipple : lsfHighNoRipple); } + LineDirection + getDirection() const + { + return getNoRipple() ? LineDirection::incoming + : LineDirection::outgoing; + } + + LineDirection + getDirectionPeer() const + { + return getNoRipplePeer() ? LineDirection::incoming + : LineDirection::outgoing; + } + /** Have we set the freeze flag on our peer */ bool getFreeze() const @@ -170,7 +193,10 @@ class PathFindTrustLine final : public TrustLineBase, makeItem(AccountID const& accountID, std::shared_ptr const& sle); static std::vector - getItems(AccountID const& accountID, ReadView const& view); + getItems( + AccountID const& accountID, + ReadView const& view, + LineDirection direction); }; // This wrapper is used for the `AccountLines` command and includes the quality diff --git a/src/test/app/Path_test.cpp b/src/test/app/Path_test.cpp index b83b0d00deb..05d23e82976 100644 --- a/src/test/app/Path_test.cpp +++ b/src/test/app/Path_test.cpp @@ -1378,6 +1378,69 @@ class Path_test : public beast::unit_test::suite } } + void + noripple_combinations() + { + using namespace jtx; + // This test will create trust lines with various values of the noRipple + // flag. alice <-> george <-> bob george will sort of act like a + // gateway, but use a different name to avoid the usual assumptions + // about gateways. + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const george = Account("george"); + auto const USD = george["USD"]; + auto test = [&](std::string casename, + bool aliceRipple, + bool bobRipple, + bool expectPath) { + testcase(casename); + + Env env = pathTestEnv(); + env.fund(XRP(10000), noripple(alice, bob, george)); + env.close(); + // Set the same flags at both ends of the trustline, even though + // only george's matter. + env(trust( + alice, + USD(100), + aliceRipple ? tfClearNoRipple : tfSetNoRipple)); + env(trust( + george, + alice["USD"](100), + aliceRipple ? tfClearNoRipple : tfSetNoRipple)); + env(trust( + bob, USD(100), bobRipple ? tfClearNoRipple : tfSetNoRipple)); + env(trust( + george, + bob["USD"](100), + bobRipple ? tfClearNoRipple : tfSetNoRipple)); + env.close(); + env(pay(george, alice, USD(70))); + env.close(); + + auto [st, sa, da] = + find_paths(env, "alice", "bob", Account("bob")["USD"](5)); + BEAST_EXPECT(equal(da, bob["USD"](5))); + + if (expectPath) + { + BEAST_EXPECT(st.size() == 1); + BEAST_EXPECT(same(st, stpath("george"))); + BEAST_EXPECT(equal(sa, alice["USD"](5))); + } + else + { + BEAST_EXPECT(st.size() == 0); + BEAST_EXPECT(equal(sa, XRP(0))); + } + }; + test("ripple -> ripple", true, true, true); + test("ripple -> no ripple", true, false, true); + test("no ripple -> ripple", false, true, true); + test("no ripple -> no ripple", false, false, false); + } + void run() override { @@ -1401,6 +1464,7 @@ class Path_test : public beast::unit_test::suite trust_auto_clear_trust_auto_clear(); xrp_to_xrp(); receive_max(); + noripple_combinations(); // The following path_find_NN tests are data driven tests // that were originally implemented in js/coffee and migrated diff --git a/src/test/rpc/AmendmentBlocked_test.cpp b/src/test/rpc/AmendmentBlocked_test.cpp index 54c87292229..a90bcdcd0c4 100644 --- a/src/test/rpc/AmendmentBlocked_test.cpp +++ b/src/test/rpc/AmendmentBlocked_test.cpp @@ -43,6 +43,12 @@ class AmendmentBlocked_test : public beast::unit_test::suite Account const ali{"ali", KeyType::secp256k1}; env.fund(XRP(10000), alice, bob, gw); env.memoize(ali); + // This close() ensures that all the accounts get created and their + // default ripple flag gets set before the trust lines are created. + // Without it, the ordering manages to create alice's trust line with + // noRipple set on gw's end. The existing tests pass either way, but + // better to do it right. + env.close(); env.trust(USD(600), alice); env.trust(USD(700), bob); env(pay(gw, alice, USD(70)));