fix: drop and score P2P objects the peer was never asked for

src/llmq/blockprocessor.cpp

@@ -63,7 +63,8 @@ CQuorumBlockProcessor::~CQuorumBlockProcessor()
 }
 
 MessageProcessingResult CQuorumBlockProcessor::ProcessMessage(const CNode& peer, std::string_view msg_type,
-                                                              CDataStream& vRecv)
+                                                              CDataStream& vRecv,
+                                                              const ConsumeRequestFn& consume_request)
 {
     if (msg_type != NetMsgType::QFCOMMITMENT) {
         return {};
@@ -72,8 +73,20 @@ MessageProcessingResult CQuorumBlockProcessor::ProcessMessage(const CNode& peer,
     CFinalCommitment qc;
     vRecv >> qc;
 
+    // A QFCOMMITMENT is only ever sent in reply to a GETDATA (see ProcessGetData), so one we have no
+    // in-flight request for was never asked for. Drop it up front: most of the checks below reject
+    // without scoring the peer -- deliberately, since we may just be lagging behind -- so an
+    // unsolicited peer could otherwise repeat the block lookups and map probes indefinitely. A bare
+    // announcement deliberately does not qualify: it would let the peer authorise its own payload by
+    // sending INV first.
+    if (!consume_request(CInv{MSG_QUORUM_FINAL_COMMITMENT, ::SerializeHash(qc)})) {
+        LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s -- unrequested commitment from peer=%d\n", __func__,
+                 peer.GetId());
+        return MisbehavingError{UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE, "unrequested quorum commitment"};
+    }
+
+    // Note: no m_to_erase, the request was already consumed by the solicitation check above.
     MessageProcessingResult ret;
-    ret.m_to_erase = CInv{MSG_QUORUM_FINAL_COMMITMENT, ::SerializeHash(qc)};
 
     if (qc.IsNull()) {
         LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor::%s -- null commitment from peer=%d\n", __func__, peer.GetId());

src/llmq/blockprocessor.h

@@ -18,6 +18,7 @@
 
 #include <gsl/pointers.h>
 
+#include <functional>
 #include <optional>
 
 class BlockValidationState;
@@ -62,7 +63,18 @@ class CQuorumBlockProcessor
                                    CQuorumSnapshotManager& qsnapman, int8_t bls_threads);
     ~CQuorumBlockProcessor();
 
-    [[nodiscard]] MessageProcessingResult ProcessMessage(const CNode& peer, std::string_view msg_type, CDataStream& vRecv)
+    //! Predicate answering "did we ask this peer for the inv?", consuming the pending request as a
+    //! side effect. Passed in rather than reached through PeerManagerInternal because net_processing
+    //! already depends on this header; see ProcessMessage for how it is used.
+    //!
+    //! Must be invoked without ::cs_main held -- the implementation takes it. Thread-safety
+    //! analysis cannot check this through the type-erased std::function, so keep any call site
+    //! outside ProcessMessage's own LOCK(::cs_main) block.
+    using ConsumeRequestFn = std::function<bool(const CInv&)>;
+
+    [[nodiscard]] MessageProcessingResult ProcessMessage(const CNode& peer, std::string_view msg_type,
+                                                         CDataStream& vRecv,
+                                                         const ConsumeRequestFn& consume_request)
         EXCLUSIVE_LOCKS_REQUIRED(!minableCommitmentsCs);
 
     bool ProcessBlock(const CBlock& block, gsl::not_null<const CBlockIndex*> pindex, BlockValidationState& state,

src/llmq/net_dkg.cpp

@@ -482,6 +482,19 @@ void NetDKG::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStre
     const uint256 hash = hw.GetHash();
 
     const NodeId from = pfrom.GetId();
+
+    // DKG messages are only ever sent in reply to a GETDATA (see NetDKG::ProcessGetData), so one we
+    // have no in-flight request for was never asked for and must not reach the pending queues, where
+    // it would be retained until a worker gets around to verifying its signature. A bare
+    // announcement deliberately does not qualify: it would let the peer authorise its own payload by
+    // sending INV first.
+    const CInv inv{static_cast<uint32_t>(inv_type), hash};
+    if (!WITH_LOCK(::cs_main, return m_peer_manager->PeerConsumeGetDataResponse(from, inv))) {
+        LogPrint(BCLog::LLMQ_DKG, "NetDKG -- received unrequested %s %s, peer=%d\n", msg_type, hash.ToString(), from);
+        m_peer_manager->PeerMisbehaving(from, UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE, "unrequested DKG message");
+        return;
+    }
+
     const bool dispatched = m_qdkgsman.DoForHandler({llmqType, quorumIndex}, [&](CDKGSessionHandler& handler) {
         CDKGPendingMessages* pending = nullptr;
         switch (inv_type) {
@@ -499,7 +512,6 @@ void NetDKG::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStre
             break;
         }
         Assume(pending != nullptr);
-        WITH_LOCK(::cs_main, m_peer_manager->PeerEraseObjectRequest(from, CInv{static_cast<uint32_t>(inv_type), hash}));
         pending->PushPendingMessage(from, std::move(pm), hash);
     });
     if (!dispatched) {

src/msg_result.h

@@ -14,6 +14,11 @@
 #include <variant>
 #include <vector>
 
+/** Misbehaviour score for an object message the peer was never asked for. Moderate rather than
+ *  fatal: a request expires after GetObjectInterval() (5s for MSG_CLSIG), so a peer answering our
+ *  GETDATA very late looks the same as one that was never asked. */
+static constexpr int UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE{10};
+
 struct MisbehavingError
 {
     int score;

src/net_processing.cpp

@@ -593,6 +593,7 @@ class PeerManagerImpl final : public PeerManager
     bool PeerIsBanned(const NodeId node_id) override EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex);
     void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     bool PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    bool PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     void PeerForgetObjectRequest(const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     void PeerPushInventory(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void PeerRelayInv(const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
@@ -3660,6 +3661,10 @@ MessageProcessingResult PeerManagerImpl::ProcessPlatformBanMessage(NodeId node,
 
     LogPrintf("PLATFORMBAN -- hash: %s protx_hash: %s height: %d peer=%d\n", hash.ToString(), ban_msg.m_protx_hash.ToString(), ban_msg.m_requested_height, node);
 
+    // NOTE: deliberately no solicitation gate here, unlike the other GETDATA-only object types.
+    // PLATFORMBAN has no local ingress (no RPC, no internal producer): the originating Dash
+    // Platform node injects the ban by pushing the message straight to a Dash Core peer, so the
+    // first hop is always unsolicited by design. See p2p_platform_ban.py.
     MessageProcessingResult ret{};
     ret.m_to_erase = CInv{MSG_PLATFORM_BAN, hash};
 
@@ -5537,16 +5542,37 @@ void PeerManagerImpl::ProcessMessage(
             PostProcessMessage(m_cj_walletman->processMessage(pfrom, m_chainman.ActiveChainstate(), m_connman, m_mempool, msg_type, vRecv), pfrom.GetId());
         }
         PostProcessMessage(CMNAuth::ProcessMessage(pfrom, peer->m_their_services, m_connman, m_mn_metaman, m_nodeman, m_mn_sync, m_dmnman->GetListAtChainTip(), msg_type, vRecv), pfrom.GetId());
-        PostProcessMessage(m_llmq_ctx->quorum_block_processor->ProcessMessage(pfrom, msg_type, vRecv), pfrom.GetId());
+        PostProcessMessage(m_llmq_ctx->quorum_block_processor->ProcessMessage(
+                               pfrom, msg_type, vRecv,
+                               [this, &pfrom](const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(!::cs_main) {
+                                   return WITH_LOCK(::cs_main,
+                                                    return PeerConsumeGetDataResponse(pfrom.GetId(), inv));
+                               }),
+                           pfrom.GetId());
         PostProcessMessage(ProcessPlatformBanMessage(pfrom.GetId(), msg_type, vRecv), pfrom.GetId());
 
         if (msg_type == NetMsgType::CLSIG) {
             if (m_chainlocks.IsEnabled()) {
                 chainlock::ChainLockSig clsig;
                 vRecv >> clsig;
-                const uint256& hash = ::SerializeHash(clsig);
-                WITH_LOCK(::cs_main, m_object_request.ReceivedResponse(pfrom.GetId(), CInv{MSG_CLSIG, hash}));
-                PostProcessMessage(m_clhandler.ProcessNewChainLock(pfrom.GetId(), clsig, *m_llmq_ctx->qman, hash), pfrom.GetId());
+                const CInv clsig_inv{MSG_CLSIG, ::SerializeHash(clsig)};
+                // A CLSIG is only ever sent in reply to a GETDATA (see ProcessGetData), so one we
+                // have no in-flight request for was never asked for. Drop it before
+                // ProcessNewChainLock, which exits without any penalty for a CLSIG at or below our
+                // best ChainLock -- and since every distinct signature blob hashes differently, an
+                // unsolicited peer could otherwise repeat that free work indefinitely. A bare
+                // announcement deliberately does not qualify: it would let the peer authorise its
+                // own payload by sending INV first. Consume after the spork gate so a CLSIG dropped
+                // while ChainLocks are disabled does not burn a later retransmit.
+                if (!WITH_LOCK(::cs_main, return PeerConsumeGetDataResponse(pfrom.GetId(), clsig_inv))) {
+                    LogPrint(BCLog::CHAINLOCKS, "CLSIG -- received unrequested CLSIG %s, peer=%d\n",
+                             clsig_inv.hash.ToString(), pfrom.GetId());
+                    Misbehaving(*peer, UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE, "unrequested clsig");
+                    return;
+                }
+                PostProcessMessage(m_clhandler.ProcessNewChainLock(pfrom.GetId(), clsig, *m_llmq_ctx->qman,
+                                                                   clsig_inv.hash),
+                                   pfrom.GetId());
             }
             return; // CLSIG
         }
@@ -6619,6 +6645,11 @@ bool PeerManagerImpl::PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv)
     return m_object_request.ReceivedResponse(nodeid, inv);
 }
 
+bool PeerManagerImpl::PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv)
+{
+    return m_object_request.ReceivedRequestedResponse(nodeid, inv);
+}
+
 void PeerManagerImpl::PeerForgetObjectRequest(const CInv& inv)
 {
     m_object_request.ForgetTxHash(inv);

src/net_processing.h

@@ -74,6 +74,12 @@ class PeerManagerInternal
      *  announcement, so a second call without a re-announcement in between returns false.
      *  Requires ::cs_main (see the PeerManagerImpl override). */
     virtual bool PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv) = 0;
+    /** Consume this peer's in-flight GETDATA for the inv and return whether one existed. Stricter
+     *  than PeerConsumeObjectRequest: a bare announcement does not qualify, only a request we
+     *  actually sent. Use for object types that are only ever sent in reply to a GETDATA, so a peer
+     *  cannot authorise its own payload by announcing it first.
+     *  Requires ::cs_main (see the PeerManagerImpl override). */
+    virtual bool PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv) = 0;
     /** Delete all peers' announcements of the inv. Call once the object is accepted (AlreadyHave
      *  turns true), so it is not requested from anyone anymore.
      *  Requires ::cs_main (see the PeerManagerImpl override). */

src/test/llmq_chainlock_tests.cpp

@@ -4,9 +4,18 @@
 
 #include <test/util/llmq_tests.h>
 #include <test/util/setup_common.h>
-
+#include <test/util/validation.h>
+
+#include <hash.h>
+#include <masternode/meta.h>
+#include <net.h>
+#include <net_processing.h>
+#include <netaddress.h>
+#include <spork.h>
 #include <streams.h>
 #include <util/strencodings.h>
+#include <validation.h>
+#include <version.h>
 
 #include <chainlock/chainlock.h>
 #include <chainlock/handler.h>
@@ -16,6 +25,8 @@
 
 #include <boost/test/unit_test.hpp>
 
+#include <memory>
+
 using chainlock::ChainLockSig;
 using namespace llmq;
 using namespace llmq::testutils;
@@ -232,4 +243,143 @@ BOOST_FIXTURE_TEST_CASE(best_chainlock_is_already_have_after_seen_cache_eviction
     BOOST_CHECK(m_node.clhandler->AlreadyHave(CInv{MSG_CLSIG, best_hash}));
 }
 
+namespace {
+//! Regtest spork key matching Params().SporkAddresses(), as used by the functional tests.
+constexpr const char* REGTEST_SPORK_PRIVKEY{"cP4EKFyJsHT39LDqgdcB43Y3YXjNyjb5Fuas1GQSeAtjnZWmZEQK"};
+
+std::unique_ptr<CNode> MakeClsigPeer(NodeId id)
+{
+    in_addr peer_in_addr{};
+    peer_in_addr.s_addr = htonl(0x0a000001 + id);
+    auto peer{std::make_unique<CNode>(id,
+                                      /*sock=*/nullptr,
+                                      /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK},
+                                      /*nKeyedNetGroupIn=*/0,
+                                      /*nLocalHostNonceIn=*/0,
+                                      /*addrBindIn=*/CAddress{},
+                                      /*addrNameIn=*/std::string{},
+                                      /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY,
+                                      /*inbound_onion=*/false)};
+    peer->nVersion = PROTOCOL_VERSION;
+    peer->SetCommonVersion(PROTOCOL_VERSION);
+    peer->fSuccessfullyConnected = true;
+    return peer;
+}
+
+void SendMessage(PeerManager& peerman, CNode& peer, const std::string& msg_type, CDataStream&& payload)
+    EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex)
+{
+    std::atomic<bool> interrupt_dummy{false};
+    peerman.ProcessMessage(peer, msg_type, payload, GetTime<std::chrono::microseconds>(), interrupt_dummy);
+}
+
+void AnnounceInv(PeerManager& peerman, CNode& peer, const CInv& inv)
+    EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex)
+{
+    CDataStream inv_stream{SER_NETWORK, PROTOCOL_VERSION};
+    inv_stream << std::vector<CInv>{inv};
+    SendMessage(peerman, peer, NetMsgType::INV, std::move(inv_stream));
+}
+
+int MisbehaviorScore(PeerManager& peerman, const CNode& peer)
+{
+    CNodeStateStats stats;
+    BOOST_REQUIRE(peerman.GetNodeStateStats(peer.GetId(), stats));
+    return stats.m_misbehavior_score;
+}
+} // namespace
+
+// A CLSIG is only ever sent in reply to a GETDATA, so one that the peer neither announced nor was
+// asked for must be dropped before ProcessNewChainLock -- which would otherwise remember its hash
+// and do that work again for every distinct signature blob, at no cost to the sender.
+BOOST_FIXTURE_TEST_CASE(unrequested_clsig_is_dropped_and_scored, TestChain100Setup)
+{
+    LOCK(NetEventsInterface::g_msgproc_mutex);
+
+    // INV announcements for non-spork objects are only tracked outside IBD; the 100 mined blocks
+    // of this fixture already take us out of it.
+    BOOST_REQUIRE(!m_node.chainman->ActiveChainstate().IsInitialBlockDownload());
+
+    // Every Dash-specific message is offered to CMNAuth first, which asserts a loaded metadata
+    // manager. The fixture leaves it unloaded, so initialise an empty cache here.
+    BOOST_REQUIRE(m_node.mn_metaman->LoadCache(/*load_cache=*/false));
+
+    // The CLSIG branch in net_processing is gated on spork 19. The test fixture builds a bare
+    // CSporkManager, so wire up the regtest signer before setting the spork.
+    for (const auto& address : Params().SporkAddresses()) {
+        BOOST_REQUIRE(m_node.sporkman->SetSporkAddress(address));
+    }
+    BOOST_REQUIRE(m_node.sporkman->SetMinSporkKeys(Params().MinSporkKeys()));
+    BOOST_REQUIRE(m_node.sporkman->SetPrivKey(REGTEST_SPORK_PRIVKEY));
+    BOOST_REQUIRE(m_node.sporkman->UpdateSpork(SPORK_19_CHAINLOCKS_ENABLED, 0).has_value());
+    BOOST_REQUIRE(m_node.chainlocks->IsEnabled());
+
+    auto unsolicited_peer{MakeClsigPeer(/*id=*/41)};
+    auto announcing_peer{MakeClsigPeer(/*id=*/42)};
+    m_node.peerman->InitializeNode(*unsolicited_peer, NODE_NETWORK);
+    m_node.peerman->InitializeNode(*announcing_peer, NODE_NETWORK);
+
+    const auto unsolicited_clsig = CreateChainLock(200, GetTestBlockHash(41));
+    const CInv unsolicited_inv{MSG_CLSIG, ::SerializeHash(unsolicited_clsig)};
+
+    // Sent twice on purpose. Without the gate the first copy would still be scored (this chain is
+    // too short to resolve a signing quorum for height 200) but the second would hit the seen-cache
+    // dedup and cost the peer nothing -- so only charging for both proves the gate is what rejected
+    // them, and that an unsolicited peer cannot keep repeating the work for free.
+    for (int i = 0; i < 2; ++i) {
+        CDataStream unsolicited_payload{SER_NETWORK, PROTOCOL_VERSION};
+        unsolicited_payload << unsolicited_clsig;
+        SendMessage(*m_node.peerman, *unsolicited_peer, NetMsgType::CLSIG, std::move(unsolicited_payload));
+    }
+
+    // Never reached ProcessNewChainLock: the hash was not recorded in the seen cache, so the peer
+    // could not have displaced a genuine entry, and it was scored for each attempt.
+    BOOST_CHECK(!m_node.clhandler->AlreadyHave(unsolicited_inv));
+    BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *unsolicited_peer),
+                      2 * UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE);
+
+    // Announcing the CLSIG is NOT enough to authorise it. An INV creates a candidate immediately,
+    // but the GETDATA only goes out later from SendMessages, so accepting on the announcement alone
+    // would let a peer authorise its own payload by racing INV and payload back to back -- which
+    // costs it nothing and defeats the gate entirely.
+    const auto announced_clsig = CreateChainLock(201, GetTestBlockHash(42));
+    const CInv announced_inv{MSG_CLSIG, ::SerializeHash(announced_clsig)};
+
+    AnnounceInv(*m_node.peerman, *announcing_peer, announced_inv);
+    {
+        const int score_before_race = MisbehaviorScore(*m_node.peerman, *announcing_peer);
+        CDataStream raced_payload{SER_NETWORK, PROTOCOL_VERSION};
+        raced_payload << announced_clsig;
+        SendMessage(*m_node.peerman, *announcing_peer, NetMsgType::CLSIG, std::move(raced_payload));
+
+        BOOST_CHECK(!m_node.clhandler->AlreadyHave(announced_inv));
+        BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *announcing_peer),
+                          score_before_race + UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE);
+    }
+
+    // Once SendMessages has actually issued the GETDATA the same payload is authorised. The
+    // rejection above must not have consumed the candidate, or no GETDATA would go out at all.
+    SetMockTime(GetTime<std::chrono::seconds>() + 61s);
+    m_node.peerman->SendMessages(announcing_peer.get());
+    const int score_before = MisbehaviorScore(*m_node.peerman, *announcing_peer);
+
+    CDataStream announced_payload{SER_NETWORK, PROTOCOL_VERSION};
+    announced_payload << announced_clsig;
+    SendMessage(*m_node.peerman, *announcing_peer, NetMsgType::CLSIG, std::move(announced_payload));
+
+    BOOST_CHECK(m_node.clhandler->AlreadyHave(announced_inv));
+    // Exactly the pre-existing invalid-CLSIG penalty and nothing else. This fixture's chain is 100
+    // blocks, so a CLSIG at height 201 resolves to no signing quorum and ProcessNewChainLock scores
+    // 10 -- which is what proves the message got past the gate. Asserting the total exactly is what
+    // would catch the gate also charging an authorised peer.
+    BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *announcing_peer), score_before + 10);
+    // The authorisation was consumed, so a replay of the same CLSIG is now unsolicited.
+    BOOST_CHECK(!WITH_LOCK(::cs_main,
+                           return m_node.peerman->PeerConsumeGetDataResponse(announcing_peer->GetId(), announced_inv)));
+
+    m_node.peerman->FinalizeNode(*unsolicited_peer);
+    m_node.peerman->FinalizeNode(*announcing_peer);
+    SetMockTime(0s);
+}
+
 BOOST_AUTO_TEST_SUITE_END()