From 177d44002f364f3ecd52bccf2181476c6b8c7ddd Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 26 Jul 2026 21:38:06 -0500 Subject: [PATCH 1/8] fix(chainlock): drop and score unrequested CLSIG messages A CLSIG is only ever sent in reply to a GETDATA, so a peer we have no in-flight request with is sending it unsolicited. Until now such a message was processed anyway, and ProcessNewChainLock returns without any penalty for a CLSIG at or below our best ChainLock -- the height check short-circuits before verification, deliberately, to avoid a verification DoS. Since every distinct signature blob hashes differently, the seen-cache dedup above it never catches a varied blob, so a peer could repeat that free work indefinitely: churning the bounded seen cache and competing with real LLMQ traffic on the quorum priority queue. Authorize on the request tracker, which already records per peer what we asked for. ReceivedResponse is too weak for this: it also accepts a CANDIDATE, which exists from the moment an INV is processed, so a peer could authorize its own payload by sending INV and the object back to back before SendMessages ever issued the GETDATA. Add ReceivedRequestedResponse, which succeeds only in the REQUESTED state and leaves the announcement untouched otherwise, so a premature payload is rejected without preventing the normal GETDATA. Also record why PLATFORMBAN is deliberately left ungated: it has no local ingress, so Dash Platform's push is always the first hop. Co-Authored-By: Claude Opus 5 --- src/msg_result.h | 5 + src/net_processing.cpp | 31 +++++- src/net_processing.h | 6 ++ src/test/llmq_chainlock_tests.cpp | 152 +++++++++++++++++++++++++++++- src/test/net_tests.cpp | 39 ++++++++ src/txrequest.cpp | 15 +++ src/txrequest.h | 12 +++ 7 files changed, 256 insertions(+), 4 deletions(-) diff --git a/src/msg_result.h b/src/msg_result.h index 1cccda1bbb2e..fddd87ed116c 100644 --- a/src/msg_result.h +++ b/src/msg_result.h @@ -14,6 +14,11 @@ #include #include +/** 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; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f55ecaa8ad8a..86729b508218 100644 --- a/src/net_processing.cpp +++ b/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}; @@ -5544,9 +5549,24 @@ void PeerManagerImpl::ProcessMessage( 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 +6639,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); diff --git a/src/net_processing.h b/src/net_processing.h index 965a50ea8c24..b5ef3c91b0ae 100644 --- a/src/net_processing.h +++ b/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). */ diff --git a/src/test/llmq_chainlock_tests.cpp b/src/test/llmq_chainlock_tests.cpp index 0a7c7357475c..a03f54bb3b1b 100644 --- a/src/test/llmq_chainlock_tests.cpp +++ b/src/test/llmq_chainlock_tests.cpp @@ -4,9 +4,18 @@ #include #include - +#include + +#include +#include +#include +#include +#include +#include #include #include +#include +#include #include #include @@ -16,6 +25,8 @@ #include +#include + 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 MakeClsigPeer(NodeId id) +{ + in_addr peer_in_addr{}; + peer_in_addr.s_addr = htonl(0x0a000001 + id); + auto peer{std::make_unique(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 interrupt_dummy{false}; + peerman.ProcessMessage(peer, msg_type, payload, GetTime(), 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{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() + 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() diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 7d4a48116554..73b1bdaa2318 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -129,6 +129,45 @@ BOOST_AUTO_TEST_CASE(peer_requested_object_authorizes_and_erases_per_peer_state) SetMockTime(0s); } +// GETDATA-only object types use the stricter PeerConsumeGetDataResponse, which -- unlike +// PeerConsumeObjectRequest -- must reject a bare announcement. Otherwise a peer could authorise its +// own payload by sending INV immediately followed by the object, before SendMessages ever turned +// that announcement into a request. +BOOST_AUTO_TEST_CASE(peer_getdata_response_requires_an_inflight_request) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + TestChainState& chainstate = + *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + auto peer{MakeTestPeer(/*id=*/0)}; + m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + + const CInv inv{MSG_SPORK, uint256S("04")}; + ProcessInv(*m_node.peerman, *peer, inv); + // Announced but not yet requested: the looser check accepts this, the stricter one must not. + BOOST_CHECK(!WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeGetDataResponse(peer->GetId(), inv))); + // The rejection left the candidate intact, so the GETDATA is still pending. + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 1U); + + // After SendMessages issues the GETDATA the announcement is REQUESTED and authorises once. + SetMockTime(GetTime() + 61s); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK(WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeGetDataResponse(peer->GetId(), inv))); + BOOST_CHECK(!WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeGetDataResponse(peer->GetId(), inv))); + + // Never announced at all: rejected, and no trace left behind. + const CInv never_announced{MSG_SPORK, uint256S("05")}; + BOOST_CHECK(!WITH_LOCK(::cs_main, + return m_node.peerman->PeerConsumeGetDataResponse(peer->GetId(), never_announced))); + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 0U); + + m_node.peerman->FinalizeNode(*peer); + chainstate.ResetIbd(); + SetMockTime(0s); +} + BOOST_AUTO_TEST_CASE(cnode_simple_test) { NodeId id = 0; diff --git a/src/txrequest.cpp b/src/txrequest.cpp index fa970cd531dc..b748ccaf1262 100644 --- a/src/txrequest.cpp +++ b/src/txrequest.cpp @@ -669,6 +669,16 @@ class TxRequestTracker::Impl { return true; } + bool ReceivedRequestedResponse(NodeId peer, const CInv& txhash) + { + // A REQUESTED announcement is never the CANDIDATE_BEST for its txhash, so only the + // (peer, false, txhash) half of the ByPeer index can hold it. + auto it = m_index.get().find(ByPeerView{peer, false, txhash}); + if (it == m_index.get().end() || it->GetState() != State::REQUESTED) return false; + MakeCompleted(m_index.project(it)); + return true; + } + size_t CountInFlight(NodeId peer) const { auto it = m_peerinfo.find(peer); @@ -735,6 +745,11 @@ bool TxRequestTracker::ReceivedResponse(NodeId peer, const CInv& txhash) return m_impl->ReceivedResponse(peer, txhash); } +bool TxRequestTracker::ReceivedRequestedResponse(NodeId peer, const CInv& txhash) +{ + return m_impl->ReceivedRequestedResponse(peer, txhash); +} + std::vector TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now, std::vector>* expired) { diff --git a/src/txrequest.h b/src/txrequest.h index 5b4709be86d5..48647c6408cb 100644 --- a/src/txrequest.h +++ b/src/txrequest.h @@ -189,6 +189,18 @@ class TxRequestTracker { */ bool ReceivedResponse(NodeId peer, const CInv& txhash); + /** Like ReceivedResponse, but only succeeds for an announcement we actually sent a GETDATA for. + * + * ReceivedResponse also accepts a CANDIDATE, which exists from the moment a peer's INV is processed. + * For object types that are only ever sent in reply to a GETDATA that is too weak to authorise an + * incoming object: a peer could announce a hash and immediately push the payload, before + * SendMessages had any chance to turn the announcement into a request. + * + * Returns false without altering the announcement unless it is in the REQUESTED state, so a + * premature payload leaves the candidate intact and the normal GETDATA still goes out. + */ + bool ReceivedRequestedResponse(NodeId peer, const CInv& txhash); + // The operations below inspect the data structure. /** Count how many REQUESTED announcements a peer has. */ From b8a1813e4c2729b855fbf2729b405ec0ff121498 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 26 Jul 2026 21:38:54 -0500 Subject: [PATCH 2/8] fix(llmq): drop and score unrequested DKG messages QCONTRIB/QCOMPLAINT/QJUSTIFICATION/QPCOMMITMENT travel inv -> getdata only (see NetDKG::ProcessGetData and RelayInvToParticipants), so one we have no in-flight request for was never asked for. Such a message was previously retained in the per-phase pending queue until a worker got around to verifying its signature. Authorize on PeerConsumeGetDataResponse before the message reaches the queues, replacing the PeerEraseObjectRequest call that discarded the same answer. A bare announcement does not qualify, so a peer cannot authorize its own payload by sending INV first. The check is placed last, after the MNAuth, size and structural checks, so the existing rejection paths and their scores are unchanged; feature_llmq_dkg_intake.py gains a case for a well-formed but unrequested QCONTRIB. Co-Authored-By: Claude Opus 5 --- src/llmq/net_dkg.cpp | 14 +++++++++++++- test/functional/feature_llmq_dkg_intake.py | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/llmq/net_dkg.cpp b/src/llmq/net_dkg.cpp index 2d9bf31d01ab..13da901da2d0 100644 --- a/src/llmq/net_dkg.cpp +++ b/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(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(inv_type), hash})); pending->PushPendingMessage(from, std::move(pm), hash); }); if (!dispatched) { diff --git a/test/functional/feature_llmq_dkg_intake.py b/test/functional/feature_llmq_dkg_intake.py index d626d178a463..6d955f262f73 100755 --- a/test/functional/feature_llmq_dkg_intake.py +++ b/test/functional/feature_llmq_dkg_intake.py @@ -12,6 +12,8 @@ from a verified peer. - structural pre-validation: malformed DKG payloads (valid quorum prefix, garbage body) are rejected before retention even from a verified peer. + - a well-formed DKG message that the peer never announced and was never asked for + is dropped before retention, even from a verified peer. The node must not crash; the sending peer must be scored (Misbehaving). """ @@ -119,6 +121,7 @@ def run_test(self): self.test_oversized_rejected(mn_node) self.test_malformed_rejected(mn_node) self.test_under_min_contribution_blobs_rejected(mn_node) + self.test_unrequested_rejected(mn_node) def test_unverified_sender_rejected(self, node): self.log.info("Pushed DKG messages from a non-verified peer are rejected (Misbehaving 10 each)") @@ -170,6 +173,20 @@ def test_under_min_contribution_blobs_rejected(self, node): wait_for_banscore(node, peer_id, 100) node.disconnect_p2ps() + def test_unrequested_rejected(self, node): + self.log.info("A well-formed but unrequested DKG message is dropped (Misbehaving 10)") + peer, peer_id = self.add_verified_peer(node) + wait_for_banscore(node, peer_id, 0) + # Passes every earlier check (verified sender, known quorum, size, structure) and is + # rejected purely because the peer neither announced it nor was asked for it. DKG + # messages only ever travel inv -> getdata, so a pushed one is unsolicited by + # definition and must not reach the pending queues. + with node.assert_debug_log(["unrequested DKG message"]): + peer.send_message(msg_dkg_raw(b"qcontrib", self.qcontrib_payload(blob_count=2))) + peer.sync_with_ping() + wait_for_banscore(node, peer_id, 10) + node.disconnect_p2ps() + if __name__ == '__main__': DkgIntakeTest().main() From 0225574204e11c17bc4a59327399ebc0ffe7c7d2 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 26 Jul 2026 21:39:31 -0500 Subject: [PATCH 3/8] fix(llmq): drop and score unrequested quorum commitments QFCOMMITMENT is only ever sent in reply to a GETDATA (commitments are announced via AddMineableCommitment -> PeerRelayInv), so one we have no in-flight request for was never asked for. Most of the checks in ProcessMessage reject without scoring the peer -- deliberately, since we may just be lagging behind or on another chain -- so an unsolicited sender could repeat the block lookups and mineable-commitment probes indefinitely at no cost. Authorize on PeerConsumeGetDataResponse before any of that work, replacing the m_to_erase round-trip that resolved to the weaker ReceivedResponse. CQuorumBlockProcessor holds no PeerManagerInternal reference (net_processing already includes this header, so taking one would make the dependency circular), so the check is passed in as a predicate from the call site. Co-Authored-By: Claude Opus 5 --- src/llmq/blockprocessor.cpp | 17 +++++++++++++++-- src/llmq/blockprocessor.h | 14 +++++++++++++- src/net_processing.cpp | 8 +++++++- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index a4793f0df3d8..1382d22a932d 100644 --- a/src/llmq/blockprocessor.cpp +++ b/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()); diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index 4cf2598b3f44..e567545a620a 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -18,6 +18,7 @@ #include +#include #include 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; + + [[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 pindex, BlockValidationState& state, diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 86729b508218..39d14b515772 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5542,7 +5542,13 @@ 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) { From 0eb4ec7f6268c00d59261924abb3ea72041b5b55 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 27 Jul 2026 22:38:52 +0300 Subject: [PATCH 4/8] test: hoist MakeTestPeer into the shared net test util llmq_chainlock_tests.cpp carried a byte-for-byte copy of the helper in net_tests.cpp, differing only in the base address it derives the peer address from. Move it to test/util/net, where the other net test helpers already live, and drop both copies. Also drop the test/util/validation.h include added alongside that copy: nothing in llmq_chainlock_tests.cpp uses TestChainState. --- src/test/llmq_chainlock_tests.cpp | 25 +++---------------------- src/test/net_tests.cpp | 20 +------------------- src/test/util/net.cpp | 20 ++++++++++++++++++++ src/test/util/net.h | 4 ++++ 4 files changed, 28 insertions(+), 41 deletions(-) diff --git a/src/test/llmq_chainlock_tests.cpp b/src/test/llmq_chainlock_tests.cpp index a03f54bb3b1b..1b843214e45b 100644 --- a/src/test/llmq_chainlock_tests.cpp +++ b/src/test/llmq_chainlock_tests.cpp @@ -3,8 +3,8 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include -#include #include #include @@ -247,25 +247,6 @@ namespace { //! Regtest spork key matching Params().SporkAddresses(), as used by the functional tests. constexpr const char* REGTEST_SPORK_PRIVKEY{"cP4EKFyJsHT39LDqgdcB43Y3YXjNyjb5Fuas1GQSeAtjnZWmZEQK"}; -std::unique_ptr MakeClsigPeer(NodeId id) -{ - in_addr peer_in_addr{}; - peer_in_addr.s_addr = htonl(0x0a000001 + id); - auto peer{std::make_unique(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) { @@ -314,8 +295,8 @@ BOOST_FIXTURE_TEST_CASE(unrequested_clsig_is_dropped_and_scored, TestChain100Set 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)}; + auto unsolicited_peer{MakeTestPeer(/*id=*/41)}; + auto announcing_peer{MakeTestPeer(/*id=*/42)}; m_node.peerman->InitializeNode(*unsolicited_peer, NODE_NETWORK); m_node.peerman->InitializeNode(*announcing_peer, NODE_NETWORK); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 73b1bdaa2318..ff1ee609adc7 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -38,25 +39,6 @@ using namespace std::literals; BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup) namespace { -std::unique_ptr MakeTestPeer(NodeId id) -{ - in_addr peer_in_addr{}; - peer_in_addr.s_addr = htonl(0x01020304 + id); - auto peer{std::make_unique(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 ProcessInv(PeerManager& peerman, CNode& peer, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) { diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 963abca9631f..ff51bf67326e 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -15,6 +15,7 @@ #include #include +#include #include void ConnmanTestMsg::Handshake(CNode& node, @@ -136,3 +137,22 @@ std::vector GetRandomNodeEvictionCandidates(int n_candida } return candidates; } + +std::unique_ptr MakeTestPeer(NodeId id) +{ + in_addr peer_in_addr{}; + peer_in_addr.s_addr = htonl(0x01020304 + id); + auto peer{std::make_unique(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; +} diff --git a/src/test/util/net.h b/src/test/util/net.h index 39a2cf980159..a5c51ca775cb 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -232,4 +232,8 @@ class StaticContentsSock : public Sock std::vector GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context); +/** Build a fully connected outbound peer with no socket, suitable for driving PeerManager message + * handling directly. Each id gets a distinct address so that per-peer state stays separate. */ +std::unique_ptr MakeTestPeer(NodeId id); + #endif // BITCOIN_TEST_UTIL_NET_H From 52f7e8f66649352329aa1d81d5b724c66c9fa14f Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 27 Jul 2026 22:39:28 +0300 Subject: [PATCH 5/8] fuzz: model ReceivedRequestedResponse in the txrequest harness The harness compares TxRequestTracker against a naive model of the same state machine, but ReceivedRequestedResponse was added without a matching operation, so the new transition had no coverage there. Add it as command 11. The model asserts the stricter contract directly: the call completes an announcement precisely when it was REQUESTED, and leaves everything else -- notably a CANDIDATE, which exists from the moment an inv is processed -- untouched. --- src/test/fuzz/txrequest.cpp | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/test/fuzz/txrequest.cpp b/src/test/fuzz/txrequest.cpp index 35767d555f6d..1e2814bf8978 100644 --- a/src/test/fuzz/txrequest.cpp +++ b/src/test/fuzz/txrequest.cpp @@ -247,6 +247,22 @@ class Tester assert(completed == expected_completed); } + void ReceivedRequestedResponse(int peer, int inv) + { + // Apply to naive structure: unlike ReceivedResponse, only a REQUESTED announcement is + // completed. Anything else -- including a CANDIDATE, which exists from the moment an inv is + // processed -- is left exactly as it was. + const bool expected_completed = m_announcements[inv][peer].m_state == State::REQUESTED; + if (expected_completed) { + m_announcements[inv][peer].m_state = State::COMPLETED; + Cleanup(inv); + } + + // Call TxRequestTracker's implementation, and compare its return value with the naive expectation. + const bool completed = m_tracker.ReceivedRequestedResponse(peer, INVS[inv]); + assert(completed == expected_completed); + } + void GetRequestable(int peer) { // Implement using naive structure: @@ -324,7 +340,7 @@ FUZZ_TARGET(txrequest) // Decode the input as a sequence of instructions with parameters auto it = buffer.begin(); while (it != buffer.end()) { - int cmd = *(it++) % 11; + int cmd = *(it++) % 12; int peer, invnum, delaynum; switch (cmd) { case 0: // Make time jump to the next event (m_time of CANDIDATE or REQUESTED) @@ -372,6 +388,11 @@ FUZZ_TARGET(txrequest) invnum = it == buffer.end() ? 0 : *(it++); tester.ReceivedResponse(peer, invnum % MAX_INVS); break; + case 11: // Received response to a GETDATA we actually sent + peer = it == buffer.end() ? 0 : *(it++) % MAX_PEERS; + invnum = it == buffer.end() ? 0 : *(it++); + tester.ReceivedRequestedResponse(peer, invnum % MAX_INVS); + break; default: assert(false); } From 8f7c7db8362177f4ec316f795307df1aa8da5953 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 28 Jul 2026 12:22:49 +0300 Subject: [PATCH 6/8] fix: do not score peers for late or superseded getdata responses The solicitation gate treats "no in-flight request" as "never asked for", but that conflates an unsolicited push with an honest answer to a GETDATA of ours whose tracker entry is simply gone. Two routine paths remove it: - Expiry. SetTimePoint turns an overdue REQUESTED announcement into COMPLETED, and MakeCompleted deletes the record outright when it was the last non-COMPLETED one for its hash -- the common case, since most objects are announced by one peer before any other. The interval is 5s for MSG_CLSIG, so a slightly slow peer leaves no trace at all. - ForgetTxHash. Accepting an object from any source erases every peer's announcement of it, stranding an in-flight request. Reachable whenever the object turns up locally while a GETDATA is out: a ChainLock we sign ourselves, or one submitted over RPC. Neither is misbehaviour, but both were scored 10. The score does not decay within a connection and MaybeDiscourageAndDisconnect exempts only NoBan, manual and local peers, so a run of them discourages the peer's address. On the long-lived connections these object types travel over -- DKG and ChainLock traffic between masternodes -- that is the wrong outcome. Record, per peer, the GETDATA-only objects we actually asked them for, and report the outcome as REQUESTED, LATE or UNREQUESTED. Only UNREQUESTED is scored; LATE is processed as before. Only we ever write to that record, so a peer cannot authorise its own payload by announcing it first, and the strict REQUESTED check is unchanged for everything else. The record is bounded on three axes, and each closes a way to abuse it: - Consumption. Each entry is erased by the answer it authorises, on the REQUESTED and LATE paths alike, so one GETDATA buys exactly one accepted object. A grace that was not consumed would let a peer induce a single request and then replay that payload indefinitely, unscored. That is not theoretical: CQuorumBlockProcessor::ProcessMessage has no payload dedup ahead of its ::cs_main block lookup, and PushPendingMessage counts a message against the sender's DKG quota before the seenMessages check. - Time. Entries age out after RECENT_OBJECT_REQUEST_TTL_INTERVALS of the per-type request interval, so a peer cannot bank an unanswered request and redeem it much later. The window is expressed in GetObjectInterval() because that is already this node's statement of how long it will wait before asking someone else: one interval to answer, one more before the answer stops counting as an answer. - Type. The record keeps the type we asked for, and both the match and the window come from it rather than from the answer. The type in an INV is whatever the peer said it was and is not bound to the payload until that payload arrives, so a peer can announce the hash of a DKG message as MSG_CLSIG -- no collision needed, it picks the hash -- and answer with the DKG message. Keying on the hash alone would authorise that, bypassing the per-type AlreadyHave suppression that would otherwise have stopped us requesting it at all. MAX_RECENT_OBJECT_REQUESTS is therefore only a memory ceiling. The TTL governs lifetime, and evicting early costs the grace for the oldest requests -- degrading them to the behaviour of the gate without this record -- never correctness. The gated types were implicit across three files; name them once in IsGetDataOnlyObject, together with why the other inv-driven types are excluded. Also correct the DKG comment, which read as though the gate ran before all meaningful work: it runs last, on purpose, so the existing rejections keep their heavier penalties, and so it bounds retention and signature verification rather than parsing. --- src/llmq/blockprocessor.cpp | 4 +- src/llmq/blockprocessor.h | 9 ++- src/llmq/net_dkg.cpp | 16 ++-- src/msg_result.h | 5 +- src/net_processing.cpp | 125 +++++++++++++++++++++++++++--- src/net_processing.h | 30 +++++-- src/test/llmq_chainlock_tests.cpp | 4 +- src/test/net_tests.cpp | 16 ++-- 8 files changed, 173 insertions(+), 36 deletions(-) diff --git a/src/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index 1382d22a932d..d84485cc8a8a 100644 --- a/src/llmq/blockprocessor.cpp +++ b/src/llmq/blockprocessor.cpp @@ -73,8 +73,8 @@ 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 + // A QFCOMMITMENT is only ever sent in reply to a GETDATA (see ProcessGetData), so one we never + // asked this peer for was pushed at us. 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 diff --git a/src/llmq/blockprocessor.h b/src/llmq/blockprocessor.h index e567545a620a..e76cc5b32cde 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -63,9 +63,12 @@ class CQuorumBlockProcessor CQuorumSnapshotManager& qsnapman, int8_t bls_threads); ~CQuorumBlockProcessor(); - //! 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. + //! Predicate answering "do we have any record of asking this peer for the inv?", consuming that + //! record as a side effect. An answer that is merely late or was superseded still returns true; + //! false means we have nothing to show we asked, which is either because we did not or because + //! the answer came too long after we did (see GetDataResponse). 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 diff --git a/src/llmq/net_dkg.cpp b/src/llmq/net_dkg.cpp index 13da901da2d0..e533aadf8777 100644 --- a/src/llmq/net_dkg.cpp +++ b/src/llmq/net_dkg.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -484,12 +485,17 @@ void NetDKG::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStre 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. + // never asked this peer for was pushed at us 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. + // + // This check runs last so that every pre-existing rejection above -- and the heavier penalty it + // carries -- is unchanged. It therefore bounds retention and signature verification, not the + // parsing and structural validation above, which an unsolicited sender still gets to trigger. const CInv inv{static_cast(inv_type), hash}; - if (!WITH_LOCK(::cs_main, return m_peer_manager->PeerConsumeGetDataResponse(from, inv))) { + if (WITH_LOCK(::cs_main, return m_peer_manager->PeerConsumeGetDataResponse(from, inv)) == + GetDataResponse::UNREQUESTED) { 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; diff --git a/src/msg_result.h b/src/msg_result.h index fddd87ed116c..9ab9f0294b53 100644 --- a/src/msg_result.h +++ b/src/msg_result.h @@ -15,8 +15,9 @@ #include /** 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. */ + * fatal: it takes a run of these to discourage a peer, which leaves room for the honest cases the + * in-flight check cannot see on its own (see GetDataResponse) to be misjudged without cutting off + * a useful peer. */ static constexpr int UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE{10}; struct MisbehavingError diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 39d14b515772..2dbe0f004678 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -34,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -92,6 +94,23 @@ static constexpr auto NONPREF_PEER_TX_DELAY{2s}; /** How long to delay requesting objects from overloaded peers (see * MAX_PEER_OBJECT_REQUEST_IN_FLIGHT). */ static constexpr auto OVERLOADED_PEER_OBJECT_DELAY{2s}; +/** How many request intervals after sending a GETDATA an answer still counts as merely late rather + * than unsolicited (see CNodeState::m_recent_object_requests). + * + * Expressed in GetObjectInterval() rather than as a wall-clock constant, because that interval is + * already this node's statement of how long it is willing to wait before asking someone else: one + * interval to answer, one more before the answer stops counting as an answer. A peer lagging beyond + * that is not slow, it is failing to serve what it advertised, and the object has long since been + * fetched elsewhere. Keeping the two in the same currency also means they stay in step if the + * per-type intervals are ever changed. */ +static constexpr int RECENT_OBJECT_REQUEST_TTL_INTERVALS{2}; +/** Memory ceiling for the per-peer record of GETDATA-only requests (see + * CNodeState::m_recent_object_requests). RECENT_OBJECT_REQUEST_TTL governs how long an entry is + * meant to live; this only stops the record growing without bound when a peer is asked for more + * objects than this within that window. Evicting early costs the late-answer grace for the oldest + * requests -- degrading them to the behaviour of the gate without this record -- never correctness, + * so it does not have to be proved large enough for any particular burst. */ +static constexpr size_t MAX_RECENT_OBJECT_REQUESTS{256}; /** How long to wait before downloading a transaction from an additional peer */ static constexpr auto GETDATA_TX_INTERVAL{60s}; /** Limit to avoid sending big packets. Not used in processing incoming GETDATA for compatibility */ @@ -433,6 +452,18 @@ struct Peer { using PeerRef = std::shared_ptr; +/** A GETDATA we sent for a GETDATA-only object: which type we asked for, and when. + * + * The type has to be remembered rather than taken from the answer, because the type in an INV is + * whatever the peer said it was and is not bound to the payload until that payload arrives. A peer + * can announce the hash of a DKG message as MSG_CLSIG, collect our GETDATA, and then send the DKG + * message: the hashes match, so a record keyed on the hash alone would authorise it and would take + * its grace from the answer's type rather than the request's. */ +struct RequestedObject { + uint32_t m_inv_type{0}; + std::chrono::microseconds m_time{0}; +}; + /** * Maintain validation-specific state about nodes, protected by cs_main, instead * by CNode's own locks. This simplifies asynchronous operation, where @@ -516,6 +547,17 @@ struct CNodeState { //! A rolling bloom filter of all announced tx CInvs to this peer. CRollingBloomFilter m_recently_announced_invs = CRollingBloomFilter{INVENTORY_MAX_RECENT_RELAY, 0.000001}; + //! The GETDATA-only objects (see IsGetDataOnlyObject) we have recently asked this peer for. The + //! tracker cannot answer "did we ever ask?" on its own: an announcement is erased once it + //! expires as the sole one for its hash, or once the object is accepted from anywhere. + //! + //! Only we ever add to this, so a peer cannot use it to authorise its own payload. An entry is + //! erased by the answer it authorises and ages out after RECENT_OBJECT_REQUEST_TTL_INTERVALS, so + //! one GETDATA buys exactly one accepted object, within a bounded window: a peer can neither + //! replay the payload it induced a request for, nor bank an unanswered request to spend later. + unordered_lru_cache + m_recent_object_requests; + CNodeState(bool is_inbound) : m_is_inbound(is_inbound) {} }; @@ -593,7 +635,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); + GetDataResponse 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); @@ -1533,6 +1575,30 @@ std::chrono::microseconds GetObjectInterval(int invType) } } +bool IsGetDataOnlyObject(int invType) +{ + // These object types only ever travel inv -> getdata -> object: the only places that send them + // are the GETDATA handlers (PeerManagerImpl::ProcessGetData and NetDKG::ProcessGetData), and no + // local producer pushes them. Receiving one we never asked for is therefore always unsolicited, + // which lets the handlers drop it before doing any work on the sender's behalf. + // + // Not every inv-driven type belongs here. QSIGREC (proactive relay to peers that sent + // QSENDRECSIGS), MSG_DSQ (SENDDSQUEUE), ISDLOCK (pushed alongside MERKLEBLOCK for BIP37 clients), + // SPORK (bulk push in reply to GETSPORKS) and PLATFORMBAN (injected by a Dash Platform node) + // all have a legitimate unsolicited-push path. + switch (invType) { + case MSG_CLSIG: + case MSG_QUORUM_FINAL_COMMITMENT: + case MSG_QUORUM_CONTRIB: + case MSG_QUORUM_COMPLAINT: + case MSG_QUORUM_JUSTIFICATION: + case MSG_QUORUM_PREMATURE_COMMITMENT: + return true; + default: + return false; + } +} + void PeerManagerImpl::AddObjectAnnouncement(const CNode& node, const CInv& inv, std::chrono::microseconds current_time) { AssertLockHeld(cs_main); @@ -5546,7 +5612,8 @@ void PeerManagerImpl::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)); + return PeerConsumeGetDataResponse(pfrom.GetId(), inv)) != + GetDataResponse::UNREQUESTED; }), pfrom.GetId()); PostProcessMessage(ProcessPlatformBanMessage(pfrom.GetId(), msg_type, vRecv), pfrom.GetId()); @@ -5557,14 +5624,15 @@ void PeerManagerImpl::ProcessMessage( vRecv >> clsig; 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))) { + // never asked this peer for was pushed at us. 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. Authorise 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)) == + GetDataResponse::UNREQUESTED) { 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"); @@ -6604,6 +6672,13 @@ bool PeerManagerImpl::SendMessages(CNode* pto) vGetData.clear(); } m_object_request.RequestedTx(pto->GetId(), inv, current_time + GetObjectInterval(inv.type)); + if (IsGetDataOnlyObject(inv.type)) { + // Remember that we asked, so that an answer arriving after the tracker entry is + // gone -- expired, or erased because the object turned up elsewhere -- is not + // mistaken for an unsolicited push. See GetDataResponse. + state.m_recent_object_requests.insert(inv.hash, + RequestedObject{inv.type, current_time}); + } } else { // We have already seen this object, no need to download. This is for belated // announcements of objects which arrived via another peer; the tracker has no @@ -6645,9 +6720,35 @@ bool PeerManagerImpl::PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv) return m_object_request.ReceivedResponse(nodeid, inv); } -bool PeerManagerImpl::PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv) +GetDataResponse PeerManagerImpl::PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv) { - return m_object_request.ReceivedRequestedResponse(nodeid, inv); + CNodeState* state = State(nodeid); + if (m_object_request.ReceivedRequestedResponse(nodeid, inv)) { + // Answered on time. Spend the late-answer grace too, so the GETDATA cannot also pay for a + // replay of the same payload. + if (state != nullptr) state->m_recent_object_requests.erase(inv.hash); + return GetDataResponse::REQUESTED; + } + // No in-flight request. Before treating this as an unsolicited push, check whether we asked this + // peer for it at all: the tracker entry is gone once the request expires as the sole one for its + // hash, or once the object is accepted from any source. Neither means the peer misbehaved. + if (state != nullptr) { + RequestedObject requested; + if (state->m_recent_object_requests.get(inv.hash, requested)) { + // Grace is one answer per GETDATA: erase it whatever the outcome, so further copies are + // unsolicited again and a peer cannot replay the payload it induced a request for. + state->m_recent_object_requests.erase(inv.hash); + // Both the type and the window come from what we asked for, never from the answer: the + // peer chose the type it announced, so letting the answer name it would let a request + // for one object type authorise another, with that other type's grace. + if (requested.m_inv_type == inv.type && + GetTime() - requested.m_time <= + GetObjectInterval(requested.m_inv_type) * RECENT_OBJECT_REQUEST_TTL_INTERVALS) { + return GetDataResponse::LATE; + } + } + } + return GetDataResponse::UNREQUESTED; } void PeerManagerImpl::PeerForgetObjectRequest(const CInv& inv) diff --git a/src/net_processing.h b/src/net_processing.h index b5ef3c91b0ae..70e136f43f82 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -45,6 +45,24 @@ static const int DISCOURAGEMENT_THRESHOLD{100}; /** Maximum number of outstanding CMPCTBLOCK requests for the same block. */ static const unsigned int MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK = 3; +/** Outcome of authorising an incoming object against what we asked a peer for. + * + * Used by object types that only ever travel inv -> getdata -> object, to tell an unsolicited push + * apart from an honest answer to a GETDATA of ours that no longer has a tracker entry. The latter + * is routine: a request expires after GetObjectInterval() and is re-pointed at another peer, and + * accepting the object from any source erases every announcement for it (see ForgetTxHash). */ +enum class GetDataResponse { + //! We had an in-flight GETDATA for this object; it has now been consumed. + REQUESTED, + //! No in-flight request, but we did send this peer a GETDATA for this object recently, so the + //! answer is merely late or was superseded. Process it, but do not treat it as unsolicited. + LATE, + //! We have no record of asking this peer for this object. Usually that is because we never did, + //! which is what makes it worth scoring -- but a record we did keep is also gone once it ages + //! out or is evicted, so an answer late enough is treated the same as one never asked for. + UNREQUESTED, +}; + struct CNodeStateStats { int m_misbehavior_score = 0; int nSyncHeight = -1; @@ -74,12 +92,14 @@ 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. + /** Consume this peer's in-flight GETDATA for the inv and report how the object was authorised. + * 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. Only UNREQUESTED leaves us with + * nothing to show we asked; see GetDataResponse for why LATE is a normal outcome for an honest + * peer, and for what else can produce UNREQUESTED besides never having asked. * Requires ::cs_main (see the PeerManagerImpl override). */ - virtual bool PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv) = 0; + virtual GetDataResponse 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). */ diff --git a/src/test/llmq_chainlock_tests.cpp b/src/test/llmq_chainlock_tests.cpp index 1b843214e45b..93b214fa0f10 100644 --- a/src/test/llmq_chainlock_tests.cpp +++ b/src/test/llmq_chainlock_tests.cpp @@ -355,8 +355,8 @@ BOOST_FIXTURE_TEST_CASE(unrequested_clsig_is_dropped_and_scored, TestChain100Set // 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))); + BOOST_CHECK(WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeGetDataResponse( + announcing_peer->GetId(), announced_inv)) == GetDataResponse::UNREQUESTED); m_node.peerman->FinalizeNode(*unsolicited_peer); m_node.peerman->FinalizeNode(*announcing_peer); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index ff1ee609adc7..0f8d17734a19 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -39,6 +39,11 @@ using namespace std::literals; BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup) namespace { +GetDataResponse ConsumeGetDataResponse(PeerManager& peerman, const CNode& peer, const CInv& inv) +{ + return WITH_LOCK(::cs_main, return peerman.PeerConsumeGetDataResponse(peer.GetId(), inv)); +} + void ProcessInv(PeerManager& peerman, CNode& peer, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) { @@ -126,23 +131,24 @@ BOOST_AUTO_TEST_CASE(peer_getdata_response_requires_an_inflight_request) auto peer{MakeTestPeer(/*id=*/0)}; m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + // MSG_SPORK is not a GETDATA-only type (see IsGetDataOnlyObject), so nothing here is ever + // softened to LATE and the strict in-flight requirement is visible on its own. const CInv inv{MSG_SPORK, uint256S("04")}; ProcessInv(*m_node.peerman, *peer, inv); // Announced but not yet requested: the looser check accepts this, the stricter one must not. - BOOST_CHECK(!WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeGetDataResponse(peer->GetId(), inv))); + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, inv) == GetDataResponse::UNREQUESTED); // The rejection left the candidate intact, so the GETDATA is still pending. BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 1U); // After SendMessages issues the GETDATA the announcement is REQUESTED and authorises once. SetMockTime(GetTime() + 61s); m_node.peerman->SendMessages(peer.get()); - BOOST_CHECK(WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeGetDataResponse(peer->GetId(), inv))); - BOOST_CHECK(!WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeGetDataResponse(peer->GetId(), inv))); + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, inv) == GetDataResponse::REQUESTED); + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, inv) == GetDataResponse::UNREQUESTED); // Never announced at all: rejected, and no trace left behind. const CInv never_announced{MSG_SPORK, uint256S("05")}; - BOOST_CHECK(!WITH_LOCK(::cs_main, - return m_node.peerman->PeerConsumeGetDataResponse(peer->GetId(), never_announced))); + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, never_announced) == GetDataResponse::UNREQUESTED); BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 0U); m_node.peerman->FinalizeNode(*peer); From d2228d5ce167c4467f110edf53af92c74673e972 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Tue, 28 Jul 2026 12:24:53 +0300 Subject: [PATCH 7/8] test: cover the getdata-response grace and its bounds The fix is only worth having if the cases it exists for are pinned, and only safe if the ways it could be abused are pinned too. Four cases in net_tests, one addition in llmq_chainlock_tests: - expired_getdata_response_is_late_not_unrequested drives the sole announcer through expiry, asserts the tracker record is gone entirely rather than merely COMPLETED, and that the answer is still LATE. The second copy is UNREQUESTED: one GETDATA, one answer. - forgotten_getdata_response_is_late_not_unrequested does the same for the ForgetTxHash path. - getdata_response_grace_does_not_cross_inv_types announces a hash as MSG_CLSIG and answers with MSG_QUORUM_CONTRIB inside the window that type would have earned, which must not be authorised. - getdata_response_grace_expires answers one second past the boundary the first case sits on, so the two pin it from either side. The two boundary cases use CLSIG_REQUEST_INTERVAL and CLSIG_LATE_GRACE rather than round numbers, so they sit exactly on the edge instead of straddling it: relaxing the comparison from <= to < fails the first, and neither would notice with looser timings. Both are kept short of TIMEOUT_INTERVAL, or MaybeSendPing marks the peer for disconnection and SendMessages returns before the getdata block. unrequested_clsig_is_dropped_and_scored gains an end-to-end replay: the same CLSIG, accepted once, is scored when sent again. --- src/test/llmq_chainlock_tests.cpp | 12 ++- src/test/net_tests.cpp | 149 ++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 3 deletions(-) diff --git a/src/test/llmq_chainlock_tests.cpp b/src/test/llmq_chainlock_tests.cpp index 93b214fa0f10..457fd7b6d880 100644 --- a/src/test/llmq_chainlock_tests.cpp +++ b/src/test/llmq_chainlock_tests.cpp @@ -354,9 +354,15 @@ BOOST_FIXTURE_TEST_CASE(unrequested_clsig_is_dropped_and_scored, TestChain100Set // 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)) == GetDataResponse::UNREQUESTED); + // One GETDATA authorises exactly one answer. Both the in-flight request and the late-answer + // grace are spent, so a replay of the very payload we asked for is unsolicited again -- a peer + // must not be able to induce one request and then repeat the payload for free. + const int score_before_replay = MisbehaviorScore(*m_node.peerman, *announcing_peer); + CDataStream replayed_payload{SER_NETWORK, PROTOCOL_VERSION}; + replayed_payload << announced_clsig; + SendMessage(*m_node.peerman, *announcing_peer, NetMsgType::CLSIG, std::move(replayed_payload)); + BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *announcing_peer), + score_before_replay + UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE); m_node.peerman->FinalizeNode(*unsolicited_peer); m_node.peerman->FinalizeNode(*announcing_peer); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 0f8d17734a19..051b7087821d 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -39,6 +39,13 @@ using namespace std::literals; BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup) namespace { +//! GetObjectInterval(MSG_CLSIG), the shortest of the per-type request intervals, and the grace that +//! follows it: RECENT_OBJECT_REQUEST_TTL_INTERVALS of them. Neither is reachable from a test, so the +//! arithmetic is spelled out here -- if either changes, the cases pinning this boundary should be +//! revisited rather than silently re-pointed at a different one. +constexpr auto CLSIG_REQUEST_INTERVAL{5s}; +constexpr auto CLSIG_LATE_GRACE{2 * CLSIG_REQUEST_INTERVAL}; + GetDataResponse ConsumeGetDataResponse(PeerManager& peerman, const CNode& peer, const CInv& inv) { return WITH_LOCK(::cs_main, return peerman.PeerConsumeGetDataResponse(peer.GetId(), inv)); @@ -156,6 +163,148 @@ BOOST_AUTO_TEST_CASE(peer_getdata_response_requires_an_inflight_request) SetMockTime(0s); } +// The tracker cannot answer "did we ever ask this peer?" on its own: an announcement is erased once +// it expires as the sole one for its hash. A peer that answers our GETDATA a little too slowly is +// then indistinguishable from one that was never asked -- unless we remember having asked. Without +// that memory an honest but slow peer accrues misbehaviour, and the score never decays within a +// connection. +BOOST_AUTO_TEST_CASE(expired_getdata_response_is_late_not_unrequested) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + TestChainState& chainstate = *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + auto peer{MakeTestPeer(/*id=*/0)}; + m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + + // MSG_CLSIG is a GETDATA-only type, and its request interval is the shortest of them all. + const CInv inv{MSG_CLSIG, uint256S("06")}; + ProcessInv(*m_node.peerman, *peer, inv); + + // Nudge past the announcement's reqtime so SendMessages issues the GETDATA. + SetMockTime(GetTime() + 2s); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 1U); + + // Answer on the last moment of the grace: the request expired at CLSIG_REQUEST_INTERVAL, and + // this peer was the only announcer, so the tracker drops the record entirely rather than keeping + // a COMPLETED one -- there is nothing left for it to consult. + SetMockTime(GetTime() + CLSIG_LATE_GRACE); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 0U); + + // The answer is late, not unsolicited: it must not be scored. + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, inv) == GetDataResponse::LATE); + // One GETDATA buys exactly one answer. Without this a peer could induce a single request and + // then replay that payload forever, unscored -- which is the abuse the gate exists to stop. + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, inv) == GetDataResponse::UNREQUESTED); + + // A hash we never asked this peer for is still unsolicited. + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, CInv{MSG_CLSIG, uint256S("07")}) == + GetDataResponse::UNREQUESTED); + + m_node.peerman->FinalizeNode(*peer); + chainstate.ResetIbd(); + SetMockTime(0s); +} + +// Accepting an object from any source erases every peer's announcement of it (ForgetTxHash), which +// likewise strands an in-flight request. Reachable in production whenever the object turns up +// locally -- a ChainLock we sign ourselves, or one submitted over RPC -- while a GETDATA is out. +BOOST_AUTO_TEST_CASE(forgotten_getdata_response_is_late_not_unrequested) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + TestChainState& chainstate = *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + auto peer{MakeTestPeer(/*id=*/0)}; + m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + + const CInv inv{MSG_CLSIG, uint256S("08")}; + ProcessInv(*m_node.peerman, *peer, inv); + + SetMockTime(GetTime() + 2s); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 1U); + + // The object arrives from somewhere else while our GETDATA is still in flight. + WITH_LOCK(::cs_main, m_node.peerman->PeerForgetObjectRequest(inv)); + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 0U); + + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, inv) == GetDataResponse::LATE); + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, inv) == GetDataResponse::UNREQUESTED); + + m_node.peerman->FinalizeNode(*peer); + chainstate.ResetIbd(); + SetMockTime(0s); +} + +// The type in an INV is whatever the peer said it was, and is not bound to the payload until that +// payload arrives. So a peer can announce the hash of an object of one gated type under another -- +// no hash collision needed, it picks the hash -- collect our GETDATA, and answer with the object it +// meant all along. The grace must be tied to the type we asked for, not the one we are handed, or +// the request authorises the substitute and lends it the wrong type's window as well. +BOOST_AUTO_TEST_CASE(getdata_response_grace_does_not_cross_inv_types) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + TestChainState& chainstate = *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + auto peer{MakeTestPeer(/*id=*/0)}; + m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + + // Announced as a ChainLock, so that is what we ask for. + const uint256 hash{uint256S("0b")}; + ProcessInv(*m_node.peerman, *peer, CInv{MSG_CLSIG, hash}); + SetMockTime(GetTime() + 2s); + m_node.peerman->SendMessages(peer.get()); + + // Strand the request so only the recorded grace is left to consult, then answer with a DKG + // message carrying that hash, inside the interval that type would have been given (120s) but + // outside the one the ChainLock request actually earned (10s). + WITH_LOCK(::cs_main, m_node.peerman->PeerForgetObjectRequest(CInv{MSG_CLSIG, hash})); + SetMockTime(GetTime() + CLSIG_LATE_GRACE + 1s); + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, CInv{MSG_QUORUM_CONTRIB, hash}) == + GetDataResponse::UNREQUESTED); + + m_node.peerman->FinalizeNode(*peer); + chainstate.ResetIbd(); + SetMockTime(0s); +} + +// The grace is bounded in time, not just in count. Without that a peer could induce a GETDATA, never +// answer it, and spend the authorisation an arbitrarily long time later -- banking one per request. +BOOST_AUTO_TEST_CASE(getdata_response_grace_expires) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + TestChainState& chainstate = *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + auto peer{MakeTestPeer(/*id=*/0)}; + m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + + const CInv inv{MSG_CLSIG, uint256S("0a")}; + ProcessInv(*m_node.peerman, *peer, inv); + SetMockTime(GetTime() + 2s); + m_node.peerman->SendMessages(peer.get()); + + // Exactly one second past the boundary that expired_getdata_response_is_late_not_unrequested + // sits on, so the two cases together pin it from both sides. + SetMockTime(GetTime() + CLSIG_LATE_GRACE + 1s); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 0U); + + BOOST_CHECK(ConsumeGetDataResponse(*m_node.peerman, *peer, inv) == GetDataResponse::UNREQUESTED); + + m_node.peerman->FinalizeNode(*peer); + chainstate.ResetIbd(); + SetMockTime(0s); +} + BOOST_AUTO_TEST_CASE(cnode_simple_test) { NodeId id = 0; From 5c35e96734d9bb217a1a401cb415e7c52ed5c37b Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 28 Jul 2026 17:55:39 -0500 Subject: [PATCH 8/8] test: cover the QFCOMMITMENT gate through the injected predicate The CLSIG and DKG gates each had a test, but nothing drove an unsolicited qfcommit through PeerManagerImpl::ProcessMessage. That left the predicate injection -- the one gate that cannot reach PeerConsumeGetDataResponse directly, because CQuorumBlockProcessor holds no PeerManagerInternal -- covered only by the compiler. The new case goes through the real dispatch path and pins: an unsolicited commitment is scored UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE for every copy, never once; a null one costs the same 10 rather than the 100 below the gate, which is what places the gate ahead of that check; an INV alone does not authorise the payload; the same payload after SendMessages has issued the GETDATA is not scored at all; and a replay of it is unsolicited again. The commitment names a quorum block we do not have, the one rejection below the gate that deliberately carries no penalty, so every point scored can only have come from the gate. Verified by neutering the lambda in net_processing to return true unconditionally: four of the five assertions fail, including the null-commitment one at 100 instead of 10. SendMessage, AnnounceInv and MisbehaviorScore move from llmq_chainlock_tests.cpp to test/util/net alongside MakeTestPeer rather than being copied a second time. Co-Authored-By: Claude Fable 5 --- src/Makefile.test.include | 1 + src/test/llmq_blockprocessor_tests.cpp | 138 +++++++++++++++++++++++++ src/test/llmq_chainlock_tests.cpp | 22 ---- src/test/util/net.cpp | 26 +++++ src/test/util/net.h | 15 +++ 5 files changed, 180 insertions(+), 22 deletions(-) create mode 100644 src/test/llmq_blockprocessor_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 807fcc9d80f5..22dea93afea6 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -135,6 +135,7 @@ BITCOIN_TESTS =\ test/key_tests.cpp \ test/lcg.h \ test/limitedmap_tests.cpp \ + test/llmq_blockprocessor_tests.cpp \ test/llmq_dkg_tests.cpp \ test/llmq_chainlock_tests.cpp \ test/llmq_commitment_tests.cpp \ diff --git a/src/test/llmq_blockprocessor_tests.cpp b/src/test/llmq_blockprocessor_tests.cpp new file mode 100644 index 000000000000..46ccfc1f1f16 --- /dev/null +++ b/src/test/llmq_blockprocessor_tests.cpp @@ -0,0 +1,138 @@ +// Copyright (c) 2026 The Dash Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +using namespace llmq; +using namespace llmq::testutils; + +BOOST_AUTO_TEST_SUITE(llmq_blockprocessor_tests) + +namespace { +CDataStream Serialized(const CFinalCommitment& qc) +{ + CDataStream payload{SER_NETWORK, PROTOCOL_VERSION}; + payload << qc; + return payload; +} +} // namespace + +// A QFCOMMITMENT is only ever sent in reply to a GETDATA, so one the peer was never asked for must +// be dropped before CQuorumBlockProcessor does any lookup work on its behalf. The block processor +// holds no PeerManagerInternal -- net_processing already includes its header, so a reference would +// be circular -- and reaches the in-flight check through a predicate injected at the call site. +// Everything below therefore goes through PeerManagerImpl::ProcessMessage: calling the handler +// directly would test the gate but not the injection, which is the part that has no compiler to +// keep it honest. +BOOST_FIXTURE_TEST_CASE(unrequested_qfcommit_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)); + + auto& blockprocessor = *m_node.llmq_ctx->quorum_block_processor; + const auto& llmq_params = GetLLMQParams(Consensus::LLMQType::LLMQ_TEST_V17); + BOOST_REQUIRE(Params().GetLLMQ(llmq_params.type).has_value()); + + auto unsolicited_peer{MakeTestPeer(/*id=*/51)}; + auto announcing_peer{MakeTestPeer(/*id=*/52)}; + m_node.peerman->InitializeNode(*unsolicited_peer, NODE_NETWORK); + m_node.peerman->InitializeNode(*announcing_peer, NODE_NETWORK); + + // quorumHash names no block we know of. That is the one rejection below the gate that + // deliberately carries no penalty -- we may simply be behind or on another chain -- so any + // score this payload collects can only have come from the gate, and the checks that do score + // cannot be mistaken for it. + const auto unsolicited_qc = CreateValidCommitment(llmq_params, GetTestBlockHash(51)); + const uint256 unsolicited_hash = ::SerializeHash(unsolicited_qc); + + // Sent twice on purpose: an unsolicited peer must not be able to repeat the block lookups and + // mineable-commitment probes for free, so both copies have to cost it. + for (int i = 0; i < 2; ++i) { + SendMessage(*m_node.peerman, *unsolicited_peer, NetMsgType::QFCOMMITMENT, Serialized(unsolicited_qc)); + } + BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *unsolicited_peer), + 2 * UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE); + // Nothing below the gate ran: had the commitment been processed it could only have ended up + // here or been rejected, and this is the observable half of that. + BOOST_CHECK(!blockprocessor.HasMineableCommitment(unsolicited_hash)); + + // The gate also runs ahead of the null-commitment check, which scores 100. A null payload from + // a peer we never asked must still cost exactly the unsolicited price -- reaching the 100 would + // mean the gate had let it through. + { + const int score_before_null = MisbehaviorScore(*m_node.peerman, *unsolicited_peer); + SendMessage(*m_node.peerman, *unsolicited_peer, NetMsgType::QFCOMMITMENT, Serialized(CFinalCommitment{})); + BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *unsolicited_peer), + score_before_null + UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE); + } + + // Announcing the commitment 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_qc = CreateValidCommitment(llmq_params, GetTestBlockHash(52)); + const CInv announced_inv{MSG_QUORUM_FINAL_COMMITMENT, ::SerializeHash(announced_qc)}; + + AnnounceInv(*m_node.peerman, *announcing_peer, announced_inv); + { + const int score_before_race = MisbehaviorScore(*m_node.peerman, *announcing_peer); + SendMessage(*m_node.peerman, *announcing_peer, NetMsgType::QFCOMMITMENT, Serialized(announced_qc)); + 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() + 61s); + m_node.peerman->SendMessages(announcing_peer.get()); + const int score_before = MisbehaviorScore(*m_node.peerman, *announcing_peer); + + SendMessage(*m_node.peerman, *announcing_peer, NetMsgType::QFCOMMITMENT, Serialized(announced_qc)); + + // Unchanged, because the unknown quorum block this commitment names is rejected without any + // penalty -- which is what proves the message got past the gate. Asserting the total exactly is + // what would catch the gate also charging a peer we did ask. + BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *announcing_peer), score_before); + + // One GETDATA authorises exactly one answer. Both the in-flight request and the late-answer + // grace are spent, so a replay of the very payload we asked for is unsolicited again -- a peer + // must not be able to induce one request and then repeat the payload for free. + SendMessage(*m_node.peerman, *announcing_peer, NetMsgType::QFCOMMITMENT, Serialized(announced_qc)); + BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *announcing_peer), + score_before + UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE); + + m_node.peerman->FinalizeNode(*unsolicited_peer); + m_node.peerman->FinalizeNode(*announcing_peer); + SetMockTime(0s); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/llmq_chainlock_tests.cpp b/src/test/llmq_chainlock_tests.cpp index 457fd7b6d880..f39ec70baa9f 100644 --- a/src/test/llmq_chainlock_tests.cpp +++ b/src/test/llmq_chainlock_tests.cpp @@ -246,28 +246,6 @@ BOOST_FIXTURE_TEST_CASE(best_chainlock_is_already_have_after_seen_cache_eviction namespace { //! Regtest spork key matching Params().SporkAddresses(), as used by the functional tests. constexpr const char* REGTEST_SPORK_PRIVKEY{"cP4EKFyJsHT39LDqgdcB43Y3YXjNyjb5Fuas1GQSeAtjnZWmZEQK"}; - -void SendMessage(PeerManager& peerman, CNode& peer, const std::string& msg_type, CDataStream&& payload) - EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) -{ - std::atomic interrupt_dummy{false}; - peerman.ProcessMessage(peer, msg_type, payload, GetTime(), 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{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 diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index ff51bf67326e..a33a71a3a4a8 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -14,8 +14,14 @@ #include #include #include +#include +#include +#include +#include +#include #include +#include #include void ConnmanTestMsg::Handshake(CNode& node, @@ -156,3 +162,23 @@ std::unique_ptr MakeTestPeer(NodeId id) peer->fSuccessfullyConnected = true; return peer; } + +void SendMessage(PeerManager& peerman, CNode& peer, const std::string& msg_type, CDataStream&& payload) +{ + std::atomic interrupt_dummy{false}; + peerman.ProcessMessage(peer, msg_type, payload, GetTime(), interrupt_dummy); +} + +void AnnounceInv(PeerManager& peerman, CNode& peer, const CInv& inv) +{ + CDataStream inv_stream{SER_NETWORK, PROTOCOL_VERSION}; + inv_stream << std::vector{inv}; + SendMessage(peerman, peer, NetMsgType::INV, std::move(inv_stream)); +} + +int MisbehaviorScore(PeerManager& peerman, const CNode& peer) +{ + CNodeStateStats stats; + Assert(peerman.GetNodeStateStats(peer.GetId(), stats)); + return stats.m_misbehavior_score; +} diff --git a/src/test/util/net.h b/src/test/util/net.h index a5c51ca775cb..60db0071c257 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -12,6 +12,8 @@ #include #include #include +#include +#include #include #include @@ -236,4 +238,17 @@ std::vector GetRandomNodeEvictionCandidates(int n_candida * handling directly. Each id gets a distinct address so that per-peer state stays separate. */ std::unique_ptr MakeTestPeer(NodeId id); +/** Hand one message to PeerManager as if it had arrived from `peer`, taking the real dispatch path + * rather than calling a subsystem handler directly. */ +void SendMessage(PeerManager& peerman, CNode& peer, const std::string& msg_type, CDataStream&& payload) + EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex); + +/** Announce a single inv from `peer`. Note that this only creates a request candidate: the GETDATA + * goes out later, from SendMessages. */ +void AnnounceInv(PeerManager& peerman, CNode& peer, const CInv& inv) + EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex); + +/** Misbehavior score PeerManager currently holds against `peer`. */ +int MisbehaviorScore(PeerManager& peerman, const CNode& peer); + #endif // BITCOIN_TEST_UTIL_NET_H