Add support for sending to human-readable names (BIP 353) #528

chuksys · 2025-04-22T00:41:55Z

This PR is a first step to add support for human-readable names as outlined in BIP 353 and implemented in LDK 0.1.

Changes

A send_to_human_readable_name function was added under Bolt12Payment.
A HrnBolt12Offer PaymentKind variant was added.
A HrnParsingFailed Error variant was added.
These new enum variants were added to the uniffi bindings.

I plan to follow up with a PR for #436 and another PR that will add HRN sending for on-chain payments.

ldk-reviews-bot · 2025-04-22T00:41:58Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

chuksys · 2025-04-22T02:27:17Z

src/payment/bolt12.rs

+	pub fn send_to_human_readable_name(
+		&self, name: &str, amount_msat: u64, dns_resolvers: Vec<Destination>,
+	) -> Result<PaymentId, Error> {


I should probably expose this method to uniffi bindings?

Also, for dns_resolvers maybe I should add a method in graph.rs which helps find all nodes that have a certain feature flag. This could make it simpler for the user to find all dns_resolver nodes.

I should probably expose this method to uniffi bindings?

Yes, you'll need to expose this method and all types it requires.

Also, for dns_resolvers maybe I should add a method in graph.rs which helps find all nodes that have a certain feature flag. This could make it simpler for the user to find all dns_resolver nodes.

Yes, populating the list of default Destinations (see above) from the network graph is probably a good idea.

ldk-reviews-bot · 2025-04-24T17:07:28Z

🔔 1st Reminder

Hey @tnull! 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-04-28T00:00:09Z

🔔 2nd Reminder

Hey @tnull! 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.

tnull

Thanks for looking into this, and please excuse the delay here!

tnull · 2025-04-29T09:31:46Z

src/payment/store.rs

+	/// [HumanReadableName]: crate::lightning::onion_message::dns_resolution::HumanReadableName
+	/// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki
+	/// [bLIP 32]: https://github.com/lightning/blips/blob/master/blip-0032.md
+	HrnBolt12Offer {


Hmm, I'm not sure we want to add a separate PaymentKind variant for this. Likely it should just be a Bolt12Offer? In the upcoming 'payment metadata store' (see #425), we might then store the HRN as associated metadata though.

tnull · 2025-04-29T09:35:04Z

src/payment/bolt12.rs

+	/// These nodes can be found by running a search through the `NetworkGraph` to find nodes that announce the
+	/// `dns_resolver` feature flag.
+	pub fn send_to_human_readable_name(
+		&self, name: &str, amount_msat: u64, dns_resolvers: Vec<Destination>,


I think rather then replicating LDK's API and have the user give the Destination for each send, in LDK Node we should probably allow users to set default DNS resolvers via the Builder and/or the Config. Here, we'd then error-out if the user didn't configure them upfront, and in the future we might be able to supply some default values.

tnull · 2025-04-29T09:36:43Z

src/payment/bolt12.rs

+	pub fn send_to_human_readable_name(
+		&self, name: &str, amount_msat: u64, dns_resolvers: Vec<Destination>,
+	) -> Result<PaymentId, Error> {


I should probably expose this method to uniffi bindings?

Yes, you'll need to expose this method and all types it requires.

Also, for dns_resolvers maybe I should add a method in graph.rs which helps find all nodes that have a certain feature flag. This could make it simpler for the user to find all dns_resolver nodes.

Yes, populating the list of default Destinations (see above) from the network graph is probably a good idea.

tnull · 2025-04-29T09:43:07Z

src/uniffi_types.rs

@@ -429,6 +430,18 @@ impl UniffiCustomTypeConverter for DateTime {
 	}
 }

+impl UniffiCustomTypeConverter for HumanReadableName {


Let's avoid introducing yet another UniffiCustomTypeConverter for this, as we're actually currently working towards removing them for Bolt11Invoice, Offer, etc.

So we should just expose the HumanReadableName as a Uniffi interface, and if we find its API can't fully exposed, we'll need to create a type wrapper in uniffi_types.rs for it that is switched based on the uniffi flag (see #522 for reference).

chuksys · 2025-04-30T17:42:52Z

Thank you for the feedback @tnull. I will address the comments and push the updated changes as soon as possible.

sangbida · 2025-05-05T12:13:28Z

Had a quick look at why some of the CI tests could be failing. I think the cln test is potentially failing because of the dnssec addition. The test env may need to be configured to support dnssec?

EDIT: Nevermind, I think this is the CLN issue: #527 The test seems to be flaky.

chuksys · 2025-05-10T09:41:15Z

I will address the comments and push the updated changes as soon as possible.

Still actively working on this. Will push updates once I'm done (likely within the next week).

tnull · 2025-05-12T08:15:03Z

Still actively working on this. Will push updates once I'm done (likely within the next week).

Cool! Also don't hesitate to reach out (e.g. via LDK Discord -- I'm _tnull there) if you have any questions and/or are blocked on a particular issue! Happy to help!

chuksys · 2025-05-12T13:18:51Z

Also don't hesitate to reach out (e.g. via LDK Discord -- I'm _tnull there) if you have any questions and/or are blocked on a particular issue! Happy to help!

Thank you!

chuksys · 2025-05-13T20:12:37Z

Hi @tnull, thanks once again for the pointer you gave me in our discord chat. I just pushed some updates that allow the user to set a list of default node_ids to use as the Destination for HRNs dns resolution. As advised, I would follow up with another PR later that would fully expose Destination from LDK to uniFFI bindings and allow using BlindedPaths as the Destination for HRNs dns resolution. I am yet to implement the method which allows the user to easily fetch dns_resolver nodes from the network_graph - I will work on that next and push when done or perhaps open a new PR for that. Thanks!

tnull

Already looks pretty good, some comments

tnull · 2025-05-15T11:05:00Z

src/builder.rs

@@ -457,6 +462,17 @@ impl NodeBuilder {
 		Ok(self)
 	}

+	/// Sets the default dns_resolvers to be used when sending payments to HRNs.


Can we link to the actual config field here (and below accordingly)?

tnull · 2025-05-15T11:06:32Z

src/config.rs

@@ -167,6 +168,11 @@ pub struct Config {
 	/// **Note:** If unset, default parameters will be used, and you will be able to override the
 	/// parameters on a per-payment basis in the corresponding method calls.
 	pub sending_parameters: Option<SendingParameters>,
+	/// The dns_resolvers node_ids to be used for resolving Human-readable Names.


nit: Let's avoid using non-local variable names like dns_resolvers and node_ids. Instead, just say "The DNS resolvers.."

tnull · 2025-05-15T11:06:46Z

src/config.rs

+	/// The dns_resolvers node_ids to be used for resolving Human-readable Names.
+	///
+	/// If set to `Some`, the values set will be used as dns_resolvers when sending to HRNs.
+	/// **Note:** If set to `None`, payments to HRNs will fail.


nit: Lets put this note in a dedicated paragraph.

tnull · 2025-05-15T11:07:59Z

src/config.rs

+	///
+	/// If set to `Some`, the values set will be used as dns_resolvers when sending to HRNs.
+	/// **Note:** If set to `None`, payments to HRNs will fail.
+	pub dns_resolvers_node_ids: Option<Vec<PublicKey>>,


Why is this an option rather than just having a Vec? Is this so that in the future it can more easily be converted to an override value, once we discover default resolvers via the network graph?

Also, should we maybe only add this as part of an new HumanReadableNamesConfig object, as we may be adding addtional configs in the future?

Also, should we maybe only add this as part of an new HumanReadableNamesConfig object, as we may be adding addtional configs in the future?

I think this is a good idea. I'll work on this.

tnull · 2025-05-15T11:19:02Z

src/lib.rs

@@ -829,6 +829,11 @@ impl Node {
 		self.config.node_alias
 	}

+	/// Returns the list of dns_resolvers that will be used to resolve HRNs.
+	pub fn dns_resolvers(&self) -> Option<Vec<PublicKey>> {


No need for this extra accessor, users can just access the config.

tnull · 2025-05-15T11:29:51Z

src/payment/bolt12.rs

+		}?;
+
+		let destinations: Vec<Destination> =
+			dns_resolvers.into_iter().map(|public_key| Destination::Node(public_key)).collect();


Let's do this directly in the match above, so we don't have to first clone and then collect again, which is an unnecessary allocation.

src/payment/bolt12.rs

tnull · 2025-05-15T11:54:40Z

src/payment/bolt12.rs

+	/// amount paid to be determined by the user.
+	///
+	/// If `dns_resolvers_node_ids` in Config is set to `None`, this operation will fail.
+	pub fn send_to_human_readable_name(


Do you think we can add a test case to check that this works somehow? Or would we need to wait for #436 to be able to create an end-to-end test?

If we can intercept the InvoiceReceived event or have an event that is emitted after the offer is retrieved, I think we can have at least a test case built around any of these events.

I think the end-to-end test would be the ideal, eventually.

tnull

Did another ~high-level-ish round of review, just commenting a few things that stood out to me.

tnull · 2025-05-29T09:41:20Z

src/builder.rs

@@ -459,6 +465,12 @@ impl NodeBuilder {
 		Ok(self)
 	}

+	/// Sets the default [`Config::hrn_config`] to be used when sending payments to HRNs.


I don't think we need to add this setter, let's drop it. Note we're not adding them for all Config values.

tnull · 2025-05-29T09:43:53Z

src/builder.rs

@@ -174,6 +175,8 @@ pub enum BuildError {
 	LoggerSetupFailed,
 	/// The given network does not match the node's previously configured network.
 	NetworkMismatch,
+	/// The dns_resolvers list provided for HRN resolution is empty


I think dns_resolvers list is outdated now. Let's link to the particular field in HumanReadableNamesConfig.

tnull · 2025-05-29T09:44:17Z

src/config.rs

@@ -103,6 +103,7 @@ pub const WALLET_KEYS_SEED_LEN: usize = 64;
 /// | `log_level`                            | Debug              |
 /// | `anchor_channels_config`               | Some(..)           |
 /// | `sending_parameters`                   | None               |
+/// | `dns_resolvers`                        | None               |


This is outdated now.

tnull · 2025-05-29T09:45:11Z

src/config.rs

@@ -167,6 +168,8 @@ pub struct Config {
 	/// **Note:** If unset, default parameters will be used, and you will be able to override the
 	/// parameters on a per-payment basis in the corresponding method calls.
 	pub sending_parameters: Option<SendingParameters>,
+	/// Configuration Options for Human-readable Names.


nit: No need to use full title case here.

Suggested change

/// Configuration Options for Human-readable Names.

/// Configuration options for Human-Readable Names (BIP 353).

Could also link to the BIP here and elsewhere.

tnull · 2025-05-29T09:59:53Z

src/config.rs

@@ -167,6 +168,8 @@ pub struct Config {
 	/// **Note:** If unset, default parameters will be used, and you will be able to override the
 	/// parameters on a per-payment basis in the corresponding method calls.
 	pub sending_parameters: Option<SendingParameters>,
+	/// Configuration Options for Human-readable Names.
+	pub hrn_config: HumanReadableNamesConfig,


Can we make this optional, defaulting to None for now? I think rather than only failing at runtime, we should refuse to startup with DnsResolversUnavailable on misconfiguration, i.e., if the user would enable it but won't supply DNS resolvers.

tnull · 2025-05-29T10:00:14Z

src/error.rs

+	/// Parsing a Human-Readable Name has failed
+	HrnParsingFailed,
+	/// The given operation failed due to `dns-resolvers` not being configured in builder.
+	DnsResolversNotConfigured,


Let's rename this DnsResolversUnavailable to mirror what we do for LiquiditySource.

tnull · 2025-05-29T10:04:04Z

src/uniffi_types.rs

+			Err(e) => Err(format!("Error creating HRN {:?}", e)),
+		};
+
+		Self { inner: hrn.expect("Error creating HRN") }


Please never panic/expect/unwrap in production code.

ldk-reviews-bot · 2025-06-02T00:00:18Z

🔔 1st Reminder

Hey @tnull! 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.

tnull · 2025-06-02T11:20:38Z

src/config.rs

 		}
 	}
 }

+/// Configuration options for Human-readable Names


Let's link to BIP353 here, too. Also, nit, let's stick with Human-Readable Names, i.e., consistent capitalization everywhere.

tnull · 2025-06-02T11:22:38Z

src/error.rs

+				write!(f, "Failed to parse a human-readable name.")
+			},
+			Self::DnsResolversUnavailable => {
+				write!(f, "The given operation failed due to `dns-resolvers` not being configured in builder.")


Suggested change

write!(f, "The given operation failed due to `dns-resolvers` not being configured in builder.")

write!(f, "The given operation failed due to DNS resolvers not being configured.")

src/payment/bolt12.rs

tnull · 2025-06-02T11:26:55Z

src/payment/store.rs

@@ -404,7 +404,7 @@ pub enum PaymentKind {
 		/// The secret used by the payment.
 		secret: Option<PaymentSecret>,
 		/// The ID of the offer this payment is for.
-		offer_id: OfferId,
+		offer_id: Option<OfferId>,


Please describe when this field would be set / unset.

tnull · 2025-06-02T11:29:18Z

src/event.rs

@@ -1417,8 +1417,20 @@ where
 					);
 				}
 			},
-			LdkEvent::InvoiceReceived { .. } => {
-				debug_assert!(false, "We currently don't handle BOLT12 invoices manually, so this event should never be emitted.");
+			LdkEvent::InvoiceReceived { payment_id, invoice, context: _, responder: _ } => {


I don't think we'd ever actually see this emitted (see the previous debug_assert here), if we don't set manually_handle_bolt12_invoices. And if we do, we really need to handle this even properly, not only to update our PaymentStore.

tnull · 2025-06-03T11:23:56Z

src/payment/bolt12.rs

+
+#[cfg(not(feature = "uniffi"))]
+pub fn maybe_convert_hrn(hrn: HumanReadableName) -> LdkHumanReadableName {
+	hrn.clone()


This clone can be dropped.

tnull · 2025-06-03T11:29:35Z

src/payment/bolt12.rs

+			destinations,
+		) {
+			Ok(()) => {
+				log_info!(self.logger, "Initiated sending {} msats to {:?}", amount_msat, hrn);


Ah, let's print the hrn as ... ₿{}@{}", hrn.user(), hrn.doman() for now, but I think we should implement Display for this upstream. Opened an issue for it: lightningdevkit/rust-lightning#3823

chuksys · 2025-06-06T08:57:18Z

src/event.rs

+				let _ = self
+					.channel_manager
+					.send_payment_for_bolt12_invoice(&invoice, context.as_ref());


I didn't handle the error here because I'm not sure if we want to return a ReplayEvent Error when we get a Bolt12PaymentError::UnexpectedInvoice or Bolt12PaymentError::DuplicateInvoice - Plus since the success or failure of the payment, triggers Event::PaymentSent or Event::PaymentFailed (which are already properly handled), I'm thinking this might be the way to go. Please let me know what you think.

Hmm, true, but we should at the very least log the error here I think.

ldk-reviews-bot · 2025-06-09T00:00:20Z

🔔 1st Reminder

Hey @tnull! 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.

tnull

This needs a rebase now that #542 landed.

tnull

Seems there are still a few rebase issues.

Feel free to squash fixups. Btw, going forward it would be helpful to provide actual commit messages behind the fixup! prefix rather than just mirroring the feature commit's headline.

tnull · 2025-06-11T12:25:42Z

src/payment/bolt12.rs

@@ -41,6 +43,18 @@ type Offer = Arc<crate::ffi::Offer>;
 type Refund = lightning::offers::refund::Refund;
 #[cfg(feature = "uniffi")]
 type Refund = Arc<crate::ffi::Refund>;
+type HumanReadableName = LdkHumanReadableName;


This seems to be missing a newline and #[cfg(not(feature = "uniffi"))] ?

tnull · 2025-06-11T12:26:59Z

src/payment/bolt12.rs

+type HumanReadableName = Arc<crate::uniffi_types::HumanReadableName>;
+
+#[cfg(not(feature = "uniffi"))]
+pub fn maybe_convert_hrn(hrn: HumanReadableName) -> LdkHumanReadableName {


Please reuse the generalized maybe_* methods now moved to ffi/mod.rs.

tnull · 2025-06-11T12:28:07Z

src/uniffi_types.rs

This file was renamed as part of #542, please don't reintroduce it here but rather make the required changes to ffi/types.rs.

chuksys · 2025-06-12T14:15:10Z

Seems there are still a few rebase issues.

Feel free to squash fixups. Btw, going forward it would be helpful to provide actual commit messages behind the fixup! prefix rather than just mirroring the feature commit's headline.

Thanks for the feedback. I've fixed the rebase issues. Going forward, I will ensure to provide commit messages for fixups.

ldk-reviews-bot · 2025-06-14T14:27:49Z

🔔 1st Reminder

Hey @tnull! 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-06-16T14:28:00Z

🔔 2nd Reminder

Hey @tnull! 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.

tnull

Excuse the delay here. I think this mostly looks good, however, I want to hold of landing this until lightningdevkit/rust-lightning#3779 has concluded, as it seems we're considering actually removing the HRN resolution part (and hence the APIs) from LDK' ChannelManager, in which case resolution would need to be handled here and we'd otherwise just use the BOLT12 flow if it resolves to an offer.

tnull · 2025-06-18T10:48:14Z

bindings/ldk_node.udl

@@ -854,4 +871,4 @@ typedef string NodeAlias;
 typedef string OrderId;

 [Custom]
-typedef string DateTime;
+typedef string DateTime;


nit: Not sure why this change here was included. Please revert.

tnull · 2025-06-18T10:51:05Z

src/event.rs

+				let _ = self
+					.channel_manager
+					.send_payment_for_bolt12_invoice(&invoice, context.as_ref());


Hmm, true, but we should at the very least log the error here I think.

…2 Offers BIP 353 introduced human-readable names which has been implemented in LDK (0.1). This commit adds support for sending to HRNs that resolve to Bolt12 Offers by using the LDK pay_for_offer_from_human_readable_name method in ChannelManager.

ldk-reviews-bot requested a review from jkczyz April 22, 2025 00:52

chuksys commented Apr 22, 2025

View reviewed changes

tnull requested review from tnull and removed request for jkczyz April 22, 2025 17:06

tnull reviewed Apr 29, 2025

View reviewed changes

TheBlueMatt mentioned this pull request May 1, 2025

Add new store for inbound payment metadata #425

Open

chuksys force-pushed the send-to-human-readable-names branch from 386f95f to 2e75be9 Compare May 13, 2025 18:31

tnull reviewed May 15, 2025

View reviewed changes

Camillarhi mentioned this pull request May 25, 2025

Switch to use bitcoin-payment-instructions #521

Open

chuksys force-pushed the send-to-human-readable-names branch from 8bf2184 to 0a9379a Compare May 28, 2025 12:42

chuksys requested a review from tnull May 29, 2025 08:49

tnull reviewed May 29, 2025

View reviewed changes

chuksys requested a review from tnull May 30, 2025 20:05

tnull reviewed Jun 2, 2025

View reviewed changes

ConorOkus mentioned this pull request Jun 2, 2025

BOLT 12 / BIP 353 reez/Monday#205

Open

tnull reviewed Jun 3, 2025

View reviewed changes

This was referenced Jun 4, 2025

Populate and set Default DNS Resolvers in Config from network graph #553

Open

Add end-to-end test for HRNs payments #555

Open

chuksys requested a review from tnull June 5, 2025 23:01

chuksys commented Jun 6, 2025

View reviewed changes

tnull reviewed Jun 10, 2025

View reviewed changes

chuksys force-pushed the send-to-human-readable-names branch from 75833b4 to eed246c Compare June 11, 2025 12:10

tnull reviewed Jun 11, 2025

View reviewed changes

chuksys force-pushed the send-to-human-readable-names branch from eed246c to 23207bc Compare June 12, 2025 14:11

chuksys requested a review from tnull June 12, 2025 14:26

tnull reviewed Jun 18, 2025

View reviewed changes

chuksys force-pushed the send-to-human-readable-names branch from 23207bc to 9e7b889 Compare June 20, 2025 17:50

chuksys added 2 commits June 20, 2025 19:57

Allow setting default DNS resolvers for HRNs

6068bf5

chuksys force-pushed the send-to-human-readable-names branch from 9e7b889 to 6068bf5 Compare June 20, 2025 18:58

	/// Configuration Options for Human-readable Names.
	/// Configuration options for Human-Readable Names (BIP 353).

	write!(f, "The given operation failed due to `dns-resolvers` not being configured in builder.")
	write!(f, "The given operation failed due to DNS resolvers not being configured.")

Add support for sending to human-readable names (BIP 353) #528

Are you sure you want to change the base?

Add support for sending to human-readable names (BIP 353) #528

Uh oh!

Conversation

chuksys commented Apr 22, 2025

Changes

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Apr 24, 2025

Uh oh!

ldk-reviews-bot commented Apr 28, 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!

Choose a reason for hiding this comment

Uh oh!

chuksys commented Apr 30, 2025

Uh oh!

sangbida commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chuksys commented May 10, 2025

Uh oh!

tnull commented May 12, 2025

Uh oh!

chuksys commented May 12, 2025

Uh oh!

chuksys commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Uh oh!

Uh oh!

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!

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

ldk-reviews-bot commented Apr 22, 2025 •

edited

Loading

sangbida commented May 5, 2025 •

edited

Loading

chuksys commented May 13, 2025 •

edited

Loading

chuksys Jun 6, 2025 •

edited

Loading