Skip to content

Support passing preimage for spontaneous payments #549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions bindings/ldk_node.udl
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

PaymentId send_with_preimage(u64 amount_msat, PublicKey node_id, SendingParameters? sending_parameters, PaymentPreimage preimage);
[Throws=NodeError]
PaymentId send_with_preimage_and_custom_tlvs(u64 amount_msat, PublicKey node_id, SendingParameters? sending_parameters, sequence<CustomTlvRecord> custom_tlvs, PaymentPreimage preimage);
};

interface OnchainPayment {
Expand Down
25 changes: 21 additions & 4 deletions src/payment/spontaneous.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,27 +57,44 @@ impl SpontaneousPayment {
pub fn send(
&self, amount_msat: u64, node_id: PublicKey, sending_parameters: Option<SendingParameters>,
) -> Result<PaymentId, Error> {
self.send_inner(amount_msat, node_id, sending_parameters, None)
self.send_inner(amount_msat, node_id, sending_parameters, None, None)
}

/// Send a spontaneous payment including a list of custom TLVs.
pub fn send_with_custom_tlvs(
&self, amount_msat: u64, node_id: PublicKey, sending_parameters: Option<SendingParameters>,
custom_tlvs: Vec<CustomTlvRecord>,
) -> Result<PaymentId, Error> {
self.send_inner(amount_msat, node_id, sending_parameters, Some(custom_tlvs))
self.send_inner(amount_msat, node_id, sending_parameters, Some(custom_tlvs), None)
}

/// 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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, will fix it. Thank you.

) -> Result<PaymentId, Error> {
self.send_inner(amount_msat, node_id, sending_parameters, None, Some(preimage))
}

/// Send a spontaneous payment with custom preimage including a list of custom TLVs.
pub fn send_with_preimage_and_custom_tlvs(
&self, amount_msat: u64, node_id: PublicKey, sending_parameters: Option<SendingParameters>,
custom_tlvs: Vec<CustomTlvRecord>, preimage: PaymentPreimage,
) -> Result<PaymentId, Error> {
self.send_inner(amount_msat, node_id, sending_parameters, Some(custom_tlvs), Some(preimage))
}

fn send_inner(
&self, amount_msat: u64, node_id: PublicKey, sending_parameters: Option<SendingParameters>,
custom_tlvs: Option<Vec<CustomTlvRecord>>,
custom_tlvs: Option<Vec<CustomTlvRecord>>, preimage: Option<PaymentPreimage>,
) -> Result<PaymentId, Error> {
let rt_lock = self.runtime.read().unwrap();
if rt_lock.is_none() {
return Err(Error::NotRunning);
}

let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
let payment_preimage = preimage
.unwrap_or_else(|| PaymentPreimage(self.keys_manager.get_secure_random_bytes()));
let payment_hash = PaymentHash::from(payment_preimage);
let payment_id = PaymentId(payment_hash.0);

Expand Down
72 changes: 70 additions & 2 deletions tests/integration_tests_rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ use common::{
use ldk_node::config::EsploraSyncConfig;
use ldk_node::liquidity::LSPS2ServiceConfig;
use ldk_node::payment::{
ConfirmationStatus, PaymentDirection, PaymentKind, PaymentStatus, QrPaymentResult,
SendingParameters,
ConfirmationStatus, PaymentDetails, PaymentDirection, PaymentKind, PaymentStatus,
QrPaymentResult, SendingParameters,
};
use ldk_node::{Builder, Event, NodeError};

Expand All @@ -29,8 +29,10 @@ use lightning::routing::gossip::{NodeAlias, NodeId};
use lightning::util::persist::KVStore;

use lightning_invoice::{Bolt11InvoiceDescription, Description};
use lightning_types::payment::PaymentPreimage;

use bitcoin::address::NetworkUnchecked;
use bitcoin::hashes::sha256::Hash as Sha256Hash;
use bitcoin::hashes::Hash;
use bitcoin::Address;
use bitcoin::Amount;
Expand Down Expand Up @@ -1381,3 +1383,69 @@ fn facade_logging() {
validate_log_entry(entry);
}
}

#[test]
fn spontaneous_send_with_custom_preimage() {
let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();
let chain_source = TestChainSource::Esplora(&electrsd);
let (node_a, node_b) = setup_two_nodes(&chain_source, false, true, false);

let address_a = node_a.onchain_payment().new_address().unwrap();
let premine_sat = 1_000_000;
premine_and_distribute_funds(
&bitcoind.client,
&electrsd.client,
vec![address_a],
Amount::from_sat(premine_sat),
);
node_a.sync_wallets().unwrap();
node_b.sync_wallets().unwrap();
open_channel(&node_a, &node_b, 500_000, true, &electrsd);
generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6);
node_a.sync_wallets().unwrap();
node_b.sync_wallets().unwrap();
expect_channel_ready_event!(node_a, node_b.node_id());
expect_channel_ready_event!(node_b, node_a.node_id());

let seed = b"test_payment_preimage";
let bytes: Sha256Hash = Sha256Hash::hash(seed);
let custom_bytes = bytes.to_byte_array();
let custom_preimage = PaymentPreimage(custom_bytes);

let amount_msat = 100_000;
let payment_id = node_a
.spontaneous_payment()
.send_with_preimage(amount_msat, node_b.node_id(), None, custom_preimage.clone())
.unwrap();

// check payment status and verify stored preimage
expect_payment_successful_event!(node_a, Some(payment_id), None);
let details: PaymentDetails =
node_a.list_payments_with_filter(|p| p.id == payment_id).first().unwrap().clone();
assert_eq!(details.status, PaymentStatus::Succeeded);
if let PaymentKind::Spontaneous { preimage: Some(pi), .. } = details.kind {
assert_eq!(pi.0, custom_bytes);
} else {
panic!("Expected a spontaneous PaymentKind with a preimage");
}

// Verify receiver side (node_b)
expect_payment_received_event!(node_b, amount_msat);
let receiver_payments: Vec<PaymentDetails> = node_b.list_payments_with_filter(|p| {
p.direction == PaymentDirection::Inbound
&& matches!(p.kind, PaymentKind::Spontaneous { .. })
});

assert_eq!(receiver_payments.len(), 1);
let receiver_details = &receiver_payments[0];
assert_eq!(receiver_details.status, PaymentStatus::Succeeded);
assert_eq!(receiver_details.amount_msat, Some(amount_msat));
assert_eq!(receiver_details.direction, PaymentDirection::Inbound);

// Verify receiver also has the same preimage
if let PaymentKind::Spontaneous { preimage: Some(pi), .. } = receiver_details.kind {
assert_eq!(pi.0, custom_bytes);
} else {
panic!("Expected receiver to have spontaneous PaymentKind with preimage");
}
}
Loading