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

pull instagibbs wants to merge 2 commits into bitcoin:master from instagibbs:relax_too_small_tx changing 6 files +63 −19
  1. instagibbs commented at 7:07 pm on October 5, 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

    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

  2. instagibbs force-pushed on Oct 5, 2022
  3. DrahtBot added the label TX fees and policy on Oct 5, 2022
  4. glozow commented at 9:49 am on October 6, 2022: member

    Concept ACK

  5. maflcko commented at 1:31 pm on October 6, 2022: member
    Maybe add a test that 65 bytes is passing now?
  6. instagibbs commented at 2:17 pm on October 6, 2022: member
    @MarcoFalke Pushed a test of min size working via testmempoolaccept, and also updated another constants
  7. instagibbs force-pushed on Oct 6, 2022
  8. instagibbs force-pushed on Oct 6, 2022
  9. instagibbs force-pushed on Oct 6, 2022
  10. in test/functional/test_framework/script_util.py:45 in 7633656867 outdated 
    47+
48+# This script cannot be spent, side-stepping nValue standardness checks
49+DUMMY_MIN_OP_RETURN_SCRIPT = CScript([OP_RETURN] * MIN_PADDING)
50+
51+# This can be spent, one shorter due to <push_MIN_PADDING> opcode
52+DUMMY_MIN_SPEND_SCRIPT = CScript([b'a' * (MIN_PADDING - 1)])
    ajtowns commented at 1:03 am on October 7, 2022:

    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?

    instagibbs commented at 2:03 pm on October 7, 2022:

    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

  11. in test/functional/test_framework/script_util.py:32 in 7633656867 outdated 
    36-# scriptPubKeys are needed, to guarantee that the minimum transaction size is
37-# met.
38-DUMMY_P2WPKH_SCRIPT = CScript([b'a' * 21])
39-DUMMY_2_P2WPKH_SCRIPT = CScript([b'b' * 21])
40-
41+# least 5 bytes. DUMMY_P2WPKH_SCRIPT should be used when requiring
    ajtowns commented at 1:10 am on October 7, 2022:

    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?

    maflcko commented at 7:28 am on October 7, 2022:

    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.

    ajtowns commented at 8:24 am on October 7, 2022:
    Oh, it’s “p2wpkh” only in the sense that it’s the same size… Weird.
    fanquake commented at 2:15 am on October 10, 2022:
    These dummy constants are now being removed in #26277 along with acceptnonstdtxn=1 usage.
  12. in test/functional/data/invalid_txs.py:126 in 7633656867 outdated 
    122@@ -122,7 +123,8 @@ class SizeTooSmall(BadTxTemplate):
123     def get_tx(self):
124         tx = CTransaction()
125         tx.vin.append(self.valid_txin)
126-        tx.vout.append(CTxOut(0, CScript([OP_TRUE])))
127+        tx.vout.append(CTxOut(0, CScript([OP_RETURN] * (MIN_PADDING - 1))))
    ajtowns commented at 1:17 am on October 7, 2022:
    Wouldn’t it be better to create an output that passes standardness checks here so that it’s only failing for one reason?
    instagibbs commented at 1:29 pm on October 7, 2022:
    Sorry what’s non-standard about this output? It’s an OP_RETURN(so it can be dust), all pushes, under 80 bytes…
    maflcko commented at 1:34 pm on October 7, 2022:

    OP_RETURN is probably not a push?

    0decodescript 6a6a6a6a6a6a
1
2{
3  "asm": "OP_RETURN OP_RETURN OP_RETURN OP_RETURN OP_RETURN OP_RETURN",
4  "desc": "raw(6a6a6a6a6a6a)#6j3qx6z8",
5  "type": "nonstandard"
6}
    instagibbs commented at 1:49 pm on October 7, 2022:
    Uhm, OP_RETURN isn’t a push, gibbs… facepalm
    instagibbs commented at 1:57 pm on October 7, 2022:
    could add OP_RESERVED to do a bit of trolling
  13. in test/functional/data/invalid_txs.py:127 in 7633656867 outdated 
    122@@ -122,7 +123,8 @@ class SizeTooSmall(BadTxTemplate):
123     def get_tx(self):
124         tx = CTransaction()
125         tx.vin.append(self.valid_txin)
126-        tx.vout.append(CTxOut(0, CScript([OP_TRUE])))
127+        tx.vout.append(CTxOut(0, CScript([OP_RETURN] * (MIN_PADDING - 1))))
128+        assert len(tx.serialize()) == MIN_STANDARD_TX_NONWITNESS_SIZE - 1
    ajtowns commented at 1:24 am on October 7, 2022:
    Should this be checking len(tx.serialize_without_witness()) == 64 ? That’s the actual case where this has an impact?
    instagibbs commented at 2:00 pm on October 7, 2022:
    Added another assert to attempt to make it abundantly clear
  14. darosior commented at 10:38 am on October 7, 2022: member
    Concept ACK. I think this needs a ML post.
  15. DrahtBot commented at 12:25 pm on October 7, 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 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.

  16. instagibbs commented at 1:22 pm on October 7, 2022: member
    @darosior if I don’t get any serious pushback through next week I can put forward a post
  17. instagibbs force-pushed on Oct 7, 2022
  18. instagibbs force-pushed on Oct 7, 2022
  19. instagibbs commented at 2:04 pm on October 7, 2022: member
    Pushed updates and a fix so the passing case is actually running, and test will fail if it doesn’t run.
  20. instagibbs force-pushed on Oct 7, 2022
  21. in test/functional/p2p_invalid_tx.py:98 in 861a276375 outdated 
    93+                # Make it one non-witness byte larger
94+                tx.vout[0].nValue = block1.vtx[0].vout[0].nValue - 1000 # 1k sat fee
95+                tx.vout[0].scriptPubKey = DUMMY_MIN_OP_RETURN_SCRIPT
96+                assert_equal(len(tx.serialize()), MIN_STANDARD_TX_NONWITNESS_SIZE)
97+                result = node.testmempoolaccept(rawtxs=[tx.serialize().hex()])
98+                assert result[0]['allowed']
    maflcko commented at 2:32 pm on October 7, 2022:
    Maybe move this to mempool_accept.py? The purpose of both files looks very similar but I guess mempool_accept feels a bit closer to host a manual check?
    instagibbs commented at 4:13 pm on October 7, 2022:
    done, somewhat duplicating the test, but seems cleaner yes
  22. instagibbs force-pushed on Oct 7, 2022
  23. instagibbs force-pushed on Oct 7, 2022
  24. in test/functional/mempool_accept.py:356 in 8b32fc4842 outdated 
    351+        coinbase.vout[0].scriptPubKey = script_to_p2wsh_script(CScript([OP_TRUE]))
352+        coinbase.rehash()
353+        block = create_block(tip, coinbase, block_time)
354+        block.solve()
355+        node.submitblock(block.serialize().hex())
356+        self.generate(node, 100)
    maflcko commented at 7:23 am on October 10, 2022:
    Seems overkill to mine 100 blocks, when you can simply create the output with the miniwallet?
    instagibbs commented at 8:18 pm on October 10, 2022:
    I went down the rabbit hole with naked OP_TRUE, forgetting that wsh(OP_TRUE) is standard for making and spending… will remove
  25. maflcko commented at 7:25 am on October 10, 2022: member
    left a test nit
  26. DrahtBot added the label Needs rebase on Oct 10, 2022
  27. instagibbs force-pushed on Oct 10, 2022
  28. DrahtBot removed the label Needs rebase on Oct 10, 2022
  29. instagibbs force-pushed on Oct 10, 2022
  30. instagibbs force-pushed on Oct 10, 2022
  31. instagibbs force-pushed on Oct 10, 2022
  32. maflcko approved
  33. maflcko commented at 7:02 am on October 11, 2022: member
    lgtm
  34. in test/functional/mempool_accept.py:363 in e5adc1a284 outdated 
    358+            result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'tx-size-small'}],
