From e4f814a8495e10f6dd243006246007f2574168ea Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Mon, 8 Jun 2026 18:05:16 -0500 Subject: [PATCH 1/5] fix: punish invalid dstx messages --- src/net_processing.cpp | 15 +++-- test/functional/p2p_dstx.py | 65 ++++++++++++++++++++++ test/functional/test_framework/messages.py | 47 ++++++++++++++++ test/functional/test_runner.py | 1 + 4 files changed, 122 insertions(+), 6 deletions(-) create mode 100755 test/functional/p2p_dstx.py diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 015408b9306a..8bc7b2ab847a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4666,12 +4666,15 @@ void PeerManagerImpl::ProcessMessage( // Process custom logic, no matter if tx will be accepted to mempool later or not if (nInvType == MSG_DSTX) { - // Validate DSTX and return bRet if we need to return from here - uint256 hashTx = tx.GetHash(); - const auto& [bRet, bDoReturn] = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); - if (bDoReturn) { - return; - } + // Validate DSTX and return bRet if we need to return from here + uint256 hashTx = tx.GetHash(); + const auto& [bRet, bDoReturn] = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); + if (bDoReturn) { + if (!bRet) { + Misbehaving(pfrom.GetId(), 10, "invalid dstx"); + } + return; + } } LOCK(cs_main); diff --git a/test/functional/p2p_dstx.py b/test/functional/p2p_dstx.py new file mode 100755 index 000000000000..d0ac0f9b1d04 --- /dev/null +++ b/test/functional/p2p_dstx.py @@ -0,0 +1,65 @@ +#!/usr/bin/env python3 +# 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. +"""Test P2P CoinJoin broadcast transaction handling.""" + +import time + +from test_framework.messages import ( + CCoinJoinBroadcastTx, + COIN, + COutPoint, + CTransaction, + CTxIn, + CTxOut, + msg_dstx, +) +from test_framework.p2p import P2PInterface +from test_framework.script import ( + CScript, + OP_CHECKSIG, + OP_DUP, + OP_EQUALVERIFY, + OP_HASH160, +) +from test_framework.test_framework import BitcoinTestFramework + + +class P2PDSTXTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.setup_clean_chain = True + self.extra_args = [["-debug=net"]] + + def make_dstx(self): + tx = CTransaction() + tx.vin = [CTxIn(COutPoint(hash=i + 1, n=0)) for i in range(3)] + p2pkh = CScript([ + OP_DUP, + OP_HASH160, + b"\x01" * 20, + OP_EQUALVERIFY, + OP_CHECKSIG, + ]) + tx.vout = [CTxOut(nValue=COIN // 1000 + 1, scriptPubKey=p2pkh) for _ in tx.vin] + return CCoinJoinBroadcastTx( + tx=tx, + m_protxHash=1, + vchSig=b"\x01" * 96, + sigTime=int(time.time()), + ) + + def run_test(self): + self.log.info("Leave IBD so unsolicited DSTX is processed") + self.generate(self.nodes[0], 1) + + peer = self.nodes[0].add_p2p_connection(P2PInterface()) + + self.log.info("Unknown masternode DSTX is treated as peer misbehavior") + with self.nodes[0].assert_debug_log(["Misbehaving", "invalid dstx"]): + peer.send_and_ping(msg_dstx(self.make_dstx())) + + +if __name__ == '__main__': + P2PDSTXTest().main() diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index cbc792c9248e..f8527385763b 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -1835,6 +1835,53 @@ def __repr__(self): return "msg_tx(tx=%s)" % (repr(self.tx)) +class CCoinJoinBroadcastTx: + __slots__ = ("tx", "m_protxHash", "vchSig", "sigTime") + + def __init__(self, tx=None, m_protxHash=0, vchSig=b"", sigTime=0): + self.tx = tx or CTransaction() + self.m_protxHash = m_protxHash + self.vchSig = vchSig + self.sigTime = sigTime + + def deserialize(self, f): + self.tx = CTransaction() + self.tx.deserialize(f) + self.m_protxHash = deser_uint256(f) + self.vchSig = deser_string(f) + self.sigTime = struct.unpack(" Date: Tue, 9 Jun 2026 00:09:58 -0500 Subject: [PATCH 2/5] fix: avoid punishing unverifiable dstx relays --- src/net_processing.cpp | 2 +- test/functional/p2p_dstx.py | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8bc7b2ab847a..8cfb5f2e9836 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3583,7 +3583,7 @@ std::pair static ValidateDSTX(CDeterministicMN if (!dmn) { LogPrint(BCLog::COINJOIN, "DSTX -- Can't find masternode %s to verify %s\n", dstx.masternodeOutpoint.ToStringShort(), hashTx.ToString()); - return {false, true}; + return {true, true}; } if (!mn_metaman.IsValidForMixingTxes(dmn->proTxHash)) { diff --git a/test/functional/p2p_dstx.py b/test/functional/p2p_dstx.py index d0ac0f9b1d04..9f796c6a76fa 100755 --- a/test/functional/p2p_dstx.py +++ b/test/functional/p2p_dstx.py @@ -56,10 +56,16 @@ def run_test(self): peer = self.nodes[0].add_p2p_connection(P2PInterface()) - self.log.info("Unknown masternode DSTX is treated as peer misbehavior") - with self.nodes[0].assert_debug_log(["Misbehaving", "invalid dstx"]): + self.log.info("Unknown masternode DSTX is ignored without peer misbehavior") + with self.nodes[0].assert_debug_log([], unexpected_msgs=["Misbehaving", "invalid dstx"]): peer.send_and_ping(msg_dstx(self.make_dstx())) + self.log.info("Structurally invalid DSTX is treated as peer misbehavior") + invalid_dstx = self.make_dstx() + invalid_dstx.tx.vout.pop() + with self.nodes[0].assert_debug_log(["Misbehaving", "invalid dstx"]): + peer.send_and_ping(msg_dstx(invalid_dstx)) + if __name__ == '__main__': P2PDSTXTest().main() From a8346482ab7aecf1516a0a5b8cfae1928e72d8ef Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Mon, 15 Jun 2026 08:16:36 -0500 Subject: [PATCH 3/5] fix: penalize unknown-masternode dstx relays with a small score Previously, DSTX messages whose masternode we couldn't find in the deterministic MN list were silently dropped with no peer penalty. This let a malicious peer flood us with unverifiable DSTXes indefinitely. Apply a small misbehavior score (1) on that path so 100 such messages from one peer cross the discouragement threshold, while keeping the existing stronger penalty (10) for clearly malformed DSTXes and bad-signature DSTXes. The ValidateDSTX return value is changed from a bool to the misbehavior score itself; the call site now applies that score directly. Extend p2p_dstx.py to cover all three observable cases: small penalty for unknown-MN, stronger penalty for malformed structure, and the cumulative discouragement of a peer flooding unknown-MN DSTXes. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/net_processing.cpp | 28 ++++++++------- test/functional/p2p_dstx.py | 72 +++++++++++++++++++++++++++++-------- 2 files changed, 74 insertions(+), 26 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8cfb5f2e9836..5ca6e8e6fe9c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3539,18 +3539,20 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& m_connman.PushMessage(&node, std::move(msg)); } -std::pair static ValidateDSTX(CDeterministicMNManager& dmnman, CDSTXManager& dstxman, ChainstateManager& chainman, - CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool, CCoinJoinBroadcastTx& dstx, uint256 hashTx) +// Returned score is the misbehavior penalty to apply to the relaying peer; 0 means no penalty. +// do_return signals the caller to stop further processing of the DSTX. +std::pair static ValidateDSTX(CDeterministicMNManager& dmnman, CDSTXManager& dstxman, ChainstateManager& chainman, + CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool, CCoinJoinBroadcastTx& dstx, uint256 hashTx) { assert(mn_metaman.IsValid()); if (!dstx.IsValidStructure()) { LogPrint(BCLog::COINJOIN, "DSTX -- Invalid DSTX structure: %s\n", hashTx.ToString()); - return {false, true}; + return {10, true}; } if (dstxman.GetDSTX(hashTx)) { LogPrint(BCLog::COINJOIN, "DSTX -- Already have %s, skipping...\n", hashTx.ToString()); - return {true, true}; // not an error + return {0, true}; // not an error } const CBlockIndex* pindex{nullptr}; @@ -3583,26 +3585,29 @@ std::pair static ValidateDSTX(CDeterministicMN if (!dmn) { LogPrint(BCLog::COINJOIN, "DSTX -- Can't find masternode %s to verify %s\n", dstx.masternodeOutpoint.ToStringShort(), hashTx.ToString()); - return {true, true}; + // We can't verify the signature here, so apply only a small penalty. + // The MN may have been removed very recently, but a peer flooding us with + // unverifiable DSTX-es should still eventually be discouraged. + return {1, true}; } if (!mn_metaman.IsValidForMixingTxes(dmn->proTxHash)) { LogPrint(BCLog::COINJOIN, "DSTX -- Masternode %s is sending too many transactions %s\n", dstx.masternodeOutpoint.ToStringShort(), hashTx.ToString()); - return {true, true}; + return {0, true}; // TODO: Not an error? Could it be that someone is relaying old DSTXes // we have no idea about (e.g we were offline)? How to handle them? } if (!dstx.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) { LogPrint(BCLog::COINJOIN, "DSTX -- CheckSignature() failed for %s\n", hashTx.ToString()); - return {false, true}; + return {10, true}; } LogPrint(BCLog::COINJOIN, "DSTX -- Got Masternode transaction %s\n", hashTx.ToString()); mempool.PrioritiseTransaction(hashTx, 0.1*COIN); mn_metaman.DisallowMixing(dmn->proTxHash); - return {true, false}; + return {0, false}; } void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr& block, bool force_processing) @@ -4666,12 +4671,11 @@ void PeerManagerImpl::ProcessMessage( // Process custom logic, no matter if tx will be accepted to mempool later or not if (nInvType == MSG_DSTX) { - // Validate DSTX and return bRet if we need to return from here uint256 hashTx = tx.GetHash(); - const auto& [bRet, bDoReturn] = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); + const auto& [misbehavior_score, bDoReturn] = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); if (bDoReturn) { - if (!bRet) { - Misbehaving(pfrom.GetId(), 10, "invalid dstx"); + if (misbehavior_score > 0) { + Misbehaving(pfrom.GetId(), misbehavior_score, "invalid dstx"); } return; } diff --git a/test/functional/p2p_dstx.py b/test/functional/p2p_dstx.py index 9f796c6a76fa..ca56e6dbf0ac 100755 --- a/test/functional/p2p_dstx.py +++ b/test/functional/p2p_dstx.py @@ -2,7 +2,13 @@ # 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. -"""Test P2P CoinJoin broadcast transaction handling.""" +"""Test P2P CoinJoin broadcast transaction handling. + +Verifies that DSTX messages with an unverifiable (unknown) masternode incur +only a small misbehavior penalty, while clearly malformed DSTXes get the +existing stronger penalty. Also exercises the cumulative behavior so that +a peer flooding unknown-MN DSTXes is eventually discouraged. +""" import time @@ -25,16 +31,26 @@ ) from test_framework.test_framework import BitcoinTestFramework +# Default DISCOURAGEMENT_THRESHOLD in net_processing.h. +DISCOURAGEMENT_THRESHOLD = 100 +# Penalty applied when the relaying peer sends a DSTX whose masternode we +# can't find in the deterministic MN list (and therefore can't verify). +UNKNOWN_MN_SCORE = 1 +# Penalty applied when the DSTX itself is structurally bad / bad signature. +INVALID_DSTX_SCORE = 10 + class P2PDSTXTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 self.setup_clean_chain = True - self.extra_args = [["-debug=net"]] + self.extra_args = [["-debug=net", "-debug=coinjoin"]] - def make_dstx(self): + def make_dstx(self, nonce=0): tx = CTransaction() - tx.vin = [CTxIn(COutPoint(hash=i + 1, n=0)) for i in range(3)] + # The nonce flows into one of the prevouts so each DSTX has a distinct + # txid (and therefore is not deduped by dstxman.GetDSTX). + tx.vin = [CTxIn(COutPoint(hash=(nonce << 8) | (i + 1), n=0)) for i in range(3)] p2pkh = CScript([ OP_DUP, OP_HASH160, @@ -42,6 +58,8 @@ def make_dstx(self): OP_EQUALVERIFY, OP_CHECKSIG, ]) + # CoinJoin::IsDenominatedAmount requires a recognised denom; the + # smallest denom is 0.001 DASH + 0.0000001 fee == COIN//1000 + 1. tx.vout = [CTxOut(nValue=COIN // 1000 + 1, scriptPubKey=p2pkh) for _ in tx.vin] return CCoinJoinBroadcastTx( tx=tx, @@ -51,20 +69,46 @@ def make_dstx(self): ) def run_test(self): + node = self.nodes[0] self.log.info("Leave IBD so unsolicited DSTX is processed") - self.generate(self.nodes[0], 1) + self.generate(node, 1) - peer = self.nodes[0].add_p2p_connection(P2PInterface()) + self.log.info("Unknown-MN DSTX => small (+%d) misbehavior penalty", UNKNOWN_MN_SCORE) + peer = node.add_p2p_connection(P2PInterface()) + # Match the substring of the Misbehaving log that identifies the score + # jump of a fresh peer (0 -> 1) along with our message tag. + with node.assert_debug_log([ + "Can't find masternode", + "Misbehaving", + "(0 -> {})".format(UNKNOWN_MN_SCORE), + "invalid dstx", + ]): + peer.send_and_ping(msg_dstx(self.make_dstx(nonce=1))) - self.log.info("Unknown masternode DSTX is ignored without peer misbehavior") - with self.nodes[0].assert_debug_log([], unexpected_msgs=["Misbehaving", "invalid dstx"]): - peer.send_and_ping(msg_dstx(self.make_dstx())) + self.log.info("Structurally invalid DSTX => stronger (+%d) misbehavior penalty", INVALID_DSTX_SCORE) + peer_invalid = node.add_p2p_connection(P2PInterface()) + bad = self.make_dstx(nonce=2) + bad.tx.vout.pop() # vin.size() != vout.size() trips IsValidStructure + with node.assert_debug_log([ + "Invalid DSTX structure", + "Misbehaving", + "(0 -> {})".format(INVALID_DSTX_SCORE), + "invalid dstx", + ]): + peer_invalid.send_and_ping(msg_dstx(bad)) - self.log.info("Structurally invalid DSTX is treated as peer misbehavior") - invalid_dstx = self.make_dstx() - invalid_dstx.tx.vout.pop() - with self.nodes[0].assert_debug_log(["Misbehaving", "invalid dstx"]): - peer.send_and_ping(msg_dstx(invalid_dstx)) + self.log.info("A peer flooding unknown-MN DSTXes is eventually discouraged") + peer_flood = node.add_p2p_connection(P2PInterface()) + # +1 per unknown-MN DSTX, so DISCOURAGEMENT_THRESHOLD distinct DSTXes + # are enough to cross the threshold exactly once on this peer. + # send_message is fire-and-forget; the node disconnects once + # discouraged, so we wait on the threshold log line directly and then + # confirm the peer was dropped. + with node.assert_debug_log(["DISCOURAGE THRESHOLD EXCEEDED"], timeout=10): + for nonce in range(DISCOURAGEMENT_THRESHOLD): + # offset nonces to avoid txid clashes with earlier sends + peer_flood.send_message(msg_dstx(self.make_dstx(nonce=1000 + nonce))) + peer_flood.wait_for_disconnect() if __name__ == '__main__': From f32588aa7850dcfcb5be9396f6bb36c86f1d52c6 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Mon, 15 Jun 2026 08:38:04 -0500 Subject: [PATCH 4/5] refactor: use named result type and score enum for ValidateDSTX Replaces the positional std::pair return with a small DSTXValidationResult struct and a DSTXValidationScore enum (NONE=0, UNKNOWN_MASTERNODE=1, INVALID=10). Behavior is unchanged; the call site reads named fields and casts the enum for Misbehaving. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/net_processing.cpp | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5ca6e8e6fe9c..2115213f5eb6 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3539,20 +3539,31 @@ void PeerManagerImpl::ProcessGetCFCheckPt(CNode& node, Peer& peer, CDataStream& m_connman.PushMessage(&node, std::move(msg)); } -// Returned score is the misbehavior penalty to apply to the relaying peer; 0 means no penalty. +// Misbehavior penalty to apply to the relaying peer; NONE means no penalty. +enum class DSTXValidationScore : int { + NONE = 0, + UNKNOWN_MASTERNODE = 1, + INVALID = 10, +}; + // do_return signals the caller to stop further processing of the DSTX. -std::pair static ValidateDSTX(CDeterministicMNManager& dmnman, CDSTXManager& dstxman, ChainstateManager& chainman, - CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool, CCoinJoinBroadcastTx& dstx, uint256 hashTx) +struct DSTXValidationResult { + DSTXValidationScore score; + bool do_return; +}; + +static DSTXValidationResult ValidateDSTX(CDeterministicMNManager& dmnman, CDSTXManager& dstxman, ChainstateManager& chainman, + CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool, CCoinJoinBroadcastTx& dstx, uint256 hashTx) { assert(mn_metaman.IsValid()); if (!dstx.IsValidStructure()) { LogPrint(BCLog::COINJOIN, "DSTX -- Invalid DSTX structure: %s\n", hashTx.ToString()); - return {10, true}; + return {DSTXValidationScore::INVALID, true}; } if (dstxman.GetDSTX(hashTx)) { LogPrint(BCLog::COINJOIN, "DSTX -- Already have %s, skipping...\n", hashTx.ToString()); - return {0, true}; // not an error + return {DSTXValidationScore::NONE, true}; // not an error } const CBlockIndex* pindex{nullptr}; @@ -3588,26 +3599,26 @@ std::pair static ValidateDSTX(CDe // We can't verify the signature here, so apply only a small penalty. // The MN may have been removed very recently, but a peer flooding us with // unverifiable DSTX-es should still eventually be discouraged. - return {1, true}; + return {DSTXValidationScore::UNKNOWN_MASTERNODE, true}; } if (!mn_metaman.IsValidForMixingTxes(dmn->proTxHash)) { LogPrint(BCLog::COINJOIN, "DSTX -- Masternode %s is sending too many transactions %s\n", dstx.masternodeOutpoint.ToStringShort(), hashTx.ToString()); - return {0, true}; + return {DSTXValidationScore::NONE, true}; // TODO: Not an error? Could it be that someone is relaying old DSTXes // we have no idea about (e.g we were offline)? How to handle them? } if (!dstx.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) { LogPrint(BCLog::COINJOIN, "DSTX -- CheckSignature() failed for %s\n", hashTx.ToString()); - return {10, true}; + return {DSTXValidationScore::INVALID, true}; } LogPrint(BCLog::COINJOIN, "DSTX -- Got Masternode transaction %s\n", hashTx.ToString()); mempool.PrioritiseTransaction(hashTx, 0.1*COIN); mn_metaman.DisallowMixing(dmn->proTxHash); - return {0, false}; + return {DSTXValidationScore::NONE, false}; } void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr& block, bool force_processing) @@ -4672,10 +4683,10 @@ void PeerManagerImpl::ProcessMessage( // Process custom logic, no matter if tx will be accepted to mempool later or not if (nInvType == MSG_DSTX) { uint256 hashTx = tx.GetHash(); - const auto& [misbehavior_score, bDoReturn] = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); - if (bDoReturn) { - if (misbehavior_score > 0) { - Misbehaving(pfrom.GetId(), misbehavior_score, "invalid dstx"); + const auto result = ValidateDSTX(*m_dmnman, m_dstxman, m_chainman, m_mn_metaman, m_mempool, dstx, hashTx); + if (result.do_return) { + if (result.score != DSTXValidationScore::NONE) { + Misbehaving(pfrom.GetId(), static_cast(result.score), "invalid dstx"); } return; } From 7b8a80f9fe0d9f39df661838d67f9f14f61a24e8 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Mon, 15 Jun 2026 10:00:16 -0500 Subject: [PATCH 5/5] ci: retrigger after unrelated flaky failure