Skip to content
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

Send Addr message before disconnecting when connection is full #1127

Closed
erikzhang opened this issue Sep 30, 2019 · 14 comments
Closed

Send Addr message before disconnecting when connection is full #1127

erikzhang opened this issue Sep 30, 2019 · 14 comments
Assignees
Labels
design Issue state - Feature accepted but the solution requires a design before being implemented enhancement Type - Changes that may affect performance, usability or add new features to existing modules. p2p Module - peer-to-peer message exchange and network optimisations, at TCP or UDP level (not HTTP).

Comments

@erikzhang
Copy link
Member

Summary
When a node's connection is full, it automatically disconnects the new connection. If the connections for all seed nodes is full, the new node will not be able to join the p2p network.

Do you have any solution you want to propose?
If the connections to the node is full, it should send an Addr message before disconnecting, so that the new connected node has a chance to get other nodes that can be connected.

Where in the software does this update applies to?

  • P2P (TCP)
@erikzhang erikzhang added the discussion Initial issue state - proposed but not yet accepted label Sep 30, 2019
@steven1227
Copy link
Member

@gsmachado Although this is a mechanical issue of the p2p network, I think you may be interested in this topic as the node selection can be used here.

@roman-khimov
Copy link
Contributor

It seems suspicious to me from the connection state point of view. As a node I would only expect Addr from the other node after I'd sent GetAddr request and receiving unsolicited Addr IMO should be treated as wrong behavior of the other side of the connection (even though IIRC it currently is handled just fine by the C# code).

Maybe the safer approach would be to drop some older connection in this case? Supposedly the peer of this older connection had already sent GetAddr and got some Addr in response, so it can tolerate that.

@shargon
Copy link
Member

shargon commented Sep 30, 2019

Agree with this, related to #1013

@ixje
Copy link
Contributor

ixje commented Sep 30, 2019

It seems suspicious to me from the connection state point of view. As a node I would only expect Addr from the other node after I'd sent GetAddr request and receiving unsolicited Addr IMO should be treated as wrong behavior of the other side of the connection (even though IIRC it currently is handled just fine by the C# code).

Not sure if the C# code these days does any tracking of outstanding requests versus data received.

on topic;
I like the idea. Last week a neo-python user faced the same issue as described in #1013.
If I understand it correctly the Addr type message will be send before the handshake is completed, right?

@erikzhang
Copy link
Member Author

Another way is to allow nodes to run in seed mode. A node running in seed mode will actively disconnect the nodes that have received the Addr message after a period of time.

@ixje
Copy link
Contributor

ixje commented Sep 30, 2019

Another way is to allow nodes to run in seed mode. A node running in seed mode will actively disconnect the nodes that have received the Addr message after a period of time.

I'm not sure what that means. What will a node that is not running in seed mode do different?

I thought the proposal is already something like (pseudo);

on_connect(node) {
   if (Localnode.connection_count >= settings.max_connections) {
       node.send_message(command='addr', address_list);
       node.disconnect();
   } else {
      node.do_handshake();
      etc..
  }
}

@lock9 lock9 added design Issue state - Feature accepted but the solution requires a design before being implemented enhancement Type - Changes that may affect performance, usability or add new features to existing modules. p2p Module - peer-to-peer message exchange and network optimisations, at TCP or UDP level (not HTTP). and removed discussion Initial issue state - proposed but not yet accepted labels Sep 30, 2019
@ixje
Copy link
Contributor

ixje commented Oct 1, 2019

Perhaps include the outcome of #1133 to this improvement.

@shargon
Copy link
Member

shargon commented Oct 1, 2019

Perhaps include the outcome of #1133 to this improvement.

Exactly, reason and peers

@shargon
Copy link
Member

shargon commented Oct 1, 2019

I can do it if no one want to implement this.

@erikzhang
Copy link
Member Author

Option 1: Send peers and then disconnect from the newly node.
Option 2: Allow the connection from the newly nodes, and disconnect from the older nodes.

@ixje
Copy link
Contributor

ixje commented Oct 1, 2019

Option 1

@shargon
Copy link
Member

shargon commented Oct 1, 2019

Option 1 (adding it the disconnection reason :P)

@Tommo-L
Copy link
Contributor

Tommo-L commented Oct 11, 2019

If the new node dosen't disconnect, it will lead to a lot of connections in the older nodes. Why not option2, the older node disconnects the new connection attached with address list.

@Tommo-L
Copy link
Contributor

Tommo-L commented Oct 11, 2019

fixed in #1154, plz review it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Issue state - Feature accepted but the solution requires a design before being implemented enhancement Type - Changes that may affect performance, usability or add new features to existing modules. p2p Module - peer-to-peer message exchange and network optimisations, at TCP or UDP level (not HTTP).
Projects
None yet
Development

No branches or pull requests

7 participants