fix: harden client-side validation of counterparty data

[Kukks]

Three client-side gaps on data that arrives from the counterparty. All of this is in pkg/client-lib / pkg/ark-lib validation used by clients; TxTree.Validate() is not called by the server's round processing, so arkd's own path is untouched.

Sighash policy. verifyOffchainTx verified a counterparty signature under the sighash type that same counterparty declared. It now checks the declared type against an allowlist, requires it to match the tx we built ourselves, and computes the verification hash from our own value rather than theirs. finalizeTx additionally refuses to sign a returned checkpoint declaring anything but SIGHASH_DEFAULT — there is no locally built checkpoint to compare against there, and we never stamp one ourselves. That guard also covers the previously unchecked pending-tx path.

Tree finality. TxTree.Validate() checked structure, version and amounts but not whether a node could actually be broadcast. It now also requires a zero locktime and a final sequence on every node. The operator-side builder already emits exactly these values.

Validate before signing. The vtxo tree was only validated at batch finalization — after nonces and signatures had already been submitted. The checks that need nothing but the tree and the commitment tx are now grouped and run at signing-start, before any nonce goes out. They still re-run at finalization against that event's commitment tx, so no check was removed or weakened.

Test plan

• New unit tests per fix, including accept cases. Disabling each check individually makes its own tests fail and leaves the accept cases passing; with the finalizeTx guard disabled the malicious PSBT reaches the signer with no error at all.
• The finality tests walk the full graph (up to 255 vtxo-tree and 171 connector-tree nodes per vector), so recursion is covered rather than just roots.
• go build ./... clean across all modules. go test ./... unchanged against baseline: same 8 failing packages and same 11 failing tests before and after, all environmental here (Redis, Postgres, Docker e2e, Windows temp-dir locks). Both modules touched (pkg/ark-lib, pkg/client-lib) were green at baseline and remain green.

No e2e run — the regtest stack needs Docker and already fails at baseline in this environment. Worth a run against a live stack before merge.

Summary by CodeRabbit

• Security

  • Strengthened transaction signing safeguards by rejecting unsupported or mismatched signature-hash settings.
  • Prevented signing or finalizing transactions that fail required validation checks.

• Bug Fixes

  • Added validation to ensure transaction trees are final and can be broadcast promptly after their parent confirms.
  • Transactions with invalid locktimes, sequences, funding, recipient amounts, or checkpoint data are now rejected before signing.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 12632e15-bc65-4a8c-8872-08748f7abf85

📥 Commits

Reviewing files that changed from the base of the PR and between f863e48 and 3ffee5c.

📒 Files selected for processing (6)

• pkg/ark-lib/tree/tx_tree.go
• pkg/ark-lib/tree/tx_tree_finality_test.go
• pkg/client-lib/batch-session/handler/default_handler.go
• pkg/client-lib/batch-session/handler/default_handler_test.go
• pkg/client-lib/offchain-tx/sighash_test.go
• pkg/client-lib/offchain-tx/utils.go

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

Walkthrough

The change enforces final transaction trees, validates VTXO trees before signing, revalidates commitment transactions during finalization, and restricts sighash types in offchain transaction verification and finalization.

Changes

Transaction validation

Layer / File(s)	Summary
Transaction tree finality checks pkg/ark-lib/tree/tx_tree.go, pkg/ark-lib/tree/tx_tree_finality_test.go	Validate rejects non-zero locktimes and non-maximum input sequences. Tests apply these mutations to every VTXO and connector tree node.
Pre-signing VTXO tree validation pkg/client-lib/batch-session/handler/default_handler.go, pkg/client-lib/batch-session/handler/default_handler_test.go	The handler validates the VTXO tree before signer initialization and nonce submission. Finalization revalidates the commitment transaction. Tests cover valid trees and invalid outpoints, sequences, and receiver amounts.
Offchain sighash policy pkg/client-lib/offchain-tx/utils.go, pkg/client-lib/offchain-tx/sighash_test.go	Verification rejects forbidden or mismatched sighash types. Finalization requires SIGHASH_DEFAULT on server-returned checkpoints. Tests cover rejected and permitted types.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: 🔵 Low · up to 3ffee

