Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only #26398

instagibbs · 2022-10-26T17:35:30Z

Follow-on to to #26265

Implements the logic not effected by Great Consensus Cleanup 2025 effort: bitcoin/bips#1800

hernanmarino

code review ACK, looks OK. I suggest to change the title of this PR though, it suggests that we are relaxing the restriction to ALLOW 64 byte transactions, and might confuse someone.

src/validation.cpp

DrahtBot · 2022-10-26T23:02:46Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26398.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan
Concept ACK	hernanmarino, darosior
Approach NACK	ajtowns
Stale ACK	theStack, rajarshimaitra, glozow, maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32521 (policy: make pathological transactions packed with legacy sigops non-standard by darosior)
#32421 (test: refactor: overhaul (w)txid determination for CTransaction objects by theStack)
#31622 (psbt: add non-default sighash types to PSBTs and unify sighash type match checking by achow101)
#29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

rajarshimaitra

tACK a67341a

Code changes looks clean and inclining for this over #26265 . Even if it feels a odd check, if hiding the CVE isn't the purpose anymore, and 64 was the only culprit, no point punishing smaller use case..

instagibbs · 2022-10-27T14:59:35Z

@rajarshimaitra fwiw, I agree, I like this test code materially more, which suggests it's the right direction, with less indirection happening

maflcko

lgtm

test/functional/data/invalid_txs.py

darosior · 2022-10-28T10:14:42Z

Concept ACK, prefer this over #26265

theStack

Concept and code-review ACK 2ddd3a8

nit: could utilize more MiniWallet magic here, in order to avoid crafting the output of the parent tx and the input of the spending tx manually (works only as long as a segwit wallet mode is used, i.e. ADDRESS_OP_TRUE with segwitv1 outputs currently):

b/test/functional/mempool_accept.py
index c2d94ddad..09f8c3b23 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -36,7 +36,6 @@ from test_framework.script_util import (
     NONSTANDARD_OP_RETURN_SCRIPT,
     NONSTANDARD_TX_NONWITNESS_SIZE,
     script_to_p2sh_script,
-    script_to_p2wsh_script,
 )
 from test_framework.util import (
     assert_equal,
@@ -340,16 +339,13 @@ 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[0], 16), seed_tx[1]), b"", SEQUENCE_FINAL))
-        tx.wit.vtxinwit = [CTxInWitness()]
-        tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
-        tx.vout.append(CTxOut(0, NONSTANDARD_OP_RETURN_SCRIPT))
+        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()), NONSTANDARD_TX_NONWITNESS_SIZE)
         assert len(tx.serialize()) != NONSTANDARD_TX_NONWITNESS_SIZE
@@ -361,7 +357,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
         )

         self.log.info('Just-below size transaction(in non-witness bytes) that is allowed')
-        tx.vout[0] = CTxOut(COIN - 1000, CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN - 2))))
+        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')}}],
@@ -370,7 +366,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
         )

         self.log.info('Just-above size transaction(in non-witness bytes) that is allowed')
-        tx.vout[0] = CTxOut(COIN - 1000, CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN))))
+        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')}}],

maflcko · 2022-11-10T10:16:50Z

review ACK 2ddd3a8 🏊

Show signature

Signature:

For historical context, see #26265 (review)

Was there a recent thread on the mailing list you could link to? I can't seem to find the one you created, as there are about 5 trillion messages about RBF.

About the practical impact of this change: My understanding is that 60-byte transactions are still not policy-allowed, because an empty scriptPubKey is nonstandard. The smallest transaction this would allow is a 61-byte transaction with one nulldata output. Also, the smallest transaction with a witness_unknown output is 64-byte and would thus remain invalid. (Obviously putting the smallest witness_unknown, or an INVALID_SPK_LEN nulldata output in a large enough transaction is already valid today)

instagibbs · 2022-11-10T13:18:24Z

@MarcoFalke I agree with your statements

https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html

Also found this old one just now that I was a part of 2 years ago and don't recall: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html

glozow

code review ACK 2ddd3a8

Could you please link the ML post in the OP and write a release note?

src/policy/policy.h

instagibbs · 2022-11-10T19:39:35Z

@glozow pushed a brief release note along with edits from @glozow and @theStack

doc/release-notes-26398.md

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

instagibbs · 2025-03-27T19:53:07Z

Re-opened due to the recent Great Consensus Cleanup work, though I still think there may be minor debate about the utility of the change.

Align with Bitcoin Core's policy by reducing the minimum non-witness transaction size from 82 to 65 bytes. This change allows for more minimal transaction cases (e.g., 1 input with 1 OP_RETURN output), while still maintaining protection against CVE-2017-12842. Matches bitcoin/bitcoin#26398

