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

pull instagibbs wants to merge 2 commits into bitcoin:master from instagibbs:relax_too_small_tx_equality changing 6 files +67 −25
  1. instagibbs commented at 5:35 pm on October 26, 2022: member

    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

    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 https://github.com/bitcoin/bitcoin/pull/26265

  2. instagibbs force-pushed on Oct 26, 2022
  3. instagibbs force-pushed on Oct 26, 2022
  4. instagibbs force-pushed on Oct 26, 2022
  5. 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.
  6. 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
  7. glozow added the label TX fees and policy on Oct 26, 2022
  8. 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
  9. 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.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK theStack, darosior, rajarshimaitra, glozow, MarcoFalke
    Concept ACK hernanmarino
    Approach NACK ajtowns

    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:

    • #26265 (POLICY: Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes 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.

  10. 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..

  11. 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
  12. 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
  13. maflcko approved
  14. maflcko commented at 3:30 pm on October 27, 2022: member
    lgtm
  15. darosior commented at 10:14 am on October 28, 2022: member
    Concept ACK, prefer this over #26265
  16. instagibbs force-pushed on Oct 28, 2022
  17. instagibbs force-pushed on Oct 28, 2022
  18. instagibbs force-pushed on Oct 28, 2022
  19. instagibbs requested review from rajarshimaitra on Oct 31, 2022
  20. instagibbs removed review request from rajarshimaitra on Oct 31, 2022
  21. instagibbs requested review from hernanmarino on Oct 31, 2022
  22. instagibbs removed review request from hernanmarino on Oct 31, 2022
  23. instagibbs requested review from rajarshimaitra on Oct 31, 2022
  24. hernanmarino approved
  25. theStack approved
  26. 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')}}],
  27. 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)

  28. 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

  29. in src/policy/policy.h:29 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
  30. 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?

  31. instagibbs force-pushed on Nov 10, 2022
  32. instagibbs commented at 7:39 pm on November 10, 2022: member
    @glozow pushed a brief release note along with edits from @glozow and @theStack
  33. 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

  34. instagibbs force-pushed on Nov 10, 2022
  35. Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 64 non-witness bytes exactly 
    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

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
    533e8ae5a1
  36. Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE replacement c248935ff1
  37. instagibbs force-pushed on Nov 10, 2022
  38. 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
  39. theStack approved
  40. theStack commented at 4:47 pm on November 11, 2022: contributor
    re-ACK c248935ff1774517d523acd622b04b2f89f344ba 📔
  41. darosior approved
  42. darosior commented at 6:19 pm on November 16, 2022: member
    utACK c248935ff1774517d523acd622b04b2f89f344ba
  43. rajarshimaitra approved
  44. rajarshimaitra commented at 2:10 pm on November 17, 2022: contributor
    ReACK c248935ff1774517d523acd622b04b2f89f344ba
  45. fanquake requested review from ajtowns on Nov 17, 2022
  46. glozow commented at 8:09 pm on November 17, 2022: member
    re ACK c248935ff1774517d523acd622b04b2f89f344ba
  47. hernanmarino approved
  48. in test/functional/mempool_accept.py:365 in c248935ff1 
    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)

  49. maflcko approved
  50. 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-----
  51. 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.

  52. 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

  53. 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.
  54. 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?

  55. 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.

  56. jonatack commented at 8:07 pm on December 8, 2022: contributor
    Tend to agree with #26398#pullrequestreview-1186725701 and prefer #26265.
  57. 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 :)

  58. 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.

  59. glozow requested review from sdaftuar on Dec 9, 2022
  60. 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.

  61. 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.

  62. 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.

  63. 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.

  64. instagibbs marked this as a draft on Dec 12, 2022
  65. 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.

  66. DrahtBot commented at 6:29 pm on December 21, 2022: contributor

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

  67. DrahtBot added the label Needs rebase on Dec 21, 2022
  68. jonatack commented at 7:42 pm on December 21, 2022: contributor
    Maybe close here, as #26265 was merged.
  69. 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.

  70. DrahtBot commented at 0:55 am on March 21, 2023: contributor

    There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
  71. instagibbs commented at 1:26 am on March 21, 2023: member
    Closing for now. We can revisit this at some later date.
  72. instagibbs closed this on Mar 21, 2023
  73. 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

  74. bitcoin locked this on Jul 26, 2024


instagibbs hernanmarino maflcko DrahtBot rajarshimaitra darosior theStack glozow ajtowns TheBlueMatt jonatack achow101 sipa sdaftuar


sdaftuar

Labels
TX fees and policy Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me