refactor: Clean exitPaths in base.ts

[shubertm]

@louisinger, @Kukks, @pietro909

Summary by CodeRabbit

• Refactor
  • Centralized and standardized script validation with shared pre-check helpers, simplifying decode flows and reducing duplicated logic.
• Bug Fixes
  • Reduced silent failures: decoding now performs explicit validity checks and logs failures for easier diagnosis.
• Tests
  • Added a test verifying encoding/decoding of time-locked multisig scripts to ensure timelock and keys round-trip correctly.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0cb473d6-fdfe-4679-8d43-bd11dfc796a7

📥 Commits

Reviewing files that changed from the base of the PR and between 63f3e1e and 8ee0e0a.

📒 Files selected for processing (3)

• src/script/base.ts
• src/script/tapscript.ts
• test/tapscript.test.ts

✅ Files skipped from review due to trivial changes (1)

• test/tapscript.test.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• src/script/base.ts
• src/script/tapscript.ts

---

Walkthrough

Compute each tap leaf script once and select decoders via new isScriptValid checks instead of exception-driven decode fallbacks; tapscript decoders gained centralized validation helpers (including getVerifyIndex and CLTV locktime handling), and per-leaf decode errors are caught and logged with console.debug.

Changes

Cohort / File(s)	Summary
Tapscript validation & decoders src/script/tapscript.ts	Added exported isScriptValid(script) for CSVMultisigTapscript, ConditionCSVMultisigTapscript, ConditionMultisigTapscript, and CLTVMultisigTapscript; introduced getVerifyIndex(asm); moved minimal-structure, VERIFY/CSV/CLTV checks into validators; decoders now call validators up-front; CLTV locktime handling accepts numeric locktimes via BigInt or decodes via MinimalScriptNum.
Exit path decoding / leaf selection src/script/base.ts	Changed per-leaf decode flow to compute scriptFromTapLeafScript(leaf) once and choose decoder using isScriptValid() rather than relying on thrown exceptions; per-leaf errors are caught and logged with console.debug.
Tests test/tapscript.test.ts	Added CLTVMultisigTapscript test that encodes and decodes a tapscript with absoluteTimelock = BigInt(16), asserting timelock and pubkeys match.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant VtxoScript
participant LeafParser as scriptFromTapLeafScript
participant Validator as isScriptValid
participant CSVDecoder as CSVMultisigTapscript.decode
participant CondCSVDecoder as ConditionCSVMultisigTapscript.decode
VtxoScript->>LeafParser: extract leaf script
LeafParser-->>VtxoScript: script
VtxoScript->>Validator: CSV.isScriptValid(script)?
alt CSV valid
VtxoScript->>CSVDecoder: decode(script)
CSVDecoder-->>VtxoScript: decoded exit path
else CSV not valid
VtxoScript->>Validator: ConditionCSV.isScriptValid(script)?
alt ConditionCSV valid
VtxoScript->>CondCSVDecoder: decode(script)
CondCSVDecoder-->>VtxoScript: decoded exit path
else neither valid
VtxoScript-->>VtxoScript: skip leaf
end
end
Note right of VtxoScript: per-leaf try/catch logs decode errors with console.debug

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Fix ScriptNum minimal encoding, improve vhtlc tests #123 — modifies tapscript CSV/CLTV multisig encode/decode logic and uses MinimalScriptNum-based decoding similar to changes here.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title mentions only base.ts exitPaths cleanup, but the actual changes span three files with substantial work on tapscript validation logic, not just base.ts refactoring.	Revise the title to reflect the main change: something like 'refactor: Refactor tapscript validation with isScriptValid helpers' or 'refactor: Introduce script validation checks in tapscript handlers'.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/script/base.ts`:
- Around line 129-138: The truthiness check on
CSVMultisigTapscript.isScriptValid and
ConditionCSVMultisigTapscript.isScriptValid is incorrect because those functions
can return Error objects (which are truthy); change each conditional to first
assign the result to a variable (e.g., const isValid =
CSVMultisigTapscript.isScriptValid(script)) and then explicitly check if isValid
is an instance of Error (skip/continue) or if it is === true before calling
CSVMultisigTapscript.decode(script); apply the same pattern for
ConditionCSVMultisigTapscript to avoid calling decode when validation failed.

