fix(net): follow up on ChainLock cache feedback

src/chainlock/handler.cpp

@@ -47,7 +47,7 @@ ChainlockHandler::ChainlockHandler(chainlock::Chainlocks& chainlocks, Chainstate
     scheduler{std::make_unique<CScheduler>()},
     scheduler_thread{
         std::make_unique<std::thread>(std::thread(util::TraceThread, "cl-schdlr", [&] { scheduler->serviceQueue(); }))},
-    seenChainLocks{SEEN_CHAINLOCKS_RETAINED_SIZE, SEEN_CHAINLOCKS_PRUNE_AFTER_SIZE}
+    seenChainLocks{SEEN_CHAINLOCKS_CUTOFF_SIZE, SEEN_CHAINLOCKS_MAX_SIZE}
 {
 }
 
@@ -89,18 +89,6 @@ size_t ChainlockHandler::SeenChainLockCacheSizeForTesting() const
     return seenChainLocks.size();
 }
 
-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);

src/chainlock/handler.h

@@ -54,16 +54,8 @@ class ChainlockHandler final : public CValidationInterface
     std::atomic<bool> tryLockChainTipScheduled{false};
     std::atomic<bool> isEnabled{false};
 
-    //! 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 partitions 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 full-map partition
-    //! passes 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};
+    static constexpr size_t SEEN_CHAINLOCKS_CUTOFF_SIZE{1024};
+    static constexpr size_t SEEN_CHAINLOCKS_MAX_SIZE{2 * SEEN_CHAINLOCKS_CUTOFF_SIZE};
 
     const CBlockIndex* lastNotifyChainLockBlockIndex GUARDED_BY(cs){nullptr};
     Uint256HashMap<std::chrono::seconds> txFirstSeenTime GUARDED_BY(cs);
@@ -86,10 +78,6 @@ 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);
-    //! 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,

src/limitedmap.h

@@ -27,25 +27,22 @@ class unordered_limitedmap
 protected:
     std::unordered_map<K, V, Hash> map;
     typedef typename std::unordered_map<K, V, Hash>::iterator iterator;
+    size_type nCutoffSize;
     size_type nMaxSize;
-    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 partition 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 partitioning is amortised over a batch of evictions instead.
-    explicit unordered_limitedmap(size_type nMaxSizeIn, size_type nPruneAfterSizeIn = 0)
+    //! nCutoffSizeIn is the number of elements retained after a prune. nMaxSizeIn is the size the
+    //! map may grow to before a prune; it defaults to nCutoffSizeIn.
+    explicit unordered_limitedmap(size_type nCutoffSizeIn, size_type nMaxSizeIn = 0)
     {
-        assert(nMaxSizeIn > 0);
-        nMaxSize = nMaxSizeIn;
-        if (nPruneAfterSizeIn == 0) {
-            nPruneAfterSize = nMaxSize;
+        assert(nCutoffSizeIn > 0);
+        nCutoffSize = nCutoffSizeIn;
+        if (nMaxSizeIn == 0) {
+            nMaxSize = nCutoffSize;
         } else {
-            nPruneAfterSize = nPruneAfterSizeIn;
+            nMaxSize = nMaxSizeIn;
         }
-        assert(nPruneAfterSize >= nMaxSize);
+        assert(nMaxSize >= nCutoffSize);
     }
     const_iterator begin() const { return map.begin(); }
     const_iterator end() const { return map.end(); }
@@ -74,26 +71,25 @@ class unordered_limitedmap
         itTarget->second = v;
     }
     //! Number of elements retained after a prune.
+    size_type cutoff_size() const { return nCutoffSize; }
+    //! Size the map is allowed to grow to before a prune is triggered. Always >= cutoff_size().
     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)
