Merge bitcoin/bitcoin#32521: policy: make pathological transactions packed with legacy sigops non-standard

96da68a qa: functional test a transaction running into the legacy sigop limit (Antoine Poinsot)
3671479 qa: unit test standardness of inputs packed with legacy sigops (Antoine Poinsot)
5863315 policy: make pathological transactions packed with legacy sigops non-standard. (Antoine Poinsot)

Pull request description:

  The Consensus Cleanup soft fork proposal includes a limit on the number of legacy signature
  operations potentially executed when validating a transaction. If this change is to be implemented
  here and activated by Bitcoin users in the future, we should make transactions that are not valid
  according to the new rules non-standard first because it would otherwise be a trivial DoS to
  potentially unupgraded miners after the soft fork activates.

  ML post: https://gnusha.org/pi/bitcoindev/49dyqqkf5NqGlGdinp6SELIoxzE_ONh3UIj6-EB8S804Id5yROq-b1uGK8DUru66eIlWuhb5R3nhRRutwuYjemiuOOBS2FQ4KWDnEh0wLuA=@protonmail.com/T/#u

ACKs for top commit:
  instagibbs:
    reACK bitcoin/bitcoin@96da68a
  maflcko:
    review ACK 96da68a 🚋
  achow101:
    ACK 96da68a
  glozow:
    light code review ACK 96da68a, looks correct to me

Tree-SHA512: 106ffe62e48952affa31c5894a404a17a3b4ea8971815828166fba89069f757366129f7807205e8c6558beb75c6f67d8f9a41000be2f8cf95be3b1a02d87bfe9

Author: achow101

src/policy/policy.cpp

@@ -163,6 +163,35 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
     return true;
 }
 
+/**
+ * Check the total number of non-witness sigops across the whole transaction, as per BIP54.
+ */
+static bool CheckSigopsBIP54(const CTransaction& tx, const CCoinsViewCache& inputs)
+{
+    Assert(!tx.IsCoinBase());
+
+    unsigned int sigops{0};
+    for (const auto& txin: tx.vin) {
+        const auto& prev_txo{inputs.AccessCoin(txin.prevout).out};
+
+        // Unlike the existing block wide sigop limit which counts sigops present in the block
+        // itself (including the scriptPubKey which is not executed until spending later), BIP54
+        // counts sigops in the block where they are potentially executed (only).
+        // This means sigops in the spent scriptPubKey count toward the limit.
+        // `fAccurate` means correctly accounting sigops for CHECKMULTISIGs(VERIFY) with 16 pubkeys
+        // or fewer. This method of accounting was introduced by BIP16, and BIP54 reuses it.
+        // The GetSigOpCount call on the previous scriptPubKey counts both bare and P2SH sigops.
+        sigops += txin.scriptSig.GetSigOpCount(/*fAccurate=*/true);
+        sigops += prev_txo.scriptPubKey.GetSigOpCount(txin.scriptSig);
+
+        if (sigops > MAX_TX_LEGACY_SIGOPS) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 /**
  * Check transaction inputs.
  *
@@ -178,13 +207,19 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
  *    as potential new upgrade hooks.
  *
  * Note that only the non-witness portion of the transaction is checked here.
+ *
+ * We also check the total number of non-witness sigops across the whole transaction, as per BIP54.
  */
 bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
 {
     if (tx.IsCoinBase()) {
         return true; // Coinbases don't use vin normally
     }
 
+    if (!CheckSigopsBIP54(tx, mapInputs)) {
+        return false;
+    }
+
     for (unsigned int i = 0; i < tx.vin.size(); i++) {
         const CTxOut& prev = mapInputs.AccessCoin(tx.vin[i].prevout).out;
 

src/policy/policy.h

@@ -38,6 +38,8 @@ static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{65};
 static constexpr unsigned int MAX_P2SH_SIGOPS{15};
 /** The maximum number of sigops we're willing to relay/mine in a single tx */
 static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5};
+/** The maximum number of potentially executed legacy signature operations in a single standard tx */
+static constexpr unsigned int MAX_TX_LEGACY_SIGOPS{2'500};
 /** Default for -incrementalrelayfee, which sets the minimum feerate increase for mempool limiting or replacement **/
 static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
 /** Default for -bytespersigop */

src/test/transaction_tests.cpp

@@ -10,6 +10,7 @@
 #include <clientversion.h>
 #include <consensus/amount.h>
 #include <consensus/tx_check.h>
+#include <consensus/tx_verify.h>
 #include <consensus/validation.h>
 #include <core_io.h>
 #include <key.h>
@@ -1053,4 +1054,99 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
     CheckIsNotStandard(t, "dust");
 }
 
+BOOST_AUTO_TEST_CASE(max_standard_legacy_sigops)
+{
+    CCoinsView coins_dummy;
+    CCoinsViewCache coins(&coins_dummy);
+    CKey key;
+    key.MakeNewKey(true);
+
+    // Create a pathological P2SH script padded with as many sigops as is standard.
+    CScript max_sigops_redeem_script{CScript() << std::vector<unsigned char>{} << key.GetPubKey()};
+    for (unsigned i{0}; i < MAX_P2SH_SIGOPS - 1; ++i) max_sigops_redeem_script << OP_2DUP << OP_CHECKSIG << OP_DROP;
+    max_sigops_redeem_script << OP_CHECKSIG << OP_NOT;
+    const CScript max_sigops_p2sh{GetScriptForDestination(ScriptHash(max_sigops_redeem_script))};
+
+    // Create a transaction fanning out as many such P2SH outputs as is standard to spend in a
+    // single transaction, and a transaction spending them.
+    CMutableTransaction tx_create, tx_max_sigops;
+    const unsigned p2sh_inputs_count{MAX_TX_LEGACY_SIGOPS / MAX_P2SH_SIGOPS};
+    tx_create.vout.reserve(p2sh_inputs_count);
+    for (unsigned i{0}; i < p2sh_inputs_count; ++i) {
+        tx_create.vout.emplace_back(424242 + i, max_sigops_p2sh);
+    }
+    auto prev_txid{tx_create.GetHash()};
+    tx_max_sigops.vin.reserve(p2sh_inputs_count);
+    for (unsigned i{0}; i < p2sh_inputs_count; ++i) {
+        tx_max_sigops.vin.emplace_back(prev_txid, i, CScript() << ToByteVector(max_sigops_redeem_script));
+    }
+
+    // p2sh_inputs_count is truncated to 166 (from 166.6666..)
+    BOOST_CHECK_LT(p2sh_inputs_count * MAX_P2SH_SIGOPS, MAX_TX_LEGACY_SIGOPS);
+    AddCoins(coins, CTransaction(tx_create), 0, false);
+
+    // 2490 sigops is below the limit.
+    BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(tx_max_sigops), coins), 2490);
+    BOOST_CHECK(::AreInputsStandard(CTransaction(tx_max_sigops), coins));
+
+    // Adding one more input will bump this to 2505, hitting the limit.
+    tx_create.vout.emplace_back(424242, max_sigops_p2sh);
+    prev_txid = tx_create.GetHash();
+    for (unsigned i{0}; i < p2sh_inputs_count; ++i) {
+        tx_max_sigops.vin[i] = CTxIn(COutPoint(prev_txid, i), CScript() << ToByteVector(max_sigops_redeem_script));
+    }
+    tx_max_sigops.vin.emplace_back(prev_txid, p2sh_inputs_count, CScript() << ToByteVector(max_sigops_redeem_script));
+    AddCoins(coins, CTransaction(tx_create), 0, false);
+    BOOST_CHECK_GT((p2sh_inputs_count + 1) * MAX_P2SH_SIGOPS, MAX_TX_LEGACY_SIGOPS);
+    BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(tx_max_sigops), coins), 2505);
+    BOOST_CHECK(!::AreInputsStandard(CTransaction(tx_max_sigops), coins));
+
+    // Now, check the limit can be reached with regular P2PK outputs too. Use a separate
+    // preparation transaction, to demonstrate spending coins from a single tx is irrelevant.
+    CMutableTransaction tx_create_p2pk;
+    const auto p2pk_script{CScript() << key.GetPubKey() << OP_CHECKSIG};
+    unsigned p2pk_inputs_count{10}; // From 2490 to 2500.
+    for (unsigned i{0}; i < p2pk_inputs_count; ++i) {
+        tx_create_p2pk.vout.emplace_back(212121 + i, p2pk_script);
+    }
+    prev_txid = tx_create_p2pk.GetHash();
+    tx_max_sigops.vin.resize(p2sh_inputs_count); // Drop the extra input.
+    for (unsigned i{0}; i < p2pk_inputs_count; ++i) {
+        tx_max_sigops.vin.emplace_back(prev_txid, i);
+    }
+    AddCoins(coins, CTransaction(tx_create_p2pk), 0, false);
+
+    // The transaction now contains exactly 2500 sigops, the check should pass.
+    BOOST_CHECK_EQUAL(p2sh_inputs_count * MAX_P2SH_SIGOPS + p2pk_inputs_count * 1, MAX_TX_LEGACY_SIGOPS);
+    BOOST_CHECK(::AreInputsStandard(CTransaction(tx_max_sigops), coins));
+
+    // Now, add some Segwit inputs. We add one for each defined Segwit output type. The limit
+    // is exclusively on non-witness sigops and therefore those should not be counted.
+    CMutableTransaction tx_create_segwit;
+    const auto witness_script{CScript() << key.GetPubKey() << OP_CHECKSIG};
+    tx_create_segwit.vout.emplace_back(121212, GetScriptForDestination(WitnessV0KeyHash(key.GetPubKey())));
+    tx_create_segwit.vout.emplace_back(131313, GetScriptForDestination(WitnessV0ScriptHash(witness_script)));
+    tx_create_segwit.vout.emplace_back(141414, GetScriptForDestination(WitnessV1Taproot{XOnlyPubKey(key.GetPubKey())}));
+    prev_txid = tx_create_segwit.GetHash();
+    for (unsigned i{0}; i < tx_create_segwit.vout.size(); ++i) {
+        tx_max_sigops.vin.emplace_back(prev_txid, i);
+    }
+
+    // The transaction now still contains exactly 2500 sigops, the check should pass.
+    AddCoins(coins, CTransaction(tx_create_segwit), 0, false);
+    BOOST_REQUIRE(::AreInputsStandard(CTransaction(tx_max_sigops), coins));
+
+    // Add one more P2PK input. We'll reach the limit.
+    tx_create_p2pk.vout.emplace_back(212121, p2pk_script);
+    prev_txid = tx_create_p2pk.GetHash();
+    tx_max_sigops.vin.resize(p2sh_inputs_count);
+    ++p2pk_inputs_count;
+    for (unsigned i{0}; i < p2pk_inputs_count; ++i) {
+        tx_max_sigops.vin.emplace_back(prev_txid, i);
+    }
+    AddCoins(coins, CTransaction(tx_create_p2pk), 0, false);
+    BOOST_CHECK_GT(p2sh_inputs_count * MAX_P2SH_SIGOPS + p2pk_inputs_count * 1, MAX_TX_LEGACY_SIGOPS);
+    BOOST_CHECK(!::AreInputsStandard(CTransaction(tx_max_sigops), coins));
+}
+
 BOOST_AUTO_TEST_SUITE_END()

