Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only

instagibbs commented at 5:35 pm on October 26, 2022: member

Follow-on to to #26265

Implements the logic not effected by Great Consensus Cleanup 2025 effort: https://github.com/bitcoin/bips/pull/1800

instagibbs force-pushed on Oct 26, 2022

hernanmarino commented at 7:45 pm on October 26, 2022: contributor

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.

in src/validation.cpp:700 in a67341a6af outdated

701-    // Transactions smaller than this are not relayed to mitigate CVE-2017-12842 by not relaying
702-    // 64-byte transactions.
703-    if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) < MIN_STANDARD_TX_NONWITNESS_SIZE)
704-        return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "tx-size-small");
705+    // Transactions exactly 64 non-witness bytes are not relayed to mitigate CVE-2017-12842.
706+    if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) == NONSTANDARD_TX_NONWITNESS_SIZE)

maflcko commented at 7:48 pm on October 26, 2022:

0    if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) == NONSTANDARD_TX_NONWITNESS_SIZE) {

nit for new code

instagibbs commented at 2:04 pm on October 28, 2022:

done

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

instagibbs renamed this:
~~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
on Oct 26, 2022

DrahtBot commented at 11:02 pm on October 26, 2022: contributor

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
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:

#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 commented at 2:45 pm on October 27, 2022: contributor

tACK a67341a6af3a65c7810a046a85a7eb1e114369b4

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 commented at 2:59 pm on October 27, 2022: member

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

in test/functional/data/invalid_txs.py:126 in a67341a6af outdated

124 
125     def get_tx(self):
126         tx = CTransaction()
127         tx.vin.append(self.valid_txin)
128-        tx.vout.append(CTxOut(0, CScript([OP_TRUE])))
129+        tx.vout.append(CTxOut(0, CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN - 1)))))

maflcko commented at 3:30 pm on October 27, 2022:

why not NONSTANDARD_OP_RETURN_SCRIPT?

instagibbs commented at 2:04 pm on October 28, 2022:

done

maflcko commented at 2:09 pm on October 28, 2022:

You’ll need to remove the unused imports now

maflcko approved

maflcko commented at 3:30 pm on October 27, 2022: member

lgtm

darosior commented at 10:14 am on October 28, 2022: member

Concept ACK, prefer this over #26265

instagibbs force-pushed on Oct 28, 2022

instagibbs requested review from rajarshimaitra on Oct 31, 2022

instagibbs removed review request from rajarshimaitra on Oct 31, 2022

instagibbs requested review from hernanmarino on Oct 31, 2022

instagibbs removed review request from hernanmarino on Oct 31, 2022

instagibbs requested review from rajarshimaitra on Oct 31, 2022

hernanmarino approved

theStack approved

theStack commented at 4:26 pm on November 9, 2022: contributor

Concept and code-review ACK 2ddd3a839a08f09c11e9a24c63c2d3a1cba235b8

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

 0b/test/functional/mempool_accept.py
 1index c2d94ddad..09f8c3b23 100755
 2--- a/test/functional/mempool_accept.py
 3+++ b/test/functional/mempool_accept.py
 4@@ -36,7 +36,6 @@ from test_framework.script_util import (
 5     NONSTANDARD_OP_RETURN_SCRIPT,
 6     NONSTANDARD_TX_NONWITNESS_SIZE,
 7     script_to_p2sh_script,
 8-    script_to_p2wsh_script,
 9 )
