taproot/script limits; default unknown-witness off; BIP8 stub #222

justinfilip · 2025-10-15T04:22:11Z

Policy items (each reject reason and where enforced).

Defaults and new flags (e.g., -v1perinputwitnesslimit, default -acceptunknownwitness=0).

Deployment stub: new deployment ID/name, TBD versionbits params, BIP8 delayed activation.

Testing plan: might add unit/functional tests for SPK size/push caps, tapscript IF-ban, push-run/IF-body, control block cap, per-input v1 cap.

Link to BIP-0444 PR/discussion.

… expose -datacarrierwitnesslimit and -v1perinputwitnesslimit

…n SetupServerArgs; remove stray scoped block in InitParameterInteraction

…ript push-run and IF-branch push-only guards; drop redundant element check

…ierwitnesslimit and field

…z tests

…mits) with TBD params and versionbits wiring

…ushlen<=256, disallow OP_IF in tapscript policy; default -acceptunknownwitness=false

kwsantiago

Needs work. Here are the main points:

No test coverage: No functional tests added for new policy rules
Performance concerns: Triple-parsing tapscripts (witness check, push runs, IF bodies) is inefficient. Should be single-pass.
Policy rationale undocumented: Why 1024, 256, 80, 257? Need comments linking to BIP-0444 or analysis.
Code duplication: Opcode parser copy-pasted 3+ times
Incomplete opcode handling: OP_0, OP_1NEGATE, OP_1-OP_16 not recognized as pushes

src/policy/policy.cpp

src/kernel/chainparams.cpp

src/kernel/mempool_options.h

src/validation.cpp

…n; constants for limits; bounds for -v1perinputwitnesslimit; fix whichType use order; add comments with BIP-0444 rationale

…itness default; note tapscript tests as follow-up

…per-input witness cap, IF ban, push-run cap, and IF-body cap

…n-witness rejection

justinfilip · 2025-10-15T16:03:39Z

All points should be addressed @kwsantiago, very much appreciate the feedback

kwsantiago

Excellent improvements @justinfilip !

Most previous issues were addressed. I left some remaining minor comments.

kwsantiago · 2025-10-15T16:30:07Z

src/policy/policy.cpp

+        // Small integer push has no explicit payload
+        return true;
+    }
+    size_t need = 0;


This is an unused variable?

kwsantiago · 2025-10-15T16:31:15Z

src/policy/policy.cpp