In `@src/script/tapscript.ts`:
- Around line 744-747: The error message for the locktime type check is
misleading: when inspecting const locktime = asm[0] the code rejects strings and
numbers but currently says "expected locktime number"; change that message to
accurately state the expected type (e.g., "Invalid script: expected locktime as
Uint8Array" or "Invalid script: expected locktime buffer") so it matches the
expectation of MinimalScriptNum.decode and clarifies that a Uint8Array/Buffer is
required; update the message in the locktime type-check branch that references
locktime/asm[0].
- Around line 416-418: In isScriptValid replace the thrown exception with an
Error return so the function respects its boolean | Error contract: change the
branch that currently does "throw new Error(`Invalid script: too short (expected
at least 3)`)” to instead "return new Error(...)" so callers receive an Error
value; locate this in the asm length check inside isScriptValid in tapscript.ts
and mirror the style used by the other validation branches.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e8085457-74a8-43b6-ad60-0930e21fbb93

📥 Commits

Reviewing files that changed from the base of the PR and between d48fcf1 and bf0927e.

📒 Files selected for processing (2)

• src/script/base.ts
• src/script/tapscript.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/script/tapscript.ts`:
- Around line 744-747: The validation in Script.decode erroneously rejects
numeric locktimes (asm[0]) even though encode emits a number for single-byte
locktimes; update Script.decode to accept both number and byte-array forms for
locktime (do not treat typeof locktime === "number" as invalid) and mirror
CSVMultisigTapscript.isScriptValid/its decode handling: allow numbers, reject
only strings, and normalize the numeric case into the same internal
representation used elsewhere (or convert numbers to the expected byte
representation before further processing).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1f671979-c4aa-44ce-ae18-e11aed0f2076

📥 Commits

Reviewing files that changed from the base of the PR and between bf0927e and 5227ef8.

📒 Files selected for processing (2)

