-
Notifications
You must be signed in to change notification settings - Fork 113
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
dd725bc
02ed479
7e6e7e7
6d20a9e
2c809b1
8696269
9b7584b
3e30347
bd668f6
ec435ce
4c1a03a
49f4013
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 |
|---|---|---|
|
|
@@ -19,12 +19,14 @@ use bip39::Mnemonic; | |
| use bitcoin::bip32::{ChildNumber, Xpriv}; | ||
| use bitcoin::secp256k1::PublicKey; | ||
| use bitcoin::{BlockHash, Network}; | ||
| use bitcoin_payment_instructions::onion_message_resolver::LDKOnionMessageDNSSECHrnResolver; | ||
| use lightning::chain::{chainmonitor, BestBlock, Watch}; | ||
| use lightning::io::Cursor; | ||
| use lightning::ln::channelmanager::{self, ChainParameters, ChannelManagerReadArgs}; | ||
| 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 +40,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::{FixedHeaders, LnurlAuthToJwtProvider, VssHeaderProvider}; | ||
|
|
||
|
|
@@ -69,8 +72,8 @@ use crate::peer_store::PeerStore; | |
| use crate::runtime::Runtime; | ||
| use crate::tx_broadcaster::TransactionBroadcaster; | ||
| use crate::types::{ | ||
| ChainMonitor, ChannelManager, DynStore, GossipSync, Graph, KeysManager, MessageRouter, | ||
| OnionMessenger, PaymentStore, PeerManager, Persister, | ||
| ChainMonitor, ChannelManager, DomainResolver, DynStore, GossipSync, Graph, HRNResolver, | ||
| KeysManager, MessageRouter, OnionMessenger, PaymentStore, PeerManager, Persister, | ||
| }; | ||
| use crate::wallet::persist::KVStoreWalletPersister; | ||
| use crate::wallet::Wallet; | ||
|
|
@@ -195,6 +198,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 { | ||
|
|
@@ -229,12 +234,20 @@ 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>), | ||
| } | ||
|
|
||
| /// A builder for an [`Node`] instance, allowing to set some configuration and module choices from | ||
| /// the getgo. | ||
| /// | ||
|
|
@@ -1536,6 +1549,32 @@ fn build_with_store_internal( | |
| })?; | ||
| } | ||
|
|
||
| let resolver = if let Some(hrn_config) = &config.hrn_config { | ||
| 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 HRN resolver. | ||
| Resolver::HRN(Arc::new(LDKOnionMessageDNSSECHrnResolver::new(Arc::clone(&network_graph)))) | ||
| }; | ||
|
|
||
| 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> | ||
| }, | ||
| }; | ||
|
|
||
| // Initialize the PeerManager | ||
| let onion_messenger: Arc<OnionMessenger> = | ||
| if let Some(AsyncPaymentsRole::Server) = async_payments_role { | ||
|
|
@@ -1547,7 +1586,7 @@ fn build_with_store_internal( | |
| message_router, | ||
| Arc::clone(&channel_manager), | ||
| Arc::clone(&channel_manager), | ||
| IgnoringMessageHandler {}, | ||
| Arc::clone(&om_resolver), | ||
| IgnoringMessageHandler {}, | ||
| )) | ||
| } else { | ||
|
|
@@ -1559,7 +1598,7 @@ fn build_with_store_internal( | |
| message_router, | ||
| Arc::clone(&channel_manager), | ||
| Arc::clone(&channel_manager), | ||
| IgnoringMessageHandler {}, | ||
| Arc::clone(&om_resolver), | ||
| IgnoringMessageHandler {}, | ||
| )) | ||
| }; | ||
|
|
@@ -1691,6 +1730,18 @@ fn build_with_store_internal( | |
| Arc::clone(&keys_manager), | ||
| )); | ||
|
|
||
| let peer_manager_clone = Arc::clone(&peer_manager); | ||
|
|
||
| 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 |
||
| Resolver::HRN(ref hrn_resolver) => { | ||
| hrn_resolver.register_post_queue_action(Box::new(move || { | ||
| peer_manager_clone.process_events(); | ||
| })); | ||
| Some(hrn_resolver) | ||
| }, | ||
| }; | ||
|
|
||
| liquidity_source.as_ref().map(|l| l.set_peer_manager(Arc::clone(&peer_manager))); | ||
|
|
||
| gossip_source.set_gossip_verifier( | ||
|
|
@@ -1797,6 +1848,7 @@ fn build_with_store_internal( | |
| node_metrics, | ||
| om_mailbox, | ||
| async_payments_role, | ||
| hrn_resolver: hrn_resolver.cloned(), | ||
| }) | ||
| } | ||
|
|
||
|
|
||
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.
Looking at the HRN resolver config, do we need the
is_hrn_resolverboolean?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
is_hrn_resolver = falseor when there's no config at all.Seems like we could simplify this by just using the presence of
hrn_configto decide which resolver to use, and drop the boolean field entirely.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.
Thanks for the review! I think
is_hrn_resolveris actually the core of this PR as it should be set explicitly by the user - a node might sethrn_configbut choose not to resolve for others but just to enable sending (#666). Instead of droppingis_hrn_resolver, I think we should instead have a newResolvervariant forIgnoringMessageHandlerwhich should be returned whenever nohrn_configis provided - I think that's more idiomatic because ifhrn_configisn't set at all, we shouldn't be doing any HRN-related operations anyway.