From 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 Mon Sep 17 00:00:00 2001
From: Suhas Daftuar <sdaftuar@gmail.com>
Date: Fri, 7 Feb 2020 04:30:41 -0500
Subject: Make TX_WITNESS_STRIPPED its own rejection reason

Previously, TX_WITNESS_MUTATED could be returned during transaction validation
for either transactions that had a witness that was non-standard, or for
transactions that had no witness but were invalid due to segwit validation
rules.

However, for txid/wtxid-relay considerations, net_processing distinguishes the
witness stripped case separately, because it affects whether a wtxid should be
able to be added to the reject filter. It is safe to add the wtxid of a
witness-mutated transaction to the filter (as that wtxid shouldn't collide with
the txid, and hence it wouldn't interfere with transaction relay from
txid-relay peers), but it is not safe to add the wtxid (== txid) of a
witness-stripped transaction to the filter, because that would interfere with
relay of another transaction with the same txid (but different wtxid) when
relaying from txid-relay peers.

Also updates the comment explaining this logic, and explaining that we can get
rid of this complexity once there's a sufficient deployment of wtxid-relaying
peers on the network.
---
 src/net_processing.cpp | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

(limited to 'src/net_processing.cpp')

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index bc09e9409..d2ac86ea4 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1160,6 +1160,7 @@ static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state,
     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:
         break;
@@ -2031,10 +2032,19 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set<uin
             // Has inputs but not accepted to mempool
             // Probably non-standard or insufficient fee
             LogPrint(BCLog::MEMPOOL, "   removed orphan tx %s\n", orphanHash.ToString());
-            if (orphanTx.HasWitness() || orphan_state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) {
-                // Do not use rejection cache for witness transactions or
-                // witness-stripped transactions, as they can have been malleated.
-                // See https://github.com/bitcoin/bitcoin/issues/8279 for details.
+            if (orphan_state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) {
+                // 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.
                 assert(recentRejects);
                 recentRejects->insert(orphanTx.GetWitnessHash());
             }
@@ -2992,10 +3002,19 @@ void ProcessMessage(
                 recentRejects->insert(tx.GetWitnessHash());
             }
         } else {
-            if (tx.HasWitness() || state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) {
-                // Do not use rejection cache for witness transactions or
-                // witness-stripped transactions, as they can have been malleated.
-                // See https://github.com/bitcoin/bitcoin/issues/8279 for details.
+            if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) {
+                // 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.
                 assert(recentRejects);
                 recentRejects->insert(tx.GetWitnessHash());
                 if (RecursiveDynamicUsage(*ptx) < 100000) {
-- 
cgit v1.2.3