chore: dont gossip to disconnected nodes if there are no connected nodes available #1051
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
ogenev
approved these changes
Dec 1, 2023
portalnet/src/overlay_service.rs
Outdated
Comment on lines
412
to
415
| let state = match set_connected { | ||
| true => ConnectionState::Connected, | ||
| false => ConnectionState::Disconnected, | ||
| }; |
There was a problem hiding this comment.
It is maybe a personal preference, but I like more the following syntax:
Suggested change
| let state = match set_connected { | |
| true => ConnectionState::Connected, | |
| false => ConnectionState::Disconnected, | |
| }; | |
| let state = if set_connected { | |
| ConnectionState::Connected | |
| } else { | |
| ConnectionState::Disconnected, | |
| } | |
| }; |
| @@ -398,7 +398,7 @@ where | |||
| command_tx | |||
| } | |||
|
|
|||
| fn add_bootnodes(&mut self, bootnode_enrs: Vec<Enr>) { | |||
| fn add_bootnodes(&mut self, bootnode_enrs: Vec<Enr>, set_connected: bool) { | |||
| // Attempt to insert bootnodes into the routing table in a disconnected state. | |||
There was a problem hiding this comment.
We need to update this comment and adding a short docstring for this function explaining the set_connected argument would be nice.
njgheorghita
reviewed
Dec 1, 2023
| @@ -398,7 +398,11 @@ where | |||
| command_tx | |||
| } | |||
|
|
|||
| fn add_bootnodes(&mut self, bootnode_enrs: Vec<Enr>) { | |||
| /// Insert a vector of enr's into the routing table | |||
There was a problem hiding this comment.
Insert a vector of enrs into the routing table
| fn add_bootnodes(&mut self, bootnode_enrs: Vec<Enr>) { | ||
| /// Insert a vector of enr's into the routing table | ||
| /// set_connected: should only be true for tests, false for production code | ||
| /// Tests what use this function are testing if adding to queue's work not if our connection code |
There was a problem hiding this comment.
Tests that use this function are testing if adding to queues works, not if our connection code works.
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.
What was wrong?
fixes #1049
How was it fixed?
Certain tests where checking if certain functions were working by putting dummy values into our kbuckets. Since we have strict standards for how we handle connected vs disconnected this test failed as it was written before this connection standard was in place.
The solution? set it as connect, this doesn't matter as these tests aren't meant to test the status of the node functionality.