Remove temporary channels#4595
Merged
pwojcikdev merged 20 commits intonanocurrency:developfrom May 4, 2024
Merged
Conversation
clemahieu
approved these changes
May 3, 2024
| { | ||
| nano::test::system system; | ||
| auto node = add_ipc_enabled_node (system); | ||
| // Add node2 first to avoid peers with ephemeral ports |
Contributor
There was a problem hiding this comment.
How does changing the order avoid ephemeral ports?
Contributor
Author
There was a problem hiding this comment.
The way tests setup nodes is that second node connects to the first one. Connection is initiated from ephemeral port to a specific listening port. Here we can only predict the listening port, so node needs to connect to node2.
Contributor
Author
There was a problem hiding this comment.
If there is a better way I'll be happy to change it. I wanted to get it working without removing asserts.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Our current networking design is strange in this aspect, that we treat connections accepted by our node (temporary channels) differently to connections that are initiated by our node. There is no fundamental reason for such behavior, TCP allows for bidirectional communication and doing so may lead to more robust network, since some peers aren't directly reachable (NAT, firewalls).
The logic for handling those temporary channels was also not great, it created a new channel for each processed message which led to inefficiencies, especially when paired with fair queuing. Look at
nano::transport::tcp_channels::process_message (...). It also used node id for channel lookup when processing messages. Here I'm associating each realtime connection with a single channel, without relying on node id, which is simpler and more robust approach.This PR moves management of lower-level connections fully to
tcp_listenerand implements them using coroutines. Each connection attempt is given a timeout (by default 60 seconds) and node id conflicts are only rejected for same IP addresses.Previous attempt to fix this issue was #3928