Align legacy script policy with P2SH policy in AreInputsStandard

151henry151 commented at 7:03 pm on November 22, 2025: contributor

Legacy scripts currently require solvability, while P2SH scripts only check sigop count. This asymmetry causes some non-solvable legacy scripts to be rejected even when they have acceptable sigop counts, while equivalent P2SH scripts would be accepted.

This change updates AreInputsStandard to check sigop count for legacy scripts instead of requiring solvability, matching P2SH behavior. Non-solvable legacy scripts with sigop counts within the limit are now accepted, consistent with P2SH.

This implements the approach suggested by roconnor-blockstream in #33882 to address the policy asymmetry between legacy and P2SH script redemption.

The test suite covers boundary conditions (0, 14, 15, and 16 sigops), different sigop types (CHECKSIG and CHECKMULTISIG), multiple inputs, and regression cases to ensure existing behavior for standard and WITNESS_UNKNOWN scripts is preserved.

DrahtBot commented at 7:03 pm on November 22, 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/33926.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29060 (Policy: Report debug message why inputs are non standard by ismaelsadeeq)

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.

LLM Linter (✨ experimental)

Possible places where named args may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

AddCoins(coins, CTransaction(txFrom), 0) in src/test/script_p2sh_tests.cpp

2025-11-26

DrahtBot added the label CI failed on Nov 22, 2025

DrahtBot commented at 7:49 pm on November 22, 2025: contributor

🚧 At least one of the CI tasks failed. Task macOS native, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/19599919918/job/56129931926 LLM reason (✨ experimental): Fuzz target crash: assertion failed (mempoolDuplicate.HaveCoin) in txmempool.cpp during process_messages, causing non-zero exit.

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.

151henry151 commented at 9:33 pm on November 22, 2025: contributor

Our change allows NONSTANDARD legacy scripts with acceptable sigop counts to pass AreInputsStandard, which breaks tests that relied on those scripts being rejected.

p2p_segwit.py uses a NONSTANDARD script (0 sigops) as self.utxo[0] that now passes instead of being rejected, so transactions that should be rejected are getting accepted.

feature_taproot.py has some spenders with NONSTANDARD scripts that are now allowed, so the test’s standardness checks don’t match the new policy.

I’ll fix both by updating the tests to match the new policy.

151henry151 commented at 6:34 pm on November 23, 2025: contributor

I updated the tests to match the new policy. The NONSTANDARD script in p2p_segwit.py now passes AreInputsStandard since it has zero sigops. For test_segwit_versions, I switched the input from the NONSTANDARD UTXO to the standard UTXOs created earlier, so the transaction now passes all checks and gets accepted. For feature_taproot.py, I adjusted the is_standard flag for the compat/nocsa spender because bare NONSTANDARD scripts with low sigop counts now pass policy checks.

Please check this all out and let me know if this looks correct or needs any adjustment.

151henry151 force-pushed on Nov 24, 2025

151henry151 commented at 9:30 pm on November 24, 2025: contributor

The process_messages fuzz test fails with Assertion failed: (mempoolDuplicate.HaveCoin(txin.prevout)) at txmempool.cpp:727 after allowing NONSTANDARD scripts with ≤15 sigops to pass AreInputsStandard.

I added a coin validity check in AreInputsStandard (verifying coin.IsSpent() before GetSigOpCount), but the test still fails. Since this PR only aligns policy behavior and shouldn’t require mempool changes, perhaps the issue is how the policy change interacts with the consistency check.

I’m not seeing why transactions accepted by the policy fail the consistency check. Any help diagnosing this would be appreciated. Perhaps you might have some ideas, @roconnor-blockstream ?

in src/policy/policy.cpp:234 in 4d06b1e94c outdated

235             // WITNESS_UNKNOWN failures are typically also caught with a policy
236             // flag in the script interpreter, but it can be helpful to catch
237             // this type of NONSTANDARD transaction earlier in transaction
238             // validation.
239             return false;
240+        } else if (whichType == TxoutType::NONSTANDARD) {

roconnor commented at 5:09 pm on November 25, 2025:

Maybe it is better to add a new TxoutType::LEGACY and just match on that (putting NONSTADARD back on the line above) rather than redoing some of the work already done by Solver.

roconnor commented at 5:56 pm on November 25, 2025: none

My best guess is that making legacy script standard means that randomly generated ScriptPubKeys are now generally mempool acceptable whereas before only very few specifically shaped ScriptPubKeys could be accepted and weren’t being generated by the fuzzer. This is exposing some kind of error in the process_messages fuzz test harness.

One thing to explore is to see if reverting PR #32822 makes the failure disappear. If so then we maybe narrowed down the problem to the recent changes to this fuzz test.

Another angle we might take is that legacy script perhaps ought to require at least one checksig operation, otherwise it is unsecured and, theoretically, eligible as an upgrade mechanism similar to Segwit. This might sweep any potential existing process_messages fuzz test issues under the rug.

151henry151 force-pushed on Nov 26, 2025

policy: align legacy script policy with P2SH

Check sigop count for legacy scripts instead of requiring solvability,
matching P2SH behavior. Add TxoutType::LEGACY and require ≥1 sigop.
Update tests to reflect the sigop requirement.

9f1cb8abd1

151henry151 force-pushed on Nov 26, 2025

Align legacy script policy with P2SH policy in AreInputsStandard #33926

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)