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

instagibbs · 2022-10-05T19:07:28Z

Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled.

There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical.

Two changes could be accomplished:

Anything not 64 bytes could be allowed
Anything above 64 bytes could be allowed

In the Great Consensus Cleanup, suggestion (2)
was proposed as a consensus change, and is the simpler of the two suggestions. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5.

The functional test is also modified to test the actual case
we care about: 64 bytes

Related mailing list discussions here:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html
And a couple years earlier:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html

glozow

Concept ACK

originally introduced in 7485488 as part of #11423
documented as CVE-2017-12842
actual explanation added to code in #16885
proposed as a consensus change in https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-March/016714.html / #15482

maflcko · 2022-10-06T13:31:43Z

Maybe add a test that 65 bytes is passing now?

instagibbs · 2022-10-06T14:17:01Z

@MarcoFalke Pushed a test of min size working via testmempoolaccept, and also updated another constants

ajtowns · 2022-10-07T01:03:54Z

test/functional/test_framework/script_util.py

+DUMMY_MIN_OP_RETURN_SCRIPT = CScript([OP_RETURN] * MIN_PADDING)
+
+# This can be spent, one shorter due to <push_MIN_PADDING> opcode
+DUMMY_MIN_SPEND_SCRIPT = CScript([b'a' * (MIN_PADDING - 1)])


Wouldn't [RETURN FALSE FALSE FALSE FALSE] be a better min op_return script, since that way it passes standardness rules?

For a spendable (but necessarily non-standard) output, [NOP NOP NOP NOP TRUE] might be more explicit? But I don't think that case ever makes sense -- if it's a "dust" amount, that you don't care about, it's better to send it to fees and use a 0-value OP_RETURN output; if it's not a dust amount and you do care about it, you'll want to secure the funds and use something that requires an output to redeem. Saving a few bytes in the bip68 functional test doesn't seem worth the effort?

assert len(*_MIN_*_SCRIPT) == MIN_PADDING might be clearer than explaining all the off-by-one things?

Wouldn't [RETURN FALSE FALSE FALSE FALSE] be a better min op_return script, since that way it passes standardness rules?

done

For a spendable

Leaving for another PR

assert len(MIN_SCRIPT) == MIN_PADDING might be clearer than explaining all the off-by-one things?

Done

ajtowns · 2022-10-07T01:10:53Z

test/functional/test_framework/script_util.py

-DUMMY_P2WPKH_SCRIPT = CScript([b'a' * 21])
-DUMMY_2_P2WPKH_SCRIPT = CScript([b'b' * 21])
-
+# least 5 bytes. DUMMY_P2WPKH_SCRIPT should be used when requiring


You replaced DUMMY_P2WPKH_SCRIPT with DUMMY_MIN_SPEND_SCRIPT but didn't update this comment

~~But I think that doesn't make sense? The former was standard, the latter requires you to use -acceptnonstdtxn=1, which seems likely to hide bugs?~~

Both are nonstandard (Edit: I guess no longer, now that the new dummy switched to OP_RETURN).

My goal was to get rid of this dummy completely by using standard scripts and/or MiniWallet, but I never got around to do it for the last remaining use here.

Oh, it's "p2wpkh" only in the sense that it's the same size... Weird.

These dummy constants are now being removed in #26277 along with acceptnonstdtxn=1 usage.

ajtowns · 2022-10-07T01:17:49Z

test/functional/data/invalid_txs.py

@@ -122,7 +123,8 @@ class SizeTooSmall(BadTxTemplate):
    def get_tx(self):
        tx = CTransaction()
        tx.vin.append(self.valid_txin)
-        tx.vout.append(CTxOut(0, CScript([OP_TRUE])))
+        tx.vout.append(CTxOut(0, CScript([OP_RETURN] * (MIN_PADDING - 1))))


Wouldn't it be better to create an output that passes standardness checks here so that it's only failing for one reason?

Sorry what's non-standard about this output? It's an OP_RETURN(so it can be dust), all pushes, under 80 bytes...

OP_RETURN is probably not a push?

decodescript 6a6a6a6a6a6a { "asm": "OP_RETURN OP_RETURN OP_RETURN OP_RETURN OP_RETURN OP_RETURN", "desc": "raw(6a6a6a6a6a6a)#6j3qx6z8", "type": "nonstandard" }

Uhm, OP_RETURN isn't a push, gibbs... facepalm

could add OP_RESERVED to do a bit of trolling

test/functional/data/invalid_txs.py

darosior · 2022-10-07T10:38:23Z

Concept ACK. I think this needs a ML post.

DrahtBot · 2022-10-07T12:25:09Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, achow101, pablomartin4btc, jonatack
Concept ACK	darosior, hernanmarino, rajarshimaitra
Stale ACK	ajtowns, MarcoFalke

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:

#26398 (Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only by instagibbs)
#23962 (Use int32_t type for most transaction size/weight values by hebasto)

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.

instagibbs · 2022-10-07T13:22:04Z

@darosior if I don't get any serious pushback through next week I can put forward a post

instagibbs · 2022-10-07T14:04:54Z

Pushed updates and a fix so the passing case is actually running, and test will fail if it doesn't run.

test/functional/p2p_invalid_tx.py

maflcko

left a test nit

test/functional/mempool_accept.py

maflcko

lgtm

ajtowns

ACK e5adc1a

Checked that the invalid_txs test triggers if the tx is accepted (by changing the tx to be 65 bytes). Also noticed that the mempool_accept tx is greater than 65 vbytes (two flag bytes, a byte for the witness stack size, a byte for the witness length, and a byte for the OP_TRUE in the witness is 5 bytes total, divided by 4 gives 1.25 extra vbytes, for a total of 65.25 vbytes), so this is also testing that additional witness bytes doesn't actually help.

Anything not 64 bytes could be allowed

I believe the minimum possible transaction is 60 bytes: 4B version, 4B locktime, 2B for in and count of 1 each (0 is illegal), 32B+4B for utxo txid and vout, 1B for scriptSig length of 0, 4B for nSequence, 8B for output amount, 1B for output scriptPubKey length of 0, and thus the minimum possible tx that only generates provably unspendable outputs is 61 bytes. Presuming there's a 64B signature in the witness (66B total to include the count of witness elements and the size of the signature), padding to from 61B to 65B is an increase of ~4% in raw data, and ~5.2% in weight/vbytes, and if you're trying to pay fees to get some meaningful transaction mined, then adding that other transaction to the denominator makes the increase even smaller. That seems pretty negligible to me, and "60-63 is okay but 64 isn't" seems like a horrible special case.

(The smallest transaction you can easily create with bitcoind seems to be 63B since createrawtransaction and createpsbt require non-empty data for the OP_RETURN, which adds a byte for the push data opcode, and a byte for the data being pushed)

test/functional/mempool_accept.py

instagibbs · 2022-10-11T12:35:08Z

Emailing the dev list now that this has a couple ACKs and no showstopping discussion

instagibbs · 2022-10-26T18:16:19Z

Slightly prefer #26398

Unless there is a compelling safety reason, and the code/tests don't become more complex, I'll leave the use-case designing to future developers who have more context/requirements than we do.

The only additional danger with #26398 is that someone writes support for transactions up to 64 non-witness bytes and doesn't test at the 64 byte marker, causing their tx to not propagate.

Any other dangers I'm missing?

maflcko · 2022-10-26T20:46:50Z

I've created a 60 byte transaction on testnet, since one didn't yet seem to exist https://mempool.space/testnet/tx/b0b076e6647711d8b80439a6ab6ed37764ec48153527629ff3033cc4d8c49751

rajarshimaitra · 2022-10-27T14:48:24Z

Concept ACK.. Preferring #26398 over this..

instagibbs · 2022-10-28T13:53:26Z

closing in favor of #26398

maflcko · 2022-12-12T09:55:36Z

review ACK b2c900e 🗝

Show signature

Signature:

instagibbs · 2022-12-12T15:18:42Z

Resurrecting this PR as #26398 appears to be too controversial. I still prefer the other solution, but this PR is superior to the status quo in legibility and usability, so I'll take it.

glozow · 2022-12-12T15:33:47Z

The policy could first relax to >64, then further to !=64 in the future, so the door on #26398 would not be completely closed. It seems that people are open to that possibility.

instagibbs · 2022-12-12T15:35:26Z

Yes leaving it in draft as I'm willing to take it up again if/when there is consensus. Thank you for your patience with my flapping PRs!

achow101 · 2022-12-12T17:42:51Z

ACK b2c900e

glozow

ACK b2c900e, with a few nits

Could you please also:

Link the ML post in the OP
Add a release note (something similar to the one in #26398)

test/functional/mempool_accept.py

Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled. There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical. Two changes could be accomplished: 1) Anything not 64 bytes could be allowed 2) Anything above 64 bytes could be allowed In the Great Consensus Cleanup, suggestion (2) was the route taken. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5. The functional test is also modified to test the actual case we care about: 64 bytes