The PR strengthens counterparty transaction validation before signing and nonce release. It is mergeable with owner awareness that parallel nonce publication can leave partial signing state after a failed submission, making cleanup or retry non-atomic.

Sequence Diagram(s)

sequenceDiagram
  participant OnTreeSigningStarted
  participant validateVtxoTreeAgainstCommitmentTx
  participant TxTreeValidate
  participant SignerSession
  participant Client
  OnTreeSigningStarted->>validateVtxoTreeAgainstCommitmentTx: validate commitment and VTXO tree
  validateVtxoTreeAgainstCommitmentTx->>TxTreeValidate: validate tree structure and finality
  TxTreeValidate-->>OnTreeSigningStarted: validation result
  OnTreeSigningStarted->>SignerSession: initialize signing session
  OnTreeSigningStarted->>Client: submit tree nonces

Loading

Suggested reviewers: altafan

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 69.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: stronger client-side validation of counterparty data across transaction trees, PSBTs, and signing flows.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/tx-validation-hardening

---

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.}

[Kukks]

Build and Scan red here is pre-existing and unrelated to this change — flagging so it isn't read as a regression.

The failing step is the Trivy image scan (severity: CRITICAL,HIGH, exit-code: 1), on a single finding:

golang.org/x/crypto  CVE-2026-56854  CRITICAL  installed v0.53.0  fixed 0.55.0
golang.org/x/crypto/ssh: Authentication bypass due to unenforced source-address restrictions

This PR changes no dependencies — the diff is 6 files, none of them go.mod or go.sum:

pkg/ark-lib/tree/tx_tree.go
pkg/ark-lib/tree/tx_tree_finality_test.go
pkg/client-lib/batch-session/handler/default_handler.go
pkg/client-lib/batch-session/handler/default_handler_test.go
pkg/client-lib/offchain-tx/sighash_test.go
pkg/client-lib/offchain-tx/utils.go

Trivy Security Scan on master already failed on 2026-09-01, before this branch existed. Clearing it needs a golang.org/x/crypto bump to ≥0.55.0, which seemed wrong to fold into an unrelated security fix — happy to do it separately if you'd prefer it here.

[arkana-ai-bot]

Arkana review — PROTOCOL CRITICAL

Two separate hardening changes bundled together, both correct.

1. TxTree.Validate() — finality checks

Adds locktime=0 and sequence=MaxTxInSequenceNum checks to every node. The motivation is sound: a malicious server could propose a vtxo tree with locked transactions that the client pre-signs without noticing; when the client later tries to unroll, Bitcoin would refuse to relay until the timelock expires, giving the server time to sweep. The fix ensures the client catches this before committing any nonces.

Both checks recurse via the existing node-walk in Validate(), which the new test confirms by mutating each node and expecting errors.

2. validateVtxoTreeAgainstCommitmentTx called before nonce submission

Previously the full vtxo tree validation happened in OnBatchFinalization (after signing). Moving it to OnTreeSigningStarted means the client refuses to emit nonces for a tree it hasn't checked. This is the right order: you validate before you commit.

The refactor extracts the subset of checks that need only the commitment PSBT into validateVtxoTreeAgainstCommitmentTx, and calls it twice (once before nonces, once at finalization against the potentially different finalization commitment tx). The double-check at finalization is correct and needed.

Review notes

• The recordingSignerSession / recordingClient test harness cleanly verifies that submittedNonces stays 0 when the tree fails validation — exactly the right assertion.
• No issues found.

Ready to merge. Human sign-off recommended given this changes the client signing session protocol.

[arkana-ai-bot]

Review — arkd#1176: harden client-side validation of counterparty data

Three independent fixes, all correct. Detailed notes below.

---

1. Sighash policy in verifyOffchainTx — pkg/client-lib/offchain-tx/utils.go

The bug (pre-patch): CalcTapscriptSignaturehash was called with signedInput.SighashType — a value declared by the counterparty in their returned PSBT. A counterparty could declare SIGHASH_NONE or SIGHASH_SINGLE, receive a signature that doesn't commit to the outputs, and replay it on a different transaction.

The fix: Two new gates before the signature check:

1. checkSighashType allowlists SigHashDefault, SigHashAll, and SigHashAll|AnyOneCanPay.
2. signedInput.SighashType != originalInput.SighashType rejects any deviation from what we built.
3. CalcTapscriptSignaturehash is then called with originalInput.SighashType (our value, not theirs).

The allowlist in checkSighashType (utils.go, new function) correctly excludes SIGHASH_NONE and SIGHASH_SINGLE. SIGHASH_ALL and SIGHASH_ALL|ANYONECANPAY are allowed through the first gate, but SigHashAll (value 1) vs SigHashDefault (value 0) will be caught by the mismatch check — the test mismatch in TestVerifySignedTxSighashPolicy confirms this path.

✅ Fix is correct. Tests cover forbidden types, mismatch, and the accept case.

---

2. Checkpoint sighash guard in finalizeTx — pkg/client-lib/offchain-tx/utils.go

checkCheckpointSighashTypes rejects any input.SighashType != SigHashDefault on checkpoints returned by the server. The PR description correctly identifies that there is no locally-built checkpoint to compare against here (unlike the submit path), so the allowlist is intentionally stricter: SIGHASH_DEFAULT only.

This guard also covers the FinalizePendingTxs path (pending.go:40), which calls finalizeTx directly without a prior VerifySignedCheckpointTxs. That path was the only one where checkpoint signing had no sighash guard at all.

✅ Fix is correct. TestFinalizeTxSighashPolicy covers forbidden types, SigHashAll, and the accept case.

Minor: TestVerifySignedCheckpointTxsSighashPolicy only walks forbiddenSighashTypes (not the SigHashAll mismatch path). Since VerifySignedCheckpointTxs routes through verifyOffchainTx which now has the mismatch check, the protection exists; it's just not explicitly exercised for checkpoints. Not a blocker.

---

3. Tree finality in TxTree.Validate() — pkg/ark-lib/tree/tx_tree.go

Two new checks applied at every node:

• LockTime != 0 → error
• input.Sequence != wire.MaxTxInSequenceNum → error

The honest builder at builder.go:228 passes locktime=0, []uint32{wire.MaxTxInSequenceNum} — the new checks are exactly aligned.

Minor comment precision: The inline comment ("A non-final tx can't be broadcasted until its timelock elapses") is slightly imprecise for the sequence check: a tx with locktime=1 and sequence=0xFFFFFFFF is still final in Bitcoin's rules (maxseq disables locktime enforcement). The enforcement is correct — it pins both fields to what the honest builder emits — but the comment should acknowledge that the sequence check also catches canonical-value violations that a locktime=0 check alone wouldn't.

✅ Fix is correct. TestTxTreeValidateFinality walks every node of up to 255-vtxo and 171-connector-tree vectors, mutates and restores both fields, and checks that Validate() recurses properly.

---

4. Validate before signing — pkg/client-lib/batch-session/handler/default_handler.go

The call to validateVtxoTreeAgainstCommitmentTx (new helper) is inserted at OnTreeSigningStarted before session.Init and SubmitTreeNonces, meaning no nonce is committed until:

• TxTree.Validate() passes (structure + finality)
• ValidateVtxoTree passes (batch-output link, cosigner keys, sweep leaf)
• validateReceivers passes (our outputs are present with correct amounts)

The same checks re-run at OnBatchFinalization via validateVtxoTree against the finalization event's commitment tx. If the operator presents a different commitment tx at finalization, the re-check catches the swap. No check was removed.

✅ Fix is correct. TestOnTreeSigningStartedValidatesBeforeSigning verifies that session.initiated and client.submittedNonces both remain zero for bad trees (wrong outpoint, non-final sequence, wrong receiver amount) and the accept case reaches the signer.

---

Cross-repo gap — go-sdk/swap/musig2_handler.go:23-68

musig2BatchSessionHandler.OnTreeSigningStarted in the go-sdk still calls session.Init and SubmitTreeNonces without any vtxo-tree validation. Users of the go-sdk's VHTLC batch handler are not covered by fixes (1) or (4) in this PR. The sighash fix in (2) only protects finalizeTx in pkg/client-lib; the go-sdk's own sign paths (identity/identity.go:480, 507, 590) use input.SighashType directly from the PSBT without an equivalent allowlist check.

Recommend tracking these as a follow-up before the next release of the go-sdk.

---

Summary

All three security issues are real and correctly fixed. The new TxTree.Validate() checks are non-breaking since they enforce values the honest builder already produces. Tests are structured correctly (accept cases + per-fix reject cases, deep-tree recursion coverage). The code is clean, the refactor of validateVtxoTree into validateVtxoTreeAgainstCommitmentTx + validateVtxoTree is clear and the re-run at finalization is the right defensive choice.

Requesting the go-sdk gap be tracked as a follow-up. Happy to re-review if anything changes.

[arkana-ai-bot]

Arkana review — 2026-09-02

⚠️ Protocol-critical — client-side VTXO tree validation order. Human reviewer required before merge.

This PR closes two distinct classes of vulnerability.

1. TxTree.Validate — locktime and sequence checks
Adding and checks to is correct. A non-final transaction can't be broadcast when the unroll path is needed; pre-signing such a tree is meaningless at best and misleading at worst. The test () verifies both properties recursively on every node, which is the right scope — a single bad leaf would previously pass.

2. OnTreeSigningStarted — validate BEFORE contributing to musig2
This is the more important fix. Previously was called in ; the refactor pulls the commitment-tx checks into and calls it at the start of , before and before . This is the correct ordering: a client must not commit any musig2 nonce or signature to a tree it hasn't verified.

verifies all four cases (valid, wrong parent outpoint, non-final sequence, wrong receiver amount) and asserts that and on rejection. Good coverage.

One question: (called at ) also calls again on the finalization commitment tx. The comment says "it may not be the one we were given when the signing session started." Is there a scenario where the server legally substitutes the commitment tx between signing and finalization? If so, is it safe to have signed the tree against the original? If not, should the second call be a hard rejection when the txid differs? Worth a comment or an explicit equality check.

Overall: the ordering fix is important and the implementation is correct. Looks ready to merge pending the above question.

[arkana-ai-bot]

Arkana review — #1176 (sha 3ffee5cb) — ⚠️ PROTOCOL-CRITICAL: flag for human review

Verdict: looks ready to merge — moves vtxo tree validation to before nonce/signature submission, closing a meaningful security gap.

What this does:

1. Splits validateVtxoTree into validateVtxoTreeAgainstCommitmentTx (needs only the vtxo tree + commitment tx) and the original (runs at finalization). The former now runs in OnTreeSigningStarted, before any nonces are submitted.
2. Adds locktime=0 and sequence=0xffffffff checks to TxTree.Validate() — nodes with a non-final timelock or sequence can't be broadcast immediately, which could delay the unroll path.
3. Tests: TestTxTreeValidateFinality verifies both checks recurse into child nodes; newTreeSigningFixture and recordingSignerSession/recordingClient verify nonces are withheld when the tree fails validation.

Why this matters:
Previously a client could be asked to contribute nonces for a tree it hadn't fully validated — only the finalization event re-ran the checks. A malformed tree (wrong amounts, wrong outputs, non-final timelocks) could thus obtain musig2 nonces from the client before the mismatch was caught. Moving the check before Init closes that window.

Good:

• validateVtxoTreeAgainstCommitmentTx is called once in OnTreeSigningStarted and again in validateVtxoTree (at finalization), catching a commitment-tx swap between the two events. Correct.
• The locktime and sequence checks are on t.Root and the test verifies they recurse through allNodes — good depth coverage.
• recordingSignerSession.initiated and recordingClient.submittedNonces make the pre-send gate observable.

One question (non-blocking):
TxTree.Validate() now rejects any non-zero locktime. If a future tree node type legitimately uses a relative timelock (e.g., a VTXO expiry node), this would need revisiting. Is the intent that all nodes in the tree must be immediately broadcastable?

⚠️ This touches musig2 nonce submission logic — please have a human reviewer sign off before merging.

[arkana-ai-bot]

Arkana review — harden client-side validation of counterparty data

SHA reviewed: 3ffee5c

This PR closes a meaningful client-side security gap: a malicious server could propose a vtxo tree whose transactions have a non-zero locktime or non-final sequence, causing the client to pre-sign an unroll path it can never broadcast. A user whose vtxos landed in such a batch would be unable to exit unless they could sweep — and only the server can sweep during the lock period.

TxTree.Validate() additions

• LockTime != 0 on the root is now rejected. Comment explains why: a non-final tx holds the unroll path hostage until the timelock elapses.
• Sequence != wire.MaxTxInSequenceNum per input is now rejected for the same reason.

Both checks are applied recursively via Validate()'s existing tree traversal (confirmed by allNodes in the test iterating all descendants).

Handler change (validateVtxoTreeAgainstCommitmentTx)

• The vtxo tree validation is now split into two phases: structural checks against the commitment tx run before SubmitTreeNonces — so the client refuses to send nonces for a tree it hasn't validated. Previously validation happened in OnBatchFinalization after nonces and partial sigs were already submitted, which was too late.
• The refactoring is clean: validateVtxoTreeAgainstCommitmentTx is then called again in validateVtxoTree to re-verify against the finalization event's commitment tx (which may differ from the signing event's). This is correct and defensive.

Tests

• tx_tree_finality_test.go: uses allNodes to mutate every node in the tree and verify the error surfaces through Validate(). Tests both locktime and sequence mutations and that restoring the value makes the tree valid again. Good coverage.
• default_handler_test.go: recordingSignerSession / recordingClient pattern cleanly tests that nonces are not submitted when the tree fails validation.

⚠️ Protocol-critical flag: this is a client-side security fix that affects how VTXOs are signed into batches. Correct as far as I can see, but requires human reviewer sign-off before merge.

Verdict: looks ready to merge. One minor observation: the comment on the Validate additions should note that this check applies to all nodes in the tree (since the method recurses), not just the root — a reader skimming the code might miss that. Not a blocker.

[arkana-ai-bot]

This PR has been open for 1+ day without review — it's a client-side security fix (pre-sign validation before nonce submission). @Kukks is anyone assigned?

[arkana-ai-bot]

Arkana automated review — #1176

⚠️ Protocol-critical — flag for human review.

This PR hardens client-side validation in two complementary places:

1. TxTree.Validate() — finality checks (pkg/ark-lib/tree/tx_tree.go)

• Rejects any tree node whose root LockTime != 0. Without this a server could propose a timelocked unroll path that the client pre-signs but cannot broadcast until the lock elapses — potentially after the batch sweep path matures, leaving the client unable to exit.
• Rejects any input with Sequence != MaxTxInSequenceNum. A non-maximal sequence enables relative lock times (BIP68), which would again delay the unroll path the client depends on.
• Both checks recurse through allNodes in the test, confirming they apply at every level of the tree, not just the root.

2. OnTreeSigningStarted — validate before signing (pkg/client-lib/batch-session/handler/default_handler.go)

• validateVtxoTreeAgainstCommitmentTx is now called at the top of OnTreeSigningStarted, before the session is initialised and any nonces are generated. Previously the tree was only validated at BatchFinalizationEvent, which is after signing. A malicious server could submit a structurally invalid or substituted tree, collect signatures, and never reach finalisation — the client would have committed signatures to a tree it hadn't verified.
• Refactoring validateVtxoTreeAgainstCommitmentTx as a standalone helper makes it callable from both the signing phase and the finalisation phase (re-run against the actual commitment tx in the event, guarding against a swap between the two phases). Clean design.

Test coverage: TestTxTreeValidateFinality exercises both locktime and sequence enforcement on every node of the tree using real test vectors. default_handler_test.go builds a realistic signing fixture. Good.

Minor observation: The PSBT decode of the commitment tx in validateVtxoTree still reads from strings.NewReader(commitmentTx) where commitmentTx is a base64/hex string — confirm this matches the actual wire format sent in BatchFinalizationEvent.Tx (should be fine if the event is already base64 PSBT, just worth a second look).

Overall: Correctness and security improvement. Looks ready to merge.

[arkana-ai-bot]

Review: looks ready to merge — early VTXO-tree validation before signing (🛡 protocol-critical, flagged for human review)

