From 07018a44f8072cec2de07735cb691241577e5f86 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 28 Apr 2025 15:37:51 -0400 Subject: [PATCH 1/3] tx download: add wtxid to reject filter even if witness was stripped We previously didn't for witness-stripped transactions due to compatibility concerns with non-wtxid-relay peers. The issue we were trying to prevent is attacker node A stripping the witness of a transaction broadcast by honest node B and submitting it stripped to a large part of the network. It would result in those nodes including the txid of the transaction (== witness-stripped wtxid) to their recent reject filter, which would significantly hinder the propagation of the honest non-stripped transaction if most connections on the network are txid based relay. Support for wtxid based relay was released with 0.21 in January 2021 and is now ubiquitous on the network. Therefore this concern does not apply anymore and we can get rid of this special case. --- src/node/txdownloadman_impl.cpp | 16 ++-------------- src/test/fuzz/txdownloadman.cpp | 1 - src/test/txdownload_tests.cpp | 1 - test/functional/p2p_orphan_handling.py | 12 ++++-------- 4 files changed, 6 insertions(+), 24 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 452c75da05d..61d889ff998 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -446,22 +446,10 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction m_txrequest.ForgetTxHash(tx.GetWitnessHash()); } } - } else if (state.GetResult() == TxValidationResult::TX_WITNESS_STRIPPED) { - add_extra_compact_tx = false; } else { // We can add the wtxid of this transaction to our reject filter. - // Do not add txids of witness transactions or witness-stripped - // transactions to the filter, as they can have been malleated; - // adding such txids to the reject filter would potentially - // interfere with relay of valid transactions from peers that - // do not support wtxid-based relay. See - // https://github.com/bitcoin/bitcoin/issues/8279 for details. - // We can remove this restriction (and always add wtxids to - // the filter even for witness stripped transactions) once - // wtxid-based relay is broadly deployed. - // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034 - // for concerns around weakening security of unupgraded nodes - // if we start doing this too early. + // Do not add txids of witness transactions as those may have been malleated. See + // https://github.com/bitcoin/bitcoin/issues/8279 for a discussion on this topic. if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) { // If the result is TX_RECONSIDERABLE, add it to m_lazy_recent_rejects_reconsiderable // because we should not download or submit this transaction by itself again, but may diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index 06385e7bddb..a6a8ba9adac 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -38,7 +38,6 @@ static TxValidationResult TESTED_TX_RESULTS[] = { TxValidationResult::TX_MISSING_INPUTS, TxValidationResult::TX_PREMATURE_SPEND, TxValidationResult::TX_WITNESS_MUTATED, - TxValidationResult::TX_WITNESS_STRIPPED, TxValidationResult::TX_CONFLICT, TxValidationResult::TX_MEMPOOL_POLICY, // Skip TX_NO_MEMPOOL diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp index 463eb210139..7eeb2442ceb 100644 --- a/src/test/txdownload_tests.cpp +++ b/src/test/txdownload_tests.cpp @@ -63,7 +63,6 @@ static std::map expected_behaviors{ {TxValidationResult::TX_MISSING_INPUTS, { 0, 0, 0, 0, 1, 0, 1}}, {TxValidationResult::TX_PREMATURE_SPEND, { 0, 1, 0, 0, 1, 0, 1}}, {TxValidationResult::TX_WITNESS_MUTATED, { 0, 1, 0, 0, 1, 0, 1}}, - {TxValidationResult::TX_WITNESS_STRIPPED, { 0, 0, 0, 0, 0, 0, 0}}, {TxValidationResult::TX_CONFLICT, { 0, 1, 0, 0, 1, 0, 1}}, {TxValidationResult::TX_MEMPOOL_POLICY, { 0, 1, 0, 0, 1, 0, 1}}, {TxValidationResult::TX_NO_MEMPOOL, { 0, 1, 0, 0, 1, 0, 1}}, diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index b40ea0e6a12..23b2b830d51 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -257,16 +257,12 @@ class OrphanHandlingTest(BitcoinTestFramework): # Relay the child. It should not be accepted because it has missing inputs. self.relay_transaction(peer2, child_invalid_witness["tx"]) assert child_invalid_witness["txid"] not in node.getrawmempool() - assert tx_in_orphanage(node, child_invalid_witness["tx"]) - # The parent should be requested since the unstripped wtxid would differ. Delayed because - # it's by txid and this is not a preferred relay peer. - self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) - peer2.wait_for_getdata([int(parent_normal["tx"].rehash(), 16)]) - - # parent_normal can be relayed again even though parent1_witness_stripped was rejected + # The parent will not be requested because its txid was added to the reject filter above (as + # the wtxid of its witness-stripped version). But it can still be relayed again if it's not + # stripped. self.relay_transaction(peer1, parent_normal["tx"]) - assert_equal(set(node.getrawmempool()), set([parent_normal["txid"], child_invalid_witness["txid"]])) + assert_equal(set(node.getrawmempool()), set([parent_normal["txid"]])) @cleanup def test_orphan_multiple_parents(self): From f5f074eacc7d9ee27f64ffe51fdcee53f6b7bb9a Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 28 Apr 2025 16:55:32 -0400 Subject: [PATCH 2/3] validation: avoid redundant scriptsig validation in PolicyScriptChecks This was performed to identify transactions whose witness was stripped and is not necessary anymore. --- src/validation.cpp | 16 +--------------- test/functional/p2p_orphan_handling.py | 13 ++++++++++--- 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 1213d8be9f9..e7c3c327fdd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1235,21 +1235,7 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) // Check input scripts and signatures. // This is done last to help prevent CPU exhaustion denial-of-service attacks. - if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache())) { - // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we - // need to turn both off, and compare against just turning off CLEANSTACK - // to see if the failure is specifically due to witness validation. - TxValidationState state_dummy; // Want reported failures to be from first CheckInputScripts - if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, ws.m_precomputed_txdata, GetValidationCache()) && - !CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, ws.m_precomputed_txdata, GetValidationCache())) { - // Only the witness is missing, so the transaction itself may be fine. - state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED, - state.GetRejectReason(), state.GetDebugMessage()); - } - return false; // state filled in by CheckInputScripts - } - - return true; + return CheckInputScripts(tx, state, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache()); } bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws) diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index 23b2b830d51..00d2d219a4e 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -130,13 +130,16 @@ class OrphanHandlingTest(BitcoinTestFramework): child = self.wallet.create_self_transfer(utxo_to_spend=parent['new_utxo']) return child["tx"].getwtxid(), child["tx"], parent["tx"] - def relay_transaction(self, peer, tx): + def relay_transaction(self, peer, tx, with_ping=True): """Relay transaction using MSG_WTX""" wtxid = int(tx.getwtxid(), 16) peer.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=wtxid)])) self.nodes[0].bumpmocktime(TXREQUEST_TIME_SKIP) peer.wait_for_getdata([wtxid]) - peer.send_and_ping(msg_tx(tx)) + if with_ping: + peer.send_and_ping(msg_tx(tx)) + else: + peer.send_without_ping(msg_tx(tx)) def create_malleated_version(self, tx): """ @@ -250,10 +253,14 @@ class OrphanHandlingTest(BitcoinTestFramework): child_invalid_witness = self.wallet.create_self_transfer(utxo_to_spend=parent_normal["new_utxo"]) # Relay the parent with witness stripped. It should not be accepted. - self.relay_transaction(peer1, parent1_witness_stripped) + self.relay_transaction(peer1, parent1_witness_stripped, with_ping=False) assert_equal(parent_normal["txid"], parent1_witness_stripped.rehash()) assert parent1_witness_stripped.rehash() not in node.getrawmempool() + # Sending the transaction without witness is a consensus failure and will cause the peer to disconnect. + peer1.wait_for_disconnect() + peer1 = node.add_p2p_connection(PeerTxRelayer()) + # Relay the child. It should not be accepted because it has missing inputs. self.relay_transaction(peer2, child_invalid_witness["tx"]) assert child_invalid_witness["txid"] not in node.getrawmempool() From 25282b653b8a5b84458a50a6792063512cf3fc9c Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 28 Apr 2025 17:04:35 -0400 Subject: [PATCH 3/3] Remove now-unused WITNESS_STRIPPED variant of TxValidationResult --- src/consensus/validation.h | 4 ---- src/net_processing.cpp | 1 - 2 files changed, 5 deletions(-) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index 2b2dd4bb686..5c1d419f6a2 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -33,10 +33,6 @@ enum class TxValidationResult { * non-standard witnesses). */ TX_WITNESS_MUTATED, - /** - * Transaction is missing a witness. - */ - TX_WITNESS_STRIPPED, /** * Tx already in mempool or conflicts with a tx in the chain * (if it conflicts with another tx in mempool, we use MEMPOOL_POLICY as it failed to reach the RBF threshold) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0b25396dd21..01e6408deff 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1850,7 +1850,6 @@ void PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat 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: