Always-online node forward invoice request #4049

elnosh · 2025-09-02T20:57:01Z

Part of #2298

As an always online node if we receive a invoice request on behalf of an async recipient, add support for forwarding the request to the recipient in case it is online to respond to the request.

ldk-reviews-bot · 2025-09-02T20:57:03Z

👋 I see @tankyleo was un-assigned.
If you'd like another reviewer assignment, please click here.

codecov · 2025-09-02T21:10:54Z

Codecov Report

❌ Patch coverage is 84.28928% with 63 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.35%. Comparing base (bf87832) to head (f46a3ff).
⚠️ Report is 94 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/async_payments_tests.rs	84.80%	50 Missing and 7 partials ⚠️
lightning/src/offers/invoice_request.rs	0.00%	3 Missing ⚠️
lightning/src/ln/channelmanager.rs	80.00%	2 Missing ⚠️
lightning/src/offers/flow.rs	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4049      +/-   ##
==========================================
- Coverage   88.76%   88.35%   -0.41%     
==========================================
  Files         176      177       +1     
  Lines      129345   131816    +2471     
  Branches   129345   131816    +2471     
==========================================
+ Hits       114812   116465    +1653     
- Misses      11925    12692     +767     
- Partials     2608     2659      +51

Flag	Coverage Δ
fuzzing	`21.62% <0.00%> (-0.39%)`	⬇️
tests	`88.19% <84.28%> (-0.41%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

valentinewallace · 2025-09-02T21:17:34Z

I mentioned to @elnosh since I've been giving input on this feature, but he moved quick and I probably won't be able to get to review this til possibly next week. @tankyleo up to you if you want to get involved in async payments review but I understand if you unassign lol.

ldk-reviews-bot · 2025-09-04T21:07:44Z

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-09-08T00:00:30Z

🔔 2nd Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo

Took a first pass, just have a conceptual question so far, thank you for the tests.

tankyleo · 2025-09-08T19:49:22Z

lightning/src/ln/channelmanager.rs

+	/// When handling an [`Event::StaticInvoiceRequested`], this should be called to forward the
+	/// [`InvoiceRequest`] over the `invoice_request_path` to the async recipient if it is online
+	/// and it will forward the [`StaticInvoice`] to the responder.
+	pub fn send_response_static_invoice_request(
+		&self, invoice: StaticInvoice, responder: Responder, invoice_request: InvoiceRequest,
+		invoice_request_path: BlindedMessagePath,
 	) -> Result<(), Bolt12SemanticError> {
+		self.flow.enqueue_invoice_request_to_forward(
+			invoice_request,
+			invoice_request_path,
+			responder.clone(),
+		);
 		self.flow.enqueue_static_invoice(invoice, responder)
 	}


Is the goal here to allow an async-recipient to "override" the invoice that would have been provided by the server since the recipient is online ? If so, should the static invoice server add a small delay before serving the invoice back to the sender to give the async-recipient a chance to respond to the sender first ?

Is the goal here to allow an async-recipient to "override" the invoice that would have been provided by the server since the recipient is online ?

yes!

should the static invoice server add a small delay before serving the invoice back to the sender to give the async-recipient a chance to respond to the sender first ?

yeah I thought about something like that to allow the receiver to respond. I got from @valentinewallace that for this first iteration to forward both of them immediately and perhaps in a future PR on the sender side detect if it received a fresh invoice from the async recipient and pay that one instead. Will defer to her :)

Did you consider splitting the forwarding part and the response part into two methods on ChannelManager ? Like this the user could first call the "forward static invoice request" method, wait some chosen amount of time, then call "respond with persisted static invoice".

Didn't consider separating like that. Thanks, I can try that 👍

I kinda feel like a better UX is more important, which I feel like a single method would be? Also this makes forwarding the invreq optional, whereas before it was required. We could handle the wait in LDK in std builds, theoretically, but in practice I don't feel like we'll ever want to increase potential payment latency for any reason. The sender can also wait a second for a fresh invoice if they choose.

Sounds good we can go with the single-method approach then @elnosh

ldk-reviews-bot · 2025-09-08T19:50:10Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

tankyleo · 2025-09-09T16:30:49Z

@elnosh Is this good to take a look ? Feel free to hit the "request review" button top right.

elnosh · 2025-09-09T16:39:16Z

@elnosh Is this good to take a look ? Feel free to hit the "request review" button top right.

ah, yes. Added a fixup breaking up the forwarding of the request as suggested.

tankyleo

This looks better to me thank you ! A couple more comments

tankyleo · 2025-09-09T19:10:46Z

lightning/src/offers/invoice_request.rs

+impl PartialEq for InvoiceRequest {
+	fn eq(&self, other: &Self) -> bool {
+		self.bytes.eq(&other.bytes)
+	}
+}
+
+impl Eq for InvoiceRequest {}
+


Here I would have kept the equality checks on all members for tests to make sure the deserialization from bytes works properly:

diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs index 8956fc6ca..d5d3c4d75 100644 --- a/lightning/src/offers/invoice_request.rs +++ b/lightning/src/offers/invoice_request.rs @@ -581,15 +581,17 @@ impl AsRef<TaggedHash> for UnsignedInvoiceRequest { /// [`Bolt12Invoice`]: crate::offers::invoice::Bolt12Invoice /// [`Offer`]: crate::offers::offer::Offer #[derive(Clone, Debug)] +#[cfg_attr(test, derive(PartialEq))] pub struct InvoiceRequest { pub(super) bytes: Vec<u8>, pub(super) contents: InvoiceRequestContents, signature: Signature, } +#[cfg(not(test))] impl PartialEq for InvoiceRequest { fn eq(&self, other: &Self) -> bool { - self.bytes.eq(&other.bytes) + self.bytes.eq(&other.bytes) && self.signature.eq(&other.signature) } }

tankyleo · 2025-09-09T19:12:15Z

lightning/src/offers/flow.rs

+		/// The invoice request that should be forwarded to the async recipient in case it is
+		/// online to respond.
+		invoice_request: InvoiceRequest,


Lets point people to OffersMessageFlow::enqueue_invoice_request_to_forward here

tankyleo · 2025-09-09T19:25:42Z

lightning/src/ln/async_payments_tests.rs

+		claim_payment_along_route(ClaimAlongRouteArgs::new(sender, route, keysend_preimage));
+	assert_eq!(res, Some(PaidBolt12Invoice::StaticInvoice(static_invoice)));
+
+	// After paying the static invoice, check that regular invoice received from async recipient is ignored.


For completeness, let's also cover the case where the async recipient responds first ?

valentinewallace

Basically looks solid, thanks for this @elnosh!

lightning/src/ln/msgs.rs

lightning/src/offers/flow.rs

lightning/src/events/mod.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2025-09-09T20:21:42Z

lightning/src/ln/channelmanager.rs

+	/// When handling an [`Event::StaticInvoiceRequested`], this should be called to forward the
+	/// [`InvoiceRequest`] over the `invoice_request_path` to the async recipient if it is online
+	/// and it will forward the [`StaticInvoice`] to the responder.
+	pub fn send_response_static_invoice_request(
+		&self, invoice: StaticInvoice, responder: Responder, invoice_request: InvoiceRequest,
+		invoice_request_path: BlindedMessagePath,
 	) -> Result<(), Bolt12SemanticError> {
+		self.flow.enqueue_invoice_request_to_forward(
+			invoice_request,
+			invoice_request_path,
+			responder.clone(),
+		);
 		self.flow.enqueue_static_invoice(invoice, responder)
 	}


I kinda feel like a better UX is more important, which I feel like a single method would be? Also this makes forwarding the invreq optional, whereas before it was required. We could handle the wait in LDK in std builds, theoretically, but in practice I don't feel like we'll ever want to increase potential payment latency for any reason. The sender can also wait a second for a fresh invoice if they choose.

Anyitechs

Thanks for your work on this @elnosh. It looks really good and helped my understanding of async receive.

Anyitechs · 2025-09-10T00:18:48Z

lightning/src/ln/async_payments_tests.rs

-	let invoice_flow_res =
-		pass_static_invoice_server_messages(&nodes[1], &nodes[2], recipient_id.clone());
-	let static_invoice = invoice_flow_res.invoice;
+	let static_invoice =


nit: is there a reason you separated the call to pass_static_invoice_server_messages() and extracting the static_invoice value elsewhere, but combined them here?

yeah from latest changes where I reverted back to send_response_static_invoice_request I separated it to also use the invoice_request_path.

Anyitechs · 2025-09-10T03:04:42Z

lightning/src/ln/channelmanager.rs

+	/// When handling an [`Event::StaticInvoiceRequested`], this should be called to forward the
+	/// [`InvoiceRequest`] over the `invoice_request_path` to the async recipient in case it is
+	/// online to respond.
+	pub fn forward_invoice_request(


noob question: I understand this is meant to be called/used when the async recipient is online, but what happens if they're not? I'd expect to see some kind of fallback mechanism that allows us to forward a static invoice, or is that something the user is expected to handle?

yeah if the recipient is not online it should fallback to paying static invoice it got from the always-online node. I reverted this method and using send_response_static_invoice_request where it does both. It forwards the invoice request to the async receiver and sends the static invoice to the payer. So if recipient is not online, payer should still get the static invoice and be able to pay that one.

elnosh · 2025-09-10T17:40:39Z

thank you for the reviews!
diff with changes. It mostly addressed:

reverting back to one method to send static and forward the invoice request.
add case in test where fresh invoice is paid first and the static invoice is ignored.
changed some comments as requested.

graphite-app · 2025-09-10T17:42:20Z

lightning/src/offers/invoice_request.rs

+#[cfg(not(test))]
+impl PartialEq for InvoiceRequest {
+	fn eq(&self, other: &Self) -> bool {
+		self.bytes.eq(&other.bytes) && self.signature.eq(&other.signature)
+	}
+}


The PartialEq implementation is conditionally excluded from test builds with #[cfg(not(test))]. This will prevent comparing InvoiceRequest instances in tests, causing compilation failures when using equality assertions or comparisons. Consider either:

Removing the conditional compilation to make PartialEq available in all contexts

Providing an alternative PartialEq implementation for test builds

This ensures consistent behavior across both production and test environments.

Suggested change

#[cfg(not(test))]

impl PartialEq for InvoiceRequest {

fn eq(&self, other: &Self) -> bool {

self.bytes.eq(&other.bytes) && self.signature.eq(&other.signature)

}

}

impl PartialEq for InvoiceRequest {

fn eq(&self, other: &Self) -> bool {

self.bytes.eq(&other.bytes) && self.signature.eq(&other.signature)

}

}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

valentinewallace

Basically LGTM, feel free to squash

valentinewallace · 2025-09-10T20:48:15Z

lightning/src/events/mod.rs

+		/// later provided to [`ChannelManager::send_response_static_invoice_request`].
+		///
+		/// [`ChannelManager::send_response_static_invoice_request`]: crate::ln::channelmanager::ChannelManager::send_response_static_invoice_request
+		invoice_request_path: BlindedMessagePath,


I'm not sure if it's awkward that we have a raw BlindedMessagePath here but use a Responder below... Not blocking though.

Hmmm yeah can be a bit awkward. I used a BlindedMessagePath since that is how it's sent in the ServeStaticInvoice message. And if passed as a Responder I think I would still need to use it as a BlindedMessagePath to build the MessageSendInstructions::WithSpecifiedReplyPath

lightning/src/ln/channelmanager.rs

lightning/src/ln/async_payments_tests.rs

lightning/src/offers/flow.rs

Previously it would generate a new nonce for this context instead of using the offer nonce. This would make it so that verification would fail later when receiving a invoice request.

valentinewallace

LGTM, feel free to squash

As a static invoice server, if we receive an invoice request on behalf of an often-offline recipient we will reply to the sender with the static invoice previously provided by the async recipient. Here, in addition to doing that we'll forward the invoice request received to the async recipient to give it a chance to reply with a fresh invoice in case it is online.

valentinewallace · 2025-09-12T15:27:58Z

lightning/src/offers/invoice_request.rs

+#[cfg(not(test))]
+impl PartialEq for InvoiceRequest {
+	fn eq(&self, other: &Self) -> bool {
+		self.bytes.eq(&other.bytes) && self.signature.eq(&other.signature)


I think based on the PartialEq implementation for Bolt12Invoice, it would be sufficient to just check the bytes here. IIRC the signature is included in the bytes

valentinewallace · 2025-09-12T16:12:05Z

@tankyleo I'm good to land this when you are

ldk-reviews-bot · 2025-09-13T00:00:15Z

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-09-15T00:00:46Z

🔔 2nd Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt · 2025-09-15T14:51:10Z

lightning/src/ln/channelmanager.rs

+			invoice_request_path,
+			responder.clone(),
+		);
 		self.flow.enqueue_static_invoice(invoice, responder)


How does this work? If we immediately send the static invoice the sender will ~always see that first and use that, and the optimistic non-static invoice will never be used.

Ah, @valentinewallace pointed out that this is really for the sender-doesn't-support-async case, not really for "upgrading" to non-static invoices when the recipient is online. We should write that somewhere...

Yeah, it was an oversight not to document that. It was actually pointed out somewhat late in the game by @joostjager here: #4045 (comment)

For more context, I had also told @elnosh that in follow-up we should add support for an always-online sender swapping out a pending outbound to a static invoice for one with a fresh invoice, if one comes in. Doesn't cover all cases though ofc.

TheBlueMatt · 2025-09-15T14:54:38Z

lightning/src/events/mod.rs

+		/// The invoice request that will be forwarded to the async recipient to give the
+		/// recipient a chance to provide an invoice in case it is online. It should be
+		/// provided to [`ChannelManager::respond_to_static_invoice_request`].
+		///


Presumably we should document here that LSPs should try to use LSPS5 to wake the client (based on the invoice_request_path first-hop, presumably? or is there a place where the LSP sees the counterparty node id? I don't see one in PersistStaticInvoice). From there the LSP should give them a second to come online before calling respond_to_static_invoice_request to make sure they have a shot at receiving the message.

Discussed it more with @valentinewallace really I think we need to be pushing the forwarded static invoice requests through the OnionMessenger forwarding pipeline (so we use the onion message mailbox logic and so that we avoid the direct-peer-connect logic) rather than the sending pipeline.

LSPs should try to use LSPS5 to wake the client (based on the invoice_request_path first-hop, presumably?

I was wondering why not this approach instead of the mailbox logic? It could be weird to have this flow outside of the mailbox but as you said it would allow the LSP to wake up the client before deciding to call respond_to_static_invoice_request to send the static invoice back. Which with the mailbox I think it will do both at the same time - generate the OnionMessageIntercepted event and send back the static invoice unless we separate them.

Also, in any of the 2 approaches, although I would think most likely the introduction node of the invoice_request_path will be the LSP, there's no way to guarantee that. So we would also need to first check the first hop and if it's not the LSP, just send the message?

This would be in addition/conjunction with the mailbox logic. LSPS5 is just a way to tell the client to run by sending a notification, we'd still have to keep the pending message in the mailbox so that we can deliver it when they come online.

joostjager · 2025-09-17T12:50:37Z

I tried to update ldk-node to this PR, but it failed the static invoice server test: lightningdevkit/ldk-node#635

RUST_BACKTRACE=1 cargo test static_invoice_server -- --nocapture

The logs are written to stdout with a node id at the start of the line, but still it is quite hard to debug. So many onion messages and/or retries and/or different paths involved.

I suspect it has something to do with the reply race introduced in this PR? Maybe it's not worth looking at, because with #4046 it seems fixed. It would be nice though to understand what's going on.

Also building ldk-node against current ldk main isn't possible atm, so it is blocking other work.

elnosh · 2025-09-18T14:39:55Z

is it good now? seems fixed in #4078

ldk-reviews-bot requested a review from tankyleo September 2, 2025 21:07

valentinewallace mentioned this pull request Sep 8, 2025

Async send always-online counterparty side #4045

Merged

tankyleo reviewed Sep 8, 2025

View reviewed changes

tankyleo reviewed Sep 9, 2025

View reviewed changes

valentinewallace reviewed Sep 9, 2025

View reviewed changes

Anyitechs reviewed Sep 10, 2025

View reviewed changes

elnosh force-pushed the server-fwd-invreq branch from be502cb to f109e16 Compare September 10, 2025 17:35

graphite-app bot reviewed Sep 10, 2025

View reviewed changes

elnosh requested review from tankyleo and valentinewallace September 10, 2025 17:54

valentinewallace reviewed Sep 10, 2025

View reviewed changes

elnosh force-pushed the server-fwd-invreq branch from f109e16 to 1266567 Compare September 10, 2025 22:08

elnosh added 2 commits September 10, 2025 17:26

Update OnionMessage with link to BOLTs

d1d51b3

Use correct nonce in InvoiceRequest context

d810708

Previously it would generate a new nonce for this context instead of using the offer nonce. This would make it so that verification would fail later when receiving a invoice request.

elnosh force-pushed the server-fwd-invreq branch from 1266567 to fa6f20d Compare September 10, 2025 22:30

tankyleo requested a review from valentinewallace September 11, 2025 18:59

valentinewallace reviewed Sep 11, 2025

View reviewed changes

elnosh force-pushed the server-fwd-invreq branch from fa6f20d to f46a3ff Compare September 11, 2025 21:34

valentinewallace approved these changes Sep 12, 2025

View reviewed changes

valentinewallace mentioned this pull request Sep 12, 2025

Async Payments #2298

Closed

45 tasks

Anyitechs approved these changes Sep 12, 2025

View reviewed changes

tankyleo approved these changes Sep 15, 2025

View reviewed changes

valentinewallace merged commit 5ae19b4 into lightningdevkit:main Sep 15, 2025
24 checks passed

TheBlueMatt reviewed Sep 15, 2025

View reviewed changes

valentinewallace mentioned this pull request Sep 15, 2025

Async send: sender-side #4046

Merged

3 tasks

joostjager mentioned this pull request Sep 16, 2025

Update static invoice store for invoice requests lightningdevkit/ldk-node#635

Merged

TheBlueMatt mentioned this pull request Sep 23, 2025

Fix forwarding OMs to OM-mailbox clients when sending a fallback invoice_request for a static invoice #4110

Closed

tankyleo removed their request for review October 10, 2025 18:25

Always-online node forward invoice request #4049

Always-online node forward invoice request #4049

Uh oh!

Conversation

elnosh commented Sep 2, 2025

Uh oh!

ldk-reviews-bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valentinewallace commented Sep 2, 2025

Uh oh!

ldk-reviews-bot commented Sep 4, 2025

Uh oh!

ldk-reviews-bot commented Sep 8, 2025

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tankyleo Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Sep 8, 2025

Uh oh!

tankyleo commented Sep 9, 2025

Uh oh!

elnosh commented Sep 9, 2025

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anyitechs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elnosh commented Sep 10, 2025

Uh oh!

graphite-app bot Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ldk-reviews-bot commented Sep 2, 2025 •

edited

Loading

codecov bot commented Sep 2, 2025 •

edited

Loading

tankyleo Sep 9, 2025 •

edited

Loading

joostjager commented Sep 17, 2025 •

edited

Loading