From 65bcbbc538234957b1f7f76b2f21ad7c138efb87 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 23 Jul 2025 10:50:33 +1000 Subject: [PATCH] net_processing: drop MaybePunishNodeForTx Do not discourage nodes even when they send us consensus invalid transactions. Because we do not discourage nodes for transactions we consider non-standard, we don't get any DoS protection from this check in adversarial scenarios, so remove the check entirely both to simplify the code and reduce the risk of splitting the network due to changes in tx relay policy. NOTE: Backport required additional adjustment in test/functional/p2p_invalid_tx Github-Pull: #33050 Rebased-From: 266dd0e10d08c0bfde63205db15d6c210a021b90 --- src/net_processing.cpp | 34 ----------------------- test/functional/data/invalid_txs.py | 20 ++++++------- test/functional/p2p_invalid_tx.py | 5 ++-- test/functional/p2p_opportunistic_1p1c.py | 6 ++-- 4 files changed, 16 insertions(+), 49 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1da3ec9d211..b25819c821b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -553,12 +553,6 @@ private: bool via_compact_block, const std::string& message = "") EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - /** - * Potentially disconnect and discourage a node based on the contents of a TxValidationState object - */ - void MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - /** Maybe disconnect a peer and discourage future connections from its address. * * @param[in] pnode The node to check. @@ -1805,32 +1799,6 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati } } -void PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state) -{ - PeerRef peer{GetPeerRef(nodeid)}; - switch (state.GetResult()) { - case TxValidationResult::TX_RESULT_UNSET: - break; - // The node is providing invalid data: - case TxValidationResult::TX_CONSENSUS: - if (peer) Misbehaving(*peer, ""); - return; - // Conflicting (but not necessarily invalid) data or different policy: - case TxValidationResult::TX_INPUTS_NOT_STANDARD: - case TxValidationResult::TX_NOT_STANDARD: - case TxValidationResult::TX_MISSING_INPUTS: - case TxValidationResult::TX_PREMATURE_SPEND: - case TxValidationResult::TX_WITNESS_MUTATED: - case TxValidationResult::TX_WITNESS_STRIPPED: - case TxValidationResult::TX_CONFLICT: - case TxValidationResult::TX_MEMPOOL_POLICY: - case TxValidationResult::TX_NO_MEMPOOL: - case TxValidationResult::TX_RECONSIDERABLE: - case TxValidationResult::TX_UNKNOWN: - break; - } -} - bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex) { AssertLockHeld(cs_main); @@ -2987,8 +2955,6 @@ std::optional PeerManagerImpl::ProcessInvalidTx(NodeId if (peer) AddKnownTx(*peer, parent_txid); } - MaybePunishNodeForTx(nodeid, state); - return package_to_validate; } diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index d2d7202d860..48ec88fde0d 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -89,7 +89,7 @@ class BadTxTemplate: class OutputMissing(BadTxTemplate): reject_reason = "bad-txns-vout-empty" - expect_disconnect = True + expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -100,7 +100,7 @@ class OutputMissing(BadTxTemplate): class InputMissing(BadTxTemplate): reject_reason = "bad-txns-vin-empty" - expect_disconnect = True + expect_disconnect = False # We use a blank transaction here to make sure # it is interpreted as a non-witness transaction. @@ -149,7 +149,7 @@ class BadInputOutpointIndex(BadTxTemplate): class DuplicateInput(BadTxTemplate): reject_reason = 'bad-txns-inputs-duplicate' - expect_disconnect = True + expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -162,7 +162,7 @@ class DuplicateInput(BadTxTemplate): class PrevoutNullInput(BadTxTemplate): reject_reason = 'bad-txns-prevout-null' - expect_disconnect = True + expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -188,7 +188,7 @@ class NonexistentInput(BadTxTemplate): class SpendTooMuch(BadTxTemplate): reject_reason = 'bad-txns-in-belowout' - expect_disconnect = True + expect_disconnect = False def get_tx(self): return create_tx_with_script( @@ -197,7 +197,7 @@ class SpendTooMuch(BadTxTemplate): class CreateNegative(BadTxTemplate): reject_reason = 'bad-txns-vout-negative' - expect_disconnect = True + expect_disconnect = False def get_tx(self): return create_tx_with_script(self.spend_tx, 0, amount=-1) @@ -205,7 +205,7 @@ class CreateNegative(BadTxTemplate): class CreateTooLarge(BadTxTemplate): reject_reason = 'bad-txns-vout-toolarge' - expect_disconnect = True + expect_disconnect = False def get_tx(self): return create_tx_with_script(self.spend_tx, 0, amount=MAX_MONEY + 1) @@ -213,7 +213,7 @@ class CreateTooLarge(BadTxTemplate): class CreateSumTooLarge(BadTxTemplate): reject_reason = 'bad-txns-txouttotal-toolarge' - expect_disconnect = True + expect_disconnect = False def get_tx(self): tx = create_tx_with_script(self.spend_tx, 0, amount=MAX_MONEY) @@ -224,7 +224,7 @@ class CreateSumTooLarge(BadTxTemplate): class InvalidOPIFConstruction(BadTxTemplate): reject_reason = "mandatory-script-verify-flag-failed (Invalid OP_IF construction)" - expect_disconnect = True + expect_disconnect = False valid_in_block = True def get_tx(self): @@ -266,7 +266,7 @@ def getDisabledOpcodeTemplate(opcode): class NonStandardAndInvalid(BadTxTemplate): """A non-standard transaction which is also consensus-invalid should return the consensus error.""" reject_reason = "mandatory-script-verify-flag-failed (OP_RETURN was encountered)" - expect_disconnect = True + expect_disconnect = False valid_in_block = False def get_tx(self): diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py index ee8c6c16ca3..3785f725fef 100755 --- a/test/functional/p2p_invalid_tx.py +++ b/test/functional/p2p_invalid_tx.py @@ -73,7 +73,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): tx = template.get_tx() node.p2ps[0].send_txs_and_test( [tx], node, success=False, - expect_disconnect=template.expect_disconnect, + expect_disconnect=False, reject_reason=template.reject_reason, ) @@ -144,7 +144,6 @@ class InvalidTxRequestTest(BitcoinTestFramework): # tx_orphan_2_no_fee, because it has too low fee (p2ps[0] is not disconnected for relaying that tx) # tx_orphan_2_invalid, because it has negative fee (p2ps[1] is disconnected for relaying that tx) - self.wait_until(lambda: 1 == len(node.getpeerinfo()), timeout=12) # p2ps[1] is no longer connected assert_equal(expected_mempool, set(node.getrawmempool())) self.log.info('Test orphan pool overflow') @@ -165,7 +164,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): node.p2ps[0].send_txs_and_test([rejected_parent], node, success=False) self.log.info('Test that a peer disconnection causes erase its transactions from the orphan pool') - with node.assert_debug_log(['Erased 100 orphan transaction(s) from peer=26']): + with node.assert_debug_log(['Erased 100 orphan transaction(s) from peer=']): self.reconnect_p2p(num_connections=1) self.log.info('Test that a transaction in the orphan pool is included in a new tip block causes erase this transaction from the orphan pool') diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py index 5fdbf74a573..def70b733a6 100755 --- a/test/functional/p2p_opportunistic_1p1c.py +++ b/test/functional/p2p_opportunistic_1p1c.py @@ -251,8 +251,10 @@ class PackageRelayTest(BitcoinTestFramework): assert tx_orphan_bad_wit.rehash() not in node_mempool # 5. Have the other peer send the tx too, so that tx_orphan_bad_wit package is attempted. - bad_orphan_sender.send_message(msg_tx(low_fee_parent["tx"])) - bad_orphan_sender.wait_for_disconnect() + bad_orphan_sender.send_and_ping(msg_tx(low_fee_parent["tx"])) + + # The bad orphan sender should not be disconnected. + bad_orphan_sender.sync_with_ping() # The peer that didn't provide the orphan should not be disconnected. parent_sender.sync_with_ping()