instagibbs · 2022-12-19T15:04:54Z

Done all as @glozow has suggested, and touched up commit messages a bit more

glozow

reACK b2aa9e8

Changes were

commit message note about Great Consensus Cleanup doing <=64
assertion that size with witness is greater than 64
comment typo fix
release note

CI failure looks unrelated

achow101 · 2022-12-20T19:14:51Z

reACK b2aa9e8

pablomartin4btc

re-ACK b2aa9e8

jonatack

ACK b2aa9e8 with some suggestions

jonatack · 2022-12-20T20:32:03Z

src/policy/policy.h

@@ -25,8 +25,8 @@ static constexpr unsigned int DEFAULT_BLOCK_MAX_WEIGHT{MAX_BLOCK_WEIGHT - 4000};
 static constexpr unsigned int DEFAULT_BLOCK_MIN_TX_FEE{1000};
 /** The maximum weight for transactions we're willing to relay/mine */
 static constexpr unsigned int MAX_STANDARD_TX_WEIGHT{400000};
-/** The minimum non-witness size for transactions we're willing to relay/mine (1 segwit input + 1 P2WPKH output = 82 bytes) */
-static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{82};
+/** The minimum non-witness size for transactions we're willing to relay/mine: one larger than 64  */


It would be clearer to keep the unit.

Suggested change

/** The minimum non-witness size for transactions we're willing to relay/mine: one larger than 64 */

/** The minimum non-witness size for transactions we're willing to relay/mine: one larger than 64 bytes */

will add if I ever touch the PR

jonatack · 2022-12-20T20:42:46Z

test/functional/data/invalid_txs.py

-        tx.vout.append(CTxOut(0, CScript([OP_TRUE])))
+        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


This check looks redundant with the one added in https://github.com/bitcoin/bitcoin/pull/26265/files#diff-a99d72c0ed66c256169e92327e04ab223e71f5ef598e14aac0ff44dc2a1194daR356, unless it's for documentation purposes.

documentation, thanks for double checking

jonatack · 2022-12-20T20:47:03Z

doc/release-notes-26265.md

+P2P and network changes
+---------
+
+- Transactions of non-witness size 65 and above are now allowed by mempool


Suggest adding the unit and perhaps also stating what the previous value was to clarify the change.

Suggested change

- Transactions of non-witness size 65 and above are now allowed by mempool

- Transactions of non-witness size 65 bytes and above are now allowed by mempool

will add if I ever touch the PR

…_SIZE to 65 non-witness bytes b2aa9e8 Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation (Greg Sanders) 8c5b364 Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes (Greg Sanders) Pull request description: Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled. There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical. Two changes could be accomplished: 1) Anything not 64 bytes could be allowed 2) Anything above 64 bytes could be allowed In the Great Consensus Cleanup, suggestion (2) was proposed as a consensus change, and is the simpler of the two suggestions. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5. The functional test is also modified to test the actual case we care about: 64 bytes Related mailing list discussions here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html And a couple years earlier: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html ACKs for top commit: achow101: reACK b2aa9e8 glozow: reACK b2aa9e8 pablomartin4btc: re-ACK bitcoin/bitcoin@b2aa9e8 jonatack: ACK b2aa9e8 with some suggestions Tree-SHA512: c1ec1af9ddcf31b2272209a4f1ee0c5607399f8172e5a1dfd4604cf98bfb933810dd9369a5917ad122add003327c9fcf6ee26995de3aca41d5c42dba527991ad

jonatack · 2022-12-21T18:28:07Z

Is this merged and should be closed? Edit: yes.

instagibbs · 2022-12-21T18:28:55Z

Looks like it. @achow101 ?

… 65 non-witness bytes b2aa9e8 Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation (Greg Sanders) 8c5b364 Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes (Greg Sanders) Pull request description: Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled. There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical. Two changes could be accomplished: 1) Anything not 64 bytes could be allowed 2) Anything above 64 bytes could be allowed In the Great Consensus Cleanup, suggestion (2) was proposed as a consensus change, and is the simpler of the two suggestions. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5. The functional test is also modified to test the actual case we care about: 64 bytes Related mailing list discussions here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html And a couple years earlier: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html ACKs for top commit: achow101: reACK b2aa9e8 glozow: reACK b2aa9e8 pablomartin4btc: re-ACK bitcoin@b2aa9e8 jonatack: ACK b2aa9e8 with some suggestions Tree-SHA512: c1ec1af9ddcf31b2272209a4f1ee0c5607399f8172e5a1dfd4604cf98bfb933810dd9369a5917ad122add003327c9fcf6ee26995de3aca41d5c42dba527991ad

