From e836375d993e60a61c1bb084d225e182099187d2 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 9 Feb 2022 19:31:22 -0500 Subject: [PATCH] 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 2892ff869c..1323917a5a 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 e5b15fd9d0..d1acb3ac1f 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 4e81bebd3c..91f4290c43 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 45da9ec112..b176388a6f 100644 --- a/src/ripple/app/paths/Pathfinder.h +++ b/src/ripple/app/paths/Pathfinder.h @@ -144,6 +144,7 @@ private: 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 a0b26ba284..c504a474ac 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 e7a7e0f74a..d4b777aeae 100644 --- a/src/ripple/app/paths/RippleLineCache.h +++ b/src/ripple/app/paths/RippleLineCache.h @@ -47,8 +47,20 @@ public: 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 @@ private: 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 @@ private: 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 12020acf71..dbd9c57c1e 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 0217f0e750..29948feea2 100644 --- a/src/ripple/app/paths/TrustLine.h +++ b/src/ripple/app/paths/TrustLine.h @@ -170,7 +170,7 @@ public: 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 54c8729222..a90bcdcd0c 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)));