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
Piper/limit connections to single ip and dial in and out ratio #935
Piper/limit connections to single ip and dial in and out ratio #935
Conversation
dc0d83b
to
453f4c6
Compare
@williambannas FYI, I decided to go ahead and do the final cleanup on this myself. Thanks for getting the majority of the work done. |
Thank you, I was getting a bit confused, I would like to continue contributing to this project, but I'm not sure what I should do differently in the future, last night I accidentally pushed my changes from the branch i was working on into my master branch, and was trying to do the re-base but i dont think I did it correctly... |
@williambannas we'd love to have you continue contributing. I'd recommend taking some time to learn your way around |
@carver sanity check review requested since I made some additional changes. |
def is_valid_connection_candidate(self, candidate: Node) -> bool: | ||
# connect to no more then 2 nodes with the same IP | ||
nodes_by_ip = groupby( | ||
operator.attrgetter('remote.address.ip'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just learned about the operator
module. Pretty cool to have all operators available as functions 🐍
p2p/server.py
Outdated
]) | ||
if total_peers > 1 and inbound_peer_count / total_peers > DIAL_IN_OUT_RATIO: | ||
# make sure to have at least 1/4 outbound connections | ||
peer.disconnect(DisconnectReason.useless_peer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, DisconnectReason.useless_peer
is really the right reason for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, my mistake. updated to use too_many_peers
(couldn't find what go-ethereum does)
tests/p2p/test_server.py
Outdated
is_full = False | ||
connected_nodes = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have test for this or maybe at least open an issue to not forget about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, looks good 👍
tests/p2p/test_server.py
Outdated
@@ -44,8 +44,15 @@ def get_open_port(): | |||
INITIATOR_REMOTE = Node(INITIATOR_PUBKEY, INITIATOR_ADDRESS) | |||
|
|||
|
|||
class MockPeerPool: | |||
class MockPeerPool(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added parens unnecessarily?
Replaces #920