Add support for resolving BIP 353 Human-Readable Names #630
Conversation
This rename reflects that this module is a unified payment interface for both QR code payments and HRN payments passed in as a string without scanning a QR code
…UnifiedPaymentResult These renamings are necessary to reflect the expanded responsibilities for this module.
This commit adds a HRN Resolver to the Node struct which will be useful for resolving HRNs when making BIP 353 payments. It also passes the HRN Resolver into UnifiedPayment.
This commit allows us to enable our node to either be able to send HRN resolution requests or acts as a resolver for 3rd party nodes We also pass HRNResolver as an optional parameter to Node
This commit will be dropped later
This commit adds an end-to-end test that asserts that HRNs are properly parsed and resolved, and sending to the corresponding offer works
|
👋 Hi! I see this is a draft PR. |
|
This PR builds upon #607. It is currently opened in draft because the end-to-end test I added runs indefinitely without resolving or timing out. |
| @@ -93,6 +93,7 @@ log = { version = "0.4.22", default-features = false, features = ["std"]} | |||
| vss-client = "0.3" | |||
| prost = { version = "0.11.6", default-features = false} | |||
| bitcoin-payment-instructions = { version = "0.5" } | |||
| lightning-dns-resolver = "0.2.0" | |||
There was a problem hiding this comment.
Please add this above to the group(s) of lightning-* imports (also the commented-out ones) and follow the scheme there.
| @@ -13,6 +13,7 @@ dictionary Config { | |||
| u64 probing_liquidity_limit_multiplier; | |||
| AnchorChannelsConfig? anchor_channels_config; | |||
| SendingParameters? sending_parameters; | |||
| boolean is_hrn_resolver; | |||
There was a problem hiding this comment.
Hmm, I think over at the first PR we discussed to introduce a dedicated config for HRN-related things, also to specify default resolvers, for example? Probably this should be part of such a config?
There was a problem hiding this comment.
Yes we did - I'll re-introduce the dedicated config for HRN-related things.
| let hrn_resolver = Arc::new(LDKOnionMessageDNSSECHrnResolver::new(Arc::clone(&network_graph))); | ||
| let resolver = if config.is_hrn_resolver { | ||
| Resolver::DNS(Arc::new(OMDomainResolver::ignoring_incoming_proofs( | ||
| "8.8.8.8:53".parse().map_err(|_| BuildError::DNSResolverSetupFailed)?, |
There was a problem hiding this comment.
We probably want to make the DNS server used configurable via the config?
There was a problem hiding this comment.
Yes that makes sense - will do.
| })?; | ||
|
|
||
| println!("Parsing instructions..."); |
There was a problem hiding this comment.
Oh, please remove any printlns.
There was a problem hiding this comment.
Yeah - will do, just left this here because I am still debugging the issue with the end-to-end test not resolving HRNs, erroring out or timing out.
| peer_manager_clone.process_events(); | ||
| })); | ||
| let hrn_resolver = match resolver { | ||
| Resolver::DNS(_) => None, |
There was a problem hiding this comment.
Huh, so if we're resolving for others, we won't be able to send to HRNs ourselves?
There was a problem hiding this comment.
From my understanding, it seems this is the case. The onion messenger can take only one type of dns_resolver at a time - either the one sending out the query or the one handling the query and responding with the proof. If our node is configured to resolve for others, we'd have to pass the appropriate resolver to the onion messenger, preventing us from sending to HRNs ourselves.
This PR makes it possible to configure LDK-Node as a DNS resolver for other nodes.
Changes
boolflag namedis_hrn_resolvertoConfig.is_hrn_resolver, a new node either has the ability to send payments to HRNs or do DNS resolution for other nodes.setup_two_nodesin the integration test suite to allow configuring a node to act as a DNS resolver for other nodes.This PR fixes #436