Skip to content

policy: cap P2MR runtime stack bytes at 1 MiB (v1.0.1) - #269

Closed
kiwidream wants to merge 2 commits into
1.x.xfrom
kiwidream/rg-257-p2mr-runtime-stack-bytes
Closed

kiwidream wants to merge 2 commits into
1.x.xfrom
kiwidream/rg-257-p2mr-runtime-stack-bytes

Conversation

@kiwidream

@kiwidream kiwidream commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

Summary

Refs #257. The consensus cap stays open.

Targets the v1.0.1 release line. 1.x.x is frozen for v1.0.1 (#181), so a maintainer must lift the freeze before merging, and the v1.0.1 release notes in #181 need this entry (fragment: doc/release-notes-269.md).

P2MR v1 bounds the initial witness stack (16 KiB per item, 128 KiB in aggregate), but at runtime EvalScript only bounds stack plus altstack to 1,000 items. Copying opcodes duplicate whole values, so a leaf of 999 × OP_DUP then 999 × OP_DROP with one 16 KiB witness item holds about 16 MiB during verification. That spend is standard today and relays.

This PR adds a policy-only runtime byte cap:

  • MAX_STANDARD_P2MR_RUNTIME_STACK_BYTES = 1 MiB (src/script/script.h): stack plus altstack bytes must never exceed it during SigVersion::P2MR execution.
  • New flag SCRIPT_VERIFY_P2MR_RUNTIME_STACK_BYTES (bit 23), added to STANDARD_SCRIPT_VERIFY_FLAGS only. It is not in MANDATORY_SCRIPT_VERIFY_FLAGS and GetBlockScriptFlags never sets it, so block validation is unchanged (static asserts pin this in the new test file).
  • New SCRIPT_ERR_P2MR_RUNTIME_STACK_BYTES ("P2MR stack and altstack bytes exceed the standard runtime limit").
  • Mempool classification needs no new code: CheckInputScripts reports a failure under any STANDARD_NOT_MANDATORY_VERIFY_FLAGS flag as TX_NOT_STANDARD / mempool-script-verify-flag-failed (...), exactly like the existing policy flags. This base does not punish peers for transaction failures, so v1.0.0 peers relaying such spends are unaffected.

Accounting cost: O(1) per opcode

P2MR has no opcode limit, so rescanning the stack per opcode would be quadratic. P2MRStackBytes keeps a running total instead. Every opcode P2MR executes, except OP_ROLL, changes at most the top 6 main-stack items (OP_2ROT reaches deepest; OP_CHECKMULTISIG fails in P2MR before touching the stack) and the top altstack item. Before each opcode the tracker sums those windows, and after it re-sums the same positions and applies the difference. OP_ROLL only moves an item, so its net effect is popping its numeric argument. The check runs right after the existing 1,000-item check, so the item-count error still wins when both trip on the same opcode. When the flag is unset, or for non-P2MR sigversions, the tracker is never constructed and behaviour, including every error code, is unchanged.

Debug and fuzz builds (ABORT_ON_FAILED_ASSUME / FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION) also Assume that the running total equals a full rescan after every opcode. This is compiled out of release builds and guards against a future opcode silently breaking the window argument. The p2mr_script fuzz target now sets the new flag, so fuzzing exercises that check. A randomized test cross-checks the running total against a full rescan after every opcode (500 random scripts over all stack-manipulating opcodes, initial stacks of 0.5 to 1 MiB). It asserts that the flagged run stops at exactly the first opcode where the rescan exceeds the cap, and otherwise matches the unflagged run exactly.

Why 1 MiB

  • It is 8 × the 128 KiB aggregate initial-stack limit. A script that does not copy items (including the wallet's CHECKSIGPQC/CHECKSIGADD multisig leaves) peaks at its initial stack plus 520-byte literals, so it cannot get near the cap.
  • It still allows 64 full-size 16 KiB copies, which is far more than any known construction needs.
  • It caps relay-path memory per input at 1 MiB, against about 16 MiB today.

Docs

doc/consensus/p2mr-v1.md is not edited. It is bound by the release conformance gate: ci/release/verify_p2mr_v1_conformance.py::validate_specification_commit requires the spec blob at the release tag to equal the blob at the reviewed specification_commit ("normative specification changed after specification_commit without updated review evidence"). Editing it in a point release would invalidate consensus review evidence for a policy-only change. Following the task instructions, the limit is documented in a new policy page, doc/policy/p2mr-script-limits.md, linked from doc/policy/README.md. That page cross-references §10 of the profile.

Known limitation

Block validation still accepts up to about 16 MiB of runtime stack per input, or about 256 MiB held at once with 16 script-check threads (MAX_SCRIPTCHECK_THREADS = 15 plus the main thread), until a later release activates a consensus cap. This is recorded in the policy doc and the release-notes fragment.

Engineering practices applied

  • EP-COMPAT: traced the flag through STANDARD_SCRIPT_VERIFY_FLAGS → ATMP PolicyScriptChecks → CheckInputScripts classification, and confirmed GetBlockScriptFlags, ConsensusScriptChecks and the script-execution cache never see it. Mixed versions: v1.0.0 nodes and miners may still relay and mine such spends. v1.0.1 rejects them from its mempool without punishing the peer, and accepts blocks containing them; the unit test covers this. ScriptError numeric values are not persisted or exposed anywhere (RPC and the corpus use names). The P2MR v1 corpus, manifest and spec are unchanged.
  • EP-VALIDATION: boundary tests at exactly the cap and at cap + 1 byte, through the real VerifyScript entry point, for every copying opcode (OP_DUP, OP_2DUP, OP_3DUP, OP_OVER, OP_2OVER, OP_PICK, OP_TUCK, OP_IFDUP), OP_ROLL, rearrangements, literal pushes, hashing, altstack (OP_TOALTSTACK/OP_FROMALTSTACK), and the initial stack. They also go through ATMP and block connection.
  • EP-STATE / EP-ERRORS: not applicable. No cache key or resource lifetime changes, and failure is a plain early set_error return.

Testing

All heavy checks ran under flock …/qbit-heavy-check.lock systemd-run --user --scope -p MemoryMax=20G -p MemorySwapMax=0.

  • Built locally: cmake --build build -j12 (RelWithDebInfo, tests, bench, wallet; no GUI/ZMQ). Passed.
  • Ran focused unit or functional tests for the changed area:
    • test_qbit --run_test=script_p2mr_runtime_stack_tests,script_p2mr_tests,txvalidation_tests,txvalidationcache_tests,script_tests,transaction_tests,script_assets_tests,p2mr_datapqchash_tests,psbt_p2mr_tests,descriptor_p2mr_tests,script_standard_tests,pqc_sigcache_tests: 232 test cases, no errors. This includes the P2MR v1 manifest/corpus tests (p2mr_v1_manifest_matches_embedded_corpus and the vector suites) and existing P2MR policy tests, with unchanged results.
    • Full unit suite build/bin/test_qbit: all 178 suites (1,302 test cases), no errors, on the pushed head. It was run as three suite batches to fit tool time limits.
    • test_runner.py --jobs=4 feature_p2mr.py feature_p2mr_activation.py feature_p2mr_weight_activation.py wallet_p2mr.py wallet_p2mr_psbt.py wallet_p2mr_multisig_restart.py mempool_accept.py: 7/7 passed.
    • Debug build (-DCMAKE_BUILD_TYPE=Debug, so ABORT_ON_FAILED_ASSUME and the rescan check are active): test_qbit --run_test=script_p2mr_runtime_stack_tests,script_p2mr_tests,txvalidation_tests,script_tests,transaction_tests,txvalidationcache_tests,pqc_sigcache_tests, 170 test cases, no errors. The same 7 functional tests as above also pass on the Debug binaries.
    • Fuzz binary (-DBUILD_FUZZ_BINARY=ON, target fuzz): compiles.
  • Ran lint or formatting checks relevant to this change: test/lint/lint-files.py, lint-includes.py, lint-include-guards.py, lint-locale-dependence.py, lint-tests.py, lint-qbit-public-docs.py, check-doc.py, lint-circular-dependencies.py, git diff --check. All passed.
  • Not run. Reason: lint-spelling.py was skipped because codespell is not installed. The Rust test/lint/test_runner was not run because cargo is not installed on the build host; CI runs it. ctest is deliberately not run (its bench_sanity_check exhausts host memory on this base).

New and changed tests:

  • src/test/script_p2mr_runtime_stack_tests.cpp:
    • the 16 KiB DUP/DROP leaf fails with the new error under standard flags, and passes under mandatory flags and under standard flags without the new flag;
    • at-cap and cap + 1 boundaries for every copying opcode, plus altstack, OP_ROLL, rearrangements, pushes, hashing and the initial stack;
    • flag-off error preservation, and item-count error precedence;
    • TAPSCRIPT gating;
    • the randomized rescan cross-check.
  • txvalidation_tests/p2mr_runtime_stack_bytes_is_policy_only:
    • ATMP rejects the DUP/DROP spend as TX_NOT_STANDARD with mempool-script-verify-flag-failed (P2MR stack and altstack bytes exceed the standard runtime limit);
    • an at-cap spend passes test_accept;
    • a block containing the over-cap spend connects.
  • src/test/fuzz/p2mr_script.cpp: both P2MR script fuzz inputs can set SCRIPT_VERIFY_P2MR_RUNTIME_STACK_BYTES.
  • src/test/script_tests.cpp: P2MR_RUNTIME_STACK_BYTES was added to the error-name table used for JSON vectors.

Target Branch

  • This PR targets main or a maintainer-requested release branch such as 0.1.x.

    Targets 1.x.x (the v1.0.1 release line), at base 4fb9dbeac3717ad545ba1c0f09fdc05fd24aa3e2.

Risk / Review Notes

  • Consensus, script, crypto, wallet, P2P, release, CI, or security-sensitive behavior changed.
  • No consensus, script, crypto, wallet, P2P, release, CI, or security-sensitive behavior changed.

Notes: this changes the script interpreter and standardness policy, but not consensus validity. All new interpreter logic is behind a flag that only standard policy sets, and the tests show that block acceptance of the over-cap spend is unchanged. Review focus: the window argument in P2MRStackBytes, which the randomized rescan test checks independently. Expect a textual conflict in src/script/interpreter.cpp with the #259 work; this change is limited to EvalScript and an anonymous-namespace helper.

Docs / Process Impact

  • I updated public docs because this PR changes user-visible behavior, integration guidance, release/process guidance, or expected validation.
  • No public docs update needed. Reason:

New doc/policy/p2mr-script-limits.md, linked from doc/policy/README.md, plus the release-notes fragment. doc/consensus/p2mr-v1.md is intentionally untouched (see Docs above).

🤖 Generated with Claude Code

P2MR v1 bounds initial witness items to 16 KiB each and 128 KiB in
aggregate, but at runtime only bounds stack plus altstack to 1,000 items.
Copying opcodes duplicate whole values, so a leaf of 999 OP_DUP then 999
OP_DROP over one 16 KiB item holds about 16 MiB while verifying, and is
standard today.

Add SCRIPT_VERIFY_P2MR_RUNTIME_STACK_BYTES, part of the standard script
verification flags only. With it, the main stack plus altstack bytes must
never exceed MAX_STANDARD_P2MR_RUNTIME_STACK_BYTES (1 MiB) during
SigVersion::P2MR execution, failing with the new
SCRIPT_ERR_P2MR_RUNTIME_STACK_BYTES. CheckInputScripts classifies the
failure as TX_NOT_STANDARD like other non-mandatory flags. Block
validation never sets the flag, so consensus validity is unchanged.

A running total is updated after every opcode by re-summing only the top
six stack items and top altstack item, which every P2MR opcode except
OP_ROLL stays within; OP_ROLL nets out to popping its argument. This
keeps the check O(1) per opcode. Debug and fuzz builds verify the total
against a full rescan. With the flag unset nothing changes.

Document the limit in doc/policy rather than the P2MR v1 profile, whose
text is bound to its reviewed specification commit by the release
conformance validator.

Refs #257

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-30T15:17:22.124633Z 2b6a899 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kiwidream

Copy link
Copy Markdown
Member Author

Closing in favour of #263, which implements the same consensus-neutral fix for #257: a policy-only 1 MiB runtime cap on P2MR stack plus altstack bytes, enforced under a new standard-only script flag. We are consolidating to one PR per v1.0.1 release-gate finding (see #174): #263 for #257, #266 for #258 and #265 for #259. Any test or design point from this branch that #263 lacks will be posted on #263 as review input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant