Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@tomaka
Copy link
Contributor

@tomaka tomaka commented May 3, 2019

My take on #2200

Runs the peerset and randomly performs actions on it over time.

I'm aware of the fact that the tests fails, however I'm unsure whether it's the test that is wrong or if it's a bug in the PSM. If possible I'd be asking for reviews to see if what I'm doing is correct.

@tomaka tomaka added A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage. labels May 3, 2019
if let Some(id) = incoming_nodes.iter().find(|(_, v)| **v == id).map(|(&id, _)| id) {
incoming_nodes.remove(&id);
}
assert!(connected_nodes.insert(id));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might want to change this to assert!(connected_nodes.insert(id) || known_nodes.remove(id));, to account for an Accept message being received after discovered was called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

known_nodes contains all the nodes that have been discovered in the past, not just nodes waiting for connection.

@gavofyork
Copy link
Member

@tomaka is this ready for merge?

@tomaka
Copy link
Contributor Author

tomaka commented May 9, 2019

@tomaka is this ready for merge?

No, the test failure is legitimate but I don't know if it's an issue in the tests or in the PSM.
I need to go back to this PR.

@gavofyork gavofyork removed the A0-please_review Pull request needs code review. label May 12, 2019
@tomaka
Copy link
Contributor Author

tomaka commented May 13, 2019

Should be ready now.

@tomaka
Copy link
Contributor Author

tomaka commented May 14, 2019

CI failure looks unrelated.

@gavofyork gavofyork added A8-looksgood and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 14, 2019
@gavofyork gavofyork merged commit 2aadc9d into paritytech:master May 14, 2019
@tomaka tomaka deleted the fix-2200 branch May 14, 2019 14:36
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019
* Randomly fuzz the PSM

* Fix test

* Run it moar
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants