Skip to content

Fail create_channel early if peer is disconnected #2437

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

Closed
dunxen opened this issue Jul 21, 2023 · 6 comments
Closed

Fail create_channel early if peer is disconnected #2437

dunxen opened this issue Jul 21, 2023 · 6 comments
Labels
good first issue Good for newcomers

Comments

@dunxen
Copy link
Contributor

dunxen commented Jul 21, 2023

We need to ensure in the case where a peer is disconnected that we fail early when we invoke ChannelManager::create_channel for their_network_key.

The only thing we currently do is check if we know about the peer at all (they're in the per_peer_state):

let peer_state_mutex = per_peer_state.get(&their_network_key)

We also need to check for .is_connected if we know about the peer.

See #2382 (comment)

@dunxen dunxen added the good first issue Good for newcomers label Jul 21, 2023
@TheBlueMatt
Copy link
Collaborator

Bonus points if we instead address #2096 (ie don't fail but rebroadcast the open_channel message on reconnect, failing in a minute or two from a timeout instead).

@henghonglee
Copy link
Contributor

@dunxen i can try this one

@dunxen
Copy link
Contributor Author

dunxen commented Jul 24, 2023

@dunxen i can try this one

Great, thanks! Matt’s bonus points would be good to handle from his comment above alternatively :)

@nafisalawalidris
Copy link

To ensure robustness, I propose adding a check for peer disconnection within this function. Currently, we only verify if we know about the peer through the per_peer_state. However, it is crucial to also consider the peer's connection status by using .is_connected. This enhancement will enable us to fail early when attempting to invoke ChannelManager::create_channel for a disconnected peer. Here's the proposed code modification:

// Existing code
let peer_state_mutex = per_peer_state.get(&their_network_key)

// Proposed modification
if let Some(peer_state) = per_peer_state.get(&their_network_key) {
if peer_state.is_connected {
// Proceed with create_channel logic
} else {
// Handle the case where the peer is disconnected
// (e.g., return an error or take appropriate action)
}
}

By implementing this check, we can proactively address issues related to disconnected peers when using ChannelManager::create_channel.

@shaavan
Copy link
Member

shaavan commented Nov 4, 2023

Hi! I couldn't help but notice the PR has been open for a little while. I've given it a shot at solving the issue. I would absolutely love to hear your thoughts on what I've come up with. Thanks a bunch! :)

@TheBlueMatt
Copy link
Collaborator

Oh, oops, dup of #2096.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants