fix: recover from failed payout resolution, and bound outbound network latency

src/app/add_invoice.rs

@@ -1,7 +1,7 @@
 use crate::app::context::AppContext;
 use crate::util::{
     enqueue_order_msg, get_order, notify_taker_reputation, show_hold_invoice, update_order_event,
-    validate_invoice,
+    validate_invoice, InvoiceCheck,
 };
 use mostro_core::db::Crud;
 use mostro_core::prelude::*;
@@ -50,18 +50,29 @@ pub async fn add_invoice_action(
     if buyer_pubkey != event.sender {
         return Err(MostroCantDo(CantDoReason::InvalidPeer));
     }
+    // Reject on status before the payment request is looked at. The status
+    // check is a local comparison while validating the request reaches out to
+    // a host named by the sender, so an order that can't accept an invoice at
+    // all should never pay for that round-trip.
+    if !matches!(
+        ord_status,
+        Status::SettledHoldInvoice | Status::WaitingBuyerInvoice
+    ) {
+        return Err(MostroCantDo(CantDoReason::NotAllowedByStatus));
+    }
+
     // We save the invoice on db
-    order.buyer_invoice = validate_invoice(&msg, &order).await?;
+    //
+    // `Online` keeps the pre-existing behavior: the buyer is submitting the
+    // destination their payout will go to, so an unreachable one should be
+    // rejected now rather than at release. Bounded by
+    // `lnurl::LNURL_TOTAL_BUDGET`.
+    order.buyer_invoice = validate_invoice(&msg, &order, InvoiceCheck::Online).await?;
+
     // Buyer can add invoice orders with WaitingBuyerInvoice status
-    match ord_status {
-        Status::SettledHoldInvoice => {
-            pay_new_invoice(&mut order, pool, &msg).await?;
-            return Ok(());
-        }
-        Status::WaitingBuyerInvoice => {}
-        _ => {
-            return Err(MostroCantDo(CantDoReason::NotAllowedByStatus));
-        }
+    if ord_status == Status::SettledHoldInvoice {
+        pay_new_invoice(&mut order, pool, &msg).await?;
+        return Ok(());
     }
 
     // Notify taker reputation
@@ -247,6 +258,62 @@ mod tests {
         ));
     }
 
+    /// A disallowed status must be rejected without the payment request ever
+    /// being resolved. The status check is a local string comparison; the
+    /// resolution is a round-trip to a host the sender chose, and the handler
+    /// runs on the shared event loop, so the cheap check has to come first.
+    #[tokio::test]
+    async fn add_invoice_action_rejects_disallowed_status_before_resolving_address() {
+        use lnurl::lnurl::LnUrl;
+
+        let pool = setup_pool().await;
+        let ctx = build_ctx(pool.clone());
+        let seller = Keys::generate().public_key();
+        let buyer = Keys::generate().public_key();
+
+        let mut order = waiting_invoice_sell_order(seller, buyer);
+        order.status = Status::Active.to_string();
+        let order = order.create(ctx.pool()).await.unwrap();
+
+        // A host that completes the handshake and then never answers: any
+        // attempt to resolve this address would burn the full LNURL budget.
+        let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
+        let port = listener.local_addr().unwrap().port();
+        let listener_task = tokio::spawn(async move {
+            let mut held = Vec::new();
+            while let Ok((stream, _)) = listener.accept().await {
+                held.push(stream);
+            }
+        });
+        let lnurl = LnUrl {
+            url: format!("http://127.0.0.1:{port}/.well-known/lnurlp/blackhole"),
+        }
+        .encode();
+
+        let msg = Message::new_order(
+            Some(order.id),
+            Some(1),
+            None,
+            Action::AddInvoice,
+            Some(Payload::PaymentRequest(None, lnurl, None)),
+        );
+        let event = buyer_event(buyer);
+
+        let start = std::time::Instant::now();
+        let result = add_invoice_action(&ctx, msg, &event, &Keys::generate()).await;
+        let elapsed = start.elapsed();
+        listener_task.abort();
+
+        assert!(
+            matches!(result, Err(MostroCantDo(CantDoReason::NotAllowedByStatus))),
+            "a disallowed status must be rejected, got {result:?}"
+        );
+        assert!(
+            elapsed < std::time::Duration::from_secs(1),
+            "status rejection must not wait on the payment request, took {elapsed:?}"
+        );
+    }
+
     #[tokio::test]
     async fn add_invoice_action_rejects_disallowed_status() {
         let pool = setup_pool().await;

src/app/dev_fee.rs

@@ -912,22 +912,16 @@ pub async fn resolve_dev_fee_invoice(
     // fee payment came from.
     let comment = dev_fee_comment(&order.id, &keys.public_key());
 
-    let payment_request = tokio::time::timeout(
-        std::time::Duration::from_secs(15),
-        resolv_ln_address(
-            DEV_FEE_LIGHTNING_ADDRESS,
-            order.dev_fee as u64,
-            Some(comment.as_str()),
-        ),
+    // No timeout wrapper here: `resolv_ln_address` bounds itself with
+    // `lnurl::LNURL_TOTAL_BUDGET`, which covers both round-trips it makes.
+    // A second, longer deadline layered on top could never fire, and would
+    // only advertise a limit that isn't the real one.
+    let payment_request = resolv_ln_address(
+        DEV_FEE_LIGHTNING_ADDRESS,
+        order.dev_fee as u64,
+        Some(comment.as_str()),
     )
     .await
-    .map_err(|_| {
-        error!(
-            "Dev fee LNURL resolution timeout for order {} ({} sats)",
-            order.id, order.dev_fee
-        );
-        MostroInternalErr(ServiceError::LnAddressParseError)
-    })?
     .map_err(|e| {
         error!(
             "Dev fee LNURL resolution failed for order {} ({} sats): {:?}",

src/app/order.rs

@@ -1,6 +1,6 @@
 use crate::app::context::AppContext;
 use crate::db::update_user_trade_index;
-use crate::util::{get_bitcoin_price, publish_order, validate_invoice};
+use crate::util::{get_bitcoin_price, publish_order, validate_invoice, InvoiceCheck};
 use mostro_core::prelude::*;
 use nostr_sdk::prelude::*;
 use nostr_sdk::Keys;
@@ -92,9 +92,6 @@ pub async fn order_action(
     let request_id = msg.get_inner_message_kind().request_id;
 
     if let Some(order) = msg.get_inner_message_kind().get_order() {
-        // Validate invoice
-        let _invoice = validate_invoice(&msg, &Order::from(order.clone())).await?;
-
         // Check if fiat currency is accepted
         let mostro_settings = &ctx.settings().mostro;
         if let Err(cause) = order.check_fiat_currency(&mostro_settings.fiat_currencies_accepted) {
@@ -132,6 +129,31 @@ pub async fn order_action(
             calculate_and_check_quote(ctx, order, fiat_amount).await?;
         }
 
+        // Validate the payment request last: every check above is local and
+        // cheap, so a malformed order is rejected without touching it.
+        //
+        // `Offline` deliberately skips resolving a lightning address or LNURL
+        // over the network. Note this is not a throwaway value — the address
+        // is persisted as `orders.buyer_invoice` (see `prepare_new_order`) and
+        // is what `do_payment` pays out to. Skipping the reachability probe
+        // here is safe *because* a failure to resolve at payout time is now
+        // handled as a payment failure: the buyer is notified and the retry
+        // job picks the order up. Checking it here instead would put an HTTP
+        // request to a sender-chosen host on the event loop for every order
+        // created, delaying every other message behind it, in exchange for a
+        // liveness answer that can go stale before it is ever used.
+        //
+        // The returned value is discarded because it is the same string that
+        // gets persisted, not a different one. For a `NewOrder` the payload is
+        // `Payload::Order`, and `Message::get_payment_request` — what
+        // `validate_invoice` reads — resolves that to `ord.buyer_invoice`,
+        // which is the exact field `prepare_new_order` copies into the stored
+        // order. (A `NewOrder` carrying any other payload never reaches here:
+        // `get_order` returns `None` and this whole block is skipped.) So the
+        // invoice that is checked and the invoice that is saved are one value.
+        let _invoice =
+            validate_invoice(&msg, &Order::from(order.clone()), InvoiceCheck::Offline).await?;
+
         let trade_index = match msg.get_inner_message_kind().trade_index {
             Some(trade_index) => trade_index,
             None => {

src/app/release.rs

@@ -16,7 +16,36 @@ use sqlx::{Pool, Sqlite};
 use std::cmp::Ordering;
 use std::str::FromStr;
 use tokio::sync::mpsc::channel;
-use tracing::info;
+use tracing::{error, info};
+
+/// Record a failed payout attempt so the order stays recoverable.
+///
+/// Every path that gives up on paying the buyer after the hold invoice has
+/// been settled has to come through here. `find_failed_payment` selects orders
+/// on `failed_payment == true`, so an attempt that returns without setting it
+/// leaves the order settled, the buyer unpaid and unnotified, and invisible to
+/// the retry job — and a buyer sending a corrected invoice does not help,
+/// since that path only resets `payment_attempts` and relies on that same job.
+///
+/// The bookkeeping can itself fail: the `UPDATE` may error, or an order whose
+/// amount or kind no longer validates bails out on the retries-exhausted
+/// branch. That leaves exactly the unrecoverable state this function exists to
+/// prevent, so it is logged at `error` rather than dropped — both callers of
+/// [`do_payment`] discard its `Result`, which makes the log the only signal an
+/// operator ever gets.
+async fn record_payout_failure(ctx: &AppContext, order: &Order, request_id: Option<u64>) {
+    match check_failure_retries(ctx, order, request_id).await {
+        Ok(failed_payment) => info!(
+            "Order id {} has {} failed payments retries",
+            failed_payment.id, failed_payment.payment_attempts
+        ),
+        Err(e) => error!(
+            "Order id {}: could not record the payment failure ({:?}); the order may stay \
+             settled and unpaid until it is recovered manually",
+            order.id, e
+        ),
+    }
+}
 
 /// Check if order has failed payment retries
 pub async fn check_failure_retries(
@@ -496,24 +525,56 @@ pub async fn do_payment(
         return Err(MostroInternalErr(ServiceError::InvoiceInvalidError));
     }
     let payment_request = if let Ok(addr) = ln_addr {
-        resolv_ln_address(&addr.to_string(), amount, None)
-            .await
-            .map_err(|_| MostroInternalErr(ServiceError::LnAddressParseError))?
+        // Resolving a lightning address is a network round-trip to a host the
+        // buyer chose. When it yields no invoice — host unreachable, too slow,
+        // or answering without a `pr` — that is a payment failure like any
+        // other and has to go through the same bookkeeping.
+        //
+        // Returning early instead would leave `failed_payment = false`, and
+        // that flag is exactly the predicate `find_failed_payment` selects on
+        // (`db.rs`). The order would sit in `SettledHoldInvoice` with the
+        // seller's funds captured, the buyer unpaid and unnotified, and
+        // invisible to the retry job — and a buyer sending a corrected
+        // invoice wouldn't help either, since that path only resets
+        // `payment_attempts` and relies on the same job to act on it.
+        match resolv_ln_address(&addr.to_string(), amount, None).await {
+            Ok(pr) if !pr.is_empty() => pr,
+            outcome => {
+                match outcome {
+                    Err(e) => info!(
+                        "Order id {}: could not resolve payout address: {:?}",
+                        order.id, e
+                    ),
+                    _ => info!("Order id {}: payout address returned no invoice", order.id),
+                }
+                record_payout_failure(ctx, &order, request_id).await;
+                return Err(MostroInternalErr(ServiceError::LnAddressParseError));
+            }
+        }
     } else {
         payment_request
     };
-    let mut ln_client_payment = LndConnector::new().await?;
+    // Reaching LND is part of paying, so a handshake that fails or times out
+    // is a failed payout attempt — not a reason to return ahead of the
+    // bookkeeping. `LndConnector::new` now bounds that handshake, which makes
+    // this path reachable on a node whose LND is wedged rather than down.
+    let mut ln_client_payment = match LndConnector::new().await {
+        Ok(client) => client,
+        Err(e) => {
+            info!(
+                "Order id {}: could not connect to LND for payout: {:?}",
+                order.id, e
+            );
+            record_payout_failure(ctx, &order, request_id).await;
+            return Err(e);
+        }
+    };
     let (tx, mut rx) = channel(100);
 
     let payment_task = ln_client_payment.send_payment(&payment_request, amount as i64, tx);
     if let Err(paymement_result) = payment_task.await {
         info!("Error during ln payment : {}", paymement_result);
-        if let Ok(failed_payment) = check_failure_retries(ctx, &order, request_id).await {
-            info!(
-                "Order id {} has {} failed payments retries",
-                failed_payment.id, failed_payment.payment_attempts
-            );
-        }
+        record_payout_failure(ctx, &order, request_id).await;
     }
 
     // Get Mostro keys from context
@@ -1653,24 +1714,96 @@ mod tests {
         ));
     }
 
+    /// A payout address that yields no invoice must be recorded as a payment
+    /// failure, not returned as a bare error.
+    ///
+    /// The distinction is the whole point: `find_failed_payment` retries on
+    /// `failed_payment == true`, so an order that fails resolution without
+    /// setting that flag is settled, unpaid and invisible to the retry job —
+    /// and the buyer never hears about it.
+    #[tokio::test]
+    async fn do_payment_records_failure_when_payout_address_yields_no_invoice() {
+        // Arrange: an order settled and awaiting payout, whose buyer invoice
+        // is a lightning address that cannot produce an invoice. Under
+        // cfg(test) the address resolves to a local well-known path that
+        // isn't served, so this never leaves the machine.
+        init_global_config();
+        let pool = create_test_pool().await;
+        let ctx = build_ctx(&pool);
+        let seller = Keys::generate().public_key();
+        let buyer = Keys::generate().public_key();
+        let mut order = fiat_sent_sell_order(seller, buyer);
+        order.status = Status::SettledHoldInvoice.to_string();
+        order.buyer_invoice = Some("nosuchpayee@localhost".to_string());
+        order.amount = 1_000;
+        order.fee = 10;
+        order.payment_attempts = 0;
+        order.failed_payment = false;
+        let order = order.create(&pool).await.unwrap();
+
+        // Act
+        let result = do_payment(&ctx, order.clone(), None).await;
+
+        // Assert: the call still fails, but it left the bookkeeping behind
+        // that makes the failure recoverable.
+        assert!(result.is_err(), "unresolvable payout address must fail");
+        let (failed_payment, payment_attempts): (bool, i64) =
+            sqlx::query_as("SELECT failed_payment, payment_attempts FROM orders WHERE id = ?")
+                .bind(order.id)
+                .fetch_one(&pool)
+                .await
+                .unwrap();
+        assert!(
+            failed_payment,
+            "failed_payment must be set so find_failed_payment can retry the order"
+        );
+        assert_eq!(
+            payment_attempts, 1,
+            "the failed attempt must be counted, not silently dropped"
+        );
+    }
+
+    /// Failing to reach LND is a failed payout attempt like any other, so it
+    /// has to leave the same bookkeeping behind. Without it a settled order
+    /// whose LND handshake fails would never be retried, since
+    /// `find_failed_payment` only selects on `failed_payment == true`.
     #[tokio::test]
     async fn do_payment_fails_fast_when_lnd_is_unreachable() {
         // Arrange: with the global config set to test defaults, the LND cert
         // path is invalid, so LndConnector::new() returns an error without
-        // any network access.
+        // any network access. The buyer invoice is not a lightning address,
+        // so address resolution is skipped and LND is the first thing tried.
         init_global_config();
         let pool = create_test_pool().await;
         let ctx = build_ctx(&pool);
         let seller = Keys::generate().public_key();
         let buyer = Keys::generate().public_key();
         let mut order = fiat_sent_sell_order(seller, buyer);
+        order.status = Status::SettledHoldInvoice.to_string();
         order.buyer_invoice = Some("lnbc1notchecked".to_string());
+        order.payment_attempts = 0;
+        order.failed_payment = false;
+        let order = order.create(&pool).await.unwrap();
 
         // Act
-        let result = do_payment(&ctx, order, None).await;
+        let result = do_payment(&ctx, order.clone(), None).await;
 
         // Assert
         assert!(result.is_err());
+        let (failed_payment, payment_attempts): (bool, i64) =
+            sqlx::query_as("SELECT failed_payment, payment_attempts FROM orders WHERE id = ?")
+                .bind(order.id)
+                .fetch_one(&pool)
+                .await
+                .unwrap();
+        assert!(
+            failed_payment,
+            "an unreachable LND must still mark the payout as failed so it can be retried"
+        );
+        assert_eq!(
+            payment_attempts, 1,
+            "the failed attempt must be counted, not silently dropped"
+        );
     }
 
     #[tokio::test]