+    size_type max_size(size_type nCutoffSizeIn, size_type nMaxSizeIn = 0)
     {
-        assert(nMaxSizeIn > 0);
-        nMaxSize = nMaxSizeIn;
-        if (nPruneAfterSizeIn == 0) {
-            nPruneAfterSize = nMaxSize;
+        assert(nCutoffSizeIn > 0);
+        nCutoffSize = nCutoffSizeIn;
+        if (nMaxSizeIn == 0) {
+            nMaxSize = nCutoffSize;
         } else {
-            nPruneAfterSize = nPruneAfterSizeIn;
+            nMaxSize = nMaxSizeIn;
         }
-        assert(nPruneAfterSize >= nMaxSize);
+        assert(nMaxSize >= nCutoffSize);
         prune();
         return nMaxSize;
     }
     void prune()
     {
-        if (map.size() <= nPruneAfterSize) {
+        if (map.size() <= nMaxSize) {
             return;
         }
 
@@ -102,8 +98,8 @@ class unordered_limitedmap
         for (auto it = map.begin(); it != map.end(); ++it) {
             iterators.emplace_back(it);
         }
-        size_type tooMuch = map.size() - nMaxSize;
-        // nPruneAfterSize >= nMaxSize > 0 keeps tooMuch inside the vector, which nth_element relies on
+        size_type tooMuch = map.size() - nCutoffSize;
+        // nMaxSize >= nCutoffSize > 0 keeps tooMuch inside the vector, which nth_element relies on
         assert(tooMuch > 0 && tooMuch < iterators.size());
         // Only the entries below the eviction boundary have to be identified, their relative order does not matter
         std::nth_element(iterators.begin(), iterators.begin() + tooMuch, iterators.end(),

src/test/limitedmap_tests.cpp

@@ -99,74 +99,75 @@ 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
+// A map constructed with a max size larger than its cutoff size must not prune on
+// every insertion past the cutoff size. Instead it is allowed to grow up to the max
+// size and is then pruned back down to the cutoff size in a single batch. This amortises the
 // cost of prune() -- which partitions every element -- over many insertions.
-BOOST_AUTO_TEST_CASE(limitedmap_prune_after_size_test)
+BOOST_AUTO_TEST_CASE(limitedmap_max_size_test)
 {
-    constexpr int RETAINED_SIZE{10};
-    constexpr int PRUNE_AFTER_SIZE{2 * RETAINED_SIZE};
+    constexpr int CUTOFF_SIZE{10};
+    constexpr int MAX_SIZE{2 * CUTOFF_SIZE};
 
-    unordered_limitedmap<int, int> map(RETAINED_SIZE, PRUNE_AFTER_SIZE);
+    unordered_limitedmap<int, int> map(CUTOFF_SIZE, MAX_SIZE);
 
-    // max_size() reports the retained size, not the temporary prune threshold
-    BOOST_CHECK_EQUAL(map.max_size(), static_cast<size_t>(RETAINED_SIZE));
+    BOOST_CHECK_EQUAL(map.cutoff_size(), static_cast<size_t>(CUTOFF_SIZE));
+    BOOST_CHECK_EQUAL(map.max_size(), static_cast<size_t>(MAX_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
+    // fill the map up to (and including) the max size. No prune may happen along the
+    // way, so every insertion is retained -- in particular size() exceeds the cutoff size
     // without anything being evicted.
-    for (int i = 0; i < PRUNE_AFTER_SIZE; i++) {
+    for (int i = 0; i < MAX_SIZE; i++) {
         map.insert(std::pair<int, int>(i, i));
         BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(i) + 1U);
     }
 
-    // nothing has been evicted yet, even though we are well past the retained size
-    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(PRUNE_AFTER_SIZE));
-    for (int i = 0; i < PRUNE_AFTER_SIZE; i++) {
+    // nothing has been evicted yet, even though we are well past the cutoff size
+    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(MAX_SIZE));
+    for (int i = 0; i < MAX_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<int, int>(PRUNE_AFTER_SIZE, PRUNE_AFTER_SIZE));
-    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(RETAINED_SIZE));
+    // crossing the max size prunes back down to the cutoff size in one batch
+    map.insert(std::pair<int, int>(MAX_SIZE, MAX_SIZE));
+    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(CUTOFF_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) {
+    // the entries retained after pruning are the ones with the highest values
+    for (int i = 0; i <= MAX_SIZE; i++) {
+        if (i <= MAX_SIZE - CUTOFF_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++) {
+    // once more exceeds the max size, at which point it drops back to the cutoff size
+    int next_key{MAX_SIZE + 1};
+    for (int i = 0; i < MAX_SIZE - CUTOFF_SIZE; i++, next_key++) {
         map.insert(std::pair<int, int>(next_key, next_key));
-        BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(RETAINED_SIZE + i) + 1U);
+        BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(CUTOFF_SIZE + i) + 1U);
     }
-    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(PRUNE_AFTER_SIZE));
+    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(MAX_SIZE));
 
     map.insert(std::pair<int, int>(next_key, next_key));
-    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(RETAINED_SIZE));
+    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(CUTOFF_SIZE));
 }
 
-// Without an explicit prune-after size the map prunes as soon as it exceeds the retained size,
+// Without an explicit max size the map prunes as soon as it exceeds the cutoff size,
 // so it never holds more than that many elements.
-BOOST_AUTO_TEST_CASE(limitedmap_default_prune_after_size_test)
+BOOST_AUTO_TEST_CASE(limitedmap_default_max_size_test)
 {
-    constexpr int RETAINED_SIZE{10};
+    constexpr int CUTOFF_SIZE{10};
 
-    unordered_limitedmap<int, int> map(RETAINED_SIZE);
-    BOOST_CHECK_EQUAL(map.max_size(), static_cast<size_t>(RETAINED_SIZE));
+    unordered_limitedmap<int, int> map(CUTOFF_SIZE);
+    BOOST_CHECK_EQUAL(map.cutoff_size(), static_cast<size_t>(CUTOFF_SIZE));
+    BOOST_CHECK_EQUAL(map.max_size(), static_cast<size_t>(CUTOFF_SIZE));
 
-    for (int i = 0; i < 4 * RETAINED_SIZE; i++) {
+    for (int i = 0; i < 4 * CUTOFF_SIZE; i++) {
         map.insert(std::pair<int, int>(i, i));
-        BOOST_CHECK_LE(map.size(), static_cast<size_t>(RETAINED_SIZE));
+        BOOST_CHECK_LE(map.size(), static_cast<size_t>(CUTOFF_SIZE));
     }
-    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(RETAINED_SIZE));
+    BOOST_CHECK_EQUAL(map.size(), static_cast<size_t>(CUTOFF_SIZE));
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/test/llmq_chainlock_tests.cpp

@@ -26,11 +26,17 @@
 #include <boost/test/unit_test.hpp>
 
 #include <memory>
+#include <vector>
 
 using chainlock::ChainLockSig;
 using namespace llmq;
 using namespace llmq::testutils;
 
+namespace {
+constexpr size_t MAX_SEEN_CHAINLOCKS{2500};
+constexpr size_t RECENT_CHAINLOCKS_TO_RETAIN{2};
+} // namespace
+
 BOOST_AUTO_TEST_SUITE(llmq_chainlock_tests)
 
 BOOST_AUTO_TEST_CASE(chainlock_construction_test)
@@ -207,35 +213,28 @@ BOOST_FIXTURE_TEST_CASE(seen_chainlock_cache_is_bounded, TestingSetup)
 {
     m_node.clhandler->CheckActiveState();
 
-    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<int32_t>(i), GetTestBlockHash(static_cast<uint32_t>(2000 + i)));
+        SetMockTime(std::chrono::seconds{100000 + static_cast<int64_t>(i)});
+        auto clsig = CreateChainLock(100, GetTestBlockHash(static_cast<uint32_t>(2000 + i)));
+        const auto hash = ::SerializeHash(clsig);
         [[maybe_unused]] const auto result =
-            m_node.clhandler->ProcessNewChainLock(/*from=*/-1, clsig, *m_node.llmq_ctx->qman, ::SerializeHash(clsig));
+            m_node.clhandler->ProcessNewChainLock(/*from=*/-1, clsig, *m_node.llmq_ctx->qman, hash);
+        return hash;
     };
 
-    // 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);
+    std::vector<uint256> recent_hashes;
+    for (size_t i = 0; i <= MAX_SEEN_CHAINLOCKS; ++i) {
+        const auto hash = process(i);
+        if (i >= MAX_SEEN_CHAINLOCKS + 1 - RECENT_CHAINLOCKS_TO_RETAIN) {
+            recent_hashes.emplace_back(hash);
+        }
+        BOOST_CHECK_LE(m_node.clhandler->SeenChainLockCacheSizeForTesting(), MAX_SEEN_CHAINLOCKS);
     }
-    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.
+    for (const auto& hash : recent_hashes) {
+        BOOST_CHECK(m_node.clhandler->AlreadyHave(CInv{MSG_CLSIG, hash}));
+    }
+    SetMockTime(0s);
 }
 
 BOOST_FIXTURE_TEST_CASE(best_chainlock_is_already_have_after_seen_cache_eviction, TestingSetup)
@@ -247,18 +246,13 @@ 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 prune_after_size = m_node.clhandler->SeenChainLockCachePruneAfterSizeForTesting();
-    BOOST_REQUIRE_GT(prune_after_size, 0U);
-
     // 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) {
+    for (size_t i = 0; i <= MAX_SEEN_CHAINLOCKS; ++i) {
         auto clsig = CreateChainLock(static_cast<int32_t>(101 + i), GetTestBlockHash(static_cast<uint32_t>(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(), prune_after_size);
+        BOOST_CHECK_LE(m_node.clhandler->SeenChainLockCacheSizeForTesting(), MAX_SEEN_CHAINLOCKS);
     }
-    BOOST_CHECK_EQUAL(m_node.clhandler->SeenChainLockCacheSizeForTesting(),
-                      m_node.clhandler->SeenChainLockCacheRetainedSizeForTesting());
 
     BOOST_CHECK(m_node.clhandler->AlreadyHave(CInv{MSG_CLSIG, best_hash}));
 }