Add the core functionality required to resolve Human Readable Names #3179

TheBlueMatt · 2024-07-14T13:42:17Z

This adds new onion messages and a new onion message handler for bLIP 32 messages. It also adds a utility which verifies the proofs and turns them into URIs or Offers. I have a followup ready which wires it all up in ChannelManager as well as a message handler that does the resolver end of bLIP 32, but the latter is blocked on #3178 and I kinda want to land them at the same time so we have some tests. Sadly this PR also adds some code that's not really testable without that, but its pretty straightforward.

codecov-commenter · 2024-07-14T13:49:33Z

Codecov Report

Attention: Patch coverage is 21.05263% with 120 lines in your changes missing coverage. Please review.

Project coverage is 90.79%. Comparing base (7e513f9) to head (03406f7).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/onion_message/dns_resolution.rs	8.33%	77 Missing ⚠️
lightning/src/onion_message/messenger.rs	36.00%	15 Missing and 1 partial ⚠️
lightning/src/onion_message/packet.rs	0.00%	8 Missing and 1 partial ⚠️
lightning/src/ln/peer_handler.rs	0.00%	6 Missing ⚠️
lightning/src/onion_message/functional_tests.rs	45.45%	6 Missing ⚠️
lightning/src/ln/offers_tests.rs	0.00%	3 Missing ⚠️
lightning/src/util/ser.rs	75.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3179      +/-   ##
==========================================
+ Coverage   89.82%   90.79%   +0.97%     
==========================================
  Files         126      127       +1     
  Lines      103134   110965    +7831     
  Branches   103134   110965    +7831     
==========================================
+ Hits        92643   100755    +8112     
+ Misses       7772     7582     -190     
+ Partials     2719     2628      -91

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

tnull · 2024-07-15T07:48:26Z

This adds new onion messages and a new onion message handler for bLIP 32 messages. It also adds a utility which verifies the proofs and turns them into URIs or Offers. I have a followup ready which wires it all up in ChannelManager as well as a message handler that does the resolver end of bLIP 32, but the latter is blocked on #3178 and I kinda want to land them at the same time so we have some tests. Sadly this PR also adds some code that's not really testable without that, but its pretty straightforward.

I'd love to pickup review on the series. Mind opening a tracking issue for the series of PRs that would allow to keep track where we're at, on what which parts are blocked, and which PRs we (reviewers) can still expect to be opened, etc.?

tnull

While BIP353 will (hopefully) be an integral part of Lightning payments soon, it still seems all the DNS stuff is more a utility. I'm wondering if in terms of code organization it would be beneficial to have this live as part of an lightning-dns-resolution crate or similar? Doing this might also allow users to reuse some of the logic without pulling in the full LDK dependency? Or do you think the logic will end up being so integral to and intertwined with other parts of LDK, that having it live it a separate crate would just be cumbersome?

lightning/Cargo.toml

TheBlueMatt · 2024-07-15T18:05:29Z

While BIP353 will (hopefully) be an integral part of Lightning payments soon, it still seems all the DNS stuff is more a utility. I'm wondering if in terms of code organization it would be beneficial to have this live as part of an lightning-dns-resolution crate or similar?

I wanted to do this, but it creates a circular reference. That crazy has to depend on lightning for some of the onion message types, and then we want lightning to depend on it for ChannelManager to be able to directly send to BIP 353 names :/

The stuff that is here is the API that folks will have to use if they want to do DNSSEC resolution over onion messages and resolve to a bitcoin: URI, but I have a branch that does the full integration at https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=shortlog;h=refs/heads/2024-07-human-readable-names-resolution for offers resolution specifically.

TheBlueMatt · 2024-07-15T18:06:15Z

I'd love to pickup review on the series. Mind opening a tracking issue for the series of PRs that would allow to keep track where we're at, on what which parts are blocked, and which PRs we (reviewers) can still expect to be opened, etc.?

I mean I can, but its only two PRs, at least after the above issue. The full branch I considered making one big PR, but it seemed like it could go in two. The second is linked above.

TheBlueMatt · 2024-08-30T19:35:33Z

Rebased.

This creates the initial DNSSEC proof and query messages in a new module in `onion_message`, as well as a new message handler to handle them. In the coming commits, a default implementation will be added which verifies DNSSEC proofs which can be used to resolve BIP 353 URIs without relying on anything outside of the lightning network.

arik-so · 2024-09-03T17:34:24Z

With the lightning-types crate, would the circular reference still be an issue?

TheBlueMatt · 2024-09-03T17:47:28Z

Yea, its a separate issue. In theory we could move the BIP 353 Onion Message Handler trait out to lightning-types (or some new crate) which lightning then depends on to implement it in OnionMessenger and ChannelManager and then we keep the current separate server-side crate with the dns resolver, but I'm not sure its worth it for the one trait.

lightning/src/onion_message/dns_resolution.rs

arik-so · 2024-09-10T16:14:05Z

lightning/src/onion_message/dns_resolution.rs

+				// (we assume no more than two hours, though the actual limits are rather
+				// complicated).
+				// Thus, we have to let the proof times be rather fuzzy.
+				if validated_rrs.valid_from > block_time + 60 * 2 {


This can be a constant.

Its basically a consensus rule assumption, so not sure its worth it :)

tnull

Generally looks good, just a few questions and nits.

I think the main question is whether we could use timer ticks rather than block connections to timeout pending requests to avoid piling onto the stack of things that we need to do on block connection, increasingly risking load spikes.

lightning/src/onion_message/dns_resolution.rs

tnull · 2024-09-12T14:45:35Z

lightning/src/onion_message/dns_resolution.rs

