From fd8ac5de58a93a3ae79d154ec56c77abdf455a2d Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:13:36 -0600 Subject: [PATCH 01/10] feat(lightning): look up a hold invoice's current state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preparatory: the cancel paths are about to consult LND before voiding an escrow, and today they have no way to ask. lookup_invoice_state mirrors lookup_payment_status, including its handling of a gRPC NotFound as Ok(None) rather than an error: an invoice LND has no record of (garbage-collected, or a hash we never created) is an answer, not a failure, and callers must be able to tell it apart from a transport problem. Added to the CancelLightning trait as well, so the cancel handler gets the capability through the seam it already uses for cancel_hold_invoice and its test stub can drive both. The stub reports Open — the unpaid escrow every existing cancel test assumes. --- src/app/cancel.rs | 39 +++++++++++++++++++++++++++++++++++++++ src/lightning/mod.rs | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/src/app/cancel.rs b/src/app/cancel.rs index ab22976d..a69f3439 100644 --- a/src/app/cancel.rs +++ b/src/app/cancel.rs @@ -4,6 +4,7 @@ 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 +12,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 +44,17 @@ 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 }) + } } /// Reset API-provided quote-derived amounts when republishing an order. @@ -799,6 +824,20 @@ 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)) }) + } } #[tokio::test] 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, From 125ce44d375b54ac234d7222a3316503415a0d1b Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:27:06 -0600 Subject: [PATCH 02/10] fix: never cancel a hold invoice the seller just paid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both cancel paths for a waiting-payment order voided the escrow without asking LND about it. If the seller's payment lands in the gap between the caller's read and the cancel RPC, canceling refunds their accepted HTLC — while hold_invoice_paid, running off the invoice subscription, tells the buyer the payment went through and to send fiat. The escrow is gone, the release can no longer settle, and nothing notices: hold_invoice_canceled only alarms past the CLTV deadline. The scheduler path is a free lottery (the timeout boundary is public, a mistimed attempt is just a normal trade) and reconfirm_timeout_eligibility does not close it — the payment lands after the re-read. The cancel_action path is worse: the seller drives both sides, paying and then canceling. classify_escrow_cancel now decides from the invoice state, keyed on status because the two waiting states give the same Accepted opposite meanings: in waiting-payment it means the seller just paid, so skip; in waiting-buyer-invoice they have paid by definition and refunding them is the point. A lookup failure also skips — delaying a cancel is recoverable, refunding a live escrow is not. The scheduler lets the next tick re-evaluate; the handler rejects so the caller retries. Also hoists the status/kind resolution above the escrow cancel: an order whose status cannot be parsed should not lose its hold invoice first and be skipped second. --- src/app/cancel.rs | 82 +++++++++++++++++++++++++++++++++++++++++++++++ src/scheduler.rs | 55 +++++++++++++++++++++++-------- 2 files changed, 124 insertions(+), 13 deletions(-) diff --git a/src/app/cancel.rs b/src/app/cancel.rs index a69f3439..9c342669 100644 --- a/src/app/cancel.rs +++ b/src/app/cancel.rs @@ -57,6 +57,62 @@ impl CancelLightning for LndConnector { } } +/// 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) +} + /// Reset API-provided quote-derived amounts when republishing an order. /// /// When an order was created with `price_from_api`, its `amount` and `fee` @@ -711,6 +767,32 @@ async fn cancel_not_active_order( return Err(MostroInternalErr(ServiceError::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 order.sent_from_maker(event.sender).is_ok() { cancel_order_by_maker( pool, diff --git a/src/scheduler.rs b/src/scheduler.rs index 5256c691..4d5df007 100644 --- a/src/scheduler.rs +++ b/src/scheduler.rs @@ -1,4 +1,5 @@ use crate::app::bond; +use crate::app::cancel::{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 @@ -544,19 +586,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 From d528d5fffc4686016c76344a400878950e557f15 Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:37:00 -0600 Subject: [PATCH 03/10] test: cover the accepted-escrow cancel guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight tests over the two layers of the guard. The classifier: Accepted and Settled skip in waiting-payment, Open / Canceled / no-record proceed, a lookup failure skips, and waiting-buyer-invoice cancels a funded escrow anyway — the asymmetry the rule is keyed on. The handler, through a stub that records whether the hold invoice was canceled, so the assertion is about the escrow and not just the row: a maker cancel and a taker cancel racing the seller's payment are both rejected with the escrow untouched and the order left in waiting-payment, an unreadable escrow state is rejected as an internal error so the caller retries, and the buyer-side timeout still returns the seller's funds. Removing the handler guard fails three of the four handler tests. The fourth is the one that must keep passing: it pins the refund path the guard must not break. --- src/app/cancel.rs | 285 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 285 insertions(+) diff --git a/src/app/cancel.rs b/src/app/cancel.rs index 9c342669..bda4416a 100644 --- a/src/app/cancel.rs +++ b/src/app/cancel.rs @@ -922,6 +922,132 @@ mod tests { } } + /// 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, + canceled: std::sync::Arc, + } + + impl StubEscrowLnClient { + fn reporting(state: Option) -> Self { + Self { + state, + fail_lookup: false, + canceled: Default::default(), + } + } + + fn unreachable() -> Self { + Self { + state: None, + fail_lookup: true, + canceled: Default::default(), + } + } + + fn escrow_was_canceled(&self) -> bool { + self.canceled.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(); + Box::pin(async move { + 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); + Box::pin(async move { + 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] async fn cancel_action_with_ctx_rejects_non_creator_for_pending_order() { let pool = Arc::new(SqlitePool::connect("sqlite::memory:").await.unwrap()); @@ -1442,6 +1568,165 @@ 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() + ); + } + #[tokio::test] async fn cancel_not_active_order_rejects_intruder() { let pool = setup_pool().await; From 005112f3956dd6f59fa16922d8b447ab1ccc247c Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:44:43 -0600 Subject: [PATCH 04/10] fix(cancel): void the escrow before persisting a maker cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cancel_order_by_maker wrote the order as canceled and only then canceled the hold invoice, so a refused or failed cancel left a canceled order behind a live escrow — still payable at LND 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 voided the invoice near the CLTV horizon. Swap the two, so the `?` returns with the order untouched and the caller retries against a state that still matches the HTLC. This is the ordering the scheduler's timeout path and the taker branch already use, for the reason the scheduler already documents: never persist a state the HTLC does not back. --- src/app/cancel.rs | 76 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 71 insertions(+), 5 deletions(-) diff --git a/src/app/cancel.rs b/src/app/cancel.rs index bda4416a..2e1d8f28 100644 --- a/src/app/cancel.rs +++ b/src/app/cancel.rs @@ -425,6 +425,19 @@ async fn cancel_order_by_maker( request_id: Option, ln_client: &mut L, ) -> Result<(), MostroError> { + // 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?; + info!("Order Id {}: Funds returned to seller", &order.id); + } // 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 @@ -432,11 +445,6 @@ async fn cancel_order_by_maker( .await .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?; } - // Cancel hold invoice if present - if let Some(hash) = &order.hash { - ln_client.cancel_hold_invoice(hash).await?; - info!("Order Id {}: Funds returned to seller", &order.id); - } enqueue_order_msg( request_id, @@ -929,6 +937,7 @@ mod tests { /// `None` models an invoice LND has no record of. state: Option, fail_lookup: bool, + fail_cancel: bool, canceled: std::sync::Arc, } @@ -937,6 +946,7 @@ mod tests { Self { state, fail_lookup: false, + fail_cancel: false, canceled: Default::default(), } } @@ -945,6 +955,17 @@ mod tests { Self { state: None, fail_lookup: true, + fail_cancel: false, + canceled: Default::default(), + } + } + + /// Unpaid escrow whose cancel LND refuses. + fn refusing_cancel() -> Self { + Self { + state: Some(InvoiceState::Open), + fail_lookup: false, + fail_cancel: true, canceled: Default::default(), } } @@ -961,7 +982,13 @@ mod tests { ) -> std::pin::Pin> + Send + 'a>> { let canceled = self.canceled.clone(); + let fail = self.fail_cancel; Box::pin(async move { + if fail { + return Err(MostroInternalErr(ServiceError::LnNodeError( + "cancel refused".to_string(), + ))); + } canceled.store(true, std::sync::atomic::Ordering::SeqCst); Ok(()) }) @@ -1727,6 +1754,45 @@ mod tests { ); } + /// 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" + ); + } + #[tokio::test] async fn cancel_not_active_order_rejects_intruder() { let pool = setup_pool().await; From 76077e92e60bb38aa2f4a4f6184e13e1ddd6f95b Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:06:54 -0600 Subject: [PATCH 05/10] fix: notify hold-invoice parties only after the transition is stored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hold_invoice_paid enqueued BuyerTookOrder / HoldInvoicePaymentAccepted (or AddInvoice) and only then published and wrote the new status — with the write swallowed twice over: dropped a publish failure and dropped the write's. So a lost transition still told the buyer the escrow was funded, and the buyer's next move on that message is to send fiat. Build the messages, persist first, then send. A failed publish or write now logs, skips the notifications, and leaves invoice_held_at at 0 so the order stays eligible for the replay find_held_invoices drives on restart — stamping it would have marked the order processed with nobody told and the HTLC accepted. Order is persist -> notify -> stamp: a failed stamp can cost a duplicate AddInvoice on replay, which is the pre-existing risk, but a notification is never lost for a transition that did happen. This does not make the transition atomic — the guard read and the write are still separate statements (see #855). It removes the case where the buyer is told to send fiat against a write that never landed. --- src/flow.rs | 149 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 122 insertions(+), 27 deletions(-) diff --git a/src/flow.rs b/src/flow.rs index 5a275329..79498e34 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,37 +150,64 @@ 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; + } + // 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 *and* before marking the invoice processed. 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. Leaving `invoice_held_at` at 0 keeps the order 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; + } + if notify_reputation { // Notify taker reputation to maker 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; - } // Update the invoice_held_at field crate::db::update_order_invoice_held_at_time(pool, order.id, Timestamp::now().as_secs() as i64) @@ -366,6 +398,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(); From d04ba470ff59f3e49c079b3d8f2bfe5feced28c8 Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:31:37 -0600 Subject: [PATCH 06/10] fix(cancel): authorize the sender before consulting the node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The escrow guard ran ahead of the maker/taker routing, so a sender who is neither party still reached LND: one lookup RPC per message, and for a funded escrow an answer of NotAllowedByStatus instead of InvalidPubkey, which told a stranger the escrow was funded — the transient a seller aiming at this race wants to detect. Resolve the role first and route on it. The duplicate sent_from_maker check and its unreachable else branch go away with it; the guard still covers both branches. The intruder test now runs against a funded escrow, so it fails if the order is ever restored. --- src/app/cancel.rs | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/src/app/cancel.rs b/src/app/cancel.rs index 2e1d8f28..cb481a5b 100644 --- a/src/app/cancel.rs +++ b/src/app/cancel.rs @@ -775,6 +775,15 @@ async fn cancel_not_active_order( return Err(MostroInternalErr(ServiceError::InvalidPubkey)); }; + // 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 @@ -801,7 +810,7 @@ async fn cancel_not_active_order( } } - if order.sent_from_maker(event.sender).is_ok() { + if sender_is_maker { cancel_order_by_maker( pool, event, @@ -812,7 +821,7 @@ async fn cancel_not_active_order( ln_client, ) .await?; - } else if event.sender == taker_pubkey { + } else { cancel_order_by_taker( pool, event, @@ -823,8 +832,6 @@ async fn cancel_not_active_order( taker_pubkey, ) .await?; - } else { - return Err(MostroCantDo(CantDoReason::InvalidPubkey)); } Ok(()) } @@ -939,6 +946,7 @@ mod tests { fail_lookup: bool, fail_cancel: bool, canceled: std::sync::Arc, + looked_up: std::sync::Arc, } impl StubEscrowLnClient { @@ -948,6 +956,7 @@ mod tests { fail_lookup: false, fail_cancel: false, canceled: Default::default(), + looked_up: Default::default(), } } @@ -957,6 +966,7 @@ mod tests { fail_lookup: true, fail_cancel: false, canceled: Default::default(), + looked_up: Default::default(), } } @@ -967,12 +977,17 @@ mod tests { fail_lookup: false, fail_cancel: true, 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 { @@ -1005,7 +1020,9 @@ mod tests { >, > { 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(), @@ -1802,15 +1819,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; @@ -1818,6 +1837,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] From c3ab9a05693c7e07e9a4c06584b109614bec9cfb Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Fri, 21 Aug 2026 01:21:33 -0600 Subject: [PATCH 07/10] fix(cancel): surface a failed cancel event instead of skipping the write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cancel_order_by_maker guarded its persist with if let Ok(..), so a failure to build the cancel event skipped the DB write, kept going, and told both parties the order was canceled — with the escrow already void one line above. Propagate instead, matching the taker branch and hold_invoice_paid. Hygiene rather than a live hole: for a Canceled transition update_order_event only errors if the event cannot be built at all. Relay rejections, a failed send and a missing Nostr client are all caught inside and queued for republish, so they return Ok. Noted in the comment so the next reader does not mistake this branch for the relay path. --- src/app/cancel.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/app/cancel.rs b/src/app/cancel.rs index cb481a5b..1aa8c211 100644 --- a/src/app/cancel.rs +++ b/src/app/cancel.rs @@ -438,13 +438,18 @@ async fn cancel_order_by_maker( ln_client.cancel_hold_invoice(hash).await?; info!("Order Id {}: Funds returned to seller", &order.id); } - // 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())))?; - } + // 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, From eda9532644c649f0eb07da7e513dad8b1d9f02e0 Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Fri, 21 Aug 2026 01:27:16 -0600 Subject: [PATCH 08/10] fix(cancel): treat an already-void escrow as a cancel that succeeded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LND reports `already canceled` / `not found` as an error, and every cancel path aborted on it. That made a partial failure unrecoverable: the paths void the escrow before persisting, so an attempt that dies in between leaves the escrow gone and the order live — and the retry meant to finish the job hits the error again. In the handler it dies on `?`; in the scheduler it loops on `continue` every tick, forever. In waiting-buyer-invoice that stall is dangerous: the seller has already paid, so the buyer can still submit an invoice, reach Active and send fiat against an escrow that no longer exists, and hold_invoice_canceled will not flag it (is_escrow_backed covers only Active / FiatSent / Dispute). cancel_escrow_idempotent reuses classify_cancel_error, the classifier the bond module already applies to its own idempotent cancels. Anything it cannot place confidently stays an error, so an unreachable LND still aborts. Applied to all four cancel paths — maker, taker, cooperative and the scheduler timeout. The cooperative path was not in the report, but the reasoning is identical and leaving one path non-convergent is the asymmetry that bites later. With this the half-done state recovers on its own: the caller's retry completes it, and failing that the next timeout tick does. --- src/app/cancel.rs | 107 ++++++++++++++++++++++++++++++++++++++++------ src/scheduler.rs | 6 ++- 2 files changed, 98 insertions(+), 15 deletions(-) diff --git a/src/app/cancel.rs b/src/app/cancel.rs index 1aa8c211..d017a627 100644 --- a/src/app/cancel.rs +++ b/src/app/cancel.rs @@ -1,4 +1,5 @@ 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}; @@ -113,6 +114,37 @@ pub(crate) async fn decide_escrow_cancel( 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. /// /// When an order was created with `price_from_api`, its `amount` and `fee` @@ -176,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 @@ -367,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); } @@ -435,7 +467,7 @@ async fn cancel_order_by_maker( // 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. @@ -949,7 +981,7 @@ mod tests { /// `None` models an invoice LND has no record of. state: Option, fail_lookup: bool, - fail_cancel: bool, + fail_cancel: Option, canceled: std::sync::Arc, looked_up: std::sync::Arc, } @@ -959,7 +991,7 @@ mod tests { Self { state, fail_lookup: false, - fail_cancel: false, + fail_cancel: None, canceled: Default::default(), looked_up: Default::default(), } @@ -969,18 +1001,31 @@ mod tests { Self { state: None, fail_lookup: true, - fail_cancel: false, + fail_cancel: None, canceled: Default::default(), looked_up: Default::default(), } } - /// Unpaid escrow whose cancel LND refuses. + /// Unpaid escrow whose cancel LND refuses with a transient error. fn refusing_cancel() -> Self { Self { state: Some(InvoiceState::Open), fail_lookup: false, - fail_cancel: true, + 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(), } @@ -1002,12 +1047,10 @@ mod tests { ) -> std::pin::Pin> + Send + 'a>> { let canceled = self.canceled.clone(); - let fail = self.fail_cancel; + let fail = self.fail_cancel.clone(); Box::pin(async move { - if fail { - return Err(MostroInternalErr(ServiceError::LnNodeError( - "cancel refused".to_string(), - ))); + if let Some(cause) = fail { + return Err(MostroInternalErr(ServiceError::LnNodeError(cause))); } canceled.store(true, std::sync::atomic::Ordering::SeqCst); Ok(()) @@ -1815,6 +1858,44 @@ mod tests { ); } + /// 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; diff --git a/src/scheduler.rs b/src/scheduler.rs index 4d5df007..dbf4508f 100644 --- a/src/scheduler.rs +++ b/src/scheduler.rs @@ -1,5 +1,5 @@ use crate::app::bond; -use crate::app::cancel::{decide_escrow_cancel, EscrowCancelDecision}; +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}; @@ -557,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 From 4e9f2e307535f6b7b834cab588784a39f2739ea8 Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:18:47 -0600 Subject: [PATCH 09/10] fix: never let the reputation notice cost the idempotency stamp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hold_invoice_paid propagated notify_taker_reputation's error, which aborts before invoice_held_at is stamped — with the transition already persisted and AddInvoice / WaitingBuyerInvoice already queued. The order stayed replayable, so a resubscribe would enqueue both prompts again. Best effort instead: log and carry on to the stamp. The notice is an extra message riding alongside a prompt that already went out, and no failure of it is worth a duplicate. --- src/flow.rs | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/src/flow.rs b/src/flow.rs index 79498e34..4ecc1cd7 100644 --- a/src/flow.rs +++ b/src/flow.rs @@ -204,9 +204,21 @@ pub async fn hold_invoice_paid( } if notify_reputation { - // Notify taker reputation to maker + // Notify taker reputation to maker. Best effort on purpose: this is an + // extra message riding alongside the `AddInvoice` already queued above, + // and letting it abort would skip the `invoice_held_at` stamp below — + // leaving the order replayable with its transition already persisted + // and its messages already sent, so a resubscribe would duplicate + // them. `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 a duplicate prompt. tracing::info!("Notifying taker reputation to maker"); - notify_taker_reputation(pool, &order).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 @@ -517,6 +529,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(); From c08da62aa4db114d8d46b1c5a8db7751d4df7afc Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:51:52 -0600 Subject: [PATCH 10/10] fix: write the idempotency marker with the transition it belongs to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit invoice_held_at was a second statement after the status write, so a failed stamp left the transition persisted with the marker at 0 — and the replay find_held_invoices drives on restart would pass the guard and queue AddInvoice and WaitingBuyerInvoice a second time. Stamp the struct before the transition instead: Crud::update writes the whole row, so status, event id and marker land together and the intermediate state stops being representable. Nothing needs ordering between them — the marker means this delivery was processed, and the transition is what processing it produces. --- src/flow.rs | 86 +++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 71 insertions(+), 15 deletions(-) diff --git a/src/flow.rs b/src/flow.rs index 4ecc1cd7..29f27794 100644 --- a/src/flow.rs +++ b/src/flow.rs @@ -161,6 +161,16 @@ pub async fn hold_invoice_paid( 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 { @@ -177,10 +187,10 @@ pub async fn hold_invoice_paid( } }; - // Bail before notifying *and* before marking the invoice processed. 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. Leaving `invoice_held_at` at 0 keeps the order eligible + // 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 { @@ -206,12 +216,11 @@ pub async fn hold_invoice_paid( 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 letting it abort would skip the `invoice_held_at` stamp below — - // leaving the order replayable with its transition already persisted - // and its messages already sent, so a resubscribe would duplicate - // them. `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 a duplicate prompt. + // 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"); if let Err(e) = notify_taker_reputation(pool, &order).await { tracing::warn!( @@ -221,11 +230,6 @@ pub async fn hold_invoice_paid( } } - // 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(()) } @@ -655,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();