Merge bitcoin/bitcoin#23512: policy: Treat taproot as always active

MarcoFalke · MarcoFalke · commit 064c729a964a · 2021-11-25T08:16:19.000+01:00
fa3e0da policy: Treat taproot as always active (MarcoFalke) Pull request description: Now that taproot is active, it can be treated as if it was always active for policy for the next major release. This simplifies the code and changes two things: * Importing `tr` descriptors can be done before the chain is fully synced. This is fine, because the wallet will already generate `tr` descriptors by default (regardless of the taproot status) after commit 47fe744. * Valid taproot spends won't be rejected from the mempool before taproot is active. This is strictly speaking a bugfix after commit 47fe744, since the wallet may generate taproot spends before the chain is fully synced. For example, a slow node or a purposefully offline node. Currently, the wallet needs the mempool to account for change. See bitcoin/bitcoin#11887. A similar change was done for segwit v0 in bitcoin/bitcoin#13120 . This effectively reverts commit c5ec036. ACKs for top commit: mjdietzx: Code Review ACK fa3e0da achow101: ACK fa3e0da sipa: utACK fa3e0da gruve-p: ACK bitcoin/bitcoin@fa3e0da gunar: Code Review + tACK fa3e0da rajarshimaitra: code review + tACK bitcoin/bitcoin@fa3e0da Tree-SHA512: c6dc7a4e6c345bdec33f256847dc63906ab1696aa683ab9b32a79e715613950884ac3a1a7a44e95f31bb28e58dd64679a616175f7e152b21f5550f3337c8e622
diff --git 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, false);
+        bool success{AreInputsStandard(tx_1, coins)};
         assert(success);
     });
     ECC_Stop();
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
@@ -287,9 +287,6 @@ class Chain
     //! to be prepared to handle this by ignoring notifications about unknown
     //! removed transactions and already added new transactions.
     virtual void requestMempoolTransactions(Notifications& notifications) = 0;
-
-    //! Check if Taproot has activated
-    virtual bool isTaprootActive() = 0;
 };
 
 //! Interface to let node manage chain clients (wallets, or maybe tools for
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
@@ -724,12 +724,6 @@ class ChainImpl : public Chain
             notifications.transactionAddedToMempool(entry.GetSharedTx(), 0 /* mempool_sequence */);
         }
     }
