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-
instagibbs commented at 4:38 pm on April 18, 2025: memberI don’t believe there are direct tests for this.
-
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.
-
DrahtBot added the label Tests on Apr 18, 2025
-
instagibbs force-pushed on Apr 18, 2025
-
DrahtBot added the label CI failed on Apr 18, 2025
-
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.
-
-
Sjors commented at 5:26 pm on April 18, 2025: memberIf they don’t exist yet, can you also add a test to show
MAX_SCRIPT_SIZE
doesn’t “apply” toOP_RETURN
? (i.e. nothing is added to the UTXO set, but the block is valid) -
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
-
DrahtBot removed the label CI failed on Apr 18, 2025
-
bitcoin deleted a comment on Apr 18, 2025
-
bitcoin deleted a comment on Apr 18, 2025
-
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 afterOP_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:
-
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 like0# (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 useMAX_SCRIPT_ELEMENT_SIZE
, but I feel that the code with asserts is already a good explanation?instagibbs commented at 1:33 pm on April 21, 2025: member@Sjors It doesn’t apply because the first clause of theIsUnspendable
check is if it starts with an OP_RETURN.test: test MAX_SCRIPT_SIZE for block validity b1ea542ae6instagibbs force-pushed on Apr 21, 2025TheCharlatan approvedTheCharlatan commented at 4:13 pm on April 21, 2025: contributorACK b1ea542ae651cec433910d8c727abc41f17a7678Christewart commented at 9:06 pm on April 30, 2025: contributorPerhaps 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
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!
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
More mirrored repositories can be found on mirror.b10c.me