359+            rawtxs=[tx.serialize().hex()],
360+            maxfeerate=0,
361+        )
362+
363+        self.log.info('Minimally-small transaction(in on-witness bytes) that is allowed')
    ajtowns commented at 8:53 am on October 11, 2022:
    “non”
    instagibbs commented at 12:30 pm on October 11, 2022:

    so this is also testing that additional witness bytes doesn’t actually help.

    Yes, I only realized this was the case when writing the actual test where I switched to wsh to avoid policy jankery. If the test could be more self-documenting, let me know.

    ajtowns commented at 8:05 pm on October 25, 2022:
    Um, I just meant that you should have written “non-witness” not “on-witness” ?
    instagibbs commented at 8:10 pm on October 25, 2022:
    unsure why I commented here, but I was responding to your bigger text!
    ajtowns commented at 8:16 pm on October 25, 2022:
    Ah; yes, now I remember realising that when I first read your comment, but apparently it didn’t stick
    instagibbs commented at 8:29 pm on December 9, 2022:
    fixed
  35. ajtowns approved
  36. ajtowns commented at 8:54 am on October 11, 2022: contributor

    ACK e5adc1a284a9292362e75501adc9f71ac6ecdf6e

    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)

  37. instagibbs commented at 12:35 pm on October 11, 2022: member
    Emailing the dev list now that this has a couple ACKs and no showstopping discussion
  38. jonatack commented at 1:51 pm on October 11, 2022: contributor

    ACK e5adc1a284a9292362e75501adc9f71ac6ecdf6e

    Reviewed code and context, ran test on master and here, and some helpful discussion with @Xekyo.

  39. instagibbs commented at 1:26 pm on October 21, 2022: member
    On the mailing list @petertodd pushes back and says checking for inequality to 64 should be preferred. @ajtowns you probably had the strongest reaction against it. Thoughts? Typical usage would clock in at 60 bytes, unless you actually wanted OP_RETURN pushes of a specific size for publication purposes.
  40. pablomartin4btc commented at 4:06 pm on October 26, 2022: member

    Tested ACK, validated the change and verified the logs of the test as shown below:

    image

  41. petertodd commented at 4:28 pm on October 26, 2022: contributor

    “That seems pretty negligible to me, and “60-63 is okay but 64 isn’t” seems like a horrible special case.”

    I don’t agree. Either way it’s an arbitrary restriction beyond the goal of preventing 64 byte transactions. Since the exploit is public, we might as well tailor the rule to exactly what we’re trying to prevent, and explain that in the docs. Having an arbitrary “no small txs” rule meanwhile is more mysterious.

    On October 26, 2022 6:06:53 PM GMT+02:00, pablomartin4btc @.> wrote: @. commented on this pull request.

    Tested ACK, validated the change and verified the logs of the test as shown below:

    image

    – Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/26265#pullrequestreview-1156860483 You are receiving this because you were mentioned.

    Message ID: @.***>

  42. instagibbs commented at 4:46 pm on October 26, 2022: member
    As a note, a serious use of this would be to directly toss funds into fees. If the spend isn’t bound to a key, f.e. wsh(OP_TRUE), this opens the door for miner MEV to save a few bytes. If the transactor makes the smallest unspendable output possible(OP_RETURN with no data), this leaves the miner with “just” 1vb on the table, where the miner has incentive to replace it with an empty scriptpubkey, creating 0-value dust. Keeping this delta as small as possible has value, looking forward.
  43. petertodd commented at 4:53 pm on October 26, 2022: contributor

    FWIW I found out about this limitation specifically while trying to spend some dust to fees.

    On October 26, 2022 6:47:00 PM GMT+02:00, Gregory Sanders @.***> wrote:

    As a note, a serious use of this would be to directly toss funds into fees. If the spend isn’t bound to a key, f.e. wsh(OP_TRUE), this opens the door for miner MEV to save a few bytes. If the transactor makes the smallest unspendable output possible(OP_RETURN with no data), this leaves the miner with “just” 1vb on the table, where the miner has incentive to replace it with an empty scriptpubkey, creating 0-value dust. Keeping this delta as small as possible has value, looking forward.

    – Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/26265#issuecomment-1292324949 You are receiving this because you were mentioned.

    Message ID: @.***>

  44. darosior commented at 4:55 pm on October 26, 2022: member

    Concept ACK on not ruling out 60-63 bytes transactions. ——- Original Message ——- Le mercredi 26 octobre 2022 à 6:47 PM, Gregory Sanders @.***> a écrit :

    As a note, a serious use of this would be to directly toss funds into fees. If the spend isn’t bound to a key, f.e. wsh(OP_TRUE), this opens the door for miner MEV to save a few bytes. If the transactor makes the smallest unspendable output possible(OP_RETURN with no data), this leaves the miner with “just” 1vb on the table, where the miner has incentive to replace it with an empty scriptpubkey, creating 0-value dust. Keeping this delta as small as possible has value, looking forward.

    — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

  45. instagibbs commented at 5:37 pm on October 26, 2022: member
    did the opposite way, for comparison https://github.com/bitcoin/bitcoin/pull/26398
  46. hernanmarino commented at 5:49 pm on October 26, 2022: contributor
    tested ACK. I’m not really sure yet about disallowing exactly 64 byte tx or doing the same for txs less than 65, though.
  47. ajtowns commented at 5:55 pm on October 26, 2022: contributor

    If the transactor makes the smallest unspendable output possible(OP_RETURN with no data), this leaves the miner with “just” 1vb on the table, where the miner has incentive to replace it with an empty scriptpubkey, creating 0-value dust. Keeping this delta as small as possible has value, looking forward.

    Seems unlikely that a miner will exactly fill their block up such that 1vb (or 4vb going from 61-byte stripped size to 65-byte stripped size) would make any difference. If you’re sending dust to fees, doing so with a SIGHASH_ANYONECANPAY|SIGHASH_ALL signature against a 0sat OP_RETURN output would allow a miner to combine multiple burn inputs into a single tx (saving 20vb per input after the first, as compared to having each as their own tx), which seems more advantageous.

  48. instagibbs commented at 6:16 pm on October 26, 2022: member

    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?

  49. maflcko commented at 8:46 pm on October 26, 2022: member
    I’ve created a 60 byte transaction on testnet, since one didn’t yet seem to exist https://mempool.space/testnet/tx/b0b076e6647711d8b80439a6ab6ed37764ec48153527629ff3033cc4d8c49751
  50. rajarshimaitra commented at 2:48 pm on October 27, 2022: contributor
    Concept ACK.. Preferring #26398 over this..
  51. instagibbs commented at 1:53 pm on October 28, 2022: member
    closing in favor of #26398
  52. instagibbs closed this on Oct 28, 2022
  53. instagibbs reopened this on Dec 9, 2022
  54. instagibbs force-pushed on Dec 9, 2022
  55. maflcko commented at 9:55 am on December 12, 2022: member

    review ACK b2c900e13065d072a82f0d5d6e9c2340917713b1 🗝

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK b2c900e13065d072a82f0d5d6e9c2340917713b1 🗝
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh+1gv+KcWLoprSo18Z3d5FkDszrsYdapDk6lkAL3GTy+i/D+fEyFS52NQ4OPgF
 8YG4lxjzu3bvYtJgdEvYvfnEoojqfTtLGboHVCvPAIoYssnyV1RerTrwPLOLyRT9n
 98P6LTHq8gaHtq4vgWBQCXAo9RyN3bSMotF1LRSrI9nrazrFUPn2WDw6DInHGLT5r
