Skip to content

Add an onion message-based DNSSEC HRN Resolver #6

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 3 commits into
base: main
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Member

Doing HRN resolution natively over the normal internet tends to be
horrendous for privacy. One of the main motivations for BIP 353 was
to limit the impacts of this by allowing for easier proxying of DNS
requests.

Here we add one such proxied request, specifically using lightning
onion messages to do the DNS requests.

Copy link

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Could you expand on why switching the dependency order is untennable? Wouldn't it still be possible if the OM-resolution code would live on the LDK side?

@TheBlueMatt
Copy link
Member Author

This crate depends on the LDK Bolt11Invoice (which is already its own crate that lightning depends on) as well as the LDK Bolt12Offer type (which is in the lightning crate). I looked briefly into moving the Bolt12Offer type into a separate crate, but it would imply breaking a lot of the lightning crate up (serialization included, which means a stable API on lots of internal stuff we don't currently expose) and just didn't seem nearly worth the effort. Instead, its easy enough to just let this crate depend on lightning.

@tnull
Copy link

tnull commented Jul 1, 2025

This crate depends on the LDK Bolt11Invoice (which is already its own crate that lightning depends on) as well as the LDK Bolt12Offer type (which is in the lightning crate). I looked briefly into moving the Bolt12Offer type into a separate crate, but it would imply breaking a lot of the lightning crate up (serialization included, which means a stable API on lots of internal stuff we don't currently expose) and just didn't seem nearly worth the effort. Instead, its easy enough to just let this crate depend on lightning.

Hmm, that's a bit unfortunate. I had hoped that we might eventually get around to splitting out BOLT12 types into a dedicated crate, in parallel to lightning-invoice. But I guess that ship has sailed by now..

@tnull
Copy link

tnull commented Jul 1, 2025

@chuksys Mind checking out this PR as part of lightningdevkit/ldk-node#521 as we now decided to handle HRN resolution in LDK Node directly, based on the changes here?

@chuksys
Copy link

chuksys commented Jul 1, 2025

@chuksys Mind checking out this PR as part of lightningdevkit/ldk-node#521 as we now decided to handle HRN resolution in LDK Node directly, based on the changes here?

Sure! On it!

@TheBlueMatt
Copy link
Member Author

Hmm, that's a bit unfortunate. I had hoped that we might eventually get around to splitting out BOLT12 types into a dedicated crate, in parallel to lightning-invoice. But I guess that ship has sailed by now..

Yea, I mean in general I'm not a fan of crate-smashing, but in this case it would have been useful...oh well.

Previously I was hoping we could swap the dependency order between
LDK and `bitcoin-payment-instructions`, but that turned out to be
untennable, so instead we should reuse the LDK `HumanReadableName`.
Doing HRN resolution natively over the normal internet tends to be
horrendous for privacy. One of the main motivations for BIP 353 was
to limit the impacts of this by allowing for easier proxying of DNS
requests.

Here we add one such proxied request, specifically using lightning
onion messages to do the DNS requests.
@TheBlueMatt TheBlueMatt force-pushed the 2025-06-onion-resolution branch from 8264cc1 to 543c909 Compare July 2, 2025 16:30
@TheBlueMatt
Copy link
Member Author

Rebased:

diff --git a/src/onion_message_resolver.rs b/src/onion_message_resolver.rs
index 9e7fa33bc..39f42bbfd 100644
--- a/src/onion_message_resolver.rs
+++ b/src/onion_message_resolver.rs
@@ -243,9 +243,16 @@ where
                }
        }

-       fn resolve_lnurl<'a>(&'a self, _: String, _: Amount, _: [u8; 32]) -> LNURLResolutionFuture<'a> {
-               let err = "resolve_lnurl shouldn't be called when we don't resolve LNURL";
-               debug_assert!(false, "{}", err);
+       fn resolve_lnurl<'a>(&'a self, _url: &'a str) -> HrnResolutionFuture<'a> {
+               let err = "DNS resolver does not support LNURL resolution";
+               Box::pin(async move { Err(err) })
+       }
+
+       fn resolve_lnurl_to_invoice<'a>(
+               &'a self, _: String, _: Amount, _: [u8; 32],
+       ) -> LNURLResolutionFuture<'a> {
+               let err = "resolve_lnurl_to_invoice shouldn't be called when we don't resolve LNURL";
+               debug_assert!(false, "{err}");
                Box::pin(async move { Err(err) })
        }
 }

Copy link

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Code changes look good to me, one question.

let mut dns_resolvers = Vec::new();
for (node_id, node) in self.network_graph.read_only().nodes().unordered_iter() {
if let Some(info) = &node.announcement_info {
// Sadly, 31 nodes currently squat on the DNS Resolver feature bit
Copy link

Choose a reason for hiding this comment

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

:(

(cc @chuksys, we'll need to do the same when we discover resolvers from the network graph).

@TheBlueMatt TheBlueMatt requested a review from tnull July 11, 2025 20:07
@TheBlueMatt TheBlueMatt force-pushed the 2025-06-onion-resolution branch from 2e04f2c to 45e53e2 Compare July 11, 2025 20:09
Copy link

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Fixup looks good, feel free to squash. Two minor questions.

fn poll(self: Pin<&mut Self>, context: &mut Context) -> Poll<HrnResolution> {
let mut state = self.0.lock().unwrap();
if let Some(res) = state.result.take() {
state.waker = None;
Copy link

Choose a reason for hiding this comment

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

Why are we setting this None here? Wouldn't this already be unset by ChannelRecv::complete? Couldn't this rather be a debug_assert to make sure it already is always None here?

let counter = self.next_id.fetch_add(1, Ordering::Relaxed) as u64;
let mut payment_id = [0; 32];
payment_id[..8].copy_from_slice(&counter.to_ne_bytes());
let payment_id = PaymentId(payment_id);
Copy link

Choose a reason for hiding this comment

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

Hmm, just to be clear: even though this is reusing the same type, it's completely decoupled from the LDK-internal payment_id now, no? I wonder if it would make sense to create a separate ResolutionId / RequestId type to reflect this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants