From 64cf27db101f127169d8cc7b1f9fb2d3a2657f6e Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Sat, 25 Jul 2026 13:32:36 -0500 Subject: [PATCH] fix: prune ChainLock seen cache with hysteresis dashpay/dash#7424 bounded ChainlockHandler::seenChainLocks by switching it to an unordered_limitedmap constructed as `seenChainLocks{MAX_SEEN_CHAINLOCKS}`. unordered_limitedmap defaults nPruneAfterSize to nMaxSize, so prune() runs on every unique insertion past 1024 entries: it allocates a vector of all 1025 iterators, std::sort()s it in full, and erases a single element, all while ChainlockHandler::cs is held. ProcessNewChainLock records the CLSIG hash before both the stale-height early return and VerifyChainLock, and stale-height CLSIGs carry no misbehavior penalty, so a peer could turn a stream of unique, unverified CLSIG hashes into a stream of O(n log n) sorts under cs -- CPU amplification on the ChainLocks relay path. Construct the cache with an explicit prune-after size of twice the retained size instead. The map now grows to 2048 entries and is pruned back to 1024 in one batch, amortising each sort over 1024 evictions rather than one, at the cost of a larger transient cache. The 2x ratio matches the default already used by unordered_lru_cache. Note that pruning less often also reduces the chance that an entry is evicted by the same prune that inserted it, since entries inserted within one second share a timestamp and tie under prune()'s comparator. Stale CLSIG duplicate suppression is unchanged: hashes are still recorded before the stale-height return, and the 24h time-based Cleanup() is unaffected. Naming now distinguishes the retained size from the temporary prune threshold (MAX_SEEN_CHAINLOCKS -> SEEN_CHAINLOCKS_RETAINED_SIZE plus a new SEEN_CHAINLOCKS_PRUNE_AFTER_SIZE), and the constructor's defaulted nPruneAfterSize is documented as the footgun it is. Tests: two new limitedmap cases prove the generic container semantics -- no prune at retained max+1, growth bounded by the trigger, and a batch prune back to the retained size on crossing it -- plus the default-threshold behavior. The ChainLock handler test no longer asserts a strict instantaneous 1024 cap; it now verifies how the handler wires the cache up and that growth past the retained size is retained until the trigger is crossed. Verified the handler test fails when the fix is reverted. --- src/chainlock/handler.cpp | 10 ++++- src/chainlock/handler.h | 16 ++++++- src/limitedmap.h | 9 ++++ src/test/limitedmap_tests.cpp | 70 +++++++++++++++++++++++++++++++ src/test/llmq_chainlock_tests.cpp | 44 +++++++++++++------ 5 files changed, 133 insertions(+), 16 deletions(-) diff --git a/src/chainlock/handler.cpp b/src/chainlock/handler.cpp index 35969fb94b12..c20ef531bf53 100644 --- a/src/chainlock/handler.cpp +++ b/src/chainlock/handler.cpp @@ -47,7 +47,7 @@ ChainlockHandler::ChainlockHandler(chainlock::Chainlocks& chainlocks, Chainstate scheduler{std::make_unique()}, scheduler_thread{ std::make_unique(std::thread(util::TraceThread, "cl-schdlr", [&] { scheduler->serviceQueue(); }))}, - seenChainLocks{MAX_SEEN_CHAINLOCKS} + seenChainLocks{SEEN_CHAINLOCKS_RETAINED_SIZE, SEEN_CHAINLOCKS_PRUNE_AFTER_SIZE} { } @@ -89,12 +89,18 @@ size_t ChainlockHandler::SeenChainLockCacheSizeForTesting() const return seenChainLocks.size(); } -size_t ChainlockHandler::SeenChainLockCacheMaxSizeForTesting() const +size_t ChainlockHandler::SeenChainLockCacheRetainedSizeForTesting() const { LOCK(cs); return seenChainLocks.max_size(); } +size_t ChainlockHandler::SeenChainLockCachePruneAfterSizeForTesting() const +{ + LOCK(cs); + return seenChainLocks.prune_after_size(); +} + void ChainlockHandler::UpdateTxFirstSeenMap(const Uint256HashSet& tx, const int64_t& time) { AssertLockNotHeld(cs); diff --git a/src/chainlock/handler.h b/src/chainlock/handler.h index 53de74deb03c..1507e4b91f02 100644 --- a/src/chainlock/handler.h +++ b/src/chainlock/handler.h @@ -54,7 +54,16 @@ class ChainlockHandler final : public CValidationInterface std::atomic tryLockChainTipScheduled{false}; std::atomic isEnabled{false}; - static constexpr size_t MAX_SEEN_CHAINLOCKS{1024}; + //! Number of recently seen CLSIG hashes retained once `seenChainLocks` is pruned. + static constexpr size_t SEEN_CHAINLOCKS_RETAINED_SIZE{1024}; + //! Size `seenChainLocks` may grow to before the next insertion prunes it back down to + //! SEEN_CHAINLOCKS_RETAINED_SIZE. Pruning sorts every entry, and CLSIG hashes are recorded + //! before the signature is verified, so pruning on each insertion past the retained size + //! lets a peer turn a stream of unique CLSIG hashes into a stream of O(n log n) sorts under + //! `cs`. Pruning only after twice the retained size amortises that cost over the entries + //! dropped in a single batch, at the price of a larger transient cache. The 2x ratio matches + //! the default in unordered_lru_cache. + static constexpr size_t SEEN_CHAINLOCKS_PRUNE_AFTER_SIZE{2 * SEEN_CHAINLOCKS_RETAINED_SIZE}; const CBlockIndex* lastNotifyChainLockBlockIndex GUARDED_BY(cs){nullptr}; Uint256HashMap txFirstSeenTime GUARDED_BY(cs); @@ -77,7 +86,10 @@ class ChainlockHandler final : public CValidationInterface bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs); void UpdateTxFirstSeenMap(const Uint256HashSet& tx, const int64_t& time) EXCLUSIVE_LOCKS_REQUIRED(!cs); size_t SeenChainLockCacheSizeForTesting() const EXCLUSIVE_LOCKS_REQUIRED(!cs); - size_t SeenChainLockCacheMaxSizeForTesting() const EXCLUSIVE_LOCKS_REQUIRED(!cs); + //! Number of entries retained after the seen cache is pruned. + size_t SeenChainLockCacheRetainedSizeForTesting() const EXCLUSIVE_LOCKS_REQUIRED(!cs); + //! Size the seen cache may grow to before it is pruned back to the retained size. + size_t SeenChainLockCachePruneAfterSizeForTesting() const EXCLUSIVE_LOCKS_REQUIRED(!cs); [[nodiscard]] MessageProcessingResult ProcessNewChainLock(NodeId from, const chainlock::ChainLockSig& clsig, const llmq::CQuorumManager& qman, diff --git a/src/limitedmap.h b/src/limitedmap.h index c719c88f9980..2b8754ce98b9 100644 --- a/src/limitedmap.h +++ b/src/limitedmap.h @@ -31,6 +31,11 @@ class unordered_limitedmap size_type nPruneAfterSize; public: + //! nMaxSizeIn is the number of elements retained after a prune. nPruneAfterSizeIn is the size + //! the map may grow to before the next insertion prunes it; it defaults to nMaxSizeIn, which + //! means prune() -- and therefore a sort of every element -- runs on *every* insertion past + //! nMaxSizeIn. Callers whose keys are attacker-supplied should pass a larger value (e.g. + //! 2 * nMaxSizeIn) so that sorting is amortised over a batch of evictions instead. explicit unordered_limitedmap(size_type nMaxSizeIn, size_type nPruneAfterSizeIn = 0) { assert(nMaxSizeIn > 0); @@ -68,7 +73,11 @@ class unordered_limitedmap return; itTarget->second = v; } + //! Number of elements retained after a prune. size_type max_size() const { return nMaxSize; } + //! Size the map is allowed to grow to before a prune is triggered. Always >= max_size(); + //! when larger, the map temporarily holds more than max_size() elements between prunes. + size_type prune_after_size() const { return nPruneAfterSize; } size_type max_size(size_type nMaxSizeIn, size_type nPruneAfterSizeIn = 0) { assert(nMaxSizeIn > 0); diff --git a/src/test/limitedmap_tests.cpp b/src/test/limitedmap_tests.cpp index f5d430e88ae3..60ec7a413345 100644 --- a/src/test/limitedmap_tests.cpp +++ b/src/test/limitedmap_tests.cpp @@ -99,4 +99,74 @@ BOOST_AUTO_TEST_CASE(limitedmap_test) BOOST_CHECK(map.empty()); } +// A map constructed with a prune-after size larger than its retained size must not prune on +// every insertion past the retained size. Instead it is allowed to grow up to the prune-after +// size and is then pruned back down to the retained size in a single batch. This amortises the +// cost of prune() -- which sorts every element -- over many insertions. +BOOST_AUTO_TEST_CASE(limitedmap_prune_after_size_test) +{ + constexpr int RETAINED_SIZE{10}; + constexpr int PRUNE_AFTER_SIZE{2 * RETAINED_SIZE}; + + unordered_limitedmap map(RETAINED_SIZE, PRUNE_AFTER_SIZE); + + // max_size() reports the retained size, not the temporary prune threshold + BOOST_CHECK_EQUAL(map.max_size(), static_cast(RETAINED_SIZE)); + + // fill the map up to (and including) the prune-after size. No prune may happen along the + // way, so every insertion is retained -- in particular size() exceeds the retained size + // without anything being evicted. + for (int i = 0; i < PRUNE_AFTER_SIZE; i++) { + map.insert(std::pair(i, i)); + BOOST_CHECK_EQUAL(map.size(), static_cast(i) + 1U); + } + + // nothing has been evicted yet, even though we are well past the retained size + BOOST_CHECK_EQUAL(map.size(), static_cast(PRUNE_AFTER_SIZE)); + for (int i = 0; i < PRUNE_AFTER_SIZE; i++) { + BOOST_CHECK(map.count(i) == 1); + } + + // crossing the prune-after size prunes back down to the retained size in one batch + map.insert(std::pair(PRUNE_AFTER_SIZE, PRUNE_AFTER_SIZE)); + BOOST_CHECK_EQUAL(map.size(), static_cast(RETAINED_SIZE)); + + // the retained entries are the ones with the highest values + for (int i = 0; i <= PRUNE_AFTER_SIZE; i++) { + if (i <= PRUNE_AFTER_SIZE - RETAINED_SIZE) { + BOOST_CHECK(map.count(i) == 0); + } else { + BOOST_CHECK(map.count(i) == 1); + } + } + + // the cycle repeats: after a prune the map grows again without evicting anything until it + // once more exceeds the prune-after size, at which point it drops back to the retained size + int next_key{PRUNE_AFTER_SIZE + 1}; + for (int i = 0; i < PRUNE_AFTER_SIZE - RETAINED_SIZE; i++, next_key++) { + map.insert(std::pair(next_key, next_key)); + BOOST_CHECK_EQUAL(map.size(), static_cast(RETAINED_SIZE + i) + 1U); + } + BOOST_CHECK_EQUAL(map.size(), static_cast(PRUNE_AFTER_SIZE)); + + map.insert(std::pair(next_key, next_key)); + BOOST_CHECK_EQUAL(map.size(), static_cast(RETAINED_SIZE)); +} + +// Without an explicit prune-after size the map prunes as soon as it exceeds the retained size, +// so it never holds more than that many elements. +BOOST_AUTO_TEST_CASE(limitedmap_default_prune_after_size_test) +{ + constexpr int RETAINED_SIZE{10}; + + unordered_limitedmap map(RETAINED_SIZE); + BOOST_CHECK_EQUAL(map.max_size(), static_cast(RETAINED_SIZE)); + + for (int i = 0; i < 4 * RETAINED_SIZE; i++) { + map.insert(std::pair(i, i)); + BOOST_CHECK_LE(map.size(), static_cast(RETAINED_SIZE)); + } + BOOST_CHECK_EQUAL(map.size(), static_cast(RETAINED_SIZE)); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/llmq_chainlock_tests.cpp b/src/test/llmq_chainlock_tests.cpp index 0a7c7357475c..ac7aed8268ce 100644 --- a/src/test/llmq_chainlock_tests.cpp +++ b/src/test/llmq_chainlock_tests.cpp @@ -196,18 +196,35 @@ BOOST_FIXTURE_TEST_CASE(seen_chainlock_cache_is_bounded, TestingSetup) { m_node.clhandler->CheckActiveState(); - const size_t max_size = m_node.clhandler->SeenChainLockCacheMaxSizeForTesting(); - BOOST_REQUIRE_GT(max_size, 0U); - - for (size_t i = 0; i < max_size + 1; ++i) { + const size_t retained_size = m_node.clhandler->SeenChainLockCacheRetainedSizeForTesting(); + const size_t prune_after_size = m_node.clhandler->SeenChainLockCachePruneAfterSizeForTesting(); + BOOST_REQUIRE_GT(retained_size, 0U); + // The cache prunes with hysteresis: it is allowed to grow past the retained size and is only + // pruned back down once it exceeds the (larger) prune-after size. Pruning sorts every entry, + // so pruning on each insertion past the retained size would be a peer-triggered CPU + // amplification path. + BOOST_REQUIRE_GT(prune_after_size, retained_size); + + const auto process = [&](size_t i) { auto clsig = CreateChainLock(static_cast(i), GetTestBlockHash(static_cast(2000 + i))); [[maybe_unused]] const auto result = m_node.clhandler->ProcessNewChainLock(/*from=*/-1, clsig, *m_node.llmq_ctx->qman, ::SerializeHash(clsig)); - BOOST_CHECK_LE(m_node.clhandler->SeenChainLockCacheSizeForTesting(), max_size); - if (i == 0) { - BOOST_CHECK_GT(m_node.clhandler->SeenChainLockCacheSizeForTesting(), 0U); - } + }; + + // Growing up to the prune-after size must not evict anything, so no prune (and no sort) has + // run yet -- in particular there is no strict cap at the retained size. + for (size_t i = 0; i < prune_after_size; ++i) { + process(i); + BOOST_CHECK_EQUAL(m_node.clhandler->SeenChainLockCacheSizeForTesting(), i + 1U); } + BOOST_CHECK_GT(m_node.clhandler->SeenChainLockCacheSizeForTesting(), retained_size); + + // Crossing the prune-after size prunes back down to the retained size in a single batch. + process(prune_after_size); + BOOST_CHECK_EQUAL(m_node.clhandler->SeenChainLockCacheSizeForTesting(), retained_size); + + // Repeated prune cycles are covered generically by limitedmap_prune_after_size_test; this + // case only pins down how ChainlockHandler wires the cache up. } BOOST_FIXTURE_TEST_CASE(best_chainlock_is_already_have_after_seen_cache_eviction, TestingSetup) @@ -219,15 +236,18 @@ BOOST_FIXTURE_TEST_CASE(best_chainlock_is_already_have_after_seen_cache_eviction BOOST_REQUIRE(m_node.chainlocks->UpdateBestChainlock(best_hash, best_clsig, /*pindex=*/nullptr)); BOOST_CHECK(m_node.clhandler->AlreadyHave(CInv{MSG_CLSIG, best_hash})); - const size_t max_size = m_node.clhandler->SeenChainLockCacheMaxSizeForTesting(); - BOOST_REQUIRE_GT(max_size, 0U); + const size_t prune_after_size = m_node.clhandler->SeenChainLockCachePruneAfterSizeForTesting(); + BOOST_REQUIRE_GT(prune_after_size, 0U); - for (size_t i = 0; i < max_size + 1; ++i) { + // Insert enough unique CLSIGs to force at least one prune of the seen cache. + for (size_t i = 0; i < prune_after_size + 1; ++i) { auto clsig = CreateChainLock(static_cast(101 + i), GetTestBlockHash(static_cast(3000 + i))); [[maybe_unused]] const auto result = m_node.clhandler->ProcessNewChainLock(/*from=*/-1, clsig, *m_node.llmq_ctx->qman, ::SerializeHash(clsig)); - BOOST_CHECK_LE(m_node.clhandler->SeenChainLockCacheSizeForTesting(), max_size); + BOOST_CHECK_LE(m_node.clhandler->SeenChainLockCacheSizeForTesting(), prune_after_size); } + BOOST_CHECK_EQUAL(m_node.clhandler->SeenChainLockCacheSizeForTesting(), + m_node.clhandler->SeenChainLockCacheRetainedSizeForTesting()); BOOST_CHECK(m_node.clhandler->AlreadyHave(CInv{MSG_CLSIG, best_hash})); }