feat(cashu): C4 — escrow primitives (2-of-3 NUT-11 lock, sign, redeem, reclaim)#236
feat(cashu): C4 — escrow primitives (2-of-3 NUT-11 lock, sign, redeem, reclaim)#236grunch wants to merge 6 commits into
Conversation
…, reclaim) Phase C4 of docs/cashu/README.md, the cryptographic heart of Cashu-mode trading. No UI and no bridge surface, so review can be about correctness alone. Stacked on C2 (#235) — the escrow methods hang off the wallet from that phase. `rust/src/cashu/escrow.rs`: - `xonly_to_cashu_pubkey` — the `02`-prefix mapping the daemon applies in `cashu_pubkey_from_xonly_hex`. Rejects anything that is not exactly 64 hex chars instead of padding or truncating: a silently reinterpreted key locks funds to nobody, and nothing downstream would notice. - `escrow_conditions` — the 2-of-3 of §2: data = P_S, pubkeys = [P_B, P_M], n_sigs = 2, SIG_INPUTS, locktime, refund = [P_S], n_sigs_refund = 1. - `fee_conditions` — 1-of-1 to Mostro. The fee is a payment, not an escrow; conditions would make it unspendable for the node. - `verify_conditions` / `verify_escrow_token` — the client-side mirror of the daemon's composite check, run *per proof*: a token whose first proof is correct and whose second is locked to the builder alone would pass a spot check and walk away with the difference. - `sign_proofs`, `combine_and_redeem`, `reclaim_after_locktime` — one signature per proof keyed by that proof's secret, matching the wire form C0 pinned. A missing peer signature fails before the mint is contacted. The spike (docs/cashu/cdk-spike.md) established that cdk exposes NUT-11 with custom tags in full, so none of this hand-builds secrets or witness encodings. Tests - Unit: the `02` mapping against a fresh key; five malformed-key shapes rejected; the built condition checked field by field against §2 (a wrong default cannot hide behind a passing round trip); a past locktime refused at construction; and verification refused for every way an escrow can be wrong — wrong seller, n_sigs=1, an extra refund key, a bare P2PK, a missing counterparty, a locktime shorter than required. - Integration (`#[ignore]`, MOSTRO_TEST_MINT_URL): full lock → verify → sign → combine → redeem against a real mint; one signature alone cannot move an escrow; an impostor's signature does not settle one; a short-changed escrow fails verification.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
Review — C4 (strict pass)
Scope reviewed: 🛑 Critical1. if !pubkeys.contains(&parties.buyer) || !pubkeys.contains(&parties.mostro) {
bail!("InvalidEscrowToken: buyer or Mostro key missing");
}This checks the two keys are present. It never checks nothing else is. A seller can build an escrow with Every other field is exact-matched, including if pubkeys.len() != 2
|| !pubkeys.contains(&parties.buyer)
|| !pubkeys.contains(&parties.mostro)
{
bail!("InvalidEscrowToken: pubkeys must be exactly [buyer, mostro]");
}Note 🔴 Major2. No currency-unit check —
3. No distinctness check on the three parties — Nothing rejects 4. Reserved-proof leak on a failed Same defect as 🟡 Minor5. 6. ✅ What is right
Test gaps
|
…-of-3 Addresses the strict review on #236. Critical - `verify_conditions` checked that the buyer and Mostro keys were *present* in `pubkeys`, never that nothing else was. A seller could lock with `pubkeys = [P_B, P_M, P_attacker]` where they hold the extra key: with `data = P_S` and `n_sigs = 2`, seller + attacker is two of four, and the escrow is spendable unilaterally the moment it is funded — while this verifier called the token good. Now matched exactly, like every other field. The test table gained the row that was missing, which is what let it through. Major - No currency-unit check. `Token::value()` sums proof amounts irrespective of denomination, so a token in another unit with the right numeric total passed the amount check. Unit is now asserted before the amount. - Nothing rejected `buyer == seller`, `seller == mostro` or `buyer == mostro`. A duplicate collapses the threshold, and whether one signature then satisfies `n_sigs = 2` is mint-implementation-defined. `EscrowParties::ensure_distinct` runs at construction and again in verification. - `build_locked_token` leaked the whole escrow amount as reserved proofs when `confirm` failed (it consumes the handle, so nothing else could release them). It now reclaims and reports how much came back. Minor - `combine_and_redeem` scanned the peer signatures per proof; indexed by secret once instead. - `reclaim_after_locktime` relied on the mint to refuse a premature spend. The locktime is in the secret, so it now says "the refund path opens in N seconds" instead of surfacing an opaque mint error. Tests: the extra-pubkey case, the three degenerate-party cases, and a wrong-key-with-right-count case that keeps the membership check honest now that the count check fires first.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…mint Same treatment as C2, applied to the escrow suite once a real nutshell showed the tests could not pass at all, and then could not pass twice. - The four funds-using tests asserted "fund the seller wallet first" and returned. They now fund themselves through `mint_for_test`, so a reviewer can actually run them. - Fixed seeds plus a fresh DB replay NUT-13 blinding secrets; the mint answers "Blinded Message is already signed" on the second run. Seeds are unique per process and per call now. - The settlement test pinned the redeemed amount and the seller's balance to the face value. nutshell's default keyset charges a swap fee, so redeeming 16 sat yields less and locking costs more. The face value is what a validator checks; the assertions bound the rest. - `one_signature_is_not_enough_to_move_an_escrow` expected the *mint* to refuse a premature reclaim. Since the local locktime check added in this PR, the client refuses first with the time remaining — a better message for the same property. Either refusal is accepted. Verified 8/8 against nutshell 0.20.3, twice in a row (MINT_RATE_LIMIT=FALSE — the mint rate limits by default).
|
@coderabbitai Review |
✅ Action performedReview finished.
|
1 participant
Phase C4 of
docs/cashu/README.md— the cryptographic heart of Cashu-modetrading. No UI, no bridge surface, so a review can be about correctness alone.
What it does
The escrow is one NUT-11 P2PK secret per proof, locked 2-of-3 (§2 of the doc):
xonly_to_cashu_pubkey— the02-prefix mapping the daemon applies incashu_pubkey_from_xonly_hex. Anything that is not exactly 64 hex charactersis rejected rather than padded or truncated: a silently reinterpreted key
locks funds to nobody, and nothing downstream would notice.
escrow_conditions/fee_conditions— the 2-of-3 above, and a plain1-of-1 to Mostro for the fee. The fee is a payment, not an escrow; giving it
the escrow's conditions would make it unspendable for the node.
verify_escrow_token— the client-side mirror of the daemon's compositecheck, run per proof. A token whose first proof is correct and whose
second is locked to the builder alone would pass a spot check and walk away
with the difference. Also checks mint and total amount.
sign_proofs/combine_and_redeem/reclaim_after_locktime— onesignature per proof, keyed by that proof's secret, matching the wire form C0
pinned. A missing peer signature fails before the mint is contacted, so a
half-signed spend is never attempted.
The spike (
docs/cashu/cdk-spike.md, in #235) established that cdk exposesNUT-11 with custom tags in full — so none of this hand-builds secrets or witness
encodings, which is what kept the phase to this size.
Tests
Unit (run in CI): the
02mapping against a fresh key; five malformed-keyshapes rejected, including a 33-byte compressed key passed where x-only was
expected; the built condition asserted field by field against §2, so a wrong
default cannot hide behind a passing round trip; a past locktime refused at
construction; and verification refused for every way an escrow can be wrong —
wrong seller key,
n_sigs=1, an extra refund key, a bare P2PK, a missingcounterparty, a locktime shorter than required.
Integration (
#[ignore], needsMOSTRO_TEST_MINT_URL) — the "done when" ofthe phase:
refused by the mint);
Verification
No
frb-generateneeded — this phase adds nothing underrust/src/api/.Test plan
x-only input (compare against
cashu_pubkey_from_xonly_hexon the Track Abranch).
until C5.
Next: C5 wires this into the take flow.
Manual verification
This PR is pure cryptography with no UI, so the verification that matters is
the integration suite — and it now genuinely runs (it could not before; see the
last commit). The security properties are the point: verify those four tests
fail if you break the code, not just that they pass.
Already run on this branch
Plus the integration suite against a real mint — 8/8, twice in a row
(nutshell 0.20.3).
A · Integration suite against a live mint
Expect 8 passed, and 8 again on a second run.
MINT_RATE_LIMIT=FALSEisrequired for the second run (nutshell rate limits by default).
Four of those eight are the security properties, against a real mint with real
blind signatures:
an_escrow_locks_and_settles_with_two_of_three_signaturesone_signature_is_not_enough_to_move_an_escrowa_signature_from_the_wrong_key_does_not_settle_an_escrowan_escrow_for_the_wrong_amount_fails_verificationB · Confirm the fix that this PR's review found
The verifier used to accept extra pubkeys, which let a seller add a key they
also control and spend the escrow alone. To convince yourself it is closed:
Then break it deliberately — in
rust/src/cashu/escrow.rs, delete thepubkeys.len() != 2check — and re-run. The test must fail on the"extra pubkey" row. Put it back.
Do the same with
three_parties_must_be_three_different_keysby removing theensure_distinct()call inescrow_conditions.C · Key encoding against the daemon
The
02-prefix mapping must match the daemon'scashu_pubkey_from_xonly_hexexactly, or the escrow locks to a key nobody holds.
cargo test --lib an_xonly_key_maps_to_the_daemons_compressed_formcovers the shape; if you havethe daemon's Track A branch checked out, feed the same x-only hex to both and
compare the output byte for byte.
D · Nothing else moved
This PR adds a module and two crate-internal accessors on the wallet. It has
no callers until C5, so a Lightning build cannot reach any of it. The
regression check is simply that
cargo testand the app behave as on the basebranch.