What this does: moves validateVtxoTreeAgainstCommitmentTx so it runs inside OnTreeSigningStarted, before the client submits any musig2 nonces. Previously the same checks only ran at OnBatchFinalized, meaning the client would sign a tree it hadn't validated yet. An adversarial server could construct a malformed tree and collect partial nonces/sigs from honest clients before they noticed.

Also adds: TxTree.Validate now rejects any node with LockTime != 0 or Sequence != MaxTxInSequenceNum. This closes the vector where a server-proposed tree could include a CSV/CLTV-delayed node that makes the unroll path non-immediately broadcastable after its parent confirms.

Correctness:

• Both checks are applied recursively (the test helper allNodes walks the whole tree) — good.
• The validateVtxoTreeAgainstCommitmentTx refactor correctly eliminates the duplicate parse and re-uses the same commitmentPtx reference.
• At finalization the checks are re-run against the event's commitment tx (not the one from signing-started), which handles the race where the server could swap the commitment between phases.

Tests: TestTxTreeValidateFinality mutates every node (not just root) for both locktime and sequence, then restores and re-validates — solid regression coverage. The defaultHandler test fixture uses a recordingSignerSession to assert nonces are never submitted for a bad tree.

Suggested question for human reviewer: Is there a second validateVtxoTreeAgainstCommitmentTx call at finalization intentional duplication, or should it be deduplicated with the one in signing-started? The PR comment says "re-run them against the commitment tx of this event" which sounds intentional, but worth confirming.

[arkana-ai-bot]

Arkana review — sha 3ffee5c ⚠️ PROTOCOL_CRITICAL — please ensure a human reviewer signs off.

What this does: two client-side hardening changes.

1. TxTree finality enforcement ()
now rejects any node whose root tx has a non-zero or a non-maximal input . The commit message explains why: a pre-signed unroll path that Bitcoin won't mine immediately is worth nothing — the batch output sweep can mature while the path is locked, giving the server (or a MITM) a window to redirect funds.

The check applies recursively to every node in the tree (confirmed by in the test, which walks all children). Good — a malicious leaf inside a subtree would otherwise slip through.

2. Validate vtxo tree before signing ()
Previously (which includes the finality and receiver checks) was called at — after nonces and signatures were already submitted. A server that provided a tree with non-final txs at signing time and swapped it for a valid one at finalization would have received signed nonces for a tree the client never verified.

This PR pulls the structural checks into and calls it at , before / / . The test () confirms that an invalid tree aborts before any nonce is generated.

at finalization re-runs the same checks against the commitment tx presented there — correct, since the commitment tx could differ between signing start and finalization.

Concerns: None. Logic is correct, tests are solid.

Looks ready to merge. Flag for human reviewer given the signing path change.

[arkana-ai-bot]

Arkana review — sha 3ffee5c ⚠️ PROTOCOL_CRITICAL — please ensure a human reviewer signs off.

What this does: two client-side hardening changes.

1. TxTree finality enforcement (pkg/ark-lib/tree/tx_tree.go)
Validate() now rejects any node whose root tx has a non-zero LockTime or a non-maximal input Sequence. The reason: a pre-signed unroll path that Bitcoin won't mine immediately is worth nothing — the batch output sweep can mature while the path is locked, giving a malicious server a window to redirect funds. The check recurses through all nodes via allNodes() in the test.

2. Validate vtxo tree before signing (pkg/client-lib/batch-session/handler/default_handler.go)
Previously validateVtxoTree (including finality and receiver checks) was only called at OnBatchFinalization — after nonces and signatures were already submitted. A server that provided a tree with non-final txs at signing time and swapped it for a valid one at finalization would have received signed nonces for a tree the client never verified.

This PR pulls the structural checks into validateVtxoTreeAgainstCommitmentTx and calls it at OnTreeSigningStarted, before Init / GetNonces / SubmitTreeNonces. The recordingSignerSession test confirms that an invalid tree aborts before any nonce is generated.

validateVtxoTree at finalization re-runs the same checks against the commitment tx presented there — correct, since the commitment tx could differ between signing start and finalization.

No concerns. Logic is correct, tests are solid. Looks ready to merge. Flagged for human review given the signing path change.

[arkana-ai-bot]

Arkana review — sha 3ffee5cb — ⚠️ protocol-critical path (client-side MuSig2 signing). Please have a human review before merging.

What it does:
Moves vtxo-tree validation to before the client submits its MuSig2 nonces. Previously the tree was validated only at batch finalization, meaning a malicious server could propose a tree containing non-final transactions (locktime > 0, or RBF-signalling sequences) and the client would have pre-signed unroll paths for a tree it hadn't yet checked.

The changes:

1. now asserts and all inputs have (0xffffffff). This ensures every node in the tree is unconditionally final — it can be broadcast as soon as its parent confirms.
2. A new validateVtxoTreeAgainstCommitmentTx method runs these checks in OnTreeSigningStarted, before Init is called and nonces submitted.
3. The same validation is re-run at finalization against the actual commitment tx (the server could swap it between rounds).

Assessment: The fix is correct and addresses a genuine gap. Tests cover the locktime and sequence checks across the full tree (recursive into children). The test fixture (newTreeSigningFixture) verifies that an invalid tree blocks nonce submission.

No issues with the implementation. Flagging for human review because this touches signing and the MuSig2 session initiation path.

[arkana-ai-bot]

Arkana review — sha 3ffee5c — protocol-critical path (client-side MuSig2 signing). Please have a human review before merging.

What it does: Moves vtxo-tree validation to before the client submits its MuSig2 nonces. Previously the tree was validated only at batch finalization, meaning a malicious server could propose a tree containing non-final transactions (locktime > 0, or RBF-signalling sequences) and the client would pre-sign unroll paths for a tree it had not yet checked.

Changes:

1. TxTree.Validate() now asserts LockTime == 0 and all inputs have Sequence == 0xffffffff. Every node in the tree is unconditionally final.
2. validateVtxoTreeAgainstCommitmentTx runs these checks in OnTreeSigningStarted, before Init is called and nonces submitted.
3. The same validation re-runs at finalization against the actual commitment tx (server could swap it between rounds).

Assessment: The fix is correct and addresses a real gap. Tests cover locktime and sequence checks across the full tree (recursive into children). The recordingClient fixture verifies that an invalid tree blocks nonce submission entirely.

Flagging for human review because this touches client-side MuSig2 session initiation.

[arkana-ai-bot]

Arkana review — 2026-09-05

Three client-side security hardening fixes. All are in pkg/client-lib / pkg/ark-lib — server-side round processing is not touched.

1. Sighash policy (pkg/client-lib/offchain-tx/utils.go)
verifyOffchainTx previously verified a counterparty signature under the sighash type that counterparty declared, computing the message hash from the counterparty's declared value. The fix:

• Adds an allowlist check rejecting types the honest stack never produces
• Requires the declared type to match the one in the tx we built ourselves
• Computes the verification hash from our own originalInput.SighashType, not the signed copy

finalizeTx now also validates checkpoint sighash types before signing — previously unchecked.

The allowlist (SigHashDefault, SigHashAll, SigHashAll|SigHashAnyOneCanPay) is intentionally tight. This is a meaningful security improvement.

2. Tree finality (pkg/ark-lib/tree/tx_tree.go)
TxTree.Validate() now requires LockTime == 0 and every input's Sequence == MaxTxInSequenceNum on every node. The honest builder already produces these values. A pre-signed unroll path with a non-final tx would be unbroadcastable until its timelock elapsed.

The finality tests walk the full graph using allNodes(), so the recursion is exercised rather than just the root. Good coverage.

3. Validate before signing (pkg/client-lib/batch-session/handler/default_handler.go)
validateVtxoTreeAgainstCommitmentTx is now called at OnTreeSigningStarted, before any nonce is submitted. The same validation still runs at finalization against that event's commitment tx. No check was weakened; the structure validation and receiver check now happen twice.

The new test (TestOnTreeSigningStartedValidatesBeforeSigning) pins that a bad tree stops nonce submission (submittedNonces == 0) and the signing session is never initiated.

No concerns with the logic. The test matrix covers accept, structure rejection, non-final rejection, wrong receiver amount. Noting that no e2e was run (per the PR description) — worth running against a live stack before merging given the signing path changes.

⚠️ Protocol-critical path (signing, client-side transaction validation) — human review required before merge. Looks correct and is a clear security improvement.