diff --git a/include/xrpl/protocol/STPathSet.h b/include/xrpl/protocol/STPathSet.h index 23f4e653c4..d527e2479f 100644 --- a/include/xrpl/protocol/STPathSet.h +++ b/include/xrpl/protocol/STPathSet.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include #include @@ -108,6 +109,9 @@ public: [[nodiscard]] bool isType(Type const& pe) const; + [[nodiscard]] size_t + getHash() const; + bool operator==(STPathElement const& t) const; @@ -171,12 +175,23 @@ public: reserve(size_t s); }; +template +void +hash_append(Hasher& h, STPath const& p) noexcept +{ + for (auto const& e : p) + { + beast::hash_append(h, e.getHash()); + } +} + //------------------------------------------------------------------------------ // A set of zero or more payment paths class STPathSet final : public STBase, public CountedObject { std::vector value_; + xrpl::hardened_hash_set seenHashes_; public: STPathSet() = default; @@ -205,9 +220,6 @@ public: std::vector::const_reference operator[](std::vector::size_type n) const; - std::vector::reference - operator[](std::vector::size_type n); - [[nodiscard]] std::vector::const_iterator begin() const; @@ -227,6 +239,9 @@ public: void emplaceBack(Args&&... args); + [[nodiscard]] bool + contains(STPath const& path) const; + private: STBase* copy(std::size_t n, void* buf) const override; @@ -515,12 +530,6 @@ STPathSet::operator[](std::vector::size_type n) const return value_[n]; } -inline std::vector::reference -STPathSet::operator[](std::vector::size_type n) -{ - return value_[n]; -} - inline std::vector::const_iterator STPathSet::begin() const { @@ -549,6 +558,7 @@ inline void STPathSet::pushBack(STPath const& e) { value_.push_back(e); + seenHashes_.emplace(value_.back()); } template @@ -556,6 +566,13 @@ inline void STPathSet::emplaceBack(Args&&... args) { value_.emplace_back(std::forward(args)...); + seenHashes_.emplace(value_.back()); +} + +inline bool +STPathSet::contains(STPath const& path) const +{ + return seenHashes_.contains(path); } } // namespace xrpl diff --git a/src/libxrpl/protocol/STPathSet.cpp b/src/libxrpl/protocol/STPathSet.cpp index 8987d05f1e..658aaa65dd 100644 --- a/src/libxrpl/protocol/STPathSet.cpp +++ b/src/libxrpl/protocol/STPathSet.cpp @@ -51,6 +51,12 @@ STPathElement::getHash(STPathElement const& element) return (hashAccount ^ hashCurrency ^ hashIssuer); } +[[nodiscard]] size_t +STPathElement::getHash() const +{ + return STPathElement::getHash(*this); +} + STPathSet::STPathSet(SerialIter& sit, SField const& name) : STBase(name) { std::vector path; @@ -126,21 +132,15 @@ STPathSet::move(std::size_t n, void* buf) bool STPathSet::assembleAdd(STPath const& base, STPathElement const& tail) { // assemble base+tail and add it to the set if it's not a duplicate - value_.push_back(base); + STPath combined = base; + combined.pushBack(tail); - auto it = value_.rbegin(); - - STPath& newPath = *it; - newPath.pushBack(tail); - - while (++it != value_.rend()) + if (!seenHashes_.insert(combined).second) { - if (*it == newPath) - { - value_.pop_back(); - return false; - } + return false; } + + value_.push_back(std::move(combined)); return true; } diff --git a/src/test/app/Path_test.cpp b/src/test/app/Path_test.cpp index 8f19a419a0..d6e4fec60b 100644 --- a/src/test/app/Path_test.cpp +++ b/src/test/app/Path_test.cpp @@ -53,6 +53,7 @@ #include #include #include +#include #include #include @@ -1866,6 +1867,103 @@ public: BEAST_EXPECT(same(st, stpath(gw_, ipe(xrpIssue())))); } + void + testAssembleAddDeduplication() + { + testcase("STPathSet::assembleAdd deduplication — O(N^2) regression"); + + static constexpr std::string_view kAccount1 = "A3F19C7B2E5D08146FB93A7C0E2D5184BC6F3A09"; + static constexpr std::string_view kAccount2 = "1D7E4B90C2A6F3851E0B9D47A2C5F8136E0A4B7D"; + static constexpr std::string_view kAccount3 = "F08C36A1D95E27B40CA1F63E8D204B7950E1C3A6"; + static constexpr std::string_view kAccount4 = "4B6209E7F1A3C85D0E94B27Af3D6018C5A7E92B4"; + static constexpr std::string_view kAccount5 = "9E2D7041BCA3F6589D013E7B2A4C6F80159D3E7A"; + static constexpr std::string_view kAccount6 = "7C5A91E384F2D06BA19C4E73D820F516B3A9C0E4"; + static constexpr std::string_view kAccount7 = "2F8B043C6A1E9D75B0C38E14F6A2D509731BC4E8"; + static constexpr std::string_view kAccount8 = "E61D9A30F47C285BA0D31E96C7B4F802513A8D6F"; + + static constexpr AccountID kAccountID1{kAccount1}; + static constexpr AccountID kAccountID2{kAccount2}; + static constexpr AccountID kAccountID3{kAccount3}; + static constexpr AccountID kAccountID4{kAccount4}; + static constexpr AccountID kAccountID5{kAccount5}; + static constexpr AccountID kAccountID6{kAccount6}; + static constexpr AccountID kAccountID7{kAccount7}; + static constexpr AccountID kAccountID8{kAccount8}; + + auto ps = STPathSet{}; + + auto createPathElements = [](auto const& account1, auto const& account2) { + auto base = STPath{}; + base.pushBack( + STPathElement{STPathElement::TypeAccount, account1, xrpCurrency(), account1}); + auto tail = + STPathElement{STPathElement::TypeAccount, account2, xrpCurrency(), account2}; + return std::make_pair(base, tail); + }; + + { + auto [base, tail] = createPathElements(kAccountID1, kAccountID2); + + for (auto i = 0uz; i < 10000; ++i) + { + ps.assembleAdd(base, tail); + } + + BEAST_EXPECT(ps.size() == 1); + } + + { + auto [base, tail] = createPathElements(kAccountID3, kAccountID4); + ps.assembleAdd(base, tail); + } + + { + auto [base, tail] = createPathElements(kAccountID5, kAccountID6); + ps.assembleAdd(base, tail); + } + + { + auto [base, tail] = createPathElements(kAccountID7, kAccountID8); + + auto before = ps.size(); + + for (auto i = 0uz; i < 10000; ++i) + { + ps.assembleAdd(base, tail); + } + + BEAST_EXPECT(ps.size() - before == 1); + } + + { + auto [base, tail] = createPathElements(kAccountID1, kAccountID3); + auto copy = base; + copy.pushBack(tail); + + auto before = ps.size(); + + ps.pushBack(copy); + ps.assembleAdd(base, tail); + + BEAST_EXPECT(ps.size() - before == 1); + } + + { + auto [base, tail] = createPathElements(kAccountID2, kAccountID4); + auto copy = base; + copy.pushBack(tail); + + auto before = ps.size(); + + ps.emplaceBack(copy); + ps.assembleAdd(base, tail); + + BEAST_EXPECT(ps.size() - before == 1); + } + + BEAST_EXPECT(ps.size() == 6); + } + void run() override { @@ -1878,6 +1976,7 @@ public: issuesPathNegativeRippleClientIssue23Smaller(); issuesPathNegativeRippleClientIssue23Larger(); qualityPathsQualitySetAndTest(); + testAssembleAddDeduplication(); trustAutoClearTrustNormalClear(); trustAutoClearTrustAutoClear(); norippleCombinations(); diff --git a/src/xrpld/rpc/detail/Pathfinder.cpp b/src/xrpld/rpc/detail/Pathfinder.cpp index 5b7f1415a2..642b5c4253 100644 --- a/src/xrpld/rpc/detail/Pathfinder.cpp +++ b/src/xrpld/rpc/detail/Pathfinder.cpp @@ -962,14 +962,10 @@ Pathfinder::isNoRippleOut(STPath const& currentPath) void addUniquePath(STPathSet& pathSet, STPath const& path) { - // TODO(tom): building an STPathSet this way is quadratic in the size - // of the STPathSet! - for (auto const& p : pathSet) + if (!pathSet.contains(path)) { - if (p == path) - return; + pathSet.pushBack(path); } - pathSet.pushBack(path); } void