Support paying directly to Human Readable Names using bLIP 32 #3283

TheBlueMatt · 2024-08-30T21:24:10Z

Based on #3179 this fully implements BIP 353 sends using bLIP 32 DNS resolution with a wonderfully simple API:

        pub fn pay_for_offer_from_human_readable_name(
                &self, name: HumanReadableName, amount_msats: u64, payment_id: PaymentId,
                retry_strategy: Retry, max_total_routing_fee_msat: Option<u64>,
                dns_resolvers: Vec<Destination>,
        ) -> Result<(), ()> {

Still needs a few trivial things piped out through to events (eg DNSSEC proofs as "proof of payment"), but its basically done!

codecov · 2024-08-30T21:31:24Z

Codecov Report

Attention: Patch coverage is 85.27132% with 76 lines in your changes missing coverage. Please review.

Project coverage is 89.67%. Comparing base (b0bd437) to head (cbdb8cb).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-dns-resolver/src/lib.rs	86.77%	34 Missing and 5 partials ⚠️
lightning/src/ln/channelmanager.rs	79.27%	16 Missing and 7 partials ⚠️
lightning/src/ln/outbound_payment.rs	83.60%	9 Missing and 1 partial ⚠️
lightning/src/onion_message/dns_resolution.rs	40.00%	1 Missing and 2 partials ⚠️
lightning/src/offers/refund.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3283      +/-   ##
==========================================
+ Coverage   89.65%   89.67%   +0.02%     
==========================================
  Files         128      129       +1     
  Lines      104821   105419     +598     
  Branches   104821   105419     +598     
==========================================
+ Hits        93980    94538     +558     
- Misses       8130     8138       +8     
- Partials     2711     2743      +32

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt · 2024-10-02T17:20:49Z

Should be ready for review now that the dependent PR landed @arik-so @tnull

arik-so

Just noticed I had this comment pending without submitting it, sorry.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2024-10-08T19:48:34Z

Added a commit to set the feature bit added in #3346.

TheBlueMatt · 2024-10-28T19:01:00Z

Rebased and addressed CI issues.

arik-so · 2024-10-29T17:57:24Z

I'm good with squashing the fixups at this point.

TheBlueMatt · 2024-10-29T18:07:44Z

Squashed the fixups without further changes.

arik-so

Nit aside, code looks good. I think the most critical portion is in the dnssec_prover crate anyway, which has already been reviewed.

lightning/src/ln/channelmanager.rs

vincenzopalazzo · 2024-11-05T09:09:16Z

lightning/src/ln/outbound_payment.rs

+	(11, AwaitingOffer) => {
+		(0, expiration, required),
+		(2, retry_strategy, required),
+		(4, max_total_routing_fee_msat, option),
+		(6, amount_msats, required),
+	},


just trying to learn here. I noted this in other places too. There is a specific reason that we use the number with a step of 2? is it probably for upgrade and downgrade?

For new types not really, no. Even/odd means future versions that don't support these fields will fail/ignore the fields, but for new fields it doesn't matter too much since new versions will trivially support them.

lightning/src/ln/channelmanager.rs

vincenzopalazzo

LGTM, I left some question/light-suggestion long the way

tnull

Did a first pass, some questions/nits.

tnull · 2024-11-06T14:07:10Z

lightning/src/ln/outbound_payment.rs

+			match payment.get() {
+				PendingOutboundPayment::Abandoned { payment_hash, reason, .. } => {
+					if payment.get().remaining_parts() == 0 {
+						pending_events.lock().unwrap().push_back((events::Event::PaymentFailed {


nit: given that rustfmt will come around here eventually, might already want to pull out the event into a single variable while we're here.

Only the indentation here changed (git show -b is happy) :)

tnull · 2024-11-06T14:10:35Z

lightning/src/offers/invoice.rs

@@ -1613,6 +1613,7 @@ mod tests {
 					payer_id: Some(&payer_pubkey()),
 					payer_note: None,
 					paths: None,
+					source_human_readable_name: None,


I'm confused: given the context described in the commit message, shouldn't this rather be called target_ or destination_readable_name? Or maybe I'm misunderstanding what source is referring to? Isn't this describing which HRN we're trying to pay to?

Or maybe just bip_353_name, given that lightning/bolts#1180 calls the field invreq_bip_353_name?

Mmm, I was saying source as in "the place I got the offer I'm paying is..." The setter method on the invoicereq is sourced_from_human_readable_name so I lined the getter up with that. The internal field I'm just calling the same as the public API for convenience. We could rename it as "destination", I suppose, but it does feel strange cause the "destination" is the BOLT 12.

Mhh, maybe target then, or receiver? (lol, it's always the same naming discussion around discerning immediate and 'final' destinations, always tricky).

Not sure if I'm alone in this but I read source_ as the exact opposite of what it's supposed to mean.

Maybe just from_human_readable_name? I don't want to imply that this is the target's unique HRN, nor even that the target is aware that this is their HRN.

Or derived_from_hrn?

Either from_human_readable_name or derived_from_hrn would be an improvement from my perspective, also like hrn_offer_derived_from you proposed elsewhere.

I'll leave it up to you 🤷‍♂️

Went with offer_from_hrn as it captures the bool-ness of the field, doesn't imply that the offer was somehow "derived" (vs just contained in the hrn) and focuses on the offer, not the struct that the method is being called on.

lightning/src/ln/channelmanager.rs

tnull · 2024-11-06T14:22:18Z

lightning/src/ln/channelmanager.rs

+		let (onion_message, context) =
+			self.hrn_resolver.resolve_name(payment_id, name, &*self.entropy_source)?;
+		let reply_paths = self.create_blinded_paths(MessageContext::DNSResolver(context))?;
+		let expiration = StaleExpiration::TimerTicks(2);


Why 2 here? Why not also 1 or 3 or 5? Also, nit: could you move these to consts?

2 is just the "minimum" value for any tick-based expiration. 1 doesn't work cause the next tick might be in 1ms, so then we'd fail always. 2 is the next number. I don't think this needs to be configurable/changeable, its just "minimum possible value".

1 doesn't work cause the next tick might be in 1ms, so then we'd fail always.

Wait, why do we have this in pay_for_offer then?:

let expiration = StaleExpiration::TimerTicks(1);

Ah, it keeps it for one more tick after 0....honestly its always one tick so I'm gonna remove the parameter and just move the logic to outbound_payment.rs.

Oops, it is actually used, anyway, fixed to 1.

lightning-dns-resolver/Cargo.toml

lightning-dns-resolver/src/lib.rs

tnull · 2024-11-06T14:39:08Z

lightning-dns-resolver/src/lib.rs

+const MAX_PENDING_RESPONSES: usize = 1024;
+struct OMResolverState {
+	resolver: SocketAddr,
+	pending_replies: Mutex<Vec<(DNSResolverMessage, MessageSendInstructions)>>,


Why not use channels for this?

You mean like MPSC channels? Not quite sure I understand the suggestion here.

Yes, sorry, 'channels' might have been a bit ambiguous here... I was thinking about tokio::sync::oneshot mostly as they seem the right tool for the job and are also know to work nicely in async contexts/stacks. They'd would also be nice as message passing wouldn't risk introducing any deadlocks when accessing the the message handler (an issue I ran into previously with LiquidityManager, for example. That said, I guess to go that route properly we'd need to make DNSResolverMessageHandler async in one form or another, which probably isn't feasible right now.

I mean this basically is a one-shot channel, just not one that supports awaiting on it....Given we want to return all the messages back at once when we get a release_pending_messages call using channels per-query would result in a lot of polling which would be much worse than just a global vec.

TheBlueMatt · 2024-11-07T15:56:58Z

Addressed feedback and rebased.

tnull

Feel free to squash fixups

tnull · 2024-11-08T13:47:56Z

lightning/src/ln/outbound_payment.rs

+						payment.remove();
+					}
+				},
+				PendingOutboundPayment::AwaitingInvoice { .. }


I'm a bit confused by this: why can't we have mark_abandoned transition these states to Abandoned, too?

Seems a bit strange to first not actually mark them abandoned and then here match on the state even though you'd think they'd always be Abandoned post-mark_abandoned?

We use the abandoned state to keep track of payments which we don't intend to retry but which have some HTLCs pending (implying we cannot yet say Event::PaymentFailed). We remove the payment once all pending HTLCs have been failed.

tnull · 2024-11-08T13:51:04Z

lightning/src/offers/invoice.rs

@@ -1613,6 +1613,7 @@ mod tests {
 					payer_id: Some(&payer_pubkey()),
 					payer_note: None,
 					paths: None,
+					source_human_readable_name: None,


Mhh, maybe target then, or receiver? (lol, it's always the same naming discussion around discerning immediate and 'final' destinations, always tricky).

Not sure if I'm alone in this but I read source_ as the exact opposite of what it's supposed to mean.

TheBlueMatt · 2024-11-08T14:52:44Z

Squashed without further changes.

tnull

~~LGTM~~, mod the pending name change for source_human_readable_name.

(See comment below)

tnull · 2024-11-11T11:39:10Z

lightning-dns-resolver/src/lib.rs

+	/// underlying handler. This is useful when this resolver is handling incoming resolution
+	/// requests but some other handler is making proof requests of remote nodes and wants to get
+	/// results.
+	pub fn with_runtime(


Hmm, sorry for bringing this up just now, but thinking about it again it seems that this approach unfortunately won't work for LDK Node as is since it would require that OMDomainResolver is setup after ChannelManager but before MessageHandler that we'd pass to PeerManager.

However, at the time all this is initialized in LDK Node we don't have the runtime available, which we only setup on Node::start. I think this could be resolved in two ways: either we'd go the FutureSpawner approach as mentioned above, which would allow LDK Node to implement as needed based on another wrapper, or we could make this an Arc<RwLock<Option<Runtime>>> and allowing to set the runtime some time after initialization.

The dnssec-resolver resolving logic relies on tokio, so I don't think we should switch to a generic FutureSpawner as it gives the (wrong) impression that you can use a different runtime. I'll just store the runtime handle in a mutex...even though that's really gross :/

Of course I assume ldk-node won't actually use this in normal mobile setups, those obviously shouldn't be doing resolutions for others...ldk-node in a server setup tho probably maybe should.

Yes, surely it wouldn’t be feasible/necessary for a mobile node, but for any 24/7 LDK Node deployment. So would be great if we found a way to make it work!

See the latest push, I think it should work for that.

tnull

LGTM, feel free to squash.

Domain names implicitly have a trailing `.`, which we require in bLIP 32 but generally shouldn't be exposing to the user in `HumanReadableName`s (after all, they're human-readable). Here we make sure the trailing `.` is dropped in `HumanReadableName`s before we re-add them when building the bLIP 32 messages.

When we resolve a Human Readable Name to a BOLT 12 `offer`, we may end up resolving to a wildcard DNS name covering all possible `user` parts. In that case, if we just blindly pay the `offer`, the recipient would have no way to tell which `user` we paid. Instead, BOLT 12 defines a field to include the HRN resolved in the `invoice_request`, which we implement here. We also take this opportunity to remove constant parameters from the `outbound_payment.rs` interface to `channelmanager.rs`

When we receive a payment to an offer we issued resolved with a human readable name, it may have been resolved using a wildcard DNS entry which we want to map to a specific recipient account locally. To do this, we need the human readable name from the `InvoiceRequest` in the `PaymentClaim{able,ed}`, which we pipe through here using `InvoiceRequestFields`.

Now that we have the ability to resolve BIP 353 Human Readable Names directly and have tracking for outbound payments waiting on an offer resolution, we can implement full BIP 353 support in `ChannelManager`. Users will need one or more known nodes which offer DNS resolution service over onion messages using bLIP 32, which they pass to `ChannelManager::pay_for_offer_from_human_readable_name`, as well as the `HumanReadableName` itself. From there, `ChannelManager` asks the DNS resolver to provide a DNSSEC proof, which it verifies, parses into an `Offer`, and then pays. For those who wish to support on-chain fallbacks, sadly, this will not work, and they'll still have to use `OMNameResolver` directly in order to use their existing `bitcoin:` URI parsing.

Now that `ChannelManager` supports using bLIP 32 to resolve BIP 353 Human Readable Names we should encourage users to use that feature by making the "default" (in various type aliases) to use `ChannelManager` as the `DNSResolverMessageHandler`.

When a lightning node wishes to send payments to a BIP 353 human readable name (using BOLT 12), it first has to resolve that name to a DNS TXT record. bLIP 32 defines a way to do so over onion messages, and this completes our implementation thereof by adding the server side. It operates by simply accepting new messages and spawning tokio tasks to do DNS lookups using the `dnsse_prover` crate. It also contains full end-to-end tests of the BIP 353 -> BOLT 12 -> payment logic using the new server code to do the resolution. Note that because we now have a workspace crate which sets the "lightning/dnssec" feature in its `dev-dependencies`, a naive `cargo test` will test the "dnssec" feature.

`OMDomainResolver` actually does support building DNSSECProofs, so should be setting the `dns_resolution` `NodeFeature` flag.

TheBlueMatt · 2024-11-12T15:54:29Z

Squashed without changes then had to rebase to address a conflict:

$ git range-diff 8c086c772f2f4def5aecc549b2d39f2ab427081d..ce3b143c314ffb640dbdb569dd6c8882e008a0f3 b0bd4371d989722aa38ddfc2f35db8922e3304bb..cbdb8cb0cb5f1e8b9e676ad2d130b8bbc2b9e640
1:  3fdc51c8b = 1:  a39e274e3 Skip the implicit trailing `.` in `HumanReadableName`'s domain
2:  126f7f455 = 2:  46df35b0f Add a new `AwaitingOffer` outbound payment state for BIP 353
3:  84887e686 = 3:  e447b4913 Add support for storing a source HRN in BOLT 12 `invoice_request`s
4:  6aa1bb2bc = 4:  8d8416b95 Store the source `HumanReadableName` in `InvoiceRequestFields`
5:  10eb7cf8a ! 5:  99d00930a Support paying Human Readable Names directly from `ChannelManager`
    @@ lightning/src/ln/channelmanager.rs: where
        /// to pay us.
        ///
     @@ lightning/src/ln/channelmanager.rs: where
    -           payment_secrets.retain(|_, inbound_payment| {
    -                   inbound_payment.expiry_time > header.time as u64
    -           });
    +                   }
    +           }
    +           max_time!(self.highest_seen_timestamp);
     +          #[cfg(feature = "dnssec")] {
     +                  let timestamp = self.highest_seen_timestamp.load(Ordering::Relaxed) as u32;
     +                  self.hrn_resolver.new_best_block(height, timestamp);
6:  ef273123b = 6:  2bbb3b708 Use `ChannelManager` as `DNSResolverMessageHandler` by default
7:  af3c4accf = 7:  8e941426c Add a `lightning-dns-resolver` crate which answers bLIP 32 queries
8:  ce3b143c3 = 8:  cbdb8cb0c Set the `dns_resolution` feature in `OMDomainResolver`

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch 2 times, most recently from b42d6e0 to d4691a1 Compare August 31, 2024 17:09

tnull self-requested a review September 2, 2024 14:30

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from d4691a1 to 9b2ef2d Compare September 30, 2024 18:51

TheBlueMatt marked this pull request as ready for review September 30, 2024 18:53

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch 2 times, most recently from 93efbec to b2cbd26 Compare October 2, 2024 17:20

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from b2cbd26 to dab9c91 Compare October 2, 2024 18:14

TheBlueMatt mentioned this pull request Oct 2, 2024

Add support for parsing the dns_resolver feature bit #3346

Merged

arik-so reviewed Oct 8, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

TheBlueMatt added the Optech Make Me Famous label Oct 8, 2024

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch 2 times, most recently from 54789fc to ae5d584 Compare October 8, 2024 19:48

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from ae5d584 to 8ea3cd4 Compare October 10, 2024 23:08

arik-so self-assigned this Oct 28, 2024

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from 8ea3cd4 to 6c7b888 Compare October 28, 2024 19:00

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from 6c7b888 to c1dfc8c Compare October 29, 2024 18:05

arik-so reviewed Oct 30, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from c1dfc8c to 5024aa9 Compare November 4, 2024 18:38

vincenzopalazzo reviewed Nov 5, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

vincenzopalazzo approved these changes Nov 5, 2024

View reviewed changes

tnull reviewed Nov 6, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from 5024aa9 to 260fe98 Compare November 7, 2024 15:55

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from 260fe98 to cc054cb Compare November 7, 2024 15:56

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch 4 times, most recently from 5be55b2 to f5c215e Compare November 7, 2024 16:22

tnull reviewed Nov 8, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from f5c215e to 31e16f8 Compare November 8, 2024 14:52

tnull reviewed Nov 11, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from 31e16f8 to c7cd9b6 Compare November 11, 2024 15:41

tnull reviewed Nov 12, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from c7cd9b6 to ce3b143 Compare November 12, 2024 15:53

TheBlueMatt added 8 commits November 12, 2024 15:53

Add a new AwaitingOffer outbound payment state for BIP 353

46df35b

Set the dns_resolution feature in OMDomainResolver

cbdb8cb

`OMDomainResolver` actually does support building DNSSECProofs, so should be setting the `dns_resolution` `NodeFeature` flag.

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution branch from ce3b143 to cbdb8cb Compare November 12, 2024 15:54

arik-so approved these changes Nov 12, 2024

View reviewed changes

tnull approved these changes Nov 12, 2024

View reviewed changes

tnull merged commit f152689 into lightningdevkit:main Nov 12, 2024
17 of 20 checks passed

Support paying directly to Human Readable Names using bLIP 32 #3283

Support paying directly to Human Readable Names using bLIP 32 #3283

Conversation

TheBlueMatt commented Aug 30, 2024

codecov bot commented Aug 30, 2024 • edited Loading

Codecov Report

TheBlueMatt commented Oct 2, 2024

arik-so left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Oct 8, 2024

TheBlueMatt commented Oct 28, 2024

arik-so commented Oct 29, 2024

TheBlueMatt commented Oct 29, 2024

arik-so left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 7, 2024

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 8, 2024

tnull left a comment • edited Loading

Choose a reason for hiding this comment

tnull Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 12, 2024

codecov bot commented Aug 30, 2024 •

edited

Loading

tnull left a comment •

edited

Loading

tnull Nov 11, 2024 •

edited

Loading