instagibbs force-pushed the relax_too_small_tx branch from 42cff87 to d034748 Compare October 5, 2022 19:15

DrahtBot added the TX fees and policy label Oct 5, 2022

glozow reviewed Oct 6, 2022

View reviewed changes

instagibbs force-pushed the relax_too_small_tx branch 3 times, most recently from bcbd1d9 to 7633656 Compare October 6, 2022 14:54

ajtowns reviewed Oct 7, 2022

View reviewed changes

DrahtBot mentioned this pull request Oct 7, 2022

test: Remove confusing DUMMY_P2WPKH_SCRIPT #26277

Merged

instagibbs force-pushed the relax_too_small_tx branch 2 times, most recently from 01d3ae5 to 7f6dee6 Compare October 7, 2022 14:02

instagibbs force-pushed the relax_too_small_tx branch from 7f6dee6 to 861a276 Compare October 7, 2022 14:29

maflcko reviewed Oct 7, 2022

View reviewed changes

test/functional/p2p_invalid_tx.py Outdated Show resolved Hide resolved

instagibbs force-pushed the relax_too_small_tx branch 2 times, most recently from f2922cc to 8b32fc4 Compare October 7, 2022 16:05

maflcko reviewed Oct 10, 2022

View reviewed changes

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Oct 10, 2022

instagibbs force-pushed the relax_too_small_tx branch from 8b32fc4 to 72f1da4 Compare October 10, 2022 20:45

DrahtBot removed the Needs rebase label Oct 10, 2022

instagibbs force-pushed the relax_too_small_tx branch 3 times, most recently from 234152a to e5adc1a Compare October 10, 2022 20:55

maflcko approved these changes Oct 11, 2022

View reviewed changes

ajtowns approved these changes Oct 11, 2022

View reviewed changes

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

instagibbs closed this Oct 28, 2022

instagibbs reopened this Dec 9, 2022

instagibbs force-pushed the relax_too_small_tx branch from e5adc1a to b2c900e Compare December 9, 2022 20:28

glozow requested a review from ajtowns December 12, 2022 15:34

glozow reviewed Dec 19, 2022

View reviewed changes

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

test/functional/mempool_accept.py Show resolved Hide resolved

instagibbs added 2 commits December 19, 2022 10:03

Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation

b2aa9e8

instagibbs force-pushed the relax_too_small_tx branch from b2c900e to b2aa9e8 Compare December 19, 2022 15:04

glozow reviewed Dec 20, 2022

View reviewed changes

pablomartin4btc reviewed Dec 20, 2022

View reviewed changes

jonatack reviewed Dec 20, 2022

View reviewed changes

glozow closed this Dec 21, 2022

ajtowns mentioned this pull request Feb 20, 2023

Prevent 64 byte txs at consensus level bitcoin-inquisition/bitcoin#24

Draft

bitcoin locked and limited conversation to collaborators Dec 22, 2023

	/** The minimum non-witness size for transactions we're willing to relay/mine: one larger than 64 */
	/** The minimum non-witness size for transactions we're willing to relay/mine: one larger than 64 bytes */

	- Transactions of non-witness size 65 and above are now allowed by mempool
	- Transactions of non-witness size 65 bytes and above are now allowed by mempool

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 6, 2022

Uh oh!

instagibbs commented Oct 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

instagibbs commented Oct 7, 2022

Uh oh!

instagibbs commented Oct 7, 2022

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Oct 11, 2022

Uh oh!

instagibbs commented Oct 26, 2022

Uh oh!

maflcko commented Oct 26, 2022

Uh oh!

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

Uh oh!

Uh oh!

instagibbs commented Oct 28, 2022

Uh oh!

maflcko commented Dec 12, 2022

Uh oh!

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

Uh oh!

instagibbs commented Oct 5, 2022 •

edited

Loading

ajtowns Oct 7, 2022 •

edited

Loading

maflcko Oct 7, 2022 •

edited

Loading

darosior commented Oct 7, 2022 •

edited

Loading

DrahtBot commented Oct 7, 2022 •

edited

Loading

rajarshimaitra commented Oct 27, 2022 •

edited

Loading

instagibbs commented Dec 12, 2022 •

edited

Loading

instagibbs Dec 20, 2022 •

edited

Loading

jonatack Dec 20, 2022 •

edited

Loading

jonatack commented Dec 21, 2022 •

edited

Loading