Support separate node announcement addresses #484
Conversation
src/config.rs
Outdated
There was a problem hiding this comment.
You'll also need to account for the new field in may_announce_channel (and corresponding tests) in this file. We use this method to check whether we allow to open or accept announced channels.
There was a problem hiding this comment.
I looked at this but I believe the may_announce_channel is still correct as-is. announcement_addresses doesn't change the behavior of when or if a node can announce channels. That is still purely determined by listening_addresses and alias. The announcement_addresses just allows an override for what addresses are actually announced.
There was a problem hiding this comment.
Ah, you are right! In fact, could you extend the node_announce_channel test case to assert that may_announce_channel is still false if listening_addresses is None and announcement_addresses is Some?
May also be good to add a check for this and log+abort early in Builder::build_with_store_internal, so that the user can't ever startup with misconfigured Node where announcement_addresses is set but listening_addresses aren't.
There was a problem hiding this comment.
Both of those suggestions sound good. For the misconfigured node do we want a new error variant or should I re-use the existing BuildError::InvalidAnnouncementAddresses? I think it's probably fine as long as there's a log_error with explanation alongside it.
Also, I think the check should not only be against unset listening_addresses but actually be if config.announcement_addrresses.is_some() && !may_announce_channel(&config).
Basically if you set announcement addresses but the node will not announce then it's misconfigured.
There was a problem hiding this comment.
Both of those suggestions sound good. For the misconfigured node do we want a new error variant or should I re-use the existing
BuildError::InvalidAnnouncementAddresses? I think it's probably fine as long as there's a log_error with explanation alongside it.
Yeah, though InvalidListeningAddresses might make more sense, no?
Basically if you set announcement addresses but the node will not announce then it's misconfigured.
Yeah, that makes sense. FWIW, could do the same for NodeAlias, as you'd also only expect it to be set if you want to announce your node.
There was a problem hiding this comment.
Well if the check is if config.announcement_addrresses.is_some() && !may_announce_channel(&config) then we don't know if it's missing listening_addresses or alias and so maybe it makes more sense for the error to be BuildError::InvalidAnnouncementAddresses` in the sense they are invalid because you didn't set the other values? Or we could add a new error specifically for some announcement field set (announcement_addresses or alias) when the node is not going to announce.
Will add the same check for NodeAlias.
There was a problem hiding this comment.
Well, maybe we should switch may_announce_channel to return a Result<(), AnnounceError>? Then, we know at the callsites why it can't announce and can take action (mostly log the right thing before returning the corresponding BuildError) based on the error type?
There was a problem hiding this comment.
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.
| @@ -412,6 +417,18 @@ impl NodeBuilder { | |||
| Ok(self) | |||
| } | |||
|
|
|||
| /// 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.
should we add the 'if unset' note here as well?
src/lib.rs
Outdated
| @@ -457,7 +457,9 @@ impl Node { | |||
| continue; | |||
| } | |||
|
|
|||
| let addresses = if let Some(addresses) = bcast_config.listening_addresses.clone() { | |||
| let addresses = if let Some(addresses) = bcast_config.announcement_addresses.clone() { | |||
There was a problem hiding this comment.
nit: use var names such as listening_addresses and announcement_addresses for better readability.
src/lib.rs
Outdated
| @@ -457,7 +457,9 @@ impl Node { | |||
| continue; | |||
| } | |||
|
|
|||
| let addresses = if let Some(addresses) = bcast_config.listening_addresses.clone() { | |||
| let addresses = if let Some(addresses) = bcast_config.announcement_addresses.clone() { | |||
There was a problem hiding this comment.
We probably need to update may_announce_channel and also the code-points where we check for availability of listening addresses for public-channel/announcements.
There was a problem hiding this comment.
I don't think we do. I mentioned this above:
I looked at this but I believe the may_announce_channel is still correct as-is. announcement_addresses doesn't change the behavior of when or if a node can announce channels. That is still purely determined by listening_addresses and alias. The announcement_addresses just allows an override for what addresses are actually announced.
| let addresses = if let Some(addresses) = bcast_config.listening_addresses.clone() { | ||
| let addresses = if let Some(addresses) = bcast_config.announcement_addresses.clone() { | ||
| addresses | ||
| } else if let Some(addresses) = bcast_config.listening_addresses.clone() { | ||
| 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.
this log will need to be updated.
There was a problem hiding this comment.
I'm not sure it does because listening_addresses is the only thing that should always be set.
src/config.rs
Outdated
There was a problem hiding this comment.
Ah, you are right! In fact, could you extend the node_announce_channel test case to assert that may_announce_channel is still false if listening_addresses is None and announcement_addresses is Some?
May also be good to add a check for this and log+abort early in Builder::build_with_store_internal, so that the user can't ever startup with misconfigured Node where announcement_addresses is set but listening_addresses aren't.
src/lib.rs
Outdated
| @@ -802,6 +804,11 @@ impl Node { | |||
| self.config.listening_addresses.clone() | |||
| } | |||
|
|
|||
| /// Returns our own announcement addresses. | |||
There was a problem hiding this comment.
Should this always return the addresses we announce, i.e., fallback to listening_addresses if announcement_addresses are None?
There was a problem hiding this comment.
Good question. I think that probably makes more sense? If they want to see what the actual announcement_addresses are they can check the node config.
|
Pushed new commits that address all review feedback. Only consideration left imo is whether or not to introduce new error variant to represent misconfigured announcement parameters. |
Fixes #422
Users running ldk-node in a container and/or behind a NAT will often want to or be forced to bind to a different address than they announce to the network.
This PR introduces a new
announcement_addressesconfiguration option that operates the same waylistening_addressesdoes but is used for constructing the node announcement message. An emptyannouncement_addresseswill operate exactly the same way as today by defaulting tolistening_addresses.