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})); }