10 from test_framework.util import (
11     assert_equal,
12@@ -340,16 +339,13 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
13             maxfeerate=0,
14         )
15
16-        # Prep for tiny-tx tests with wsh(OP_TRUE) output
17-        seed_tx = self.wallet.send_to(from_node=node, scriptPubKey=script_to_p2wsh_script(CScript([OP_TRUE])), amount=COIN)
18+        # Prep for tiny-tx tests with MiniWallet anyone-can-spend output
19+        seed_utxo = self.wallet.send_self_transfer(from_node=node)["new_utxo"]
20         self.generate(node, 1)
21
22         self.log.info('A tiny transaction(in non-witness bytes) that is disallowed')
23-        tx = CTransaction()
24-        tx.vin.append(CTxIn(COutPoint(int(seed_tx[0], 16), seed_tx[1]), b"", SEQUENCE_FINAL))
25-        tx.wit.vtxinwit = [CTxInWitness()]
26-        tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
27-        tx.vout.append(CTxOut(0, NONSTANDARD_OP_RETURN_SCRIPT))
28+        tx = self.wallet.create_self_transfer(utxo_to_spend=seed_utxo)["tx"]
29+        tx.vout[0].scriptPubKey = NONSTANDARD_OP_RETURN_SCRIPT
30         # Note it's only non-witness size that matters!
31         assert_equal(len(tx.serialize_without_witness()), NONSTANDARD_TX_NONWITNESS_SIZE)
32         assert len(tx.serialize()) != NONSTANDARD_TX_NONWITNESS_SIZE
33@@ -361,7 +357,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
34         )
35
36         self.log.info('Just-below size transaction(in non-witness bytes) that is allowed')
37-        tx.vout[0] = CTxOut(COIN - 1000, CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN - 2))))
38+        tx.vout[0] = CTxOut(int(seed_utxo["value"] * COIN) - 1000, CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN - 2))))
39         assert_equal(len(tx.serialize_without_witness()), NONSTANDARD_TX_NONWITNESS_SIZE - 1)
40         self.check_mempool_result(
41             result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal('0.00001000')}}],
42@@ -370,7 +366,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
43         )
44
45         self.log.info('Just-above size transaction(in non-witness bytes) that is allowed')
46-        tx.vout[0] = CTxOut(COIN - 1000, CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN))))
47+        tx.vout[0] = CTxOut(int(seed_utxo["value"] * COIN) - 1000, CScript([OP_RETURN] + ([OP_0] * (INVALID_SPK_LEN))))
48         assert_equal(len(tx.serialize_without_witness()), NONSTANDARD_TX_NONWITNESS_SIZE + 1)
49         self.check_mempool_result(
50             result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal('0.00001000')}}],

maflcko commented at 10:16 am on November 10, 2022: member

review ACK 2ddd3a839a08f09c11e9a24c63c2d3a1cba235b8 🏊

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 2ddd3a839a08f09c11e9a24c63c2d3a1cba235b8 🏊
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUieVQv/RIMmNfx0FNL6Wk6XQNc/KquyZHRhr+3NSfMXIBwH02jyoc1lHbE6DJlB
 8unJMOHwggGGzJT8Ohzq83c3P2sU2sTP7KSGKywtge5S6+FBY9sZNvlSbY3Xg7NNW
 9tokt9W7VCUYDDvVZpKV+HKnOV+qL4ZHDBYcu33JUDXZnvgkD7nc36kN7Rf15unNT
10j2tVQ21XV0E/Ok3y1W+C3I6S0tDao6JdpwcHT3+ei8fCp7tAAbzSFpGhutWd6Wpg
11GRTwvsmr3ZM5UlDaYUHGh1oGsqQvHnWhKh6nNF/j1iKpTOEYiyRR40pPxifUF+wG
12FHc+b9GuKYLnNXcaarmxOmtsHh1SFtjjEBotTw9mRYdSlkTFN6vboTppCtyqefhg
13XxcmzBSnuKt0AA3l/7sZIBTWqxcf4N0kqOjzMIFiDaJJklqwn/vvUdxzYYXpoj6T
14jP4EzBc5wFUgPLZCdLCQ56dtjZTeHjNOWktdAr+0uFcpvxvNOMQeLpDDNhhf/ukK
15A+FIuasE
16=cyTE
17-----END PGP SIGNATURE-----

For historical context, see #26265#pullrequestreview-1132684424

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 commented at 1:18 pm on November 10, 2022: member

@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

in src/policy/policy.h:36 in 2ddd3a839a outdated

