-
Notifications
You must be signed in to change notification settings - Fork 787
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Remove temporary channels #4595
Remove temporary channels #4595
Conversation
@@ -1710,36 +1710,64 @@ TEST (rpc, keepalive) | |||
TEST (rpc, peers) | |||
{ | |||
nano::test::system system; | |||
auto node = add_ipc_enabled_node (system); | |||
// Add node2 first to avoid peers with ephemeral ports |
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.
How does changing the order avoid ephemeral ports?
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 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.
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.
If there is a better way I'll be happy to change it. I wanted to get it working without removing asserts.
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_listener
and 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