-
Notifications
You must be signed in to change notification settings - Fork 99
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
Support separate node announcement addresses #484
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 |
---|---|---|
|
@@ -7,8 +7,8 @@ | |
|
||
use crate::chain::{ChainSource, DEFAULT_ESPLORA_SERVER_URL}; | ||
use crate::config::{ | ||
default_user_config, Config, EsploraSyncConfig, DEFAULT_LOG_FILENAME, DEFAULT_LOG_LEVEL, | ||
WALLET_KEYS_SEED_LEN, | ||
default_user_config, may_announce_channel, AnnounceError, Config, EsploraSyncConfig, | ||
DEFAULT_LOG_FILENAME, DEFAULT_LOG_LEVEL, WALLET_KEYS_SEED_LEN, | ||
}; | ||
|
||
use crate::connection::ConnectionManager; | ||
|
@@ -32,8 +32,7 @@ use crate::types::{ | |
}; | ||
use crate::wallet::persist::KVStoreWalletPersister; | ||
use crate::wallet::Wallet; | ||
use crate::Node; | ||
use crate::{io, NodeMetrics}; | ||
use crate::{io, Node, NodeMetrics}; | ||
|
||
use lightning::chain::{chainmonitor, BestBlock, Watch}; | ||
use lightning::io::Cursor; | ||
|
@@ -148,6 +147,8 @@ pub enum BuildError { | |
InvalidChannelMonitor, | ||
/// The given listening addresses are invalid, e.g. too many were passed. | ||
InvalidListeningAddresses, | ||
/// The given announcement addresses are invalid, e.g. too many were passed. | ||
InvalidAnnouncementAddresses, | ||
/// The provided alias is invalid. | ||
InvalidNodeAlias, | ||
/// We failed to read data from the [`KVStore`]. | ||
|
@@ -184,6 +185,9 @@ impl fmt::Display for BuildError { | |
write!(f, "Failed to watch a deserialized ChannelMonitor") | ||
}, | ||
Self::InvalidListeningAddresses => write!(f, "Given listening addresses are invalid."), | ||
Self::InvalidAnnouncementAddresses => { | ||
write!(f, "Given announcement addresses are invalid.") | ||
}, | ||
Self::ReadFailed => write!(f, "Failed to read from store."), | ||
Self::WriteFailed => write!(f, "Failed to write to store."), | ||
Self::StoragePathAccessFailed => write!(f, "Failed to access the given storage path."), | ||
|
@@ -413,6 +417,22 @@ impl NodeBuilder { | |
Ok(self) | ||
} | ||
|
||
/// Sets the IP address and TCP port which [`Node`] will announce to the gossip network that it accepts connections on. | ||
/// | ||
/// **Note**: If unset, the [`listening_addresses`] will be used as the list of addresses to announce. | ||
/// | ||
/// [`listening_addresses`]: Self::set_listening_addresses | ||
pub fn set_announcement_addresses( | ||
&mut self, announcement_addresses: Vec<SocketAddress>, | ||
) -> Result<&mut Self, BuildError> { | ||
if announcement_addresses.len() > 100 { | ||
return Err(BuildError::InvalidAnnouncementAddresses); | ||
} | ||
|
||
self.config.announcement_addresses = Some(announcement_addresses); | ||
Ok(self) | ||
} | ||
|
||
/// Sets the node alias that will be used when broadcasting announcements to the gossip | ||
/// network. | ||
/// | ||
|
@@ -776,6 +796,17 @@ impl ArcedNodeBuilder { | |
self.inner.write().unwrap().set_listening_addresses(listening_addresses).map(|_| ()) | ||
} | ||
|
||
/// Sets the IP address and TCP port which [`Node`] will announce to the gossip network that it accepts connections on. | ||
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. Please align the docs on the |
||
/// | ||
/// **Note**: If unset, the [`listening_addresses`] will be used as the list of addresses to announce. | ||
/// | ||
/// [`listening_addresses`]: Self::set_listening_addresses | ||
pub fn set_announcement_addresses( | ||
&self, announcement_addresses: Vec<SocketAddress>, | ||
) -> Result<(), BuildError> { | ||
self.inner.write().unwrap().set_announcement_addresses(announcement_addresses).map(|_| ()) | ||
} | ||
|
||
/// Sets the node alias that will be used when broadcasting announcements to the gossip | ||
/// network. | ||
/// | ||
|
@@ -880,6 +911,23 @@ fn build_with_store_internal( | |
liquidity_source_config: Option<&LiquiditySourceConfig>, seed_bytes: [u8; 64], | ||
logger: Arc<Logger>, kv_store: Arc<DynStore>, | ||
) -> Result<Node, BuildError> { | ||
if let Err(err) = may_announce_channel(&config) { | ||
if config.announcement_addresses.is_some() { | ||
log_error!(logger, "Announcement addresses were set but some required configuration options for node announcement are missing: {}", err); | ||
let build_error = if matches!(err, AnnounceError::MissingNodeAlias) { | ||
BuildError::InvalidNodeAlias | ||
} else { | ||
BuildError::InvalidListeningAddresses | ||
}; | ||
return Err(build_error); | ||
} | ||
|
||
if config.node_alias.is_some() { | ||
log_error!(logger, "Node alias was set but some required configuration options for node announcement are missing: {}", err); | ||
return Err(BuildError::InvalidListeningAddresses); | ||
} | ||
} | ||
|
||
// Initialize the status fields. | ||
let is_listening = Arc::new(AtomicBool::new(false)); | ||
let node_metrics = match read_node_metrics(Arc::clone(&kv_store), Arc::clone(&logger)) { | ||
|
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. You'll also need to account for the new field in 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 looked at this but I believe the 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. Ah, you are right! In fact, could you extend the May also be good to add a check for this and log+abort early in 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. Both of those suggestions sound good. For the misconfigured node do we want a new error variant or should I re-use the existing Also, I think the check should not only be against unset listening_addresses but actually be Basically if you set announcement addresses but the node will not announce then it's misconfigured. 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.
Yeah, though
Yeah, that makes sense. FWIW, could do the same for 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. Well if the check is Will add the same check for NodeAlias. 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. Well, maybe we should switch 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. Gave this a shot in the latest commit. The error message handling feels a bit verbose now but I tried to dry it up best I could. Check it out and let me know what you think. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -414,7 +414,7 @@ impl Node { | |
let bcast_node_metrics = Arc::clone(&self.node_metrics); | ||
let mut stop_bcast = self.stop_sender.subscribe(); | ||
let node_alias = self.config.node_alias.clone(); | ||
if may_announce_channel(&self.config) { | ||
if may_announce_channel(&self.config).is_ok() { | ||
runtime.spawn(async move { | ||
// We check every 30 secs whether our last broadcast is NODE_ANN_BCAST_INTERVAL away. | ||
#[cfg(not(test))] | ||
|
@@ -457,8 +457,10 @@ impl Node { | |
continue; | ||
} | ||
|
||
let addresses = if let Some(addresses) = bcast_config.listening_addresses.clone() { | ||
addresses | ||
let addresses = if let Some(announcement_addresses) = bcast_config.announcement_addresses.clone() { | ||
announcement_addresses | ||
} else if let Some(listening_addresses) = bcast_config.listening_addresses.clone() { | ||
listening_addresses | ||
} else { | ||
debug_assert!(false, "We checked whether the node may announce, so listening addresses should always be set"); | ||
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 log will need to be updated. 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'm not sure it does because listening_addresses is the only thing that should always be set. |
||
continue; | ||
|
@@ -802,6 +804,14 @@ impl Node { | |
self.config.listening_addresses.clone() | ||
} | ||
|
||
/// Returns the addresses that the node will announce to the network. | ||
pub fn announcement_addresses(&self) -> Option<Vec<SocketAddress>> { | ||
self.config | ||
.announcement_addresses | ||
.clone() | ||
.or_else(|| self.config.listening_addresses.clone()) | ||
} | ||
|
||
/// Returns our node alias. | ||
pub fn node_alias(&self) -> Option<NodeAlias> { | ||
self.config.node_alias | ||
|
@@ -1201,19 +1211,19 @@ impl Node { | |
&self, node_id: PublicKey, address: SocketAddress, channel_amount_sats: u64, | ||
push_to_counterparty_msat: Option<u64>, channel_config: Option<ChannelConfig>, | ||
) -> Result<UserChannelId, Error> { | ||
if may_announce_channel(&self.config) { | ||
self.open_channel_inner( | ||
node_id, | ||
address, | ||
channel_amount_sats, | ||
push_to_counterparty_msat, | ||
channel_config, | ||
true, | ||
) | ||
} else { | ||
log_error!(self.logger, "Failed to open announced channel as the node hasn't been sufficiently configured to act as a forwarding node. Please make sure to configure listening addreesses and node alias"); | ||
if let Err(err) = may_announce_channel(&self.config) { | ||
log_error!(self.logger, "Failed to open announced channel as the node hasn't been sufficiently configured to act as a forwarding node: {}", err); | ||
return Err(Error::ChannelCreationFailed); | ||
} | ||
|
||
self.open_channel_inner( | ||
node_id, | ||
address, | ||
channel_amount_sats, | ||
push_to_counterparty_msat, | ||
channel_config, | ||
true, | ||
) | ||
} | ||
|
||
/// Manually sync the LDK and BDK wallets with the current chain state and update the fee rate | ||
|
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.
should we add the 'if unset' note here as well?