+	/// block.
+	///
+	/// This will call back to resolve some pending queries which have timed out.
+	pub fn new_best_block(&self, height: u32, time: u32) {


Mh, no big fan of doing more and more stuff exactly on block connection. Given that we call OnionMessenger::timer_tick_occured in BP anyways, couldn't we use that here?

The issue is we need the current time here (to validate proofs), not just a timer. We could have both a timer and accept new best block info, but that seems kinda wasteful?

Mhh, true. I think we discussed that before, but would it work to just use SystemTime for std and otherwise introduce a timer trait that non-std users would need to use? FWIW, we need something like this in lightning-liquidity also when validating requests against wallclock (on non-std we currently just omit validating this part, but we should lightningdevkit/lightning-liquidity#54). If we bring the liquidity crate into rust-lightning soon, it might make sense to share an interface?

I think we discussed that before, but would it work to just use SystemTime for std and otherwise introduce a timer trait that non-std users would need to use?

I think adding such a trait would just be a massive footgun - users would implement against the std interface then forget to call the other trait when switching to/building for no-std. Generally, we try to avoid any material interface differences as much as possible to avoid things like this.

FWIW, we need something like this in lightning-liquidity also when validating requests against wallclock (on non-std we currently just omit validating this part, but we should lightningdevkit/lightning-liquidity#54). If we bring the liquidity crate into rust-lightning soon, it might make sense to share an interface?

Meh, if the parameters are stale the LSP can just reject, right? Not checking seems...fine?

Meh, if the parameters are stale the LSP can just reject, right? Not checking seems...fine?

This is on the LSP-side.

This is on the LSP-side.

Ah, right....well if LSPs are fine with +/- two hours you can always use block time. And, probably they are fine with that?

Mhh, yeah, but as mentioned above I'd really prefer to get away from piling everything onto block connection to avoid the ensuing load spike(s). Could we consider building an LDK-internal clock that is synchronized on block connection, but otherwise driven by timing_tick_occured? Having such an internal interface/utility would solve these issues that we reguarly see re-emerging, and would allow us to schedule things somewhere in the huge interval between block connections?

Mhh, yeah, but as mentioned above I'd really prefer to get away from piling everything onto block connection to avoid the ensuing load spike(s).

Hmm, is there much load spike now that we don't persist ChannelMonitors every block?

Could we consider building an LDK-internal clock that is synchronized on block connection, but otherwise driven by timing_tick_occured? Having such an internal interface/utility would solve these issues that we reguarly see re-emerging, and would allow us to schedule things somewhere in the huge interval between block connections?

I'm not sure what issues we see here? There's some timeouts that we don't allow to be very granular, but for the most part that's not a big deal, is it?

tnull · 2024-09-12T14:50:48Z

lightning/src/onion_message/dns_resolution.rs

+	/// [`Self::resolve_name`] are returned.
+	pub fn handle_dnssec_proof_for_offer(
+		&self, msg: DNSSECProof, context: DNSResolverContext,
+	) -> Option<(Vec<(HumanReadableName, PaymentId)>, Offer)> {


Can we introduce structs for the return type here and the one from handle_dnssec_proof_for_uri to make them a bit more ... human readable?

Does it need it? I'm not sure how a list of (HumanReadableName, PaymentId)s and an Offer is confusing?

I mean it's an option of a tuple of a list of a tuple of a human readable name and a payment id, and an offer.

Just accessing all that data in the return type could have a bit simpler API, no?

I'm not sure how we'd clean it up, is my point. If we put it in a struct it'd be

struct Thing { payments: Vec<(HumanReadableName, PaymentId)>, offer: Offer, }

the offer name is obviously totally useless just based on the type, and the payments name is similarly not that interesting given each payment has a PaymentId attached to it? It might improve callsites marginally cause they can access return_value.payments, but mostly they should be doing let (payments, offer) = ... instead, which gives the same impact (and its not like we can misuse any of this cause the types are pretty strict).

lightning/src/onion_message/dns_resolution.rs

tnull · 2024-09-12T14:53:41Z

lightning/src/onion_message/dns_resolution.rs

+	}
+}
+
+/// A struct containing the two parts of a BIP 353 Human Readable Name - the user and domain parts.


nit: Tick struct

Its a word, not a reference to a variable/type. Do we normally tick words?

Its a word, not a reference to a variable/type. Do we normally tick words?

Is it though? "struct" is an abbreviation at best, it's really a keyword / abstract type of the language? In any case, it's a nit, so feel free to leave as is.

I don't think we tick struct anywhere else.

This adds the requisite message parsing and handling code for the new DNSSEC messages to `OnionMessenger`.

When we make a DNSSEC query with a reply path, we don't want to allow the DNS resolver to attempt to respond to various nodes to try to detect (through timining or other analysis) whether we were the one who made the query. Thus, we need to include a nonce in the context in our reply path, which we set up here by creating a new context type for DNS resolutions.

BIP 353 `HumanReadableName`s are represented as `₿user@domain` and can be resolved using DNS into a `bitcoin:` URI. In the next commit, we will add such a resolver using onion messages to fetch records from the DNS, which will rely on this new type to get name information from outside LDK.

This adds a new utility struct, `OMNameResolver`, which implements the core functionality required to resolve Human Readable Names, namely generating `DNSSECQuery` onion messages, tracking the state of requests, and ultimately receiving and verifying `DNSSECProof` onion messages. It tracks pending requests with a `PaymentId`, allowing for easy integration into `ChannelManager` in a coming commit - mapping received proofs to `PaymentId`s which we can then complete by handing them `Offer`s to pay. It does not, directly, implement `DNSResolverMessageHandler`, but an implementation of `DNSResolverMessageHandler` becomes trivial with `OMNameResolver` handling the inbound messages and creating the messages to send.

vincenzopalazzo · 2024-09-13T13:00:33Z

lightning/src/onion_message/dns_resolution.rs

Commit 6b760e7 could be fold in the previous one

Yes, all the commits that start with f are intended to be squashed prior to merge. They exist to aid reviewers in comparing a previous version of the PR to the new one by leaving existing commits unchanged and only adding new "fixup" commits.

vincenzopalazzo

LGTM, I start adding comments on the git history but I think we did not rework it yet.

vincenzopalazzo · 2024-09-13T13:17:14Z

lightning/Cargo.toml

@@ -32,6 +32,8 @@ unsafe_revoked_tx_signing = []
 no-std = ["hashbrown", "possiblyrandom", "libm"]
 std = []

+dnssec = ["dnssec-prover/validation"]


thinking if we can make the dnssec-prover dependencies optional and included just when this feature is specified?

We maybe could, but it seems nice to use the types from it, and the crate weighs basically nothing if we just pull the types from it and don't enable the validation feature.

I see, and I agree. I was thinking that maybe the dnssec will be a popular feature that we want enabled most of the time

Yea, it likely is (and maybe we should make it default, even), but I think even still we can use the types from it no matter what.

tnull

LGTM. Feel free to squash the fixups, IMO.

Going forward, will the client and the resolver always be modal, or can we think of a scenario where both need to be active at the same time (i.e., allowing to resolve locally for others, but also to query other nodes)?

tnull · 2024-09-19T09:17:13Z

lightning/src/onion_message/dns_resolution.rs

+
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
+/// A message which is sent to a DNSSEC prover requesting a DNSSEC proof for the given name.
+pub struct DNSSECQuery(pub Name);


nit: Add newline (here and below).

tnull · 2024-09-19T09:17:45Z

lightning/src/onion_message/dns_resolution.rs

+	/// The name which the query was for. The proof may not contain a DNS RR for exactly this name
+	/// if it contains a wildcard RR which contains this name instead.
+	pub name: Name,
+	/// The RFC 9102-formatted DNSSEC proof.


nit: Link to RFC.

tnull · 2024-09-19T09:24:41Z

lightning/src/onion_message/dns_resolution.rs

+	///
+	/// If we provide DNS resolution services to third parties, we should respond with a
+	/// [`DNSSECProof`] message.
+	fn dnssec_query(


Should these methods have a handle_ prefix?

tnull · 2024-09-19T09:29:57Z

lightning/Cargo.toml

@@ -12,7 +12,7 @@ Still missing tons of error-handling. See GitHub issues for suggested projects i
 edition = "2021"

 [package.metadata.docs.rs]
-features = ["std"]
+features = ["std", "dnssec"]


Might want to rebase after #3289 has been merged to double-check there is no silent merge conflict?

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution-1 branch 2 times, most recently from 7203f95 to 51c74d9 Compare July 14, 2024 23:50

tnull self-requested a review July 15, 2024 07:02

tnull reviewed Jul 15, 2024

View reviewed changes

lightning/Cargo.toml Show resolved Hide resolved

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution-1 branch from 51c74d9 to 66017a7 Compare July 15, 2024 12:46

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution-1 branch from 66017a7 to 12a8d39 Compare July 15, 2024 19:21

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution-1 branch 2 times, most recently from 080de8d to fb54dd4 Compare August 30, 2024 19:35

TheBlueMatt mentioned this pull request Aug 30, 2024

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

Draft

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution-1 branch from fb54dd4 to 5e594b7 Compare August 30, 2024 23:46

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution-1 branch from 5e594b7 to 41627c6 Compare August 30, 2024 23:51

arik-so reviewed Sep 6, 2024

View reviewed changes

lightning/src/onion_message/dns_resolution.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution-1 branch from 41627c6 to 083a572 Compare September 6, 2024 16:02

arik-so previously approved these changes Sep 10, 2024

View reviewed changes

TheBlueMatt dismissed arik-so’s stale review via 08d4d52 September 10, 2024 17:31

arik-so previously approved these changes Sep 10, 2024

View reviewed changes

tnull reviewed Sep 12, 2024

View reviewed changes

TheBlueMatt added 5 commits September 12, 2024 15:33

f sp

6b760e7

f use hostname type

513bb88

Parse and handle DNSResolverMessages in OnionMessenger

e1a87af

This adds the requisite message parsing and handling code for the new DNSSEC messages to `OnionMessenger`.

TheBlueMatt added 5 commits September 12, 2024 15:47

f use Hostname's validation

3a6440a

f use struct for queries

7018be4

f take an entropysource to force rng use

eaa1e2e

f add dnssec feature to docsrs

03406f7

TheBlueMatt dismissed arik-so’s stale review via 03406f7 September 12, 2024 16:05

TheBlueMatt force-pushed the 2024-07-human-readable-names-resolution-1 branch from 08d4d52 to 03406f7 Compare September 12, 2024 16:05

vincenzopalazzo reviewed Sep 13, 2024

View reviewed changes

vincenzopalazzo approved these changes Sep 13, 2024

View reviewed changes

tnull reviewed Sep 19, 2024

View reviewed changes

Add the core functionality required to resolve Human Readable Names #3179

Are you sure you want to change the base?

Add the core functionality required to resolve Human Readable Names #3179

Conversation

TheBlueMatt commented Jul 14, 2024

codecov-commenter commented Jul 14, 2024 • edited by codecov bot Loading

Codecov Report

tnull commented Jul 15, 2024

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 15, 2024

TheBlueMatt commented Jul 15, 2024

TheBlueMatt commented Aug 30, 2024

arik-so commented Sep 3, 2024

TheBlueMatt commented Sep 3, 2024

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

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

tnull Sep 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Sep 12, 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

vincenzopalazzo 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

tnull left a comment • 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

codecov-commenter commented Jul 14, 2024 •

edited by codecov bot

Loading

tnull Sep 16, 2024 •

edited

Loading

tnull Sep 12, 2024 •

edited

Loading

tnull left a comment •

edited

Loading