105qyclAWQ0p7gmGnhlmvXhvaZjWcC8rRsQJ91QGgEJhojz1RCmhcXvTpN0Wu19UyV
11uthSM/T20CSpzYvMDBdmbmiBLzByB8daMK4d5JLB0VSnvKvbwH/06u7pm+jZ/ANz
12hgZS1wqzZ1/XoQNj5WmMm2pl7hxHMdjoTrRHtgdRthYzPUg9HYR/nQyNkXFOIlAY
13oPvLKFKP93gHMkEoO0gVpvbD1dIl42iqbYLLkMca2Bc/lAYWR5de5GTiSRyr6wbH
14P5BP+0NZ6g3bX61A9P71/QIQv9woYjGe9Zp+5qzW9B2/41K0jQFmfHqiIAgmZLGp
15Qw//HuSE
16=8HQY
17-----END PGP SIGNATURE-----
  56. instagibbs commented at 3:18 pm on December 12, 2022: member
    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.
  57. glozow commented at 3:33 pm on December 12, 2022: member
    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.
  58. glozow requested review from ajtowns on Dec 12, 2022
  59. instagibbs commented at 3:35 pm on December 12, 2022: member
    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!
  60. achow101 commented at 5:42 pm on December 12, 2022: member
    ACK b2c900e13065d072a82f0d5d6e9c2340917713b1
  61. in test/functional/mempool_accept.py:343 in b2c900e130 outdated 
    339@@ -333,6 +340,34 @@ def run_test(self):
340             maxfeerate=0,
341         )
342 
343+        # Prep for tin-tx tests with wsh(OP_TRUE) output
    glozow commented at 11:04 am on December 19, 2022:

    nit

    0        # Prep for tiny-tx tests with wsh(OP_TRUE) output
    instagibbs commented at 3:04 pm on December 19, 2022:
    done
  62. in test/functional/mempool_accept.py:355 in b2c900e130 outdated 
    349+        tx.vin.append(CTxIn(COutPoint(int(seed_tx[0], 16), seed_tx[1]), b"", SEQUENCE_FINAL))