• src/script/base.ts
• src/script/tapscript.ts

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/script/tapscript.ts`:
- Around line 535-540: The reverse scan that finds the delimiter currently
continues after finding a "VERIFY", causing verifyIndex to point to the leftmost
match; change the loop that sets verifyIndex in the function using the asm
reverse-scan so it breaks immediately when asm[i] === "VERIFY" (i.e., stop on
the first match from the end) and apply the identical early-break change to the
reverse-scans in ConditionCSVMultisigTapscript.decode() and
ConditionMultisigTapscript.decode() so validation and decoding use the same
rightmost VERIFY delimiter.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 30fee71b-646b-4189-b52c-099eb3ea2b67

📥 Commits

Reviewing files that changed from the base of the PR and between 5227ef8 and e8e6744.

📒 Files selected for processing (1)

• src/script/tapscript.ts

[arkanaai]

🔍 Review: refactor: Clean exitPaths in base.ts

Overall: Solid refactor — extracting validation into isScriptValid() pre-checks is cleaner than exception-driven control flow. A few items to flag:

✅ Good changes

• Pre-validation via isScriptValid() before decode() eliminates exception-as-control-flow pattern
• CLTVMultisigTapscript now correctly handles number-typed locktimes (previously threw on them) — this was a latent bug
• Consistent pattern across all tapscript namespaces

⚠️ Issues to consider

1. ConditionMultisigTapscript reuses ConditionCSVMultisigTapscript.isScriptValid

import isScriptValid = ConditionCSVMultisigTapscript.isScriptValid;

This works today because both have identical validation (find VERIFY opcode, length ≥ 1). But it creates a silent coupling — if CSV-specific checks are added to ConditionCSVMultisigTapscript.isScriptValid later, they'll leak into ConditionMultisigTapscript. Consider giving ConditionMultisigTapscript its own isScriptValid even if it's identical for now.

2. ConditionCSVMultisigTapscript.decode() — verifyIndex can be -1 after validation removal
After isScriptValid() passes (which checks VERIFY exists), decode() re-scans for verifyIndex. This is safe since isScriptValid guarantees VERIFY is present. However, the removed -1 guard at line 484 means if isScriptValid is ever bypassed or the method is called directly without pre-validation, asm.slice(0, -1) would silently return almost the full array — a subtle failure mode. The same applies to ConditionMultisigTapscript.decode().

3. Empty catch in VtxoScript.exitPaths()

} catch (e) {}

The old nested try-catch also swallowed errors, so this isn't a regression — but now that validation is separate, decode() should only throw for truly unexpected issues (e.g., corrupted multisig data). A console.warn or debug log here could help surface malformed scripts during development.

4. boolean | Error return type is unconventional
Callers must check isValid instanceof Error vs === true. Consider returning true | Error for type narrowing, or a plain boolean (since the Error objects aren't used beyond re-throwing in decode).

Security note

The exit path decoding is critical for VTXO validation. The refactored validation logic preserves all original checks. The CLTVMultisigTapscript locktime fix is actually an improvement — previously valid number-typed locktimes were incorrectly rejected, which could cause valid CLTV exit paths to be silently dropped.

[arkanaai]

🔍 Arkana Review — clean-exit-path

Overall: Good refactor moving from exception-driven flow control to explicit validation. A few issues to flag.

✅ What looks good

1. VtxoScript.exitPaths in base.ts: Replacing the nested try/catch cascade with isScriptValid() pre-checks is cleaner and more predictable. The previous pattern used exceptions for flow control (try CSVMultisig → catch → try ConditionCSVMultisig → catch), which made debugging difficult and had performance overhead.

2. CSVMultisigTapscript.isScriptValid and CLTVMultisigTapscript.isScriptValid: Returning boolean | Error is a reasonable pattern — callers can check instanceof Error for the reason, or just check truthiness. The validation logic correctly mirrors the original decode preconditions.

3. getVerifyIndex extraction: DRYing up the VERIFY opcode search into a helper. The early return on finding the index is a minor efficiency improvement over the original which continued scanning backwards.

⚠️ Issues to address

1. Inconsistent isScriptValid return types (medium)

CSVMultisigTapscript.isScriptValid and CLTVMultisigTapscript.isScriptValid return boolean | Error, while ConditionCSVMultisigTapscript.isScriptValid and ConditionMultisigTapscript.isScriptValid return true but throw on invalid input. This is an inconsistent API contract.

The call sites also differ: base.ts uses if (CSVMultisigTapscript.isScriptValid(script) === true) but ConditionCSVMultisigTapscript.decode calls isScriptValid(script) and lets it throw. Pick one pattern — the boolean | Error return is safer for validation, throwing is simpler for decode preconditions. I'd suggest making them all return boolean | Error since these are "isValid" functions, and having the decode methods throw when the returned value is an Error.

2. CLTVMultisigTapscript locktime type change (review carefully)

The original code rejected number type for locktime. The new isScriptValid only rejects string, and decode now handles number by converting via BigInt(locktime). This is a behavioral change — was number-type locktime previously considered invalid for a reason? If small locktimes fit in a JS number, this is correct. But verify this doesn't break the reconstructed comparison at the end of decode, since MinimalScriptNum.decode vs BigInt() could yield different representations.

3. ConditionCSVMultisigTapscript.decode — validation result ignored

The call isScriptValid(script) without checking the result relies on the throw behavior. This works but is confusing given the function name suggests it returns a validity check. If the pattern is to throw, the function should be named something like assertScriptValid.

🔒 Security note

This code handles VTXO script decoding — it's on the critical path for validating exit paths. The logic changes are mechanical (moving validation into separate functions), so the security properties should be preserved. However, the locktime type relaxation in CLTVMultisigTapscript warrants extra testing to ensure no edge cases allow malformed scripts to pass validation.

[arkanaai]

Review: Clean exitPaths in base.ts

Protocol correctness: ✅
The refactor improves exit path validation without altering protocol behavior. Pre-checks via isScriptValid() make failures explicit rather than silent.

Security analysis:

• Script validation: isScriptValid() extracts all pre-decode checks for both CSV and non-CSV branches — good defensive pattern. Prevents malformed script decoding.
• Early exit behavior: Old code silently continue on decode failures; new code adds console.debug() logging. This improves observability without changing semantic behavior.
• Exit path integrity: No changes to the actual VTXO script structure or verification logic — purely refactored validation flow.

Key improvements:

1. DRY principle: isScriptValid() eliminates duplicate validation across three namespaces (CSVMultisigTapscript, ConditionCSVMultisigTapscript, ConditionMultisigTapscript)
2. Better error paths: Returns true | Error instead of throwing, allowing callers to decide handling strategy
3. Logging on failures: console.debug("Failed to decode script", e) helps diagnose malformed or unexpected scripts in production

Minor note: Ensure the getVerifyIndex() helper in ConditionCSVMultisigTapscript and ConditionMultisigTapscript has early return on match (it does — good catch at line 531).

✅ Ready to merge.