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: 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/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 b40ea0e6a12..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,23 +253,23 @@ 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() - 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):