diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index e4111a74e3ff..c7744c12edac 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -187,7 +187,7 @@ bool Consensus::CheckSigopsBIP54(const CTransaction& tx, const CCoinsViewCache& return true; } -bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee, bool enforce_bip54) +bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee) { // are the actual inputs available? if (!inputs.HaveInputs(tx)) { @@ -195,10 +195,6 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, strprintf("%s: inputs missing/spent", __func__)); } - if (enforce_bip54 && !Consensus::CheckSigopsBIP54(tx, inputs)) { - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-legacy-sigops", "too many legacy sigops (BIP54)"); - } - CAmount nValueIn = 0; for (unsigned int i = 0; i < tx.vin.size(); ++i) { const COutPoint &prevout = tx.vin[i].prevout; diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index c568fcf43abc..f387fed7fd6d 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -28,10 +28,9 @@ bool CheckSigopsBIP54(const CTransaction& tx, const CCoinsViewCache& inputs); * Check whether all inputs of this transaction are valid (no double spends and amounts) * This does not modify the UTXO set. This does not check scripts and sigs. * @param[out] txfee Set to the transaction fee if successful. - * @param[in] enforce_bip54 Whether to perform the BIP54 sigops check. * Preconditions: tx.IsCoinBase() is false. */ -[[nodiscard]] bool CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee, bool enforce_bip54); +[[nodiscard]] bool CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee); } // namespace Consensus /** Auxiliary functions for transaction validation (ideally should not be exposed) */ diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index fdf749bcd233..59d053b04aa6 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -181,6 +181,8 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat * DUP CHECKSIG DROP ... repeated 100 times... OP_1 * * 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) { @@ -188,6 +190,10 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) return true; // Coinbases don't use vin normally } + if (!Consensus::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/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 6ddc3fbf697e..c435ed91ba79 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -255,12 +255,7 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) // It is not allowed to call CheckTxInputs if CheckTransaction failed return; } - if (transaction.IsCoinBase()) { - // It is not allowed to call CheckTxInputs on a coinbase transaction. - return; - } - const bool enforce_bip54{fuzzed_data_provider.ConsumeBool()}; - if (Consensus::CheckTxInputs(transaction, state, coins_view_cache, fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max()), tx_fee_out, enforce_bip54)) { + if (Consensus::CheckTxInputs(transaction, state, coins_view_cache, fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max()), tx_fee_out)) { assert(MoneyRange(tx_fee_out)); } }, diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 8b22f7570bdc..08302fab3907 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -1021,9 +1021,6 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) BOOST_AUTO_TEST_CASE(max_standard_legacy_sigops) { - TxValidationState state; - const int dummy_height{0}; - CAmount dummy_fee; CCoinsView coins_dummy; CCoinsViewCache coins(&coins_dummy); CKey key; @@ -1055,8 +1052,7 @@ BOOST_AUTO_TEST_CASE(max_standard_legacy_sigops) // 2490 sigops is below the limit. BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(tx_max_sigops), coins), 2490); - BOOST_CHECK(Consensus::CheckTxInputs(CTransaction(tx_max_sigops), state, coins, dummy_height, dummy_fee, /*enforce_bip54=*/true)); - BOOST_CHECK(state.IsValid()); + 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); @@ -1068,9 +1064,7 @@ BOOST_AUTO_TEST_CASE(max_standard_legacy_sigops) AddCoins(coins, CTransaction(tx_create), 0, false); BOOST_CHECK_GT((p2sh_inputs_count + 1) * MAX_P2SH_SIGOPS, MAX_TX_BIP54_SIGOPS); BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(tx_max_sigops), coins), 2505); - BOOST_CHECK(!Consensus::CheckTxInputs(CTransaction(tx_max_sigops), state, coins, dummy_height, dummy_fee, /*enforce_bip54=*/true)); - BOOST_CHECK(state.IsInvalid()); - state = TxValidationState{}; + 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. @@ -1089,8 +1083,7 @@ BOOST_AUTO_TEST_CASE(max_standard_legacy_sigops) // 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_BIP54_SIGOPS); - BOOST_CHECK(Consensus::CheckTxInputs(CTransaction(tx_max_sigops), state, coins, dummy_height, dummy_fee, /*enforce_bip54=*/true)); - BOOST_CHECK(state.IsValid()); + 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. @@ -1106,8 +1099,7 @@ BOOST_AUTO_TEST_CASE(max_standard_legacy_sigops) // The transaction now still contains exactly 2500 sigops, the check should pass. AddCoins(coins, CTransaction(tx_create_segwit), 0, false); - BOOST_CHECK(Consensus::CheckTxInputs(CTransaction(tx_max_sigops), state, coins, dummy_height, dummy_fee, /*enforce_bip54=*/true)); - BOOST_CHECK(state.IsValid()); + BOOST_CHECK(::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); @@ -1119,8 +1111,7 @@ BOOST_AUTO_TEST_CASE(max_standard_legacy_sigops) } AddCoins(coins, CTransaction(tx_create_p2pk), 0, false); BOOST_CHECK_GT(p2sh_inputs_count * MAX_P2SH_SIGOPS + p2pk_inputs_count * 1, MAX_TX_BIP54_SIGOPS); - BOOST_CHECK(!Consensus::CheckTxInputs(CTransaction(tx_max_sigops), state, coins, dummy_height, dummy_fee, /*enforce_bip54=*/true)); - BOOST_CHECK(state.IsInvalid()); + BOOST_CHECK(!::AreInputsStandard(CTransaction(tx_max_sigops), coins)); } /** Sanity check the return value of SpendsNonAnchorWitnessProg for various output types. */ diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 59c4a7005b74..3a5a3fb306d3 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -777,7 +777,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei TxValidationState dummy_state; // Not used. CheckTxInputs() should always pass CAmount txfee = 0; assert(!tx.IsCoinBase()); - assert(Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, spendheight, txfee, /*enforce_bip54=*/true)); + assert(Consensus::CheckTxInputs(tx, dummy_state, mempoolDuplicate, spendheight, txfee)); for (const auto& input: tx.vin) mempoolDuplicate.SpendCoin(input.prevout); AddCoins(mempoolDuplicate, tx, std::numeric_limits::max()); } diff --git a/src/validation.cpp b/src/validation.cpp index d631eb04ef05..45759fe6576d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -786,6 +786,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) // Alias what we need out of ws TxValidationState& state = ws.m_state; + // Whether to return consensus errors for BIP 54 failures. + const bool bip54_active{DeploymentActiveAfter(m_active_chainstate.m_chain.Tip(), m_active_chainstate.m_chainman, Consensus::DEPLOYMENT_CONSENSUSCLEANUP)}; + if (!CheckTransaction(tx, state)) { return false; // state filled in by CheckTransaction } @@ -800,7 +803,12 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_NOT_STANDARD, reason); } - // Transactions smaller than 65 non-witness bytes are not relayed to mitigate CVE-2017-12842. + // To mitigate CVE-2017-12842, transactions smaller than 65 non-witness bytes are not relayed, + // and BIP 54 extends this protection at consensus by making the 64-byte case invalid. + const auto stripped_size{::GetSerializeSize(TX_NO_WITNESS(tx))}; + if (bip54_active && stripped_size == INVALID_TX_NONWITNESS_SIZE) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "txn-size-64", "Transactions with a witness-stripped size of exactly 64 bytes are invalid."); + } if (::GetSerializeSize(TX_NO_WITNESS(tx)) < MIN_STANDARD_TX_NONWITNESS_SIZE) return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "tx-size-small"); @@ -880,10 +888,16 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } // The mempool holds txs for the next block, so pass height+1 to CheckTxInputs - if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees, /*enforce_bip54=*/true)) { + if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees)) { return false; // state filled in by CheckTxInputs } + // If BIP 54 is active, return a consensus error on legacy sigops violation. Otherwise + // a standardness error will be returned below. + if (bip54_active && !Consensus::CheckSigopsBIP54(tx, m_view)) { + return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-legacy-sigops", "too many legacy sigops (BIP54)"); + } + if (m_pool.m_opts.require_standard && !AreInputsStandard(tx, m_view)) { return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs"); } @@ -2633,7 +2647,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, { CAmount txfee = 0; TxValidationState tx_state; - if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee, /*enforce_bip54=*/enforce_bip54)) { + if (!Consensus::CheckTxInputs(tx, tx_state, view, pindex->nHeight, txfee)) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), @@ -2647,6 +2661,11 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, break; } + if (enforce_bip54 && !Consensus::CheckSigopsBIP54(tx, view)) { + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-txns-legacy-sigops", + "contains a transaction with too many legacy sigops (BIP54)"); + } + // Check that transaction is BIP68 final // BIP68 lock checks (as opposed to nLockTime checks) must // be in ConnectBlock because they require the UTXO set diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index e7d45ae1b389..8709dc65dd1a 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -111,7 +111,7 @@ def get_tx(self): # The following check prevents exploit of lack of merkle # tree depth commitment (CVE-2017-12842) class SizeExactly64(BadTxTemplate): - reject_reason = "tx-size-small" + reject_reason = "txn-size-64" expect_disconnect = False valid_in_block = False block_reject_reason = "bad-txns-size" diff --git a/test/functional/feature_bip54.py b/test/functional/feature_bip54.py index 5d55a848065a..6030ec366fa1 100755 --- a/test/functional/feature_bip54.py +++ b/test/functional/feature_bip54.py @@ -23,8 +23,10 @@ OP_CHECKSIG, OP_DUP, OP_ENDIF, + OP_IF, OP_NOT, OP_NOTIF, + OP_0, OP_1, ) from test_framework.test_framework import BitcoinTestFramework @@ -97,6 +99,18 @@ def submit_block_many_sigops_split(self, node): coinbase_spk = self.wallet.get_output_script().hex() self.generateblock(node, f"raw({coinbase_spk})", txs) + def submit_tx_many_sigops(self, node): + """Create and submit to the mempool a transaction that violates the BIP54 sigops limit.""" + # A bare scriptPubKey that accounts for 2501 sigops + prep_spk = CScript([OP_0, OP_IF] + [OP_CHECKMULTISIG] * 125 + [OP_CHECKSIG, OP_ENDIF, OP_1]) + prep_tx = self.create_prep_tx(prep_spk) + tx = self.create_spend_tx(prep_tx, 0, CScript()) + + # Mine the (non-standard) preparation tx and submit the BIP54-invalid tx. + coinbase_spk = self.wallet.get_output_script().hex() + self.generateblock(node, f"raw({coinbase_spk})", [prep_tx.serialize().hex()]) + node.sendrawtransaction(tx.serialize().hex()) + def timewarp_attack(self, node): """Perform a pseudo Timewarp attack. Pseudo because regtest does not have retargets, so we only do the first period.""" # Reach the end of the current difficulty adjustment period. @@ -305,9 +319,8 @@ def submit_nontimelocked_cb(self, node): if err is not None: raise JSONRPCException({"message": err, "code": -25}) - def mine_block_64byte(self, node): - """Create a block that contains a 64-byte (Segwit) transaction.""" - # Create a 64-byte tx that spends a single Segwit input and contains a single p2a output. + def create_64byte_tx(self): + """Create a 64-byte tx that spends a single Segwit input and contains a single p2a output.""" prevout = self.wallet.get_utxo(confirmed_only=True) tx = CTransaction() tx.vin = [CTxIn(COutPoint(int(prevout['txid'], 16), prevout['vout']))] @@ -315,6 +328,11 @@ def mine_block_64byte(self, node): tx.vout = [CTxOut(0, anchor_spk)] self.wallet.sign_tx(tx) assert_equal(len(tx.serialize_without_witness()), 64) + return tx + + def mine_block_64byte(self, node): + """Create a block that contains a 64-byte (Segwit) transaction.""" + tx = self.create_64byte_tx() # Mine a block containing that transaction. prev_hash = node.getbestblockhash() @@ -328,6 +346,11 @@ def mine_block_64byte(self, node): return block + def submit_tx_64byte(self, node): + """Submit a 64-byte mempool transaction.""" + tx = self.create_64byte_tx() + node.sendrawtransaction(tx.serialize().hex()) + def run_test(self): node = self.nodes[0] self.wallet = MiniWallet(node) @@ -346,6 +369,10 @@ def run_test(self): # - Accept a block containing a 64-byte transaction. block_64b = self.mine_block_64byte(node).serialize().hex() assert_equal(node.submitblock(block_64b), None) + # - Return a standardness error for legacy sigops violation in mempool submission + assert_raises_rpc_error(-26, "bad-txns-nonstandard-inputs", self.submit_tx_many_sigops, node) + # - Return a standardness error for 64-byte transactions in mempool submission + assert_raises_rpc_error(-26, "tx-size-small", self.submit_tx_64byte, node) # Create a block with a version such as it will lock in the BIP54 deployment, then transition to activate. self.log.info("Activating BIP54") @@ -376,6 +403,10 @@ def run_test(self): # - Refuse a block containing a 64-byte transaction block_64b = self.mine_block_64byte(node).serialize().hex() assert_equal(node.submitblock(block_64b), "bad-txns-size") + # - Return a consensus error for legacy sigops violation in mempool submission + assert_raises_rpc_error(-26, "bad-txns-legacy-sigops", self.submit_tx_many_sigops, node) + # - Return a consensus error for 64-byte transactions in mempool submission + assert_raises_rpc_error(-26, "txn-size-64", self.submit_tx_64byte, node) if __name__ == "__main__": diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index d53a2d087a7e..70896c162588 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -417,7 +417,7 @@ def run_test(self): assert_greater_than(len(tx.serialize()), 64) self.check_mempool_result( - result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'tx-size-small'}], + result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'txn-size-64'}], rawtxs=[tx.serialize().hex()], maxfeerate=0, )