From 991891625aebb6856edbb5164d82c4f79ad64507 Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 7 Aug 2025 06:52:58 -0400 Subject: [PATCH 1/4] Upload Conan dependencies upon merge into develop (#5654) This change uploads built Conan dependencies to the Conan remote upon merge into the develop branch. At the moment, whenever Conan dependencies change, we need to remember to manually push them to our Conan remote, so they are cached for future reuse. If we forget to do so, these changed dependencies need to be rebuilt over and over again, which can take a long time. --- .github/actions/dependencies/action.yml | 10 +++++++++- .github/workflows/macos.yml | 2 ++ .github/workflows/nix.yml | 2 ++ .github/workflows/windows.yml | 2 ++ 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/.github/actions/dependencies/action.yml b/.github/actions/dependencies/action.yml index e32d8934ba..8dc78450c1 100644 --- a/.github/actions/dependencies/action.yml +++ b/.github/actions/dependencies/action.yml @@ -15,7 +15,7 @@ runs: echo "Updated Conan remote 'xrplf' to ${CONAN_URL}." else conan remote add --index 0 xrplf ${CONAN_URL} - echo "Added new conan remote 'xrplf' at ${CONAN_URL}." + echo "Added new Conan remote 'xrplf' at ${CONAN_URL}." fi - name: list missing binaries id: binaries @@ -36,3 +36,11 @@ runs: --options:host "&:xrpld=True" \ --settings:all build_type=${{ inputs.configuration }} \ .. + - name: upload dependencies + if: ${{ env.CONAN_URL != '' && env.CONAN_LOGIN_USERNAME_XRPLF != '' && env.CONAN_PASSWORD_XRPLF != '' && github.ref_type == 'branch' && github.ref_name == github.event.repository.default_branch }} + shell: bash + run: | + echo "Logging into Conan remote 'xrplf' at ${CONAN_URL}." + conan remote login xrplf "${{ env.CONAN_LOGIN_USERNAME_XRPLF }}" --password "${{ env.CONAN_PASSWORD_XRPLF }}" + echo "Uploading dependencies for configuration '${{ inputs.configuration }}'." + conan upload --all --confirm --remote xrplf . --settings build_type=${{ inputs.configuration }} diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml index c056468fc6..ee912fda40 100644 --- a/.github/workflows/macos.yml +++ b/.github/workflows/macos.yml @@ -19,6 +19,8 @@ concurrency: # to pollute conan/profiles directory with settings which might not work for others env: CONAN_URL: https://conan.ripplex.io + CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.CONAN_USERNAME }} + CONAN_PASSWORD_XRPLF: ${{ secrets.CONAN_TOKEN }} CONAN_GLOBAL_CONF: | core.download:parallel={{os.cpu_count()}} core.upload:parallel={{os.cpu_count()}} diff --git a/.github/workflows/nix.yml b/.github/workflows/nix.yml index 14e98e3fb0..c58c97364d 100644 --- a/.github/workflows/nix.yml +++ b/.github/workflows/nix.yml @@ -20,6 +20,8 @@ concurrency: # to pollute conan/profiles directory with settings which might not work for others env: CONAN_URL: https://conan.ripplex.io + CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.CONAN_USERNAME }} + CONAN_PASSWORD_XRPLF: ${{ secrets.CONAN_TOKEN }} CONAN_GLOBAL_CONF: | core.download:parallel={{ os.cpu_count() }} core.upload:parallel={{ os.cpu_count() }} diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index 4de92c8049..0b3fa5ff09 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -22,6 +22,8 @@ concurrency: # to pollute conan/profiles directory with settings which might not work for others env: CONAN_URL: https://conan.ripplex.io + CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.CONAN_USERNAME }} + CONAN_PASSWORD_XRPLF: ${{ secrets.CONAN_TOKEN }} CONAN_GLOBAL_CONF: | core.download:parallel={{os.cpu_count()}} core.upload:parallel={{os.cpu_count()}} From 94decc753b515e7499808ca0d5b9e24d172c691e Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Thu, 7 Aug 2025 22:04:07 +0100 Subject: [PATCH 2/4] perf: Move mutex to the partition level (#5486) This change introduces two key optimizations: * Mutex scope reduction: Limits the lock to individual partitions within `TaggedCache`, reducing contention. * Decoupling: Removes the tight coupling between `LedgerHistory` and `TaggedCache`, improving modularity and testability. Lock contention analysis based on eBPF showed significant improvements as a result of this change. --- include/xrpl/basics/SHAMapHash.h | 1 - include/xrpl/basics/TaggedCache.h | 26 +-- include/xrpl/basics/TaggedCache.ipp | 197 +++++++++--------- .../xrpl/basics/partitioned_unordered_map.h | 12 ++ include/xrpl/protocol/Protocol.h | 1 - src/test/basics/TaggedCache_test.cpp | 24 +-- src/xrpld/app/ledger/LedgerHistory.cpp | 15 +- src/xrpld/rpc/handlers/GetCounts.cpp | 2 +- 8 files changed, 135 insertions(+), 143 deletions(-) diff --git a/include/xrpl/basics/SHAMapHash.h b/include/xrpl/basics/SHAMapHash.h index 2d2dcdc3ef..1ec326409c 100644 --- a/include/xrpl/basics/SHAMapHash.h +++ b/include/xrpl/basics/SHAMapHash.h @@ -21,7 +21,6 @@ #define RIPPLE_BASICS_SHAMAP_HASH_H_INCLUDED #include -#include #include diff --git a/include/xrpl/basics/TaggedCache.h b/include/xrpl/basics/TaggedCache.h index 99c91fe393..7eace6fe72 100644 --- a/include/xrpl/basics/TaggedCache.h +++ b/include/xrpl/basics/TaggedCache.h @@ -90,9 +90,6 @@ public: int getCacheSize() const; - int - getTrackSize() const; - float getHitRate(); @@ -170,9 +167,6 @@ public: bool retrieve(key_type const& key, T& data); - mutex_type& - peekMutex(); - std::vector getKeys() const; @@ -193,11 +187,14 @@ public: private: SharedPointerType - initialFetch(key_type const& key, std::lock_guard const& l); + initialFetch(key_type const& key); void collect_metrics(); + Mutex& + lockPartition(key_type const& key) const; + private: struct Stats { @@ -300,8 +297,8 @@ private: [[maybe_unused]] clock_type::time_point const& now, typename KeyValueCacheType::map_type& partition, SweptPointersVector& stuffToSweep, - std::atomic& allRemovals, - std::lock_guard const&); + std::atomic& allRemoval, + Mutex& partitionLock); [[nodiscard]] std::thread sweepHelper( @@ -310,14 +307,12 @@ private: typename KeyOnlyCacheType::map_type& partition, SweptPointersVector&, std::atomic& allRemovals, - std::lock_guard const&); + Mutex& partitionLock); beast::Journal m_journal; clock_type& m_clock; Stats m_stats; - mutex_type mutable m_mutex; - // Used for logging std::string m_name; @@ -328,10 +323,11 @@ private: clock_type::duration const m_target_age; // Number of items cached - int m_cache_count; + std::atomic m_cache_count; cache_type m_cache; // Hold strong reference to recent objects - std::uint64_t m_hits; - std::uint64_t m_misses; + std::atomic m_hits; + std::atomic m_misses; + mutable std::vector partitionLocks_; }; } // namespace ripple diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 16a3f7587a..c909ec6ad1 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -22,6 +22,7 @@ #include #include +#include namespace ripple { @@ -60,6 +61,7 @@ inline TaggedCache< , m_hits(0) , m_misses(0) { + partitionLocks_ = std::vector(m_cache.partitions()); } template < @@ -105,8 +107,13 @@ TaggedCache< KeyEqual, Mutex>::size() const { - std::lock_guard lock(m_mutex); - return m_cache.size(); + std::size_t totalSize = 0; + for (size_t i = 0; i < partitionLocks_.size(); ++i) + { + std::lock_guard lock(partitionLocks_[i]); + totalSize += m_cache.map()[i].size(); + } + return totalSize; } template < @@ -129,32 +136,7 @@ TaggedCache< KeyEqual, Mutex>::getCacheSize() const { - std::lock_guard lock(m_mutex); - return m_cache_count; -} - -template < - class Key, - class T, - bool IsKeyCache, - class SharedWeakUnionPointer, - class SharedPointerType, - class Hash, - class KeyEqual, - class Mutex> -inline int -TaggedCache< - Key, - T, - IsKeyCache, - SharedWeakUnionPointer, - SharedPointerType, - Hash, - KeyEqual, - Mutex>::getTrackSize() const -{ - std::lock_guard lock(m_mutex); - return m_cache.size(); + return m_cache_count.load(std::memory_order_relaxed); } template < @@ -177,9 +159,10 @@ TaggedCache< KeyEqual, Mutex>::getHitRate() { - std::lock_guard lock(m_mutex); - auto const total = static_cast(m_hits + m_misses); - return m_hits * (100.0f / std::max(1.0f, total)); + auto const hits = m_hits.load(std::memory_order_relaxed); + auto const misses = m_misses.load(std::memory_order_relaxed); + float const total = float(hits + misses); + return hits * (100.0f / std::max(1.0f, total)); } template < @@ -202,9 +185,12 @@ TaggedCache< KeyEqual, Mutex>::clear() { - std::lock_guard lock(m_mutex); + for (auto& mutex : partitionLocks_) + mutex.lock(); m_cache.clear(); - m_cache_count = 0; + for (auto& mutex : partitionLocks_) + mutex.unlock(); + m_cache_count.store(0, std::memory_order_relaxed); } template < @@ -227,11 +213,9 @@ TaggedCache< KeyEqual, Mutex>::reset() { - std::lock_guard lock(m_mutex); - m_cache.clear(); - m_cache_count = 0; - m_hits = 0; - m_misses = 0; + clear(); + m_hits.store(0, std::memory_order_relaxed); + m_misses.store(0, std::memory_order_relaxed); } template < @@ -255,7 +239,7 @@ TaggedCache< KeyEqual, Mutex>::touch_if_exists(KeyComparable const& key) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(lockPartition(key)); auto const iter(m_cache.find(key)); if (iter == m_cache.end()) { @@ -297,8 +281,6 @@ TaggedCache< auto const start = std::chrono::steady_clock::now(); { - std::lock_guard lock(m_mutex); - if (m_target_size == 0 || (static_cast(m_cache.size()) <= m_target_size)) { @@ -330,12 +312,13 @@ TaggedCache< m_cache.map()[p], allStuffToSweep[p], allRemovals, - lock)); + partitionLocks_[p])); } for (std::thread& worker : workers) worker.join(); - m_cache_count -= allRemovals; + int removals = allRemovals.load(std::memory_order_relaxed); + m_cache_count.fetch_sub(removals, std::memory_order_relaxed); } // At this point allStuffToSweep will go out of scope outside the lock // and decrement the reference count on each strong pointer. @@ -369,7 +352,8 @@ TaggedCache< { // Remove from cache, if !valid, remove from map too. Returns true if // removed from cache - std::lock_guard lock(m_mutex); + + std::lock_guard lock(lockPartition(key)); auto cit = m_cache.find(key); @@ -382,7 +366,7 @@ TaggedCache< if (entry.isCached()) { - --m_cache_count; + m_cache_count.fetch_sub(1, std::memory_order_relaxed); entry.ptr.convertToWeak(); ret = true; } @@ -420,17 +404,16 @@ TaggedCache< { // Return canonical value, store if needed, refresh in cache // Return values: true=we had the data already - std::lock_guard lock(m_mutex); + std::lock_guard lock(lockPartition(key)); auto cit = m_cache.find(key); - if (cit == m_cache.end()) { m_cache.emplace( std::piecewise_construct, std::forward_as_tuple(key), std::forward_as_tuple(m_clock.now(), data)); - ++m_cache_count; + m_cache_count.fetch_add(1, std::memory_order_relaxed); return false; } @@ -479,12 +462,12 @@ TaggedCache< data = cachedData; } - ++m_cache_count; + m_cache_count.fetch_add(1, std::memory_order_relaxed); return true; } entry.ptr = data; - ++m_cache_count; + m_cache_count.fetch_add(1, std::memory_order_relaxed); return false; } @@ -560,10 +543,11 @@ TaggedCache< KeyEqual, Mutex>::fetch(key_type const& key) { - std::lock_guard l(m_mutex); - auto ret = initialFetch(key, l); + std::lock_guard lock(lockPartition(key)); + + auto ret = initialFetch(key); if (!ret) - ++m_misses; + m_misses.fetch_add(1, std::memory_order_relaxed); return ret; } @@ -627,8 +611,8 @@ TaggedCache< Mutex>::insert(key_type const& key) -> std::enable_if_t { - std::lock_guard lock(m_mutex); clock_type::time_point const now(m_clock.now()); + std::lock_guard lock(lockPartition(key)); auto [it, inserted] = m_cache.emplace( std::piecewise_construct, std::forward_as_tuple(key), @@ -668,29 +652,6 @@ TaggedCache< return true; } -template < - class Key, - class T, - bool IsKeyCache, - class SharedWeakUnionPointer, - class SharedPointerType, - class Hash, - class KeyEqual, - class Mutex> -inline auto -TaggedCache< - Key, - T, - IsKeyCache, - SharedWeakUnionPointer, - SharedPointerType, - Hash, - KeyEqual, - Mutex>::peekMutex() -> mutex_type& -{ - return m_mutex; -} - template < class Key, class T, @@ -714,10 +675,13 @@ TaggedCache< std::vector v; { - std::lock_guard lock(m_mutex); v.reserve(m_cache.size()); - for (auto const& _ : m_cache) - v.push_back(_.first); + for (std::size_t i = 0; i < partitionLocks_.size(); ++i) + { + std::lock_guard lock(partitionLocks_[i]); + for (auto const& entry : m_cache.map()[i]) + v.push_back(entry.first); + } } return v; @@ -743,11 +707,12 @@ TaggedCache< KeyEqual, Mutex>::rate() const { - std::lock_guard lock(m_mutex); - auto const tot = m_hits + m_misses; + auto const hits = m_hits.load(std::memory_order_relaxed); + auto const misses = m_misses.load(std::memory_order_relaxed); + auto const tot = hits + misses; if (tot == 0) - return 0; - return double(m_hits) / tot; + return 0.0; + return double(hits) / tot; } template < @@ -771,18 +736,16 @@ TaggedCache< KeyEqual, Mutex>::fetch(key_type const& digest, Handler const& h) { - { - std::lock_guard l(m_mutex); - if (auto ret = initialFetch(digest, l)) - return ret; - } + std::lock_guard lock(lockPartition(digest)); + + if (auto ret = initialFetch(digest)) + return ret; auto sle = h(); if (!sle) return {}; - std::lock_guard l(m_mutex); - ++m_misses; + m_misses.fetch_add(1, std::memory_order_relaxed); auto const [it, inserted] = m_cache.emplace(digest, Entry(m_clock.now(), std::move(sle))); if (!inserted) @@ -809,9 +772,10 @@ TaggedCache< SharedPointerType, Hash, KeyEqual, - Mutex>:: - initialFetch(key_type const& key, std::lock_guard const& l) + Mutex>::initialFetch(key_type const& key) { + std::lock_guard lock(lockPartition(key)); + auto cit = m_cache.find(key); if (cit == m_cache.end()) return {}; @@ -819,7 +783,7 @@ TaggedCache< Entry& entry = cit->second; if (entry.isCached()) { - ++m_hits; + m_hits.fetch_add(1, std::memory_order_relaxed); entry.touch(m_clock.now()); return entry.ptr.getStrong(); } @@ -827,12 +791,13 @@ TaggedCache< if (entry.isCached()) { // independent of cache size, so not counted as a hit - ++m_cache_count; + m_cache_count.fetch_add(1, std::memory_order_relaxed); entry.touch(m_clock.now()); return entry.ptr.getStrong(); } m_cache.erase(cit); + return {}; } @@ -861,10 +826,11 @@ TaggedCache< { beast::insight::Gauge::value_type hit_rate(0); { - std::lock_guard lock(m_mutex); - auto const total(m_hits + m_misses); + auto const hits = m_hits.load(std::memory_order_relaxed); + auto const misses = m_misses.load(std::memory_order_relaxed); + auto const total = hits + misses; if (total != 0) - hit_rate = (m_hits * 100) / total; + hit_rate = (hits * 100) / total; } m_stats.hit_rate.set(hit_rate); } @@ -895,12 +861,16 @@ TaggedCache< typename KeyValueCacheType::map_type& partition, SweptPointersVector& stuffToSweep, std::atomic& allRemovals, - std::lock_guard const&) + Mutex& partitionLock) { return std::thread([&, this]() { + beast::setCurrentThreadName("sweep-KVCache"); + int cacheRemovals = 0; int mapRemovals = 0; + std::lock_guard lock(partitionLock); + // Keep references to all the stuff we sweep // so that we can destroy them outside the lock. stuffToSweep.reserve(partition.size()); @@ -984,12 +954,16 @@ TaggedCache< typename KeyOnlyCacheType::map_type& partition, SweptPointersVector&, std::atomic& allRemovals, - std::lock_guard const&) + Mutex& partitionLock) { return std::thread([&, this]() { + beast::setCurrentThreadName("sweep-KCache"); + int cacheRemovals = 0; int mapRemovals = 0; + std::lock_guard lock(partitionLock); + // Keep references to all the stuff we sweep // so that we can destroy them outside the lock. { @@ -1024,6 +998,29 @@ TaggedCache< }); } +template < + class Key, + class T, + bool IsKeyCache, + class SharedWeakUnionPointer, + class SharedPointerType, + class Hash, + class KeyEqual, + class Mutex> +inline Mutex& +TaggedCache< + Key, + T, + IsKeyCache, + SharedWeakUnionPointer, + SharedPointerType, + Hash, + KeyEqual, + Mutex>::lockPartition(key_type const& key) const +{ + return partitionLocks_[m_cache.partition_index(key)]; +} + } // namespace ripple #endif diff --git a/include/xrpl/basics/partitioned_unordered_map.h b/include/xrpl/basics/partitioned_unordered_map.h index 4e503ad0fa..ecaf16a47e 100644 --- a/include/xrpl/basics/partitioned_unordered_map.h +++ b/include/xrpl/basics/partitioned_unordered_map.h @@ -277,6 +277,12 @@ public: return map_; } + partition_map_type const& + map() const + { + return map_; + } + iterator begin() { @@ -321,6 +327,12 @@ public: return cend(); } + std::size_t + partition_index(key_type const& key) const + { + return partitioner(key); + } + private: template void diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 898fd06fbd..bd39233cca 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -22,7 +22,6 @@ #include #include -#include #include diff --git a/src/test/basics/TaggedCache_test.cpp b/src/test/basics/TaggedCache_test.cpp index 797838fcfa..ce33455110 100644 --- a/src/test/basics/TaggedCache_test.cpp +++ b/src/test/basics/TaggedCache_test.cpp @@ -58,10 +58,10 @@ public: // Insert an item, retrieve it, and age it so it gets purged. { BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.getTrackSize() == 0); + BEAST_EXPECT(c.size() == 0); BEAST_EXPECT(!c.insert(1, "one")); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.getTrackSize() == 1); + BEAST_EXPECT(c.size() == 1); { std::string s; @@ -72,7 +72,7 @@ public: ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.getTrackSize() == 0); + BEAST_EXPECT(c.size() == 0); } // Insert an item, maintain a strong pointer, age it, and @@ -80,7 +80,7 @@ public: { BEAST_EXPECT(!c.insert(2, "two")); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.getTrackSize() == 1); + BEAST_EXPECT(c.size() == 1); { auto p = c.fetch(2); @@ -88,14 +88,14 @@ public: ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.getTrackSize() == 1); + BEAST_EXPECT(c.size() == 1); } // Make sure its gone now that our reference is gone ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.getTrackSize() == 0); + BEAST_EXPECT(c.size() == 0); } // Insert the same key/value pair and make sure we get the same result @@ -111,7 +111,7 @@ public: ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.getTrackSize() == 0); + BEAST_EXPECT(c.size() == 0); } // Put an object in but keep a strong pointer to it, advance the clock a @@ -121,24 +121,24 @@ public: // Put an object in BEAST_EXPECT(!c.insert(4, "four")); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.getTrackSize() == 1); + BEAST_EXPECT(c.size() == 1); { // Keep a strong pointer to it auto const p1 = c.fetch(4); BEAST_EXPECT(p1 != nullptr); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.getTrackSize() == 1); + BEAST_EXPECT(c.size() == 1); // Advance the clock a lot ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.getTrackSize() == 1); + BEAST_EXPECT(c.size() == 1); // Canonicalize a new object with the same key auto p2 = std::make_shared("four"); BEAST_EXPECT(c.canonicalize_replace_client(4, p2)); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.getTrackSize() == 1); + BEAST_EXPECT(c.size() == 1); // Make sure we get the original object BEAST_EXPECT(p1.get() == p2.get()); } @@ -146,7 +146,7 @@ public: ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.getTrackSize() == 0); + BEAST_EXPECT(c.size() == 0); } } }; diff --git a/src/xrpld/app/ledger/LedgerHistory.cpp b/src/xrpld/app/ledger/LedgerHistory.cpp index ccec209bd4..dcbd722120 100644 --- a/src/xrpld/app/ledger/LedgerHistory.cpp +++ b/src/xrpld/app/ledger/LedgerHistory.cpp @@ -63,8 +63,6 @@ LedgerHistory::insert( ledger->stateMap().getHash().isNonZero(), "ripple::LedgerHistory::insert : nonzero hash"); - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); - bool const alreadyHad = m_ledgers_by_hash.canonicalize_replace_cache( ledger->info().hash, ledger); if (validated) @@ -76,7 +74,6 @@ LedgerHistory::insert( LedgerHash LedgerHistory::getLedgerHash(LedgerIndex index) { - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); if (auto it = mLedgersByIndex.find(index); it != mLedgersByIndex.end()) return it->second; return {}; @@ -86,13 +83,11 @@ std::shared_ptr LedgerHistory::getLedgerBySeq(LedgerIndex index) { { - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); auto it = mLedgersByIndex.find(index); if (it != mLedgersByIndex.end()) { uint256 hash = it->second; - sl.unlock(); return getLedgerByHash(hash); } } @@ -108,7 +103,6 @@ LedgerHistory::getLedgerBySeq(LedgerIndex index) { // Add this ledger to the local tracking by index - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); XRPL_ASSERT( ret->isImmutable(), @@ -458,8 +452,6 @@ LedgerHistory::builtLedger( XRPL_ASSERT( !hash.isZero(), "ripple::LedgerHistory::builtLedger : nonzero hash"); - std::unique_lock sl(m_consensus_validated.peekMutex()); - auto entry = std::make_shared(); m_consensus_validated.canonicalize_replace_client(index, entry); @@ -500,8 +492,6 @@ LedgerHistory::validatedLedger( !hash.isZero(), "ripple::LedgerHistory::validatedLedger : nonzero hash"); - std::unique_lock sl(m_consensus_validated.peekMutex()); - auto entry = std::make_shared(); m_consensus_validated.canonicalize_replace_client(index, entry); @@ -535,10 +525,9 @@ LedgerHistory::validatedLedger( bool LedgerHistory::fixIndex(LedgerIndex ledgerIndex, LedgerHash const& ledgerHash) { - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); + auto ledger = m_ledgers_by_hash.fetch(ledgerHash); auto it = mLedgersByIndex.find(ledgerIndex); - - if ((it != mLedgersByIndex.end()) && (it->second != ledgerHash)) + if (ledger && (it != mLedgersByIndex.end()) && (it->second != ledgerHash)) { it->second = ledgerHash; return false; diff --git a/src/xrpld/rpc/handlers/GetCounts.cpp b/src/xrpld/rpc/handlers/GetCounts.cpp index 3c1d8cccdd..2987da46d5 100644 --- a/src/xrpld/rpc/handlers/GetCounts.cpp +++ b/src/xrpld/rpc/handlers/GetCounts.cpp @@ -114,7 +114,7 @@ getCountsJson(Application& app, int minObjectCount) ret[jss::treenode_cache_size] = app.getNodeFamily().getTreeNodeCache()->getCacheSize(); ret[jss::treenode_track_size] = - app.getNodeFamily().getTreeNodeCache()->getTrackSize(); + static_cast(app.getNodeFamily().getTreeNodeCache()->size()); std::string uptime; auto s = UptimeClock::now(); From 39b5031ab5efa543e3007fb3fc7e199381ee68fb Mon Sep 17 00:00:00 2001 From: Bart Date: Fri, 8 Aug 2025 08:47:36 -0400 Subject: [PATCH 3/4] Switch Conan 1 commands to Conan 2 and fix credentials (#5655) This change updates some incorrect Conan commands for Conan 2. As some flags do not exist in Conan 2, such as --settings build_type=[configuration], the commands have been adjusted accordingly. This change further uses the org-level variables and secrets rather than the repo-level ones. --- .github/actions/dependencies/action.yml | 38 ++++++++++--------------- .github/workflows/libxrpl.yml | 12 ++++---- .github/workflows/macos.yml | 9 ++++-- .github/workflows/nix.yml | 11 +++---- .github/workflows/windows.yml | 11 +++---- 5 files changed, 39 insertions(+), 42 deletions(-) diff --git a/.github/actions/dependencies/action.yml b/.github/actions/dependencies/action.yml index 8dc78450c1..0bd28f15dd 100644 --- a/.github/actions/dependencies/action.yml +++ b/.github/actions/dependencies/action.yml @@ -2,33 +2,25 @@ name: dependencies inputs: configuration: required: true -# An implicit input is the environment variable `build_dir`. +# Implicit inputs are the environment variables `build_dir`, CONAN_REMOTE_URL, +# CONAN_REMOTE_USERNAME, and CONAN_REMOTE_PASSWORD. The latter two are only +# used to upload newly built dependencies to the Conan remote. runs: using: composite steps: - name: add Conan remote - if: env.CONAN_URL != '' + if: ${{ env.CONAN_REMOTE_URL != '' }} shell: bash run: | - if conan remote list | grep -q 'xrplf'; then - conan remote update --index 0 --url ${CONAN_URL} xrplf - echo "Updated Conan remote 'xrplf' to ${CONAN_URL}." - else - conan remote add --index 0 xrplf ${CONAN_URL} - echo "Added new Conan remote 'xrplf' at ${CONAN_URL}." - fi - - name: list missing binaries - id: binaries - shell: bash - # Print the list of dependencies that would need to be built locally. - # A non-empty list means we have "failed" to cache binaries remotely. - run: | - echo missing=$(conan info . --build missing --settings build_type=${{ inputs.configuration }} --json 2>/dev/null | grep '^\[') | tee ${GITHUB_OUTPUT} + echo "Adding Conan remote 'xrplf' at ${{ env.CONAN_REMOTE_URL }}." + conan remote add --index 0 --force xrplf ${{ env.CONAN_REMOTE_URL }} + echo "Listing Conan remotes." + conan remote list - name: install dependencies shell: bash run: | - mkdir ${build_dir} - cd ${build_dir} + mkdir -p ${{ env.build_dir }} + cd ${{ env.build_dir }} conan install \ --output-folder . \ --build missing \ @@ -37,10 +29,10 @@ runs: --settings:all build_type=${{ inputs.configuration }} \ .. - name: upload dependencies - if: ${{ env.CONAN_URL != '' && env.CONAN_LOGIN_USERNAME_XRPLF != '' && env.CONAN_PASSWORD_XRPLF != '' && github.ref_type == 'branch' && github.ref_name == github.event.repository.default_branch }} + if: ${{ env.CONAN_REMOTE_URL != '' && env.CONAN_REMOTE_USERNAME != '' && env.CONAN_REMOTE_PASSWORD != '' && github.ref_type == 'branch' && github.ref_name == github.event.repository.default_branch }} shell: bash run: | - echo "Logging into Conan remote 'xrplf' at ${CONAN_URL}." - conan remote login xrplf "${{ env.CONAN_LOGIN_USERNAME_XRPLF }}" --password "${{ env.CONAN_PASSWORD_XRPLF }}" - echo "Uploading dependencies for configuration '${{ inputs.configuration }}'." - conan upload --all --confirm --remote xrplf . --settings build_type=${{ inputs.configuration }} + echo "Logging into Conan remote 'xrplf' at ${{ env.CONAN_REMOTE_URL }}." + conan remote login xrplf "${{ env.CONAN_REMOTE_USERNAME }}" --password "${{ env.CONAN_REMOTE_PASSWORD }}" + echo "Uploading dependencies." + conan upload '*' --confirm --check --remote xrplf diff --git a/.github/workflows/libxrpl.yml b/.github/workflows/libxrpl.yml index a8746fe297..8223b7996f 100644 --- a/.github/workflows/libxrpl.yml +++ b/.github/workflows/libxrpl.yml @@ -1,8 +1,8 @@ name: Check libXRPL compatibility with Clio env: - CONAN_URL: https://conan.ripplex.io - CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.CONAN_USERNAME }} - CONAN_PASSWORD_XRPLF: ${{ secrets.CONAN_TOKEN }} + CONAN_REMOTE_URL: ${{ vars.CONAN_REMOTE_URL }} + CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.CONAN_REMOTE_USERNAME }} + CONAN_PASSWORD_XRPLF: ${{ secrets.CONAN_REMOTE_PASSWORD }} on: pull_request: paths: @@ -46,10 +46,10 @@ jobs: - name: Add Conan remote shell: bash run: | + echo "Adding Conan remote 'xrplf' at ${{ env.CONAN_REMOTE_URL }}." + conan remote add xrplf ${{ env.CONAN_REMOTE_URL }} --insert 0 --force + echo "Listing Conan remotes." conan remote list - conan remote remove xrplf || true - # Do not quote the URL. An empty string will be accepted (with a non-fatal warning), but a missing argument will not. - conan remote add xrplf ${{ env.CONAN_URL }} --insert 0 - name: Parse new version id: version shell: bash diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml index ee912fda40..6c6091cc2e 100644 --- a/.github/workflows/macos.yml +++ b/.github/workflows/macos.yml @@ -18,9 +18,12 @@ concurrency: # This part of Conan configuration is specific to this workflow only; we do not want # to pollute conan/profiles directory with settings which might not work for others env: - CONAN_URL: https://conan.ripplex.io - CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.CONAN_USERNAME }} - CONAN_PASSWORD_XRPLF: ${{ secrets.CONAN_TOKEN }} + CONAN_REMOTE_URL: ${{ vars.CONAN_REMOTE_URL }} + CONAN_REMOTE_USERNAME: ${{ secrets.CONAN_REMOTE_USERNAME }} + CONAN_REMOTE_PASSWORD: ${{ secrets.CONAN_REMOTE_PASSWORD }} + # This part of the Conan configuration is specific to this workflow only; we + # do not want to pollute the 'conan/profiles' directory with settings that + # might not work for other workflows. CONAN_GLOBAL_CONF: | core.download:parallel={{os.cpu_count()}} core.upload:parallel={{os.cpu_count()}} diff --git a/.github/workflows/nix.yml b/.github/workflows/nix.yml index c58c97364d..5beb5d291d 100644 --- a/.github/workflows/nix.yml +++ b/.github/workflows/nix.yml @@ -16,12 +16,13 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true -# This part of Conan configuration is specific to this workflow only; we do not want -# to pollute conan/profiles directory with settings which might not work for others env: - CONAN_URL: https://conan.ripplex.io - CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.CONAN_USERNAME }} - CONAN_PASSWORD_XRPLF: ${{ secrets.CONAN_TOKEN }} + CONAN_REMOTE_URL: ${{ vars.CONAN_REMOTE_URL }} + CONAN_REMOTE_USERNAME: ${{ secrets.CONAN_REMOTE_USERNAME }} + CONAN_REMOTE_PASSWORD: ${{ secrets.CONAN_REMOTE_PASSWORD }} + # This part of the Conan configuration is specific to this workflow only; we + # do not want to pollute the 'conan/profiles' directory with settings that + # might not work for other workflows. CONAN_GLOBAL_CONF: | core.download:parallel={{ os.cpu_count() }} core.upload:parallel={{ os.cpu_count() }} diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index 0b3fa5ff09..9e2322b119 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -18,12 +18,13 @@ on: concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true -# This part of Conan configuration is specific to this workflow only; we do not want -# to pollute conan/profiles directory with settings which might not work for others env: - CONAN_URL: https://conan.ripplex.io - CONAN_LOGIN_USERNAME_XRPLF: ${{ secrets.CONAN_USERNAME }} - CONAN_PASSWORD_XRPLF: ${{ secrets.CONAN_TOKEN }} + CONAN_REMOTE_URL: ${{ vars.CONAN_REMOTE_URL }} + CONAN_REMOTE_USERNAME: ${{ secrets.CONAN_REMOTE_USERNAME }} + CONAN_REMOTE_PASSWORD: ${{ secrets.CONAN_REMOTE_PASSWORD }} + # This part of the Conan configuration is specific to this workflow only; we + # do not want to pollute the 'conan/profiles' directory with settings that + # might not work for other workflows. CONAN_GLOBAL_CONF: | core.download:parallel={{os.cpu_count()}} core.upload:parallel={{os.cpu_count()}} From 86ef16dbebda2f4dc9c367a08a0f2ca85c22ce1f Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 8 Aug 2025 17:13:32 -0400 Subject: [PATCH 4/4] Fix: Don't flag consensus as stalled prematurely (#5627) Fix stalled consensus detection to prevent false positives in situations where there are no disputed transactions. Stalled consensus detection was added to 2.5.0 in response to a network consensus halt that caused a round to run for over an hour. However, it has a flaw that makes it very easy to have false positives. Those false positives are usually mitigated by other checks that prevent them from having an effect, but there have been several instances of validators "running ahead" because there are circumstances where the other checks are "successful", allowing the stall state to be checked. --- src/test/consensus/Consensus_test.cpp | 183 ++++++++++++++++----- src/xrpld/app/consensus/RCLValidations.cpp | 2 +- src/xrpld/consensus/Consensus.cpp | 10 +- src/xrpld/consensus/Consensus.h | 24 ++- src/xrpld/consensus/DisputedTx.h | 27 ++- 5 files changed, 192 insertions(+), 54 deletions(-) diff --git a/src/test/consensus/Consensus_test.cpp b/src/test/consensus/Consensus_test.cpp index db56ab58c6..7899336a6f 100644 --- a/src/test/consensus/Consensus_test.cpp +++ b/src/test/consensus/Consensus_test.cpp @@ -1136,6 +1136,10 @@ public: ConsensusParms p; std::size_t peersUnchanged = 0; + auto logs = std::make_unique(beast::severities::kError); + auto j = logs->journal("Test"); + auto clog = std::make_unique(); + // Three cases: // 1 proposing, initial vote yes // 2 proposing, initial vote no @@ -1172,10 +1176,15 @@ public: BEAST_EXPECT(proposingFalse.getOurVote() == false); BEAST_EXPECT(followingTrue.getOurVote() == true); BEAST_EXPECT(followingFalse.getOurVote() == false); - BEAST_EXPECT(!proposingTrue.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(!proposingFalse.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(!followingTrue.stalled(p, false, peersUnchanged)); - BEAST_EXPECT(!followingFalse.stalled(p, false, peersUnchanged)); + BEAST_EXPECT( + !proposingTrue.stalled(p, true, peersUnchanged, j, clog)); + BEAST_EXPECT( + !proposingFalse.stalled(p, true, peersUnchanged, j, clog)); + BEAST_EXPECT( + !followingTrue.stalled(p, false, peersUnchanged, j, clog)); + BEAST_EXPECT( + !followingFalse.stalled(p, false, peersUnchanged, j, clog)); + BEAST_EXPECT(clog->str() == ""); // I'm in the majority, my vote should not change BEAST_EXPECT(!proposingTrue.updateVote(5, true, p)); @@ -1189,10 +1198,15 @@ public: BEAST_EXPECT(!followingFalse.updateVote(10, false, p)); peersUnchanged = 2; - BEAST_EXPECT(!proposingTrue.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(!proposingFalse.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(!followingTrue.stalled(p, false, peersUnchanged)); - BEAST_EXPECT(!followingFalse.stalled(p, false, peersUnchanged)); + BEAST_EXPECT( + !proposingTrue.stalled(p, true, peersUnchanged, j, clog)); + BEAST_EXPECT( + !proposingFalse.stalled(p, true, peersUnchanged, j, clog)); + BEAST_EXPECT( + !followingTrue.stalled(p, false, peersUnchanged, j, clog)); + BEAST_EXPECT( + !followingFalse.stalled(p, false, peersUnchanged, j, clog)); + BEAST_EXPECT(clog->str() == ""); // Right now, the vote is 51%. The requirement is about to jump to // 65% @@ -1282,10 +1296,15 @@ public: BEAST_EXPECT(followingFalse.getOurVote() == false); peersUnchanged = 3; - BEAST_EXPECT(!proposingTrue.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(!proposingFalse.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(!followingTrue.stalled(p, false, peersUnchanged)); - BEAST_EXPECT(!followingFalse.stalled(p, false, peersUnchanged)); + BEAST_EXPECT( + !proposingTrue.stalled(p, true, peersUnchanged, j, clog)); + BEAST_EXPECT( + !proposingFalse.stalled(p, true, peersUnchanged, j, clog)); + BEAST_EXPECT( + !followingTrue.stalled(p, false, peersUnchanged, j, clog)); + BEAST_EXPECT( + !followingFalse.stalled(p, false, peersUnchanged, j, clog)); + BEAST_EXPECT(clog->str() == ""); // Threshold jumps to 95% BEAST_EXPECT(proposingTrue.updateVote(220, true, p)); @@ -1322,12 +1341,60 @@ public: for (peersUnchanged = 0; peersUnchanged < 6; ++peersUnchanged) { - BEAST_EXPECT(!proposingTrue.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(!proposingFalse.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(!followingTrue.stalled(p, false, peersUnchanged)); - BEAST_EXPECT(!followingFalse.stalled(p, false, peersUnchanged)); + BEAST_EXPECT( + !proposingTrue.stalled(p, true, peersUnchanged, j, clog)); + BEAST_EXPECT( + !proposingFalse.stalled(p, true, peersUnchanged, j, clog)); + BEAST_EXPECT( + !followingTrue.stalled(p, false, peersUnchanged, j, clog)); + BEAST_EXPECT( + !followingFalse.stalled(p, false, peersUnchanged, j, clog)); + BEAST_EXPECT(clog->str() == ""); } + auto expectStalled = [this, &clog]( + int txid, + bool ourVote, + int ourTime, + int peerTime, + int support, + std::uint32_t line) { + using namespace std::string_literals; + + auto const s = clog->str(); + expect(s.find("stalled"), s, __FILE__, line); + expect( + s.starts_with("Transaction "s + std::to_string(txid)), + s, + __FILE__, + line); + expect( + s.find("voting "s + (ourVote ? "YES" : "NO")) != s.npos, + s, + __FILE__, + line); + expect( + s.find("for "s + std::to_string(ourTime) + " rounds."s) != + s.npos, + s, + __FILE__, + line); + expect( + s.find( + "votes in "s + std::to_string(peerTime) + " rounds.") != + s.npos, + s, + __FILE__, + line); + expect( + s.ends_with( + "has "s + std::to_string(support) + "% support. "s), + s, + __FILE__, + line); + clog = std::make_unique(); + }; + for (int i = 0; i < 1; ++i) { BEAST_EXPECT(!proposingTrue.updateVote(250 + 10 * i, true, p)); @@ -1342,22 +1409,34 @@ public: BEAST_EXPECT(followingFalse.getOurVote() == false); // true vote has changed recently, so not stalled - BEAST_EXPECT(!proposingTrue.stalled(p, true, 0)); + BEAST_EXPECT(!proposingTrue.stalled(p, true, 0, j, clog)); + BEAST_EXPECT(clog->str() == ""); // remaining votes have been unchanged in so long that we only // need to hit the second round at 95% to be stalled, regardless // of peers - BEAST_EXPECT(proposingFalse.stalled(p, true, 0)); - BEAST_EXPECT(followingTrue.stalled(p, false, 0)); - BEAST_EXPECT(followingFalse.stalled(p, false, 0)); + BEAST_EXPECT(proposingFalse.stalled(p, true, 0, j, clog)); + expectStalled(98, false, 11, 0, 2, __LINE__); + BEAST_EXPECT(followingTrue.stalled(p, false, 0, j, clog)); + expectStalled(97, true, 11, 0, 97, __LINE__); + BEAST_EXPECT(followingFalse.stalled(p, false, 0, j, clog)); + expectStalled(96, false, 11, 0, 3, __LINE__); // true vote has changed recently, so not stalled - BEAST_EXPECT(!proposingTrue.stalled(p, true, peersUnchanged)); + BEAST_EXPECT( + !proposingTrue.stalled(p, true, peersUnchanged, j, clog)); + BEAST_EXPECTS(clog->str() == "", clog->str()); // remaining votes have been unchanged in so long that we only // need to hit the second round at 95% to be stalled, regardless // of peers - BEAST_EXPECT(proposingFalse.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(followingTrue.stalled(p, false, peersUnchanged)); - BEAST_EXPECT(followingFalse.stalled(p, false, peersUnchanged)); + BEAST_EXPECT( + proposingFalse.stalled(p, true, peersUnchanged, j, clog)); + expectStalled(98, false, 11, 6, 2, __LINE__); + BEAST_EXPECT( + followingTrue.stalled(p, false, peersUnchanged, j, clog)); + expectStalled(97, true, 11, 6, 97, __LINE__); + BEAST_EXPECT( + followingFalse.stalled(p, false, peersUnchanged, j, clog)); + expectStalled(96, false, 11, 6, 3, __LINE__); } for (int i = 1; i < 3; ++i) { @@ -1374,19 +1453,31 @@ public: // true vote changed 2 rounds ago, and peers are changing, so // not stalled - BEAST_EXPECT(!proposingTrue.stalled(p, true, 0)); + BEAST_EXPECT(!proposingTrue.stalled(p, true, 0, j, clog)); + BEAST_EXPECTS(clog->str() == "", clog->str()); // still stalled - BEAST_EXPECT(proposingFalse.stalled(p, true, 0)); - BEAST_EXPECT(followingTrue.stalled(p, false, 0)); - BEAST_EXPECT(followingFalse.stalled(p, false, 0)); + BEAST_EXPECT(proposingFalse.stalled(p, true, 0, j, clog)); + expectStalled(98, false, 11 + i, 0, 2, __LINE__); + BEAST_EXPECT(followingTrue.stalled(p, false, 0, j, clog)); + expectStalled(97, true, 11 + i, 0, 97, __LINE__); + BEAST_EXPECT(followingFalse.stalled(p, false, 0, j, clog)); + expectStalled(96, false, 11 + i, 0, 3, __LINE__); // true vote changed 2 rounds ago, and peers are NOT changing, // so stalled - BEAST_EXPECT(proposingTrue.stalled(p, true, peersUnchanged)); + BEAST_EXPECT( + proposingTrue.stalled(p, true, peersUnchanged, j, clog)); + expectStalled(99, true, 1 + i, 6, 97, __LINE__); // still stalled - BEAST_EXPECT(proposingFalse.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(followingTrue.stalled(p, false, peersUnchanged)); - BEAST_EXPECT(followingFalse.stalled(p, false, peersUnchanged)); + BEAST_EXPECT( + proposingFalse.stalled(p, true, peersUnchanged, j, clog)); + expectStalled(98, false, 11 + i, 6, 2, __LINE__); + BEAST_EXPECT( + followingTrue.stalled(p, false, peersUnchanged, j, clog)); + expectStalled(97, true, 11 + i, 6, 97, __LINE__); + BEAST_EXPECT( + followingFalse.stalled(p, false, peersUnchanged, j, clog)); + expectStalled(96, false, 11 + i, 6, 3, __LINE__); } for (int i = 3; i < 5; ++i) { @@ -1401,15 +1492,27 @@ public: BEAST_EXPECT(followingTrue.getOurVote() == true); BEAST_EXPECT(followingFalse.getOurVote() == false); - BEAST_EXPECT(proposingTrue.stalled(p, true, 0)); - BEAST_EXPECT(proposingFalse.stalled(p, true, 0)); - BEAST_EXPECT(followingTrue.stalled(p, false, 0)); - BEAST_EXPECT(followingFalse.stalled(p, false, 0)); + BEAST_EXPECT(proposingTrue.stalled(p, true, 0, j, clog)); + expectStalled(99, true, 1 + i, 0, 97, __LINE__); + BEAST_EXPECT(proposingFalse.stalled(p, true, 0, j, clog)); + expectStalled(98, false, 11 + i, 0, 2, __LINE__); + BEAST_EXPECT(followingTrue.stalled(p, false, 0, j, clog)); + expectStalled(97, true, 11 + i, 0, 97, __LINE__); + BEAST_EXPECT(followingFalse.stalled(p, false, 0, j, clog)); + expectStalled(96, false, 11 + i, 0, 3, __LINE__); - BEAST_EXPECT(proposingTrue.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(proposingFalse.stalled(p, true, peersUnchanged)); - BEAST_EXPECT(followingTrue.stalled(p, false, peersUnchanged)); - BEAST_EXPECT(followingFalse.stalled(p, false, peersUnchanged)); + BEAST_EXPECT( + proposingTrue.stalled(p, true, peersUnchanged, j, clog)); + expectStalled(99, true, 1 + i, 6, 97, __LINE__); + BEAST_EXPECT( + proposingFalse.stalled(p, true, peersUnchanged, j, clog)); + expectStalled(98, false, 11 + i, 6, 2, __LINE__); + BEAST_EXPECT( + followingTrue.stalled(p, false, peersUnchanged, j, clog)); + expectStalled(97, true, 11 + i, 6, 97, __LINE__); + BEAST_EXPECT( + followingFalse.stalled(p, false, peersUnchanged, j, clog)); + expectStalled(96, false, 11 + i, 6, 3, __LINE__); } } } diff --git a/src/xrpld/app/consensus/RCLValidations.cpp b/src/xrpld/app/consensus/RCLValidations.cpp index a04047c78a..5305c95357 100644 --- a/src/xrpld/app/consensus/RCLValidations.cpp +++ b/src/xrpld/app/consensus/RCLValidations.cpp @@ -136,7 +136,7 @@ RCLValidationsAdaptor::acquire(LedgerHash const& hash) if (!ledger) { - JLOG(j_.debug()) + JLOG(j_.warn()) << "Need validated ledger for preferred ledger analysis " << hash; Application* pApp = &app_; diff --git a/src/xrpld/consensus/Consensus.cpp b/src/xrpld/consensus/Consensus.cpp index fb57687df0..d4edb1445c 100644 --- a/src/xrpld/consensus/Consensus.cpp +++ b/src/xrpld/consensus/Consensus.cpp @@ -139,11 +139,11 @@ checkConsensusReached( return false; } - // We only get stalled when every disputed transaction unequivocally has 80% - // (minConsensusPct) agreement, either for or against. That is: either under - // 20% or over 80% consensus (repectively "nay" or "yay"). This prevents - // manipulation by a minority of byzantine peers of which transactions make - // the cut to get into the ledger. + // We only get stalled when there are disputed transactions and all of them + // unequivocally have 80% (minConsensusPct) agreement, either for or + // against. That is: either under 20% or over 80% consensus (repectively + // "nay" or "yay"). This prevents manipulation by a minority of byzantine + // peers of which transactions make the cut to get into the ledger. if (stalled) { CLOG(clog) << "consensus stalled. "; diff --git a/src/xrpld/consensus/Consensus.h b/src/xrpld/consensus/Consensus.h index f3265cf381..df6cedccff 100644 --- a/src/xrpld/consensus/Consensus.h +++ b/src/xrpld/consensus/Consensus.h @@ -84,8 +84,8 @@ shouldCloseLedger( agree @param stalled the network appears to be stalled, where neither we nor our peers have changed their vote on any disputes in a - while. This is undesirable, and will cause us to end consensus - without 80% agreement. + while. This is undesirable, and should be rare, and will cause us to + end consensus without 80% agreement. @param parms Consensus constant parameters @param proposing whether we should count ourselves @param j journal for logging @@ -1712,15 +1712,29 @@ Consensus::haveConsensus( << ", disagree=" << disagree; ConsensusParms const& parms = adaptor_.parms(); - // Stalling is BAD + // Stalling is BAD. It means that we have a consensus on the close time, so + // peers are talking, but we have disputed transactions that peers are + // unable or unwilling to come to agreement on one way or the other. bool const stalled = haveCloseTimeConsensus_ && + !result_->disputes.empty() && std::ranges::all_of(result_->disputes, - [this, &parms](auto const& dispute) { + [this, &parms, &clog](auto const& dispute) { return dispute.second.stalled( parms, mode_.get() == ConsensusMode::proposing, - peerUnchangedCounter_); + peerUnchangedCounter_, + j_, + clog); }); + if (stalled) + { + std::stringstream ss; + ss << "Consensus detects as stalled with " << (agree + disagree) << "/" + << prevProposers_ << " proposers, and " << result_->disputes.size() + << " stalled disputed transactions."; + JLOG(j_.error()) << ss.str(); + CLOG(clog) << ss.str(); + } // Determine if we actually have consensus or not result_->state = checkConsensus( diff --git a/src/xrpld/consensus/DisputedTx.h b/src/xrpld/consensus/DisputedTx.h index 4ed31b77ca..e774c8366c 100644 --- a/src/xrpld/consensus/DisputedTx.h +++ b/src/xrpld/consensus/DisputedTx.h @@ -85,7 +85,12 @@ public: //! Are we and our peers "stalled" where we probably won't change //! our vote? bool - stalled(ConsensusParms const& p, bool proposing, int peersUnchanged) const + stalled( + ConsensusParms const& p, + bool proposing, + int peersUnchanged, + beast::Journal j, + std::unique_ptr const& clog) const { // at() can throw, but the map is built by hand to ensure all valid // values are available. @@ -123,8 +128,24 @@ public: int const weight = support / total; // Returns true if the tx has more than minCONSENSUS_PCT (80) percent // agreement. Either voting for _or_ voting against the tx. - return weight > p.minCONSENSUS_PCT || - weight < (100 - p.minCONSENSUS_PCT); + bool const stalled = + weight > p.minCONSENSUS_PCT || weight < (100 - p.minCONSENSUS_PCT); + + if (stalled) + { + // stalling is an error condition for even a single + // transaction. + std::stringstream s; + s << "Transaction " << ID() << " is stalled. We have been voting " + << (getOurVote() ? "YES" : "NO") << " for " << currentVoteCounter_ + << " rounds. Peers have not changed their votes in " + << peersUnchanged << " rounds. The transaction has " << weight + << "% support. "; + JLOG(j_.error()) << s.str(); + CLOG(clog) << s.str(); + } + + return stalled; } //! The disputed transaction.