-    bool isTaprootActive() override
-    {
-        LOCK(::cs_main);
-        const CBlockIndex* tip = Assert(m_node.chainman)->ActiveChain().Tip();
-        return DeploymentActiveAfter(tip, Params().GetConsensus(), Consensus::DEPLOYMENT_TAPROOT);
-    }
     NodeContext& m_node;
 };
 } // namespace
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
@@ -161,13 +161,13 @@ 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 taproot_active)
+bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
 {
-    if (tx.IsCoinBase())
+    if (tx.IsCoinBase()) {
         return true; // Coinbases don't use vin normally
+    }
 
-    for (unsigned int i = 0; i < tx.vin.size(); i++)
-    {
+    for (unsigned int i = 0; i < tx.vin.size(); i++) {
         const CTxOut& prev = mapInputs.AccessCoin(tx.vin[i].prevout).out;
 
         std::vector<std::vector<unsigned char> > vSolutions;
@@ -189,9 +189,6 @@ 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
@@ -105,10 +105,9 @@ bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeR
 /**
 * Check for standard transaction types
 * @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 taproot_active);
+bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);
 /**
 * 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
@@ -221,8 +221,7 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
                 assert(expected_code_path);
             },
             [&] {
-                (void)AreInputsStandard(CTransaction{random_mutable_transaction}, coins_view_cache, false);
-                (void)AreInputsStandard(CTransaction{random_mutable_transaction}, coins_view_cache, true);
+                (void)AreInputsStandard(CTransaction{random_mutable_transaction}, coins_view_cache);
             },
             [&] {
                 TxValidationState state;
diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp
@@ -98,8 +98,7 @@ FUZZ_TARGET_INIT(transaction, initialize_transaction)
 
     CCoinsView coins_view;
     const CCoinsViewCache coins_view_cache(&coins_view);
-    (void)AreInputsStandard(tx, coins_view_cache, false);
-    (void)AreInputsStandard(tx, coins_view_cache, true);
+    (void)AreInputsStandard(tx, coins_view_cache);
     (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
@@ -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, false));
+    BOOST_CHECK(::AreInputsStandard(CTransaction(txTo), coins));
     // 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, false));
+    BOOST_CHECK(!::AreInputsStandard(CTransaction(txToNonStd1), coins));
     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, false));
+    BOOST_CHECK(!::AreInputsStandard(CTransaction(txToNonStd2), coins));
     BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 20U);
 }
 
diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp
@@ -406,7 +406,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, false));
+    BOOST_CHECK(AreInputsStandard(CTransaction(t1), coins));
 }
 
 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
@@ -723,8 +723,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     }
 
     // Check for non-standard pay-to-script-hash in inputs
-    const bool taproot_active = DeploymentActiveAfter(m_active_chainstate.m_chain.Tip(), args.m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_TAPROOT);
-    if (fRequireStandard && !AreInputsStandard(tx, m_view, taproot_active)) {
+    if (fRequireStandard && !AreInputsStandard(tx, m_view)) {
         return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
     }
 
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
@@ -1548,18 +1548,6 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
             }
         }
 
-        // Taproot descriptors cannot be imported if Taproot is not yet active.
-        // Check if this is a Taproot descriptor
-        CTxDestination dest;
-        ExtractDestination(scripts[0], dest);
-        if (std::holds_alternative<WitnessV1Taproot>(dest)) {
-            // Check if Taproot is active
-            if (!wallet.chain().isTaprootActive()) {
-                // Taproot is not active, raise an error
-                throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import tr() descriptor when Taproot is not active");
-            }
-        }
-
         // If private keys are enabled, check some things.
         if (!wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
            if (keys.keys.empty()) {
diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py
@@ -98,6 +98,7 @@
     program_to_witness
 )
 from collections import OrderedDict, namedtuple
+from enum import Enum
 from io import BytesIO
 import json
 import hashlib
@@ -456,7 +457,7 @@ def to_script(elem):
 # Each spender is a tuple of:
 # - A scriptPubKey which is to be spent from (CScript)
 # - A comment describing the test (string)
-# - Whether the spending (on itself) is expected to be standard (bool)
+# - Whether the spending (on itself) is expected to be standard (Enum.Standard)
 # - A tx-signing lambda returning (scriptsig, witness_stack), taking as inputs:
 #   - A transaction to sign (CTransaction)
 #   - An input position (int)
@@ -468,8 +469,14 @@ def to_script(elem):
 # - Whether this test demands being placed in a txin with no corresponding txout (for testing SIGHASH_SINGLE behavior)
 
 Spender = namedtuple("Spender", "script,comment,is_standard,sat_function,err_msg,sigops_weight,no_fail,need_vin_vout_mismatch")
+# The full node versions that treat the tx standard.
+# ALL means any version
+# V23 means the major version 23.0 and any later version
+# NONE means no version
+Standard = Enum('Standard', 'ALL V23 NONE')
 
-def make_spender(comment, *, tap=None, witv0=False, script=None, pkh=None, p2sh=False, spk_mutate_pre_p2sh=None, failure=None, standard=True, err_msg=None, sigops_weight=0, need_vin_vout_mismatch=False, **kwargs):
+
+def make_spender(comment, *, tap=None, witv0=False, script=None, pkh=None, p2sh=False, spk_mutate_pre_p2sh=None, failure=None, standard=Standard.ALL, err_msg=None, sigops_weight=0, need_vin_vout_mismatch=False, **kwargs):
     """Helper for constructing Spender objects using the context signing framework.
 
     * tap: a TaprootInfo object (see taproot_construct), for Taproot spends (cannot be combined with pkh, witv0, or script)
@@ -479,13 +486,18 @@ def make_spender(comment, *, tap=None, witv0=False, script=None, pkh=None, p2sh=
     * p2sh: whether the output is P2SH wrapper (this is supported even for Taproot, where it makes the output unencumbered)
     * spk_mutate_pre_psh: a callable to be applied to the script (before potentially P2SH-wrapping it)
     * failure: a dict of entries to override in the context when intentionally failing to spend (if None, no_fail will be set)
-    * standard: whether the (valid version of) spending is expected to be standard
+    * standard: whether the (valid version of) spending is expected to be standard (True is mapped to Standard.ALL, False is mapped to Standard.NONE)
     * err_msg: a string with an expected error message for failure (or None, if not cared about)
     * sigops_weight: the pre-taproot sigops weight consumed by a successful spend
     * need_vin_vout_mismatch: whether this test requires being tested in a transaction input that has no corresponding
                               transaction output.
     """
 
+    if standard == True:
+        standard = Standard.ALL
+    elif standard == False:
+        standard = Standard.NONE
+
     conf = dict()
 
     # Compute scriptPubKey and set useful defaults based on the inputs.
@@ -1170,12 +1182,12 @@ def spenders_taproot_inactive():
     tap = taproot_construct(pub, scripts)
 
     # 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_valid", key=sec, tap=tap, standard=Standard.V23)
     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 (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_valid", key=sec, tap=tap, leaf="pk", standard=Standard.V23, 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")])
@@ -1205,7 +1217,7 @@ def dump_json_test(tx, input_utxos, idx, success, failure):
 
     # The "final" field indicates that a spend should be always valid, even with more validation flags enabled
     # than the listed ones. Use standardness as a proxy for this (which gives a conservative underestimate).
-    if spender.is_standard:
+    if spender.is_standard == Standard.ALL:
         fields.append(("final", True))
 
     def dump_witness(wit):
@@ -1470,8 +1482,13 @@ def test_spenders(self, node, spenders, input_counts):
                 for i in range(len(input_utxos)):
                     tx.vin[i].scriptSig = input_data[i][i != fail_input][0]
                     tx.wit.vtxinwit[i].scriptWitness.stack = input_data[i][i != fail_input][1]
+                taproot_spend_policy = Standard.V23 if node.version is None else Standard.ALL
                 # Submit to mempool to check standardness
-                is_standard_tx = fail_input is None and all(utxo.spender.is_standard for utxo in input_utxos) and tx.nVersion >= 1 and tx.nVersion <= 2
+                is_standard_tx = (
+                    fail_input is None  # Must be valid to be standard
+                    and (all(utxo.spender.is_standard == Standard.ALL or utxo.spender.is_standard == taproot_spend_policy for utxo in input_utxos))  # All inputs must be standard
+                    and tx.nVersion >= 1  # The tx version must be standard
+                    and tx.nVersion <= 2)
                 tx.rehash()
                 msg = ','.join(utxo.spender.comment + ("*" if n == fail_input else "") for n, utxo in enumerate(input_utxos))
                 if is_standard_tx:
diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py
@@ -242,20 +242,15 @@ def do_test_addr(self, comment, pattern, privmap, treefn, keys):
             assert_equal(len(rederive), 1)
             assert_equal(rederive[0], addr_g)
 
-        # tr descriptors cannot be imported when Taproot is not active
+        # tr descriptors can be imported regardless of Taproot status
         result = self.privs_tr_enabled.importdescriptors([{"desc": desc, "timestamp": "now"}])
         assert(result[0]["success"])
         result = self.pubs_tr_enabled.importdescriptors([{"desc": desc_pub, "timestamp": "now"}])
         assert(result[0]["success"])
-        if desc.startswith("tr"):
-            result = self.privs_tr_disabled.importdescriptors([{"desc": desc, "timestamp": "now"}])
-            assert(not result[0]["success"])
-            assert_equal(result[0]["error"]["code"], -4)
-            assert_equal(result[0]["error"]["message"], "Cannot import tr() descriptor when Taproot is not active")
-            result = self.pubs_tr_disabled.importdescriptors([{"desc": desc_pub, "timestamp": "now"}])
-            assert(not result[0]["success"])
-            assert_equal(result[0]["error"]["code"], -4)
-            assert_equal(result[0]["error"]["message"], "Cannot import tr() descriptor when Taproot is not active")
+        result = self.privs_tr_disabled.importdescriptors([{"desc": desc, "timestamp": "now"}])
+        assert result[0]["success"]
+        result = self.pubs_tr_disabled.importdescriptors([{"desc": desc_pub, "timestamp": "now"}])
+        assert result[0]["success"]
 
     def do_test_sendtoaddress(self, comment, pattern, privmap, treefn, keys_pay, keys_change):
         self.log.info("Testing %s through sendtoaddress" % comment)

Original file line number	Diff line number	Diff line change
`@@ -724,12 +724,6 @@ class ChainImpl : public Chain`
`724`	`724`	`notifications.transactionAddedToMempool(entry.GetSharedTx(), 0 /* mempool_sequence */);`
`725`	`725`	`}`
`726`	`726`	`}`
`727`		`- bool isTaprootActive() override`
`728`		`- {`
`729`		`- LOCK(::cs_main);`
`730`		`- const CBlockIndex* tip = Assert(m_node.chainman)->ActiveChain().Tip();`
`731`		`- return DeploymentActiveAfter(tip, Params().GetConsensus(), Consensus::DEPLOYMENT_TAPROOT);`
`732`		`- }`
`733`	`727`	`NodeContext& m_node;`
`734`	`728`	`};`
`735`	`729`	`} // namespace`
Original file line number	Diff line number	Diff line change
`@@ -161,13 +161,13 @@ bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeR`
`161`	`161`	`*`
`162`	`162`	`* Note that only the non-witness portion of the transaction is checked here.`
`163`	`163`	`*/`
`164`		`-bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, bool taproot_active)`
	`164`	`+bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)`