24@@ -25,8 +25,8 @@ static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT - 4000};
25 static constexpr unsigned int DEFAULT_BLOCK_MIN_TX_FEE{1000};
26 /** The maximum weight for transactions we're willing to relay/mine */
27 static constexpr unsigned int MAX_STANDARD_TX_WEIGHT{400000};
28-/** The minimum non-witness size for transactions we're willing to relay/mine (1 segwit input + 1 P2WPKH output = 82 bytes) */
29-static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{82};
30+/** The non-witness size for transactions we're unwilling to relay/mine: CVE-2017-12842 */
31+static constexpr unsigned int NONSTANDARD_TX_NONWITNESS_SIZE{64};

glozow commented at 7:09 pm on November 10, 2022:

micro-nit (since technically there are many nonstandard sizes)

0/** In addition to transactions that are too big, do not relay/mine ones that are 64 non-witness bytes exactly: CVE-2017-12842 */
1static constexpr unsigned int NONSTANDARD_TX_NONWITNESS_SIZE{64};

instagibbs commented at 7:29 pm on November 10, 2022:

taken, thanks

glozow commented at 7:13 pm on November 10, 2022: member

code review ACK 2ddd3a839a08f09c11e9a24c63c2d3a1cba235b8

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

instagibbs force-pushed on Nov 10, 2022

instagibbs commented at 7:39 pm on November 10, 2022: member

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

in doc/release-notes-26398.md:6 in 331788ef23 outdated

0@@ -0,0 +1,6 @@
1+P2P and network changes
2+---------
3+
4+- Transactions of non-witness size below 82, aside from 64, are now allowed by mempool
5+  and relay policy. This is to better reflect the actual afforded protections
6+  against CVE-2017-12842 and open up additional use-cases of smaller transaction sizes.

maflcko commented at 7:40 pm on November 10, 2022:

0  against CVE-2017-12842 and open up additional use-cases of smaller transaction sizes. (#26398)

nit

instagibbs force-pushed on Nov 10, 2022

instagibbs renamed this:
~~Relax MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only~~
Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only
on Nov 10, 2022

theStack approved

theStack commented at 4:47 pm on November 11, 2022: contributor

re-ACK c248935ff1774517d523acd622b04b2f89f344ba 📔

darosior approved

darosior commented at 6:19 pm on November 16, 2022: member

utACK c248935ff1774517d523acd622b04b2f89f344ba

rajarshimaitra approved

rajarshimaitra commented at 2:10 pm on November 17, 2022: contributor

ReACK c248935ff1774517d523acd622b04b2f89f344ba

fanquake requested review from ajtowns on Nov 17, 2022

glozow commented at 8:09 pm on November 17, 2022: member

re ACK c248935ff1774517d523acd622b04b2f89f344ba

hernanmarino approved

in test/functional/mempool_accept.py:380 in c248935ff1 outdated

360+            result_expected=[{'txid': tx.rehash(), 'allowed': True, 'vsize': tx.get_vsize(), 'fees': { 'base': Decimal('0.00001000')}}],
361+            rawtxs=[tx.serialize().hex()],
362+            maxfeerate=0,
363+        )
364+
365+        self.log.info('Just-above size transaction(in non-witness bytes) that is allowed')

maflcko commented at 9:42 am on November 18, 2022:

0        self.log.info('Just-above size transaction (in non-witness bytes) that is allowed')

nit (don’t address unless there are other reasons)

maflcko approved

maflcko commented at 9:42 am on November 18, 2022: member

re-ACK c248935ff1774517d523acd622b04b2f89f344ba 🔂

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK c248935ff1774517d523acd622b04b2f89f344ba  🔂
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUga0wv/dgeeqerB07SV45VcbTQHnx3h4jSd/stPjmAeGIVrdlZM7qUR8hWuCkZa
 86POSO3Din/scSPyczaeju8FjbdCbIoA/TPJwwBmMRulz7lGFu8VzeGDLKBzmKtr8
 9ZypWUKba48vdQ9NDLFd2A/bcjWYKjqgJvOCYBmcQVqVfwvbKOPcgJw8XeZCY/8Ut
1021HFlklTdsruvVxptzyIh5zkX3Ni60YI9OkMt2N2w7qTWbBI9cjFspHIRG72z83/
11Wc2CCpV9weFnSTQ8BeK2nbalSVOEhz8JQFQfUhamKHNX/2AQmqVO51Ix/+PvV93K
12LimjCQEpGNlsm5looiNLJQBYF32yUF65aphEvAJeSCPW0VNyGcUfTPEvUQIFztrc
13f5KqRjFAf8u7ZhpmeB/peQY8cnhgFpooV9UH9/78SZarNgrF3G4qUivkjKFXcG8K
14P7RcnyDQxsUvFJxyOu5ArPhvy5stKBwl/dTUSuxCKEob0R/B6wYkTLXRtpD7xGHb
15/tyJW6gC
16=CXbv
17-----END PGP SIGNATURE-----

ajtowns commented at 8:16 pm on November 18, 2022: contributor

We’ve previously had a proposal to make txs that are smaller than 65 bytes consensus invalid; if we instead make txs that are between 61-64 bytes standard/relayable, it would make reviving that proposal as-is substantially more difficult. I don’t think there’s any substantial benefit to saving 4vbytes when you’re spending all the funds to fees anyway, and if there are multiple all-to-fees txs in a block, then it’s already more sensible to use ANYONECANPAY|ALL or ANYONECANPAY|NONE signatures so that they can be aggregated, limiting the total cost of disallowing 60-64 byte txs to 4 vbytes per block.

It’s easier to relax policy safely than it is to make it more strict – relaxing policy doesn’t break existing applications/pre-signed txs, while making policy more strict can. That means that it’s easy to go from “>= 65 bytes is okay” to “!= 64 bytes is okay”. But the reverse isn’t true; if this approach is harmful, it’s much harder to correct it to a more restrictive approach.

EDIT to add: Approach NACK.

instagibbs commented at 8:32 pm on November 18, 2022: member

It’s easier to relax policy safely than it is to make it more strict – relaxing policy doesn’t break existing applications/pre-signed txs, while making policy more strict can. That means that it’s easy to go from “>= 65 bytes is okay” to “!= 64 bytes is okay”. But the reverse isn’t true; if this approach is harmful, it’s much harder to correct it to a more restrictive approach.

This is the strongest argument against this in my view, but it applies similarly, to perhaps a lesser degree, to relaxing it at all. If we’re worried that it’s going to cause issues, maybe we just take the 22 byte padding hit(or whatever the constant is) or do batching if possible.

Or this proposal can just sit around for another couple release cycles to gather directed feedback from others including the author of https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-March/016714.html ?

f.e., unless there is strong argument against it, I’d rather re-litigate the Great Consensus Cleanup to switch to this check instead, or learn why this is wrongheaded

TheBlueMatt commented at 8:05 pm on November 25, 2022: contributor

I don’t think there’s a super strong reason to restrict < 64 byte transactions vs exactly 64 byte transactions. It is a substantially simpler check to test for < 64 and it seems to remove some footguns (oops, added an extra byte and now my tx is invalid), but of course if you’re doing anything approaching 64 bytes you’re burning to fee (or OP_TRUE, but ideally don’t do that). I think at the time we didn’t really have any evidence that people wanted this kinda thing often enough that we care about it saving the 3 bytes, and obviously anything that is burning output(s) to fee really should be aggregable (eg lightning anchor outputs are, I believe). If there’s a clear use-case where it really is required that the outputs be non-aggregable, then I’d be fine with this, but ISTM just making things aggregable is a much, much better outcome.

instagibbs commented at 3:35 pm on November 28, 2022: member

It is a substantially simpler check to test for < 64

At least at the policy layer I don’t see how that’s true.

I think at the time we didn’t really have any evidence that people wanted this kinda thing often enough that we care about it saving the 3 bytes, and obviously anything that is burning output(s) to fee really should be aggregable (eg lightning anchor outputs are, I believe). If there’s a clear use-case where it really is required that the outputs be non-aggregable, then I’d be fine with this, but ISTM just making things aggregable is a much, much better outcome.

Pinning with today’s policies is a bit thorny, but arguably worse in batched setting. With the current thinking of V3-style setups, those are going to be non-aggregatable simply due to pinning vectors, but that’s not a useful observation for today’s network, and perhaps the check can be further relaxed later with a V3 policy upgrade. So this could be two-staged as more information comes in about future policy changes. Maybe batched bumps become safer in the future with more work.

So to recap, the core debate here is that there are very few known use-cases for a further relaxation and chance for wallet authors to not understand the 64 byte exception, weighed against (imo) the slight degradation in code clarity and documentation. The policy could be further relaxed later in tandem with other updates to the network.

This sound correct?

darosior commented at 3:40 pm on November 28, 2022: member

So to recap, the core debate here is that there are very few known use-cases for a further relaxation and chance for wallet authors to not understand the 64 byte exception, weighed against (imo) the slight degradation in code clarity and documentation

Sounds like it would be as confusing to a wallet developer if policy ruled out <64 bytes transactions while only 64-bytes transactions are causing an issue. So i don’t think it’s really an argument for being stricter.

jonatack commented at 8:07 pm on December 8, 2022: member

Tend to agree with #26398#pullrequestreview-1186725701 and prefer #26265.

instagibbs commented at 8:52 pm on December 8, 2022: member

Based on the balance of the discussion with light(?) preferences on each side and my belief we’re not going to uncover any critical knowledge continuing, I think this is ready to be considered for merge or close as appropriate.

edit: people can of course continue discussing, I’ve just exhausted my arguments :)

achow101 commented at 8:52 pm on December 8, 2022: member

We’ve previously had a proposal to make txs that are smaller than 65 bytes consensus invalid; if we instead make txs that are between 61-64 bytes standard/relayable, it would make reviving that proposal as-is substantially more difficult.

I don’t think it would. The proposal could be just as easily changed to match this standardness policy. It doesn’t look like there’s any safety reason to disallow <64 byte txs in that proposal either.

I don’t think there’s any substantial benefit to saving 4vbytes when you’re spending all the funds to fees anyway, and if there are multiple all-to-fees txs in a block, then it’s already more sensible to use ANYONECANPAY|ALL or ANYONECANPAY|NONE signatures so that they can be aggregated, limiting the total cost of disallowing 60-64 byte txs to 4 vbytes per block.

I could foresee a situation where users would want to prefer ANYONECANPAY|ALL in order to ensure that their funds only go to fees. For that to work with aggregation, everyone would have to sign a tx with the same output. A single OP_RETURN seems like it would be the obvious output script to settle on. If a user had a single native segwit UTXO they wanted to burn in this way, they would need to be able to create a <64 byte tx to be broadcast and relayed so that someone else (e.g. the miner) can do the aggregation.

glozow requested review from sdaftuar on Dec 9, 2022

ajtowns commented at 4:59 pm on December 9, 2022: contributor

We’ve previously had a proposal to make txs that are smaller than 65 bytes consensus invalid; if we instead make txs that are between 61-64 bytes standard/relayable, it would make reviving that proposal as-is substantially more difficult.

I don’t think it would. The proposal could be just as easily changed […]

Yes, you could change the proposal, but that’s the point – this relay policy change would prevent us from deploying the original proposal and would force us to change it. The conservative approach to making a consensus change is to minimise the things that you limit; the conservative approach to making a relay policy change is to minimise the things that you allow.

I could foresee a situation where users would want to prefer ANYONECANPAY|ALL in order to ensure that their funds only go to fees. For that to work with aggregation, everyone would have to sign a tx with the same output. A single OP_RETURN seems like it would be the obvious output script to settle on.

A scriptPubKey of 6a03464545 (OP_RETURN "FEE") would be equally straightforward to settle on, and would generate a 65 byte tx, for a single input and single output, no scriptSig, and after the witness was stripped. Unlike a plain OP_RETURN, it’s also something you can actually generate today using createrawtransaction or createpsbt.

I could see an argument that it would be better to add a targeted exception to only allow txs to be under 85 bytes after stripping the witness if they have a single output that spends to exactly that scriptPubKey (and, perhaps, where all the tx’s signatures are ANYONECANPAY). That way any txs that are trying to burn to fees are strongly encouraged to be able to be aggregated (and aggregating them could perhaps eventually be automated as part of getblocktemplate or atmp). @instagibbs

