refactor: improve channel announcement logic and fix binding tests

This commit addresses changes necessary to:
- fix failing tests for generated bindings
- remove unnecessary error variant previously introduced to capture
  failure associated with opening announced channels, and re-use
  existing variants that better capture the reasons, i.e. `InvalidNodeAlias`
  and `InvalidSocketAddress`, why opening an announced channel failed.
- correct visibility specifiers for objects, and
- cleanup nitpicks

Specific modifications across several files include:
- updating the UDL file, as well as tests related to python and kotlin
  that call `open_channel` and/or open_announced_channel
- repositioning/rearranging methods and struct fields
- introducing enums (`ChannelAnnouncementStatus` & `ChannelAnnouncementBlocker`)
  to capture and codify channel announceable eligibility, providing reasons
  for unannounceable channels
- modifying `can_announce_channel` to utilize the aforementioned enums,
  as opposed to simply returning a boolean value.
- cleaning up and renaming `connect_open_channel` to `open_channel_inner`,
  and maintaining a boolean flag for channel announcement
- updating documentation, unit, and integration tests that factor all
  these changes

Author: enigbe

bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt

@@ -175,7 +175,7 @@ class LibraryTest {
         assertEquals(100000uL, totalBalance1)
         assertEquals(100000uL, totalBalance2)
 
-        node1.connectOpenChannel(nodeId2, listenAddress2, 50000u, null, null, true)
+        node1.openChannel(nodeId2, listenAddress2, 50000u, null, null)
 
         val channelPendingEvent1 = node1.waitNextEvent()
         println("Got event: $channelPendingEvent1")

bindings/ldk_node.udl

@@ -8,6 +8,7 @@ dictionary Config {
 	string? log_dir_path;
 	Network network;
 	sequence<SocketAddress>? listening_addresses;
+	NodeAlias? node_alias;
 	u64 onchain_wallet_sync_interval_secs;
 	u64 wallet_sync_interval_secs;
 	u64 fee_rate_cache_update_interval_secs;
@@ -16,7 +17,6 @@ dictionary Config {
 	LogLevel log_level;
 	AnchorChannelsConfig? anchor_channels_config;
 	SendingParameters? sending_parameters;
-	NodeAlias? node_alias; 
 };
 
 dictionary AnchorChannelsConfig {
@@ -62,6 +62,7 @@ interface Node {
 	void event_handled();
 	PublicKey node_id();
 	sequence<SocketAddress>? listening_addresses();
+	NodeAlias? node_alias();
 	Bolt11Payment bolt11_payment();
 	Bolt12Payment bolt12_payment();
 	SpontaneousPayment spontaneous_payment();
@@ -213,7 +214,6 @@ enum NodeError {
 	"InsufficientFunds",
 	"LiquiditySourceUnavailable",
 	"LiquidityFeeTooHigh",
-	"OpenAnnouncedChannelFailed"
 };
 
 dictionary NodeStatus {

bindings/python/src/ldk_node/test_ldk_node.py

@@ -155,7 +155,7 @@ def test_channel_full_cycle(self):
         print("TOTAL 2:", total_balance_2)
         self.assertEqual(total_balance_2, 100000)
 
-        node_1.connect_open_channel(node_id_2, listening_addresses_2[0], 50000, None, None, True)
+        node_1.open_channel(node_id_2, listening_addresses_2[0], 50000, None, None)
 
         channel_pending_event_1 = node_1.wait_next_event()
         assert isinstance(channel_pending_event_1, Event.CHANNEL_PENDING)

src/builder.rs

@@ -307,21 +307,22 @@ impl NodeBuilder {
 		Ok(self)
 	}
 
-	/// Sets the level at which [`Node`] will log messages.
-	pub fn set_log_level(&mut self, level: LogLevel) -> &mut Self {
-		self.config.log_level = level;
-		self
-	}
-
-	/// Sets the alias the [`Node`] will use in its announcement. The provided
-	/// alias must be a valid UTF-8 string.
+	/// Sets the alias the [`Node`] will use in its announcement.
+	///
+	/// The provided alias must be a valid UTF-8 string.
 	pub fn set_node_alias(&mut self, node_alias: String) -> Result<&mut Self, BuildError> {
 		let node_alias = sanitize_alias(&node_alias)?;
 
 		self.config.node_alias = Some(node_alias);
 		Ok(self)
 	}
 
+	/// Sets the level at which [`Node`] will log messages.
+	pub fn set_log_level(&mut self, level: LogLevel) -> &mut Self {
+		self.config.log_level = level;
+		self
+	}
+
 	/// Builds a [`Node`] instance with a [`SqliteStore`] backend and according to the options
 	/// previously configured.
 	pub fn build(&self) -> Result<Node, BuildError> {
@@ -514,16 +515,16 @@ impl ArcedNodeBuilder {
 		self.inner.write().unwrap().set_listening_addresses(listening_addresses).map(|_| ())
 	}
 
-	/// Sets the level at which [`Node`] will log messages.
-	pub fn set_log_level(&self, level: LogLevel) {
-		self.inner.write().unwrap().set_log_level(level);
-	}
-
 	/// Sets the node alias.
 	pub fn set_node_alias(&self, node_alias: String) -> Result<(), BuildError> {
 		self.inner.write().unwrap().set_node_alias(node_alias).map(|_| ())
 	}
 
+	/// Sets the level at which [`Node`] will log messages.
+	pub fn set_log_level(&self, level: LogLevel) {
+		self.inner.write().unwrap().set_log_level(level);
+	}
+
 	/// Builds a [`Node`] instance with a [`SqliteStore`] backend and according to the options
 	/// previously configured.
 	pub fn build(&self) -> Result<Arc<Node>, BuildError> {
@@ -1070,7 +1071,7 @@ fn seed_bytes_from_config(
 }
 
 /// Sanitize the user-provided node alias to ensure that it is a valid protocol-specified UTF-8 string.
-pub fn sanitize_alias(alias_str: &str) -> Result<NodeAlias, BuildError> {
+pub(crate) fn sanitize_alias(alias_str: &str) -> Result<NodeAlias, BuildError> {
 	let alias = alias_str.trim();
 
 	// Alias must be 32-bytes long or less.
@@ -1085,9 +1086,7 @@ pub fn sanitize_alias(alias_str: &str) -> Result<NodeAlias, BuildError> {
 
 #[cfg(test)]
 mod tests {
-	use lightning::routing::gossip::NodeAlias;
-
-	use crate::{builder::sanitize_alias, BuildError};
+	use super::{sanitize_alias, BuildError, NodeAlias};
 
 	#[test]
 	fn sanitize_empty_node_alias() {

src/config.rs

@@ -87,6 +87,7 @@ pub(crate) const WALLET_KEYS_SEED_LEN: usize = 64;
 /// | `log_dir_path`                         | None               |
 /// | `network`                              | Bitcoin            |
 /// | `listening_addresses`                  | None               |
+/// | `node_alias`                  		 | None               |
 /// | `default_cltv_expiry_delta`            | 144                |
 /// | `onchain_wallet_sync_interval_secs`    | 80                 |
 /// | `wallet_sync_interval_secs`            | 30                 |
@@ -112,9 +113,14 @@ pub struct Config {
 	pub network: Network,
 	/// The addresses on which the node will listen for incoming connections.
 	///
-	/// **Note**: Node announcements will only be broadcast if the node_alias and the
-	/// listening_addresses are set.
+	/// **Note**: Node announcements will only be broadcast if the `node_alias` and the
+	/// `listening_addresses` are set.
 	pub listening_addresses: Option<Vec<SocketAddress>>,
+	/// The node alias to be used in announcements.
+	///
+	/// **Note**: Node announcements will only be broadcast if the `node_alias` and the
+	/// `listening_addresses` are set.
+	pub node_alias: Option<NodeAlias>,
 	/// The time in-between background sync attempts of the onchain wallet, in seconds.
 	///
 	/// **Note:** A minimum of 10 seconds is always enforced.
@@ -167,11 +173,6 @@ 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 sending_parameters: Option<SendingParameters>,
-	/// The node alias to be used in announcements.
-	///
-	/// **Note**: Node announcements will only be broadcast if the node_alias and the
-	/// listening_addresses are set.
-	pub node_alias: Option<NodeAlias>,
 }
 
 impl Default for Config {
@@ -275,33 +276,68 @@ pub fn default_config() -> Config {
 	Config::default()
 }
 
+/// Specifies reasons why a channel cannot be announced.
+#[derive(Debug, PartialEq)]
+pub(crate) enum ChannelAnnouncementBlocker {
+	/// The node alias is not set.
+	MissingNodeAlias,
+	/// The listening addresses are not set.
+	MissingListeningAddresses,
+	// This listening addresses is set but the vector is empty.
+	EmptyListeningAddresses,
+}
+
+/// Enumeration defining the announcement status of a channel.
+#[derive(Debug, PartialEq)]
+pub(crate) enum ChannelAnnouncementStatus {
+	/// The channel is announceable.
+	Announceable,
+	/// The channel is not announceable.
+	Unannounceable(ChannelAnnouncementBlocker),
+}
+
 /// Checks if a node is can announce a channel based on the configured values of both the node's
-/// alias and its listening addresses. If either of them is unset, the node cannot announce the
-/// channel.
-pub fn can_announce_channel(config: &Config) -> bool {
-	let are_addresses_set =
-		config.listening_addresses.clone().map_or(false, |addr_vec| !addr_vec.is_empty());
-	let is_alias_set = config.node_alias.is_some();
-
-	is_alias_set && are_addresses_set
+/// alias and its listening addresses.
+///
+/// If either of them is unset, the node cannot announce the channel. This ability to announce/
+/// unannounce a channel is codified with `ChannelAnnouncementStatus`
+pub(crate) fn can_announce_channel(config: &Config) -> ChannelAnnouncementStatus {
+	if config.node_alias.is_none() {
+		return ChannelAnnouncementStatus::Unannounceable(
+			ChannelAnnouncementBlocker::MissingNodeAlias,
+		);
+	}
+
+	match &config.listening_addresses {
+		None => ChannelAnnouncementStatus::Unannounceable(
+			ChannelAnnouncementBlocker::MissingListeningAddresses,
+		),
+		Some(addresses) if addresses.is_empty() => ChannelAnnouncementStatus::Unannounceable(
+			ChannelAnnouncementBlocker::EmptyListeningAddresses,
+		),
+		Some(_) => ChannelAnnouncementStatus::Announceable,
+	}
 }
 
 pub(crate) fn default_user_config(config: &Config) -> UserConfig {
 	// Initialize the default config values.
 	//
-	// Note that methods such as Node::connect_open_channel might override some of the values set
-	// here, e.g. the ChannelHandshakeConfig, meaning these default values will mostly be relevant
-	// for inbound channels.
+	// Note that methods such as Node::open_channel and Node::open_announced_channel might override
+	// some of the values set here, e.g. the ChannelHandshakeConfig, meaning these default values
+	// will mostly be relevant for inbound channels.
 	let mut user_config = UserConfig::default();
 	user_config.channel_handshake_limits.force_announced_channel_preference = false;
 	user_config.manually_accept_inbound_channels = true;
 	user_config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx =
 		config.anchor_channels_config.is_some();
 
-	if !can_announce_channel(config) {
-		user_config.accept_forwards_to_priv_channels = false;
-		user_config.channel_handshake_config.announced_channel = false;
-		user_config.channel_handshake_limits.force_announced_channel_preference = true;
+	match can_announce_channel(config) {
+		ChannelAnnouncementStatus::Announceable => (),
+		ChannelAnnouncementStatus::Unannounceable(_) => {
+			user_config.accept_forwards_to_priv_channels = false;
+			user_config.channel_handshake_config.announced_channel = false;
+			user_config.channel_handshake_limits.force_announced_channel_preference = true;
+		},
 	}
 
 	user_config
@@ -311,17 +347,23 @@ pub(crate) fn default_user_config(config: &Config) -> UserConfig {
 mod tests {
 	use std::str::FromStr;
 
-	use lightning::{ln::msgs::SocketAddress, routing::gossip::NodeAlias};
-
-	use crate::config::can_announce_channel;
+	use crate::config::ChannelAnnouncementStatus;
 
+	use super::can_announce_channel;
 	use super::Config;
+	use super::NodeAlias;
+	use super::SocketAddress;
 
 	#[test]
 	fn node_can_announce_channel() {
 		// Default configuration with node alias and listening addresses unset
 		let mut node_config = Config::default();
-		assert_eq!(can_announce_channel(&node_config), false);
+		assert_eq!(
+			can_announce_channel(&node_config),
+			ChannelAnnouncementStatus::Unannounceable(
+				crate::config::ChannelAnnouncementBlocker::MissingNodeAlias
+			)
+		);
 
 		// Set node alias with listening addresses unset
 		let alias_frm_str = |alias: &str| {
@@ -330,18 +372,28 @@ mod tests {
 			NodeAlias(bytes)
 		};
 		node_config.node_alias = Some(alias_frm_str("LDK_Node"));
-		assert_eq!(can_announce_channel(&node_config), false);
+		assert_eq!(
+			can_announce_channel(&node_config),
+			ChannelAnnouncementStatus::Unannounceable(
+				crate::config::ChannelAnnouncementBlocker::MissingListeningAddresses
+			)
+		);
 
 		// Set node alias with an empty list of listening addresses
 		node_config.listening_addresses = Some(vec![]);
-		assert_eq!(can_announce_channel(&node_config), false);
+		assert_eq!(
+			can_announce_channel(&node_config),
+			ChannelAnnouncementStatus::Unannounceable(
+				crate::config::ChannelAnnouncementBlocker::EmptyListeningAddresses
+			)
+		);
 
 		// Set node alias with a non-empty list of listening addresses
 		let socket_address =
 			SocketAddress::from_str("localhost:8000").expect("Socket address conversion failed.");
 		if let Some(ref mut addresses) = node_config.listening_addresses {
 			addresses.push(socket_address);
 		}
-		assert_eq!(can_announce_channel(&node_config), true);
+		assert_eq!(can_announce_channel(&node_config), ChannelAnnouncementStatus::Announceable);
 	}
 }

src/error.rs

@@ -110,10 +110,6 @@ pub enum Error {
 	LiquiditySourceUnavailable,
 	/// The given operation failed due to the LSP's required opening fee being too high.
 	LiquidityFeeTooHigh,
-	/// Returned when trying to open an announced channel with a peer. This
-	/// error occurs when a [`crate::Node`'s] alias or listening addresses
-	/// are unconfigured.
-	OpenAnnouncedChannelFailed,
 }
 
 impl fmt::Display for Error {
@@ -185,7 +181,6 @@ impl fmt::Display for Error {
 			Self::LiquidityFeeTooHigh => {
 				write!(f, "The given operation failed due to the LSP's required opening fee being too high.")
 			},
-			Self::OpenAnnouncedChannelFailed => write!(f, "Failed to open an announced channel."),
 		}
 	}
 }