From 5863315e33ba9b75a1e5189ee3da3d7311bbf193 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 19 Mar 2025 17:25:06 -0400 Subject: [PATCH 1/3] policy: make pathological transactions packed with legacy sigops non-standard. 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 prevent the ability for someone to broadcast a transaction through the p2p network that is not valid according to the new rules. This is because if it was possible it would be a trivial DoS to potentially unupgraded miners after the soft fork activates. We do not know for sure whether users will activate the Consensus Cleanup. However if they do such transactions must have been made non-standard long in advance, due to the time it takes for most nodes on the network to upgrade. In addition this limit may only be run into by pathological transactions which pad the Script with sigops but do not use actual signatures when spending, as otherwise they would run into the standard transaction size limit. --- src/policy/policy.cpp | 35 +++++++++++++++++++++++++++++++++++ src/policy/policy.h | 2 ++ 2 files changed, 37 insertions(+) diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index fdc2fdb9cdf..48f2a6a7446 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -163,6 +163,35 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& 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,6 +207,8 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& 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) { @@ -185,6 +216,10 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) 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; diff --git a/src/policy/policy.h b/src/policy/policy.h index e62efa02e3e..f9a18561bce 100644 --- a/src/policy/policy.h +++ b/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 */ From 367147954d16c961bbd28c361abf27b4cb665f10 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 19 Mar 2025 17:35:14 -0400 Subject: [PATCH 2/3] qa: unit test standardness of inputs packed with legacy sigops Check bounds and different output types. --- src/test/transaction_tests.cpp | 96 ++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 54b3216a4c6..5f03641e99e 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -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{} << 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() From 96da68a38fa295d2414685739c41b8626e198d27 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 20 Mar 2025 09:44:10 -0400 Subject: [PATCH 3/3] qa: functional test a transaction running into the legacy sigop limit It's useful to have an end-to-end test in addition to the unit test to sanity check the RPC error as well as making sure the transaction is otherwise fully standard. --- test/functional/mempool_sigoplimit.py | 45 +++++++++++++++++++ test/functional/test_framework/script_util.py | 6 +++ 2 files changed, 51 insertions(+) diff --git a/test/functional/mempool_sigoplimit.py b/test/functional/mempool_sigoplimit.py index 23ddf0bac9c..6b2a0268c4d 100755 --- a/test/functional/mempool_sigoplimit.py +++ b/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.messages import ( ) 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 @@ class BytesPerSigOpTest(BitcoinTestFramework): # 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 @@ class BytesPerSigOpTest(BitcoinTestFramework): self.generate(self.wallet, 1) self.test_sigops_package() + self.test_legacy_sigops_stdness() if __name__ == '__main__': diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py index 938183ece4c..fce32e138ee 100755 --- a/test/functional/test_framework/script_util.py +++ b/test/functional/test_framework/script_util.py @@ -22,6 +22,12 @@ from test_framework.script import ( sha256, ) +# 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