350+        tx.wit.vtxinwit = [CTxInWitness()]
351+        tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE])]
352+        tx.vout.append(CTxOut(0, CScript([OP_RETURN] + ([OP_0] * (MIN_PADDING - 2)))))
353+        # Note it's only non-witness size that matters!
354+        assert_equal(len(tx.serialize_without_witness()), 64)
    glozow commented at 11:07 am on December 19, 2022:

    nit: based on the comment I think something like this would be appropriate to add: (import assert_greater_than from util)

    0        # Note it's only non-witness size that matters!
1        assert_greater_than(len(tx.serialize()), 64)
2        assert_equal(len(tx.serialize_without_witness()), 64)
    instagibbs commented at 3:04 pm on December 19, 2022:
    done
  63. glozow commented at 11:18 am on December 19, 2022: member

    ACK b2c900e130, 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)
  64. Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes 
    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
    8c5b3646b5
  65. Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation b2aa9e8528
  66. instagibbs force-pushed on Dec 19, 2022
  67. instagibbs commented at 3:04 pm on December 19, 2022: member
    Done all as @glozow has suggested, and touched up commit messages a bit more
  68. glozow commented at 10:24 am on December 20, 2022: member

    reACK b2aa9e85289fc654106a890c35935e9c76c411fb

    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

  69. achow101 commented at 7:14 pm on December 20, 2022: member
    reACK b2aa9e85289fc654106a890c35935e9c76c411fb
  71. in src/policy/policy.h:28 in b2aa9e8528 
    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 minimum non-witness size for transactions we're willing to relay/mine: one larger than 64  */
    jonatack commented at 8:32 pm on December 20, 2022:

    It would be clearer to keep the unit.

    0/** The minimum non-witness size for transactions we're willing to relay/mine: one larger than 64 bytes */
    instagibbs commented at 9:44 pm on December 20, 2022:
    will add if I ever touch the PR
  72. in test/functional/data/invalid_txs.py:128 in b2aa9e8528 
    122@@ -122,7 +123,9 @@ class SizeTooSmall(BadTxTemplate):
123     def get_tx(self):
124         tx = CTransaction()
125         tx.vin.append(self.valid_txin)
126-        tx.vout.append(CTxOut(0, CScript([OP_TRUE])))
127+        tx.vout.append(CTxOut(0, CScript([OP_RETURN] + ([OP_0] * (MIN_PADDING - 2)))))
128+        assert len(tx.serialize_without_witness()) == 64
129+        assert MIN_STANDARD_TX_NONWITNESS_SIZE - 1 == 64
    jonatack commented at 8:42 pm on December 20, 2022:
    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.
    instagibbs commented at 9:44 pm on December 20, 2022:
    documentation, thanks for double checking
  73. in doc/release-notes-26265.md:4 in b2aa9e8528 
    0@@ -0,0 +1,6 @@
1+P2P and network changes
2+---------
3+
4+- Transactions of non-witness size 65 and above are now allowed by mempool
    jonatack commented at 8:47 pm on December 20, 2022:

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

    0- Transactions of non-witness size 65 bytes and above are now allowed by mempool
    instagibbs commented at 9:44 pm on December 20, 2022:
    will add if I ever touch the PR
  74. jonatack commented at 8:49 pm on December 20, 2022: contributor
    ACK b2aa9e85289fc654106a890c35935e9c76c411fb with some suggestions
  75. achow101 referenced this in commit f3bc1a7282 on Dec 21, 2022
  76. jonatack commented at 6:28 pm on December 21, 2022: contributor
    Is this merged and should be closed? Edit: yes.
  77. instagibbs commented at 6:28 pm on December 21, 2022: member
    Looks like it. @achow101 ?
  78. glozow closed this on Dec 21, 2022
  79. sidhujag referenced this in commit 5224ae78a2 on Dec 21, 2022
  80. jessebarton referenced this in commit 3b41220e02 on Apr 7, 2023
  81. bitcoin locked this on Dec 22, 2023


instagibbs glozow maflcko ajtowns fanquake darosior DrahtBot jonatack pablomartin4btc petertodd hernanmarino rajarshimaitra achow101


ajtowns

Labels
TX fees and policy

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-26 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me