Policy: require minimal push in bare multisig output scripts #35225

pull pinheadmz wants to merge 2 commits into bitcoin:master from pinheadmz:p2ms-nonstandard-nonminimal changing 3 files +56 −13

pinheadmz commented at 7:45 PM on May 6, 2026: member

Closes #23285 by adding CheckMinimalPush() in MatchMultisig() in solver.cpp which is only used for policy checks (not consensus).

First commit asserts current behavior on master with unit and functional tests, second commit tightens the policy.

I noticed CheckMinimalPush() has an assert(0 <= opcode && opcode <= OP_PUSHDATA4). That continues to be protected by GetScriptOp() which has guard if (opcode <= OP_PUSHDATA4)

To address concern about current usage I claude-generated this script which efficiently scanned all blk*.dat files from a fully-synced node and reported all occurrences of PUSHDATA in a P2MS output (report below).

Block	Date	vout	Type	Ops	Transaction
244029	2013-06-30	0	1-of-3	OP_PUSHDATA1 ×2	c49b3c445c89d832289de0fd3b0281efdcce418333dacd028061e8de9f0a6f10
244029	2013-06-30	0	1-of-3	OP_PUSHDATA1 ×3	72590fcf0d8021bad77826c5008eaca3541f81d212d55bb7c02ec6a4bf584404
244042	2013-06-30	0	1-of-3	OP_PUSHDATA1 ×2	2d201879608ed2d14c362dff713a6d17d680cb42d5175dfe42e960e94736be04
262733	2013-10-10	3	1-of-2	OP_PUSHDATA1 ×1	055cd2d8ff819ccd7182a08818bf85f79f22e089b81c4f6325014436e172d9b9
263066	2013-10-12	3	1-of-2	OP_PUSHDATA1 ×1	d8773a20a841b4105a98fb2f448f3d9bae2be06f0b9221c02a7cb4d2077f67b9
263069	2013-10-12	2	1-of-2	OP_PUSHDATA1 ×1	5b2169a66a937571767f278097aab307ba95a5dd5beaf9e98a64802218511a52
263096	2013-10-12	2	1-of-2	OP_PUSHDATA1 ×1	c6f2bf13c8742fe5136dba82ce76d1513732d5a5b2d24c8d1bc1a4b92b15cff4
263101	2013-10-12	2	1-of-2	OP_PUSHDATA1 ×1	40af216d3207ba85a47a913a06ace72a10d433ebeb1e12a5c36713a2b11964aa
263421	2013-10-13	2	1-of-3	OP_PUSHDATA1 ×3	11378d2a2bb5514dc7bd5d3bf2eb94dd197e235e9337cac404cf79aa3119e53c
263421	2013-10-13	3	1-of-3	OP_PUSHDATA1 ×3	11378d2a2bb5514dc7bd5d3bf2eb94dd197e235e9337cac404cf79aa3119e53c
263421	2013-10-13	4	1-of-1	OP_PUSHDATA1 ×1	11378d2a2bb5514dc7bd5d3bf2eb94dd197e235e9337cac404cf79aa3119e53c
264469	2013-10-18	1	1-of-3	OP_PUSHDATA1 ×3	92a9a88b078d69de943d9d5d1d3e6b51652952200578e655e50cafaf45a17c34
264469	2013-10-18	2	1-of-3	OP_PUSHDATA1 ×2	92a9a88b078d69de943d9d5d1d3e6b51652952200578e655e50cafaf45a17c34
264469	2013-10-18	1	1-of-3	OP_PUSHDATA1 ×3	7c0eec816b67145a67cbfd479e3c3474df814e31c2e6ff752a48a5a6754c6c2c
264469	2013-10-18	2	1-of-3	OP_PUSHDATA1 ×2	7c0eec816b67145a67cbfd479e3c3474df814e31c2e6ff752a48a5a6754c6c2c
268068	2013-11-05	0	1-of-2	OP_PUSHDATA1 ×1	8e2c7cec5006949e1929f70961da8f85eebfe06a4979d611ec93ab384eaa34ed
268068	2013-11-05	0	1-of-2	OP_PUSHDATA1 ×1	5e29ff050333e2d01a1995cee2ec81f7549baeaef56aa38cb912184794837e84
337908	2015-01-07	2	1-of-1	OP_PUSHDATA1 ×1	04ab6edee514a0ee2aeff48ca672d6f47d6c40923abb0d78aa33ec7e2001eb09

DrahtBot added the label TX fees and policy on May 6, 2026
DrahtBot commented at 7:45 PM on May 6, 2026: 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/35225.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.
in src/test/multisig_tests.cpp:40 in 36462c7a31
```
  36 | @@ -37,6 +37,15 @@ sign_multisig(const CScript& scriptPubKey, const std::vector<CKey>& keys, const
  37 |      return result;
  38 |  }
  39 |  
  40 | +static const auto is_standard{[](const CScript& spk) {
```
luke-jr commented at 3:03 PM on May 7, 2026:

Why not a normal function?

pinheadmz commented at 3:31 PM on May 7, 2026:

Good question! Since this helper is used in more than one scope now it doesn't help to be written as a lambda, fixed.
pinheadmz force-pushed on May 7, 2026
test: assert current policy allows bare multisig with nonminimal push 8175855065
POLICY: require bare multisig outputs use minimal push for pubkeys c5f4b6514d
pinheadmz force-pushed on May 7, 2026
DrahtBot added the label CI failed on May 7, 2026
DrahtBot commented at 3:37 PM on May 7, 2026: contributor


🚧 At least one of the CI tasks failed. <sub>Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/25505604946/job/74850274331</sub> <sub>LLM reason (✨ experimental): CI failed because the build stopped on a -Werror=ignored-qualifiers compiler error in src/test/multisig_tests.cpp (function return type qualifiers ignored).</sub>

<details><summary>Hints</summary>

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.

</details>
DrahtBot removed the label CI failed on May 7, 2026
junbyjun1238 commented at 2:39 PM on May 11, 2026: none

The linked scanner (parse_multisig) appears to record any use of OP_PUSHDATA1/2/4 as non-minimal, but OP_PUSHDATA1 is the minimal encoding for 76..255-byte payloads per CheckMinimalPush(). So the scan may be overcounting: it would flag a legitimate OP_PUSHDATA1 push of, say, an 84-byte or 120-byte payload as non-minimal, even though that encoding is correct for those sizes.

The cases this PR actually rejects are pubkey pushes that (a) pass CPubKey::ValidSize() and (b) fail CheckMinimalPush(). Could the scan be narrowed to that combination, so the table directly reflects the outputs this PR would no longer classify as standard bare multisig?
pinheadmz commented at 3:30 PM on May 11, 2026: member

OP_PUSHDATA1 is the minimal encoding for 76..255-byte payloads

Yes

The cases this PR actually rejects are pubkey pushes that (a) pass CPubKey::ValidSize() and (b) fail CheckMinimalPush().

That is correct and keep in mind, policy already requires the pubkeys in this context are either 65 or 33 bytes. So everything in that list is either already non-standard or will be if this pull request is merged.

Could the scan be narrowed to that combination

You're right the table is a superset of what would be rejected by the policy change in this PR, and I did not consider that when generating it. For example I notice that tx c6f2bf13c8742fe5136dba82ce76d1513732d5a5b2d24c8d1bc1a4b92b15cff4 above has a valid "minimal" push of 118 bytes.

However, the point of the data is to convince reviewers that no current bitcoin software or bitcoin-as-transport software would be affected by the change. I modified the explanation of the data in the PR description to improve accuracy but I think the point is, we haven't seen a P2MS with PUSHDATA in over 10 years and even before that it was very rare.

Contributors

Labels

Linked (view graph)

#23285 Missing CheckMinimalPush check in MatchMultisig: largest IsStandard multisig n-of-3 is 12 bytes too large?

Policy: require minimal push in bare multisig output scripts #35225

Code Coverage & Benchmarks

Reviews