Merge 25282b653b into 51d76634fb

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)

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:

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
+        // Do not add txids of witness transactions as those may have been malleated. See
-        // transactions to the filter, as they can have been malleated;
+        // https://github.com/bitcoin/bitcoin/issues/8279 for a discussion on this topic.
-        // 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.
         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

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

src/test/txdownload_tests.cpp

@@ -63,7 +63,6 @@ static std::map<TxValidationResult, Behaviors> 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}},

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())) {
+    return 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;
 }
 
 bool MemPoolAccept::ConsensusScriptChecks(const ATMPArgs& args, Workspace& ws)

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])
+        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
+        # The parent will not be requested because its txid was added to the reject filter above (as
-        # it's by txid and this is not a preferred relay peer.
+        # the wtxid of its witness-stripped version). But it can still be relayed again if it's not
-        self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY)
+        # stripped.
-        peer2.wait_for_getdata([int(parent_normal["tx"].rehash(), 16)])
-
-        # parent_normal can be relayed again even though parent1_witness_stripped was rejected
         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):