fix: bound outbound network latency in message handling

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::*;
@@ -51,7 +51,12 @@ pub async fn add_invoice_action(
         return Err(MostroCantDo(CantDoReason::InvalidPeer));
     }
     // 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 => {

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,19 @@ 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. Creating an order only records the payment
+        // request — the result here is discarded — and the address still has
+        // to resolve when the payout is actually made, which is where an
+        // unreachable host is worth acting on. Checking it here would put an
+        // HTTP request to a sender-chosen host on the event loop for every
+        // order created, delaying every other message behind it.
+        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/take_sell.rs

@@ -5,6 +5,7 @@ use crate::db::{buyer_has_pending_order, update_user_trade_index};
 use crate::util::{
     enqueue_order_msg, get_dev_fee, get_fiat_amount_requested, get_market_amount_and_fee,
     get_order, set_waiting_invoice_status, show_hold_invoice, update_order_event, validate_invoice,
+    InvoiceCheck,
 };
 use mostro_core::db::Crud;
 use mostro_core::prelude::*;
@@ -171,7 +172,12 @@ pub async fn take_sell_action(
 
     // Validate invoice and get payment request if present
     // NOW dev_fee is set correctly for proper validation
-    let payment_request = validate_invoice(&msg, &order).await?;
+    //
+    // `Online` keeps the pre-existing behavior: this is the buyer committing
+    // to a payout destination, so an unreachable one should block the take
+    // rather than surface at release time. The round-trip is bounded by
+    // `lnurl::LNURL_TOTAL_BUDGET`.
+    let payment_request = validate_invoice(&msg, &order, InvoiceCheck::Online).await?;
 
     let trade_index = match msg.get_inner_message_kind().trade_index {
         Some(trade_index) => trade_index,

src/lightning/invoice.rs

@@ -194,6 +194,43 @@ pub async fn is_valid_invoice(
     validate_bolt11_invoice(&payment_request, amount, fee).await
 }
 
+/// Validates a payment request without performing any network I/O.
+///
+/// Identical to [`is_valid_invoice`] for BOLT11 invoices — every BOLT11 check
+/// is local, so none of them are skipped. The difference is lightning
+/// addresses and LNURLs: those are accepted on syntax alone, with no
+/// round-trip to the host they name.
+///
+/// # Why a separate entry point
+///
+/// Resolving a lightning address means an HTTP request to a host the sender
+/// picked, and callers on the event loop pay that latency before anything
+/// else can be processed. Where reachability isn't yet needed, checking it
+/// early buys nothing: the address still has to resolve at payout time, and
+/// that's where an unreachable host is actually actionable. Order creation
+/// is the clear case — it stores the address and discards the validation
+/// result — so it uses this variant and lets the network check happen when
+/// the address is really used.
+///
+/// Use [`is_valid_invoice`] where reachability is part of the decision being
+/// made, such as accepting a buyer's payout destination on a take.
+pub async fn is_valid_invoice_offline(
+    payment_request: String,
+    amount: Option<u64>,
+    fee: Option<u64>,
+) -> Result<(), MostroError> {
+    // A parseable lightning address or LNURL is all this variant checks:
+    // both constructors fully validate the syntax they accept.
+    if LightningAddress::from_str(&payment_request).is_ok()
+        || LnUrl::from_str(&payment_request).is_ok()
+    {
+        return Ok(());
+    }
+
+    // Anything else must still clear full BOLT11 validation, which is local.
+    validate_bolt11_invoice(&payment_request, amount, fee).await
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -417,6 +454,108 @@ mod tests {
         );
     }
 
+    /// Bind a listener that completes the TCP handshake and then never
+    /// answers, on an OS-assigned port, and return an LNURL pointing at it.
+    ///
+    /// A lightning address can't be used here: under `cfg(test)`
+    /// `extract_lnurl` pins those to port 8080, which `start_test_server`
+    /// already owns. An LNURL carries its own URL, so it can target an
+    /// ephemeral port and stay independent of the other tests.
+    async fn start_blackhole_server() -> (String, tokio::task::JoinHandle<()>) {
+        let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind");
+        let port = listener.local_addr().expect("local addr").port();
+        let handle = tokio::spawn(async move {
+            // Hold every accepted socket open without writing a response.
+            let mut held = Vec::new();
+            while let Ok((stream, _)) = listener.accept().await {
+                held.push(stream);
+            }
+        });
+        let url = format!("http://127.0.0.1:{port}/.well-known/lnurlp/blackhole");
+        (LnUrl { url }.encode(), handle)
+    }
+
+    /// A host that accepts the connection and then stalls must not be able to
+    /// hold invoice validation open for longer than the LNURL budget. The
+    /// connect timeout is deliberately not exercised: the handshake succeeds,
+    /// so only the total budget can end this.
+    #[tokio::test]
+    async fn lnurl_validation_against_unresponsive_host_is_time_bounded() {
+        init_settings_test();
+        let (lnurl, handle) = start_blackhole_server().await;
+
+        let start = std::time::Instant::now();
+        let result = is_valid_invoice(lnurl, None, None).await;
+        let elapsed = start.elapsed();
+        handle.abort();
+
+        assert!(result.is_err(), "an unresponsive host must not validate");
+        assert!(
+            elapsed < crate::lnurl::LNURL_TOTAL_BUDGET + std::time::Duration::from_secs(2),
+            "validation took {elapsed:?}, which exceeds the LNURL budget of {:?}",
+            crate::lnurl::LNURL_TOTAL_BUDGET
+        );
+    }
+
+    /// The offline variant is what order creation uses: a syntactically valid
+    /// LNURL passes without any HTTP round-trip, so an unresponsive host costs
+    /// nothing at all.
+    #[tokio::test]
+    async fn offline_validation_of_lnurl_does_no_network_io() {
+        init_settings_test();
+        let (lnurl, handle) = start_blackhole_server().await;
+
+        let start = std::time::Instant::now();
+        let result = is_valid_invoice_offline(lnurl, None, None).await;
+        let elapsed = start.elapsed();
+        handle.abort();
+
+        assert!(
+            result.is_ok(),
+            "a syntactically valid LNURL must pass offline validation: {result:?}"
+        );
+        assert!(
+            elapsed < std::time::Duration::from_secs(1),
+            "offline validation must not contact the host, but took {elapsed:?}"
+        );
+    }
+
+    #[tokio::test]
+    async fn offline_validation_accepts_well_formed_lightning_address() {
+        init_settings_test();
+        assert!(
+            is_valid_invoice_offline("MostroP2P@example.com".to_string(), None, None)
+                .await
+                .is_ok(),
+            "a well-formed lightning address must pass offline validation"
+        );
+    }
+
+    /// Dropping the network round-trip must not drop the local checks: a
+    /// payment request that is neither an address nor an LNURL still has to
+    /// clear full BOLT11 validation.
+    #[tokio::test]
+    async fn offline_validation_still_rejects_malformed_payment_request() {
+        init_settings_test();
+        assert!(
+            is_valid_invoice_offline("not-a-payment-request".to_string(), None, None)
+                .await
+                .is_err(),
+            "garbage must still be rejected without network access"
+        );
+    }
+
+    #[tokio::test]
+    async fn offline_validation_still_enforces_bolt11_amount_checks() {
+        init_settings_test();
+        let payment_request = build_test_invoice(Some(1_000_000), 86_400);
+        assert_eq!(
+            is_valid_invoice_offline(payment_request, Some(5_000), None).await,
+            Err(MostroInternalErr(ServiceError::InvoiceInvalidError)),
+            "BOLT11 amount mismatch must still be caught offline"
+        );
+    }
+
     #[tokio::test]
     async fn test_lnurl_validation_with_test_server() {
         init_settings_test();

src/lightning/mod.rs

@@ -15,7 +15,9 @@ use mostro_core::prelude::*;
 use nostr_sdk::nostr::hashes::hex::FromHex;
 use nostr_sdk::nostr::secp256k1::rand::{self, RngCore};
 use std::cmp::Ordering;
+use std::time::Duration;
 use tokio::sync::mpsc::Sender;
+use tokio::time::timeout;
 use tracing::info;
 
 #[derive(Clone)]
@@ -90,17 +92,39 @@ fn decode_hash32(field: &str, value: &str) -> Result<Vec<u8>, MostroError> {
     Ok(bytes)
 }
 
+/// Cap on establishing a gRPC connection to LND.
+///
+/// `fedimint_tonic_lnd::connect` has no deadline of its own, and several
+/// handlers open a fresh connection while running on the single event-loop
+/// task in `app::run` — so an unreachable or wedged LND would otherwise stall
+/// message processing for everyone, with no upper bound.
+///
+/// Bounding the *connection* is safe in a way that bounding the RPCs after it
+/// is not: if this elapses, no request was issued and no invoice state was
+/// touched, so failing is unambiguous. The RPCs themselves are deliberately
+/// left unbounded here — a timeout on `settle_invoice` would report failure
+/// for an operation LND may still have completed.
+const LND_CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
+
 impl LndConnector {
     pub async fn new() -> Result<Self, MostroError> {
         let ln_settings = Settings::get_ln();
 
         // Connecting to LND requires only host, port, cert file, and macaroon file
-        let client = fedimint_tonic_lnd::connect(
-            ln_settings.lnd_grpc_host.clone(),
-            ln_settings.lnd_cert_file.clone(),
-            ln_settings.lnd_macaroon_file.clone(),
+        let client = timeout(
+            LND_CONNECT_TIMEOUT,
+            fedimint_tonic_lnd::connect(
+                ln_settings.lnd_grpc_host.clone(),
+                ln_settings.lnd_cert_file.clone(),
+                ln_settings.lnd_macaroon_file.clone(),
+            ),
         )
         .await
+        .map_err(|_| {
+            MostroInternalErr(ServiceError::LnNodeError(format!(
+                "timed out connecting to LND after {LND_CONNECT_TIMEOUT:?}"
+            )))
+        })?
         .map_err(|e| MostroInternalErr(ServiceError::LnNodeError(e.to_string())))?;
 
         // Safe unwrap here

src/lnurl.rs

@@ -3,11 +3,39 @@ use mostro_core::prelude::*;
 use once_cell::sync::Lazy;
 use reqwest::Client;
 use serde_json::Value;
+use std::time::Duration;
+use tokio::time::timeout;
 use tracing::{error, warn};
 
+/// Cap on a single HTTP round-trip to an LNURL endpoint.
+const LNURL_REQUEST_TIMEOUT: Duration = Duration::from_secs(4);
+
+/// Cap on establishing the TCP/TLS connection. Separate from the request
+/// timeout so an unroutable or filtered host fails fast instead of spending
+/// the whole request budget on a handshake that will never complete.
+const LNURL_CONNECT_TIMEOUT: Duration = Duration::from_secs(2);
+
+/// Redirect budget. Refusing redirects outright would break real endpoints
+/// (`www` to apex, provider migrations), but the chain has to be finite.
+const LNURL_MAX_REDIRECTS: usize = 3;
+
+/// Total wall-clock budget for one LNURL operation, covering every HTTP
+/// round-trip it makes.
+///
+/// [`HTTP_CLIENT`]'s own timeout is *per request*, and [`resolv_ln_address`]
+/// issues two sequential requests where the second host is chosen by the
+/// first response — so a per-request cap alone lets a remote endpoint cost
+/// twice that. This constant is the one that actually bounds the operation.
+///
+/// It matters because the callers run on the single event-loop task in
+/// `app::run`: this is how long one message can hold up everyone else's.
+pub const LNURL_TOTAL_BUDGET: Duration = Duration::from_secs(5);
+
 pub static HTTP_CLIENT: Lazy<Client> = Lazy::new(|| {
     Client::builder()
-        .timeout(std::time::Duration::from_secs(10))
+        .timeout(LNURL_REQUEST_TIMEOUT)
+        .connect_timeout(LNURL_CONNECT_TIMEOUT)
+        .redirect(reqwest::redirect::Policy::limited(LNURL_MAX_REDIRECTS))
         .user_agent(concat!("mostro/", env!("CARGO_PKG_VERSION")))
         .build()
         .expect("valid reqwest Client")
@@ -53,7 +81,22 @@ async fn extract_lnurl(address: &str) -> Result<String, MostroError> {
     Ok(url)
 }
 
+/// Check that `address` resolves to a live LNURL-pay endpoint.
+///
+/// Bounded by [`LNURL_TOTAL_BUDGET`]: the address is remote input and the
+/// host it names is under no obligation to answer, so a caller on the event
+/// loop must not be able to wait on it indefinitely. A host that exhausts
+/// the budget is reported as unreachable, which is what it is from here.
 pub async fn ln_exists(address: &str) -> Result<(), MostroError> {
+    timeout(LNURL_TOTAL_BUDGET, ln_exists_inner(address))
+        .await
+        .map_err(|_| {
+            warn!("LNURL existence check exceeded the {LNURL_TOTAL_BUDGET:?} budget");
+            MostroInternalErr(ServiceError::NoAPIResponse)
+        })?
+}
+
+async fn ln_exists_inner(address: &str) -> Result<(), MostroError> {
     // Get the url from the str - could be a LNURL or a Lightning Address
     let url = extract_lnurl(address).await?;
     // Make the request to the LNURL
@@ -150,10 +193,30 @@ fn build_callback_url(
 ///   server rejected the request or doesn't support `payRequest`
 /// * `Err(MostroError)` - If the address/LNURL can't be resolved or the HTTP
 ///   exchange fails
+///
+/// Bounded by [`LNURL_TOTAL_BUDGET`] across *both* round-trips it makes: the
+/// callback host comes from the first response, so without a total budget a
+/// remote server could spend the per-request timeout twice.
 pub async fn resolv_ln_address(
     address: &str,
     amount: u64,
     comment: Option<&str>,
+) -> Result<String, MostroError> {
+    timeout(
+        LNURL_TOTAL_BUDGET,
+        resolv_ln_address_inner(address, amount, comment),
+    )
+    .await
+    .map_err(|_| {
+        warn!("LNURL resolution exceeded the {LNURL_TOTAL_BUDGET:?} budget");
+        MostroInternalErr(ServiceError::NoAPIResponse)
+    })?
+}
+
+async fn resolv_ln_address_inner(
+    address: &str,
+    amount: u64,
+    comment: Option<&str>,
 ) -> Result<String, MostroError> {
     // Get the url from the str - could be a LNURL or a Lightning Address
     let url = extract_lnurl(address).await?;