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

p2p: support EnableGossipService in p2p streams #6073

Merged

Conversation

algorandskiy
Copy link
Contributor

Summary

Added EnableGossipService support to p2p net: stream manager checks connection direction and either does not open a new /algorand-ws/1.0.0 (in Connected notification) or closes incoming stream in a stream handler.

Additionally I removed useless incoming vs outgoing checks with warnings since their assumption are wrong: Connected notification is fired for both incoming and outgoing connections.

Test Plan

Added tests to ensure:

  1. Node with EnableGossipService=false can still accept and send traffic
  2. Relay and node with EnableGossipService=false cannot communicate.

@algorandskiy algorandskiy added Enhancement p2p Work related to the p2p project labels Jul 19, 2024
@algorandskiy algorandskiy self-assigned this Jul 19, 2024
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 56.30%. Comparing base (edda2ee) to head (4673c8b).

Files Patch % Lines
network/p2p/streams.go 0.00% 14 Missing ⚠️
network/p2p/p2p.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6073      +/-   ##
==========================================
+ Coverage   55.95%   56.30%   +0.34%     
==========================================
  Files         488      488              
  Lines       69602    69595       -7     
==========================================
+ Hits        38945    39182     +237     
+ Misses      27967    27763     -204     
+ Partials     2690     2650      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

One still is receiving blocks and certs when gossip service is disabled right? Do we doublecheck that/is it worth doublechecking that?

network/p2p/streams.go Show resolved Hide resolved
gmalouf
gmalouf previously approved these changes Jul 19, 2024
network/p2p/streams.go Show resolved Hide resolved
network/p2p/streams.go Show resolved Hide resolved
network/p2pNetwork_test.go Outdated Show resolved Hide resolved
network/p2pNetwork_test.go Show resolved Hide resolved
jasonpaulos
jasonpaulos previously approved these changes Jul 25, 2024
@algorandskiy
Copy link
Contributor Author

Merged master into

@algorandskiy algorandskiy merged commit 578684e into algorand:master Jul 29, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement p2p Work related to the p2p project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants