arkscript: Tighten custom policy validation

[sputn1ck]

What changed

• require custom predicates to parse as complete standalone script fragments
• reject unsupported tapscript opcodes in custom predicates
• restrict typed CSV values to canonical non-zero block delays
• apply the checks consistently across direct policy admission and the standard
  VTXO, vHTLC, and checkpoint builders
• add regression coverage for malformed predicates and noncanonical CSV
  encodings

Why

Policy templates should have one interpretation across semantic validation,
script generation, persistence, and recovery. These changes align compiler and
validator behavior so malformed or ambiguous custom templates fail closed.

Compatibility and rollout

Built-in policy templates and canonical production delays are unchanged.
Persisted custom templates that use unsupported predicate fragments or
noncanonical CSV values will no longer compile and should be audited before
rollout.

Checks

• go test ./lib/arkscript ./lib/tx/checkpoint ./lib/tx/oor
• make lint-changed-local
• make build
• make fmt-changed
• make tidy-module-check
• make commitmsg-lint range="origin/main..HEAD"
• git diff --check

[sputn1ck]

@claude please review

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[Copilot]

Pull request overview

This PR hardens the lib/arkscript Condition.Predicate escape hatch so opaque predicate bytes cannot (a) short-circuit tapscript execution via OP_SUCCESSx or (b) change parsing boundaries via incomplete pushes that consume the typed inner clause.

Changes:

• Validate Condition.Predicate as a complete standalone script fragment and reject any tapscript OP_SUCCESSx opcodes during Condition.Script() compilation.
• Make ValidatePolicy fail closed by compiling every leaf upfront (and rejecting nil leaves) before applying structural policy invariants.
• Add regression tests for OP_SUCCESSx opcodes, “OP_SUCCESS byte in pushdata”, and incomplete-push boundary smuggling; update predicate bytes in existing tests to remain valid under the new rules.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
lib/arkscript/validate.go	Compiles/leaves upfront (and rejects nil) so admission fails closed on unsafe/invalid predicate fragments.
lib/arkscript/validate_test.go	Adds policy-level regression for OP_SUCCESS predicate rejection; updates predicate bytes to avoid incomplete pushes.
lib/arkscript/policy_template_test.go	Updates test predicates to use a valid opcode (OP_NOP) under the new predicate completeness rules.
lib/arkscript/node.go	Adds predicate safety validation (complete fragment + reject OP_SUCCESSx) to Condition.Script().
lib/arkscript/node_test.go	Adds comprehensive predicate-safety regressions (all OP_SUCCESSx, pushed bytes, incomplete pushes).
docs/policy_arkscript_review_guide.md	Documents the predicate safety boundary and the new bypass prevention guarantees.
docs/arkscript_spec.md	Updates spec text to describe predicate validation constraints and removes brittle line references.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @sputn1ck's task in 4m 35s —— View job

---

Review: arkscript — reject unsafe condition predicates & noncanonical CSV locks

• Gather context (diff, changed files)
• Review node.go predicate + CSV safety changes
• Review validate.go / standard_vtxo.go / vhtlc.go / checkpoint.go
• Review tests and docs
• Post review

Overall this is a well-scoped, correct hardening change. Both invariants fail closed, the tests are thorough (exhaustive OP_SUCCESSx sweep, per-bit CSV sweep, push-boundary case), and the docs/CLAUDE/AGENTS metadata are kept in sync. I have no blocking concerns — a few notes below.

Strengths

• CSV cap at 65535 is a genuine correctness fix, not an arbitrary limit. Block-mode BIP-68 only has a 16-bit value field (wire.SequenceLockTimeMask). Any value above 65535 overflows into the type-flag / disable bits, so it never meant what a raw structural comparison assumed. validateCSVLock in lib/arkscript/node.go:111 correctly rejects 0, time-mode, disable, and reserved-high-bit encodings, and it is now threaded through every producer (CSV.Script, StandardVTXOTemplate, DecodeStandardVTXOParams, VHTLCOpts.validate, and via UnilateralCSVTimeoutTapLeaf the checkpoint builder). Good, consistent coverage.
• Predicate boundary defense is the right shape. Tokenizing the predicate to completion (lib/arkscript/node.go:169) closes the incomplete-trailing-push vector (the OP_DATA_34-swallows-inner case), and ScriptHasOpSuccess closes the immediate-success vector. Validating the predicate standalone before AddOps(predicate); AddOps(inner) is what makes the concatenation boundary safe. Built-in sha256Condition / AbsoluteLockTimeCondition predicates are complete fragments, so canonical vHTLC/VTXO shapes are unaffected (golden vectors still pass).
• ValidatePolicy now compiles every leaf (lib/arkscript/validate.go:60), so direct admission callers get the same fail-closed behavior as template compilation — plus a defensive nil-leaf check.

Notes / nits (non-blocking)

1. Redundant zero-checks. StandardVTXOTemplate (standard_vtxo.go:50) and VHTLCOpts.validate (vhtlc.go:339-347) still special-case == 0 immediately before calling validateCSVLock, which already rejects 0. Harmless, and the dedicated messages are arguably clearer, so keeping them is fine — just flagging the duplication.

2. Conditional-imbalance in predicates is not rejected, but is fail-closed. The tokenizer treats a lone OP_IF (no matching OP_ENDIF) as a "complete fragment," so such a predicate compiles. At execution the unbalanced conditional fails the script, so it cannot bypass or consume the inner clause — no security hole. Might be worth a one-line note in the spec's "predicate safety boundary" section that the guarantee is fail-closed, not well-formed-conditional, so a future reader doesn't assume more than the check provides.

3. Double compilation on the admission path. ValidatePolicy compiles each leaf and most callers compile again downstream. Negligible (admission-time, tiny scripts), just noting it's now 2× per leaf.

4. Coverage suggestion: the OP_SUCCESS test exercises single-byte predicates. Consider one case where a valid opcode precedes an OP_SUCCESSx (e.g. OP_1 <OP_SUCCESSx>) to confirm the check is position-independent, and one where an incomplete push and an OP_SUCCESS coexist (documents that the completeness error wins). Optional — current coverage is already strong.

Verified: predicate/CSV producers are consistently routed through the new validators, no non-test code constructs time-mode CSV locks (LockTimeToSequence(true, …) is unused outside tests), and the built-in predicates remain complete fragments. Did not re-run the suite locally (sandbox blocked go test); relying on the PR's stated make unit pkg=lib/arkscript pass.
· branch kon/arkscript-reject-op-success

[bhandras]

Reviewed at dfb8ad8b. Approving the client/compiler hardening.

The policy compiler now fails closed on the consensus-sensitive constructions in scope: incomplete predicate fragments, tapscript OP_SUCCESS opcodes, and noncanonical CSV encodings including the disable/type/reserved-bit cases. Direct policy validation compiles each leaf before reasoning about its typed structure, and the standard VTXO/vHTLC constructors apply the same canonical CSV guard.

The focused policy tests, full unit/race jobs, SQLite/PostgreSQL system tests, lint, static checks, and cross-compilation are green. The remaining scoped-doc advisory failure is an external review-action limit rather than a code failure.

This approval is for the client-side compiler and structural validator scope. Operator-specific policy terms such as the negotiated minimum exit window remain an admission-layer responsibility and should still be enforced by the service consuming these policies.