Repository navigation
Conversation
Add SCRIPT_VERIFY_P2MR_STACK_BYTES, a standard (non-mandatory) script verification flag that limits the total size of the stack and altstack to 1 MiB after each opcode of a P2MR v1 script. The 1,000-item runtime limit alone lets element-copying opcodes grow the interpreter stack to about 1,000 times the largest initial witness item. Block validity is unchanged; the flag applies to mempool admission and the other users of STANDARD_SCRIPT_VERIFY_FLAGS. The total is maintained by re-measuring only the stack elements an opcode can change, so the check adds constant work per opcode. Refs #257 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
SCRIPT_VERIFY_P2MR_STACK_BYTES failed with SCRIPT_ERR_STACK_SIZE, the error of the consensus 1,000-item stack limit. A mempool rejection then read "Stack size limit exceeded" whether the spend broke the policy byte limit or the consensus item limit. Add SCRIPT_ERR_P2MR_STACK_BYTES, "Standard P2MR stack byte limit exceeded", and return it when the byte limit is exceeded. The consensus item limit is checked first after each opcode, so a spend that breaks both on the same opcode still reports SCRIPT_ERR_STACK_SIZE. Refs #257 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The conformance oracle derived each resource case's policy result from its consensus result, so it could not express a policy-only limit, and the corpus had no case for the 1 MiB runtime stack-byte limit. Teach the oracle's evaluator OP_DUP, OP_DROP and an optional limit on stack bytes after each opcode. Add a runtime-stack-bytes resource kind that the oracle evaluates under consensus and policy separately, with boundary cases at exactly 1 MiB (standard) and 1 MiB plus one byte (consensus-valid, nonstandard with SCRIPT_ERR_P2MR_STACK_BYTES). Regenerate the boundary corpus and the manifest. The C++ consumer builds the same spend and checks both cases under consensus and standard flags. Refs #257 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Section 11 said a known leaf over any runtime resource limit is consensus-invalid, which does not hold for the policy-only 1 MiB stack-byte limit. Narrow that row to consensus limits, add a row for otherwise valid spends over the byte limit, and describe the policy check and its error in section 10. Refs #257 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review input from the duplicate branch for #257 (closed #269, Worth fixing before merge
Nice to have
Worth keeping from #263: the The tests, error code, policy doc and release note are on |
|
Merged #271 into this branch as a fast-forward (
This addresses points 1 and 3 of the earlier review comment. Required Merge Gate, Core Checks and Full Validation are now running on this head against |
The oracle cases added for the stack-byte limit changed src/test/data/p2mr_v1_manifest.json, so the draft support matrix still named the old corpus manifest digest and CheckedInP2MRIntegrationMatrixTest failed in Full Validation. Point the matrix and its generated markdown at the new digest. Every component is still not-tested or not-applicable, with no evidence, so no test result changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
This adds
SCRIPT_VERIFY_P2MR_STACK_BYTES, a standard, non-mandatory script verification flag. It limits the combined size of the stack and the altstack to 1 MiB after each opcode of a P2MR v1 script.This is a policy-only change with no consensus effect. Block validity is unchanged. The flag applies to mempool admission and to the other users of
STANDARD_SCRIPT_VERIFY_FLAGS.Why (#257):
What changed:
src/script/interpreter.h: addsSCRIPT_VERIFY_P2MR_STACK_BYTES = (1U << 23).src/script/script.h: addsMAX_STANDARD_P2MR_V1_STACK_BYTES = 1 MiB.static_assertchecks that the cap is at least 128 KiB of initial items plus 1,000 maximum-size (520-byte) pushes.src/script/interpreter.cpp:StackBytesFrom()and keeps the total incrementally inEvalScript.SCRIPT_ERR_STACK_SIZE.DUP/DROPpair. Instead, each opcode re-measures only the elements it can change: the top six stack elements (the depthOP_2ROTreaches) and the top altstack element.OP_ROLL: it only reorders the elements below its operand, so its window is the operand alone.OP_CHECKMULTISIG: P2MR rejects it before it touches the stack.src/policy/policy.h: the flag is added toSTANDARD_SCRIPT_VERIFY_FLAGS, so it is inSTANDARD_NOT_MANDATORY_VERIFY_FLAGS.TX_NOT_STANDARDwithmempool-script-verify-flag-failed (Stack size limit exceeded), and there is no peer penalty.GetBlockScriptFlagsis unchanged.doc/consensus/p2mr-v1.md: adds one table row: "Main stack plus alt-stack bytes after each opcode | 1 MiB | policy".Compatibility:
STANDARD_SCRIPT_VERIFY_FLAGS, so a wallet-built spend of a duplication-heavy leaf is reported incomplete, which matches relay.multi_aspend stays well under 1 MiB.Not part of this PR: a consensus-level limit. Making the flag mandatory from a height, or charging validation weight for copied bytes, would be a soft fork. It needs its own proposal, activation plan and release coordination.
Refs #257
Testing
Built with
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_GUI=OFF …(targetstest_qbit qbitd qbit-cli). Every result below is at this head,185836514a.New unit tests in
src/test/script_p2mr_tests.cpp:p2mr_dup_drop_leaf_runtime_stack_bytes: a duplication-heavy leaf is valid under consensus flags and rejected under the new flag and underSTANDARD_SCRIPT_VERIFY_FLAGS.p2mr_runtime_stack_bytes_boundary:DUP,2DUP,3DUP,OVER,2OVER,PICK,TUCKandIFDUP, with the altstack,ROLL,2ROT,2SWAP,ROT,SWAPandNIPin between.Results of
build/bin/test_qbit --run_test='script_p2mr_tests/p2mr_dup_drop_leaf_runtime_stack_bytes:script_p2mr_tests/p2mr_runtime_stack_bytes_boundary':Accounting cross-check (a temporary assert, not committed):
tracked == StackBytesFrom(stack, 0) + StackBytesFrom(altstack, 0)after every opcode under the flag.script_p2mr_tests,script_tests,transaction_tests,p2mr_datapqchash_tests,descriptor_p2mr_testsandpsbt_p2mr_testspass with it: 192 cases, 292,592 assertions.Regression:
build/bin/test_qbit --run_test='script_p2mr_tests:script_tests:transaction_tests:txvalidationcache_tests:pqc_sigcache_tests:p2mr_datapqchash_tests:descriptor_p2mr_tests:psbt_p2mr_tests:script_standard_tests:sigopcount_tests:miniscript_tests:script_p2sh_tests:script_segwit_tests:txpackage_tests:mempool_tests:feebumper_tests:pqc_tests'. All 296 cases pass, with 515,954 assertions.Functional:
test/functional/test_runner.py feature_p2mr.py, with the newtest_runtime_stack_bytes_policy_rejection.testmempoolacceptrejects the leaf with exactly that reason, and a block containing it is accepted bysubmitblock.feature_p2mr_activation,feature_p2mr_weight_activation,feature_p2mr_reorg,mempool_accept,wallet_p2mr,wallet_p2mr_psbtandrpc_packagesalso pass.Other changes:
src/test/transaction_tests.cpp: a flag-name map entry.src/test/fuzz/p2mr_script.cpp: the flag is added to the fuzzed flag sets. This was syntax-checked with the unit-test flags. The fuzz binary was not built.Not run: Windows and macOS, the fuzz binary, and a measurement of real-node memory while connecting a block.
Target Branch
mainor a maintainer-requested release branch such as0.1.x.This targets
1.x.xfor v1.0.1 (#181 prepares that release), and then 1.1.0.Risk / Review Notes
Notes:
MANDATORY_SCRIPT_VERIFY_FLAGSandGetBlockScriptFlagsare unchanged, so block validity is not affected.OP_ROLLas the one exception (a permutation below a one-element window).caseinEvalScriptwas checked against it, and the randomized cross-check found no mismatch.Assumeof the full re-sum would make that explicit. That is a candidate follow-up.Docs / Process Impact
Choose exactly one:
doc/consensus/p2mr-v1.mdlists the new 1 MiB policy limit. The v1.0.1 release notes should mention it as a new standardness rule.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.