arkscript: Tighten custom policy validation

docs/arkscript_spec.md

@@ -86,12 +86,12 @@ witness script on the stack).
 
 ```go
 type CSV struct {
-    Lock  uint32  // BIP-68 encoded sequence value
+    Lock  uint32  // canonical block-mode BIP-68 value, 1..65535
     Inner Node
 }
 ```
 
-Script encoding (from `CSV.Script`, `lib/arkscript/node.go:82-101`):
+Script encoding (from `CSV.Script` in `lib/arkscript/node.go`):
 
 ```
 <inner> <lock> OP_CHECKSEQUENCEVERIFY OP_DROP
@@ -100,7 +100,9 @@ Script encoding (from `CSV.Script`, `lib/arkscript/node.go:82-101`):
 At runtime the spending transaction's input `nSequence` must satisfy the
 BIP-68 relative-lock relation to the UTXO's confirmation height. Callers
 read the required sequence from a `SpendPath.RequiredSequence` computed by
-`DeriveSequence` (see §6).
+`DeriveSequence` (see §6). Ark policies support block-mode CSV only. Compilation
+rejects zero, the time-mode and disable flags, and every reserved high bit so
+policy validation compares exactly the value consensus enforces.
 
 #### `Condition` — generic predicate prefix
 
@@ -111,33 +113,37 @@ type Condition struct {
 }
 ```
 
-Script encoding (from `Condition.Script`, `lib/arkscript/node.go:120-141`):
+Script encoding (from `Condition.Script` in `lib/arkscript/node.go`):
 
 ```
 <predicate> <inner>
 ```
 
 `Condition` is the extension point for non-signature preconditions (hash
 locks, absolute locktimes, payment-hash preimages). Helper builders live in
-`lib/arkscript/node.go:148-190`:
+`lib/arkscript/node.go`:
 
 - `Hash160Condition(hash []byte)` — `HASH160 <hash> EQUALVERIFY`.
 - `AbsoluteLockTimeCondition(lock uint32)` — `<lock> CLTV DROP`.
 - `PaymentHash160Condition(paymentHash)` — Lightning payment-hash predicate
   that also enforces the 32-byte preimage-size rule.
 
 The predicate bytes are opaque to the AST walker for the purposes of
-`ContainsKey` and key extraction (`lib/arkscript/validate.go:208-218`). This
-is intentional: the AST reasons about *who can sign*, not about *what
-hashlock values are in play*.
+`ContainsKey` and key extraction in `lib/arkscript/validate.go`. This is
+intentional: the AST reasons about *who can sign*, not about *what hashlock
+values are in play*. Compilation still requires the predicate to be a complete
+script fragment and rejects every tapscript `OP_SUCCESSx` opcode. These checks
+prevent a raw prefix from consuming the typed inner clause as push data or
+making the leaf succeed before that clause executes.
 
 ### 2.3 What is NOT in the AST
 
 - No `OR` nodes. Ark expresses alternatives as separate tap leaves; the
   tapscript merkle tree already provides the OR semantics.
 - No `AND` nodes. Multisig is N-of-N; chain multiple signatures inside a
   single `Multisig`. CSV-gated signatures compose via `CSV{Inner: Multisig}`.
-- No custom opcode escape hatch. A future node kind requires a code change
+- No custom AST node escape hatch. `Condition` accepts raw predicate fragments,
+  subject to the safety checks above. A future node kind requires a code change
   in `lib/arkscript` and an encoding version bump (see §3.5).
 
 ---

docs/policy_arkscript_review_guide.md

@@ -147,7 +147,10 @@ the owner can recover funds without operator cooperation.
 
 `ExtractCSVDelay(node)` walks the AST for the outermost `CSV` node. If an exit
 leaf has no CSV wrapper, the policy is rejected. This is the critical safety
-property — see the security analysis below.
+property — see the security analysis below. Compilation also requires the CSV
+operand to be a canonical block-mode BIP-68 value in `1..65535`. This rejects
+the disable flag, time-mode flag, and reserved bits before structural policy
+validation compares the delay.
 
 ### Invariant 4: Minimum exit delay
 
@@ -213,7 +216,9 @@ unilateral refund leaf is reachable only after the invoice expiry.
    - The UTXO must have been confirmed for at least that many blocks.
 
    This means an exit spend is physically impossible until the CSV delay has
-   passed since the on-chain commitment.
+   passed since the on-chain commitment. The compiler's canonical block-mode
+   check is essential here: consensus treats a CSV operand with bit 31 set as
+   a NOP and masks reserved high bits during comparison.
 
 5. During that delay window, the operator can observe the unilateral exit
    attempt and broadcast the appropriate **forfeit transaction** (which uses a
@@ -239,10 +244,12 @@ correctly be flagged as an ungated exit leaf and rejected.
 
 **What about Condition predicates that embed keys?** The `Condition.Predicate`
 is opaque bytes — the validator does not parse it for operator keys. This is
-safe because the predicate only adds _restrictions_ (hashlocks, timelocks). A
-predicate cannot _grant_ spending authority — that comes from the `Inner`
-Multisig. The validator correctly checks only the Multisig nodes for key
-presence.
+safe because compilation rejects incomplete script fragments and all tapscript
+`OP_SUCCESSx` opcodes. An incomplete push could otherwise consume the typed
+inner script as data, while `OP_SUCCESSx` would make the leaf succeed before
+the inner signature checks execute. Once those bypasses are excluded, the
+predicate can only add conditions; spending authority still comes from the
+`Inner` Multisig.
 
 **Edge case: empty Multisig in exit leaf?** Impossible — `Multisig.Script()`
 returns an error if `len(Keys) == 0`, and `PolicyTemplate.Compile()` would
@@ -257,7 +264,8 @@ unspendable by anyone, which is safe (funds locked, not stolen).
 | Bypass CSV via key-path spend | Internal key is NUMS (unspendable)             |
 | Fake operator key in policy   | Operator validates own key presence at submit   |
 | Ungated exit leaf injection   | `ValidatePolicy` rejects non-CSV exit leaves   |
-| Predicate smuggling operator  | Predicates add restrictions, not authority      |
+| Predicate bypasses inner node | Compile rejects partial pushes and `OP_SUCCESSx` |
+| CSV flag or reserved-bit bypass | Compile permits only block delays `1..65535` |
 | Insufficient exit delay       | `MinExitDelay` check enforces operator minimum  |
 
 ---

lib/arkscript/AGENTS.md

@@ -74,6 +74,9 @@ validated invariants.
 - CSV encoding uses `blockchain.LockTimeToSequence(false, exitDelay)` to store
   the BIP-68 block-mode sequence value, not the raw block count. Decoders that
   compare raw block counts against CSV lock values must convert accordingly.
+- CSV values are canonical non-zero block-mode encodings in `1..65535`.
+  Time-mode, disable, and reserved high bits are rejected so structural delay
+  comparisons cannot diverge from the value enforced by consensus.
 - Canonical leaf ordering: sorted by version then lexicographic script bytes.
 - All taproot outputs use the unspendable ARK NUMS key for key path (no
   key-path spend possible).

lib/arkscript/CLAUDE.md

@@ -74,6 +74,9 @@ validated invariants.
 - CSV encoding uses `blockchain.LockTimeToSequence(false, exitDelay)` to store
   the BIP-68 block-mode sequence value, not the raw block count. Decoders that
   compare raw block counts against CSV lock values must convert accordingly.
+- CSV values are canonical non-zero block-mode encodings in `1..65535`.
+  Time-mode, disable, and reserved high bits are rejected so structural delay
+  comparisons cannot diverge from the value enforced by consensus.
 - Canonical leaf ordering: sorted by version then lexicographic script bytes.
 - All taproot outputs use the unspendable ARK NUMS key for key path (no
   key-path spend possible).

lib/arkscript/checkpoint.go

@@ -20,11 +20,8 @@ type CheckpointPolicy struct {
 	// CSV unroll leaf.
 	OperatorKey *btcec.PublicKey
 
-	// CSVDelay is the relative timelock enforced by the
-	// operator-controlled leaf.
-	//
-	// This is a raw BIP-68 sequence value interpreted by
-	// OP_CHECKSEQUENCEVERIFY.
+	// CSVDelay is the canonical block delay enforced by the
+	// operator-controlled leaf. It must be in the range 1..65535.
 	CSVDelay uint32
 }
 

lib/arkscript/node.go

@@ -6,6 +6,7 @@ import (
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcec/v2/schnorr"
 	"github.com/btcsuite/btcd/txscript/v2"
+	"github.com/btcsuite/btcd/wire/v2"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/lntypes"
 )
@@ -70,7 +71,7 @@ func (m *Multisig) nodeSealed() {}
 // The inner expression is evaluated first, then the CSV check is performed.
 // Canonical encoding: <inner> <lock> OP_CHECKSEQUENCEVERIFY OP_DROP.
 type CSV struct {
-	// Lock is the BIP-68 encoded relative locktime value.
+	// Lock is a canonical block-mode BIP-68 delay in the range 1..65535.
 	Lock uint32
 
 	// Inner is the expression to gate with the timelock.
@@ -82,6 +83,9 @@ func (c *CSV) Script() ([]byte, error) {
 	if c.Inner == nil {
 		return nil, fmt.Errorf("csv: inner node is nil")
 	}
+	if err := validateCSVLock(c.Lock); err != nil {
+		return nil, err
+	}
 
 	// Compile the inner expression first.
 	innerScript, err := c.Inner.Script()
@@ -99,6 +103,22 @@ func (c *CSV) Script() ([]byte, error) {
 	return builder.Script()
 }
 
+// validateCSVLock ensures a CSV operand is the canonical BIP-68 encoding of a
+// non-zero block delay. Ark policies do not support time-mode CSV values, and
+// accepting any high bit would let the raw validator comparison diverge from
+// consensus. In particular, bit 31 makes OP_CHECKSEQUENCEVERIFY a NOP, while
+// reserved bits are masked away during script execution.
+func validateCSVLock(lock uint32) error {
+	const maxBlockDelay = uint32(wire.SequenceLockTimeMask)
+
+	if lock == 0 || lock > maxBlockDelay {
+		return fmt.Errorf("csv: lock must be a canonical block delay "+
+			"in range [1, %d], got 0x%08x", maxBlockDelay, lock)
+	}
+
+	return nil
+}
+
 // nodeSealed implements the Node interface.
 func (c *CSV) nodeSealed() {}
 
@@ -124,6 +144,9 @@ func (c *Condition) Script() ([]byte, error) {
 	if len(c.Predicate) == 0 {
 		return nil, fmt.Errorf("condition: predicate script is empty")
 	}
+	if err := validateConditionPredicate(c.Predicate); err != nil {
+		return nil, err
+	}
 
 	innerScript, err := c.Inner.Script()
 	if err != nil {
@@ -138,6 +161,27 @@ func (c *Condition) Script() ([]byte, error) {
 	return builder.Script()
 }
 
+// validateConditionPredicate ensures a raw predicate cannot change how the
+// typed inner clause is parsed or bypass its execution. In particular, an
+// incomplete data push could consume the inner script as pushed bytes, while
+// OP_SUCCESS would make the entire tapscript succeed before the inner clause
+// executes.
+func validateConditionPredicate(predicate []byte) error {
+	tokenizer := txscript.MakeScriptTokenizer(0, predicate)
+	for tokenizer.Next() {
+	}
+	if err := tokenizer.Err(); err != nil {
+		return fmt.Errorf("condition: predicate is not a complete "+
+			"script fragment: %w", err)
+	}
+	if txscript.ScriptHasOpSuccess(predicate) {
+		return fmt.Errorf("condition: predicate contains OP_SUCCESS " +
+			"opcode")
+	}
+
+	return nil
+}
+
 // nodeSealed implements the Node interface.
 func (c *Condition) nodeSealed() {}
 

lib/arkscript/node_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcec/v2/schnorr"
 	"github.com/btcsuite/btcd/txscript/v2"
+	"github.com/btcsuite/btcd/wire/v2"
 	"github.com/lightninglabs/wavelength/internal/testutils"
 	"github.com/stretchr/testify/require"
 )
@@ -124,6 +125,42 @@ func TestCSVNilInner(t *testing.T) {
 	require.Contains(t, err.Error(), "inner node is nil")
 }
 
+// TestCSVRejectsNonCanonicalLock verifies that CSV compilation accepts only
+// canonical, non-zero block-mode BIP-68 values. Every bit above the consensus
+// value mask either changes units, disables CSV, or is ignored by consensus.
+func TestCSVRejectsNonCanonicalLock(t *testing.T) {
+	t.Parallel()
+
+	key, _ := testutils.CreateKey(1)
+	compile := func(lock uint32) error {
+		node := &CSV{
+			Lock: lock,
+			Inner: &Multisig{
+				Keys: []*btcec.PublicKey{
+					key,
+				},
+			},
+		}
+
+		_, err := node.Script()
+
+		return err
+	}
+
+	require.ErrorContains(t, compile(0), "canonical block delay")
+
+	for bit := 16; bit < 32; bit++ {
+		lock := uint32(144) | uint32(1)<<bit
+		err := compile(lock)
+		require.ErrorContains(
+			t, err, "canonical block delay", "bit %d", bit,
+		)
+	}
+
+	require.NoError(t, compile(1))
+	require.NoError(t, compile(uint32(wire.SequenceLockTimeMask)))
+}
+
 // TestAbsoluteLockTimeCondition tests the opaque absolute locktime prefix.
 func TestAbsoluteLockTimeCondition(t *testing.T) {
 	t.Parallel()
@@ -207,6 +244,91 @@ func TestConditionNilInner(t *testing.T) {
 	require.Contains(t, err.Error(), "inner node is nil")
 }
 
+// TestConditionRejectsOpSuccess verifies that opaque predicates cannot use
+// tapscript's immediate-success opcodes to bypass the typed inner clause.
+func TestConditionRejectsOpSuccess(t *testing.T) {
+	t.Parallel()
+
+	key, _ := testutils.CreateKey(1)
+	checked := 0
+
+	for opcode := 0; opcode <= 0xff; opcode++ {
+		predicate := []byte{byte(opcode)}
+		if !txscript.ScriptHasOpSuccess(predicate) {
+			continue
+		}
+
+		checked++
+		node := &Condition{
+			Predicate: predicate,
+			Inner: &Multisig{
+				Keys: []*btcec.PublicKey{
+					key,
+				},
+			},
+		}
+
+		_, err := node.Script()
+		require.Error(t, err, "opcode 0x%x", opcode)
+		require.Contains(
+			t, err.Error(),
+			"OP_SUCCESS", "opcode 0x%x", opcode,
+		)
+	}
+
+	require.NotZero(t, checked)
+}
+
+// TestConditionAllowsOpSuccessByteInPushData verifies that an OP_SUCCESS byte
+// carried as push data is not mistaken for an executable opcode.
+func TestConditionAllowsOpSuccessByteInPushData(t *testing.T) {
+	t.Parallel()
+
+	predicate, err := txscript.NewScriptBuilder().
+		AddData([]byte{txscript.OP_RESERVED}).
+		AddOp(txscript.OP_DROP).
+		Script()
+	require.NoError(t, err)
+	require.False(t, txscript.ScriptHasOpSuccess(predicate))
+
+	key, _ := testutils.CreateKey(1)
+	node := &Condition{
+		Predicate: predicate,
+		Inner: &Multisig{
+			Keys: []*btcec.PublicKey{
+				key,
+			},
+		},
+	}
+
+	_, err = node.Script()
+	require.NoError(t, err)
+}
+
+// TestConditionRejectsIncompletePush verifies that a predicate cannot consume
+// the typed inner script as data by ending with an incomplete push opcode.
+func TestConditionRejectsIncompletePush(t *testing.T) {
+	t.Parallel()
+
+	key, _ := testutils.CreateKey(1)
+	node := &Condition{
+		// A single-key Multisig compiles to exactly 34 bytes. Without
+		// standalone predicate validation, this opcode consumes that
+		// entire signature clause as push data.
+		Predicate: []byte{
+			txscript.OP_DATA_34,
+		},
+		Inner: &Multisig{
+			Keys: []*btcec.PublicKey{
+				key,
+			},
+		},
+	}
+
+	_, err := node.Script()
+	require.ErrorContains(t, err, "not a complete script fragment")
+}
+
 // TestASTMatchesGoldenVectors verifies that the AST produces byte-identical
 // scripts to the golden test vectors from the current implementation.
 func TestASTMatchesGoldenVectors(t *testing.T) {