-
Notifications
You must be signed in to change notification settings - Fork 116
Add support for resolving BIP 353 Human-Readable Names #630
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ use lightning::ln::channelmanager::{self, ChainParameters, ChannelManagerReadArg | |
| use lightning::ln::msgs::{RoutingMessageHandler, SocketAddress}; | ||
| use lightning::ln::peer_handler::{IgnoringMessageHandler, MessageHandler}; | ||
| use lightning::log_trace; | ||
| use lightning::onion_message::dns_resolution::DNSResolverMessageHandler; | ||
| use lightning::routing::gossip::NodeAlias; | ||
| use lightning::routing::router::DefaultRouter; | ||
| use lightning::routing::scoring::{ | ||
|
|
@@ -38,6 +39,7 @@ use lightning::util::persist::{ | |
| }; | ||
| use lightning::util::ser::ReadableArgs; | ||
| use lightning::util::sweep::OutputSweeper; | ||
| use lightning_dns_resolver::OMDomainResolver; | ||
| use lightning_persister::fs_store::FilesystemStore; | ||
| use vss_client::headers::VssHeaderProvider; | ||
|
|
||
|
|
@@ -72,8 +74,9 @@ use crate::peer_store::PeerStore; | |
| use crate::runtime::Runtime; | ||
| use crate::tx_broadcaster::TransactionBroadcaster; | ||
| use crate::types::{ | ||
| ChainMonitor, ChannelManager, DynStore, DynStoreWrapper, GossipSync, Graph, KeysManager, | ||
| MessageRouter, OnionMessenger, PaymentStore, PeerManager, Persister, SyncAndAsyncKVStore, | ||
| ChainMonitor, ChannelManager, DomainResolver, DynStore, DynStoreWrapper, GossipSync, Graph, | ||
| HRNResolver, KeysManager, MessageRouter, OnionMessenger, PaymentStore, PeerManager, Persister, | ||
| SyncAndAsyncKVStore, | ||
| }; | ||
| use crate::wallet::persist::KVStoreWalletPersister; | ||
| use crate::wallet::Wallet; | ||
|
|
@@ -185,6 +188,8 @@ pub enum BuildError { | |
| NetworkMismatch, | ||
| /// The role of the node in an asynchronous payments context is not compatible with the current configuration. | ||
| AsyncPaymentsConfigMismatch, | ||
| /// An attempt to setup a DNS Resolver failed. | ||
| DNSResolverSetupFailed, | ||
| } | ||
|
|
||
| impl fmt::Display for BuildError { | ||
|
|
@@ -217,12 +222,21 @@ impl fmt::Display for BuildError { | |
| "The async payments role is not compatible with the current configuration." | ||
| ) | ||
| }, | ||
| Self::DNSResolverSetupFailed => { | ||
| write!(f, "An attempt to setup a DNS resolver has failed.") | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::error::Error for BuildError {} | ||
|
|
||
| enum Resolver { | ||
| HRN(Arc<HRNResolver>), | ||
| DNS(Arc<DomainResolver>), | ||
| Ignore(Arc<IgnoringMessageHandler>), | ||
| } | ||
|
|
||
| /// A builder for an [`Node`] instance, allowing to set some configuration and module choices from | ||
| /// the getgo. | ||
| /// | ||
|
|
@@ -1444,7 +1458,34 @@ fn build_with_store_internal( | |
| })?; | ||
| } | ||
|
|
||
| let hrn_resolver = Arc::new(LDKOnionMessageDNSSECHrnResolver::new(Arc::clone(&network_graph))); | ||
| let resolver = if let Some(hrn_config) = &config.hrn_config { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the HRN resolver config, do we need the Right now we check both if the config exists andthe boolean value, but we end up with the same HRN resolver when the config exists with Seems like we could simplify this by just using the presence of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review! I think
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point about
Just to clarify, do you mean that if hrn_config is not set at all, the resolver would be None?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Here is what I am talking about -->
if Already implemented this locally, will push updates asap.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also note that in this implementation, enabling DNS resolution for other nodes would prevent us from being able to send to HRNs ourselves (#630 (comment))
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes this looks like a valid approach. false and None returns different values. Thanks |
||
| if hrn_config.is_hrn_resolver { | ||
| let dns_addr = hrn_config.dns_server_address.as_str(); | ||
|
|
||
| Resolver::DNS(Arc::new(OMDomainResolver::ignoring_incoming_proofs( | ||
| dns_addr.parse().map_err(|_| BuildError::DNSResolverSetupFailed)?, | ||
| ))) | ||
| } else { | ||
| Resolver::HRN(Arc::new(LDKOnionMessageDNSSECHrnResolver::new(Arc::clone( | ||
| &network_graph, | ||
| )))) | ||
| } | ||
| } else { | ||
| // hrn_config is None, default to the IgnoringMessaageHandler. | ||
| Resolver::Ignore(Arc::new(IgnoringMessageHandler {})) | ||
| }; | ||
|
|
||
| let om_resolver = match resolver { | ||
| Resolver::DNS(ref dns_resolver) => { | ||
| Arc::clone(dns_resolver) as Arc<dyn DNSResolverMessageHandler + Send + Sync> | ||
| }, | ||
| Resolver::HRN(ref hrn_resolver) => { | ||
| Arc::clone(hrn_resolver) as Arc<dyn DNSResolverMessageHandler + Send + Sync> | ||
| }, | ||
| Resolver::Ignore(ref ignoring_handler) => { | ||
| Arc::clone(ignoring_handler) as Arc<dyn DNSResolverMessageHandler + Send + Sync> | ||
| }, | ||
| }; | ||
|
|
||
| // Initialize the PeerManager | ||
| let onion_messenger: Arc<OnionMessenger> = | ||
|
|
@@ -1457,7 +1498,7 @@ fn build_with_store_internal( | |
| message_router, | ||
| Arc::clone(&channel_manager), | ||
| Arc::clone(&channel_manager), | ||
| Arc::clone(&hrn_resolver), | ||
| Arc::clone(&om_resolver), | ||
| IgnoringMessageHandler {}, | ||
| )) | ||
| } else { | ||
|
|
@@ -1469,7 +1510,7 @@ fn build_with_store_internal( | |
| message_router, | ||
| Arc::clone(&channel_manager), | ||
| Arc::clone(&channel_manager), | ||
| Arc::clone(&hrn_resolver), | ||
| Arc::clone(&om_resolver), | ||
| IgnoringMessageHandler {}, | ||
| )) | ||
| }; | ||
|
|
@@ -1599,9 +1640,16 @@ fn build_with_store_internal( | |
|
|
||
| let peer_manager_clone = Arc::clone(&peer_manager); | ||
|
|
||
| hrn_resolver.register_post_queue_action(Box::new(move || { | ||
| peer_manager_clone.process_events(); | ||
| })); | ||
| let hrn_resolver = match resolver { | ||
| Resolver::DNS(_) => None, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, so if we're resolving for others, we won't be able to send to HRNs ourselves?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my understanding, it seems this is the case. The onion messenger can take only one type of
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, coming back to this: it seems in case we act as a bLIP-32 service we want to use an additional instance of |
||
| Resolver::HRN(ref hrn_resolver) => { | ||
| hrn_resolver.register_post_queue_action(Box::new(move || { | ||
| peer_manager_clone.process_events(); | ||
| })); | ||
| Some(hrn_resolver) | ||
| }, | ||
| Resolver::Ignore(_) => None, | ||
| }; | ||
|
|
||
| liquidity_source.as_ref().map(|l| l.set_peer_manager(Arc::clone(&peer_manager))); | ||
|
|
||
|
|
@@ -1716,7 +1764,7 @@ fn build_with_store_internal( | |
| node_metrics, | ||
| om_mailbox, | ||
| async_payments_role, | ||
| hrn_resolver, | ||
| hrn_resolver: hrn_resolver.cloned(), | ||
| }) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,7 +119,8 @@ pub(crate) const HRN_RESOLUTION_TIMEOUT_SECS: u64 = 5; | |
| /// | `probing_liquidity_limit_multiplier` | 3 | | ||
| /// | `log_level` | Debug | | ||
| /// | `anchor_channels_config` | Some(..) | | ||
| /// | `route_parameters` | None | | ||
| /// | `route_parameters` | None | | ||
| /// | `hrn_config` | Some(..) | | ||
| /// | ||
| /// See [`AnchorChannelsConfig`] and [`RouteParametersConfig`] for more information regarding their | ||
| /// respective default values. | ||
|
|
@@ -184,6 +185,10 @@ pub struct Config { | |
| /// **Note:** If unset, default parameters will be used, and you will be able to override the | ||
| /// parameters on a per-payment basis in the corresponding method calls. | ||
| pub route_parameters: Option<RouteParametersConfig>, | ||
| /// Configuration options for Human-Readable Names ([BIP 353]). | ||
| /// | ||
| /// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki | ||
| pub hrn_config: Option<HumanReadableNamesConfig>, | ||
| } | ||
|
|
||
| impl Default for Config { | ||
|
|
@@ -198,6 +203,34 @@ impl Default for Config { | |
| anchor_channels_config: Some(AnchorChannelsConfig::default()), | ||
| route_parameters: None, | ||
| node_alias: None, | ||
| hrn_config: Some(HumanReadableNamesConfig::default()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Configuration options for Human-Readable Names ([BIP 353]). | ||
| /// | ||
| /// [BIP 353]: https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki | ||
| #[derive(Debug, Clone)] | ||
| pub struct HumanReadableNamesConfig { | ||
| /// The Default DNS resolvers to be used for resolving Human-Readable Names. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Can just drop |
||
| /// | ||
| /// If not empty, the values set will be used as DNS resolvers when sending to HRNs. | ||
| /// | ||
| /// **Note:** If empty, DNS resolvers would be selected from the network graph. | ||
| pub default_dns_resolvers: Vec<PublicKey>, | ||
| /// This allows us to use our node as a DNS resolver for 3rd party HRN resolutions. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This is oddly formulated: if set this allows others to use our nodes for HRN resolutions, no? Probably also want to link to the bLIP here? |
||
| pub is_hrn_resolver: bool, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, maybe we should make this a |
||
| /// The DNS Server which will be used for resolving HRNs. | ||
| pub dns_server_address: String, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is confusing. How does this interact with |
||
| } | ||
|
|
||
| impl Default for HumanReadableNamesConfig { | ||
| fn default() -> Self { | ||
| HumanReadableNamesConfig { | ||
| default_dns_resolvers: Vec::new(), | ||
| is_hrn_resolver: false, | ||
| dns_server_address: String::new(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
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.
Do we still need this if we add a corresponding
enumat the config-level? If so, can we name this a bit more HRN-specific at least?