+        if (pos >= s.size()) return false;
+        pushed_len = s[pos++];
+    } else if (op == OP_PUSHDATA2) {
+        if (pos + 1 >= s.size()) return false;


if (pos + 1 >= s.size()) should be if (pos + 2 > s.size()) for consistency (needs 2 bytes). Same pattern in lines 73, 94, 95.

kwsantiago · 2025-10-15T16:32:30Z

src/policy/policy.cpp

+        pushed_len = s[pos++];
+    } else if (op == OP_PUSHDATA2) {
+        if (pos + 1 >= s.size()) return false;
+        pushed_len = s[pos] | (uint32_t(s[pos+1]) << 8);


On lines 70 and 74, casting to uint32_t then assigning to uint64_t pushed_len. If PUSHDATA4 has value > 2^32-1, the upper bits are lost.

Should cast directly to uint64_t.

kwsantiago · 2025-10-15T16:33:16Z

src/policy/policy.cpp

+    size_t payload = 0;
+    if (op >= 0x01 && op <= 0x4b) payload = op;
+    else if (op == OP_PUSHDATA1) { if (pos >= s.size()) return false; payload = s[pos++]; }
+    else if (op == OP_PUSHDATA2) { if (pos + 1 >= s.size()) return false; payload = s[pos] | (uint32_t(s[pos+1]) << 8); pos += 2; }


AdvanceOneOp repeats the same bounds checks as ParsePush.

Consider refactoring to share logic.

kwsantiago · 2025-10-15T16:34:35Z

src/policy/policy.cpp

+            } else if (op == OP_ELSE) {
+                if (!if_stack.empty()) {
+                    size_t body_start = if_stack.back();
+                    size_t body_end = j - 1;


When body_end == body_start, no branch is added, but this represents an empty IF body.

Should this be checked/rejected?

kwsantiago · 2025-10-15T16:39:40Z

src/test/bip444_tests.cpp

+    control[0] = 0xc0;
+
+    // Leaf with OP_IF containing 80-byte push-only body (would be rejected by IF ban first)
+    CScript leaf_if_80 = CScript() << OP_FALSE << OP_IF << std::vector<unsigned char>(80, 0) << OP_ENDIF << OP_TRUE;


On lines 228 and 237, these scripts use OP_FALSE << OP_IF which means the IF body never executes. The test is checking IF ban, not IF-body limit.

Consider testing actual IF-body limit separately if IF ban is lifted.

kwsantiago · 2025-10-15T16:40:03Z

src/init.cpp

        args.SoftSetArg("-blockmaxweight", "4000000");
    }

+


Empty lines added without purpose. Remove.

kwsantiago · 2025-10-15T16:40:41Z

src/node/mempool_args.cpp

    mempool_opts.persist_v1_dat = argsman.GetBoolArg("-persistmempoolv1", mempool_opts.persist_v1_dat);

+    // Policy flags to constrain per-input witness size for segwit v1
+    if (auto lim = argsman.GetIntArg("-v1perinputwitnesslimit")) {


User sets -v1perinputwitnesslimit=50, gets 128 with no warning.

Should log when clamping occurs or return error for out-of-range values.

kwsantiago · 2025-10-15T16:41:19Z

src/kernel/mempool_options.h

    bool permitephemeral_send{DEFAULT_PERMITEPHEMERAL_SEND};
    bool permitephemeral_dust{DEFAULT_PERMITEPHEMERAL_DUST};
    bool persist_v1_dat{DEFAULT_PERSIST_V1_DAT};
+    // Policy to limit inscription-like data in witnesses/tapscripts


"inscription-like data" is vague.

Be more specific: "Total witness bytes per v1 input to prevent witness stuffing/inscription abuse."

kwsantiago · 2025-10-15T16:42:02Z

doc/release-notes-bip444.md

+
+## References
+
+- BIP-0444: https://github.com/bitcoin/bips/blob/master/bip-0444.mediawiki (pending merge)


BIP-0444 link shows "(pending merge)" but links to non-existent file (404).

Update when BIP number is assigned or remove until finalized.

justinfilip · 2025-10-15T19:31:54Z

Excellent improvements @justinfilip !

Most previous issues were addressed. I left some remaining minor comments.

Appreciate the continual feedback and have noted the additional code review you've done.

Before I continue with addressing the code problems you've pointed out, I'd like to get your thoughts on my response in the BIP discussion: bitcoin/bips#2005 (comment)

kwsantiago · 2025-10-15T22:19:00Z

I'd like to get @luke-jr thoughts on moving forward with this on Knots, given that Core shut down the BIP discussion.

Justin Filip added 9 commits October 14, 2025 20:49

policy: add push-only witness filters and per-input v1 witness limit;…

4582579

… expose -datacarrierwitnesslimit and -v1perinputwitnesslimit

init: register -datacarrierwitnesslimit and -v1perinputwitnesslimit i…

4ab41d5

…n SetupServerArgs; remove stray scoped block in InitParameterInteraction

policy: wire witness standardness to mempool options; implement tapsc…

65e4bd0

…ript push-run and IF-branch push-only guards; drop redundant element check

policy: finalize witness standardness wiring; remove unused -datacarr…

ef9ad50

…ierwitnesslimit and field

tests: fix include style and usage for IsWitnessStandard(opts) in fuz…

9369191

…z tests

consensus: add BIP8 deployment stub for BIP-0444 (taproot_pushonly_li…

312f14d

…mits) with TBD params and versionbits wiring

policy: tighten defaults and filters: spk size<=34 (non-NULL_DATA), p…

46e9deb

…ushlen<=256, disallow OP_IF in tapscript policy; default -acceptunknownwitness=false

policy: cap Taproot control block size to 257 bytes

9b891c1

harmonize comments and names with expanded scope (script limits)

6de2837

kwsantiago suggested changes Oct 15, 2025

View reviewed changes

Justin Filip added 5 commits October 15, 2025 10:18

policy: refactor script parsing to helpers; single-pass tapscript sca…

b960a57

…n; constants for limits; bounds for -v1perinputwitnesslimit; fix whichType use order; add comments with BIP-0444 rationale

tests: add policy tests for scriptPubKey size/push caps and unknown w…

45599f5

…itness default; note tapscript tests as follow-up

tests: add BIP-0444 policy unit tests for taproot control block cap, …

1870ad6

…per-input witness cap, IF ban, push-run cap, and IF-body cap

doc: add release notes for BIP-0444 policy changes and default unknow…

54a587c

…n-witness rejection

chainparams: clarify signet uses ALWAYS_ACTIVE for easy BIP-0444 testing

6b17603

kwsantiago suggested changes Oct 15, 2025

View reviewed changes

luke-jr changed the title ~~BIP-0444 taproot/script limits; default unknown-witness off; BIP8 stub~~ taproot/script limits; default unknown-witness off; BIP8 stub Oct 15, 2025


		## References

		- BIP-0444: https://github.com/bitcoin/bips/blob/master/bip-0444.mediawiki (pending merge)

taproot/script limits; default unknown-witness off; BIP8 stub #222

Are you sure you want to change the base?

taproot/script limits; default unknown-witness off; BIP8 stub #222

Conversation

justinfilip commented Oct 15, 2025

Uh oh!

kwsantiago left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinfilip commented Oct 15, 2025

Uh oh!

kwsantiago left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinfilip commented Oct 15, 2025

Uh oh!

kwsantiago commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kwsantiago commented Oct 15, 2025 •

edited

Loading