Follow-on to to #26265
Implements the logic not effected by Great Consensus Cleanup 2025 effort: https://github.com/bitcoin/bips/pull/1800
Follow-on to to #26265
Implements the logic not effected by Great Consensus Cleanup 2025 effort: https://github.com/bitcoin/bips/pull/1800
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)
0 if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) == NONSTANDARD_TX_NONWITNESS_SIZE) {
nit for new code
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26398.
See the guideline for information on the review process.
Type | Reviewers |
---|---|
Concept ACK | hernanmarino |
Approach NACK | ajtowns |
Stale ACK | theStack, darosior, 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.
Reviewers, this pull request conflicts with the following ones:
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.
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..
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)))))
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')}}],
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)
@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
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};
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};
code review ACK 2ddd3a839a08f09c11e9a24c63c2d3a1cba235b8
Could you please link the ML post in the OP and write a release note?
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.
0 against CVE-2017-12842 and open up additional use-cases of smaller transaction sizes. (#26398)
nit
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')
0 self.log.info('Just-above size transaction (in non-witness bytes) that is allowed')
nit (don’t address unless there are other reasons)
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-----
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.
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
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?
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.
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 :)
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.
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.
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.
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.
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.
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.
https://github.com/lightning/bolts/pull/1096#discussion_r1276546764
Just noting that some LN spec work is touching these limits
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.
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
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) {