fix: bound outbound network latency in message handling - #843
Conversation
Message handlers run sequentially on a single event-loop task, so any outbound request they make delays every other user's messages for as long as it takes. Several of those requests had no effective upper bound, which made overall responsiveness depend on hosts the node doesn't control. Three changes, smallest blast radius first: 1. `order_action` now runs its local checks (fiat currency, amounts, range limits, quote) before touching the payment request, so a malformed order costs nothing. 2. Order creation validates the payment request without network I/O. It only records the request — the validation result was already discarded — and a lightning address still has to resolve at payout time, which is where an unreachable host is actually actionable. Takes and invoice submission keep resolving over the network, where reachability is part of the decision being made. 3. LNURL operations get a total wall-clock budget covering every round-trip, a connect timeout, and a finite redirect chain. The per-request timeout alone was not enough: `resolv_ln_address` makes two sequential requests where the second host comes from the first response. `LndConnector::new` gets a connect timeout too; the RPCs after it are deliberately left alone, since a timeout on `settle_invoice` would report failure for work LND may have done. Adds regression coverage that an unresponsive host cannot hold validation open past the budget, that offline validation does no network I/O, and that both check modes agree on every BOLT11 case.
WalkthroughInvoice validation now has explicit offline and online modes. Order creation uses offline checks after local validation, while payout-related actions use online checks. LNURL requests and LND connection setup now have bounded timeouts with expanded tests. ChangesInvoice validation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OrderCreation
participant PayoutAction
participant validate_invoice
participant LNURL
OrderCreation->>validate_invoice: Offline validation after local checks
PayoutAction->>validate_invoice: Online validation
validate_invoice->>LNURL: Resolve Lightning Address or LNURL
LNURL-->>validate_invoice: Validation result or bounded timeout
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7232f16e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let _invoice = | ||
| validate_invoice(&msg, &Order::from(order.clone()), InvoiceCheck::Offline).await?; |
There was a problem hiding this comment.
When a buy-order maker supplies a syntactically valid Lightning address whose endpoint is unreachable, Offline now publishes the order although the previous validation rejected it. After a seller completes the trade, release_action settles the seller's hold invoice before calling do_payment; address resolution can then return early with an error that is ignored, without invoking check_failure_retries or requesting a replacement invoice. This leaves the order in SettledHoldInvoice with the buyer unpaid and no scheduled retry, so retain online validation for buy-order maker invoices or route resolution failures through the payment-failure recovery flow.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/util.rs (1)
1451-1481: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
///doc comment now that the signature changed.
validate_invoicegained acheck: InvoiceCheckparameter that materially changes behavior (network vs. no network), but the function itself still has no doc comment — only inline//comments. A short///summary pointing callers atInvoiceCheck's variants would help prevent a future caller from picking the wrong mode.As per coding guidelines,
src/**/*.rs: "Document non-obvious public APIs with///documentation comments."📝 Suggested doc comment
+/// Validates the message's payment request, if present, against `order`. +/// +/// See [`InvoiceCheck`] for what `Offline` vs `Online` mode changes. pub async fn validate_invoice( msg: &Message, order: &Order, check: InvoiceCheck, ) -> Result<Option<String>, MostroError> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util.rs` around lines 1451 - 1481, Add a concise /// documentation comment immediately above validate_invoice describing its validation behavior and directing callers to InvoiceCheck variants for choosing offline versus online validation. Keep the existing implementation and inline comments unchanged.Source: Coding guidelines
src/lnurl.rs (1)
196-219: 🩺 Stability & Availability | 🔵 TrivialTotal budget (5s) is intentionally tighter than two sequential per-request caps (4s each).
resolv_ln_address_innerissues two sequential requests, each individually allowed up toLNURL_REQUEST_TIMEOUT(4s) byHTTP_CLIENT, but the outerLNURL_TOTAL_BUDGETcaps the whole operation at 5s. This is explicitly documented as intentional, but it means a legitimate provider whose two hops together take, say, 3s + 3s will be reported as unreachable even though it would eventually have answered. Worth watching in production — if false "unreachable" rejections ontake_sell/add_invoice(Online mode) turn out to be common for real LNURL providers, the budget may need widening.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lnurl.rs` around lines 196 - 219, The LNURL total timeout intentionally caps both sequential requests at 5 seconds despite each request allowing 4 seconds; preserve this behavior and the existing timeout handling in resolv_ln_address and resolv_ln_address_inner. No code change is required unless production monitoring later shows frequent false unreachable errors, in which case revisit LNURL_TOTAL_BUDGET.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lnurl.rs`:
- Around line 196-219: The LNURL total timeout intentionally caps both
sequential requests at 5 seconds despite each request allowing 4 seconds;
preserve this behavior and the existing timeout handling in resolv_ln_address
and resolv_ln_address_inner. No code change is required unless production
monitoring later shows frequent false unreachable errors, in which case revisit
LNURL_TOTAL_BUDGET.
In `@src/util.rs`:
- Around line 1451-1481: Add a concise /// documentation comment immediately
above validate_invoice describing its validation behavior and directing callers
to InvoiceCheck variants for choosing offline versus online validation. Keep the
existing implementation and inline comments unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c5a04609-09cb-42dd-b86f-b626ec889a54
📒 Files selected for processing (7)
src/app/add_invoice.rssrc/app/order.rssrc/app/take_sell.rssrc/lightning/invoice.rssrc/lightning/mod.rssrc/lnurl.rssrc/util.rs
|
Superseded by #845, which reworks this branch after review. The change to skip network validation at order creation rested on a wrong assumption: the payment request supplied there is not a throwaway value — it is persisted as |
1 participant
What
Bounds how long a single message handler can wait on an outbound network call, and stops doing one of those calls where it isn't needed yet.
Message handlers run sequentially on one event-loop task (
app::run), so any request a handler makes delays every other message behind it for the full duration. A few of those requests had no effective upper bound, which left overall responsiveness dependent on third-party hosts the node doesn't control — a slow or unreachable LNURL endpoint, or an LND that isn't answering, would hold up unrelated traffic.Changes
1. Cheap checks first in
order_actionFiat currency, fiat amount, sats amount, range limits and the quote lookup are all local. They now run before the payment request is looked at, so a malformed order is rejected without any further work.
2. Order creation validates the payment request without network I/O
validate_invoicetakes a newInvoiceCheckargument:Offline— syntax plus the full local BOLT11 rules, no HTTP.Online— additionally resolves lightning addresses and LNURLs, as before.Order creation uses
Offline. It only records the payment request (the validation result was already discarded), and a lightning address still has to resolve when the payout is actually made — which is where an unreachable host is worth acting on.take_sellandadd_invoicekeep usingOnline: there the buyer is committing to a payout destination, so reachability is genuinely part of the decision. No change in behavior for those two paths.3. Real bounds on the remaining calls
LNURL_TOTAL_BUDGET, 5s) covering every round-trip in one operation, a 2s connect timeout, per-request timeout 10s → 4s, and a finite redirect chain (3). The per-request timeout alone wasn't sufficient, sinceresolv_ln_addressmakes two sequential requests where the second host comes from the first response.LndConnector::new: a 10s connect timeout, where previously there was none. The RPCs after it are deliberately left unbounded — a timeout onsettle_invoicewould report failure for an operation LND may well have completed, which is a worse failure mode than waiting.Test plan
cargo test— 1051 passed, 0 failedcargo clippy --all-targets -- -D warnings— cleancargo fmt --allNew coverage:
lnurl_validation_against_unresponsive_host_is_time_bounded— a host that completes the TCP handshake and then never answers cannot hold validation open past the budget. Uses an ephemeral port via LNURL rather than the fixed 8080 thatstart_test_serverowns, so it doesn't race the existing tests.offline_validation_of_lnurl_does_no_network_io— the offline path returns immediately against that same unresponsive host.offline_validation_still_rejects_malformed_payment_request/..._still_enforces_bolt11_amount_checks— dropping the round-trip doesn't drop any local check.validate_invoice_pathsnow runs its whole table under both modes, pinning down that they agree on every BOLT11 case.Manual check: create an order with a lightning address on a host that is slow to respond, and confirm the order is created without the daemon pausing; then take a sell and confirm an unreachable address is still rejected at that point.
Follow-ups (not in this PR)
SpamGate— it currently has no counter or token bucket, and the existing gate only applies to thenip44transport whilegift-wrapis still the default.show_hold_invoiceopens a freshSqlitePoolper take (util.rs) instead of reusingctx.pool().tokio::spawnis not the answer: the current serialization is what keeps the order state machine free of races, so that needs per-order locking designed alongside it.Summary by CodeRabbit