Set node alias #330

enigbe · 2024-07-25T17:39:35Z

What this PR does:

Adds a function to allow users set the node alias.
If set, ensures that the provided alias is broadcast with the node announcement

Related issue(s):

Allow to set node alias #304

tnull

Excuse the delay here!

Thank you for looking into this, approach already looks pretty good! Just a few comments.

tnull · 2024-07-30T18:33:48Z

src/builder.rs

@@ -132,6 +132,8 @@ pub enum BuildError {
 	WalletSetupFailed,
 	/// We failed to setup the logger.
 	LoggerSetupFailed,
+	/// The provided alias is invalid
+	InvalidNodeAlias(String),


Ah, to easily make the error type work with our language bindings, this should be a simple unit variant, i.e., we need to drop the String parameter.

also, nit: Could you move that up to the other Invalid variants?

I have simplified the error variant. Initially I wanted to provide context to the caller so they can be informed about why the provided alias is being rejected, but I ran into problems with the language bindings.

src/builder.rs

src/lib.rs

tnull

Thanks! I think beyond checking for the node alias and listening addresses before broadcasting, we should also modify the default_user_config method in Config to, if any of the two are unconfigured:

a) set UserConfig::accept_forwards_to_priv_channels to false
b) set ChannelHandshakeConfig::announced_channel to false
c) set ChannelHandshakeLimits::force_announced_channel_preference to true

Additionally, we'll want to return an error from connect_open_channel if any of the two are unconfigured and we try to open an announced channel. To this end, it might be bit cleaner if we'd split the method in two (open_channel and open_announced_channel) rather than having it take a simple bool.

TLDR: We should use the configured listening addresses and node alias as indicators whether we are a forwarding node or not. If we're not, we should disallow announced channels.

Let me know if you'd be up for these additional changes, otherwise I'm happy to pick them up as a follow-up.

tnull · 2024-08-07T09:22:32Z

README.md

@@ -60,6 +60,9 @@ LDK Node currently comes with a decidedly opinionated set of design choices:
 - Gossip data may be sourced via Lightning's peer-to-peer network or the [Rapid Gossip Sync](https://docs.rs/lightning-rapid-gossip-sync/*/lightning_rapid_gossip_sync/) protocol.
 - Entropy for the Lightning and on-chain wallets may be sourced from raw bytes or a [BIP39](https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki) mnemonic. In addition, LDK Node offers the means to generate and persist the entropy bytes to disk.

+**Note**: 
+Regarding node announcements, we have decided not to broadcast these announcements if either the node's listening addresses or its node alias are not set.


Hmm, I don't think this belongs here in the main readme. But, we should add a comment to that effect in the Rust docs.

I agree too. I wasn't sure where it belonged but this comment gave me a hint.

tnull · 2024-08-07T09:25:13Z

src/config.rs

