Make node's address optional when opening channel #832
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,7 +54,7 @@ | |
| //! | ||
| //! let node_id = PublicKey::from_str("NODE_ID").unwrap(); | ||
| //! let node_addr = SocketAddress::from_str("IP_ADDR:PORT").unwrap(); | ||
| //! node.open_channel(node_id, Some(node_addr), 10000, None, None).unwrap(); | ||
| //! | ||
| //! let event = node.wait_next_event(); | ||
| //! println!("EVENT: {:?}", event); | ||
|
|
@@ -1126,39 +1126,47 @@ impl Node { | |
| } | ||
|
|
||
| fn open_channel_inner( | ||
| &self, node_id: PublicKey, address: Option<SocketAddress>, | ||
| channel_amount_sats: FundingAmount, push_to_counterparty_msat: Option<u64>, | ||
| channel_config: Option<ChannelConfig>, announce_for_forwarding: bool, | ||
| ) -> Result<UserChannelId, Error> { | ||
| if !*self.is_running.read().unwrap() { | ||
| return Err(Error::NotRunning); | ||
| } | ||
|
|
||
| // if we don't have the socket address, check if we are already connected | ||
| let address = match address { | ||
| Some(address) => { | ||
| // We need to use our main runtime here as a local runtime might not be around to poll | ||
| // connection futures going forward. | ||
| let con_cm = Arc::clone(&self.connection_manager); | ||
| let con_addr = address.clone(); | ||
| self.runtime.block_on(async move { | ||
| con_cm.connect_peer_if_necessary(node_id, con_addr).await | ||
|
Contributor
There was a problem hiding this comment. This was already present, but I would find it surprising if I specified an IPV4 address, but I was already connected to the peer via OnionV3, and the IPV4 setting at channel open was silently ignored and replaced with the OnionV3 address. Works also the other way around. WDYT ?
Contributor
Author
There was a problem hiding this comment. While I agree, not really sure what the alternative would be? We could throw an error if the socket addr doesn't match the one we are currently connected with but that seems kinda flaky and not what people would expect.
Contributor
There was a problem hiding this comment. one potential non-flaky route in case of a mismatch: disconnect from the peer, and reconnect using the address specified in the open parameter ?
Contributor
There was a problem hiding this comment. though this is likely out-of-scope for this PR as we'd want things to be consistent also with the Curious what @tnull thinks nonetheless. |
||
| })?; | ||
| Some(address) | ||
| }, | ||
| None => { | ||
| // If we are connected, grab the socket address as we need to make sure we have it persisted | ||
| // in our peer storage for future reconnections. | ||
| let peer = | ||
| self.peer_manager.peer_by_node_id(&node_id).ok_or(Error::NotConnected)?; | ||
| peer.socket_address | ||
| }, | ||
| }; | ||
|
|
||
| let channel_amount_sats = match channel_amount_sats { | ||
| FundingAmount::Exact { amount_sats } => { | ||
| // Check funds availability after connection (includes anchor reserve | ||
| // calculation). | ||
| self.check_sufficient_funds_for_channel(amount_sats, &node_id)?; | ||
| amount_sats | ||
| }, | ||
| FundingAmount::Max => { | ||
| // Determine max funding amount from all available on-chain funds. | ||
| let cur_anchor_reserve_sats = | ||
| total_anchor_channels_reserve_sats(&self.channel_manager, &self.config); | ||
| let new_channel_reserve = self.new_channel_anchor_reserve_sats(&node_id)?; | ||
| let total_anchor_reserve_sats = cur_anchor_reserve_sats + new_channel_reserve; | ||
|
|
||
| let fee_rate = | ||
|
|
@@ -1197,20 +1205,21 @@ impl Node { | |
| ); | ||
|
|
||
| match self.channel_manager.create_channel( | ||
| node_id, | ||
| channel_amount_sats, | ||
| push_msat, | ||
| user_channel_id, | ||
| None, | ||
| Some(user_config), | ||
| ) { | ||
| Ok(_) => { | ||
| log_info!(self.logger, "Initiated channel creation with peer {}. ", node_id); | ||
|
|
||
| if let Some(address) = address { | ||
| let peer_info = PeerInfo { node_id, address }; | ||
| self.peer_store.add_peer(peer_info)?; | ||
| } | ||
|
|
||
| Ok(UserChannelId(user_channel_id)) | ||
| }, | ||
| Err(e) => { | ||
|
|
@@ -1224,7 +1233,7 @@ impl Node { | |
| let init_features = self | ||
| .peer_manager | ||
| .peer_by_node_id(peer_node_id) | ||
| .ok_or(Error::NotConnected)? | ||
| .init_features; | ||
| let anchor_channel = init_features.requires_anchors_zero_fee_htlc_tx(); | ||
| Ok(new_channel_anchor_reserve_sats(&self.config, peer_node_id, anchor_channel)) | ||
|
|
@@ -1262,12 +1271,16 @@ impl Node { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Open a new unannounced channel with a node. | ||
| /// | ||
| /// To open an announced channel, see [`Node::open_announced_channel`]. | ||
| /// | ||
| /// Disconnects and reconnects are handled automatically. | ||
| /// | ||
| /// If `address` is provided, the node will connect to the peer at the given address before | ||
| /// opening the channel. If `address` is `None`, the node must already be connected to the | ||
| /// peer, otherwise [`Error::NotConnected`] will be returned. | ||
| /// | ||
| /// If `push_to_counterparty_msat` is set, the given value will be pushed (read: sent) to the | ||
| /// channel counterparty on channel open. This can be useful to start out with the balance not | ||
| /// entirely shifted to one side, therefore allowing to receive payments from the getgo. | ||
|
|
@@ -1280,7 +1293,7 @@ impl Node { | |
| /// | ||
| /// [`AnchorChannelsConfig::per_channel_reserve_sats`]: crate::config::AnchorChannelsConfig::per_channel_reserve_sats | ||
| pub fn open_channel( | ||
| &self, node_id: PublicKey, address: Option<SocketAddress>, channel_amount_sats: u64, | ||
|
benthecarman marked this conversation as resolved.
|
||
| push_to_counterparty_msat: Option<u64>, channel_config: Option<ChannelConfig>, | ||
| ) -> Result<UserChannelId, Error> { | ||
| self.open_channel_inner( | ||
|
|
@@ -1293,16 +1306,20 @@ impl Node { | |
| ) | ||
| } | ||
|
|
||
| /// Open a new announced channel with a node. | ||
| /// | ||
| /// This will return an error if the node has not been sufficiently configured to operate as a | ||
| /// forwarding node that can properly announce its existence to the public network graph, i.e., | ||
| /// [`Config::listening_addresses`] and [`Config::node_alias`] are unset. | ||
| /// | ||
| /// To open an unannounced channel, see [`Node::open_channel`]. | ||
| /// | ||
| /// Disconnects and reconnects are handled automatically. | ||
| /// | ||
| /// If `address` is provided, the node will connect to the peer at the given address before | ||
| /// opening the channel. If `address` is `None`, the node must already be connected to the | ||
| /// peer, otherwise [`Error::NotConnected`] will be returned. | ||
| /// | ||
| /// If `push_to_counterparty_msat` is set, the given value will be pushed (read: sent) to the | ||
| /// channel counterparty on channel open. This can be useful to start out with the balance not | ||
| /// entirely shifted to one side, therefore allowing to receive payments from the getgo. | ||
|
|
@@ -1315,7 +1332,7 @@ impl Node { | |
| /// | ||
| /// [`AnchorChannelsConfig::per_channel_reserve_sats`]: crate::config::AnchorChannelsConfig::per_channel_reserve_sats | ||
| pub fn open_announced_channel( | ||
| &self, node_id: PublicKey, address: Option<SocketAddress>, channel_amount_sats: u64, | ||
| push_to_counterparty_msat: Option<u64>, channel_config: Option<ChannelConfig>, | ||
| ) -> Result<UserChannelId, Error> { | ||
| if let Err(err) = may_announce_channel(&self.config) { | ||
|
|
@@ -1333,13 +1350,17 @@ impl Node { | |
| ) | ||
| } | ||
|
|
||
| /// Open a new unannounced channel with a node, using all available on-chain funds minus fees | ||
| /// and anchor reserves. | ||
| /// | ||
| /// To open an announced channel, see [`Node::open_announced_channel_with_all`]. | ||
| /// | ||
| /// Disconnects and reconnects are handled automatically. | ||
| /// | ||
| /// If `address` is provided, the node will connect to the peer at the given address before | ||
| /// opening the channel. If `address` is `None`, the node must already be connected to the | ||
| /// peer, otherwise [`Error::NotConnected`] will be returned. | ||
| /// | ||
| /// If `push_to_counterparty_msat` is set, the given value will be pushed (read: sent) to the | ||
| /// channel counterparty on channel open. This can be useful to start out with the balance not | ||
| /// entirely shifted to one side, therefore allowing to receive payments from the getgo. | ||
|
|
@@ -1348,8 +1369,8 @@ impl Node { | |
| /// | ||
| /// [`AnchorChannelsConfig::per_channel_reserve_sats`]: crate::config::AnchorChannelsConfig::per_channel_reserve_sats | ||
| pub fn open_channel_with_all( | ||
| &self, node_id: PublicKey, address: Option<SocketAddress>, | ||
| push_to_counterparty_msat: Option<u64>, channel_config: Option<ChannelConfig>, | ||
| ) -> Result<UserChannelId, Error> { | ||
| self.open_channel_inner( | ||
| node_id, | ||
|
|
@@ -1361,8 +1382,8 @@ impl Node { | |
| ) | ||
| } | ||
|
|
||
| /// Open a new announced channel with a node, using all available on-chain funds minus fees | ||
| /// and anchor reserves. | ||
| /// | ||
| /// This will return an error if the node has not been sufficiently configured to operate as a | ||
| /// forwarding node that can properly announce its existence to the public network graph, i.e., | ||
|
|
@@ -1372,6 +1393,10 @@ impl Node { | |
| /// | ||
| /// Disconnects and reconnects are handled automatically. | ||
| /// | ||
| /// If `address` is provided, the node will connect to the peer at the given address before | ||
| /// opening the channel. If `address` is `None`, the node must already be connected to the | ||
| /// peer, otherwise [`Error::NotConnected`] will be returned. | ||
| /// | ||
| /// If `push_to_counterparty_msat` is set, the given value will be pushed (read: sent) to the | ||
| /// channel counterparty on channel open. This can be useful to start out with the balance not | ||
| /// entirely shifted to one side, therefore allowing to receive payments from the getgo. | ||
|
|
@@ -1380,8 +1405,8 @@ impl Node { | |
| /// | ||
| /// [`AnchorChannelsConfig::per_channel_reserve_sats`]: crate::config::AnchorChannelsConfig::per_channel_reserve_sats | ||
| pub fn open_announced_channel_with_all( | ||
| &self, node_id: PublicKey, address: Option<SocketAddress>, | ||
| push_to_counterparty_msat: Option<u64>, channel_config: Option<ChannelConfig>, | ||
| ) -> Result<UserChannelId, Error> { | ||
| 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}"); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Coming back to this after some time, I have to say I still find the new API changes here not great. That we suddenly have this new error case instead of just .. connecting to the peer IMO shows that it just makes the API less explicit, less clear, and more complicated, as users now suddenly will have to handle one more edge case. I'm still not entirely convinced it's worth it tbh.
However, if you insist, this error variant should like be named more along the lines of
PeerAddressUnknownor similar, as the issue here really is not that we're not connected but that we now have this API that allows users to call it without all necessary data to connect to the involved peers in certain cases.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.
Upon further thought, I'd be happy to drop the proposed API changes here too.
I have a question: what should we do if the user sets an onion v3 address in
open_channelfor the peer, but we are already connected to that peer on IPv4 ?My initial hunch is to disconnect the peer, and reconnect over onionV3, with perhaps a warning in the logs "hey note we were already connected to that peer on IPv4".
On master, we currently silently drop the onion V3 address, and use the existing IPv4 connection, which I don't think is what we want.
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.
Problem with this is that if we fail to connect over tor, then we've disconnected the peer and interrupted our existing channel for nothing. We can't necessarily just reconnect to them as well as it may be an inbound connection.
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 that's a good point. i think i'm good to not touch anything then.
Uh oh!
There was an error while loading. Please reload this page.
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.
The way to have the least surprising api would probably be to remove the socket address from
open_channelcompletely and require the user to callconnect_peerbefore. However, that is pretty annoying to the user and not what people have come to expect.This PR is becoming more effort than it is worth. Fine with closing we can't come to agreement
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.
good to close if @tnull is too