This is the strongest argument against this in my view, but it applies similarly, to perhaps a lesser degree, to relaxing it at all.

There’s a clear argument why relaxing it at all is useful: for burning a keypath spend to fees, it saves about 17% of the transaction size; and further I don’t think anyone’s raised any concerns about that being problematic. (To be fair, I’ve only seen it hypothesised that that’s a useful thing to do, not any protocols where people are actually burning entire outputs to fees, so perhaps it would be better to leave this unmerged until there is a demonstrated use case. Particularly since this is only making things more efficient, not introducing new functionality)

The same isn’t true of allowing 60-63 bytes as well as 65+ bytes. That only saves an additional 4%, and does introduce weird special cases that have been objected to. Maybe those objections can be overcome, but just ignoring them with “I have a light preference for doing it the other way” isn’t a remotely conservative approach to development, especially in areas that impact the possibilities for future consensus changes.

instagibbs commented at 5:23 pm on December 9, 2022: member

There’s a clear argument why relaxing it at all is useful

To be clear, I was talking about security argument, not arguing about utility. The danger of changing status quo doesn’t dissipate by a half measure. In absence of a security issue, I in general don’t find arguments about utility interesting to make a decision.

sipa commented at 8:17 pm on December 9, 2022: member

I have a very slight preference for this approach compared to the less-than-64-bytes approach, but really am fine with either.

My reasoning is that it’s just closer to what we actually want to protect against, and I don’t see technical reasons why lengths 61-63 are in any way worse than say length 65-67 from a network perspective. Almost all policy-relevant questions would involve vsize/weight, not non-witness size; this is a particular exception to that, for a very specific reason. That reason doesn’t apply to any lengths except 64.

I see the argument about it hurting the prospects of the existing old proposal to consensus-outlaw <64, but I’m not sure why that would matter that much. Arguably, for consensus the preference should be outlawing as little as possible, as reverting things there is orders of magnitude harder than doing the same for policy.

sdaftuar commented at 8:18 pm on December 9, 2022: member

I think I largely agree with @ajtowns’ arguments, but I don’t have a strong view.

It seems to me that the essential question is whether requiring transactions to be >64 bytes is a simpler/more logical consensus rule than merely requiring transactions not be exactly 64 bytes. It seems to me like there’s no clear agreement yet over which is better, and I’m not sure it’s worth the effort to try to reach a consensus on this either, given that if this were ever championed as a consensus rule we’d need to socialize the idea amongst many more people than those reviewing these PRs. So given that, I think the most reasonable thing to do would be to set the policy to >64 for now, since that is most compatible with either choice in the future (ie we can always relax it further).

I could see an argument that it would be better to add a targeted exception to only allow txs to be under 85 bytes after stripping the witness if they have a single output that spends to exactly that scriptPubKey (and, perhaps, where all the tx’s signatures are ANYONECANPAY). That way any txs that are trying to burn to fees are strongly encouraged to be able to be aggregated (and aggregating them could perhaps eventually be automated as part of getblocktemplate or atmp).

This seems like a pretty good idea to me, too. I don’t think there’s a compelling reason for transactions under 82 bytes (ignoring witness) to be permitted if they are not for this use case, is there? In general I think it’s better to relax these kinds of things in a targeted way to make sure we don’t inadvertently introduce DoS vectors or other unadvisable network behavior.

instagibbs marked this as a draft on Dec 12, 2022

instagibbs commented at 3:17 pm on December 12, 2022: member

I could see an argument that it would be better to add a targeted exception to only allow txs to be under 85 bytes after stripping the witness if they have a single output that spends to exactly that scriptPubKey (and, perhaps, where all the tx’s signatures are ANYONECANPAY). That way any txs that are trying to burn to fees are strongly encouraged to be able to be aggregated (and aggregating them could perhaps eventually be automated as part of getblocktemplate or atmp).

I believe this would be a further step backwards vs the status quo today when it comes to code legibility; improving the legibility was part of the intended effect of my PR.