@@ -147,6 +147,11 @@ pub struct Config {
 	/// closure. We *will* however still try to get the Anchor spending transactions confirmed
 	/// on-chain with the funds available.
 	pub anchor_channels_config: Option<AnchorChannelsConfig>,
+	/// The node alias to be used in announcements.
+	///
+	/// **Note**: This is required if, alongside a valid public socket address, node announcements


nit: Can we reformulate that to make it a bit clearer: "Node announcements will only be broadcast if .." and also add the comment to listening_addresses?

src/lib.rs

tnull · 2024-08-07T09:41:05Z

src/lib.rs

@@ -646,13 +647,18 @@ impl Node {
 							}

 							let addresses = bcast_config.listening_addresses.clone().unwrap_or(Vec::new());
+							let alias = node_alias.clone().map(|alias| {
+								let mut buf = [0_u8; 32];
+								buf[..alias.as_bytes().len()].copy_from_slice(alias.as_bytes());


This array access introduces a reachable panic (index out of range) as we check the length of alias when set via set_node_alias, but not if set in the Config directly.

I tend towards not actually storing as a String in Config, but as LDK's lightning::routing::gossip::NodeAlias type and then providing a (sanitizing) UniffiCustomTypeConverter implementation for bindings (see

ldk-node/src/uniffi_types.rs

Line 266 in 952b889

impl UniffiCustomTypeConverter for Txid {

). This would ensure the alias can never be above 32 bytes, and we'd provide sanitization when converting between String and NodeAlias([u8; 32]). What do you think?

I do agree with this approach. At the time I submitted the PR I was still getting familiar with the codebase and didn't know about the custom type converter. I have implemented it for NodeAlias with the necessary sanitization to ensure that both methods of setting the node alias are safe.

tnull · 2024-08-07T09:41:58Z

src/lib.rs

 								continue;
 							}

-							bcast_pm.broadcast_node_announcement([0; 3], [0; 32], addresses);
+							bcast_pm.broadcast_node_announcement([0; 3], alias.unwrap(), addresses);


Please avoid introducing any unwraps, even if they are obviously safe right now. Just use if let Some() and continue in the else case.

tnull · 2024-08-07T09:47:06Z

src/lib.rs


-							if addresses.is_empty() {
-								// Skip if we are not listening on any addresses.
+							if addresses.is_empty() || alias.is_none() {


I think we could move the static checks on config.addresses.is_empty() and config.node_alias.is_none() up before actually spawning the task. As currently the Config isn't mutable during runtime, there is really no reason we need to spawn the task and periodically run through all these checks if any of the two are unset.

enigbe · 2024-08-07T11:56:23Z

Thanks! I think beyond checking for the node alias and listening addresses before broadcasting, we should also modify the default_user_config method in Config to, if any of the two are unconfigured:

a) set UserConfig::accept_forwards_to_priv_channels to false b) set ChannelHandshakeConfig::announced_channel to false c) set ChannelHandshakeLimits::force_announced_channel_preference to true

Additionally, we'll want to return an error from connect_open_channel if any of the two are unconfigured and we try to open an announced channel. To this end, it might be bit cleaner if we'd split the method in two (open_channel and open_announced_channel) rather than having it take a simple bool.

TLDR: We should use the configured listening addresses and node alias as indicators whether we are a forwarding node or not. If we're not, we should disallow announced channels.

Let me know if you'd be up for these additional changes, otherwise I'm happy to pick them up as a follow-up.

Thanks for the review. I am happy to keep working on this until it is ready.

tnull · 2024-08-27T08:20:03Z

Thanks for the review. I am happy to keep working on this until it is ready.

Thanks again! Do you still intend to do this in this PR, or would you prefer we land this and do additional changes in a follow up?

Btw. this also needs a rebase already, and there will be larger code changes upcoming in the LDK/BDK/rust-bitcoin upgrade PR, which will likely also lead to further rebase conflicts.

enigbe · 2024-08-27T09:14:35Z

Thanks for the review. I am happy to keep working on this until it is ready.

Thanks again! Do you still intend to do this in this PR, or would you prefer we land this and do additional changes in a follow up?

Btw. this also needs a rebase already, and there will be larger code changes upcoming in the LDK/BDK/rust-bitcoin upgrade PR, which will likely also lead to further rebase conflicts.

Apologies for the delay. I am almost done and will be pushing changes today. I'll rebase and get it ready for another look before the end of the day.

tnull · 2024-08-27T09:27:21Z

Apologies for the delay. I am almost done and will be pushing changes today. I'll rebase and get it ready for another look before the end of the day.

No worries! Cool, thank you!

What this commit does: Implements a method `set_node_alias` on NodeBuilder to allow callers customize/set the value of the node alias. This method sanitizes the user-provided alias by ensuring the following: + Node alias is UTF-8-encoded String + Node alias is non-empty + Node alias cannot exceed 32 bytes + Node alias is only valid up to the first null byte. Every character after the null byte is discraded Additionally, a test case is provided to cover sanitizing empty node alias, as well as an alias with emojis (copied and modified from rust-lightning) and a sandwiched null byte.

What this commit does: Broadcasts node announcement with the user-provided alias, if set, else, uses the default [0u8;32]. Additionally, adds a random node alias generating function for use in the generation of random configuration.

alias sanitization - Skips broadcasting node announcement in the event that either the node alias or the listening addresses are not set. - Aligns the InvalidNodeAlias error variant with the others to make it work with language bindings. - Simplifies the method to set the node alias. - Cleans up the alias sanitizing function to ensure that protocol- compliant aliases (in this case, empty strings) are not flagged. Additionally, removes the check for sandwiched null byte. - Finally, adds the relevant update to struct and interface to reflect changes in Rust types.

…onfigured

What this commit does: + Updates the sanitization function for node alias to return NodeAlias + Updates the node alias type in the configuration to NodeAlias and implements a conversion to/from String for bindings + With this update, regardless of where the alias is set, i.e. in the set_node_alias or directly, sanitization occurs.

What this commit does: + Decomposes connect_open_channel into two different functions: open_channel and open_announced_channel. This allows opening announced channels based on configured node alias and listening addresses values. + This enforces channel announcement only on the condition that both configuration values are set. + Additionally, a new error variant `OpenAnnouncedChannelFailed` is introduced to capture failure. Note: I thought I added the `InvalidNodeAlias` variant in the previous commit

What this commit does: + Replaces calls to `connect_open_channel` with `open_channel` and `open_announced_channel` where appropriate. Status: Work In Progress (WIP) Observation: + The integration tests are now flaky and need further investigation to ascertain the reason(s) why and then to fix.

What this commit does: + Removes the wrapping Arc from the channel config. This is a missed update after rebasing.

enigbe · 2024-08-29T09:10:54Z

Thanks for the review. I am happy to keep working on this until it is ready.

Thanks again! Do you still intend to do this in this PR, or would you prefer we land this and do additional changes in a follow up?
Btw. this also needs a rebase already, and there will be larger code changes upcoming in the LDK/BDK/rust-bitcoin upgrade PR, which will likely also lead to further rebase conflicts.

Apologies for the delay. I am almost done and will be pushing changes today. I'll rebase and get it ready for another look before the end of the day.

Hi @tnull, I have rebased and pushed recommended changes. Unfortunately, the integration tests are flaky and I am uncertain about why. Currently investigating.

enigbe · 2024-09-05T08:55:11Z

Thanks for the review. I am happy to keep working on this until it is ready.

Thanks again! Do you still intend to do this in this PR, or would you prefer we land this and do additional changes in a follow up?
Btw. this also needs a rebase already, and there will be larger code changes upcoming in the LDK/BDK/rust-bitcoin upgrade PR, which will likely also lead to further rebase conflicts.

Apologies for the delay. I am almost done and will be pushing changes today. I'll rebase and get it ready for another look before the end of the day.

Hi @tnull, I have rebased and pushed recommended changes. Unfortunately, the integration tests are flaky and I am uncertain about why. Currently investigating.

Hi @tnull, thanks for your review so far. Have you had some time to check out the recent changes I have made? I have addressed the comments raised and would be grateful for any further feedback or recommendation.

tnull

Hi @tnull, thanks for your review so far. Have you had some time to check out the recent changes I have made? I have addressed the comments raised and would be grateful for any further feedback or recommendation.

Excuse the delay here! Did another round, already looks pretty good, just a few comments.

To make CI pass you'll need to adjust all prior uses of connect_open_channel/connectOpenChannel in the bindings tests (i.e., in bindings/kotlin/ldk-node-jvm/lib/src/test/kotlin/org/lightningdevkit/ldknode/LibraryTest.kt for Kotlin and bindings/python/src/ldk_node/test_ldk_node.py for Python).

There are also a few references to connect_open_channel left in tests and comments which should be renamed accordingly.

src/builder.rs

tnull · 2024-09-06T09:11:28Z

src/builder.rs

+mod tests {
+	use lightning::routing::gossip::NodeAlias;
+
+	use crate::{builder::sanitize_alias, BuildError};


nit: Given these tests are in a sub-module of builder, I think you don't need to import these types at all here?

tnull · 2024-09-06T09:12:41Z

src/config.rs

@@ -103,6 +104,9 @@ pub struct Config {
 	/// The used Bitcoin network.
 	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


nit: Please tick node_alias and listening_addresses here and below, as they refer to field names.

tnull · 2024-09-06T09:14:04Z

src/config.rs

+	///
+	/// **Note**: Node announcements will only be broadcast if the node_alias and the
+	/// listening_addresses are set.
+	pub node_alias: Option<NodeAlias>,


Could we move this up, just after listening_addresses? Also, could you add it (and the used default value) to the table in the docs above?

tnull · 2024-09-06T09:23:45Z

src/lib.rs

@@ -116,15 +117,17 @@ pub use io::utils::generate_entropy_mnemonic;
 #[cfg(feature = "uniffi")]
 use uniffi_types::*;

+pub use builder::sanitize_alias;


As mentioned above, please drop this export.

tnull · 2024-09-06T09:26:13Z

src/lib.rs

@@ -646,13 +651,20 @@ impl Node {
 							}

 							let addresses = bcast_config.listening_addresses.clone().unwrap_or(Vec::new());
+							let alias = node_alias.clone().map(|alias| {


Not quite sure why this is necessary? Could just do this below:

if let Some(node_alias) = node_alias.as_ref() { bcast_pm.broadcast_node_announcement([0; 3], node_alias.0, addresses); } else { continue }

tnull · 2024-09-06T09:29:20Z

src/lib.rs

@@ -1185,10 +1197,9 @@ impl Node {
 	/// opening the channel.
 	///
 	/// Returns a [`UserChannelId`] allowing to locally keep track of the channel.
-	pub fn connect_open_channel(
+	fn connect_open_channel(


Let's rename this open_channel_internal and let's keep the announce_channel flag.

open_channel and open_announced_channel can then just call open_channel_internal setting the flag accordingly.

tnull · 2024-09-06T09:31:23Z

src/lib.rs

@@ -1250,12 +1261,13 @@ impl Node {
 		}

 		let mut user_config = default_user_config(&self.config);
-		user_config.channel_handshake_config.announced_channel = announce_channel;
+		let can_announce_channel = can_announce_channel(&self.config);
+		user_config.channel_handshake_config.announced_channel = can_announce_channel;


This would mean that we would always open announced channels when we can. But announced nodes may also want to open private channels. As mentioned above, we should just keep the announced_channel in this method, but error out if it's set while can_announce_channel is false.

tnull · 2024-09-06T09:33:07Z

src/lib.rs

@@ -1288,6 +1300,38 @@ impl Node {
 		}
 	}

+	/// Opens a channel with a peer.


Please move the docs from connect_open_channel here and also copy them to open_announced_channel.

enigbe mentioned this pull request Jul 25, 2024

Allow to set node alias #304

Open

enigbe changed the title ~~Set node alias~~ WIP: Set node alias Jul 29, 2024

tnull reviewed Jul 30, 2024

View reviewed changes

enigbe force-pushed the feat-set-node-alias branch from 63e791a to 8791d7c Compare August 6, 2024 12:35

enigbe changed the title ~~WIP: Set node alias~~ Set node alias Aug 6, 2024

enigbe marked this pull request as ready for review August 6, 2024 14:48

tnull reviewed Aug 7, 2024

View reviewed changes

enigbe added 10 commits August 29, 2024 08:29

feat: broadcast node announcement with set alias

eb9b5e6

What this commit does: Broadcasts node announcement with the user-provided alias, if set, else, uses the default [0u8;32]. Additionally, adds a random node alias generating function for use in the generation of random configuration.

docs: clarify node announcement broadcast logic

24ef2ec

refactor: update UserConfig if node alias/listening addresses are unc…

b37cf01

…onfigured

docs: cleanup documentation

fc5e626

fix: change channel config type in open_(announced)_channel

3775bad

What this commit does: + Removes the wrapping Arc from the channel config. This is a missed update after rebasing.

enigbe force-pushed the feat-set-node-alias branch from cfd764b to 3775bad Compare August 29, 2024 07:41

enigbe added 2 commits August 29, 2024 09:17

fix: remove broken intra doc link

461a23f

fix: remove unstable feature is_some_and

25089e3

enigbe requested a review from tnull August 29, 2024 09:07

tnull reviewed Sep 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set node alias #330

Set node alias #330

enigbe commented Jul 25, 2024

tnull left a comment

tnull Jul 30, 2024

enigbe Aug 6, 2024

tnull left a comment

tnull Aug 7, 2024

enigbe Aug 29, 2024

tnull Aug 7, 2024

tnull Aug 7, 2024

enigbe Aug 29, 2024

tnull Aug 7, 2024

tnull Aug 7, 2024

enigbe commented Aug 7, 2024

tnull commented Aug 27, 2024 •

edited

Loading

enigbe commented Aug 27, 2024

tnull commented Aug 27, 2024

enigbe commented Aug 29, 2024

enigbe commented Sep 5, 2024

tnull left a comment

tnull Sep 6, 2024

tnull Sep 6, 2024

tnull Sep 6, 2024

tnull Sep 6, 2024

tnull Sep 6, 2024

tnull Sep 6, 2024

tnull Sep 6, 2024

tnull Sep 6, 2024

Set node alias #330

Are you sure you want to change the base?

Set node alias #330

Conversation

enigbe commented Jul 25, 2024

What this PR does:

Related issue(s):

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enigbe commented Aug 7, 2024

tnull commented Aug 27, 2024 • edited Loading

enigbe commented Aug 27, 2024

tnull commented Aug 27, 2024

enigbe commented Aug 29, 2024

enigbe commented Sep 5, 2024

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Aug 27, 2024 •

edited

Loading