test: add P2P exchange escrow integration tests#24
Conversation
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
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_EQUALVERIFYprevents input-stuffing attacks — good- Fee computation via
OP_MUL64/OP_DIV64with overflow/div-zero guards (OP_VERIFYafter each) is correct - Pre-approved destination addresses prevent fund diversion even if CSFS keys are compromised
OP_GREATERTHANOREQUAL64allows overpaying fees (not just exact match) — reasonable design choice
BuyerRefund (Leaf 2) ✅
- No
OP_INSPECTNUMINPUTScheck 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_LESSTHAN64enforcesinput_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_LESSTHAN64check
Test Quality
- 10 test cases across 3 functions with both positive and negative paths
submitAndExpectFailurehelper 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)
-
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 failextractWitnessInfoif someone tried to use them. Fine for test isolation, but worth a comment noting this VTXO is only valid for the topup path. -
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. 👍
There was a problem hiding this comment.
🔍 Follow-up Review — Arkana (post-7145498)
New commits since last review: merge conflict resolution (d406c61) and test/error message adaptation (7145498).
Changes
-
Error message expectation updated from
"spendingtapscript is not a MultisigClosure"→"unexpected error while decoding tapscript"— matches theDecodeClosure()refactor. Correct. -
Pre-approved destination enforcement —
SellerConfirmnow enforcesoutput[0] → buyerSpk,BuyerRefundenforcesoutput[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. -
Witness field type adapted from
[]bytetowire.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
closurePubKeysfunction (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. 👍
Implements a 6-leaf Taproot escrow contract using CSFS attestations and transaction introspection opcodes. Tests cover: - Leaf 0 (SellerConfirm): CSFS seller attestation + fee enforcement - Leaf 1 (ArbitratorToBuyer): CSFS server attestation + fee enforcement - Leaf 2 (BuyerRefund): CSFS buyer CANCEL attestation - Leaf 3 (ArbitratorToSeller): CSFS server CANCEL attestation - Leaf 5 (TopupPath): recursive covenant (same spk, strictly more value) Negative tests: wrong CSFS message, wrong party attestation, fee too low, wrong fee address, output value not greater, wrong scriptPubKey.
- Fix errcheck on conn.Close() with nolint directive - Fix trailing whitespace in comment alignment - Add TestP2PEscrowArbitratorToBuyer (Leaf 1) and TestP2PEscrowArbitratorToSeller (Leaf 3) to exercise all builders
schnorr.Sign requires a 32-byte message hash. The oracle messages (0x01/0x02 || trade_id) are 33 bytes, so we SHA256 hash them first.
- Replace wrong-message test with wrong-key test (CSFS doesn't verify message content, only signature validity) - Simplify topup test to use local script execution since multi-input txs are needed for the value increase but BuildTxs enforces balance - Remove unused variables from topup test
- Change fee enforcement from flat minFeeSats to percentage-based using OP_MUL64/OP_DIV64 with basis points (e.g. 200 = 2%) - Make tradeID an external []byte parameter instead of deterministically computed from pubkeys - Add unilateral CSV-only exit paths (seller-only and buyer-only) as additional closures in the VTXO taproot tree, alongside the existing collaborative MultisigClosure path
- Add arbitratorPubKey field to escrowParams (distinct from serverPubKey) - serverPubKey = Arkade operator (signs collaborative MultisigClosure) - arbitratorPubKey = dispute resolver (CSFS attestations in Leaf 1/3) - Update unilateral exits: buyer+seller CSV, seller-only CSV*2
Use vtxoScript.Encode() to provide all closure scripts in the RevealedTapscripts field, not just the collaborative closure. The Ark server needs the complete taproot tree to verify the VTXO.
The collaborative MultisigClosure path now uses CLTVMultisigClosure with an absolute block height locktime, while unilateral exit paths retain CSV relative timelocks.
…or, introspector) - Remove unused serverPubKey from escrowParams (operator key comes from Ark address) - Rename ownerPubKey → buyerPubKey, serverSigner → operatorSigner in createEscrowVtxoScript - Fix comments: "server attests" → "arbitrator attests" - Rename serverPrivKey → wrongPrivKey in negative test (was only used as random wrong key)
…e unsupported) The introspector's ReadArkadeScript only decodes MultisigClosure (see pkg/arkade/script.go:55 TODO). Using CLTVMultisigClosure causes the introspector to silently skip validation (negative tests pass) and the Ark server to reject signatures (INVALID_SIGNATURE). Keep the CSV unilateral exit paths unchanged since those are not used by the collaborative spend path.
…ations - Fix non-multisig tapscript test: error message changed from "spendingtapscript is not a MultisigClosure" to "unexpected error while decoding tapscript" after switching to DecodeClosure() in ReadArkadeScript. - Add pre-approved buyer/seller destination address enforcement: SellerConfirm enforces output[0] → buyerSpk, BuyerRefund enforces output[0] → sellerSpk. - Adapt Witness field from []byte to wire.TxWitness (upstream change).
…ripts - Add buyerSpk/sellerSpk to escrowParams for pre-approved destinations - SellerConfirm: enforce output[0] → buyerSpk, output[0].value >= inputValue - fee, OP_INSPECTNUMOUTPUTS 2 (exactly buyer + fee outputs) - BuyerRefund: enforce output[0] → sellerSpk, output[0].value >= inputValue, OP_INSPECTNUMOUTPUTS 1 (single seller output) - Switch arbitrator from CSFS to OP_CHECKSIG (signs tx directly) - Drop introspector from arbitrator paths - Add CLTV seller self-release path (seller + operator, no introspector) - 7-closure taproot tree with buyer/seller/arbitrator/CLTV/CSV paths - Adapt Witness field from []byte to wire.TxWitness (upstream change) - Fix ReadArkadeScript call signature in tx_test.go
Kukks
force-pushed
the
feat/p2p-escrow-example
branch
from
7145498 to
f550e85
Compare
BuildTxs adds 2 system outputs (OP_RETURN extension + P2A anchor) that the Arkade script cannot predict. Output destinations and amounts are still enforced at specific indices, so extra outputs cannot divert funds.
There was a problem hiding this comment.
🔍 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:
output[0].spk == buyerSpk(destination locked)output[1].spk == feeSpk(fee destination locked)output[1].value >= inputValue * bp / 10000(minimum fee)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. 👍
🔍 Arkana PR ReviewTest-only PR — P2P exchange escrow integration tests SummaryComprehensive integration test suite for P2P escrow using Arkade introspection opcodes (CSFS, Analysis✅ Good coverage of attack vectors:
✅ Escrow architecture looks sound:
Observations
VerdictWell-structured test suite. No production code changes, so no security risk from merge. The tests themselves validate important security invariants of the escrow contract. |
1 participant
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
ReadArkadeScriptto support all closure types (was previously hardcoded toMultisigClosureonly).Parameters
sellerPubKeybuyerPubKeyarbitratorPubKeybuyerSpkOP_INSPECTOUTPUTSCRIPTPUBKEYon SellerConfirmsellerSpkOP_INSPECTOUTPUTSCRIPTPUBKEYon BuyerRefundfeeSpkOP_INSPECTOUTPUTSCRIPTPUBKEYfeeBasisPointsOP_MUL64/OP_DIV64computationcltvTimeoutcsvTimeouttradeIDOracle messages
SHA256(0x01 || tradeID)— attested by seller (payment received) via CSFSSHA256(0x02 || tradeID)— attested by buyer (voluntary refund) via CSFSVTXO 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.
<buyer> OP_CHECKSIGVERIFY <introspector> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG<seller> OP_CHECKSIGFROMSTACK(RELEASE) OP_VERIFYOP_INSPECTNUMINPUTS 1 OP_EQUALVERIFY0 OP_INSPECTOUTPUTSCRIPTPUBKEY <buyer_ver> OP_EQUALVERIFY <buyer_prog> OP_EQUALVERIFY1 OP_INSPECTOUTPUTSCRIPTPUBKEY <fee_ver> OP_EQUALVERIFY <fee_prog> OP_EQUALVERIFYOP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTVALUE <bp> OP_MUL64 OP_VERIFY <10000> OP_DIV64 OP_VERIFY OP_SWAP OP_DROP1 OP_INSPECTOUTPUTVALUE OP_SWAP OP_GREATERTHANOREQUAL64 OP_VERIFYOP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTVALUE 1 OP_INSPECTOUTPUTVALUE OP_SUB64 OP_VERIFY0 OP_INSPECTOUTPUTVALUE OP_SWAP OP_GREATERTHANOREQUAL64<seller> OP_CHECKSIGVERIFY <introspector> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG<buyer> OP_CHECKSIGFROMSTACK(CANCEL) OP_VERIFY0 OP_INSPECTOUTPUTSCRIPTPUBKEY <seller_ver> OP_EQUALVERIFY <seller_prog> OP_EQUALVERIFYOP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTVALUE 0 OP_INSPECTOUTPUTVALUE OP_SWAP OP_GREATERTHANOREQUAL64<buyer> OP_CHECKSIGVERIFY <introspector> OP_CHECKSIGVERIFY <operator> OP_CHECKSIGOP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTSCRIPTPUBKEY OP_1 OP_EQUALVERIFY0 OP_INSPECTOUTPUTSCRIPTPUBKEY OP_1 OP_EQUALVERIFY OP_EQUALVERIFYOP_PUSHCURRENTINPUTINDEX OP_INSPECTINPUTVALUE 0 OP_INSPECTOUTPUTVALUE OP_LESSTHAN64<buyer> OP_CHECKSIGVERIFY <arbitrator> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG<seller> OP_CHECKSIGVERIFY <arbitrator> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG<locktime> OP_CHECKLOCKTIMEVERIFY OP_DROP<seller> OP_CHECKSIGVERIFY <operator> OP_CHECKSIG<sequence> OP_CHECKSEQUENCEVERIFY OP_DROP<buyer> OP_CHECKSIGVERIFY <seller> OP_CHECKSIG<sequence> OP_CHECKSEQUENCEVERIFY OP_DROP<seller> OP_CHECKSIGIntrospector changes
ReadArkadeScriptnow usesscriptlib.DecodeClosure()instead of hardcodingMultisigClosure.Decode(), supporting all closure typesDesign decisions
input_value x basis_points / 10000computed on-chain viaOP_MUL64/OP_DIV64Test coverage (3 test functions, 10 cases)
TestP2PEscrowSellerConfirm— wrong key, fee too low, wrong fee address, wrong buyer destination, validTestP2PEscrowBuyerRefund— wrong key rejection, wrong seller destination, valid buyer CANCELTestP2PEscrowTopupPath— wrong scriptPubKey, same-value rejectionTest plan
go test ./test/ -run TestP2PEscrow -v— all 3 test functions pass