glozow

Thanks for rebasing! Needs release note.

src/wallet/spend.cpp

ajtowns · 2025-03-31T11:04:58Z

I suggest we reconsider this. To add on to the earlier points in favor of this approach, I ran into "tx-size-small" when testing new policies with packages: a 1-in-1-out child spending the p2a of a parent can be less than 64 bytes.

That's easily fixed by having the 1-out be a 0sat OP_RETURN pushing three bytes of data... I remain Concept and Approach NACK on this, supporting 60-63 byte txs but not 64 byte txs just seems like it's asking for trouble. ie, just add the extra bytes to your output in your tests, this isn't worth optimising. And if it were worth optimising, we should actually optimise it.

OTOH, it might be plausible to just not worry about small-tx's at all, in which case the check could just be removed entirely, avoiding any discontinuities...

instagibbs · 2025-03-31T12:27:55Z

@ajtowns Bit of a tomato/tomato situation imo, because I think the rules as-is invite more trouble than clearly marking the exact case we're worried about.

The strongest case against this PR, imo, is not some application-level danger about discontinuity but the fact that once you open it up it's difficult to close. But it's been many years and no one has made an argument why this would be dangerous at a network level.

OTOH, it might be plausible to just not worry about small-tx's at all, in which case the check could just be removed entirely, avoiding any discontinuities...

If we end up deciding to not ban 64b by consensus, we should seriously consider this.

darosior · 2025-03-31T16:43:41Z

Concept ACK.

I don't buy that the discontinuity would cause any more confusion than making harmless transactions non-standard. The upgrade hook argument made sense back when this PR was closed, but i don't think it holds anymore now that no proposal includes invalidating <64 bytes transactions.

OTOH, it might be plausible to just not worry about small-tx's at all, in which case the check could just be removed entirely, avoiding any discontinuities...

This goes completely against your earlier argument of keeping our options open with regard to upgrade hooks! There is a current proposal to make those transactions invalid, so making them standard now seems unnecessarily risky.

If we end up deciding to not ban 64b by consensus, we should seriously consider this.

I'm sympathetic to this view but i'm not sure. We might decide it's not worth changing consensus over this, but it's not necessarily a reason to make it easier to exploit?

ajtowns · 2025-03-31T17:47:55Z

OTOH, it might be plausible to just not worry about small-tx's at all, in which case the check could just be removed entirely, avoiding any discontinuities...

This goes completely against your earlier argument of keeping our options open with regard to upgrade hooks! There is a current proposal to make those transactions invalid, so making them standard now seems unnecessarily risky.

If there is a need to worry about small transactions, then the conservative thing to do in consensus is forbid as little as possible (because things that have been forbidden are very difficult to re-enable), and the conservative thing to do in policy is to continue to forbid everything that's not obviously useful (because things that are enabled are very difficult to forbid). In both cases, it's the conservative option because it keeps more future possibilities open. Personally, I think ">65 is okay" is easier to deal with than "60 isn't really okay for different reasons, 61, 62, 63 are okay, 64 isn't, 65+ is".

