Support passing preimage for spontaneous payments #549

aagbotemi · 2025-05-31T12:00:12Z

This PR adds support for specifying custom preimages when sending spontaneous (keysend) payments.

Solution

Added send_with_preimage() method
Added send_with_preimage_and_custom_tlvs() method for advanced use cases
Modified internal send_inner() to accept optional preimage parameter and generate random preimage only when custom preimage is not provided.

An integration test is added to verify logic, and the new methods are added to the bindings.

Fixes #535

ldk-reviews-bot · 2025-05-31T12:00:15Z

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

chuksys

Just shared a few thoughts regarding the overall design of this API and its extensibility.

chuksys · 2025-06-01T08:43:14Z

src/payment/spontaneous.rs

+	/// Send a spontaneous with custom preimage
+	pub fn send_with_preimage(
+		&self, amount_msat: u64, node_id: PublicKey, sending_parameters: Option<SendingParameters>,
+		preimage: PaymentPreimage,


I'm wondering if we should expand the purpose of SendingParameters a bit to allow passing of any custom parameters required for sending payments (not just routing and pathfinding parameters).

This approach would allow us to include preimage as an Option within SendingParameters, providing a single, coherent place for all payment-sending customizations.

A significant benefit of this change would be the ability to deprecate or drop specialized functions like send_with_preimage and send_with_custom_tlvs (in another PR), reducing overall boilerplate for users.

What do you think about this?

I initially hesitated about mixing routing parameters with payment content. Your point about expanding SendingParameters to handle all payment customization makes sense for the long-term API design, and the benefit outweigh the conceptual mixing.

I'll refactor to move preimage into SendingParameters as an Option. I can drop the send_with_preimage and send_with_preimage_and_custom_tlvs, then send_with_custom_tlvs can be dropped in another PR.

Thank you.

I'm wondering if we should expand the purpose of SendingParameters a bit to allow passing of any custom parameters required for sending payments (not just routing and pathfinding parameters).

Ah, no, I don't think we should do that. Note that after LDK 0.2 release (cf. #462) we'll replace SendingParameters with LDK's RouteParametersConfig everywhere, so really shouldn't make any changes prior to it. Also, while we might be fine to expose the preimage for keysend payments, I don't think we'd like to allow users to override it for BOLT11/BOLT12 generally.

I'll refactor to move preimage into SendingParameters as an Option.

Please revert this. I think we can either have separate send APIs as you previously suggested, or add the preimage as an optional field to the existing ones. Will take a look after the revert, but your previous approach might be fine.

Ah, no, I don't think we should do that. Note that after LDK 0.2 release (cf. #462) we'll replace SendingParameters with LDK's RouteParametersConfig everywhere, so really shouldn't make any changes prior to it. Also, while we might be fine to expose the preimage for keysend payments, I don't think we'd like to allow users to override it for BOLT11/BOLT12 generally.

Thanks for the clarification!

chuksys · 2025-06-01T15:39:47Z

src/payment/spontaneous.rs

+		self.send_inner(amount_msat, node_id, sending_parameters, Some(custom_tlvs), None)
+	}
+
+	/// Send a spontaneous with custom preimage


I think you missed the word "payment" here (after spontaneous) - that's if we'd still be adding this method.

Yeah, I missed that. Thank you. I am removing the method.

chuksys

If we are going to use sending_parameters to pass in the custom preimage, I think we should allow the user to set it in config or pass it directly.

chuksys · 2025-06-02T07:09:13Z

src/payment/spontaneous.rs

@@ -77,7 +77,10 @@ impl SpontaneousPayment {
 			return Err(Error::NotRunning);
 		}

-		let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
+		let payment_preimage = sending_parameters


If we are doing this, I think we'll need to use override_params instead as that allows send_inner to select sending_parameters set in the config (if not passed into the send method directly).

No, I don't think we should do this, see above.

tnull

As mentioned above I don't think adding this field to SendingParameters is the way to go here (note it would also allowing setting global default preimage if users would override the field in Config, which is actually dangerous as preimages can never be reused). Please revert to you previous draft, will review then.

tnull · 2025-06-02T11:15:53Z

src/payment/spontaneous.rs

@@ -77,7 +77,10 @@ impl SpontaneousPayment {
 			return Err(Error::NotRunning);
 		}

-		let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
+		let payment_preimage = sending_parameters


No, I don't think we should do this, see above.

aagbotemi · 2025-06-02T12:21:07Z

Please revert to you previous draft, will review then.

Alright. Will do that. Thank you.

aagbotemi · 2025-06-02T13:30:41Z

Please revert to you previous draft, will review then.

Hello @tnull, I have reverted to the previous draft. This is ready.

Camillarhi · 2025-06-02T18:43:48Z

Nice work on this PR! The custom preimage functionality is implemented cleanly, and the test coverage looks good.

One small enhancement suggestion: we could add verification that node_b received the payment to complete end-to-end verification of the payment flow.

aagbotemi · 2025-06-03T09:09:53Z

One small enhancement suggestion: we could add verification that node_b received the payment to complete end-to-end verification of the payment flow.

Thanks for the suggestion! I've added the end-to-end verification as requested. It also ensure the custom preimage flows correctly through the entire payment process from sender to receiver.

tnull

Basically LGTM, but now that we actually take a PaymentPreimage from user input we should probably avoid the UniffiCustomTypeConverter for it, and rather expose a 'proper' newtype wrapper for the bindings. See #542 for inspiration on this, for example.

tnull · 2025-06-03T10:19:56Z

bindings/ldk_node.udl

@@ -213,6 +213,10 @@ interface SpontaneousPayment {
 	PaymentId send_with_custom_tlvs(u64 amount_msat, PublicKey node_id, SendingParameters? sending_parameters, sequence<CustomTlvRecord> custom_tlvs);
 	[Throws=NodeError]
 	void send_probes(u64 amount_msat, PublicKey node_id);
+	[Throws=NodeError]


nit: Let's move these new entries to the correct location w.r.t. API, i.e. below send_with_custom_tlvs.

tnull · 2025-06-03T10:21:10Z

src/payment/spontaneous.rs

+	/// Send a spontaneous payment with custom preimage
+	pub fn send_with_preimage(
+		&self, amount_msat: u64, node_id: PublicKey, sending_parameters: Option<SendingParameters>,
+		preimage: PaymentPreimage,


If we add this as a new required parameter, let's move it before the optional parameters, i.e., Option<SendingParameters> (here and below).

Alright, will fix it. Thank you.

aagbotemi · 2025-06-04T09:02:13Z

Basically LGTM, but now that we actually take a PaymentPreimage from user input we should probably avoid the UniffiCustomTypeConverter for it, and rather expose a 'proper' newtype wrapper for the bindings. See #542 for inspiration on this, for example.

Thank you for the feedback, I will fix it

chuksys

Generally looks good to me - would appreciate some clarity on why we have maybe_wrap and maybe_wrap_arc.

chuksys · 2025-06-25T22:38:06Z

src/ffi/mod.rs

+pub fn maybe_wrap_arc<T>(ldk_type: impl Into<T>) -> std::sync::Arc<T> {
 	std::sync::Arc::new(ldk_type.into())
 }

+#[cfg(feature = "uniffi")]
+pub fn maybe_wrap<T>(ldk_type: impl Into<T>) -> T {
+	ldk_type.into()
+}
+


I'm probably missing something here but I'm wondering why we need to "split" maybe_wrap into these 2 functions.

maybe_wrap and maybe_wrap_arc is to handle different type wrapping requirements depending on whether the uniffi feature is enabled. maybe_wrap_arc wraps the input type in a std::sync::Arc<T>. maybe_wrap performs a simple into() conversion without additional wrapping, used in cases where the type doesn’t need to be wrapped in an Arc.

From my understanding, I thought maybe_wrap (as it was before) already handled wrapping depending on whether the uniFFI feature flag is enabled - if enabled wrap in an Arc, if not return same value as passed in?

I noticed maybe_wrap (this new version) is used to conditionally wrap the preimage - does this mean the preimage never needs to be wrapped in an Arc even when the uniFFI feature flag is enabled?

In #542, the wrapper types introduced were for structs with a bit more complexity and state than PaymentPreimage. From my understanding, it makes sense to allocate them on the heap because they are also expensive to clone. PaymentPreimage is simpler, wraps 32 bytes, and should be cheaper to clone and doesn't need to be Arc'ed.

Also, when compiled with uniffi, there was a bit of conflict about converting maybe_wrap into a macro so it can handle the different cases (i.e. types needing heap allocation vs types that do not), however I think @tnull may not like the idea for reasons stated in #526.

This change is easily revertible though if Arcing is preferred.

In #542, the wrapper types introduced were for structs with a bit more complexity and state than PaymentPreimage. From my understanding, it makes sense to allocate them on the heap because they are also expensive to clone. PaymentPreimage is simpler, wraps 32 bytes, and should be cheaper to clone and doesn't need to be Arc'ed.

Thanks for the clarification! This makes sense.

chuksys · 2025-06-25T22:45:55Z

src/ffi/mod.rs

+#[cfg(not(feature = "uniffi"))]
+pub fn maybe_wrap_arc<T>(value: T) -> T {
+	value
+}
+
 #[cfg(not(feature = "uniffi"))]
 pub fn maybe_wrap<T>(value: T) -> T {
 	value
 }


These 2 fns are basically the same, please is there a reason why we need both of them?

both maybe_wrap_arc and maybe_wrap are identical, simply returning the input value. The reason for keeping them as separate functions is to maintain a consistent API that aligns with their distinct behaviors when the uniffi feature is enabled

aagbotemi · 2025-06-25T23:24:12Z

Hi @tnull @enigbe. I pushed some update that expose a 'proper' newtype wrapper for the bindings.

enigbe · 2025-06-26T10:05:43Z

src/ffi/mod.rs

+pub fn maybe_wrap_arc<T>(ldk_type: impl Into<T>) -> std::sync::Arc<T> {
 	std::sync::Arc::new(ldk_type.into())
 }

+#[cfg(feature = "uniffi")]
+pub fn maybe_wrap<T>(ldk_type: impl Into<T>) -> T {
+	ldk_type.into()
+}
+


In #542, the wrapper types introduced were for structs with a bit more complexity and state than PaymentPreimage. From my understanding, it makes sense to allocate them on the heap because they are also expensive to clone. PaymentPreimage is simpler, wraps 32 bytes, and should be cheaper to clone and doesn't need to be Arc'ed.

Also, when compiled with uniffi, there was a bit of conflict about converting maybe_wrap into a macro so it can handle the different cases (i.e. types needing heap allocation vs types that do not), however I think @tnull may not like the idea for reasons stated in #526.

This change is easily revertible though if Arcing is preferred.

enigbe · 2025-06-26T10:28:45Z

bindings/ldk_node.udl

@@ -446,6 +450,10 @@ dictionary PaymentDetails {
 	u64 latest_update_timestamp;
 };

+dictionary PaymentPreimage {
+	bytes inner;


I wonder about what would be the most appropriate description for [u8;N]: bytes vs sequence<u8>?

bytes represents fixed-size binary data like preimage better

enigbe · 2025-06-26T10:35:53Z

@aagbotemi
Just added a couple of comments.
You'll need to give CI another kick to get the failing Rust Tests to pass.

- integration test for passing preimage for spontaneous payment - bindings for passing preimage for spontaneous payment - refactor: move preimage from method parameter to SendingParameters - revert to parameter-based approach - verify node_b recieved payment for end-to-end verification - expose a proper newtype wrapper for the PaymentPreimage bindings - trigger CI

ldk-reviews-bot requested a review from TheBlueMatt May 31, 2025 12:10

chuksys reviewed Jun 1, 2025

View reviewed changes

chuksys reviewed Jun 2, 2025

View reviewed changes

tnull removed the request for review from TheBlueMatt June 2, 2025 11:15

tnull reviewed Jun 2, 2025

View reviewed changes

tnull reviewed Jun 3, 2025

View reviewed changes

aagbotemi force-pushed the support-preimage-spontaneous-payment branch 2 times, most recently from 2ffdc53 to 5194ccc Compare June 25, 2025 21:04

chuksys reviewed Jun 25, 2025

View reviewed changes

enigbe reviewed Jun 26, 2025

View reviewed changes

aagbotemi force-pushed the support-preimage-spontaneous-payment branch from 68a141d to c33d291 Compare June 26, 2025 13:53

Support passing preimage for spontaneous payments #549

Are you sure you want to change the base?

Support passing preimage for spontaneous payments #549

Uh oh!

Conversation

aagbotemi commented May 31, 2025

Solution

Uh oh!

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

Uh oh!

Uh oh!

chuksys 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!

aagbotemi Jun 1, 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!

chuksys 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!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aagbotemi commented Jun 2, 2025

Uh oh!

aagbotemi commented Jun 2, 2025

Uh oh!

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

Uh oh!

Uh oh!

aagbotemi commented Jun 3, 2025

Uh oh!

tnull 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!

aagbotemi commented Jun 4, 2025

Uh oh!

chuksys 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!

Choose a reason for hiding this comment

Uh oh!

aagbotemi Jun 25, 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!

aagbotemi commented Jun 25, 2025

ldk-reviews-bot commented May 31, 2025 •

edited

Loading

aagbotemi Jun 1, 2025 •

edited

Loading

Camillarhi commented Jun 2, 2025 •

edited

Loading

aagbotemi Jun 25, 2025 •

edited

Loading