Introduce Flow utilities and OffersMessageFlow implementation #3639

shaavan · 2025-03-03T22:18:58Z

This PR presents an alternative approach to #3412.

It introduces the Flow trait and its implementation, OffersMessageFlow, as a mid-level API that abstracts most Offers-related logic out of ChannelManager, while allowing ChannelManager to be parameterized over Flow.

This improves modularity and separation of concerns, making the Offers-related code more maintainable. Additionally, it provides a set of functional utilities for users who do not use ChannelManager in their workflow.

ldk-reviews-bot · 2025-03-03T22:19:01Z

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

shaavan · 2025-03-03T22:26:28Z

Right now, this PR is a proof of concept for the approach—it introduces a basic set of utilities to move a significant chunk of Offers-related code out of ChannelManager.

What's in the current version:

Added initial utilities for creating Offers, Refunds, Blinded Paths, and handling message enqueue/dequeue directly within Flow.
Started using these utilities inside Offer payers and builders.

About the CI failures:

The CI is failing because of some documentation issues in this version.
All functional tests for the std case are passing locally.

If this approach looks good, I'll expand it to cover Offer handlers next!

shaavan · 2025-03-03T22:27:13Z

@TheBlueMatt, @jkczyz
A gentle ping! :)

jkczyz

Let's iterate a bit more on this. We should have the MessageContext creation and thus the BlindedPath creation be the domain of OffersMessageFlow, IMO. This means passing a closure to the utility functions to create the blinded paths. Likewise with BlindedPaymentPaths.

jkczyz · 2025-03-04T15:52:23Z

lightning/src/offers/flow.rs

+	message_router: MR,
+
+	our_network_pubkey: PublicKey,
+	highest_seen_timestamp: AtomicUsize,


We'd need to update this any time a block is processed. So either we need an Arc to share with ChannelManager or have Flow require chain::Listen such that ChannelManager (or whoever owns the Flow) can call block_connected.

That makes sense! I’ve implemented the chain::Listen trait for OffersMessageFlow in pr3639.02. This allows block_connected to be called as needed.

Let me know if this approach aligns with what you had in mind! Thanks a lot for the pointer!

jkczyz · 2025-03-04T15:53:55Z

lightning/src/offers/flow.rs

+	#[cfg(not(any(test, feature = "_test_utils")))]
+	pending_offers_messages: Mutex<Vec<(OffersMessage, MessageSendInstructions)>>,
+	#[cfg(any(test, feature = "_test_utils"))]
+	pub(crate) pending_offers_messages: Mutex<Vec<(OffersMessage, MessageSendInstructions)>>,
+
+	pending_async_payments_messages: Mutex<Vec<(AsyncPaymentsMessage, MessageSendInstructions)>>,
+
+	#[cfg(feature = "dnssec")]
+	pending_dns_onion_messages: Mutex<Vec<(DNSResolverMessage, MessageSendInstructions)>>,


I think the idea here was to have the owner such as ChannelManager own the message queues since it would implement the message handlers.

I was deciding between two designs: having ChannelManager hold the queues or having OffersMessageFlow hold them. I chose the latter because:

The long-term goal is to fully separate offers-related code from ChannelManager, and moving the queues out will be a major step in that direction.

Keeping the queues within OffersMessageFlow helps maintain cleaner utility function signatures, as users don’t need to explicitly pass the queues as parameters.

Let me know your thoughts on this approach!

I kinda like having the queues in the flow? It makes it more of a useful "object", rather than a "bag of functions".

jkczyz · 2025-03-04T15:58:50Z

lightning/src/offers/flow.rs

+	fn create_offer_builder(
+		&self, nonce: Nonce,
+	) -> Result<OfferBuilder<DerivedMetadata, secp256k1::All>, Bolt12SemanticError> {
+		let node_id = self.get_our_node_id();
+		let expanded_key = &self.inbound_payment_key;
+		let secp_ctx = &self.secp_ctx;
+
+		let builder = OfferBuilder::deriving_signing_pubkey(node_id, expanded_key, nonce, secp_ctx)
+			.chain_hash(self.chain_hash);
+
+		Ok(builder)
+	}


I'd question what value utilities like these are really providing to the the user.

The idea I was exploring was to create a mid-level API that remains modular enough to be used in multiple contexts.

For example, self.flow.create_offer_builder simplifies both create_offer_builder and create_async_receive_offer_builder, making the API more reusable and reducing duplication.

Would love to hear your thoughts on whether this with indented design approach.

I'm similarly skeptical of adding methods that do "nothing". In cases where we're delegating blinded message path building to the flow, fine, but here I'm pretty skeptical this is adding much value.

jkczyz · 2025-03-04T16:04:31Z

lightning/src/offers/flow.rs

+	MR::Target: MessageRouter,
+{
+	fn create_offer_builder(
+		&self, nonce: Nonce,


Seems like the Nonce could be generated from the EntropySource. Though I imagine we need the Nonce for the MessageContext. Don't we want OffersMessageFlow to determine the correct MessageContext to give to the BlindedPaths? Otherwise, the onus is on the user to pass the correct context. So this implies the blinded paths should be created within this method.

Makes sense! My initial reasoning was to keep offer building and blinded path creation separate, as this allowed us to use the same self.flow.create_offer_builder for both create_offer_builder (where we create & append the path later) and create_async_receive_offer_builder (where we already have a vector of paths to append).

That said, I think we can take a middle ground by passing an optional closure as input—if present, it would handle the creation and appending of paths. This keeps things flexible while reducing the onus on the user to manually pass the correct context.

Let me know what you think of this approach. Thanks!

jkczyz · 2025-03-04T16:08:35Z

lightning/src/offers/flow.rs

+	fn create_invoice_builder<'a>(
+		&'a self, refund: &'a Refund, payment_paths: Vec<BlindedPaymentPath>,
+		payment_hash: PaymentHash,
+	) -> Result<InvoiceBuilder<'a, DerivedSigningPubkey>, Bolt12SemanticError> {


Note we need this not only for refunds but also for handling invoice requests. I'm wondering though if we'd rather want a method that does more similar to request_refund_payment and pay_for_offer?

Looking through the section of code where we create an invoice from an invoice request, it seems significantly different from invoice creation from a refund. Given our current trait design, introducing a separate function feels like a cleaner solution.

On the other hand, we could design the Flow trait to be more similar to request_refund_payment and pay_for_offer, but wouldn’t that make it too high-level of an API?

@TheBlueMatt, would love to hear your thoughts on this!

Similarly, I feel like it'd be nice to have a higher-level API on the flow. If the goal is to move bolt12-specific logic out of ChannelManager as much as possible, we should target a high-level API and then make the Flow object configurable where we see demand for configuration, IMO.

jkczyz · 2025-03-04T16:09:17Z

lightning/src/offers/flow.rs

+	}
+
+	fn create_blinded_paths(
+		&self, peers: Vec<MessageForwardNode>, context: MessageContext,


Again, it seems OffersMessageFlow should be the one determining which context to use.

Right, hiding the context in the flow gives it some material value for the user cause that's a bit of complexity that the user wouldn't have to implement.

jkczyz · 2025-03-04T16:26:05Z

lightning/src/offers/flow.rs


 #[cfg(feature = "dnssec")]
 use crate::onion_message::dns_resolution::DNSResolverMessage;

+pub trait Flow {


After going through the PR, I'm not sure how useful the Flow trait is. Either the caller of the utilities can do any processing between calls or we can parameterize OffersMessageFlow with traits (e.g., currency conversion).

I think I agree with your point. We could make OffersMessageFlow a direct parameter of ChannelManager without needing the Flow trait as an intermediary. This way, we still remove Bolt12 responsibilities from ChannelManager while giving users the flexibility to implement their own handlers using OffersMessageFlow utilities.

@TheBlueMatt, I’d love to hear your thoughts on this approach!

I think it depends a bit on where we land on the API itself - if its high-level and we allow the OffersMessageFlow to be configured where there is a need for options, great, we can drop the trait. If we end up with something where we want it to not be super configurable but rather overridable, we can keep the flow.

TheBlueMatt

Were we going to move most of the OffersMessageHandler logic into flow.rs?

shaavan · 2025-03-06T13:11:11Z

Updated from pr3639.01 to pr3639.02 (diff):
Addressed @jkczyz comments

Changes:

Implement chain::Listen of OffersMessageFlow, to allow keep the highest_seen_timestamp synced.

shaavan · 2025-03-06T13:28:31Z

@TheBlueMatt

Were we going to move most of the OffersMessageHandler logic into flow.rs?

Yes, that’s the goal! For this version, I’ve primarily abstracted out the Bolt12 payers/builders from ChannelManager. I wanted to first get a concept ACK on this approach before proceeding with abstracting the handlers out of ChannelManager.

TheBlueMatt · 2025-03-10T18:36:18Z

Yea, its just a bit hard to evaluate without that change - there's presumably some state that can move into the flow handler...

shaavan · 2025-03-17T19:04:43Z

Updated from pr3639.02 to pr3639.03 (diff):
Addressed @jkczyz and @TheBlueMatt comments

Changes:

Completed the Flow approach by incorporating changes across all relevant functions, handlers, and queues.

Points of notice:

In this approach, OffersMessageFlow is no longer parameterized on Route/MessageRouter. Instead, path creation is passed in as a closure to OffersMessageFlow. This removes the need for multiple layers of parameterization for Router/MessageRouter.
I've aimed to keep this as a mid-level API that removes the need for users of ChannelManager to manually handle context creation.
While the approach is functionally complete (all tests passed when I ran cargo test 🚀), I haven’t yet worked on documentation or run rustfmt, so CI will likely be sad :)

shaavan · 2025-03-17T19:08:09Z

Also, a gentle ping @TheBlueMatt and @jkczyz — thank you so much for all your inputs and ideas! Really grateful for them.
Looking forward to your thoughts on the completed approach! 🙌

jkczyz · 2025-03-18T15:58:19Z

lightning/src/ln/channelmanager.rs

+		self.flow.enqueue_invoice_request(invoice_request.clone(), payment_id, Some(nonce), |context| {
+			self.create_blinded_paths(context)
+		})?;

-		Ok(())
+		create_pending_payment(&invoice_request, nonce)


@TheBlueMatt Here there a is change such that we enqueue the invoice requests prior to creating the pending payment. Seems like this could be an issue?

@shaavan Could you remind me why the ordering needed to be changed here?

Sure!

I noticed that with the original ordering (create_pending_payment -> self.flow.enqueue_invoice_request), if we try to pay for an offer and fail during the Blinded Path creation stage, we still end up creating a pending payment with that PaymentId in pending_outbound_payments.

So, if we try to call pay_for_offer again with the same payment_id, we hit a DuplicatePaymentId error — because the payment was already enqueued in pending_outbound_payments during the first (failed) attempt.

Happy to share a branch that reproduces the error if it's helpful. Thanks!

@TheBlueMatt Here there a is change such that we enqueue the invoice requests prior to creating the pending payment. Seems like this could be an issue?

Hmm, don't see why it would be? Its all in a single PersistenceNotifierGuard so the intermediate state cannot be written to disk (and we don't write the queue to disk anyway), and while in a theoretical edge case we could get the message out and get a response back faster than we can add the pending payment (causing us to consider the responding Invoice un-requested), I'm somewhat skeptical its worth worrying about here.

jkczyz · 2025-03-18T16:01:05Z

lightning/src/ln/channelmanager.rs

-			debug_assert!(false);
-			return Err(Bolt12SemanticError::MissingIssuerSigningPubkey);
-		}
+		self.flow.enqueue_invoice_request(invoice_request.clone(), payment_id, Some(nonce), |context| {


It's unfortunate that the user needs to know that they must pass the same nonce here as they had passed to create_invoice_request_builder.

That's true! Maybe we can add a short note in the enqueue_invoice_request documentation mentioning that the nonce the user passes here must exactly match the nonce used to create the invoice_request.

Alternatively, we could consider renaming the parameter to invoice_request_nonce to make the connection clearer.

Let me know what you think — Thanks!

TheBlueMatt · 2025-03-25T01:45:32Z

lightning/src/ln/channelmanager.rs

-				Err(()) => {
-					self.abandon_payment_with_reason(payment_id, PaymentFailureReason::BlindedPathCreationFailed);
+			if self.flow.enqueue_async_payment_messages(invoice, payment_id, |context| {
+				self.create_blinded_paths(context)


Rather than passing a lambda for create_blinded_paths to the flow, can we just pass the output of self.list_usable_channels() and let the flow handle the rest? Seems like that would further reduce bolt12-specific code in ChannelManager. Somewhat awkwardly, LNDK probably needs to be able to specify its own blinded payment paths, but would probably prefer the flow specify the blinded message paths. If we end up making this change that probably means we should add a separate method for LNDK to provide its own blinded path.

TheBlueMatt · 2025-03-25T01:46:16Z

lightning/src/offers/flow.rs

+	#[cfg(not(any(test, feature = "_test_utils")))]
+	pending_offers_messages: Mutex<Vec<(OffersMessage, MessageSendInstructions)>>,
+	#[cfg(any(test, feature = "_test_utils"))]
+	pub(crate) pending_offers_messages: Mutex<Vec<(OffersMessage, MessageSendInstructions)>>,
+
+	pending_async_payments_messages: Mutex<Vec<(AsyncPaymentsMessage, MessageSendInstructions)>>,
+
+	#[cfg(feature = "dnssec")]
+	pending_dns_onion_messages: Mutex<Vec<(DNSResolverMessage, MessageSendInstructions)>>,


I kinda like having the queues in the flow? It makes it more of a useful "object", rather than a "bag of functions".

TheBlueMatt · 2025-03-25T01:47:31Z

lightning/src/offers/flow.rs

+	fn create_offer_builder(
+		&self, nonce: Nonce,
+	) -> Result<OfferBuilder<DerivedMetadata, secp256k1::All>, Bolt12SemanticError> {
+		let node_id = self.get_our_node_id();
+		let expanded_key = &self.inbound_payment_key;
+		let secp_ctx = &self.secp_ctx;
+
+		let builder = OfferBuilder::deriving_signing_pubkey(node_id, expanded_key, nonce, secp_ctx)
+			.chain_hash(self.chain_hash);
+
+		Ok(builder)
+	}


I'm similarly skeptical of adding methods that do "nothing". In cases where we're delegating blinded message path building to the flow, fine, but here I'm pretty skeptical this is adding much value.

TheBlueMatt · 2025-03-25T01:51:07Z

lightning/src/offers/flow.rs

+	fn create_invoice_builder<'a>(
+		&'a self, refund: &'a Refund, payment_paths: Vec<BlindedPaymentPath>,
+		payment_hash: PaymentHash,
+	) -> Result<InvoiceBuilder<'a, DerivedSigningPubkey>, Bolt12SemanticError> {


Similarly, I feel like it'd be nice to have a higher-level API on the flow. If the goal is to move bolt12-specific logic out of ChannelManager as much as possible, we should target a high-level API and then make the Flow object configurable where we see demand for configuration, IMO.

TheBlueMatt · 2025-03-25T01:52:42Z

lightning/src/offers/flow.rs


 #[cfg(feature = "dnssec")]
 use crate::onion_message::dns_resolution::DNSResolverMessage;

+pub trait Flow {


I think it depends a bit on where we land on the API itself - if its high-level and we allow the OffersMessageFlow to be configured where there is a need for options, great, we can drop the trait. If we end up with something where we want it to not be super configurable but rather overridable, we can keep the flow.

TheBlueMatt · 2025-03-25T01:55:06Z

lightning/src/ln/channelmanager.rs

+		self.flow.enqueue_invoice_request(invoice_request.clone(), payment_id, Some(nonce), |context| {
+			self.create_blinded_paths(context)
+		})?;

-		Ok(())
+		create_pending_payment(&invoice_request, nonce)


@TheBlueMatt Here there a is change such that we enqueue the invoice requests prior to creating the pending payment. Seems like this could be an issue?

Hmm, don't see why it would be? Its all in a single PersistenceNotifierGuard so the intermediate state cannot be written to disk (and we don't write the queue to disk anyway), and while in a theoretical edge case we could get the message out and get a response back faster than we can add the pending payment (causing us to consider the responding Invoice un-requested), I'm somewhat skeptical its worth worrying about here.

shaavan force-pushed the flow branch from 65ace16 to eb764be Compare March 4, 2025 12:30

jkczyz reviewed Mar 4, 2025

View reviewed changes

TheBlueMatt reviewed Mar 5, 2025

View reviewed changes

shaavan force-pushed the flow branch from eb764be to 286ccd7 Compare March 6, 2025 13:11

jkczyz mentioned this pull request Mar 7, 2025

Multiple InvoiceReceived events for single pay_for_offer call #3653

Open

valentinewallace mentioned this pull request Mar 17, 2025

Client-side of static invoice server #3618

Open

shaavan added 5 commits March 17, 2025 23:29

Introduce Flow trait

287c932

Introduce OffersMessageFlow

8adea39

Implement Flow for OffersMessageFlow

859f637

Introduce Flow parameter in ChannelManager

e6077fb

Introduce Flow usage in ChannelManager

3a78267

shaavan force-pushed the flow branch from 286ccd7 to 3a78267 Compare March 17, 2025 19:04

shaavan requested review from TheBlueMatt and jkczyz March 17, 2025 19:05

jkczyz reviewed Mar 18, 2025

View reviewed changes

TheBlueMatt removed their request for review March 18, 2025 19:13

TheBlueMatt reviewed Mar 25, 2025

View reviewed changes

Introduce Flow utilities and OffersMessageFlow implementation #3639

Are you sure you want to change the base?

Introduce Flow utilities and OffersMessageFlow implementation #3639

Conversation

shaavan commented Mar 3, 2025

ldk-reviews-bot commented Mar 3, 2025 • edited Loading

shaavan commented Mar 3, 2025

What's in the current version:

About the CI failures:

shaavan commented Mar 3, 2025

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

shaavan commented Mar 6, 2025

shaavan commented Mar 6, 2025

TheBlueMatt commented Mar 10, 2025

shaavan commented Mar 17, 2025

shaavan commented Mar 17, 2025

jkczyz Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Mar 3, 2025 •

edited

Loading

jkczyz Mar 4, 2025 •

edited

Loading

jkczyz Mar 4, 2025 •

edited

Loading

jkczyz Mar 18, 2025 •

edited

Loading