-
Notifications
You must be signed in to change notification settings - Fork 128
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
ab8fe4b
91ccc40
817c14c
8a4307d
9f2a292
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 |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| name: CI Checks - HRN Integration Tests | ||
|
|
||
| on: [push, pull_request] | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| hrn_integration: | ||
| name: HRN Tests - Ubuntu Stable | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout source code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Install Rust stable | ||
|
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. Let's copy what we do over at |
||
| run: | | ||
| rustup update stable | ||
| rustup default stable | ||
|
|
||
| - name: Enable caching for bitcoind/electrs | ||
| id: cache-deps | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: bin/ | ||
| key: hrn-deps-${{ runner.os }}-${{ hashFiles('scripts/download_bitcoind_electrs.sh') }} | ||
|
|
||
| - name: Download bitcoind/electrs | ||
| if: steps.cache-deps.outputs.cache-hit != 'true' | ||
| run: | | ||
| source ./scripts/download_bitcoind_electrs.sh | ||
| mkdir -p bin | ||
| mv "$BITCOIND_EXE" bin/bitcoind | ||
| mv "$ELECTRS_EXE" bin/electrs | ||
|
|
||
| - name: Set environment variables | ||
| run: | | ||
| echo "BITCOIND_EXE=$(pwd)/bin/bitcoind" >> "$GITHUB_ENV" | ||
| echo "ELECTRS_EXE=$(pwd)/bin/electrs" >> "$GITHUB_ENV" | ||
|
|
||
| - name: Run HRN Integration Tests | ||
| env: | ||
| RUSTFLAGS: "--cfg no_download --cfg hrn_tests" | ||
| run: cargo test --test integration_tests_hrn | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ use std::collections::HashMap; | |
| use std::convert::TryInto; | ||
| use std::default::Default; | ||
| use std::path::PathBuf; | ||
| use std::sync::{Arc, Mutex, Once, RwLock}; | ||
| use std::sync::{Arc, Mutex, Once, RwLock, Weak}; | ||
| use std::time::SystemTime; | ||
| use std::{fmt, fs}; | ||
|
|
||
|
|
@@ -19,12 +19,13 @@ use bitcoin::bip32::{ChildNumber, Xpriv}; | |
| use bitcoin::key::Secp256k1; | ||
| use bitcoin::secp256k1::PublicKey; | ||
| use bitcoin::{BlockHash, Network}; | ||
| use bitcoin_payment_instructions::dns_resolver::DNSHrnResolver; | ||
| use bitcoin_payment_instructions::onion_message_resolver::LDKOnionMessageDNSSECHrnResolver; | ||
| use lightning::chain::{chainmonitor, BestBlock}; | ||
| 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::{ | ||
|
|
@@ -39,13 +40,15 @@ use lightning::util::persist::{ | |
| }; | ||
| use lightning::util::ser::ReadableArgs; | ||
| use lightning::util::sweep::OutputSweeper; | ||
| use lightning::{log_trace, log_warn}; | ||
| use lightning_dns_resolver::OMDomainResolver; | ||
| use lightning_persister::fs_store::FilesystemStore; | ||
| use vss_client::headers::VssHeaderProvider; | ||
|
|
||
| use crate::chain::ChainSource; | ||
| use crate::config::{ | ||
| default_user_config, may_announce_channel, AnnounceError, AsyncPaymentsRole, | ||
| BitcoindRestClientConfig, Config, ElectrumSyncConfig, EsploraSyncConfig, | ||
| BitcoindRestClientConfig, Config, ElectrumSyncConfig, EsploraSyncConfig, HRNResolverConfig, | ||
| DEFAULT_ESPLORA_SERVER_URL, DEFAULT_LOG_FILENAME, DEFAULT_LOG_LEVEL, | ||
| }; | ||
| use crate::connection::ConnectionManager; | ||
|
|
@@ -76,8 +79,8 @@ use crate::runtime::{Runtime, RuntimeSpawner}; | |
| use crate::tx_broadcaster::TransactionBroadcaster; | ||
| use crate::types::{ | ||
| AsyncPersister, ChainMonitor, ChannelManager, DynStore, DynStoreWrapper, GossipSync, Graph, | ||
| KeysManager, MessageRouter, OnionMessenger, PaymentStore, PeerManager, PendingPaymentStore, | ||
| Persister, SyncAndAsyncKVStore, | ||
| HRNResolver, KeysManager, MessageRouter, OnionMessenger, PaymentStore, PeerManager, | ||
| PendingPaymentStore, Persister, SyncAndAsyncKVStore, | ||
| }; | ||
| use crate::wallet::persist::KVStoreWalletPersister; | ||
| use crate::wallet::Wallet; | ||
|
|
@@ -189,6 +192,10 @@ 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, | ||
| /// Failed to set up the peer manager. | ||
| PeerManagerSetupFailed, | ||
|
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. I don't think we'd ever want to run into this case. It's also very unclear what happened if we do. |
||
| } | ||
|
|
||
| impl fmt::Display for BuildError { | ||
|
|
@@ -221,6 +228,12 @@ 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.") | ||
| }, | ||
| Self::PeerManagerSetupFailed => { | ||
| write!(f, "Failed to set up the peer manager.") | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1545,7 +1558,74 @@ fn build_with_store_internal( | |
| })?; | ||
| } | ||
|
|
||
| let hrn_resolver = Arc::new(LDKOnionMessageDNSSECHrnResolver::new(Arc::clone(&network_graph))); | ||
| let peer_manager_hook: Arc<Mutex<Option<Weak<PeerManager>>>> = Arc::new(Mutex::new(None)); | ||
| let mut hrn_resolver_out = 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. Hmm, a comment here could help explaining what we're doing here exactly. Also not quite sure why |
||
|
|
||
| let om_resolver: Arc<dyn DNSResolverMessageHandler + Send + Sync> = match &config.hrn_config { | ||
| None => Arc::new(IgnoringMessageHandler {}), | ||
|
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. Should we just default to DNS resolution? |
||
| Some(hrn_config) => { | ||
| let runtime_handle = runtime.handle(); | ||
|
|
||
| let client_resolver: Arc<dyn DNSResolverMessageHandler + Send + Sync> = | ||
| match &hrn_config.resolution_config { | ||
| HRNResolverConfig::Blip32 => { | ||
| let hrn_res = Arc::new(LDKOnionMessageDNSSECHrnResolver::new(Arc::clone( | ||
| &network_graph, | ||
| ))); | ||
| hrn_resolver_out = Some(Arc::new(HRNResolver::Onion(Arc::clone(&hrn_res)))); | ||
|
|
||
| let pm_hook_clone = Arc::clone(&peer_manager_hook); | ||
|
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. Why do we need to clone this? |
||
| hrn_res.register_post_queue_action(Box::new(move || { | ||
| if let Ok(guard) = pm_hook_clone.lock() { | ||
| if let Some(pm) = guard.as_ref().and_then(|weak| weak.upgrade()) { | ||
| pm.process_events(); | ||
| } | ||
| } | ||
| })); | ||
| hrn_res as Arc<dyn DNSResolverMessageHandler + Send + Sync> | ||
| }, | ||
| HRNResolverConfig::Dns { dns_server_address, .. } => { | ||
| let addr = dns_server_address | ||
| .parse() | ||
| .map_err(|_| BuildError::DNSResolverSetupFailed)?; | ||
| let hrn_res = Arc::new(DNSHrnResolver(addr)); | ||
| hrn_resolver_out = Some(Arc::new(HRNResolver::Local(hrn_res))); | ||
|
|
||
| let resolver = | ||
| Arc::new(OMDomainResolver::<IgnoringMessageHandler>::with_runtime( | ||
| addr, | ||
| None, | ||
| Some(runtime_handle.clone()), | ||
| )); | ||
| resolver as Arc<dyn DNSResolverMessageHandler + Send + Sync> | ||
| }, | ||
| }; | ||
|
|
||
| if let HRNResolverConfig::Dns { | ||
|
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, shouldn't we just move this into the large |
||
| enable_hrn_resolution_service: true, | ||
| ref dns_server_address, | ||
| .. | ||
| } = hrn_config.resolution_config | ||
| { | ||
| if may_announce_channel(&config).is_ok() { | ||
| let service_dns_addr = dns_server_address | ||
| .parse() | ||
| .map_err(|_| BuildError::DNSResolverSetupFailed)?; | ||
|
|
||
| Arc::new(OMDomainResolver::with_runtime( | ||
| service_dns_addr, | ||
| Some(client_resolver), | ||
| Some(runtime_handle.clone()), | ||
|
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. I believe we can avoid cloning here? |
||
| )) as Arc<dyn DNSResolverMessageHandler + Send + Sync> | ||
| } else { | ||
| log_warn!(logger, "Unable to act as an HRN resolution service. To act as an HRN resolution service, the node must be configured to announce channels."); | ||
| client_resolver | ||
| } | ||
| } else { | ||
| client_resolver | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| // Initialize the PeerManager | ||
| let onion_messenger: Arc<OnionMessenger> = | ||
|
|
@@ -1558,7 +1638,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 { | ||
|
|
@@ -1570,7 +1650,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 {}, | ||
| )) | ||
| }; | ||
|
|
@@ -1691,7 +1771,7 @@ fn build_with_store_internal( | |
| BuildError::InvalidSystemTime | ||
| })?; | ||
|
|
||
| let peer_manager = Arc::new(PeerManager::new( | ||
| let peer_manager: Arc<PeerManager> = Arc::new(PeerManager::new( | ||
|
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. Is turbofishing really necessary here? |
||
| msg_handler, | ||
| cur_time.as_secs().try_into().map_err(|e| { | ||
| log_error!(logger, "Failed to get current time: {}", e); | ||
|
|
@@ -1702,12 +1782,11 @@ fn build_with_store_internal( | |
| Arc::clone(&keys_manager), | ||
| )); | ||
|
|
||
| let peer_manager_clone = Arc::downgrade(&peer_manager); | ||
| hrn_resolver.register_post_queue_action(Box::new(move || { | ||
| if let Some(upgraded_pointer) = peer_manager_clone.upgrade() { | ||
| upgraded_pointer.process_events(); | ||
| } | ||
| })); | ||
| if let Ok(mut guard) = peer_manager_hook.lock() { | ||
|
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. We should just |
||
| *guard = Some(Arc::downgrade(&peer_manager)); | ||
| } else { | ||
| return Err(BuildError::PeerManagerSetupFailed); | ||
| } | ||
|
|
||
| liquidity_source.as_ref().map(|l| l.set_peer_manager(Arc::downgrade(&peer_manager))); | ||
|
|
||
|
|
@@ -1814,7 +1893,7 @@ fn build_with_store_internal( | |
| node_metrics, | ||
| om_mailbox, | ||
| async_payments_role, | ||
| hrn_resolver, | ||
| hrn_resolver: Arc::new(hrn_resolver_out), | ||
| #[cfg(cycle_tests)] | ||
| _leak_checker, | ||
| }) | ||
|
|
||
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.
nit: Please avoid whitespace lines between steps.