-
Notifications
You must be signed in to change notification settings - Fork 72
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
Set node alias #330
base: main
Are you sure you want to change the base?
Set node alias #330
Conversation
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.
Excuse the delay here!
Thank you for looking into this, approach already looks pretty good! Just a few comments.
src/builder.rs
Outdated
@@ -132,6 +132,8 @@ pub enum BuildError { | |||
WalletSetupFailed, | |||
/// We failed to setup the logger. | |||
LoggerSetupFailed, | |||
/// The provided alias is invalid | |||
InvalidNodeAlias(String), |
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.
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?
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.
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.
63e791a
to
8791d7c
Compare
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! 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.
README.md
Outdated
@@ -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. |
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.
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.
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.
I agree too. I wasn't sure where it belonged but this comment gave me a hint.
src/config.rs
Outdated
@@ -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 |
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: 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
Outdated
@@ -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()); |
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.
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
Line 266 in 952b889
impl UniffiCustomTypeConverter for Txid { |
String
and NodeAlias([u8; 32])
. What do you think?
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.
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.
src/lib.rs
Outdated
continue; | ||
} | ||
|
||
bcast_pm.broadcast_node_announcement([0; 3], [0; 32], addresses); | ||
bcast_pm.broadcast_node_announcement([0; 3], alias.unwrap(), addresses); |
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.
Please avoid introducing any unwraps, even if they are obviously safe right now. Just use if let Some()
and continue
in the else
case.
src/lib.rs
Outdated
|
||
if addresses.is_empty() { | ||
// Skip if we are not listening on any addresses. | ||
if addresses.is_empty() || alias.is_none() { |
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.
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.
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. |
No worries! Cool, thank you! |
cfd764b
to
3775bad
Compare
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. |
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.
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
Outdated
mod tests { | ||
use lightning::routing::gossip::NodeAlias; | ||
|
||
use crate::{builder::sanitize_alias, BuildError}; |
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: Given these tests are in a sub-module of builder
, I think you don't need to import these types at all here?
src/config.rs
Outdated
@@ -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 |
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 tick node_alias
and listening_addresses
here and below, as they refer to field names.
src/config.rs
Outdated
/// | ||
/// **Note**: Node announcements will only be broadcast if the node_alias and the | ||
/// listening_addresses are set. | ||
pub node_alias: Option<NodeAlias>, |
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.
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?
src/lib.rs
Outdated
@@ -116,15 +117,17 @@ pub use io::utils::generate_entropy_mnemonic; | |||
#[cfg(feature = "uniffi")] | |||
use uniffi_types::*; | |||
|
|||
pub use builder::sanitize_alias; |
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.
As mentioned above, please drop this export.
src/lib.rs
Outdated
@@ -646,13 +651,20 @@ impl Node { | |||
} | |||
|
|||
let addresses = bcast_config.listening_addresses.clone().unwrap_or(Vec::new()); | |||
let alias = node_alias.clone().map(|alias| { |
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.
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
}
src/lib.rs
Outdated
@@ -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( |
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.
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.
src/lib.rs
Outdated
@@ -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; |
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.
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
.
src/lib.rs
Outdated
@@ -1288,6 +1300,38 @@ impl Node { | |||
} | |||
} | |||
|
|||
/// Opens a channel with a peer. |
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.
Please move the docs from connect_open_channel
here and also copy them to open_announced_channel
.
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.
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.
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
25089e3
to
c2589f3
Compare
What this PR does:
Related issue(s):