test/functional/mempool_sigoplimit.py

@@ -3,6 +3,7 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test sigop limit mempool policy (`-bytespersigop` parameter)"""
+from copy import deepcopy
 from decimal import Decimal
 from math import ceil
 
@@ -17,23 +18,30 @@
 )
 from test_framework.script import (
     CScript,
+    OP_2DUP,
     OP_CHECKMULTISIG,
     OP_CHECKSIG,
+    OP_DROP,
     OP_ENDIF,
     OP_FALSE,
     OP_IF,
+    OP_NOT,
     OP_RETURN,
     OP_TRUE,
 )
 from test_framework.script_util import (
     keys_to_multisig_script,
     script_to_p2wsh_script,
+    script_to_p2sh_script,
+    MAX_STD_LEGACY_SIGOPS,
+    MAX_STD_P2SH_SIGOPS,
 )
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
     assert_greater_than,
     assert_greater_than_or_equal,
+    assert_raises_rpc_error,
 )
 from test_framework.wallet import MiniWallet
 from test_framework.wallet_util import generate_keypair
@@ -174,6 +182,42 @@ def create_bare_multisig_tx(utxo_to_spend=None):
         # Transactions are tiny in weight
         assert_greater_than(2000, tx_parent.get_weight() + tx_child.get_weight())
 
+    def test_legacy_sigops_stdness(self):
+        self.log.info("Test a transaction with too many legacy sigops in its inputs is non-standard.")
+
+        # Restart with the default settings
+        self.restart_node(0)
+
+        # Create a P2SH script with 15 sigops.
+        _, dummy_pubkey = generate_keypair()
+        packed_redeem_script = [dummy_pubkey]
+        for _ in range(MAX_STD_P2SH_SIGOPS - 1):
+            packed_redeem_script += [OP_2DUP, OP_CHECKSIG, OP_DROP]
+        packed_redeem_script = CScript(packed_redeem_script + [OP_CHECKSIG, OP_NOT])
+        packed_p2sh_script = script_to_p2sh_script(packed_redeem_script)
+
+        # Create enough outputs to reach the sigops limit when spending them all at once.
+        outpoints = []
+        for _ in range(int(MAX_STD_LEGACY_SIGOPS / MAX_STD_P2SH_SIGOPS) + 1):
+            res = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=packed_p2sh_script, amount=1_000)
+            txid = int.from_bytes(bytes.fromhex(res["txid"]), byteorder="big")
+            outpoints.append(COutPoint(txid, res["sent_vout"]))
+        self.generate(self.nodes[0], 1)
+
+        # Spending all these outputs at once accounts for 2505 legacy sigops and is non-standard.
+        nonstd_tx = CTransaction()
+        nonstd_tx.vin = [CTxIn(op, CScript([b"", packed_redeem_script])) for op in outpoints]
+        nonstd_tx.vout = [CTxOut(0, CScript([OP_RETURN, b""]))]
+        assert_raises_rpc_error(-26, "bad-txns-nonstandard-inputs", self.nodes[0].sendrawtransaction, nonstd_tx.serialize().hex())
+
+        # Spending one less accounts for 2490 legacy sigops and is standard.
+        std_tx = deepcopy(nonstd_tx)
+        std_tx.vin.pop()
+        self.nodes[0].sendrawtransaction(std_tx.serialize().hex())
+
+        # Make sure the original, non-standard, transaction can be mined.
+        self.generateblock(self.nodes[0], output="raw(42)", transactions=[nonstd_tx.serialize().hex()])
+
     def run_test(self):
         self.wallet = MiniWallet(self.nodes[0])
 
@@ -191,6 +235,7 @@ def run_test(self):
             self.generate(self.wallet, 1)
 
         self.test_sigops_package()
+        self.test_legacy_sigops_stdness()
 
 
 if __name__ == '__main__':

test/functional/test_framework/script_util.py

@@ -35,6 +35,12 @@
     hash160,
 )
 
+# Maximum number of potentially executed legacy signature operations in validating a transaction.
+MAX_STD_LEGACY_SIGOPS = 2_500
+
+# Maximum number of sigops per standard P2SH redeemScript.
+MAX_STD_P2SH_SIGOPS = 15
+
 # To prevent a "tx-size-small" policy rule error, a transaction has to have a
 # non-witness size of at least 65 bytes (MIN_STANDARD_TX_NONWITNESS_SIZE in
 # src/policy/policy.h). Considering a Tx with the smallest possible single