But all of that is only relevant if there is a reason to worry about small transactions in the first place; if at some point it's established that there isn't a reason to worry about them, there's no reason to forbid anything it in either consensus or policy -- there's no need to worry about future options if it's established that the future options are definitely not needed. I don't believe that's been done yet, but it's starting to seem plausible to me. [EDIT: This isn't plausible.]

Sjors · 2025-04-01T08:46:16Z

Suggested PR title: Policy: reallow short transactions except 64-byte

And extra sentence for the description:

Replace lower bound rule MIN_STANDARD_TX_NONWITNESS_SIZE{65} with just an exception NONSTANDARD_TX_NONWITNESS_SIZE{64}. This makes policy more aligned with the proposed consensus change in the Great Consensus Cleanup, but even if that is never activated, this is the most narrow rule which addresses CVE-2017-12842.

As for the actual change, I'm not sure either way. But to expand on @ajtowns's comment:

the conservative thing to do in policy is to continue to forbid everything that's not obviously useful

And @glozow's finding:

I ran into "tx-size-small" when testing new policies with packages: a 1-in-1-out child spending the p2a of a parent can be less than 64 bytes.

It seems there's a potential footgun in wallets that can generate < 64 transactions, and then for whatever reason one byte gets added and the transaction is stuck. From that perspective it's safer to have a policy that simplify requires > 64 bytes.

So from the perspective of using policy as a way to encourage safe designs, the current approach is better.

On the flip side, someone may feel that the current policy is too "pedantic" and start running relays using the commit from this PR. But that applies to every single policy != consensus rule, and is probably only an (economic) issue for very common transactions.

DrahtBot · 2025-05-21T09:46:49Z

🐙 This pull request conflicts with the target branch and needs rebase.

TheCharlatan · 2025-05-22T11:37:22Z

Concept ACK

TheCharlatan

ACK 72ddf42

While I don't think the changes here are pressing, I'm for aligning policy more closely with the actual behaviour we'd like to prohibit and (eventually) enforce.

It seems there's a potential footgun in wallets that can generate < 64 transactions, and then for whatever reason one byte gets added and the transaction is stuck.

This seems a bit constructed. Why would a wallet correctly report a failed send for < 64, but fail to do so with 64 because of this rule change? Scenarios where transactions get stuck because of fees are far likelier in any case.

instagibbs mentioned this pull request Oct 26, 2022

POLICY: Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes #26265

Closed

instagibbs force-pushed the relax_too_small_tx_equality branch 3 times, most recently from 4824737 to a67341a Compare October 26, 2022 19:26

hernanmarino reviewed Oct 26, 2022

View reviewed changes

glozow added the TX fees and policy label Oct 26, 2022

instagibbs changed the title ~~Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 64 non-witness bytes exactly~~ Relax MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only Oct 26, 2022

maflcko reviewed Oct 26, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Oct 27, 2022

Use int32_t type for most transaction size/weight values #23962

Merged

rajarshimaitra reviewed Oct 27, 2022

View reviewed changes

maflcko approved these changes Oct 27, 2022

View reviewed changes

test/functional/data/invalid_txs.py Outdated Show resolved Hide resolved

instagibbs force-pushed the relax_too_small_tx_equality branch 3 times, most recently from c361621 to 2ddd3a8 Compare October 28, 2022 14:17

instagibbs requested review from rajarshimaitra and hernanmarino and removed request for rajarshimaitra and hernanmarino October 31, 2022 17:41

hernanmarino approved these changes Oct 31, 2022

View reviewed changes

theStack approved these changes Nov 9, 2022

View reviewed changes

glozow reviewed Nov 10, 2022

View reviewed changes

src/policy/policy.h Outdated Show resolved Hide resolved

instagibbs force-pushed the relax_too_small_tx_equality branch from 2ddd3a8 to a75b0fd Compare November 10, 2022 19:29

maflcko reviewed Nov 10, 2022

View reviewed changes

doc/release-notes-26398.md Outdated Show resolved Hide resolved

instagibbs force-pushed the relax_too_small_tx_equality branch from a873be7 to 72ddf42 Compare March 27, 2025 19:42

DrahtBot added the CI failed label Mar 27, 2025

instagibbs marked this pull request as ready for review March 27, 2025 19:51

DrahtBot removed Needs rebase CI failed labels Mar 27, 2025

DrahtBot mentioned this pull request Mar 28, 2025

psbt: add non-default sighash types to PSBTs and unify sighash type match checking #31622

Merged

darosior mentioned this pull request Mar 28, 2025

Policy: Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 rust-bitcoin/rust-bitcoin#4114

Merged

glozow reviewed Mar 28, 2025

View reviewed changes

src/wallet/spend.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 11, 2025

wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys #29675

Open

DrahtBot mentioned this pull request May 5, 2025

test: refactor: overhaul (w)txid determination for CTransaction objects #32421

Merged

DrahtBot mentioned this pull request May 15, 2025

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

Merged

DrahtBot added the Needs rebase label May 21, 2025

TheCharlatan approved these changes Jun 17, 2025

View reviewed changes

DrahtBot requested review from hernanmarino, rajarshimaitra, darosior, glozow, maflcko, theStack and ajtowns June 17, 2025 10:56

Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only #26398

Are you sure you want to change the base?

Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only #26398

Uh oh!

Conversation

instagibbs commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Oct 27, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darosior commented Oct 28, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 10, 2022

Uh oh!

instagibbs commented Nov 10, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Mar 27, 2025

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajtowns commented Mar 31, 2025

Uh oh!

instagibbs commented Mar 31, 2025

Uh oh!

darosior commented Mar 31, 2025

Uh oh!

ajtowns commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Apr 1, 2025

Uh oh!

DrahtBot commented May 21, 2025

Uh oh!

TheCharlatan commented May 22, 2025

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Oct 26, 2022 •

edited

Loading

DrahtBot commented Oct 26, 2022 •

edited

Loading

instagibbs commented Nov 10, 2022 •

edited

Loading

ajtowns commented Mar 31, 2025 •

edited

Loading