Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only #26398
Conversation
instagibbs
force-pushed
the
relax_too_small_tx_equality
branch
3 times, most recently
from
4824737
to
a67341a
Compare
instagibbs
changed the title
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
|
@rajarshimaitra fwiw, I agree, I like this test code materially more, which suggests it's the right direction, with less indirection happening |
|
Concept ACK, prefer this over #26265 |
instagibbs
force-pushed
the
relax_too_small_tx_equality
branch
3 times, most recently
from
c361621
to
2ddd3a8
Compare
instagibbs
requested review from
rajarshimaitra and
hernanmarino
and removed request for
rajarshimaitra and
hernanmarino
There was a problem hiding this comment.
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')}}],
|
review ACK 2ddd3a8 🏊 Show signatureSignature: 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 |
|
@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 |
instagibbs
force-pushed
the
relax_too_small_tx_equality
branch
from
2ddd3a8
to
a75b0fd
Compare
There was a problem hiding this comment.
re-ACK c248935 🔂
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK c248935ff1774517d523acd622b04b2f89f344ba 🔂
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUga0wv/dgeeqerB07SV45VcbTQHnx3h4jSd/stPjmAeGIVrdlZM7qUR8hWuCkZa
6POSO3Din/scSPyczaeju8FjbdCbIoA/TPJwwBmMRulz7lGFu8VzeGDLKBzmKtr8
ZypWUKba48vdQ9NDLFd2A/bcjWYKjqgJvOCYBmcQVqVfwvbKOPcgJw8XeZCY/8Ut
21HFlklTdsruvVxptzyIh5zkX3Ni60YI9OkMt2N2w7qTWbBI9cjFspHIRG72z83/
Wc2CCpV9weFnSTQ8BeK2nbalSVOEhz8JQFQfUhamKHNX/2AQmqVO51Ix/+PvV93K
LimjCQEpGNlsm5looiNLJQBYF32yUF65aphEvAJeSCPW0VNyGcUfTPEvUQIFztrc
f5KqRjFAf8u7ZhpmeB/peQY8cnhgFpooV9UH9/78SZarNgrF3G4qUivkjKFXcG8K
P7RcnyDQxsUvFJxyOu5ArPhvy5stKBwl/dTUSuxCKEob0R/B6wYkTLXRtpD7xGHb
/tyJW6gC
=CXbv
-----END PGP SIGNATURE-----
| maxfeerate=0, | ||
| ) | ||
|
|
||
| self.log.info('Just-above size transaction(in non-witness bytes) that is allowed') |
There was a problem hiding this comment.
| self.log.info('Just-above size transaction(in non-witness bytes) that is allowed') | |
| self.log.info('Just-above size transaction (in non-witness bytes) that is allowed') |
nit (don't address unless there are other reasons)
There was a problem hiding this comment.
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.
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 |
|
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. |
At least at the policy layer I don't see how that's true.
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? |
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. |
|
Tend to agree with #26398 (review) and prefer #26265. |
|
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 :) |
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 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. |
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.
A scriptPubKey of 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).
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. |
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).
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 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. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Maybe close here, as #26265 was merged. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Closing for now. We can revisit this at some later date. |
|
lightning/bolts#1096 (comment) Just noting that some LN spec work is touching these limits |
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
To enable the maximum use-cases, (1) was taken.
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
Alternative to #26265