`165`	`165`	`{`
`166`		`- if (tx.IsCoinBase())`
	`166`	`+ if (tx.IsCoinBase()) {`
`167`	`167`	`return true; // Coinbases don't use vin normally`
	`168`	`+ }`
`168`	`169`
`169`		`- for (unsigned int i = 0; i < tx.vin.size(); i++)`
`170`		`- {`
	`170`	`+ for (unsigned int i = 0; i < tx.vin.size(); i++) {`
`171`	`171`	`const CTxOut& prev = mapInputs.AccessCoin(tx.vin[i].prevout).out;`
`172`	`172`
`173`	`173`	`std::vector<std::vector<unsigned char> > vSolutions;`
`@@ -189,9 +189,6 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs,`
`189`	`189`	`if (subscript.GetSigOpCount(true) > MAX_P2SH_SIGOPS) {`
`190`	`190`	`return false;`
`191`	`191`	`}`
`192`		`- } else if (whichType == TxoutType::WITNESS_V1_TAPROOT) {`
`193`		`- // Don't allow Taproot spends unless Taproot is active.`
`194`		`- if (!taproot_active) return false;`
`195`	`192`	`}`
`196`	`193`	`}`
`197`	`194`
Original file line number	Diff line number	Diff line change
`@@ -406,7 +406,7 @@ BOOST_AUTO_TEST_CASE(test_Get)`
`406`	`406`	`t1.vout[0].nValue = 90*CENT;`
`407`	`407`	`t1.vout[0].scriptPubKey << OP_1;`
`408`	`408`
`409`		`- BOOST_CHECK(AreInputsStandard(CTransaction(t1), coins, false));`
	`409`	`+ BOOST_CHECK(AreInputsStandard(CTransaction(t1), coins));`
`410`	`410`	`}`
`411`	`411`
`412`	`412`	`static void CreateCreditAndSpend(const FillableSigningProvider& keystore, const CScript& outscript, CTransactionRef& output, CMutableTransaction& input, bool success = true)`
Original file line number	Diff line number	Diff line change
`@@ -723,8 +723,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)`
`723`	`723`	`}`
`724`	`724`
`725`	`725`	`// Check for non-standard pay-to-script-hash in inputs`
`726`		`- const bool taproot_active = DeploymentActiveAfter(m_active_chainstate.m_chain.Tip(), args.m_chainparams.GetConsensus(), Consensus::DEPLOYMENT_TAPROOT);`
`727`		`- if (fRequireStandard && !AreInputsStandard(tx, m_view, taproot_active)) {`
	`726`	`+ if (fRequireStandard && !AreInputsStandard(tx, m_view)) {`
`728`	`727`	`return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");`
`729`	`728`	`}`
`730`	`729`