Merge #19620: Add txids with non-standard inputs to reject filter

9f88ded82b2898ca63d44c08072f1ba52f0e18d7 test addition of unknown segwit spends to txid reject filter (Gregory Sanders)
7989901c7eb62ca28b3d1e5d5831041a7267e495 Add txids with non-standard inputs to reject filter (Suhas Daftuar)

Pull request description:

  Our policy checks for non-standard inputs depend only on the non-witness
  portion of a transaction: we look up the scriptPubKey of the input being
  spent from our UTXO set (which is covered by the input txid), and the p2sh
  checks only rely on the scriptSig portion of the input.

  Consequently it's safe to add txids of transactions that fail these checks to
  the reject filter, as the witness is irrelevant to the failure. This is helpful
  for any situation where we might request the transaction again via txid (either
  from txid-relay peers, or if we might fetch the transaction via txid due to
  parent-fetching of orphans).

  Further, in preparation for future witness versions being deployed on the
  network, ensure that WITNESS_UNKNOWN transactions are rejected in
  AreInputsStandard(), so that transactions spending v1 (or greater) witness
  outputs will fall into this category of having their txid added to the reject
  filter.

ACKs for top commit:
  ajtowns:
    ACK 9f88ded82b2898ca63d44c08072f1ba52f0e18d7 - code review
  jnewbery:
    Code review ACK 9f88ded82b2898ca63d44c08072f1ba52f0e18d7
  ariard:
    Code Review/Tested ACK 9f88ded
  naumenkogs:
    utACK 9f88ded82b2898ca63d44c08072f1ba52f0e18d7
  jonatack:
    ACK 9f88ded82b2

Tree-SHA512: 1e93c0a5b68cb432524780ffc0093db893911fdfed9e2ed17f888e59114cc75d2a07062aefad4e5ce2e87c9270886117a8abb3c78fb889c9b9f31967f1777148

5 files changed, 55 insertions, 8 deletions

diff --git a/src/consensus/validation.h b/src/consensus/validation.h
index 8de7a8f2d8..2a93a090d6 100644
--- a/src/consensus/validation.h
+++ b/src/consensus/validation.h
@@ -26,7 +26,8 @@ enum class TxValidationResult {
      * is uninteresting.
      */
     TX_RECENT_CONSENSUS_CHANGE,
-    TX_NOT_STANDARD,          //!< didn't meet our local policy rules
+    TX_INPUTS_NOT_STANDARD,   //!< inputs (covered by txid) failed policy rules
+    TX_NOT_STANDARD,          //!< otherwise didn't meet our local policy rules
     TX_MISSING_INPUTS,        //!< transaction was missing some of its inputs
     TX_PREMATURE_SPEND,       //!< transaction spends a coinbase too early, or violates locktime/sequence locks
     /**
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c503a97be5..72c8e65c6b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -187,7 +187,7 @@ namespace {
      * million to make it highly unlikely for users to have issues with this
      * filter.
      *
-     * We only need to add wtxids to this filter. For non-segwit
+     * We typically only add wtxids to this filter. For non-segwit
      * transactions, the txid == wtxid, so this only prevents us from
      * re-downloading non-segwit transactions when communicating with
      * non-wtxidrelay peers -- which is important for avoiding malleation
@@ -196,6 +196,12 @@ namespace {
      * the reject filter store wtxids is exactly what we want to avoid
      * redownload of a rejected transaction.
      *
+     * In cases where we can tell that a segwit transaction will fail
+     * validation no matter the witness, we may add the txid of such
+     * transaction to the filter as well. This can be helpful when
+     * communicating with txid-relay peers or if we were to otherwise fetch a
+     * transaction via txid (eg in our orphan handling).
+     *
      * Memory used: 1.3 MB
      */
     std::unique_ptr<CRollingBloomFilter> recentRejects GUARDED_BY(cs_main);
@@ -1161,6 +1167,7 @@ static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state,
         }
     // Conflicting (but not necessarily invalid) data or different policy:
     case TxValidationResult::TX_RECENT_CONSENSUS_CHANGE:
+    case TxValidationResult::TX_INPUTS_NOT_STANDARD:
     case TxValidationResult::TX_NOT_STANDARD:
     case TxValidationResult::TX_MISSING_INPUTS:
     case TxValidationResult::TX_PREMATURE_SPEND:
@@ -2052,6 +2059,19 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set<uin
                 // if we start doing this too early.
                 assert(recentRejects);
                 recentRejects->insert(orphanTx.GetWitnessHash());
+                // If the transaction failed for TX_INPUTS_NOT_STANDARD,
+                // then we know that the witness was irrelevant to the policy
+                // failure, since this check depends only on the txid
+                // (the scriptPubKey being spent is covered by the txid).
+                // Add the txid to the reject filter to prevent repeated
+                // processing of this transaction in the event that child
+                // transactions are later received (resulting in
+                // parent-fetching by txid via the orphan-handling logic).
+                if (orphan_state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && orphanTx.GetWitnessHash() != orphanTx.GetHash()) {
+                    // We only add the txid if it differs from the wtxid, to
+                    // avoid wasting entries in the rolling bloom filter.
+                    recentRejects->insert(orphanTx.GetHash());
+                }
             }
             EraseOrphanTx(orphanHash);
             done = true;
