test: script: boundary at exactly 65535 bytes must use OP

brunoerg commented at 12:56 PM on March 25, 2026: contributor

I noticed that the following mutant is not killed by any current test (can be tested with: cmake --build build -j $(nproc) && ./build/bin/test_bitcoin && ./build/test/functional/test_runner.py -j $(nproc) -F):

diff --git a/src/script/script.cpp b/src/script/script.cpp
index 3f764aaf21..5cff51d2cf 100644
--- a/src/script/script.cpp
+++ b/src/script/script.cpp
@@ -387,7 +387,7 @@ bool CheckMinimalPush(const std::vector<unsigned char>& data, opcodetype opcode)
     } else if (data.size() <= 255) {
         // Must have used OP_PUSHDATA.
         return opcode == OP_PUSHDATA1;
-    } else if (data.size() <= 65535) {
+    } else if (data.size() < 65535) {
         // Must have used OP_PUSHDATA2.
         return opcode == OP_PUSHDATA2;
     }

This PR addresses it by adding a new test case to ensure that the boundary at exactly 65535 bytes must use OP_PUSHDATA2 as well.

test: script: boundary at exactly 65535 bytes must use OP_PUSHDATA2 f899674639

DrahtBot added the label Tests on Mar 25, 2026

DrahtBot commented at 12:56 PM on March 25, 2026: 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	kevkevinpal, danielabrozzoni, darosior, w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

kevkevinpal commented at 3:24 PM on March 25, 2026: contributor

tACK f899674

I was able to test that

We were not covering this test on master 2fe76ed8324af44c985b96455a05c3e8bec0a03e
The test suite failed with patch applied on f899674
The test suite passed without the patch applied on f899674

danielabrozzoni commented at 9:43 PM on March 25, 2026: member

Nice catch! Concept ACK

I believe CheckMinimalPush is mostly tested in script_json_test (testcases in src/test/data/script_tests.json), however, it wouldn't be practical to include the 65535 bytes string there, so it makes sense to have a separate test for that.

While we're at it, should we also check that 65536 bytes uses OP_PUSHDATA4? EDIT: this is incorrect: #34919 (comment)

diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
index b07870207c..87dc079a12 100644
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -1461,6 +1461,11 @@ BOOST_AUTO_TEST_CASE(script_CheckMinimalPush_boundary)
     std::vector<unsigned char> data(65535, '\x42');
     BOOST_CHECK(CheckMinimalPush(data, OP_PUSHDATA2));
     BOOST_CHECK(!CheckMinimalPush(data, OP_PUSHDATA4));
+
+    // Test the boundary at exactly 65536 bytes: must use OP_PUSHDATA4.
+    std::vector<unsigned char> data2(65536, '\x42');
+    BOOST_CHECK(!CheckMinimalPush(data2, OP_PUSHDATA2));
+    BOOST_CHECK(CheckMinimalPush(data2, OP_PUSHDATA4));
 }
 
 BOOST_AUTO_TEST_CASE(script_GetScriptAsm)

brunoerg commented at 12:23 AM on March 26, 2026: contributor

While we're at it, should we also check that 65536 bytes uses OP_PUSHDATA4?

I don't think your suggestion is correct. The CheckMinimalPush returns true for any opcode when data.size() > 65535, so this check from your diff would fail: BOOST_CHECK(!CheckMinimalPush(data2, OP_PUSHDATA2));.

danielabrozzoni commented at 10:52 AM on March 26, 2026: member

I don't think your suggestion is correct.

Yes, sorry about that!

danielabrozzoni approved

danielabrozzoni commented at 10:55 AM on March 26, 2026: member

tACK f89967463933c5e1c7dd3e841813035c1ff260dc

in src/test/script_tests.cpp:1463 in f899674639

1454 | @@ -1455,6 +1455,14 @@ BOOST_AUTO_TEST_CASE(script_IsPushOnly_on_invalid_scripts)
1455 |      BOOST_CHECK(!CScript(direct, direct+sizeof(direct)).IsPushOnly());
1456 |  }
1457 |  
1458 | +BOOST_AUTO_TEST_CASE(script_CheckMinimalPush_boundary)
1459 | +{
1460 | +    // Test the boundary at exactly 65535 bytes: must use OP_PUSHDATA2, not OP_PUSHDATA4.
1461 | +    std::vector<unsigned char> data(65535, '\x42');
1462 | +    BOOST_CHECK(CheckMinimalPush(data, OP_PUSHDATA2));
1463 | +    BOOST_CHECK(!CheckMinimalPush(data, OP_PUSHDATA4));

darosior commented at 9:40 PM on March 26, 2026:

nit, feel free to ignore.

These two checks are virtually equivalent. For checking boundary i like to simply add one to assert that it is the last value that will produce a given result. For instance:

diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
index b07870207cb..454fcba3ec6 100644
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -1460,7 +1460,8 @@ BOOST_AUTO_TEST_CASE(script_CheckMinimalPush_boundary)
     // Test the boundary at exactly 65535 bytes: must use OP_PUSHDATA2, not OP_PUSHDATA4.
     std::vector<unsigned char> data(65535, '\x42');
     BOOST_CHECK(CheckMinimalPush(data, OP_PUSHDATA2));
-    BOOST_CHECK(!CheckMinimalPush(data, OP_PUSHDATA4));
+    data.push_back('\x42');
+    BOOST_CHECK(CheckMinimalPush(data, OP_PUSHDATA4));
 }
 
 BOOST_AUTO_TEST_CASE(script_GetScriptAsm)

brunoerg commented at 10:02 PM on March 27, 2026:

Will leave as is for now to not invalidate ACKs. Thanks.

darosior approved

darosior commented at 9:41 PM on March 26, 2026: member

utACK f89967463933c5e1c7dd3e841813035c1ff260dc

w0xlt commented at 11:23 PM on March 26, 2026: contributor

ACK f89967463933c5e1c7dd3e841813035c1ff260dc

fanquake merged this on Mar 28, 2026

fanquake closed this on Mar 28, 2026

brunoerg deleted the branch on Mar 30, 2026

test: script: boundary at exactly 65535 bytes must use OP_PUSHDATA2 #34919

Reviews