diff --git a/src/app/cancel.rs b/src/app/cancel.rs index ab22976d..d017a627 100644 --- a/src/app/cancel.rs +++ b/src/app/cancel.rs @@ -1,9 +1,11 @@ use crate::app::bond; +use crate::app::bond::flow::{classify_cancel_error, CancelOutcome}; use crate::app::context::AppContext; use crate::app::dispute::close_dispute_after_user_resolution; use crate::db::{edit_pubkeys_order, update_order_to_initial_state}; use crate::lightning::LndConnector; use crate::util::{enqueue_order_msg, get_order, update_order_event}; +use fedimint_tonic_lnd::lnrpc::invoice::InvoiceState; use mostro_core::db::Crud; use mostro_core::prelude::*; use nostr_sdk::prelude::*; @@ -11,11 +13,24 @@ use sqlx::{Pool, Sqlite}; use std::str::FromStr; use tracing::{info, warn}; +/// The Lightning capabilities the cancel paths need: cancel an escrow, and +/// first find out whether it is safe to. pub trait CancelLightning { fn cancel_hold_invoice<'a>( &'a mut self, hash: &'a str, ) -> std::pin::Pin> + Send + 'a>>; + + /// Current state of the escrow invoice at LND, `None` when the node has no + /// record of it. See [`LndConnector::lookup_invoice_state`]. + fn lookup_invoice_state<'a>( + &'a mut self, + hash: &'a str, + ) -> std::pin::Pin< + Box< + dyn std::future::Future, MostroError>> + Send + 'a, + >, + >; } impl CancelLightning for LndConnector { @@ -30,6 +45,104 @@ impl CancelLightning for LndConnector { .map(|_| ()) }) } + + fn lookup_invoice_state<'a>( + &'a mut self, + hash: &'a str, + ) -> std::pin::Pin< + Box< + dyn std::future::Future, MostroError>> + Send + 'a, + >, + > { + Box::pin(async move { LndConnector::lookup_invoice_state(self, hash).await }) + } +} + +/// What to do with an escrow hold invoice a cancel path is about to void. +#[derive(Debug, PartialEq)] +pub(crate) enum EscrowCancelDecision { + /// Nothing is locked in — cancel the invoice. + Cancel, + /// The seller's HTLC is accepted *right now*: canceling refunds it while + /// `hold_invoice_paid` is concurrently telling the buyer the payment went + /// through. Leave the escrow alone and let the trade advance. + SkipPaid, + /// LND could not be asked. Skipping only delays a cancel; canceling blind + /// can refund a live escrow, so this is the safe direction. + SkipUnknown(String), +} + +/// Decide whether an escrow invoice may be canceled, from the order's status +/// and the invoice's state at LND. +/// +/// The two waiting states give the same `Accepted` opposite meanings: +/// +/// - `waiting-payment` — the seller is supposed *not* to have paid yet. An +/// accepted HTLC means they just did, in the gap between the caller's read +/// and now. That is the race that turns a timeout (or a counterparty +/// cancel) into a refund of a live escrow while the buyer is being told to +/// send fiat. Skip. +/// - `waiting-buyer-invoice` — the seller has already paid by definition, and +/// returning their funds is precisely what canceling here is for. Cancel. +/// +/// `Settled` cannot arise in `waiting-payment` (only a release settles, and +/// only from a live trade), but it is treated like `Accepted`: whatever it +/// means, it is not something to void blindly. `Open`, `Canceled` and an +/// invoice LND has no record of all mean no live escrow. +pub(crate) fn classify_escrow_cancel( + status: Status, + lookup: Result, MostroError>, +) -> EscrowCancelDecision { + if status != Status::WaitingPayment { + return EscrowCancelDecision::Cancel; + } + match lookup { + Ok(Some(InvoiceState::Accepted)) | Ok(Some(InvoiceState::Settled)) => { + EscrowCancelDecision::SkipPaid + } + Ok(_) => EscrowCancelDecision::Cancel, + Err(e) => EscrowCancelDecision::SkipUnknown(e.to_string()), + } +} + +/// [`classify_escrow_cancel`] evaluated against the live node. +pub(crate) async fn decide_escrow_cancel( + ln_client: &mut L, + status: Status, + hash: &str, +) -> EscrowCancelDecision { + classify_escrow_cancel(status, ln_client.lookup_invoice_state(hash).await) +} + +/// Cancel an escrow hold invoice, treating an invoice LND has already voided +/// as success. +/// +/// `cancel_hold_invoice` reports `already canceled` / `not found` as an error, +/// which is a fact rather than a failure — and aborting on it strands the +/// order. Every cancel path here voids the escrow before persisting, so a +/// first attempt that dies in between leaves exactly that shape: the escrow is +/// gone and the order is still live. Without this, the retry that should +/// finish the job (the caller's, or the scheduler's next timeout tick) hits +/// the error again and can never converge. +/// +/// [`classify_cancel_error`] is the same classifier the bond module uses for +/// its own idempotent cancels; anything it cannot place confidently stays an +/// error, so a transient LND problem still aborts. +pub(crate) async fn cancel_escrow_idempotent( + ln_client: &mut L, + order_id: uuid::Uuid, + hash: &str, +) -> Result<(), MostroError> { + match ln_client.cancel_hold_invoice(hash).await { + Ok(()) => Ok(()), + Err(e) => match classify_cancel_error(&e) { + CancelOutcome::AlreadyDone => { + info!("Order Id {order_id}: escrow was already void at LND ({e}); continuing"); + Ok(()) + } + CancelOutcome::Transient => Err(e), + }, + } } /// Reset API-provided quote-derived amounts when republishing an order. @@ -95,7 +208,7 @@ async fn cancel_cooperative_execution_step_2( // Cancel hold invoice if present; if funds were locked, this returns them to the seller. if let Some(hash) = &order.hash { // We return funds to seller - ln_client.cancel_hold_invoice(hash).await?; + cancel_escrow_idempotent(ln_client, order.id, hash).await?; info!( "Cooperative cancel: Order Id {}: Funds returned to seller", &order.id @@ -286,7 +399,7 @@ async fn cancel_order_by_taker_inner( ) -> Result<(), MostroError> { // Cancel hold invoice if present if let Some(hash) = &order.hash { - ln_client.cancel_hold_invoice(hash).await?; + cancel_escrow_idempotent(ln_client, order.id, hash).await?; info!("Order Id {}: Funds returned to seller", &order.id); } @@ -344,18 +457,31 @@ async fn cancel_order_by_maker( request_id: Option, ln_client: &mut L, ) -> Result<(), MostroError> { - // We publish a new replaceable kind nostr event with the status updated - if let Ok(order_updated) = update_order_event(my_keys, Status::Canceled, &order).await { - order_updated - .update(pool) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - } - // Cancel hold invoice if present + // Void the escrow *before* persisting the cancel. On a cancel failure `?` + // returns with the order untouched, so the caller retries against a state + // that still matches the HTLC. Persisting first left a canceled order + // whose hold invoice was still live at LND — payable for as long as its + // expiry allows, and cleaned up by nothing: `find_held_invoices` and the + // escrow-deadline guardian both ignore canceled orders, so the seller's + // funds would sit locked until LND itself voided the invoice near the + // CLTV horizon. Same ordering, and the same reasoning, as the scheduler's + // timeout path and the taker branch above. if let Some(hash) = &order.hash { - ln_client.cancel_hold_invoice(hash).await?; + cancel_escrow_idempotent(ln_client, order.id, hash).await?; info!("Order Id {}: Funds returned to seller", &order.id); } + // We publish a new replaceable kind nostr event with the status updated. + // A failure here must surface instead of being skipped: the escrow is + // already void above, so silently dropping the write left the order live + // behind a dead escrow *and* told both parties it was canceled. Note that + // a relay rejection is not this branch — `update_order_event` queues those + // for republish and returns `Ok` — so this only fires when the event could + // not be built at all. Mirrors the taker branch and `hold_invoice_paid`. + let order_updated = update_order_event(my_keys, Status::Canceled, &order).await?; + order_updated + .update(pool) + .await + .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; enqueue_order_msg( request_id, @@ -686,7 +812,42 @@ async fn cancel_not_active_order( return Err(MostroInternalErr(ServiceError::InvalidPubkey)); }; - if order.sent_from_maker(event.sender).is_ok() { + // Resolve the caller's role *before* the escrow guard below, which talks + // to LND: a sender who is neither party must be rejected as such, without + // costing a lookup RPC and without learning from the error whether the + // escrow is funded. + let sender_is_maker = order.sent_from_maker(event.sender).is_ok(); + if !sender_is_maker && event.sender != taker_pubkey { + return Err(MostroCantDo(CantDoReason::InvalidPubkey)); + } + + // Never void an escrow that is already funded. Both branches below cancel + // the hold invoice, and in `waiting-payment` an accepted HTLC means the + // seller paid between this handler's read and now — refunding them here + // would leave the buyer sending fiat against nothing, since + // `hold_invoice_paid` is concurrently telling them the payment landed. + if let Some(hash) = order.hash.as_deref() { + let status = order.get_order_status().map_err(MostroInternalErr)?; + match decide_escrow_cancel(ln_client, status, hash).await { + EscrowCancelDecision::Cancel => {} + EscrowCancelDecision::SkipPaid => { + warn!( + "Order Id {}: refusing to cancel — the seller's escrow payment just landed", + order.id + ); + return Err(MostroCantDo(CantDoReason::NotAllowedByStatus)); + } + EscrowCancelDecision::SkipUnknown(cause) => { + warn!( + "Order Id {}: could not read the escrow state before canceling ({cause}); rejecting so the caller retries", + order.id + ); + return Err(MostroInternalErr(ServiceError::LnNodeError(cause))); + } + } + } + + if sender_is_maker { cancel_order_by_maker( pool, event, @@ -697,7 +858,7 @@ async fn cancel_not_active_order( ln_client, ) .await?; - } else if event.sender == taker_pubkey { + } else { cancel_order_by_taker( pool, event, @@ -708,8 +869,6 @@ async fn cancel_not_active_order( taker_pubkey, ) .await?; - } else { - return Err(MostroCantDo(CantDoReason::InvalidPubkey)); } Ok(()) } @@ -799,6 +958,186 @@ mod tests { { Box::pin(async move { Ok(()) }) } + + /// Unpaid escrow: the state every existing cancel test assumes. + fn lookup_invoice_state<'a>( + &'a mut self, + _hash: &'a str, + ) -> std::pin::Pin< + Box< + dyn std::future::Future, MostroError>> + + Send + + 'a, + >, + > { + Box::pin(async move { Ok(Some(InvoiceState::Open)) }) + } + } + + /// Escrow stub that reports a chosen invoice state and records whether the + /// hold invoice was canceled, so a test can assert the escrow was *not* + /// touched — the whole point of the guard. + struct StubEscrowLnClient { + /// `None` models an invoice LND has no record of. + state: Option, + fail_lookup: bool, + fail_cancel: Option, + canceled: std::sync::Arc, + looked_up: std::sync::Arc, + } + + impl StubEscrowLnClient { + fn reporting(state: Option) -> Self { + Self { + state, + fail_lookup: false, + fail_cancel: None, + canceled: Default::default(), + looked_up: Default::default(), + } + } + + fn unreachable() -> Self { + Self { + state: None, + fail_lookup: true, + fail_cancel: None, + canceled: Default::default(), + looked_up: Default::default(), + } + } + + /// Unpaid escrow whose cancel LND refuses with a transient error. + fn refusing_cancel() -> Self { + Self { + state: Some(InvoiceState::Open), + fail_lookup: false, + fail_cancel: Some("cancel refused".to_string()), + canceled: Default::default(), + looked_up: Default::default(), + } + } + + /// LND reports the invoice as already void — a fact, not a failure. + fn already_canceled() -> Self { + Self { + state: Some(InvoiceState::Canceled), + fail_lookup: false, + fail_cancel: Some( + "code=Unknown message=invoice with that hash already canceled".to_string(), + ), + canceled: Default::default(), + looked_up: Default::default(), + } + } + + fn escrow_was_canceled(&self) -> bool { + self.canceled.load(std::sync::atomic::Ordering::SeqCst) + } + + fn escrow_was_looked_up(&self) -> bool { + self.looked_up.load(std::sync::atomic::Ordering::SeqCst) + } + } + + impl CancelLightning for StubEscrowLnClient { + fn cancel_hold_invoice<'a>( + &'a mut self, + _hash: &'a str, + ) -> std::pin::Pin> + Send + 'a>> + { + let canceled = self.canceled.clone(); + let fail = self.fail_cancel.clone(); + Box::pin(async move { + if let Some(cause) = fail { + return Err(MostroInternalErr(ServiceError::LnNodeError(cause))); + } + canceled.store(true, std::sync::atomic::Ordering::SeqCst); + Ok(()) + }) + } + + fn lookup_invoice_state<'a>( + &'a mut self, + _hash: &'a str, + ) -> std::pin::Pin< + Box< + dyn std::future::Future, MostroError>> + + Send + + 'a, + >, + > { + let (state, fail) = (self.state, self.fail_lookup); + let looked_up = self.looked_up.clone(); + Box::pin(async move { + looked_up.store(true, std::sync::atomic::Ordering::SeqCst); + if fail { + Err(MostroInternalErr(ServiceError::LnNodeError( + "node unreachable".to_string(), + ))) + } else { + Ok(state) + } + }) + } + } + + // --------------------------------------------------------------- + // classify_escrow_cancel + // --------------------------------------------------------------- + + /// The invariant the whole guard exists for: an accepted HTLC on an order + /// that is still waiting for the seller's payment means they paid just + /// now, so canceling would refund a live escrow. + #[test] + fn escrow_cancel_skips_a_funded_waiting_payment_escrow() { + for state in [InvoiceState::Accepted, InvoiceState::Settled] { + assert_eq!( + classify_escrow_cancel(Status::WaitingPayment, Ok(Some(state))), + EscrowCancelDecision::SkipPaid, + "{state:?} must not be voided" + ); + } + } + + /// Nothing locked in — including an invoice LND no longer has a record of, + /// which cannot be refunding anyone. + #[test] + fn escrow_cancel_proceeds_when_nothing_is_locked_in() { + for lookup in [ + Ok(Some(InvoiceState::Open)), + Ok(Some(InvoiceState::Canceled)), + Ok(None), + ] { + assert_eq!( + classify_escrow_cancel(Status::WaitingPayment, lookup), + EscrowCancelDecision::Cancel + ); + } + } + + /// Delaying a cancel is recoverable; refunding a live escrow is not. + #[test] + fn escrow_cancel_skips_when_lnd_cannot_be_asked() { + let err = MostroInternalErr(ServiceError::LnNodeError("boom".to_string())); + assert!(matches!( + classify_escrow_cancel(Status::WaitingPayment, Err(err)), + EscrowCancelDecision::SkipUnknown(_) + )); + } + + /// The asymmetry that makes the guard status-keyed: in + /// `waiting-buyer-invoice` the seller has paid by definition, and handing + /// their funds back is exactly what the cancel is for. + #[test] + fn escrow_cancel_still_refunds_the_seller_in_waiting_buyer_invoice() { + assert_eq!( + classify_escrow_cancel( + Status::WaitingBuyerInvoice, + Ok(Some(InvoiceState::Accepted)) + ), + EscrowCancelDecision::Cancel + ); } #[tokio::test] @@ -1321,6 +1660,242 @@ mod tests { assert!(after.buyer_pubkey.is_none()); } + /// A cancel racing the seller's payment must not void the escrow: the + /// buyer is being told the payment landed at that very moment, so a refund + /// here leaves them sending fiat against nothing. + #[tokio::test] + async fn maker_cancel_is_rejected_when_the_escrow_is_already_funded() { + set_global_config(); + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let maker = Keys::generate().public_key(); + let taker = Keys::generate().public_key(); + + let mut order = create_pending_order(maker, taker); + order.status = Status::WaitingPayment.to_string(); + order.hash = Some("stub-hold-invoice-hash".to_string()); + let order = order.create(ctx.pool()).await.unwrap(); + + let event = create_unwrapped_message_with_pubkey(maker); + let mut ln = StubEscrowLnClient::reporting(Some(InvoiceState::Accepted)); + let result = cancel_action_generic( + &ctx, + cancel_msg(order.id), + &event, + &Keys::generate(), + &mut ln, + ) + .await; + + assert!( + matches!(result, Err(MostroCantDo(CantDoReason::NotAllowedByStatus))), + "a funded escrow must reject the cancel: {result:?}" + ); + assert!( + !ln.escrow_was_canceled(), + "the seller's accepted HTLC must not be refunded" + ); + assert_eq!( + order_by_id(ctx.pool(), order.id).await.status, + Status::WaitingPayment.to_string(), + "the order must be left for the trade to advance" + ); + } + + /// The guard sits above the maker/taker routing, so the taker side of the + /// same race is covered too. + #[tokio::test] + async fn taker_cancel_is_rejected_when_the_escrow_is_already_funded() { + set_global_config(); + let pool = setup_pool().await; + set_global_db_pool(&pool); + let ctx = build_ctx(pool.clone()); + let maker = Keys::generate().public_key(); + let taker = Keys::generate().public_key(); + + let mut order = create_pending_order(maker, taker); + order.status = Status::WaitingPayment.to_string(); + order.master_seller_pubkey = Some(maker.to_string()); + order.hash = Some("stub-hold-invoice-hash".to_string()); + let order = order.create(ctx.pool()).await.unwrap(); + + let event = create_unwrapped_message_with_pubkey(taker); + let mut ln = StubEscrowLnClient::reporting(Some(InvoiceState::Accepted)); + let result = cancel_action_generic( + &ctx, + cancel_msg(order.id), + &event, + &Keys::generate(), + &mut ln, + ) + .await; + + assert!( + matches!(result, Err(MostroCantDo(CantDoReason::NotAllowedByStatus))), + "a funded escrow must reject the cancel: {result:?}" + ); + assert!(!ln.escrow_was_canceled()); + assert_eq!( + order_by_id(ctx.pool(), order.id).await.status, + Status::WaitingPayment.to_string() + ); + } + + /// An unreadable escrow state is rejected rather than canceled blind, and + /// as an internal error so the caller retries instead of believing the + /// order is gone. + #[tokio::test] + async fn cancel_is_rejected_when_the_escrow_state_cannot_be_read() { + set_global_config(); + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let maker = Keys::generate().public_key(); + let taker = Keys::generate().public_key(); + + let mut order = create_pending_order(maker, taker); + order.status = Status::WaitingPayment.to_string(); + order.hash = Some("stub-hold-invoice-hash".to_string()); + let order = order.create(ctx.pool()).await.unwrap(); + + let event = create_unwrapped_message_with_pubkey(maker); + let mut ln = StubEscrowLnClient::unreachable(); + let result = cancel_action_generic( + &ctx, + cancel_msg(order.id), + &event, + &Keys::generate(), + &mut ln, + ) + .await; + + assert!( + matches!(result, Err(MostroInternalErr(ServiceError::LnNodeError(_)))), + "an unreadable escrow must reject the cancel: {result:?}" + ); + assert!(!ln.escrow_was_canceled()); + assert_eq!( + order_by_id(ctx.pool(), order.id).await.status, + Status::WaitingPayment.to_string() + ); + } + + /// The buyer-side timeout still refunds the seller: in + /// `waiting-buyer-invoice` the escrow is funded on purpose, and the guard + /// must not stand in the way of returning it. + #[tokio::test] + async fn waiting_buyer_invoice_cancel_still_returns_the_funded_escrow() { + set_global_config(); + let pool = setup_pool().await; + set_global_db_pool(&pool); + let ctx = build_ctx(pool.clone()); + let maker = Keys::generate().public_key(); + let taker = Keys::generate().public_key(); + + let mut order = create_pending_order(maker, taker); + order.status = Status::WaitingBuyerInvoice.to_string(); + order.master_seller_pubkey = Some(maker.to_string()); + order.hash = Some("stub-hold-invoice-hash".to_string()); + let order = order.create(ctx.pool()).await.unwrap(); + + let event = create_unwrapped_message_with_pubkey(taker); + let mut ln = StubEscrowLnClient::reporting(Some(InvoiceState::Accepted)); + let result = cancel_action_generic( + &ctx, + cancel_msg(order.id), + &event, + &Keys::generate(), + &mut ln, + ) + .await; + + assert!(result.is_ok(), "taker cancel must succeed: {result:?}"); + assert!( + ln.escrow_was_canceled(), + "the seller's funds must be returned" + ); + assert_eq!( + order_by_id(ctx.pool(), order.id).await.status, + Status::Pending.to_string() + ); + } + + /// The escrow is voided before the cancel is persisted, so a refused + /// cancel leaves the order intact for the caller to retry. Persisting + /// first would strand a canceled order behind a live, still-payable hold + /// invoice that no job cleans up. + #[tokio::test] + async fn maker_cancel_is_not_persisted_when_the_escrow_cancel_fails() { + set_global_config(); + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let maker = Keys::generate().public_key(); + let taker = Keys::generate().public_key(); + + let mut order = create_pending_order(maker, taker); + order.status = Status::WaitingPayment.to_string(); + order.hash = Some("stub-hold-invoice-hash".to_string()); + let order = order.create(ctx.pool()).await.unwrap(); + + let event = create_unwrapped_message_with_pubkey(maker); + let mut ln = StubEscrowLnClient::refusing_cancel(); + let result = cancel_action_generic( + &ctx, + cancel_msg(order.id), + &event, + &Keys::generate(), + &mut ln, + ) + .await; + + assert!( + matches!(result, Err(MostroInternalErr(ServiceError::LnNodeError(_)))), + "a refused escrow cancel must surface: {result:?}" + ); + assert_eq!( + order_by_id(ctx.pool(), order.id).await.status, + Status::WaitingPayment.to_string(), + "the order must not be canceled while its hold invoice is live" + ); + } + + /// An escrow LND already voided must not block the cancel: that is the + /// retry that finishes a first attempt which died between voiding the + /// escrow and persisting, and without this it can never converge. + #[tokio::test] + async fn maker_cancel_converges_when_the_escrow_is_already_void() { + set_global_config(); + let pool = setup_pool().await; + let ctx = build_ctx(pool.clone()); + let maker = Keys::generate().public_key(); + let taker = Keys::generate().public_key(); + + let mut order = create_pending_order(maker, taker); + order.status = Status::WaitingPayment.to_string(); + order.hash = Some("stub-hold-invoice-hash".to_string()); + let order = order.create(ctx.pool()).await.unwrap(); + + let event = create_unwrapped_message_with_pubkey(maker); + let mut ln = StubEscrowLnClient::already_canceled(); + let result = cancel_action_generic( + &ctx, + cancel_msg(order.id), + &event, + &Keys::generate(), + &mut ln, + ) + .await; + + assert!( + result.is_ok(), + "an already-void escrow must not abort the cancel: {result:?}" + ); + assert_eq!( + order_by_id(ctx.pool(), order.id).await.status, + Status::Canceled.to_string(), + "the retry must finish what the first attempt left half-done" + ); + } + #[tokio::test] async fn cancel_not_active_order_rejects_intruder() { let pool = setup_pool().await; @@ -1330,15 +1905,17 @@ mod tests { let mut order = create_pending_order(maker, taker); order.status = Status::WaitingPayment.to_string(); + order.hash = Some("stub-hold-invoice-hash".to_string()); let order = order.create(ctx.pool()).await.unwrap(); let event = create_unwrapped_message_with_pubkey(Keys::generate().public_key()); + let mut ln = StubEscrowLnClient::reporting(Some(InvoiceState::Accepted)); let result = cancel_action_generic( &ctx, cancel_msg(order.id), &event, &Keys::generate(), - &mut StubLnClient, + &mut ln, ) .await; @@ -1346,6 +1923,14 @@ mod tests { result, Err(MostroCantDo(CantDoReason::InvalidPubkey)) )); + // The escrow guard runs after authorization, so a stranger neither + // costs a lookup RPC nor learns from the error that the escrow is + // funded — the funded escrow above would otherwise answer + // `NotAllowedByStatus`. + assert!( + !ln.escrow_was_looked_up(), + "an unauthorized sender must not reach the node" + ); } #[tokio::test] diff --git a/src/flow.rs b/src/flow.rs index 5a275329..29f27794 100644 --- a/src/flow.rs +++ b/src/flow.rs @@ -19,6 +19,13 @@ use tracing::info; /// lived on in LND — is a no-op that returns `Ok` after a warning, never an /// error, since there is nothing for the caller to retry. /// +/// Notifications are enqueued only after the new status is persisted: the +/// buyer's next step on `HoldInvoicePaymentAccepted` is to send fiat, so it +/// must never be sent for a transition that did not land. A failed publish or +/// write returns `Ok` without notifying and without stamping +/// `invoice_held_at`, leaving the order eligible for the replay +/// `crate::db::find_held_invoices` drives on restart. +/// /// Not atomic: the check and the write are separate statements, so a cancel /// running concurrently on the main loop can still interleave. See #855. pub async fn hold_invoice_paid( @@ -100,6 +107,12 @@ pub async fn hold_invoice_paid( ); let status; + // Messages are built here but sent only once the transition is durably + // persisted (see below), so a party is never told the escrow advanced on + // the strength of a write that did not land. + let mut pending_msgs: Vec<(Action, Option, PublicKey)> = Vec::new(); + let mut notify_reputation = false; + // Dev fee is NOT charged to users - it's paid by mostrod from its earnings if order.buyer_invoice.is_some() { status = Status::Active; @@ -107,27 +120,19 @@ pub async fn hold_invoice_paid( // We send a confirmation message to seller let mut seller_order_data = order_data.clone(); seller_order_data.amount = order.amount.saturating_add(order.fee); - enqueue_order_msg( - request_id, - Some(order.id), + pending_msgs.push(( Action::BuyerTookOrder, Some(Payload::Order(seller_order_data)), seller_pubkey, - None, - ) - .await; + )); // We send a message to buyer saying seller paid let mut buyer_order_data = order_data.clone(); buyer_order_data.amount = order.amount.saturating_sub(order.fee); - enqueue_order_msg( - request_id, - Some(order.id), + pending_msgs.push(( Action::HoldInvoicePaymentAccepted, Some(Payload::Order(buyer_order_data)), buyer_pubkey, - None, - ) - .await; + )); } else { let new_amount = order_data.amount - order.fee; order_data.amount = new_amount; @@ -145,43 +150,86 @@ pub async fn hold_invoice_paid( // `updated_order.update` below. order.set_timestamp_now(); // We ask to buyer for a new invoice - enqueue_order_msg( - request_id, - Some(order.id), + pending_msgs.push(( Action::AddInvoice, Some(Payload::Order(order_data)), buyer_pubkey, - None, - ) - .await; + )); // We send a message to seller we are waiting for buyer invoice + pending_msgs.push((Action::WaitingBuyerInvoice, None, seller_pubkey)); + + notify_reputation = true; + } + // Stamp the idempotency marker on the struct so the transition below + // carries it: `Crud::update` writes the whole row, so status, event id and + // marker land in one statement. A separate write after the transition left + // a window where a failed stamp meant a persisted transition with + // `invoice_held_at == 0`, and the replay `find_held_invoices` drives would + // pass the guard and queue the prompts a second time. There is nothing to + // order between them: the marker means "this delivery was processed", and + // the transition is what processing it produces. + order.invoice_held_at = Timestamp::now().as_secs() as i64; + + // We publish a new replaceable kind nostr event with the status updated + // and update on local database the status and new event id + let persisted = match crate::util::update_order_event(my_keys, status, &order).await { + Ok(updated_order) => match updated_order.update(pool).await { + Ok(_) => true, + Err(e) => { + tracing::error!(order_id = %order.id, "hold_invoice_paid: could not persist {status}: {e}"); + false + } + }, + Err(e) => { + tracing::error!(order_id = %order.id, "hold_invoice_paid: could not publish {status}: {e}"); + false + } + }; + + // Bail before notifying. The parties must never be told the escrow + // advanced — least of all the buyer, whose next step is to send fiat — on + // the strength of a transition that was not stored. The marker rode in + // that same failed write, so it is still 0 and the order stays eligible + // for the replay `find_held_invoices` drives on restart, which re-runs + // this whole flow. + if !persisted { + tracing::warn!( + order_id = %order.id, + "hold_invoice_paid: transition not persisted; not notifying, leaving the order for the invoice replay to retry" + ); + return Ok(()); + } + + for (action, payload, destination) in pending_msgs { enqueue_order_msg( request_id, Some(order.id), - Action::WaitingBuyerInvoice, - None, - seller_pubkey, + action, + payload, + destination, None, ) .await; + } - // Notify taker reputation to maker + if notify_reputation { + // Notify taker reputation to maker. Best effort on purpose: this is an + // extra message riding alongside the `AddInvoice` already queued above, + // and the transition it belongs to is already committed — failing the + // call here would report an error for work that did land. + // `notify_taker_reputation` keys on the order's *current* status (the + // same contract `add_invoice` relies on), and can also fail on a + // missing master pubkey; neither is worth failing the delivery over. tracing::info!("Notifying taker reputation to maker"); - notify_taker_reputation(pool, &order).await?; - } - // We publish a new replaceable kind nostr event with the status updated - // and update on local database the status and new event id - if let Ok(updated_order) = crate::util::update_order_event(my_keys, status, &order).await { - // Update order on db - let _ = updated_order.update(pool).await; + if let Err(e) = notify_taker_reputation(pool, &order).await { + tracing::warn!( + order_id = %order.id, + "hold_invoice_paid: taker reputation notice skipped: {e}" + ); + } } - // Update the invoice_held_at field - crate::db::update_order_invoice_held_at_time(pool, order.id, Timestamp::now().as_secs() as i64) - .await - .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; - Ok(()) } @@ -366,6 +414,69 @@ mod tests { order.create(pool).await.unwrap() } + /// Every message this flow sends is queued by order id, so a test can + /// assert that nothing at all was sent. + async fn queued_actions_for(order_id: uuid::Uuid) -> Vec { + crate::config::MESSAGE_QUEUES + .queue_order_msg + .read() + .await + .iter() + .filter(|(msg, _)| msg.get_inner_message_kind().id == Some(order_id)) + .map(|(msg, _)| msg.get_inner_message_kind().action.clone()) + .collect() + } + + /// The buyer's next move on `HoldInvoicePaymentAccepted` is to send fiat, + /// so it must never go out for a transition that was not stored — and the + /// order must stay replayable rather than be stamped as processed. A + /// read-only pool fails the write while leaving the status read intact. + #[tokio::test] + async fn hold_invoice_paid_does_not_notify_when_the_write_fails() { + init_global_settings(); + let pool = sqlx::sqlite::SqlitePoolOptions::new() + .max_connections(1) + .connect(":memory:") + .await + .unwrap(); + sqlx::migrate!("./migrations").run(&pool).await.unwrap(); + let hash = "cc".repeat(32); + let buyer = create_test_keys().public_key().to_string(); + let seller = create_test_keys().public_key().to_string(); + let order = insert_order_with_hash( + &pool, + &hash, + Status::WaitingPayment, + Some("lnbcrt1invoice".to_string()), + Some(buyer), + Some(seller), + None, + ) + .await; + // One connection in the pool, so the flag holds for every later query. + sqlx::query("PRAGMA query_only = ON") + .execute(&pool) + .await + .unwrap(); + + let result = hold_invoice_paid(&hash, Some(1), &pool, &create_test_keys()).await; + + assert!( + result.is_ok(), + "a lost write is not the caller's to retry: {result:?}" + ); + let after = crate::db::find_order_by_hash(&pool, &hash).await.unwrap(); + assert_eq!(after.status, Status::WaitingPayment.to_string()); + assert_eq!( + after.invoice_held_at, 0, + "the order must stay eligible for the invoice replay" + ); + assert!( + queued_actions_for(order.id).await.is_empty(), + "no party may be told the escrow advanced" + ); + } + #[tokio::test] async fn hold_invoice_paid_with_buyer_invoice_activates_order() { init_global_settings(); @@ -422,6 +533,51 @@ mod tests { assert!(updated.invoice_held_at > 0, "invoice_held_at must be set"); } + /// A failing reputation notice must not cost the idempotency stamp: the + /// transition is already persisted and the prompts already queued, so + /// aborting here would leave the order replayable and a resubscribe would + /// duplicate them. Driven from the one status/kind pair + /// `notify_taker_reputation` refuses — sell order still in + /// `waiting-payment`, which no take path actually produces (`take_sell` + /// without an invoice goes straight to `waiting-buyer-invoice`), so this + /// stands in for any failure it can return, a missing master pubkey + /// included. + #[tokio::test] + async fn hold_invoice_paid_stamps_even_when_the_reputation_notice_fails() { + init_global_settings(); + let pool = create_migrated_pool().await; + let hash = "ee".repeat(32); + let buyer = create_test_keys().public_key().to_string(); + let seller = create_test_keys().public_key().to_string(); + let master_buyer = create_test_keys().public_key().to_string(); + let order = insert_order_with_hash( + &pool, + &hash, + Status::WaitingPayment, + None, + Some(buyer), + Some(seller), + Some(master_buyer), + ) + .await; + + let result = hold_invoice_paid(&hash, None, &pool, &create_test_keys()).await; + assert!( + result.is_ok(), + "a refused reputation notice must not fail the flow: {result:?}" + ); + + let updated = crate::db::find_order_by_hash(&pool, &hash).await.unwrap(); + assert_eq!(updated.status, Status::WaitingBuyerInvoice.to_string()); + assert!( + updated.invoice_held_at > 0, + "the order must be marked processed, or a replay duplicates the prompts" + ); + let queued = queued_actions_for(order.id).await; + assert!(queued.contains(&Action::AddInvoice)); + assert!(queued.contains(&Action::WaitingBuyerInvoice)); + } + #[tokio::test] async fn hold_invoice_paid_reanchors_taken_at_for_buyer_duty() { init_global_settings(); @@ -503,6 +659,58 @@ mod tests { ); } + /// The marker rides in the same write as the transition, so once the flow + /// has run its prompts cannot be queued twice — which is what a replayed + /// `Accepted` would do on every restart. Driven from + /// `waiting-buyer-invoice`, the else-branch's own output state, where the + /// status is unchanged by the transition and the marker is the only thing + /// standing between a replay and a duplicate prompt. + #[tokio::test] + async fn hold_invoice_paid_replay_does_not_duplicate_the_prompts() { + init_global_settings(); + let pool = create_migrated_pool().await; + let hash = "77".repeat(32); + let buyer = create_test_keys().public_key().to_string(); + let seller = create_test_keys().public_key().to_string(); + let master_buyer = create_test_keys().public_key().to_string(); + let order = insert_order_with_hash( + &pool, + &hash, + Status::WaitingBuyerInvoice, + None, + Some(buyer), + Some(seller), + Some(master_buyer), + ) + .await; + + let keys = create_test_keys(); + assert!(hold_invoice_paid(&hash, None, &pool, &keys).await.is_ok()); + let stamped = crate::db::find_order_by_hash(&pool, &hash).await.unwrap(); + assert!( + stamped.invoice_held_at > 0, + "the transition must carry the marker" + ); + + // The replay LND delivers on every resubscribe. + assert!(hold_invoice_paid(&hash, None, &pool, &keys).await.is_ok()); + + let queued = queued_actions_for(order.id).await; + assert_eq!( + queued.iter().filter(|a| **a == Action::AddInvoice).count(), + 1, + "the buyer must be asked for an invoice exactly once: {queued:?}" + ); + assert_eq!( + queued + .iter() + .filter(|a| **a == Action::WaitingBuyerInvoice) + .count(), + 1, + "the seller must be told exactly once: {queued:?}" + ); + } + #[tokio::test] async fn hold_invoice_paid_is_a_noop_on_redelivery() { init_global_settings(); diff --git a/src/lightning/mod.rs b/src/lightning/mod.rs index 1860b85f..237c5743 100644 --- a/src/lightning/mod.rs +++ b/src/lightning/mod.rs @@ -286,6 +286,46 @@ impl LndConnector { .min()) } + /// Current state of a hold invoice at LND, or `None` when the node has no + /// record of it (already garbage-collected, or a hash we never created). + /// + /// Callers about to cancel an escrow need this: on an order still waiting + /// for the seller's payment, `Accepted` means their HTLC is locked in + /// *right now*, and canceling refunds it. See + /// `crate::app::cancel::classify_escrow_cancel`. + pub async fn lookup_invoice_state( + &mut self, + hash: &str, + ) -> Result, MostroError> { + let r_hash = decode_hash32("payment hash", hash)?; + + let invoice = match self + .client + .lightning() + .lookup_invoice(PaymentHash { + r_hash, + ..Default::default() + }) + .await + { + Ok(invoice) => invoice.into_inner(), + Err(status) => { + if status.code() == fedimint_tonic_lnd::tonic::Code::NotFound { + return Ok(None); + } + return Err(MostroInternalErr(ServiceError::LnNodeError(format!( + "code={:?} message={}", + status.code(), + status.message() + )))); + } + }; + + InvoiceState::try_from(invoice.state) + .map(Some) + .map_err(|e| MostroInternalErr(ServiceError::LnNodeError(e.to_string()))) + } + pub async fn send_payment( &mut self, payment_request: &str, diff --git a/src/scheduler.rs b/src/scheduler.rs index 5256c691..dbf4508f 100644 --- a/src/scheduler.rs +++ b/src/scheduler.rs @@ -1,4 +1,5 @@ use crate::app::bond; +use crate::app::cancel::{cancel_escrow_idempotent, decide_escrow_cancel, EscrowCancelDecision}; use crate::app::context::AppContext; use crate::app::dev_fee::run_dev_fee_cycle; use crate::app::release::{do_payment, reconcile_inflight_payout}; @@ -503,9 +504,50 @@ async fn job_cancel_orders(ctx: AppContext) { if order.status == Status::WaitingBuyerInvoice.to_string() || order.status == Status::WaitingPayment.to_string() { + // Resolved before touching the escrow: an order whose + // status or kind cannot be parsed must not have its + // hold invoice canceled either, and the escrow guard + // below needs the status. + let (order_status, order_kind) = + match (order.get_order_status(), order.get_order_kind()) { + (Ok(status), Ok(kind)) => (status, kind), + _ => { + tracing::warn!( + "Error getting order status or kind in order {} cancel", + order.id + ); + continue; + } + }; // If hold invoice is paid return funds to seller // We return funds to seller if let Some(hash) = order.hash.as_ref() { + // The seller may have paid in the gap between + // `reconfirm_timeout_eligibility` above and this + // moment — the timeout boundary is public, so this + // is also where a seller can aim deliberately. + // Canceling then refunds their live escrow while + // `hold_invoice_paid` tells the buyer to send + // fiat, so ask LND first and leave a paid escrow + // alone: the next tick sees the order active (or + // re-anchored) and skips it on its own. + match decide_escrow_cancel(&mut ln_client, order_status, hash).await { + EscrowCancelDecision::Cancel => {} + EscrowCancelDecision::SkipPaid => { + warn!( + "scheduler_timeout: order {} has a funded escrow — the seller paid at the timeout boundary; leaving it for the trade to advance", + order.id + ); + continue; + } + EscrowCancelDecision::SkipUnknown(cause) => { + warn!( + "scheduler_timeout: could not read the escrow state for order {} ({cause}); skipping so next tick retries", + order.id + ); + continue; + } + } // The cancel must succeed before we clear the // order. Falling through on error would take the // order out of `find_order_by_seconds`'s @@ -515,7 +557,9 @@ async fn job_cancel_orders(ctx: AppContext) { // Same reasoning as the bond slash/release below: // stay eligible and retry rather than persist a // state that doesn't match the HTLC. - if let Err(e) = ln_client.cancel_hold_invoice(hash).await { + if let Err(e) = + cancel_escrow_idempotent(&mut ln_client, order.id, hash).await + { error!( "scheduler_timeout: cancel_hold_invoice failed for order {} ({e}); skipping cancel/republish so next tick retries", order.id @@ -544,19 +588,6 @@ async fn job_cancel_orders(ctx: AppContext) { order.fee = 0; } - // Get order status and kind - let (order_status, order_kind) = - match (order.get_order_status(), order.get_order_kind()) { - (Ok(status), Ok(kind)) => (status, kind), - _ => { - tracing::warn!( - "Error getting order status or kind in order {} cancel", - order.id - ); - continue; - } - }; - // Phase 4: run the bond slash/release **before** any // DB mutation that takes the order out of // `find_order_by_seconds`'s