BIP 54: return the appropriate type of error according to activation status in mempool submission

src/consensus/tx_verify.cpp

@@ -187,18 +187,14 @@ 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)) {
         return state.Invalid(TxValidationResult::TX_MISSING_INPUTS, "bad-txns-inputs-missingorspent",
                          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;

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) */

src/policy/policy.cpp

@@ -181,13 +181,19 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& 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)
 {
     if (tx.IsCoinBase()) {
         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;
 

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<int>(0, std::numeric_limits<int>::max()), tx_fee_out, enforce_bip54)) {
+                if (Consensus::CheckTxInputs(transaction, state, coins_view_cache, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, std::numeric_limits<int>::max()), tx_fee_out)) {
                     assert(MoneyRange(tx_fee_out));
                 }
             },

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. */

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<int>::max());
     }

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

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"

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,16 +319,20 @@ 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']))]
         anchor_spk = CScript([OP_1, b"\x4e\x73"])
         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__":

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,
         )