diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 42486408b59d..297c15917fda 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -147,6 +147,7 @@ BITCOIN_TESTS =\ test/llmq_hash_tests.cpp \ test/llmq_invalid_type_tests.cpp \ test/llmq_params_tests.cpp \ + test/llmq_qgetdata_tests.cpp \ test/llmq_snapshot_tests.cpp \ test/llmq_utils_tests.cpp \ test/logging_tests.cpp \ diff --git a/src/llmq/net_quorum.cpp b/src/llmq/net_quorum.cpp index 1727a8f90b0b..16f6a45757fc 100644 --- a/src/llmq/net_quorum.cpp +++ b/src/llmq/net_quorum.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -68,6 +69,11 @@ void NetQuorum::Stop() workerPool.stop(true); } +void NetQuorum::Schedule(CScheduler& scheduler) +{ + scheduler.scheduleEvery([this] { m_qman.CleanupExpiredDataRequests(); }, std::chrono::minutes{1}); +} + void NetQuorum::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) { if (msg_type == NetMsgType::QGETDATA) { @@ -104,28 +110,51 @@ void NetQuorum::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataS return misbehave; }; - const CQuorumDataRequestKey key(pfrom.GetVerifiedProRegTxHash(), false, request.GetQuorumHash(), request.GetLLMQType()); - const bool request_limit_exceeded = !m_qman.RegisterDataRequest(key, request, /*add_expiry_bias=*/false); - + // Cheap pre-checks before tracking: invalid type / unknown block must not grow + // mapQuorumDataRequests — their keyspace is attacker-controlled and unbounded, but + // rejecting them costs no storage lookup. Replies are ~request-sized (no amplification). if (!Params().GetLLMQ(request.GetLLMQType()).has_value()) { - // Unlike the misses below, this one cannot be explained by the peer being ahead of - // us: no quorum of an unregistered type can exist on this chain, so there is - // nothing to ask about. Answer with the error anyway, then score in full. - sendQDATA(CQuorumDataRequest::Errors::QUORUM_TYPE_INVALID, request_limit_exceeded); + // Unregistered type cannot exist on this chain — answer, then score in full. + sendQDATA(CQuorumDataRequest::Errors::QUORUM_TYPE_INVALID, /*request_limit_exceeded=*/false); m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100, "invalid llmqType in QGETDATA"); return; } - const CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(request.GetQuorumHash())); - if (pQuorumBaseBlockIndex == nullptr) { - if (sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, request_limit_exceeded)) { - m_peer_manager->PeerMisbehaving(pfrom.GetId(), 25, "request limit exceeded"); + const CBlockIndex* pQuorumBaseBlockIndex{nullptr}; + { + LOCK(::cs_main); + const auto* pindex = m_chainman.m_blockman.LookupBlockIndex(request.GetQuorumHash()); + if (pindex != nullptr && m_chainman.ActiveChain().Contains(pindex)) { + pQuorumBaseBlockIndex = pindex; } + } + if (pQuorumBaseBlockIndex == nullptr) { + // Not misbehavior: the requester may simply be ahead of us or on another fork. + sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, /*request_limit_exceeded=*/false); + return; + } + + // Active-chain keys are bounded by chain blocks × LLMQ types, so register the request + // before the commitment lookup: the lookup misses EvoDB's caches for blocks without a + // mined commitment, and rate limiting must gate that cost, not the other way around. + const CQuorumDataRequestKey key(pfrom.GetVerifiedProRegTxHash(), false, request.GetQuorumHash(), + request.GetLLMQType()); + const auto registration = m_qman.RegisterDataRequest(key, request, /*add_expiry_bias=*/false); + if (registration == DataRequestRegistration::RequesterLimitExceeded) { + m_peer_manager->PeerMisbehaving(pfrom.GetId(), 25, "too many quorum data requests"); + return; + } + if (registration == DataRequestRegistration::CapacityExhausted) { return; } + const bool request_limit_exceeded = registration == DataRequestRegistration::RateLimited; const auto pQuorum = m_qman.GetQuorum(request.GetLLMQType(), request.GetQuorumHash()); if (pQuorum == nullptr) { + // No mined commitment for this block. Not misbehavior on its own: the commitment is + // mined after the quorum base block, so we can know the base block while still lagging + // behind the commitment the requester saw. Repeats within the expiry window were + // rate-limited above and do score. if (sendQDATA(CQuorumDataRequest::Errors::QUORUM_NOT_FOUND, request_limit_exceeded)) { m_peer_manager->PeerMisbehaving(pfrom.GetId(), 25, "request limit exceeded"); } @@ -321,7 +350,7 @@ DataRequestStatus NetQuorum::RequestQuorumData(CNode& peer, const CQuorum& quoru quorum.m_quorum_base_block_index->GetBlockHash(), quorum.qc->llmqType); const CQuorumDataRequest request(quorum.qc->llmqType, quorum.m_quorum_base_block_index->GetBlockHash(), nDataMask, proTxHash); - if (!m_qman.RegisterDataRequest(key, request)) { + if (m_qman.RegisterDataRequest(key, request) != DataRequestRegistration::Accepted) { return m_qman.GetDataRequestStatus(peer.GetVerifiedProRegTxHash(), /*we_requested=*/true, quorum.m_quorum_base_block_index->GetBlockHash(), quorum.qc->llmqType); } diff --git a/src/llmq/net_quorum.h b/src/llmq/net_quorum.h index 5ba27c69b379..e5b42dc5ae1c 100644 --- a/src/llmq/net_quorum.h +++ b/src/llmq/net_quorum.h @@ -61,6 +61,7 @@ class NetQuorum final : public NetHandler, public CValidationInterface void Start() override; void Stop() override; void Interrupt() override { quorumThreadInterrupt(); } + void Schedule(CScheduler& scheduler) override; protected: // CValidationInterface diff --git a/src/llmq/quorumsman.cpp b/src/llmq/quorumsman.cpp index d58b73ef55e1..769595a612b3 100644 --- a/src/llmq/quorumsman.cpp +++ b/src/llmq/quorumsman.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -339,6 +340,18 @@ void CQuorumManager::CleanupExpiredDataRequests() const auto it = mapQuorumDataRequests.begin(); while (it != mapQuorumDataRequests.end()) { if (it->second.IsExpired(/*add_bias=*/true)) { + if (!it->first.m_we_requested) { + // On counter desync, skip the decrement: over-counting fails toward stricter + // limits, while an underflow would disable the caps entirely. + auto count_it = m_inbound_request_counts.find(it->first.proRegTx); + if (Assume(count_it != m_inbound_request_counts.end() && count_it->second > 0) && + --count_it->second == 0) { + m_inbound_request_counts.erase(count_it); + } + if (Assume(m_inbound_request_count > 0)) { + --m_inbound_request_count; + } + } it = mapQuorumDataRequests.erase(it); } else { ++it; @@ -433,19 +446,37 @@ CQuorumCPtr CQuorumManager::GetQuorum(Consensus::LLMQType llmqType, return BuildQuorumFromCommitment(llmqType, pQuorumBaseBlockIndex, populate_cache); } -bool CQuorumManager::RegisterDataRequest(const CQuorumDataRequestKey& key, const CQuorumDataRequest& request, - bool add_expiry_bias) const +DataRequestRegistration CQuorumManager::RegisterDataRequest(const CQuorumDataRequestKey& key, + const CQuorumDataRequest& request, bool add_expiry_bias) const { LOCK(cs_data_requests); - auto [old_pair, inserted] = mapQuorumDataRequests.emplace(key, request); - if (!inserted) { - if (old_pair->second.IsExpired(add_expiry_bias)) { - old_pair->second = request; - return true; + if (auto it = mapQuorumDataRequests.find(key); it != mapQuorumDataRequests.end()) { + if (!it->second.IsExpired(add_expiry_bias)) { + return DataRequestRegistration::RateLimited; } - return false; + it->second = request; + return DataRequestRegistration::Accepted; } - return true; + + if (!key.m_we_requested) { + if (m_inbound_request_count >= MAX_INBOUND_DATA_REQUESTS) { + return DataRequestRegistration::CapacityExhausted; + } + const auto count_it = m_inbound_request_counts.find(key.proRegTx); + if (count_it != m_inbound_request_counts.end() && count_it->second >= MAX_INBOUND_DATA_REQUESTS_PER_REQUESTER) { + // All unauthenticated qwatch peers share the null identity, so exhaustion of that + // budget is not attributable to the connection that happened to arrive last. + return key.proRegTx.IsNull() ? DataRequestRegistration::CapacityExhausted + : DataRequestRegistration::RequesterLimitExceeded; + } + } + + mapQuorumDataRequests.emplace(key, request); + if (!key.m_we_requested) { + ++m_inbound_request_counts[key.proRegTx]; + ++m_inbound_request_count; + } + return DataRequestRegistration::Accepted; } CQuorumManager::DataResponseValidation CQuorumManager::ValidateDataResponse( diff --git a/src/llmq/quorumsman.h b/src/llmq/quorumsman.h index b2fad2533ee9..dc539927a077 100644 --- a/src/llmq/quorumsman.h +++ b/src/llmq/quorumsman.h @@ -48,6 +48,18 @@ class CDKGSessionManager; class CQuorumBlockProcessor; class CQuorumSnapshotManager; +//! A peer normally needs data for only a handful of quorums at once. These limits keep +//! attacker-controlled inbound tracking bounded independently of request expiry or block production. +static constexpr size_t MAX_INBOUND_DATA_REQUESTS_PER_REQUESTER{64}; +static constexpr size_t MAX_INBOUND_DATA_REQUESTS{4096}; + +enum class DataRequestRegistration : uint8_t { + Accepted, + RateLimited, + RequesterLimitExceeded, + CapacityExhausted, +}; + /** * The quorum manager maintains quorums which were mined on chain. When a quorum is requested from the manager, * it will lookup the commitment (through CQuorumBlockProcessor) and build a CQuorum object from it. @@ -70,6 +82,8 @@ class CQuorumManager final mutable Mutex cs_data_requests; mutable std::unordered_map mapQuorumDataRequests GUARDED_BY(cs_data_requests); + mutable std::unordered_map m_inbound_request_counts GUARDED_BY(cs_data_requests); + mutable size_t m_inbound_request_count GUARDED_BY(cs_data_requests){0}; mutable Mutex m_cs_maps; mutable std::map> mapQuorumsCache @@ -140,9 +154,10 @@ class CQuorumManager final bool IsMasternode() const; bool IsWatching() const; - //! Request tracking for QGETDATA/QDATA — used by NetQuorum and RPC - bool RegisterDataRequest(const CQuorumDataRequestKey& key, const CQuorumDataRequest& request, - bool add_expiry_bias = true) const + //! Request tracking for QGETDATA/QDATA — used by NetQuorum and RPC. Inbound entries are + //! bounded per requester and globally; outbound entries initiated by us do not consume either budget. + DataRequestRegistration RegisterDataRequest(const CQuorumDataRequestKey& key, const CQuorumDataRequest& request, + bool add_expiry_bias = true) const EXCLUSIVE_LOCKS_REQUIRED(!cs_data_requests); enum class DataResponseValidation : uint8_t { OK, NotRequested, AlreadyReceived, Mismatch }; DataResponseValidation ValidateDataResponse(const CQuorumDataRequestKey& key, diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp index 4e6de89f11c6..1f4c6f39d0e8 100644 --- a/src/rpc/quorums.cpp +++ b/src/rpc/quorums.cpp @@ -938,7 +938,7 @@ static RPCHelpMan quorum_getdata() if (!quorum->m_quorum_base_block_index) return false; const llmq::CQuorumDataRequest request(llmqType, quorum->qc->quorumHash, nDataMask, proTxHash); const llmq::CQuorumDataRequestKey key(pNode->GetVerifiedProRegTxHash(), true, quorum->qc->quorumHash, llmqType); - if (!llmq_ctx.qman->RegisterDataRequest(key, request)) return false; + if (llmq_ctx.qman->RegisterDataRequest(key, request) != llmq::DataRequestRegistration::Accepted) return false; connman.PushMessage(pNode, CNetMsgMaker(pNode->GetCommonVersion()).Make(NetMsgType::QGETDATA, request)); return true; }); diff --git a/src/test/llmq_qgetdata_tests.cpp b/src/test/llmq_qgetdata_tests.cpp new file mode 100644 index 000000000000..ae52ad997fff --- /dev/null +++ b/src/test/llmq_qgetdata_tests.cpp @@ -0,0 +1,241 @@ +// 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; + +namespace { + +//! Minimal QuorumRole so NetQuorum accepts QGETDATA as if we were a masternode. +struct MockMasternodeRole final : public QuorumRole { + explicit MockMasternodeRole(CQuorumManager& qman) : QuorumRole(qman) {} + bool IsMasternode() const override { return true; } + bool IsWatching() const override { return false; } + bool SetQuorumSecretKeyShare(CQuorum& /*quorum*/, Span /*skContributions*/) const override + { + return false; + } +}; + +struct QGetDataSetup : public TestingSetup { + MockMasternodeRole m_role; + std::unique_ptr m_net_quorum; + + QGetDataSetup() : + TestingSetup{CBaseChainParams::REGTEST}, + m_role{*m_node.llmq_ctx->qman} + { + BOOST_REQUIRE(m_node.connman); + BOOST_REQUIRE(m_node.peerman); + BOOST_REQUIRE(m_node.dmnman); + BOOST_REQUIRE(m_node.llmq_ctx); + BOOST_REQUIRE(m_node.mn_sync); + BOOST_REQUIRE(m_node.sporkman); + BOOST_REQUIRE(m_node.chainman); + + // Mirror init.cpp: TestingSetup does not register NetQuorum, so install + // one with a masternode role so the QGETDATA gate opens for qwatch peers. + m_net_quorum = std::make_unique( + m_node.peerman.get(), *m_node.llmq_ctx->bls_worker, *m_node.connman, *m_node.dmnman, + *m_node.llmq_ctx->qman, *m_node.llmq_ctx->qsnapman, *m_node.chainman, *m_node.mn_sync, + *m_node.sporkman, &m_role, /*nodeman=*/nullptr, DEFAULT_WORKER_COUNT, QvvecSyncModeMap{}, + /*quorums_recovery=*/false); + } + + ~QGetDataSetup() + { + m_net_quorum.reset(); + } +}; + +std::unique_ptr MakePeer(NodeId id) +{ + in_addr peer_in_addr{}; + peer_in_addr.s_addr = htonl(0x0a000001 + static_cast(id)); + auto peer{std::make_unique(id, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_in_addr, 9999}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::INBOUND, + /*inbound_onion=*/false)}; + peer->nVersion = PROTOCOL_VERSION; + peer->SetCommonVersion(PROTOCOL_VERSION); + peer->fSuccessfullyConnected = true; + // Unauthenticated QWATCH path: any peer can set this flag and then send QGETDATA. + peer->qwatch = true; + return peer; +} + +void AssertMisbehaviorScore(PeerManager& peerman, const CNode& peer, int expected) +{ + CNodeStateStats stats; + BOOST_REQUIRE(peerman.GetNodeStateStats(peer.GetId(), stats)); + BOOST_CHECK_EQUAL(stats.m_misbehavior_score, expected); +} + +CDataStream MakeQGetDataStream(Consensus::LLMQType llmq_type, const uint256& quorum_hash, uint16_t data_mask, + const uint256& protx_hash) +{ + CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; + stream << static_cast(llmq_type); + stream << quorum_hash; + stream << data_mask; + stream << protx_hash; + return stream; +} + +} // namespace + +BOOST_FIXTURE_TEST_SUITE(llmq_qgetdata_tests, QGetDataSetup) + +// An unknown block hash can mean the requester is ahead of us or on another fork, so it +// is answered with QUORUM_BLOCK_NOT_FOUND without scoring — but it must never create a +// tracking-map entry, or distinct hashes would grow the map without bound. +BOOST_AUTO_TEST_CASE(qgetdata_unknown_block_not_scored_not_tracked) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + auto peer{MakePeer(/*id=*/2)}; + m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + AssertMisbehaviorScore(*m_node.peerman, *peer, 0); + + const uint256 protx_hash{uint256S("0x44")}; + for (const auto& hash_str : {"0x33", "0x55"}) { + const uint256 quorum_hash{uint256S(hash_str)}; + auto stream = MakeQGetDataStream(Consensus::LLMQType::LLMQ_TEST, quorum_hash, + CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR, protx_hash); + m_net_quorum->ProcessMessage(*peer, NetMsgType::QGETDATA, stream); + AssertMisbehaviorScore(*m_node.peerman, *peer, 0); + BOOST_CHECK(!m_node.llmq_ctx->qman->IsDataRequestPending(peer->GetVerifiedProRegTxHash(), + /*we_requested=*/false, quorum_hash, + Consensus::LLMQType::LLMQ_TEST)); + } +} + +// An active-chain block without a mined commitment is answered with QUORUM_NOT_FOUND and +// the first request is not scored: the commitment is mined after the quorum base block, +// so we can know the base block while still lagging behind the commitment the requester +// saw. Unlike the unknown-block miss, the key is bounded by real chain blocks, so the +// request is registered BEFORE the commitment lookup: the miss is an uncached EvoDB read, +// and repeats within the expiry window must be rate-limited and scored instead of +// re-running it for free. +BOOST_AUTO_TEST_CASE(qgetdata_active_non_quorum_unscored_once_then_rate_limited) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + auto peer{MakePeer(/*id=*/3)}; + m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + AssertMisbehaviorScore(*m_node.peerman, *peer, 0); + + const uint256 quorum_hash{ + WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Genesis()->GetBlockHash())}; + const uint256 protx_hash{uint256S("0x66")}; + + auto send_request = [&]() { + auto stream = MakeQGetDataStream(Consensus::LLMQType::LLMQ_TEST, quorum_hash, + CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR, protx_hash); + m_net_quorum->ProcessMessage(*peer, NetMsgType::QGETDATA, stream); + }; + + // First miss: registered, answered, unscored. + send_request(); + AssertMisbehaviorScore(*m_node.peerman, *peer, 0); + BOOST_CHECK(m_node.llmq_ctx->qman->IsDataRequestPending(peer->GetVerifiedProRegTxHash(), /*we_requested=*/false, + quorum_hash, Consensus::LLMQType::LLMQ_TEST)); + + // Repeats hit the registered entry and score like any other rate-limited request. + for (int score = 25; score <= 75; score += 25) { + send_request(); + AssertMisbehaviorScore(*m_node.peerman, *peer, score); + BOOST_CHECK(m_node.llmq_ctx->qman->IsDataRequestPending(peer->GetVerifiedProRegTxHash(), + /*we_requested=*/false, quorum_hash, + Consensus::LLMQType::LLMQ_TEST)); + } +} + +BOOST_AUTO_TEST_CASE(qgetdata_request_tracking_is_bounded) +{ + const uint256 requester{uint256S("0x77")}; + SetMockTime(100000); + + for (size_t i = 0; i < MAX_INBOUND_DATA_REQUESTS_PER_REQUESTER; ++i) { + const uint256 hash{ArithToUint256(arith_uint256{i + 1})}; + const CQuorumDataRequestKey key{requester, /*we_requested=*/false, hash, Consensus::LLMQType::LLMQ_TEST}; + const CQuorumDataRequest request{Consensus::LLMQType::LLMQ_TEST, hash, + CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR}; + BOOST_CHECK(m_node.llmq_ctx->qman->RegisterDataRequest(key, request, /*add_expiry_bias=*/false) == + DataRequestRegistration::Accepted); + } + + const uint256 over_hash{uint256S("0xdead")}; + const CQuorumDataRequestKey over_key{requester, /*we_requested=*/false, over_hash, Consensus::LLMQType::LLMQ_TEST}; + const CQuorumDataRequest over_request{Consensus::LLMQType::LLMQ_TEST, over_hash, + CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR}; + BOOST_CHECK(m_node.llmq_ctx->qman->RegisterDataRequest(over_key, over_request, /*add_expiry_bias=*/false) == + DataRequestRegistration::RequesterLimitExceeded); + BOOST_CHECK(!m_node.llmq_ctx->qman->IsDataRequestPending(requester, /*we_requested=*/false, over_hash, + Consensus::LLMQType::LLMQ_TEST)); + + // Timer-driven cleanup can recover capacity even when no block arrives. + SetMockTime(100000 + 361); + m_node.llmq_ctx->qman->CleanupExpiredDataRequests(); + BOOST_CHECK(m_node.llmq_ctx->qman->RegisterDataRequest(over_key, over_request, /*add_expiry_bias=*/false) == + DataRequestRegistration::Accepted); + SetMockTime(0); +} + +BOOST_AUTO_TEST_CASE(qgetdata_global_capacity_does_not_limit_outbound_requests) +{ + for (size_t i = 0; i < MAX_INBOUND_DATA_REQUESTS; ++i) { + const uint256 requester{ArithToUint256(arith_uint256{i / MAX_INBOUND_DATA_REQUESTS_PER_REQUESTER + 1})}; + const uint256 hash{ArithToUint256(arith_uint256{i + 1})}; + const CQuorumDataRequestKey key{requester, /*we_requested=*/false, hash, Consensus::LLMQType::LLMQ_TEST}; + const CQuorumDataRequest request{Consensus::LLMQType::LLMQ_TEST, hash, + CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR}; + BOOST_REQUIRE(m_node.llmq_ctx->qman->RegisterDataRequest(key, request, /*add_expiry_bias=*/false) == + DataRequestRegistration::Accepted); + } + + const uint256 extra_requester{uint256S("0xbeef")}; + const uint256 extra_hash{uint256S("0xfeed")}; + const CQuorumDataRequest extra_request{Consensus::LLMQType::LLMQ_TEST, extra_hash, + CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR}; + BOOST_CHECK(m_node.llmq_ctx->qman->RegisterDataRequest({extra_requester, /*we_requested=*/false, extra_hash, + Consensus::LLMQType::LLMQ_TEST}, + extra_request, /*add_expiry_bias=*/false) == + DataRequestRegistration::CapacityExhausted); + BOOST_CHECK(!m_node.llmq_ctx->qman->IsDataRequestPending(extra_requester, /*we_requested=*/false, extra_hash, + Consensus::LLMQType::LLMQ_TEST)); + BOOST_CHECK(m_node.llmq_ctx->qman->RegisterDataRequest({extra_requester, /*we_requested=*/true, extra_hash, + Consensus::LLMQType::LLMQ_TEST}, + extra_request, /*add_expiry_bias=*/false) == + DataRequestRegistration::Accepted); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/test/functional/p2p_quorum_data.py b/test/functional/p2p_quorum_data.py index baa65f98267a..6805bc42498b 100755 --- a/test/functional/p2p_quorum_data.py +++ b/test/functional/p2p_quorum_data.py @@ -120,14 +120,13 @@ def wait_for_banscore(node, peer_id, expected_score): def get_score(): for peer in node.getpeerinfo(): if peer["id"] == peer_id: - if (peer["banscore"] == expected_score): - # The score matches the one we expected. - # Wait a bit to make sure it won't change - # to avoid false positives. - time.sleep(0.1) return peer["banscore"] return None wait_until_helper(lambda: get_score() == expected_score, timeout=6) + # Re-read after a settle delay: the first read can race a pending bump, + # in particular when the expected score is the peer's current one (e.g. 0). + time.sleep(0.1) + assert_equal(get_score(), expected_score) def p2p_connection(node, uacomment=None): @@ -326,9 +325,18 @@ def send_bad_qdata_expect_disconnect(bad_qdata): qgetdata_invalid_block = msg_qgetdata(protx_hash_int, 100, 0x01, protx_hash_int) qgetdata_invalid_quorum = msg_qgetdata(int(mn2.get_node(self).getblockhash(0), 16), 100, 0x01, protx_hash_int) qgetdata_invalid_no_member = msg_qgetdata(quorum_hash_int, 100, 0x02, quorum_hash_int) + # Block and commitment misses can be plain chain skew, so they are not scored. + # The qdata reply is queued before any scoring would run, so receiving it does + # not prove scoring is done: sync with a ping to make the zero reads meaningful. p2p_mn2.test_qgetdata(qgetdata_invalid_block, QUORUM_BLOCK_NOT_FOUND) + p2p_mn2.sync_with_ping() + wait_for_banscore(mn2.get_node(self), id_p2p_mn2, 0) p2p_mn2.test_qgetdata(qgetdata_invalid_quorum, QUORUM_NOT_FOUND) + p2p_mn2.sync_with_ping() + wait_for_banscore(mn2.get_node(self), id_p2p_mn2, 0) p2p_mn2.test_qgetdata(qgetdata_invalid_no_member, MASTERNODE_IS_NO_MEMBER) + p2p_mn2.sync_with_ping() + wait_for_banscore(mn2.get_node(self), id_p2p_mn2, 0) # An unregistered LLMQ type is answered like the misses above, but unlike them it # cannot be explained by the peer being ahead of us, so it is scored in full too. # Kept last: the peer is dropped once it is.