Skip to content

Introduce check for is peer is disconnected #2705

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
wants to merge 1 commit into from

Conversation

shaavan
Copy link
Member

@shaavan shaavan commented Nov 4, 2023

resolves #2437

  • This PR introduces a check for if the peer is disconnected.
  • Use this check to fail early in create_channel function.

- Use it for the create_channel function to fail early
@shaavan
Copy link
Member Author

shaavan commented Nov 4, 2023

Hey there,

I've been working on adding a test for this new check, but I've encountered some hiccups while using the current functions for it:

Here's the version I've come up with:

fn check_disconnected_from_peer_error<T>(res_err: Result<T, APIError>, expected_public_key: PublicKey) {
		let expected_message = format!("Disconnected from the node: {}", expected_public_key);
		check_api_error_message(expected_message, res_err)
	}

	#[test]
	fn test_channel_creation_fail_fast_when_peers_disconnected() {
		let chanmon_cfgs = create_chanmon_cfgs(2);
		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);

		nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id());
		nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id());

		//values
		let node_1_public_key = nodes[1].node.get_our_node_id();

		check_disconnected_from_peer_error(nodes[0].node.create_channel(node_1_public_key, 1_000_000, 500_000_000, 42, None), node_1_public_key);
	}

But I realized that using the peer_disconnected function will also automatically remove the peer from per_peer_state:

if remove_peer {
			per_peer_state.remove(counterparty_node_id);
		}

I thought about creating a new function just for setting peer_state.is_connected = false, but it might be a bit too much. Also, making this change within the test itself could potentially lead to incorrect use of functions and variables.

I'm still quite new to the repository, so any advice on the right way to approach adding this test would be fantastic. Thanks a bunch for your help! :)

@TheBlueMatt
Copy link
Collaborator

The peer won't get removed if there's another channel with that peer already.

That said, dunno if you noticed in that issue my comment about "bonus points"? I'd kinda generally rather we go entirely the opposite direction here - instead of failing, "open" the channel, and add support for rebroadcasting the open_channel message.

@shaavan
Copy link
Member Author

shaavan commented Nov 6, 2023

Yep! I checked your "bonus points" comment.
That approach seems a more logical one to handle the case where a peer disconnects just at the wrong time.

Let me give that issue a try!

@shaavan
Copy link
Member Author

shaavan commented Nov 6, 2023

And btw thanks for the suggestion for the test.
It worked perfectly locally :)

@TheBlueMatt
Copy link
Collaborator

Should this be closed in favor of #2725?

@shaavan
Copy link
Member Author

shaavan commented Nov 13, 2023

Yes!

Closing this PR in favor of #2725

@shaavan shaavan closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail create_channel early if peer is disconnected
2 participants