feat: avoid duplicate autoconnection attempt #1742
Merged
Conversation
This file contains 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
Gossip autoconnection could result in two nodes attempting to connect to each other concurrently. To fix this issue, only one node initiate the connection now. The selection criteria is arbitrary but deterministic, currently the greater zid of both.
oteffahi
suggested changes
Jan 28, 2025
Comment on lines
513
to
516
| if !self.autoconnect.is_empty() | ||
| && self.autoconnect.matches(whatami) | ||
| && select_autoconnect_node(self.graph[self.idx].zid, zid) | ||
| { |
There was a problem hiding this comment.
This is a more complex problem than what it seems. Consider the following:
- Node1 is a peer, it is setup to autoconnect to routers and peers.
- Node2 is a router, it is setup to autoconnect only to routers.
- Node2 has a higher ZID, and therefore is the one returned by
select_autoconnect_node.
In thise case, Node1 will not attempt to connect because it is not the selected node, and Node2 will not autoconnect because it does not have peers in its autoconnect config, and the connection will never be established...
There was a problem hiding this comment.
Unless we add autoconnect config to the exchanged gossip information, the other solutions may involve closing one of the two links during or after opening the transport.
oteffahi
reviewed
Jan 28, 2025
oteffahi
suggested changes
Jan 29, 2025
Co-authored-by: Oussama Teffahi <[email protected]>
oteffahi
reviewed
Jan 31, 2025
oteffahi
reviewed
Jan 31, 2025
Co-authored-by: Oussama Teffahi <[email protected]>
Co-authored-by: Oussama Teffahi <[email protected]>
oteffahi
suggested changes
Feb 7, 2025
| /// (e.g. autoconnect_strategy : { "to-router": "always", "to-peer": "greater-zid" }), | ||
| /// or different values for router or peer mode | ||
| /// (e.g. autoconnect_strategy : { peer: { "to-router": "always", "to-peer": "greater-zid" } }). | ||
| autoconnect_strategy: { peer: { "to-router": "always", "to-peer": "always" } }, |
There was a problem hiding this comment.
Need to add the default config for router
There was a problem hiding this comment.
There is no default autoconnection for router, I could put the one for peer though
There was a problem hiding this comment.
Whatever the current behavior is should be the default, I assume it's "always" for all cases
| /// (e.g. autoconnect_strategy : { "to-router": "always", "to-peer": "greater-zid" }), | ||
| /// or different values for router or peer mode | ||
| /// (e.g. autoconnect_strategy : { peer: { "to-router": "always", "to-peer": "greater-zid" } }). | ||
| autoconnect_strategy: { peer: { "to-router": "always", "to-peer": "always" } }, |
There was a problem hiding this comment.
Need to add the default config for router
Mallets
reviewed
Feb 7, 2025
Mallets
approved these changes
Feb 7, 2025
This was referenced Feb 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Gossip autoconnection could result in two nodes attempting to connect to each other concurrently.
To fix this issue, only one node initiate the connection now. The selection criteria is arbitrary but deterministic, currently the greater zid of both.