From 72ddf4237bf14c1519a4d4dfc9d5ee39dbc5ed88 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Wed, 5 Oct 2022 14:50:30 -0400 Subject: [PATCH] Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 64 non-witness bytes exactly Restricting sizes below 64 vbytes interferes with specific use-cases, and does not interfere with proposed consensus changes such as the Great Consensus Cleanup of 2025. We change software to protect against the case we care about: 64 bytes --- src/policy/policy.h | 4 +- src/validation.cpp | 7 ++-- src/wallet/spend.cpp | 2 +- test/functional/data/invalid_txs.py | 14 +++---- test/functional/mempool_accept.py | 40 ++++++++++--------- test/functional/rpc_psbt.py | 4 +- test/functional/test_framework/script_util.py | 17 ++++---- 7 files changed, 45 insertions(+), 43 deletions(-) diff --git a/src/policy/policy.h b/src/policy/policy.h index 2151ec13dd0..91f1f31cc42 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -32,8 +32,8 @@ static constexpr unsigned int MINIMUM_BLOCK_RESERVED_WEIGHT{2000}; static constexpr unsigned int DEFAULT_BLOCK_MIN_TX_FEE{1000}; /** The maximum weight for transactions we're willing to relay/mine */ static constexpr int32_t MAX_STANDARD_TX_WEIGHT{400000}; -/** The minimum non-witness size for transactions we're willing to relay/mine: one larger than 64 */ -static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{65}; +/** In addition to transactions that are too big, do not relay/mine ones that are 64 non-witness bytes exactly: CVE-2017-12842 */ +static constexpr unsigned int NONSTANDARD_TX_NONWITNESS_SIZE{64}; /** Maximum number of signature check operations in an IsStandard() P2SH script */ static constexpr unsigned int MAX_P2SH_SIGOPS{15}; /** The maximum number of sigops we're willing to relay/mine in a single tx */ diff --git a/src/validation.cpp b/src/validation.cpp index fedcb9ca57f..2996a33057a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -794,9 +794,10 @@ 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. - if (::GetSerializeSize(TX_NO_WITNESS(tx)) < MIN_STANDARD_TX_NONWITNESS_SIZE) - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "tx-size-small"); + // Transactions exactly 64 non-witness bytes are not relayed to mitigate CVE-2017-12842. + if (::GetSerializeSize(TX_NO_WITNESS(tx)) == NONSTANDARD_TX_NONWITNESS_SIZE) { + return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "tx-bad-nonwit-size"); + } // Only accept nLockTime-using transactions that can be mined in the next // block; we don't want our mempool filled up with transactions that can't diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 7b26cf15bd3..c6873eb7323 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1027,7 +1027,7 @@ static util::Result CreateTransactionInternal( coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs; coin_selection_params.m_max_tx_weight = coin_control.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT); - int minimum_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR; + int minimum_tx_weight = (NONSTANDARD_TX_NONWITNESS_SIZE + 1) * WITNESS_SCALE_FACTOR; if (coin_selection_params.m_max_tx_weight.value() < minimum_tx_weight || coin_selection_params.m_max_tx_weight.value() > MAX_STANDARD_TX_WEIGHT) { return util::Error{strprintf(_("Maximum transaction weight must be between %d and %d"), minimum_tx_weight, MAX_STANDARD_TX_WEIGHT)}; } diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index d2d7202d860..8e88a47bdf7 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -46,15 +46,14 @@ from test_framework.script import ( OP_MOD, OP_MUL, OP_OR, - OP_RETURN, OP_RIGHT, OP_RSHIFT, OP_SUBSTR, OP_XOR, ) from test_framework.script_util import ( - MIN_PADDING, - MIN_STANDARD_TX_NONWITNESS_SIZE, + NONSTANDARD_OP_RETURN_SCRIPT, + NONSTANDARD_TX_NONWITNESS_SIZE, script_to_p2sh_script, ) basic_p2sh = script_to_p2sh_script(CScript([OP_0])) @@ -115,17 +114,16 @@ class InputMissing(BadTxTemplate): # The following check prevents exploit of lack of merkle # tree depth commitment (CVE-2017-12842) -class SizeTooSmall(BadTxTemplate): - reject_reason = "tx-size-small" +class BadNonWitSize(BadTxTemplate): + reject_reason = "tx-bad-nonwit-size" expect_disconnect = False valid_in_block = True def get_tx(self): tx = CTransaction() tx.vin.append(self.valid_txin) - tx.vout.append(CTxOut(0, CScript([OP_RETURN] + ([OP_0] * (MIN_PADDING - 2))))) - assert len(tx.serialize_without_witness()) == 64 - assert MIN_STANDARD_TX_NONWITNESS_SIZE - 1 == 64 + tx.vout.append(CTxOut(0, NONSTANDARD_OP_RETURN_SCRIPT)) + assert len(tx.serialize_without_witness()) == NONSTANDARD_TX_NONWITNESS_SIZE tx.calc_sha256() return tx diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 9014ab260e1..c153693a1d5 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -33,17 +33,16 @@ from test_framework.script import ( sign_input_legacy, ) from test_framework.script_util import ( - DUMMY_MIN_OP_RETURN_SCRIPT, + INVALID_SPK_LEN, keys_to_multisig_script, - MIN_PADDING, - MIN_STANDARD_TX_NONWITNESS_SIZE, + NONSTANDARD_OP_RETURN_SCRIPT, + NONSTANDARD_TX_NONWITNESS_SIZE, PAY_TO_ANCHOR, script_to_p2sh_script, script_to_p2wsh_script, ) from test_framework.util import ( assert_equal, - assert_greater_than, assert_raises_rpc_error, sync_txindex, ) @@ -352,30 +351,35 @@ class MempoolAcceptanceTest(BitcoinTestFramework): maxfeerate=0, ) - # Prep for tiny-tx tests with wsh(OP_TRUE) output - seed_tx = self.wallet.send_to(from_node=node, scriptPubKey=script_to_p2wsh_script(CScript([OP_TRUE])), amount=COIN) + # Prep for tiny-tx tests with MiniWallet anyone-can-spend output + seed_utxo = self.wallet.send_self_transfer(from_node=node)["new_utxo"] self.generate(node, 1) self.log.info('A tiny transaction(in non-witness bytes) that is disallowed') - tx = CTransaction() - tx.vin.append(CTxIn(COutPoint(int(seed_tx["txid"], 16), seed_tx["sent_vout"]), b"", SEQUENCE_FINAL)) - tx.wit.vtxinwit = [CTxInWitness()] - tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])] - tx.vout.append(CTxOut(0, CScript([OP_RETURN] + ([OP_0] * (MIN_PADDING - 2))))) + tx = self.wallet.create_self_transfer(utxo_to_spend=seed_utxo)["tx"] + tx.vout[0].scriptPubKey = NONSTANDARD_OP_RETURN_SCRIPT # Note it's only non-witness size that matters! - assert_equal(len(tx.serialize_without_witness()), 64) - assert_equal(MIN_STANDARD_TX_NONWITNESS_SIZE - 1, 64) - assert_greater_than(len(tx.serialize()), 64) + assert_equal(len(tx.serialize_without_witness()), NONSTANDARD_TX_NONWITNESS_SIZE) + assert len(tx.serialize()) != NONSTANDARD_TX_NONWITNESS_SIZE 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': 'tx-bad-nonwit-size'}], rawtxs=[tx.serialize().hex()], maxfeerate=0, ) - self.log.info('Minimally-small transaction(in non-witness bytes) that is allowed') - tx.vout[0] = CTxOut(COIN - 1000, DUMMY_MIN_OP_RETURN_SCRIPT) - assert_equal(len(tx.serialize_without_witness()), MIN_STANDARD_TX_NONWITNESS_SIZE) + self.log.info('Just-below size transaction(in non-witness bytes) that is allowed') + tx.vout[0] = CTxOut(int(seed_utxo["value"] * COIN) - 1000, CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN - 2)))) + assert_equal(len(tx.serialize_without_witness()), NONSTANDARD_TX_NONWITNESS_SIZE - 1) + self.check_mempool_result( + result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal('0.00001000')}}], + rawtxs=[tx.serialize().hex()], + maxfeerate=0, + ) + + self.log.info('Just-above size transaction(in non-witness bytes) that is allowed') + tx.vout[0] = CTxOut(int(seed_utxo["value"] * COIN) - 1000, CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN)))) + assert_equal(len(tx.serialize_without_witness()), NONSTANDARD_TX_NONWITNESS_SIZE + 1) self.check_mempool_result( result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal('0.00001000')}}], rawtxs=[tx.serialize().hex()], diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 04e68a97af7..bfcbe45d07d 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -34,7 +34,7 @@ from test_framework.psbt import ( PSBT_OUT_TAP_TREE, ) from test_framework.script import CScript, OP_TRUE -from test_framework.script_util import MIN_STANDARD_TX_NONWITNESS_SIZE +from test_framework.script_util import NONSTANDARD_TX_NONWITNESS_SIZE from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_approx, @@ -215,7 +215,7 @@ class PSBTTest(BitcoinTestFramework): self.log.info("Test for invalid maximum transaction weights") dest_arg = [{self.nodes[0].getnewaddress(): 1}] - min_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR + min_tx_weight = (NONSTANDARD_TX_NONWITNESS_SIZE + 1) * WITNESS_SCALE_FACTOR assert_raises_rpc_error(-4, f"Maximum transaction weight must be between {min_tx_weight} and {MAX_STANDARD_TX_WEIGHT}", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": -1}) assert_raises_rpc_error(-4, f"Maximum transaction weight must be between {min_tx_weight} and {MAX_STANDARD_TX_WEIGHT}", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": 0}) assert_raises_rpc_error(-4, f"Maximum transaction weight must be between {min_tx_weight} and {MAX_STANDARD_TX_WEIGHT}", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": MAX_STANDARD_TX_WEIGHT + 1}) diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py index 938183ece4c..cba190ce188 100755 --- a/test/functional/test_framework/script_util.py +++ b/test/functional/test_framework/script_util.py @@ -22,8 +22,8 @@ from test_framework.script import ( sha256, ) -# 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 +# To prevent a "tx-bad-nonwit-size" policy rule error, a transaction may not have a +# non-witness size of 64 bytes (NONSTANDARD_TX_NONWITNESS_SIZE in # src/policy/policy.h). Considering a Tx with the smallest possible single # input (blank, empty scriptSig), and with an output omitting the scriptPubKey, # we get to a minimum size of 60 bytes: @@ -32,16 +32,15 @@ from test_framework.script import ( # Blank Input: 32 [PrevTxHash] + 4 [Index] + 1 [scriptSigLen] + 4 [SeqNo] = 41 bytes # Output: 8 [Amount] + 1 [scriptPubKeyLen] = 9 bytes # -# Hence, the scriptPubKey of the single output has to have a size of at -# least 5 bytes. -MIN_STANDARD_TX_NONWITNESS_SIZE = 65 -MIN_PADDING = MIN_STANDARD_TX_NONWITNESS_SIZE - 10 - 41 - 9 -assert MIN_PADDING == 5 +# Hence, the scriptPubKey of the single output MUST NOT have a size of 4. +NONSTANDARD_TX_NONWITNESS_SIZE = 64 +INVALID_SPK_LEN = NONSTANDARD_TX_NONWITNESS_SIZE - 10 - 41 - 9 +assert INVALID_SPK_LEN == 4 # This script cannot be spent, allowing dust output values under # standardness checks -DUMMY_MIN_OP_RETURN_SCRIPT = CScript([OP_RETURN] + ([OP_0] * (MIN_PADDING - 1))) -assert len(DUMMY_MIN_OP_RETURN_SCRIPT) == MIN_PADDING +NONSTANDARD_OP_RETURN_SCRIPT = CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN - 1))) +assert len(NONSTANDARD_OP_RETURN_SCRIPT) == INVALID_SPK_LEN PAY_TO_ANCHOR = CScript([OP_1, bytes.fromhex("4e73")])