Merge #20165: Only relay Taproot spends if next block has it active

3d0556d41087f945ed0a47a5d770076ad42ce432 Increase feature_taproot inactive test coverage (Pieter Wuille)
525cbd425e2f6a1dbd0febc53d7ada22cec4661f Only relay Taproot spends if next block has it active (Pieter Wuille)

Pull request description:

  There should be no change to mempool transaction behavior for witness v1 transactions as long as no activation is defined. Until that point, we should treat the consensus rules as under debate, and for soft-fork safety, that means spends should be treated as non-standard.

  It's possible to go further: don't relay them unless the consensus rules are actually active for the next block. This extends non-relay to the period where a deployment is defined, started, locked in, or failed. I see no downsides to this, and the code change is very simple.

ACKs for top commit:
  Sjors:
    utACK 3d0556d41087f945ed0a47a5d770076ad42ce432
  MarcoFalke:
    review ACK 3d0556d41087f945ed0a47a5d770076ad42ce432 🏓
  jnewbery:
    utACK 3d0556d41087f945ed0a47a5d770076ad42ce432

Tree-SHA512: ca625a2981716b4b44e8f3722718fd25fd04e25bf3ca1684924b8974fca49f7c1d438fdd9dcdfbc091a442002e20d441d42c41a0e2096e74a61068da6c60267a

9 files changed, 28 insertions, 16 deletions

diff --git a/src/bench/ccoins_caching.cpp b/src/bench/ccoins_caching.cpp
index 116de98b14..d5275b0b76 100644
--- a/src/bench/ccoins_caching.cpp
+++ b/src/bench/ccoins_caching.cpp
@@ -45,7 +45,7 @@ static void CCoinsCaching(benchmark::Bench& bench)
     // Benchmark.
     const CTransaction tx_1(t1);
     bench.run([&] {
-        bool success = AreInputsStandard(tx_1, coins);
+        bool success = AreInputsStandard(tx_1, coins, false);
         assert(success);
     });
     ECC_Stop();
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
index 69f2b456f1..91997aa883 100644
--- a/src/policy/policy.cpp
+++ b/src/policy/policy.cpp
@@ -155,7 +155,7 @@ bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeR
  *
  * Note that only the non-witness portion of the transaction is checked here.
  */
-bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
+bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, bool taproot_active)
 {
     if (tx.IsCoinBase())
         return true; // Coinbases don't use vin normally
@@ -183,6 +183,9 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
             if (subscript.GetSigOpCount(true) > MAX_P2SH_SIGOPS) {
                 return false;
             }
+        } else if (whichType == TxoutType::WITNESS_V1_TAPROOT) {
+            // Don't allow Taproot spends unless Taproot is active.
+            if (!taproot_active) return false;
         }
     }
 
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 51d67b9390..8090dff4c6 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -95,10 +95,11 @@ bool IsStandard(const CScript& scriptPubKey, TxoutType& whichType);
 bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason);
     /**
      * Check for standard transaction types
-     * @param[in] mapInputs    Map of previous transactions that have outputs we're spending
+     * @param[in] mapInputs       Map of previous transactions that have outputs we're spending
+     * @param[in] taproot_active  Whether or taproot consensus rules are active (used to decide whether spends of them are permitted)
      * @return True if all inputs (scriptSigs) use only standard transaction forms
      */
-bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);
+bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, bool taproot_active);
     /**
      * Check if the transaction is over standard P2WSH resources limit:
      * 3600bytes witnessScript size, 80bytes per witness stack element, 100 witness stack elements
diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp
index c186bef7ae..ac034809b0 100644
--- a/src/test/fuzz/coins_view.cpp
+++ b/src/test/fuzz/coins_view.cpp
@@ -229,7 +229,8 @@ void test_one_input(const std::vector<uint8_t>& buffer)
             break;
         }
         case 1: {
-            (void)AreInputsStandard(CTransaction{random_mutable_transaction}, coins_view_cache);
+            (void)AreInputsStandard(CTransaction{random_mutable_transaction}, coins_view_cache, false);
+            (void)AreInputsStandard(CTransaction{random_mutable_transaction}, coins_view_cache, true);
             break;
         }
         case 2: {
diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp
index d6deb7fc3d..4f972dea1c 100644
--- a/src/test/fuzz/transaction.cpp
+++ b/src/test/fuzz/transaction.cpp
@@ -95,7 +95,8 @@ void test_one_input(const std::vector<uint8_t>& buffer)
 
     CCoinsView coins_view;
     const CCoinsViewCache coins_view_cache(&coins_view);
-    (void)AreInputsStandard(tx, coins_view_cache);
+    (void)AreInputsStandard(tx, coins_view_cache, false);
+    (void)AreInputsStandard(tx, coins_view_cache, true);
     (void)IsWitnessStandard(tx, coins_view_cache);
 
     UniValue u(UniValue::VOBJ);
diff --git a/src/test/script_p2sh_tests.cpp b/src/test/script_p2sh_tests.cpp
index f6824a4e5e..856ec6346d 100644
--- a/src/test/script_p2sh_tests.cpp
+++ b/src/test/script_p2sh_tests.cpp
@@ -343,7 +343,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
     txTo.vin[3].scriptSig << OP_11 << OP_11 << std::vector<unsigned char>(oneAndTwo.begin(), oneAndTwo.end());
     txTo.vin[4].scriptSig << std::vector<unsigned char>(fifteenSigops.begin(), fifteenSigops.end());
 
-    BOOST_CHECK(::AreInputsStandard(CTransaction(txTo), coins));
+    BOOST_CHECK(::AreInputsStandard(CTransaction(txTo), coins, false));
     // 22 P2SH sigops for all inputs (1 for vin[0], 6 for vin[3], 15 for vin[4]
     BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txTo), coins), 22U);
 
@@ -356,7 +356,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
     txToNonStd1.vin[0].prevout.hash = txFrom.GetHash();
     txToNonStd1.vin[0].scriptSig << std::vector<unsigned char>(sixteenSigops.begin(), sixteenSigops.end());
 
-    BOOST_CHECK(!::AreInputsStandard(CTransaction(txToNonStd1), coins));
+    BOOST_CHECK(!::AreInputsStandard(CTransaction(txToNonStd1), coins, false));
     BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd1), coins), 16U);
 
     CMutableTransaction txToNonStd2;
@@ -368,7 +368,7 @@ BOOST_AUTO_TEST_CASE(AreInputsStandard)
     txToNonStd2.vin[0].prevout.hash = txFrom.GetHash();
     txToNonStd2.vin[0].scriptSig << std::vector<unsigned char>(twentySigops.begin(), twentySigops.end());
 
-    BOOST_CHECK(!::AreInputsStandard(CTransaction(txToNonStd2), coins));
+    BOOST_CHECK(!::AreInputsStandard(CTransaction(txToNonStd2), coins, false));
     BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 20U);
 }
 
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
index b7ee280336..1f520074b1 100644
--- a/src/test/transaction_tests.cpp
+++ b/src/test/transaction_tests.cpp
@@ -305,7 +305,7 @@ BOOST_AUTO_TEST_CASE(test_Get)
     t1.vout[0].nValue = 90*CENT;
     t1.vout[0].scriptPubKey << OP_1;
 
-    BOOST_CHECK(AreInputsStandard(CTransaction(t1), coins));
+    BOOST_CHECK(AreInputsStandard(CTransaction(t1), coins, false));
 }
 
 static void CreateCreditAndSpend(const FillableSigningProvider& keystore, const CScript& outscript, CTransactionRef& output, CMutableTransaction& input, bool success = true)
diff --git a/src/validation.cpp b/src/validation.cpp
index 423b93479a..8241cb159f 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -690,7 +690,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     }
 
     // Check for non-standard pay-to-script-hash in inputs
-    if (fRequireStandard && !AreInputsStandard(tx, m_view)) {
+    const auto& params = args.m_chainparams.GetConsensus();
+    auto taproot_state = VersionBitsState(::ChainActive().Tip(), params, Consensus::DEPLOYMENT_TAPROOT, versionbitscache);
+    if (fRequireStandard && !AreInputsStandard(tx, m_view, taproot_state == ThresholdState::ACTIVE)) {
         return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
     }
 
diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py
index 7b534c1c2f..3e47e24a3b 100755
--- a/test/functional/feature_taproot.py
+++ b/test/functional/feature_taproot.py
@@ -1129,13 +1129,13 @@ def spenders_taproot_inactive():
     ]
     tap = taproot_construct(pub, scripts)
 
-    # Test that keypath spending is valid & standard if compliant, but valid and nonstandard otherwise.
-    add_spender(spenders, "inactive/keypath_valid", key=sec, tap=tap)
+    # Test that keypath spending is valid & non-standard, regardless of validity.
+    add_spender(spenders, "inactive/keypath_valid", key=sec, tap=tap, standard=False)
     add_spender(spenders, "inactive/keypath_invalidsig", key=sec, tap=tap, standard=False, sighash=bitflipper(default_sighash))
     add_spender(spenders, "inactive/keypath_empty", key=sec, tap=tap, standard=False, witness=[])
 
-    # Same for scriptpath spending (but using future features like annex, leaf versions, or OP_SUCCESS is nonstandard).
-    add_spender(spenders, "inactive/scriptpath_valid", key=sec, tap=tap, leaf="pk", inputs=[getter("sign")])
+    # Same for scriptpath spending (and features like annex, leaf versions, or OP_SUCCESS don't change this)
+    add_spender(spenders, "inactive/scriptpath_valid", key=sec, tap=tap, leaf="pk", standard=False, inputs=[getter("sign")])
     add_spender(spenders, "inactive/scriptpath_invalidsig", key=sec, tap=tap, leaf="pk", standard=False, inputs=[getter("sign")], sighash=bitflipper(default_sighash))
     add_spender(spenders, "inactive/scriptpath_invalidcb", key=sec, tap=tap, leaf="pk", standard=False, inputs=[getter("sign")], controlblock=bitflipper(default_controlblock))
     add_spender(spenders, "inactive/scriptpath_valid_unkleaf", key=sec, tap=tap, leaf="future_leaf", standard=False, inputs=[getter("sign")])
@@ -1451,7 +1451,11 @@ class TaprootTest(BitcoinTestFramework):
 
         # Pre-taproot activation tests.
         self.log.info("Pre-activation tests...")
-        self.test_spenders(self.nodes[0], spenders_taproot_inactive(), input_counts=[1, 2, 2, 2, 2, 3])
+        # Run each test twice; once in isolation, and once combined with others. Testing in isolation
+        # means that the standardness is verified in every test (as combined transactions are only standard
+        # when all their inputs are standard).
+        self.test_spenders(self.nodes[0], spenders_taproot_inactive(), input_counts=[1])
+        self.test_spenders(self.nodes[0], spenders_taproot_inactive(), input_counts=[2, 3])
 
 
 if __name__ == '__main__':