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/llmq/blockprocessor.cpp b/src/llmq/blockprocessor.cpp index a4793f0df3d8..d84485cc8a8a 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 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 + // 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..e76cc5b32cde 100644 --- a/src/llmq/blockprocessor.h +++ b/src/llmq/blockprocessor.h @@ -18,6 +18,7 @@ #include +#include #include class BlockValidationState; @@ -62,7 +63,21 @@ class CQuorumBlockProcessor CQuorumSnapshotManager& qsnapman, int8_t bls_threads); ~CQuorumBlockProcessor(); - [[nodiscard]] MessageProcessingResult ProcessMessage(const CNode& peer, std::string_view msg_type, CDataStream& vRecv) + //! 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 + //! 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/llmq/net_dkg.cpp b/src/llmq/net_dkg.cpp index 2d9bf31d01ab..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 @@ -482,6 +483,24 @@ 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 + // 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)) == + 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; + } + const bool dispatched = m_qdkgsman.DoForHandler({llmqType, quorumIndex}, [&](CDKGSessionHandler& handler) { CDKGPendingMessages* pending = nullptr; switch (inv_type) { @@ -499,7 +518,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/src/msg_result.h b/src/msg_result.h index 1cccda1bbb2e..9ab9f0294b53 100644 --- a/src/msg_result.h +++ b/src/msg_result.h @@ -14,6 +14,12 @@ #include #include +/** Misbehaviour score for an object message the peer was never asked for. Moderate rather than + * 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 { int score; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f55ecaa8ad8a..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,6 +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); + 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); @@ -1532,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); @@ -3660,6 +3727,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 +5608,39 @@ 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)) != + GetDataResponse::UNREQUESTED; + }), + 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 + // 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"); + return; + } + PostProcessMessage(m_clhandler.ProcessNewChainLock(pfrom.GetId(), clsig, *m_llmq_ctx->qman, + clsig_inv.hash), + pfrom.GetId()); } return; // CLSIG } @@ -6578,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 @@ -6619,6 +6720,37 @@ bool PeerManagerImpl::PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv) return m_object_request.ReceivedResponse(nodeid, inv); } +GetDataResponse PeerManagerImpl::PeerConsumeGetDataResponse(NodeId nodeid, const CInv& 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) { m_object_request.ForgetTxHash(inv); diff --git a/src/net_processing.h b/src/net_processing.h index 965a50ea8c24..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,6 +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 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 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/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); } 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 0a7c7357475c..f39ec70baa9f 100644 --- a/src/test/llmq_chainlock_tests.cpp +++ b/src/test/llmq_chainlock_tests.cpp @@ -3,10 +3,19 @@ // 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 @@ -16,6 +25,8 @@ #include +#include + using chainlock::ChainLockSig; using namespace llmq; using namespace llmq::testutils; @@ -232,4 +243,108 @@ 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"}; +} // 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{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); + + 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); + // 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); + SetMockTime(0s); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 7d4a48116554..051b7087821d 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,23 +39,16 @@ using namespace std::literals; BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup) namespace { -std::unique_ptr MakeTestPeer(NodeId id) +//! 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) { - 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; + return WITH_LOCK(::cs_main, return peerman.PeerConsumeGetDataResponse(peer.GetId(), inv)); } void ProcessInv(PeerManager& peerman, CNode& peer, const CInv& inv) @@ -129,6 +123,188 @@ 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); + + // 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(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(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(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); + chainstate.ResetIbd(); + 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; diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 963abca9631f..a33a71a3a4a8 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -14,7 +14,14 @@ #include #include #include +#include +#include +#include +#include +#include +#include +#include #include void ConnmanTestMsg::Handshake(CNode& node, @@ -136,3 +143,42 @@ 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; +} + +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 39a2cf980159..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 @@ -232,4 +234,21 @@ 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); + +/** 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 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. */ 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()