From 9cab2551deff6ba75d9db52d22df73c851b4cc9b Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 9 Feb 2022 19:31:22 -0500 Subject: [PATCH 1/4] Don't load trust lines that can't participate in path finding * "A path is considered invalid if and only if it enters and exits an address node through trust lines where No Ripple has been enabled for that address." (https://xrpl.org/rippling.html#specifics) * When loading trust lines for an account "Alice" which was reached via a trust line that has the No Ripple flag set on Alice's side, do not use or cache any of Alice's trust lines which have the No Ripple flag set on Alice's side. For typical "end-user" accounts, this will return no trust lines. --- src/ripple/app/paths/AccountCurrencies.cpp | 4 ++-- src/ripple/app/paths/PathRequest.cpp | 2 ++ src/ripple/app/paths/Pathfinder.cpp | 8 ++++++-- src/ripple/app/paths/Pathfinder.h | 1 + src/ripple/app/paths/RippleLineCache.cpp | 7 ++++--- src/ripple/app/paths/RippleLineCache.h | 22 ++++++++++++++++++---- src/ripple/app/paths/TrustLine.cpp | 18 +++++++++++++----- src/ripple/app/paths/TrustLine.h | 2 +- src/test/rpc/AmendmentBlocked_test.cpp | 6 ++++++ 9 files changed, 53 insertions(+), 17 deletions(-) diff --git a/src/ripple/app/paths/AccountCurrencies.cpp b/src/ripple/app/paths/AccountCurrencies.cpp index 2892ff869c9..1323917a5ac 100644 --- a/src/ripple/app/paths/AccountCurrencies.cpp +++ b/src/ripple/app/paths/AccountCurrencies.cpp @@ -33,7 +33,7 @@ accountSourceCurrencies( if (includeXRP) currencies.insert(xrpCurrency()); - for (auto const& rspEntry : lrCache->getRippleLines(account)) + for (auto const& rspEntry : lrCache->getRippleLines(account, true)) { auto& saBalance = rspEntry.getBalance(); @@ -64,7 +64,7 @@ accountDestCurrencies( currencies.insert(xrpCurrency()); // Even if account doesn't exist - for (auto const& rspEntry : lrCache->getRippleLines(account)) + for (auto const& rspEntry : lrCache->getRippleLines(account, true)) { auto& saBalance = rspEntry.getBalance(); 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..91f4290c435 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, + bool outgoing, bool isDstCurrency, AccountID const& dstAccount, std::function const& continueCallback) @@ -735,7 +736,7 @@ Pathfinder::getPathsOut( { count = app_.getOrderBookDB().getBookSize(issue); - for (auto const& rspEntry : mRLCache->getRippleLines(account)) + for (auto const& rspEntry : mRLCache->getRippleLines(account, outgoing)) { if (currency != rspEntry.getLimit().getCurrency()) { @@ -976,7 +977,8 @@ Pathfinder::addLink( bool const bIsNoRippleOut(isNoRippleOut(currentPath)); bool const bDestOnly(addFlags & afAC_LAST); - auto& rippleLines(mRLCache->getRippleLines(uEndAccount)); + auto& rippleLines( + mRLCache->getRippleLines(uEndAccount, !bIsNoRippleOut)); AccountCandidates candidates; candidates.reserve(rippleLines.size()); @@ -986,6 +988,7 @@ Pathfinder::addLink( if (continueCallback && !continueCallback()) return; auto const& acct = rs.getAccountIDPeer(); + bool const outgoing = !rs.getNoRipplePeer(); if (hasEffectiveDestination && (acct == mDstAccount)) { @@ -1047,6 +1050,7 @@ Pathfinder::addLink( int out = getPathsOut( uEndCurrency, acct, + outgoing, bIsEndCurrency, mEffectiveDst, continueCallback); diff --git a/src/ripple/app/paths/Pathfinder.h b/src/ripple/app/paths/Pathfinder.h index 45da9ec1126..b176388a6f8 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, + bool outgoing, 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..c504a474ac5 100644 --- a/src/ripple/app/paths/RippleLineCache.cpp +++ b/src/ripple/app/paths/RippleLineCache.cpp @@ -42,16 +42,17 @@ RippleLineCache::~RippleLineCache() } std::vector const& -RippleLineCache::getRippleLines(AccountID const& accountID) +RippleLineCache::getRippleLines(AccountID const& accountID, bool outgoing) { - AccountKey key(accountID, hasher_(accountID)); + AccountKey key( + accountID, outgoing, hasher_(std::pair{accountID, outgoing})); std::lock_guard sl(mLock); auto [it, inserted] = lines_.emplace(key, std::vector()); if (inserted) - it->second = PathFindTrustLine::getItems(accountID, *mLedger); + it->second = PathFindTrustLine::getItems(accountID, *mLedger, outgoing); JLOG(journal_.debug()) << "RippleLineCache getRippleLines for ledger " << mLedger->info().seq << " found " diff --git a/src/ripple/app/paths/RippleLineCache.h b/src/ripple/app/paths/RippleLineCache.h index e7a7e0f74a3..d4b777aeae6 100644 --- a/src/ripple/app/paths/RippleLineCache.h +++ b/src/ripple/app/paths/RippleLineCache.h @@ -47,8 +47,20 @@ class RippleLineCache final : public CountedObject return mLedger; } + /** Find the trust lines associated with an account. + + @param accountID The account + @param outgoing 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::vector const& - getRippleLines(AccountID const& accountID); + getRippleLines(AccountID const& accountID, bool outgoing); private: std::mutex mLock; @@ -61,10 +73,11 @@ class RippleLineCache final : public CountedObject struct AccountKey final : public CountedObject { AccountID account_; + bool outgoing_; std::size_t hash_value_; - AccountKey(AccountID const& account, std::size_t hash) - : account_(account), hash_value_(hash) + AccountKey(AccountID const& account, bool outgoing, std::size_t hash) + : account_(account), outgoing_(outgoing), hash_value_(hash) { } @@ -76,7 +89,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_ && + outgoing_ == lhs.outgoing_; } std::size_t diff --git a/src/ripple/app/paths/TrustLine.cpp b/src/ripple/app/paths/TrustLine.cpp index 12020acf714..dbd9c57c1e2 100644 --- a/src/ripple/app/paths/TrustLine.cpp +++ b/src/ripple/app/paths/TrustLine.cpp @@ -61,15 +61,19 @@ PathFindTrustLine::makeItem( namespace detail { template std::vector -getTrustLineItems(AccountID const& accountID, ReadView const& view) +getTrustLineItems( + AccountID const& accountID, + ReadView const& view, + bool outgoing = true) { std::vector items; forEachItem( view, accountID, - [&items, &accountID](std::shared_ptr const& sleCur) { + [&items, &accountID, &outgoing]( + std::shared_ptr const& sleCur) { auto ret = T::makeItem(accountID, sleCur); - if (ret) + if (ret && (outgoing || !ret->getNoRipple())) items.push_back(std::move(*ret)); }); @@ -78,9 +82,13 @@ getTrustLineItems(AccountID const& accountID, ReadView const& view) } // namespace detail std::vector -PathFindTrustLine::getItems(AccountID const& accountID, ReadView const& view) +PathFindTrustLine::getItems( + AccountID const& accountID, + ReadView const& view, + bool outgoing) { - return detail::getTrustLineItems(accountID, view); + return detail::getTrustLineItems( + accountID, view, outgoing); } RPCTrustLine::RPCTrustLine( diff --git a/src/ripple/app/paths/TrustLine.h b/src/ripple/app/paths/TrustLine.h index 0217f0e750a..29948feea2e 100644 --- a/src/ripple/app/paths/TrustLine.h +++ b/src/ripple/app/paths/TrustLine.h @@ -170,7 +170,7 @@ 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, bool outgoing); }; // This wrapper is used for the `AccountLines` command and includes the quality 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))); From 33da40453b2e03abc5afcc187ea920ae3f6a8e29 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 18 Feb 2022 18:28:49 -0500 Subject: [PATCH 2/4] Track total trustlines, avoid duplications --- src/ripple/app/paths/AccountCurrencies.cpp | 35 +-- src/ripple/app/paths/Pathfinder.cpp | 248 +++++++++++---------- src/ripple/app/paths/RippleLineCache.cpp | 85 +++++-- src/ripple/app/paths/RippleLineCache.h | 17 +- src/ripple/app/paths/TrustLine.cpp | 3 + 5 files changed, 232 insertions(+), 156 deletions(-) diff --git a/src/ripple/app/paths/AccountCurrencies.cpp b/src/ripple/app/paths/AccountCurrencies.cpp index 1323917a5ac..e0716f955ba 100644 --- a/src/ripple/app/paths/AccountCurrencies.cpp +++ b/src/ripple/app/paths/AccountCurrencies.cpp @@ -33,18 +33,22 @@ accountSourceCurrencies( if (includeXRP) currencies.insert(xrpCurrency()); - for (auto const& rspEntry : lrCache->getRippleLines(account, true)) + if (auto const lines = lrCache->getRippleLines(account, true)) { - 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 +68,15 @@ accountDestCurrencies( currencies.insert(xrpCurrency()); // Even if account doesn't exist - for (auto const& rspEntry : lrCache->getRippleLines(account, true)) + if (auto const lines = lrCache->getRippleLines(account, true)) { - 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/Pathfinder.cpp b/src/ripple/app/paths/Pathfinder.cpp index 91f4290c435..dededae78f8 100644 --- a/src/ripple/app/paths/Pathfinder.cpp +++ b/src/ripple/app/paths/Pathfinder.cpp @@ -736,33 +736,37 @@ Pathfinder::getPathsOut( { count = app_.getOrderBookDB().getBookSize(issue); - for (auto const& rspEntry : mRLCache->getRippleLines(account, outgoing)) + if (auto const lines = mRLCache->getRippleLines(account, outgoing)) { - 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; + } } } } @@ -977,120 +981,126 @@ Pathfinder::addLink( bool const bIsNoRippleOut(isNoRippleOut(currentPath)); bool const bDestOnly(addFlags & afAC_LAST); - auto& rippleLines( - mRLCache->getRippleLines(uEndAccount, !bIsNoRippleOut)); - - AccountCandidates candidates; - candidates.reserve(rippleLines.size()); - - for (auto const& rs : rippleLines) + if (auto const lines = + mRLCache->getRippleLines(uEndAccount, !bIsNoRippleOut)) { - if (continueCallback && !continueCallback()) - return; - auto const& acct = rs.getAccountIDPeer(); - bool const outgoing = !rs.getNoRipplePeer(); + 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(); + bool const outgoing = !rs.getNoRipplePeer(); - 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, + outgoing, + 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, - outgoing, - 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/RippleLineCache.cpp b/src/ripple/app/paths/RippleLineCache.cpp index c504a474ac5..f7566189257 100644 --- a/src/ripple/app/paths/RippleLineCache.cpp +++ b/src/ripple/app/paths/RippleLineCache.cpp @@ -26,40 +26,87 @@ 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& +std::shared_ptr> RippleLineCache::getRippleLines(AccountID const& accountID, bool outgoing) { - AccountKey key( - accountID, outgoing, hasher_(std::pair{accountID, outgoing})); + auto const hash = hasher_(accountID); + AccountKey key(accountID, outgoing, hash); + AccountKey otherkey(accountID, !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 outgoing 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 " << (outgoing ? "outgoing" : "incoming") + << " trust lines for account " << accountID << " found " << size + << (outgoing ? " incoming" : " outgoing") << " trust lines. " + << (outgoing ? "Deleting the subset of incoming" + : "Returning the superset of outgoing") + << " trust lines. "; + if (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, outgoing); + { + assert(it->second == nullptr); + auto lines = PathFindTrustLine::getItems(accountID, *ledger_, outgoing); + 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.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 d4b777aeae6..ee5cbbb6420 100644 --- a/src/ripple/app/paths/RippleLineCache.h +++ b/src/ripple/app/paths/RippleLineCache.h @@ -44,7 +44,7 @@ class RippleLineCache final : public CountedObject std::shared_ptr const& getLedger() const { - return mLedger; + return ledger_; } /** Find the trust lines associated with an account. @@ -59,14 +59,14 @@ class RippleLineCache final : public CountedObject @accountID's side. @return Returns a vector of the usable trust lines. */ - std::vector const& + std::shared_ptr> getRippleLines(AccountID const& accountID, bool outgoing); private: std::mutex mLock; ripple::hardened_hash<> hasher_; - std::shared_ptr mLedger; + std::shared_ptr ledger_; beast::Journal journal_; @@ -111,8 +111,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 dbd9c57c1e2..55f3267642e 100644 --- a/src/ripple/app/paths/TrustLine.cpp +++ b/src/ripple/app/paths/TrustLine.cpp @@ -76,6 +76,9 @@ getTrustLineItems( if (ret && (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; } From 374b7ec4942090fe854a1e146d0035d9fdb4fe05 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 21 Mar 2022 18:52:25 -0400 Subject: [PATCH 3/4] [FOLD] Convert the "outgoing" bool to an enum * (Naming things is Hard) --- src/ripple/app/paths/AccountCurrencies.cpp | 6 ++-- src/ripple/app/paths/Pathfinder.cpp | 14 +++++---- src/ripple/app/paths/Pathfinder.h | 2 +- src/ripple/app/paths/RippleLineCache.cpp | 36 +++++++++++++++------- src/ripple/app/paths/RippleLineCache.h | 15 +++++---- src/ripple/app/paths/TrustLine.cpp | 11 ++++--- src/ripple/app/paths/TrustLine.h | 28 ++++++++++++++++- 7 files changed, 80 insertions(+), 32 deletions(-) diff --git a/src/ripple/app/paths/AccountCurrencies.cpp b/src/ripple/app/paths/AccountCurrencies.cpp index e0716f955ba..18452725b67 100644 --- a/src/ripple/app/paths/AccountCurrencies.cpp +++ b/src/ripple/app/paths/AccountCurrencies.cpp @@ -33,7 +33,8 @@ accountSourceCurrencies( if (includeXRP) currencies.insert(xrpCurrency()); - if (auto const lines = lrCache->getRippleLines(account, true)) + if (auto const lines = + lrCache->getRippleLines(account, LineDirection::outgoing)) { for (auto const& rspEntry : *lines) { @@ -68,7 +69,8 @@ accountDestCurrencies( currencies.insert(xrpCurrency()); // Even if account doesn't exist - if (auto const lines = lrCache->getRippleLines(account, true)) + if (auto const lines = + lrCache->getRippleLines(account, LineDirection::outgoing)) { for (auto const& rspEntry : *lines) { diff --git a/src/ripple/app/paths/Pathfinder.cpp b/src/ripple/app/paths/Pathfinder.cpp index dededae78f8..71a4afa7563 100644 --- a/src/ripple/app/paths/Pathfinder.cpp +++ b/src/ripple/app/paths/Pathfinder.cpp @@ -708,7 +708,7 @@ int Pathfinder::getPathsOut( Currency const& currency, AccountID const& account, - bool outgoing, + LineDirection direction, bool isDstCurrency, AccountID const& dstAccount, std::function const& continueCallback) @@ -736,7 +736,7 @@ Pathfinder::getPathsOut( { count = app_.getOrderBookDB().getBookSize(issue); - if (auto const lines = mRLCache->getRippleLines(account, outgoing)) + if (auto const lines = mRLCache->getRippleLines(account, direction)) { for (auto const& rspEntry : *lines) { @@ -981,8 +981,10 @@ Pathfinder::addLink( bool const bIsNoRippleOut(isNoRippleOut(currentPath)); bool const bDestOnly(addFlags & afAC_LAST); - if (auto const lines = - mRLCache->getRippleLines(uEndAccount, !bIsNoRippleOut)) + if (auto const lines = mRLCache->getRippleLines( + uEndAccount, + bIsNoRippleOut ? LineDirection::incoming + : LineDirection::outgoing)) { auto& rippleLines = *lines; @@ -994,7 +996,7 @@ Pathfinder::addLink( if (continueCallback && !continueCallback()) return; auto const& acct = rs.getAccountIDPeer(); - bool const outgoing = !rs.getNoRipplePeer(); + LineDirection const direction = rs.getDirectionPeer(); if (hasEffectiveDestination && (acct == mDstAccount)) { @@ -1058,7 +1060,7 @@ Pathfinder::addLink( int out = getPathsOut( uEndCurrency, acct, - outgoing, + direction, bIsEndCurrency, mEffectiveDst, continueCallback); diff --git a/src/ripple/app/paths/Pathfinder.h b/src/ripple/app/paths/Pathfinder.h index b176388a6f8..375e5e24677 100644 --- a/src/ripple/app/paths/Pathfinder.h +++ b/src/ripple/app/paths/Pathfinder.h @@ -144,7 +144,7 @@ class Pathfinder : public CountedObject getPathsOut( Currency const& currency, AccountID const& account, - bool outgoing, + 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 f7566189257..2487924ff0e 100644 --- a/src/ripple/app/paths/RippleLineCache.cpp +++ b/src/ripple/app/paths/RippleLineCache.cpp @@ -39,29 +39,40 @@ RippleLineCache::~RippleLineCache() } std::shared_ptr> -RippleLineCache::getRippleLines(AccountID const& accountID, bool outgoing) +RippleLineCache::getRippleLines( + AccountID const& accountID, + LineDirection direction) { auto const hash = hasher_(accountID); - AccountKey key(accountID, outgoing, hash); - AccountKey otherkey(accountID, !outgoing, hash); + AccountKey key(accountID, direction, hash); + AccountKey otherkey( + accountID, + direction == LineDirection::outgoing ? LineDirection::incoming + : LineDirection::outgoing, + hash); std::lock_guard sl(mLock); auto [it, inserted] = [&]() { if (auto otheriter = lines_.find(otherkey); otheriter != lines_.end()) { - // The whole point of using the outgoing flag is to reduce the + // 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 " << (outgoing ? "outgoing" : "incoming") + << "Request for " + << (direction == LineDirection::outgoing ? "outgoing" + : "incoming") << " trust lines for account " << accountID << " found " << size - << (outgoing ? " incoming" : " outgoing") << " trust lines. " - << (outgoing ? "Deleting the subset of incoming" - : "Returning the superset of outgoing") + << (direction == LineDirection::outgoing ? " incoming" + : " outgoing") + << " trust lines. " + << (direction == LineDirection::outgoing + ? "Deleting the subset of incoming" + : "Returning the superset of outgoing") << " trust lines. "; - if (outgoing) + 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 @@ -89,7 +100,8 @@ RippleLineCache::getRippleLines(AccountID const& accountID, bool outgoing) if (inserted) { assert(it->second == nullptr); - auto lines = PathFindTrustLine::getItems(accountID, *ledger_, outgoing); + auto lines = + PathFindTrustLine::getItems(accountID, *ledger_, direction); if (lines.size()) { it->second = std::make_shared>( @@ -102,7 +114,9 @@ RippleLineCache::getRippleLines(AccountID const& accountID, bool outgoing) auto const size = it->second ? it->second->size() : 0; JLOG(journal_.trace()) << "getRippleLines for ledger " << ledger_->info().seq << " found " << size - << (key.outgoing_ ? " outgoing" : " incoming") + << (key.direction_ == LineDirection::outgoing + ? " outgoing" + : " incoming") << " lines for " << (inserted ? "new " : "existing ") << accountID << " out of a total of " << lines_.size() << " accounts and " diff --git a/src/ripple/app/paths/RippleLineCache.h b/src/ripple/app/paths/RippleLineCache.h index ee5cbbb6420..590c50082f7 100644 --- a/src/ripple/app/paths/RippleLineCache.h +++ b/src/ripple/app/paths/RippleLineCache.h @@ -50,7 +50,7 @@ class RippleLineCache final : public CountedObject /** Find the trust lines associated with an account. @param accountID The account - @param outgoing Whether the account is an "outgoing" link on the path. + @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 @@ -60,7 +60,7 @@ class RippleLineCache final : public CountedObject @return Returns a vector of the usable trust lines. */ std::shared_ptr> - getRippleLines(AccountID const& accountID, bool outgoing); + getRippleLines(AccountID const& accountID, LineDirection direction); private: std::mutex mLock; @@ -73,11 +73,14 @@ class RippleLineCache final : public CountedObject struct AccountKey final : public CountedObject { AccountID account_; - bool outgoing_; + LineDirection direction_; std::size_t hash_value_; - AccountKey(AccountID const& account, bool outgoing, std::size_t hash) - : account_(account), outgoing_(outgoing), hash_value_(hash) + AccountKey( + AccountID const& account, + LineDirection direction, + std::size_t hash) + : account_(account), direction_(direction), hash_value_(hash) { } @@ -90,7 +93,7 @@ class RippleLineCache final : public CountedObject operator==(AccountKey const& lhs) const { return hash_value_ == lhs.hash_value_ && account_ == lhs.account_ && - outgoing_ == lhs.outgoing_; + direction_ == lhs.direction_; } std::size_t diff --git a/src/ripple/app/paths/TrustLine.cpp b/src/ripple/app/paths/TrustLine.cpp index 55f3267642e..14a5d6f8823 100644 --- a/src/ripple/app/paths/TrustLine.cpp +++ b/src/ripple/app/paths/TrustLine.cpp @@ -64,16 +64,17 @@ std::vector getTrustLineItems( AccountID const& accountID, ReadView const& view, - bool outgoing = true) + LineDirection direction = LineDirection::outgoing) { std::vector items; forEachItem( view, accountID, - [&items, &accountID, &outgoing]( + [&items, &accountID, &direction]( std::shared_ptr const& sleCur) { auto ret = T::makeItem(accountID, sleCur); - if (ret && (outgoing || !ret->getNoRipple())) + 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 @@ -88,10 +89,10 @@ std::vector PathFindTrustLine::getItems( AccountID const& accountID, ReadView const& view, - bool outgoing) + LineDirection direction) { return detail::getTrustLineItems( - accountID, view, outgoing); + accountID, view, direction); } RPCTrustLine::RPCTrustLine( diff --git a/src/ripple/app/paths/TrustLine.h b/src/ripple/app/paths/TrustLine.h index 29948feea2e..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, bool outgoing); + getItems( + AccountID const& accountID, + ReadView const& view, + LineDirection direction); }; // This wrapper is used for the `AccountLines` command and includes the quality From 3355bb732ffa284a4101cbbb287f41d837fbd38a Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 22 Mar 2022 18:39:18 -0400 Subject: [PATCH 4/4] [FOLD] Add unit tests for the different values of the noRipple flag * Also remove an unused variable that's giving VS a problem --- src/test/app/Path_test.cpp | 64 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) 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