-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add an onion message-based DNSSEC HRN Resolver #6
Conversation
TheBlueMatt
commented
Jun 29, 2025
8258114
to
8264cc1
Compare
There was a problem hiding this 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?
This crate depends on the LDK |
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 |
@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! |
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`.
8264cc1
to
543c909
Compare
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) })
}
} |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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).
2e04f2c
to
45e53e2
Compare
There was a problem hiding this 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.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, its just a counter here. We could move it to a generic [u8; 32]
upstream, but it is still kinda a "payment id", we want to make a payment (that's why we're resolving an HRN!) and we are assigning an ID for that payment :)
@TheBlueMatt gentle ping here |
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.
45e53e2
to
dc2b29a
Compare
Oops, sorry. Squashed and moved to a debug assertion: $ git diff-tree -U1 45e53e2 dc2b29a
diff --git a/src/onion_message_resolver.rs b/src/onion_message_resolver.rs
index 136af218f..5bdc7ab5e 100644
--- a/src/onion_message_resolver.rs
+++ b/src/onion_message_resolver.rs
@@ -67,3 +67,3 @@ impl Future for ChannelRecv {
if let Some(res) = state.result.take() {
- state.waker = None;
+ debug_assert!(state.waker.is_none());
Poll::Ready(res) |