Make node's address optional when opening channel#832
Make node's address optional when opening channel#832benthecarman wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
There was a problem hiding this comment.
In the ldk-server PR, I mentioned potentially looking up the socket address from the gossip network graph. CLN does this.
Upon further thought, I'm thinking it might be best to let users implement that feature themselves, using the existing API calls on ldk-node.
benthecarman
force-pushed
the
opt-socket-addr
branch
from
6f7bbb5 to
3cb0f3f
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
So, as mentioned in #490 and lightningdevkit/ldk-server#159 I'm not the biggest fan of this change as the API now indicates "address is optional", which it isn't. However, it seems everybody else disagrees with me, and I do not feel too strongly about the matter, so the changes are fine by me.
I would prefer to not test them via channel_full_cycle though, but rather add a dedicated/separate test case for it, if we deem them test-worhty.
tests/common/mod.rs
Outdated
| pub(crate) async fn do_channel_full_cycle<E: ElectrumApi>( | ||
| node_a: TestNode, node_b: TestNode, bitcoind: &BitcoindClient, electrsd: &E, allow_0conf: bool, | ||
| expect_anchor_channel: bool, force_close: bool, | ||
| expect_anchor_channel: bool, force_close: bool, connect_first: bool, |
There was a problem hiding this comment.
Hmm, rather than adding yet another bool here, please create a dedicated test for it. Alternatively, it might better fit to add it to connection_restart_behavior or a similar test.
There was a problem hiding this comment.
gave it its own test
I guess we could? What was the reasoning for your change of mind? |
I could totally see us implementing this behavior here in ldk-node, but here's my case against: The principle of least surprise. If I were not familiar with LN gossip messages, I would find it surprising if I omitted the address, and had never connected to the peer before, and my node actually connected to the peer at channel open using a Also, how would we pick which address to prioritize among the addresses the node has specified in its I could see this happening at a higher level, but overall I find making a decision here at ldk-node to be too opinionated, and we should leave users the freedom to specify the exact behavior themselves using the existing API calls on ldk-node. |
Previously we always required providing the node's address when opening a channel, this made the api annoying in some cases as you may already be connected to the peer but still need to provide the socket address. Here we make it optional and will return a `ConnectionFailed` error when the socket address is not provided and we are not connected to the peer.
benthecarman
force-pushed
the
opt-socket-addr
branch
from
3cb0f3f to
02daf3f
Compare
Makes sense, though arguably, if you're really after the principle of least surprise, you might want to leave the argument as required. :) |
| // 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::ConnectionFailed)?; |
There was a problem hiding this comment.
Hmm, it's a bit weird that this looks directly at peer manager and doesn't incorporate any connections that are inflight in ConnectionManager (i.e., two channel opens in quick succession will still have the second one fail, potentially?).
Apart from that we probably want to introduce a dedicated error type that communicates why we failed to open a channel, and ConnectionFailed seems wrong here (as we didn't even attempt the connection in the first place, for example)
4 participants
Previously we always required providing the node's address when opening a channel, this made the api annoying in some cases as you may already be connected to the peer but still need to provide the socket address.
Here we make it optional and will return a
ConnectionFailederror when the socket address is not provided and we are not connected to the peer.