test: test MAX_SCRIPT_SIZE for block validity #32304

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, theStack

   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:

   • #32421 (test: refactor: overhaul (w)txid determination for CTransaction objects by theStack)

   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.

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:

    • https://bitcoin.stackexchange.com/a/78880/4948
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!
21. theStack approved
22. 
    theStack commented at 1:40 pm on May 12, 2025: contributor

    ACK b1ea542ae651cec433910d8c727abc41f17a7678

    nit: if you have to retouch, could add a log message (self.log.info)

Contributors
instagibbs DrahtBot Sjors TheCharlatan Christewart theStack

Labels
Tests