Seeing as this PR is controversial enough and I believe #26265 (comment) is an improvement on status quo, I am marking this one as Draft to remove eyes in favor of the other PR.

DrahtBot added the label Needs rebase on Dec 21, 2022

jonatack commented at 7:42 pm on December 21, 2022: member

Maybe close here, as #26265 was merged.

achow101 commented at 9:18 pm on December 21, 2022: member

Maybe close here, as #26265 was merged.

I don’t think #26265 precludes this PR being merged at some point in the future.

instagibbs commented at 1:26 am on March 21, 2023: member

Closing for now. We can revisit this at some later date.

instagibbs closed this on Mar 21, 2023

instagibbs commented at 4:48 pm on July 27, 2023: member

https://github.com/lightning/bolts/pull/1096#discussion_r1276546764

Just noting that some LN spec work is touching these limits

bitcoin locked this on Jul 26, 2024

glozow commented at 6:09 pm on March 27, 2025: member

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.

Additionally, if we want to mirror the Great Consensus Cleanup, it now disallows ==64 instead of <=64 (see https://github.com/bitcoin/bips/pull/1800). So the “We’ve previously had a proposal to make txs that are smaller than 65 bytes consensus invalid; if we instead make txs that are between 61-64 bytes standard/relayable, it would make reviving that proposal as-is substantially more difficult.” concern here is resolved now.

bitcoin unlocked this on Mar 27, 2025

instagibbs commented at 6:12 pm on March 27, 2025: member

I can rebase this, or @darosior can if he likes. Let’s just not duplicate work.

darosior commented at 6:12 pm on March 27, 2025: member

I agree. I’ll rebase this and open a new PR unless @instagibbs beats me to it here.

darosior commented at 6:19 pm on March 27, 2025: member

If the next block’s (889707) hash represents an even uint256 i’ll do it, if it’s odd you do.

instagibbs reopened this on Mar 27, 2025

instagibbs force-pushed on Mar 27, 2025

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

72ddf4237b

instagibbs force-pushed on Mar 27, 2025

DrahtBot added the label CI failed on Mar 27, 2025

instagibbs marked this as ready for review on Mar 27, 2025

instagibbs commented at 7:53 pm on March 27, 2025: member

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.

DrahtBot removed the label Needs rebase on Mar 27, 2025

DrahtBot removed the label CI failed on Mar 27, 2025

TheBlueMatt referenced this in commit 2fe3687899 on Mar 28, 2025

in src/wallet/spend.cpp:1031 in 72ddf4237b

1026@@ -1027,7 +1027,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1027     coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
1028     coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs;
1029     coin_selection_params.m_max_tx_weight = coin_control.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT);
1030-    int minimum_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR;
1031+    int minimum_tx_weight = (NONSTANDARD_TX_NONWITNESS_SIZE + 1) * WITNESS_SCALE_FACTOR;
1032     if (coin_selection_params.m_max_tx_weight.value() < minimum_tx_weight || coin_selection_params.m_max_tx_weight.value() > MAX_STANDARD_TX_WEIGHT) {

glozow commented at 3:27 pm on March 28, 2025:

This looks incorrect, it’s still using <=64

darosior commented at 3:46 pm on March 28, 2025:

This looks correct to me? It preserves existing behaviour, i don’t think the wallet needs to be updated to support 61 62 and 63 bytes transactions in this PR?

instagibbs commented at 3:48 pm on March 28, 2025:

I intended to maintain current behavior, which I think I’ve done?

glozow commented at 6:18 pm on March 28, 2025:

Ah I see!

glozow commented at 3:28 pm on March 28, 2025: member

Thanks for rebasing! Needs release note.

ajtowns commented at 11:04 am on March 31, 2025: contributor

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 commented at 12:27 pm on March 31, 2025: member

@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 commented at 4:43 pm on March 31, 2025: member

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 commented at 5:47 pm on March 31, 2025: contributor

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.

Sjors commented at 8:46 am on April 1, 2025: member

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.

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

Code Coverage & Benchmarks

Reviews

Conflicts