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

fix p2p max_peers_connected_works test #1002

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

leviathanbeak
Copy link
Contributor

@leviathanbeak leviathanbeak commented Feb 8, 2023

We've been having certain issues with this test especially in the CI,
here's the analysis and the fix.

First problem is the assertion put on the swarm itself instead of our PeerManager:

if p2p_service.swarm.connected_peers().count() > peer_limit { panic!() }

While generally this should not return more peers than allowed, one of the reasons we have PeerManager is to actually contain number of peers connected logic, so moving the assertion to

if p2p_service.peer_manager().total_peers_connected() > peer_limit { panic!() }

should suffice since PeerManager is the one disconnecting surplus peers.

Other, and bigger, issue with the test was the amount of simulated nodes we used in our example.
Having 100+ nodes running makes the test itself unnecessarily slow.
This became even more evident now that we made our Heartbeat protocol ask for a disconnect from a node that did not reply within 2 seconds with a Block Height. So nodes started disconnecting from each other and the test would never hit its exit point.

Actually there is no need to have that amount of nodes, instead same assertion can be achieved with a smaller amount, so I have decreased the amount of nodes in the test.

@leviathanbeak leviathanbeak added bug Something isn't working fuel-p2p labels Feb 8, 2023
@leviathanbeak leviathanbeak self-assigned this Feb 8, 2023
@leviathanbeak leviathanbeak merged commit b721d62 into master Feb 8, 2023
@leviathanbeak leviathanbeak deleted the leviathanbeak/refactor_p2p_test branch February 8, 2023 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fuel-p2p
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants