test: add P2P exchange escrow integration tests

[Kukks]

Summary

Closes #22

Implements the P2P exchange escrow contract as integration tests with 3 Arkade script leaves and a full taproot VTXO tree (7 closures). Also extends ReadArkadeScript to support all closure types (was previously hardcoded to MultisigClosure only).

Parameters

Param	Type	Description
sellerPubKey	32-byte x-only pubkey	Seller's public key — used in CSFS attestations and seller-keyed closures
buyerPubKey	32-byte x-only pubkey	Buyer's public key — used in CSFS attestations and buyer-keyed closures
arbitratorPubKey	32-byte x-only pubkey	Arbitrator's public key — OP_CHECKSIG on arbitrator closures
buyerSpk	scriptPubKey	Pre-approved buyer destination — checked via OP_INSPECTOUTPUTSCRIPTPUBKEY on SellerConfirm
sellerSpk	scriptPubKey	Pre-approved seller destination — checked via OP_INSPECTOUTPUTSCRIPTPUBKEY on BuyerRefund
feeSpk	scriptPubKey	Fee output address — checked via OP_INSPECTOUTPUTSCRIPTPUBKEY
feeBasisPoints	uint64	Fee as basis points (e.g. 200 = 2%) — used in OP_MUL64/OP_DIV64 computation
cltvTimeout	int64	Absolute locktime (block height) for seller self-release path
csvTimeout	int64	Relative locktime (blocks) for unilateral exit paths
tradeID	32 bytes	External trade identifier — used to derive RELEASE and CANCEL oracle messages

Oracle messages

• RELEASE: SHA256(0x01 || tradeID) — attested by seller (payment received) via CSFS
• CANCEL: SHA256(0x02 || tradeID) — attested by buyer (voluntary refund) via CSFS

VTXO taproot tree

Each row is one spendable path. Tapscript opcodes are the Bitcoin Script in the tapscript itself. Arkade script opcodes are validated by the introspector engine inside that tapscript.

Path	When used	Tapscript opcodes	Arkade script opcodes
Seller confirms	Seller received fiat payment and attests RELEASE. Buyer claims BTC to pre-approved address. Fee (input x bp / 10000) enforced. Buyer amount enforced (input - fee).	<buyer> OP_CHECKSIGVERIFY <introspector> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG	<seller> OP_CHECKSIGFROMSTACK(RELEASE) OP_VERIFY OP_INSPECTNUMINPUTS 1 OP_EQUALVERIFY 0 OP_INSPECTOUTPUTSCRIPTPUBKEY <buyer_ver> OP_EQUALVERIFY <buyer_prog> OP_EQUALVERIFY 1 OP_INSPECTOUTPUTSCRIPTPUBKEY <fee_ver> OP_EQUALVERIFY <fee_prog> OP_EQUALVERIFY OP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTVALUE <bp> OP_MUL64 OP_VERIFY <10000> OP_DIV64 OP_VERIFY OP_SWAP OP_DROP 1 OP_INSPECTOUTPUTVALUE OP_SWAP OP_GREATERTHANOREQUAL64 OP_VERIFY OP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTVALUE 1 OP_INSPECTOUTPUTVALUE OP_SUB64 OP_VERIFY 0 OP_INSPECTOUTPUTVALUE OP_SWAP OP_GREATERTHANOREQUAL64
Buyer cancels	Buyer voluntarily cancels the trade. Full amount returns to pre-approved seller address. No fee. Amount enforced (seller gets >= input).	<seller> OP_CHECKSIGVERIFY <introspector> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG	<buyer> OP_CHECKSIGFROMSTACK(CANCEL) OP_VERIFY 0 OP_INSPECTOUTPUTSCRIPTPUBKEY <seller_ver> OP_EQUALVERIFY <seller_prog> OP_EQUALVERIFY OP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTVALUE 0 OP_INSPECTOUTPUTVALUE OP_SWAP OP_GREATERTHANOREQUAL64
Topup	Anyone adds more BTC to the escrow (e.g. top-up or consolidation). No fee.	<buyer> OP_CHECKSIGVERIFY <introspector> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG	OP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTSCRIPTPUBKEY OP_1 OP_EQUALVERIFY 0 OP_INSPECTOUTPUTSCRIPTPUBKEY OP_1 OP_EQUALVERIFY OP_EQUALVERIFY OP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTVALUE 0 OP_INSPECTOUTPUTVALUE OP_LESSTHAN64
Arbitrator releases to buyer	Dispute resolved in buyer's favor. Arbitrator signs the transaction directly, controlling exact outputs and fee. No Arkade script.	<buyer> OP_CHECKSIGVERIFY <arbitrator> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG	—
Arbitrator refunds seller	Dispute resolved in seller's favor. Arbitrator signs the transaction directly, controlling exact outputs. No Arkade script, no fee.	<seller> OP_CHECKSIGVERIFY <arbitrator> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG	—
Seller self-release (CLTV)	After CLTV expires, seller reclaims funds if buyer and arbitrator are absent. No introspector, no Arkade script, no fee.	<locktime> OP_CHECKLOCKTIMEVERIFY OP_DROP <seller> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG	—
Mutual exit (CSV)	Buyer and seller mutually agree to exit without operator/introspector. Available after relative timelock. No fee.	<sequence> OP_CHECKSEQUENCEVERIFY OP_DROP <buyer> OP_CHECKSIGVERIFY <seller> OP_CHECKSIG	—
Seller-only recovery (CSV)	Last resort if buyer, arbitrator, and operator are all unresponsive. Available after double relative timelock. No fee.	<sequence> OP_CHECKSEQUENCEVERIFY OP_DROP <seller> OP_CHECKSIG	—

Introspector changes

• ReadArkadeScript now uses scriptlib.DecodeClosure() instead of hardcoding MultisigClosure.Decode(), supporting all closure types

Design decisions

• Arbitrator uses OP_CHECKSIG: Signs transactions directly, controlling exact outputs — no CSFS or Arkade needed
• Three closure types: Buyer+introspector (RELEASE), seller+introspector (CANCEL), arbitrator (disputes) — each with operator
• Percentage-based fees: input_value x basis_points / 10000 computed on-chain via OP_MUL64/OP_DIV64
• External trade ID: 32-byte externally provided value, not derived from pubkeys
• Pre-approved destinations: Buyer/seller receive addresses locked into the contract — prevents fund diversion even if CSFS keys are compromised
• Seller CLTV self-release: Pure seller + operator multisig with absolute timelock, no introspector
• CSV unilateral exits are additive: Always available alongside collaborative paths

Test coverage (3 test functions, 10 cases)

• TestP2PEscrowSellerConfirm — wrong key, fee too low, wrong fee address, wrong buyer destination, valid
• TestP2PEscrowBuyerRefund — wrong key rejection, wrong seller destination, valid buyer CANCEL
• TestP2PEscrowTopupPath — wrong scriptPubKey, same-value rejection

Test plan

• go test ./test/ -run TestP2PEscrow -v — all 3 test functions pass
• CI green (format, lint, unit, integration, Trivy)
• Arbitrator path tests (pure multisig, requires manual witness construction)

[ghost]

Review — P2P Exchange Escrow Integration Tests

Thorough and well-structured PR. The escrow contract design is solid, and the test coverage for the three Arkade-scripted leaves is good. A few observations:

pkg/arkade/script.go — closurePubKeys + ReadArkadeScript

The refactor from hardcoded MultisigClosure.Decode() to scriptlib.DecodeClosure() is clean and correct. The closurePubKeys type switch covers all current closure types. One minor note: if a new closure type is added to scriptlib that also embeds MultisigClosure, this function will silently return nil — might be worth a comment or a compile-time interface check, but not blocking.

Security Review

SellerConfirm (Leaf 0) ✅

• CSFS attestation correctly binds seller identity to RELEASE message
• OP_INSPECTNUMINPUTS 1 OP_EQUALVERIFY prevents input-stuffing attacks — good
• Fee computation via OP_MUL64/OP_DIV64 with overflow/div-zero guards (OP_VERIFY after each) is correct
• Pre-approved destination addresses prevent fund diversion even if CSFS keys are compromised
• OP_GREATERTHANOREQUAL64 allows overpaying fees (not just exact match) — reasonable design choice

BuyerRefund (Leaf 2) ✅

• No OP_INSPECTNUMINPUTS check here, but since output is locked to pre-approved seller address, extra inputs would only donate funds to the seller — not exploitable
• No output value enforcement (could send less than full escrow minus miner fee), but since seller + introspector + operator all co-sign, this requires 3-party collusion to underpay — acceptable
• Full refund to seller with no fee deduction on cancel is correct behavior

Topup (Leaf 5) ✅

• Recursive covenant correctly enforces output[0].spk == input[current].spk
• OP_LESSTHAN64 enforces input_value < output_value — strictly increasing, prevents no-op spends
• No input count restriction is intentional — topups may consolidate multiple inputs
• CASE 2 test correctly validates that equal-value spends fail the OP_LESSTHAN64 check

Test Quality

• 10 test cases across 3 functions with both positive and negative paths
• submitAndExpectFailure helper reduces boilerplate nicely
• Negative tests cover the right attack vectors: wrong CSFS key, fee underpayment, wrong destinations, wrong scriptPubKey, same-value rejection

Minor Nits (Non-blocking)

1. Topup test feeSpk: []byte{0x6a} — This OP_RETURN scriptPubKey is fine since the topup leaf never checks it, but other leaves in this same VTXO tree (SellerConfirm) would fail extractWitnessInfo if someone tried to use them. Fine for test isolation, but worth a comment noting this VTXO is only valid for the topup path.

2. TODO: Arbitrator path tests — Acknowledged in the PR body. These are pure multisig paths (no Arkade script), so lower risk, but would complete the coverage.

Overall: clean design, correct security properties, good test coverage. 👍

[ghost]

🔍 Follow-up Review — Arkana (post-7145498)

New commits since last review: merge conflict resolution (d406c61) and test/error message adaptation (7145498).

Changes

1. Error message expectation updated from "spendingtapscript is not a MultisigClosure" → "unexpected error while decoding tapscript" — matches the DecodeClosure() refactor. Correct.

2. Pre-approved destination enforcement — SellerConfirm now enforces output[0] → buyerSpk, BuyerRefund enforces output[0] → sellerSpk. This was already described in the PR design but now implemented in the Arkade scripts. Security-positive change — prevents fund diversion even if CSFS keys are compromised.

3. Witness field type adapted from []byte to wire.TxWitness — upstream API change.

⚠️ Cross-PR Conflict with #30

This PR's closurePubKeys helper function and the inline type switch in #30 implement the same logic differently. #30 also removes the inputIndex parameter from ReadArkadeScript. After #30 merges, this PR will need a rebase to:

• Drop the closurePubKeys function (replaced by #30's inline switch)
• Update call sites to the new 3-param signature

Verdict

No new security concerns. The pre-approved destination enforcement strengthens the contract. Merge #30 first, rebase this, then ship. 👍

[ghost]

🔍 Follow-up Review — Arkana (post-787e7e2)

New commits since last review add arbitrator leaf tests, enforce pre-approved destinations + output amounts, then remove output count enforcement (OP_INSPECTNUMOUTPUTS).

Output Count Removal — Security Analysis ✅

The removal of OP_INSPECTNUMOUTPUTS is safe. Reasoning:

SellerConfirm: The script enforces:

1. output[0].spk == buyerSpk (destination locked)
2. output[1].spk == feeSpk (fee destination locked)
3. output[1].value >= inputValue * bp / 10000 (minimum fee)
4. output[0].value >= inputValue - output[1].value (buyer gets remainder)

Extra outputs (OP_RETURN, P2A anchor from BuildTxs) cannot divert funds because Bitcoin consensus prevents creating value. If total output value exceeds input value, the transaction is invalid. Extra outputs can only reduce output[0] or output[1] values, which would violate the enforced constraints.

BuyerRefund: The script enforces output[0].value >= inputValue and output[0].spk == sellerSpk. Extra outputs would need to source their value from somewhere — they can't, because output[0] already claims the full input value. Any extra output would push total outputs above input value → invalid tx.

Topup: No output count was ever enforced — the script only checks output[0].spk == input[current].spk and output[0].value > input.value.

New Additions Since Last Review

• Arbitrator tests (Leaf 1 & 3) now implemented — pure multisig paths exercised ✅
• CSFS oracle message hashing — SHA256(prefix || tradeID) to get 32-byte schnorr message ✅
• Arbitrator design change — switched from CSFS to direct OP_CHECKSIG. This is simpler and correct: the arbitrator signs the transaction itself, controlling exact outputs. No Arkade script needed for dispute resolution paths.
• CLTV seller self-release — pure seller+operator with absolute timelock, no introspector. Correct fallback for absent buyer+arbitrator.

Cross-PR Status

PR #30 has merged. This PR's latest force-push (commits starting 17893fb) appears to be rebased on top. The closurePubKeys helper and ReadArkadeScript signatures should be aligned — confirmed by the updated error expectation in tx_test.go.

Verdict

No security concerns with the output count removal — the value + destination constraints are sufficient. Ship it. 👍

[ghost]

🔍 Arkana PR Review

Test-only PR — P2P exchange escrow integration tests

Summary

Comprehensive integration test suite for P2P escrow using Arkade introspection opcodes (CSFS, OP_INSPECTOUTPUTSCRIPTPUBKEY, OP_INSPECTOUTPUTVALUE, etc.). Tests 5 leaf paths: seller confirm (CSFS attestation), buyer refund, arbitrator resolution, CLTV self-release, and recursive covenant topup.

Analysis

✅ Good coverage of attack vectors:

• Tests wrong CSFS message, insufficient fees, wrong destinations, wrong signer keys
• Fee enforcement tested at exact boundary (2% = 1000 sats on 50000)
• Topup covenant correctly enforces output_value > input_value and same scriptPubKey

✅ Escrow architecture looks sound:

• 7-leaf tapscript tree with appropriate access controls per path
• Introspector key is tweaked with arkade script hash — ties the introspector's signature authority to the specific contract logic
• Arbitrator paths use direct OP_CHECKSIG (no Arkade/introspection), which is correct — arbitrator is a trusted party resolving disputes, not enforcing contract logic

Observations

1. Leaf 5 (topup): No signature required — purely covenant-enforced. This is intentional (anyone can add funds), but worth confirming this is the desired UX. A griefing vector exists where someone could topup with dust, though the output_value > input_value check mitigates this.

2. OP_GREATERTHANOREQUAL64 vs OP_LESSTHAN64 in topup: The topup leaf uses OP_LESSTHAN64 for the value check, which checks input < output. This is correct for "output must be strictly greater" semantics.

3. No test for the mutual exit path (Leaf 6: buyer + seller CSV) — this is the most likely cooperative resolution path and might warrant a test.

Verdict

Well-structured test suite. No production code changes, so no security risk from merge. The tests themselves validate important security invariants of the escrow contract.

[arkana-ai-bot]

Review — arkade-os/emulator PR #24

All findings cite file:line in the PR diff. CI is green but several correctness gaps need resolution before merge.

---

H1 — Missing OP_INSPECTNUMINPUTS in buildLeaf2BuyerRefund

test/p2p_escrow_test.go:184–210

buildLeaf0SellerConfirm enforces a single-input transaction (OP_INSPECTNUMINPUTS 1 OP_EQUALVERIFY), but buildLeaf2BuyerRefund has no equivalent check. The value guard output[0] >= input[current].value does not prevent an additional co-spent input from having its value claimed by a third output. If a buyer and a colluding counter-party can arrange a multi-input spend (or if the operator crafts one), the buyer-refund path can leak value from other VTXOs in the same transaction.

SellerConfirm added this check deliberately; BuyerRefund must match it for consistency and safety.

---

H2 — No OP_INSPECTNUMOUTPUTS in any Arkade script

test/p2p_escrow_test.go:103–165, 184–210, 217–236

None of the three Arkade scripts bounds the number of outputs. SellerConfirm's value constraints (output[0] >= input - actual_fee and output[1] >= min_fee) do not sum to input value, so a third output can silently absorb the gap. BuyerRefund (output[0] >= input) conserves value only when fee is zero and no third output exists. Topup checks only output[0].

A malicious operator co-signing one of these paths can add extra outputs. The Arkade script is the only layer that constrains operator behaviour; it must close this.

---

H3 — CSFS message content is not enforced on-chain

test/p2p_escrow_test.go:80–102, 175–183

<seller_pk> OP_CHECKSIGFROMSTACK OP_VERIFY

The message is taken from the witness stack; the script does not check that the message equals SHA256(0x01 || tradeID) (RELEASE) or SHA256(0x02 || tradeID) (CANCEL). Any 32-byte value signed with the correct key passes. A seller can sign an arbitrary nonce and spend the SellerConfirm path; a buyer can do the same for BuyerRefund. The oracle message semantics described in the PR description are not enforced on-chain.

If this is intentional (the message is only for external auditability), it must be prominently documented. If it is not intentional, add:

OP_SHA256                  # hash the top-of-stack message
<expected_release_hash> OP_EQUALVERIFY  # bind to RELEASE

before the CSFS check. The test suite has no case that demonstrates a wrong message fails (because it wouldn't), which is a coverage gap that masks this behaviour.

---

H4 — Single Arkade hash per VTXO; multi-path production escrow is untested

test/p2p_escrow_test.go:255–321, 408–411, 705–708, 973–976

createEscrowVtxoScript takes a single arkadeScriptHash and applies it to every buyer-collab closure in the tapscript tree. Each of the three integration tests funds a separate VTXO with a single Arkade hash:

• TestP2PEscrowSellerConfirm → VTXO tweaked with ArkadeScriptHash(sellerConfirmScript)
• TestP2PEscrowBuyerRefund → VTXO tweaked with ArkadeScriptHash(buyerRefundScript)
• TestP2PEscrowTopupPath → VTXO tweaked with ArkadeScriptHash(topupScript)

A production P2P escrow requires one VTXO address that supports all three spending paths simultaneously, with each leaf's introspector key tweaked with the hash of that leaf's Arkade script. The current test suite does not demonstrate this and createEscrowVtxoScript cannot express it. This gap must be addressed before this can be called a complete escrow integration test.

---

M1 — Unchecked index into signedByIntrospectorCheckpoints

test/p2p_escrow_test.go:616, 893

introspectorCheckpointPtx, err := psbt.NewFromRawBytes(
    strings.NewReader(signedByIntrospectorCheckpoints[i]), true)

If the introspector returns fewer checkpoint entries than the server (len(signedByIntrospectorCheckpoints) < len(signedByServerCheckpoints)), this is a panic in both TestP2PEscrowSellerConfirm and TestP2PEscrowBuyerRefund. Add:

require.Len(t, signedByIntrospectorCheckpoints, len(signedByServerCheckpoints),
    "introspector checkpoint count mismatch")

before the loop.

---

M2 — out.PkScript[2:] slice without length guard

test/p2p_escrow_test.go:443, 738, 1011

if bytes.Equal(out.PkScript[2:], schnorr.SerializePubKey(escrowAddr.VtxoTapKey)) {

If any output in the funding transaction has a PkScript shorter than 3 bytes (e.g., an OP_RETURN with no payload), this panics. Guard with:

if len(out.PkScript) > 2 && bytes.Equal(out.PkScript[2:], ...)

---

M3 — Missing negative test: wrong CSFS message

test/p2p_escrow_test.go:522–531, 788–817

TestP2PEscrowSellerConfirm covers wrong-key but not wrong-message (e.g., seller signing cancelMsg with their key). Because message content is not enforced (see H3), this case would pass — and the test suite never exposes that. Add a case that calls signCSFS(sellerPrivKey, cancelMsg) and asserts failure. If it passes, that is itself the bug report for H3.

---

L1 — Stale comment at p2p_escrow_test.go:233

// OP_GREATERTHAN64 pops b then a, checks a < b (i.e. input < output)
AddOp(arkade.OP_LESSTHAN64).

Comment names the wrong opcode. Should read OP_LESSTHAN64.

---

L2 — Misleading test label in Topup CASE 2

test/p2p_escrow_test.go:1078–1100

Comment header says "CASE 2: Valid — output[0] has same scriptPubKey (same value, passes spk check)" but the test asserts require.Error. The label should say "CASE 2: Invalid — same value fails the topup value check".

---

pkg/arkade/script.go change — no issues

The extension from hardcoded MultisigClosure.Decode() to scriptlib.DecodeClosure() is correct. All five closure types extract PubKeys identically. The default case returns a typed error. The new tx_test.go case exercising OP_TRUE → "failed to decode tapscript" correctly covers the new code path.

---

Cross-repo

ReadArkadeScript is consumed by go-sdk (vhtlc/noninteractive.go). The new closure-type support is additive (no existing paths removed), so no breakage. No changes to proto, wire format, or exported types.

---

Summary: Changes needed on H1–H4 before merge. M1–M2 are test-only panics but real; fix in the same pass. The script.go change is fine to ship as-is.