Support unilateral exit with VHTLC ancestor#228
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 9 minutes and 25 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughClient batch flows now use a VTXO batch-tree TxGraph for collecting/building/signing transactions; unilateral-exit handling was refactored to finalize taproot witnesses from PSBT data (supporting VHTLC descendants); docs, error messages, and an ignored e2e test were added/updated. ChangesBatch-tree graph and unilateral exit finalization refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ark-core/src/unilateral_exit.rs`:
- Around line 588-603: The code currently panics when accessing
psbt.unsigned_tx.input[VTXO_INPUT_INDEX], indexing outputs by vout, and with
.expect on witness_utxo; replace these panic paths in the branch finalization
logic so the function returns a typed Error instead. Validate that
VTXO_INPUT_INDEX is within psbt.unsigned_tx.input.len() (use get on
psbt.unsigned_tx.input and return Err if missing), check that
virtual_tx_previous_output.vout as usize < other_psbt.output.len() before
cloning the output, and change the .expect("witness UTXO in path") to ok_or(...)
to return a descriptive Error; propagate errors through the surrounding function
(e.g., via Result<..., Error>) and use the existing error enum or create an
appropriate variant for malformed branch data. Ensure references to
virtual_tx_previous_output, psbt, unilateral_exit_branch, commitment_txs, and
compute_txid are used when constructing the error messages for diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bebd5d5-d8a6-4ea9-aed1-4e246949d149
📒 Files selected for processing (8)
ark-client/src/batch.rsark-client/src/unilateral_exit.rsark-core/src/batch.rsark-core/src/send.rsark-core/src/server.rsark-core/src/tree_tx_output_script.rsark-core/src/unilateral_exit.rse2e-tests/tests/boltz_reverse_vhtlc_unilateral_exit.rs
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — PR #228: Support unilateral exit with VHTLC ancestor
Verdict: Request changes (protocol-critical — requires human sign-off)
This PR generalizes the unilateral exit witness construction from a hardcoded 2-of-2 multisig script to a generic taproot script-spend finalizer that handles any combination of CHECKSIG/CHECKSIGVERIFY pubkeys plus condition witness elements (e.g. VHTLC preimages). The approach is sound and the witness ordering is correct for all current script formats in this codebase.
✅ What looks good
-
Witness ordering is correct. I verified against the VHTLC claim script in
ark-core/src/vhtlc.rs:171-184(OP_HASH160 <hash> OP_EQUAL OP_VERIFY <receiver> CHECKSIGVERIFY <server> CHECKSIG). The new code pushes reversed signatures then condition elements, which places condition data at the top of the stack — consumed first by hash-check opcodes before CHECKSIG executes. This matches the old hardcoded behavior for 2-of-2 scripts and extends correctly to VHTLC scripts. -
extract_checksig_pubkeysis more robust than the oldextract_pubkeys_from_2of2_script. It uses instruction-level parsing instead of raw byte offsets, handles arbitrary script lengths, and has existing test coverage. -
Deprecated wrapper for
sign_unilateral_exit_treepreserves backward compatibility. -
Condition element decoding in
condition_witness_elementshas proper bounds checking (element count vs remaining buffer, per-element length validation, overflow guards). -
E2E test (
boltz_reverse_vhtlc_unilateral_exit.rs) covers the full happy path includingbitcoinconsensus::verify. -
No cross-repo breakage.
sign_unilateral_exit_treeandextract_pubkeys_from_2of2_scriptare only consumed within rust-sdk itself. Other SDKs (ts-sdk, go-sdk, dotnet-sdk) use different architectures.
⚠️ Issues to address
1. Implicit script layout assumption — needs documentation
ark-core/src/unilateral_exit.rs — finalize_taproot_script_spend_witness
The function assumes condition witness elements always go at the top of the stack (pushed after signatures in the witness). This is correct for ALL current scripts (VHTLC claim, unilateral claim) because they put hash-check opcodes before CHECKSIG. But this is an implicit invariant that will silently produce invalid witnesses if anyone creates a script with CHECKSIG before the condition check.
Request: Add a doc comment on finalize_taproot_script_spend_witness explicitly stating this contract: "Condition witness elements are placed at the top of the initial stack. This requires that condition-checking opcodes (OP_HASH160, OP_SIZE, etc.) appear before any CHECKSIG/CHECKSIGVERIFY in the tapleaf script."
2. .expect("witness UTXO in path") is a panic in protocol-critical code
ark-core/src/unilateral_exit.rs — finalize_unilateral_exit_tree, near line 596 (in the diff)
.expect("witness UTXO in path");If a VTXO's previous output doesn't match any transaction in the branch or commitment set, this panics instead of returning an error. In a unilateral exit scenario — which is already an emergency path — a panic here could prevent the user from recovering funds.
Request: Replace with .ok_or_else(|| Error::transaction("witness UTXO not found in unilateral exit path"))?
(This is pre-existing, but you're touching this code path and it's protocol-critical.)
📝 Nits (non-blocking)
-
tap_scriptsiteration order:finalize_taproot_script_spend_witnesspicks the first satisfiable leaf fromBTreeMap<ControlBlock, ...>iteration. This works because the caller only populates signatures for the intended leaf, but it's worth a brief comment noting that exactly one leaf should have complete signatures. -
No unit test for
condition_witness_elementsdecoder in isolation. The encoding lives inintent.rs::encode_witnessandboltz.rs::inject_preimage_into_psbt. A round-trip unit testencode_witness → condition_witness_elementswould catch format drift between encoder and decoder without needing the full E2E environment. -
Boltz.rs encoder uses
vec![1]raw byte (lines 1460, 1712, 2132, 3574) whileintent.rs::encode_witnessuseswrite_compact_size. Both produce identical output for small counts, but the inconsistency is a maintenance footprint. Consider having boltz.rs callencode_witnessinstead of rolling its own encoding. (Out of scope for this PR, but worth a follow-up.)
🔒 Protocol-critical flag
This PR modifies unilateral exit witness construction — the code path users depend on to recover funds when the Ark server is uncooperative. Even though the logic is correct, a human must review and approve before merge. Incorrect witness construction here = funds permanently locked.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Arkana Code Review — PR #228: Support unilateral exit with VHTLC ancestor
Verdict: Request changes (protocol-critical — requires human sign-off)
This PR generalizes the unilateral exit witness construction from a hardcoded 2-of-2 multisig script to a generic taproot script-spend finalizer that handles any combination of CHECKSIG/CHECKSIGVERIFY pubkeys plus condition witness elements (e.g. VHTLC preimages). The approach is sound and the witness ordering is correct for all current script formats in this codebase.
✅ What looks good
-
Witness ordering is correct. I verified against the VHTLC claim script in
ark-core/src/vhtlc.rs:171-184(OP_HASH160 <hash> OP_EQUAL OP_VERIFY <receiver> CHECKSIGVERIFY <server> CHECKSIG). The new code pushes reversed signatures then condition elements, which places condition data at the top of the stack — consumed first by hash-check opcodes before CHECKSIG executes. This matches the old hardcoded behavior for 2-of-2 scripts and extends correctly to VHTLC scripts. -
extract_checksig_pubkeysis more robust than the oldextract_pubkeys_from_2of2_script. It uses instruction-level parsing instead of raw byte offsets, handles arbitrary script lengths, and has existing test coverage. -
Deprecated wrapper for
sign_unilateral_exit_treepreserves backward compatibility. -
Condition element decoding in
condition_witness_elementshas proper bounds checking (element count vs remaining buffer, per-element length validation, overflow guards). -
E2E test (
boltz_reverse_vhtlc_unilateral_exit.rs) covers the full happy path includingbitcoinconsensus::verify. -
No cross-repo breakage.
sign_unilateral_exit_treeandextract_pubkeys_from_2of2_scriptare only consumed within rust-sdk itself. Other SDKs (ts-sdk, go-sdk, dotnet-sdk) use different architectures.
⚠️ Issues to address
1. Implicit script layout assumption — needs documentation
ark-core/src/unilateral_exit.rs — finalize_taproot_script_spend_witness
The function assumes condition witness elements always go at the top of the stack (pushed after signatures in the witness). This is correct for ALL current scripts (VHTLC claim, unilateral claim) because they put hash-check opcodes before CHECKSIG. But this is an implicit invariant that will silently produce invalid witnesses if anyone creates a script with CHECKSIG before the condition check.
Request: Add a doc comment on finalize_taproot_script_spend_witness explicitly stating this contract: "Condition witness elements are placed at the top of the initial stack. This requires that condition-checking opcodes (OP_HASH160, OP_SIZE, etc.) appear before any CHECKSIG/CHECKSIGVERIFY in the tapleaf script."
2. .expect("witness UTXO in path") is a panic in protocol-critical code
ark-core/src/unilateral_exit.rs — finalize_unilateral_exit_tree, near the witness_utxo lookup
.expect("witness UTXO in path");If a VTXO's previous output doesn't match any transaction in the branch or commitment set, this panics instead of returning an error. In a unilateral exit scenario — which is already an emergency path — a panic here could prevent the user from recovering funds.
Request: Replace with .ok_or_else(|| Error::transaction("witness UTXO not found in unilateral exit path"))?
(This is pre-existing, but you're touching this code path and it's protocol-critical.)
📝 Nits (non-blocking)
-
tap_scriptsiteration order:finalize_taproot_script_spend_witnesspicks the first satisfiable leaf fromBTreeMap<ControlBlock, ...>iteration. This works because the caller only populates signatures for the intended leaf, but it's worth a brief comment noting that exactly one leaf should have complete signatures. -
No unit test for
condition_witness_elementsdecoder in isolation. The encoding lives inintent.rs::encode_witnessandboltz.rs::inject_preimage_into_psbt. A round-trip unit testencode_witness → condition_witness_elementswould catch format drift between encoder and decoder without needing the full E2E environment. -
Boltz.rs encoder uses
vec![1]raw byte (lines ~1460, ~1712, ~2132, ~3574) whileintent.rs::encode_witnessuseswrite_compact_size. Both produce identical output for small counts, but the inconsistency is a maintenance footprint. Consider having boltz.rs callencode_witnessinstead of rolling its own encoding. (Out of scope for this PR, but worth a follow-up.)
🔒 Protocol-critical flag
This PR modifies unilateral exit witness construction — the code path users depend on to recover funds when the Ark server is uncooperative. Even though the logic is correct, a human must review and approve before merge. Incorrect witness construction here = funds permanently locked.
🤖 Reviewed by Arkana
luckysori
changed the title
There was a problem hiding this comment.
Arkana Code Review — PR #228 (follow-up)
Verdict: Approve ✅
All three issues from my previous review are resolved in the latest commits:
-
f4258d2—.expect()replaced with proper error handling infinalize_unilateral_exit_tree. The panic on missingwitness_utxois now a descriptiveError::ad_hocwith the outpoint included. Correct fix for protocol-critical code. -
1167fc5— Doc comment added onfinalize_taproot_script_spend_witnessdocumenting the condition-first script layout invariant. Matches exactly what I requested. -
774497c— Round-trip unit test forcondition_witness_elementsdecoder covering edge cases (empty element, 253-byte varint boundary, regular data).encode_witnessvisibility minimally bumped topub(crate)— no leakage.
🔒 Protocol-critical reminder
This PR modifies unilateral exit witness construction — the emergency fund recovery path. The code is correct, but a human must still review and approve before merge.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Arkana Code Review — PR #228 (follow-up)
Verdict: Approve ✅
All three issues from my previous review are resolved in the latest commits:
-
f4258d2—.expect()replaced with proper error handling infinalize_unilateral_exit_tree. The panic on missingwitness_utxois now a descriptiveError::ad_hocwith the outpoint included. Correct fix for protocol-critical code. -
1167fc5— Doc comment added onfinalize_taproot_script_spend_witnessdocumenting the condition-first script layout invariant. Matches exactly what I requested. -
774497c— Round-trip unit test forcondition_witness_elementsdecoder covering edge cases (empty element, 253-byte varint boundary, regular data).encode_witnessvisibility minimally bumped topub(crate)— no leakage.
🔒 Protocol-critical reminder
This PR modifies unilateral exit witness construction — the emergency fund recovery path. The code is correct, but a human must still review and approve before merge.
🤖 Reviewed by Arkana
1 participant
Fix #225.
This only expands the number of ancestor scripts that are supported, but I wonder if a better solution would be for the server to return ancestor virtual transactions that are already finalised. After all, the transaction has already been accepted into the virtual mempool, so the client here is only repeating what the original sender + Arkade had to do to publish it in the first place. It's possible that this feature already exists and I'm not aware of it.
Summary by CodeRabbit
Improvements
Documentation
Tests