fix: bound ChainLock seen cache

src/chainlock/handler.cpp

@@ -46,7 +46,8 @@ ChainlockHandler::ChainlockHandler(chainlock::Chainlocks& chainlocks, Chainstate
     m_mn_sync{mn_sync},
     scheduler{std::make_unique<CScheduler>()},
     scheduler_thread{
-        std::make_unique<std::thread>(std::thread(util::TraceThread, "cl-schdlr", [&] { scheduler->serviceQueue(); }))}
+        std::make_unique<std::thread>(std::thread(util::TraceThread, "cl-schdlr", [&] { scheduler->serviceQueue(); }))},
+    seenChainLocks{MAX_SEEN_CHAINLOCKS}
 {
 }
 
@@ -70,9 +71,28 @@ void ChainlockHandler::Start()
 void ChainlockHandler::Stop() { scheduler->stop(); }
 
 bool ChainlockHandler::AlreadyHave(const CInv& inv) const
+{
+    {
+        LOCK(cs);
+        if (seenChainLocks.count(inv.hash) != 0) {
+            return true;
+        }
+    }
+
+    chainlock::ChainLockSig clsig;
+    return m_chainlocks.GetChainLockByHash(inv.hash, clsig);
+}
+
+size_t ChainlockHandler::SeenChainLockCacheSizeForTesting() const
+{
+    LOCK(cs);
+    return seenChainLocks.size();
+}
+
+size_t ChainlockHandler::SeenChainLockCacheMaxSizeForTesting() const
 {
     LOCK(cs);
-    return seenChainLocks.count(inv.hash) != 0;
+    return seenChainLocks.max_size();
 }
 
 void ChainlockHandler::UpdateTxFirstSeenMap(const Uint256HashSet& tx, const int64_t& time)
@@ -91,13 +111,16 @@ MessageProcessingResult ChainlockHandler::ProcessNewChainLock(const NodeId from,
 
     {
         LOCK(cs);
-        if (!seenChainLocks.emplace(hash, GetTime<std::chrono::seconds>()).second) {
+        if (seenChainLocks.count(hash) != 0) {
             return {};
         }
+        seenChainLocks.insert({hash, GetTime<std::chrono::seconds>()});
 
-        // height is expect to check twice: preliminary (for optimization) and inside UpdateBestsChainlock (as mutex is not kept during validation)
+        // Height is checked twice: preliminary (for optimization) and inside
+        // UpdateBestChainlock, as this mutex is not kept during validation.
         if (clsig.getHeight() <= m_chainlocks.GetBestChainLockHeight()) {
-            // no need to process older/same CLSIGs
+            // Remember the hash so AlreadyHave() suppresses repeated requests
+            // for stale CLSIG announcements.
             return {};
         }
     }
@@ -295,7 +318,9 @@ void ChainlockHandler::Cleanup()
         LOCK(cs);
         for (auto it = seenChainLocks.begin(); it != seenChainLocks.end();) {
             if (GetTime<std::chrono::seconds>() - it->second >= CLEANUP_SEEN_TIMEOUT) {
-                it = seenChainLocks.erase(it);
+                const auto hash = it->first;
+                ++it;
+                seenChainLocks.erase(hash);
             } else {
                 ++it;
             }

src/chainlock/handler.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_CHAINLOCK_HANDLER_H
 #define BITCOIN_CHAINLOCK_HANDLER_H
 
+#include <limitedmap.h>
 #include <net_types.h>
 #include <primitives/transaction.h>
 #include <protocol.h>
@@ -53,10 +54,12 @@ class ChainlockHandler final : public CValidationInterface
     std::atomic<bool> tryLockChainTipScheduled{false};
     std::atomic<bool> isEnabled{false};
 
+    static constexpr size_t MAX_SEEN_CHAINLOCKS{1024};
+
     const CBlockIndex* lastNotifyChainLockBlockIndex GUARDED_BY(cs){nullptr};
     Uint256HashMap<std::chrono::seconds> txFirstSeenTime GUARDED_BY(cs);
 
-    std::map<uint256, std::chrono::seconds> seenChainLocks GUARDED_BY(cs);
+    unordered_limitedmap<uint256, std::chrono::seconds, StaticSaltedHasher> seenChainLocks GUARDED_BY(cs);
 
     CleanupThrottler<NodeClock> cleanupThrottler;
 
@@ -73,6 +76,8 @@ 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);
 
     [[nodiscard]] MessageProcessingResult ProcessNewChainLock(NodeId from, const chainlock::ChainLockSig& clsig,
                                                               const llmq::CQuorumManager& qman,

src/test/llmq_chainlock_tests.cpp

@@ -9,14 +9,18 @@
 #include <util/strencodings.h>
 
 #include <chainlock/chainlock.h>
+#include <chainlock/handler.h>
+#include <llmq/context.h>
+#include <msg_result.h>
+#include <protocol.h>
 
 #include <boost/test/unit_test.hpp>
 
 using chainlock::ChainLockSig;
 using namespace llmq;
 using namespace llmq::testutils;
 
-BOOST_FIXTURE_TEST_SUITE(llmq_chainlock_tests, BasicTestingSetup)
+BOOST_AUTO_TEST_SUITE(llmq_chainlock_tests)
 
 BOOST_AUTO_TEST_CASE(chainlock_construction_test)
 {
@@ -167,4 +171,65 @@ BOOST_AUTO_TEST_CASE(chainlock_malformed_data_test)
     }
 }
 
+BOOST_FIXTURE_TEST_CASE(stale_chainlocks_are_remembered_for_duplicate_suppression, TestingSetup)
+{
+    m_node.clhandler->CheckActiveState();
+
+    auto best_clsig = CreateChainLock(100, GetTestBlockHash(1));
+    BOOST_REQUIRE(m_node.chainlocks->UpdateBestChainlock(::SerializeHash(best_clsig), best_clsig, /*pindex=*/nullptr));
+
+    BOOST_CHECK_EQUAL(m_node.clhandler->SeenChainLockCacheSizeForTesting(), 0U);
+
+    for (uint32_t i = 0; i < 10; ++i) {
+        auto stale_clsig = CreateChainLock(100, GetTestBlockHash(1000 + i));
+        const auto hash = ::SerializeHash(stale_clsig);
+
+        [[maybe_unused]] const auto result =
+            m_node.clhandler->ProcessNewChainLock(/*from=*/0, stale_clsig, *m_node.llmq_ctx->qman, hash);
+
+        BOOST_CHECK(m_node.clhandler->AlreadyHave(CInv{MSG_CLSIG, hash}));
+        BOOST_CHECK_EQUAL(m_node.clhandler->SeenChainLockCacheSizeForTesting(), static_cast<size_t>(i) + 1U);
+    }
+}
+
+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) {
+        auto clsig = CreateChainLock(static_cast<int32_t>(i), GetTestBlockHash(static_cast<uint32_t>(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);
+        }
+    }
+}
+
+BOOST_FIXTURE_TEST_CASE(best_chainlock_is_already_have_after_seen_cache_eviction, TestingSetup)
+{
+    m_node.clhandler->CheckActiveState();
+
+    auto best_clsig = CreateChainLock(100, GetTestBlockHash(1));
+    const auto best_hash = ::SerializeHash(best_clsig);
+    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);
+
+    for (size_t i = 0; i < max_size + 1; ++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(), max_size);
+    }
+
+    BOOST_CHECK(m_node.clhandler->AlreadyHave(CInv{MSG_CLSIG, best_hash}));
+}
+
 BOOST_AUTO_TEST_SUITE_END()