test: test MAX_SCRIPT_SIZE for block validity #32304

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:2025-04-max_script_size_test changing 2 files +32 −2
  1. instagibbs commented at 4:38 pm on April 18, 2025: member
    I don’t believe there are direct tests for this.
  2. DrahtBot commented at 4:38 pm on April 18, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32304.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Tests on Apr 18, 2025
  4. instagibbs force-pushed on Apr 18, 2025
  5. DrahtBot added the label CI failed on Apr 18, 2025
  6. DrahtBot commented at 4:56 pm on April 18, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40791436030

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  7. Sjors commented at 5:26 pm on April 18, 2025: member
    If they don’t exist yet, can you also add a test to show MAX_SCRIPT_SIZE doesn’t “apply” to OP_RETURN? (i.e. nothing is added to the UTXO set, but the block is valid)
  8. instagibbs commented at 5:46 pm on April 18, 2025: member
    @Sjors can you detail what the test would be? OP_RETURNs are already not entered into the utxo set at any length
  9. DrahtBot removed the label CI failed on Apr 18, 2025
  10. bitcoin deleted a comment on Apr 18, 2025
  11. bitcoin deleted a comment on Apr 18, 2025
  12. Sjors commented at 10:48 am on April 21, 2025: member

    @instagibbs while writing my reply on a mailinglist post I found myself wondering if the MAX_SCRIPT_SIZE limit applied to the text after OP_RETURN, either in standardness (if the 83 byte limit is dropped) or in consensus.

    An OP_RETURN with a bit over 10,000 bytes (but less than 100K) would answer that, without reading the code.

    Related:

  13. in test/functional/feature_block.py:118 in fc3525ac93 outdated
    113@@ -112,10 +114,20 @@ def run_test(self):
    114         b_dup_cb = self.update_block('dup_cb', [])
    115         self.send_blocks([b_dup_cb])
    116 
    117-        b0 = self.next_block(0)
    118+        # Add gigantic boundary scripts that respect all other limits
    119+        max_valid_script = CScript([b'\x01' * 520] * 19 + [b'\x01' * 62])
    


    TheCharlatan commented at 12:19 pm on April 21, 2025:

    I think it would be good to mention that this split is done for pushdata accounting and to use MAX_SCRIPT_ELEMENT_SIZE instead of 520. Maybe something like

    0# (OP_PUSHDATA2 + 2 byte length + b'\x01' * 520) * 19 + (1 byte length + b'\x01' * 62)  
    

    instagibbs commented at 1:41 pm on April 21, 2025:
    Pushed a change to use MAX_SCRIPT_ELEMENT_SIZE, but I feel that the code with asserts is already a good explanation?
  14. instagibbs commented at 1:33 pm on April 21, 2025: member
    @Sjors It doesn’t apply because the first clause of the IsUnspendable check is if it starts with an OP_RETURN.
  15. test: test MAX_SCRIPT_SIZE for block validity b1ea542ae6
  16. instagibbs force-pushed on Apr 21, 2025
  17. TheCharlatan approved
  18. TheCharlatan commented at 4:13 pm on April 21, 2025: contributor
    ACK b1ea542ae651cec433910d8c727abc41f17a7678
  19. Christewart commented at 9:06 pm on April 30, 2025: contributor

    Perhaps worth adding a P2WSH test while you are at it?

    P2WSH allows maximum script size of 10,000 bytes, as the 520-byte push limit is bypassed.

    https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#p2wsh

  20. instagibbs commented at 1:07 pm on May 1, 2025: member
    @Christewart that’s a different rule, if you add a test PR and it’s not already covered I’ll ack it!

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: 2025-05-05 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me