@@ -2943,7 +2963,7 @@ void ProcessMessage(
 
         // We do the AlreadyHave() check using wtxid, rather than txid - in the
         // absence of witness malleation, this is strictly better, because the
-        // recent rejects filter may contain the wtxid but will never contain
+        // recent rejects filter may contain the wtxid but rarely contains
         // the txid of a segwit transaction that has been rejected.
         // In the presence of witness malleation, it's possible that by only
         // doing the check with wtxid, we could overlook a transaction which
@@ -3035,6 +3055,17 @@ void ProcessMessage(
                 // if we start doing this too early.
                 assert(recentRejects);
                 recentRejects->insert(tx.GetWitnessHash());
+                // If the transaction failed for TX_INPUTS_NOT_STANDARD,
+                // then we know that the witness was irrelevant to the policy
+                // failure, since this check depends only on the txid
+                // (the scriptPubKey being spent is covered by the txid).
+                // Add the txid to the reject filter to prevent repeated
+                // processing of this transaction in the event that child
+                // transactions are later received (resulting in
+                // parent-fetching by txid via the orphan-handling logic).
+                if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && tx.GetWitnessHash() != tx.GetHash()) {
+                    recentRejects->insert(tx.GetHash());
+                }
                 if (RecursiveDynamicUsage(*ptx) < 100000) {
                     AddToCompactExtraTransactions(ptx);
                 }
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
index c56abaf6c9..0e9820da1e 100644
--- a/src/policy/policy.cpp
+++ b/src/policy/policy.cpp
@@ -152,6 +152,8 @@ bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeR
  * script can be anything; an attacker could use a very
  * expensive-to-check-upon-redemption script like:
  *   DUP CHECKSIG DROP ... repeated 100 times... OP_1
+ *
+ * Note that only the non-witness portion of the transaction is checked here.
  */
 bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
 {
@@ -164,7 +166,11 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
 
         std::vector<std::vector<unsigned char> > vSolutions;
         TxoutType whichType = Solver(prev.scriptPubKey, vSolutions);
-        if (whichType == TxoutType::NONSTANDARD) {
+        if (whichType == TxoutType::NONSTANDARD || whichType == TxoutType::WITNESS_UNKNOWN) {
+            // WITNESS_UNKNOWN failures are typically also caught with a policy
+            // flag in the script interpreter, but it can be helpful to catch
+            // this type of NONSTANDARD transaction earlier in transaction
+            // validation.
             return false;
         } else if (whichType == TxoutType::SCRIPTHASH) {
             std::vector<std::vector<unsigned char> > stack;
diff --git a/src/validation.cpp b/src/validation.cpp
index 81af972777..84c106f064 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -688,8 +688,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     }
 
     // Check for non-standard pay-to-script-hash in inputs
-    if (fRequireStandard && !AreInputsStandard(tx, m_view))
-        return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-nonstandard-inputs");
+    if (fRequireStandard && !AreInputsStandard(tx, m_view)) {
+        return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
+    }
 
     // Check for non-standard witness in P2WSH
     if (tx.HasWitness() && fRequireStandard && !IsWitnessStandard(tx, m_view))
diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
index 728212ca23..2233fa5998 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -1418,7 +1418,7 @@ class SegWitTest(BitcoinTestFramework):
         temp_utxo.pop()  # last entry in temp_utxo was the output we just spent
         temp_utxo.append(UTXO(tx2.sha256, 0, tx2.vout[0].nValue))
 
-        # Spend everything in temp_utxo back to an OP_TRUE output.
+        # Spend everything in temp_utxo into an segwit v1 output.
         tx3 = CTransaction()
         total_value = 0
         for i in temp_utxo:
@@ -1426,8 +1426,16 @@ class SegWitTest(BitcoinTestFramework):
             tx3.wit.vtxinwit.append(CTxInWitness())
             total_value += i.nValue
         tx3.wit.vtxinwit[-1].scriptWitness.stack = [witness_program]
-        tx3.vout.append(CTxOut(total_value - 1000, CScript([OP_TRUE])))
+        tx3.vout.append(CTxOut(total_value - 1000, script_pubkey))
         tx3.rehash()
+
+        # First we test this transaction against fRequireStandard=true node
+        # making sure the txid is added to the reject filter
+        self.std_node.announce_tx_and_wait_for_getdata(tx3)
+        test_transaction_acceptance(self.nodes[1], self.std_node, tx3, with_witness=True, accepted=False, reason="bad-txns-nonstandard-inputs")
+        # Now the node will no longer ask for getdata of this transaction when advertised by same txid
+        self.std_node.announce_tx_and_wait_for_getdata(tx3, timeout=5, success=False)
+
         # Spending a higher version witness output is not allowed by policy,
         # even with fRequireStandard=false.
         test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=True, accepted=False, reason="reserved for soft-fork upgrades")