diff --git a/.github/actions/dependencies/action.yml b/.github/actions/dependencies/action.yml index e32d8934ba..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 \ @@ -36,3 +28,11 @@ runs: --options:host "&:xrpld=True" \ --settings:all build_type=${{ inputs.configuration }} \ .. + - name: upload dependencies + 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 ${{ 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 c056468fc6..6c6091cc2e 100644 --- a/.github/workflows/macos.yml +++ b/.github/workflows/macos.yml @@ -18,7 +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_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 14e98e3fb0..5beb5d291d 100644 --- a/.github/workflows/nix.yml +++ b/.github/workflows/nix.yml @@ -16,10 +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_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 4de92c8049..9e2322b119 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -18,10 +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_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/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 0599a9e7dd..898fd06fbd 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -23,8 +23,6 @@ #include #include #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/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/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/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. 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();