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

Track pending outgoing connections to peers #398

Closed
sangaman opened this issue Aug 25, 2018 · 12 comments
Closed

Track pending outgoing connections to peers #398

sangaman opened this issue Aug 25, 2018 · 12 comments
Assignees
Labels
p2p Peer to peer networking
Milestone

Comments

@sangaman
Copy link
Collaborator

Right now, on startup it's common to get a flood of NODES packets all at once which have a lot of overlap, which then results in multiple outgoing connection attempts to those nodes. There's no mechanism currently to check if we're attempting to connect to a given node. For this issue I suggest that we track a pendingPeers Map in Pool which maps peer's node pub keys to the unopened Peer object. When the peer connection either succeeds and the open event is fired, or fails and the close event is fired, we remove the peer from pendingPeers and add to peers if successful.

@sangaman sangaman added the p2p Peer to peer networking label Aug 25, 2018
@moshababo
Copy link
Collaborator

So you're suggesting to ignore nodes which their pubkey is already listed on pendingPeers?

@sangaman
Copy link
Collaborator Author

Exactly.

@moshababo
Copy link
Collaborator

Sounds good to me. If we'll want to support parallelism (specially due to connection retries, which can take ages), we'll need to introduce address in addition or instead of pubKey.

@sangaman
Copy link
Collaborator Author

Yup. Maybe we introduce parallelism later, for now though I think this approach would be a significant improvement.

@kilrau
Copy link
Contributor

kilrau commented Aug 27, 2018

Introducing pendingPeers will ensure we only try to connect to a given pubKey once all the time (not only on startup...)? @sangaman

If yes, that's what we need!

This was referenced Aug 27, 2018
@sangaman
Copy link
Collaborator Author

Yeah, well we already have checks in place to not attempt a connection to a peer we're already connected to. This will check to make sure we're not attempting a connection a peer we're already attempting to connect to.

@kilrau
Copy link
Contributor

kilrau commented Aug 27, 2018

Ok thx. As per #392 (comment) I suggest pendingPeers contains pubKey@IP:port, otherwise we can't prevent connection attempts per listeningAddress

@kilrau kilrau added this to the 1.0.0-alpha.2 milestone Aug 27, 2018
@sangaman
Copy link
Collaborator Author

I think just nodepubkey would be enough actually, since all outgoing connections reference a peer nodepubkey, we just check if we have a pending peer for the given nodepubkey.

@kilrau
Copy link
Contributor

kilrau commented Aug 27, 2018

If we do #392 the serial way yes, if we do retries in parallel we would need pubKey@IP:port

@sangaman
Copy link
Collaborator Author

Yeah that's true. But I'm pretty sure I prefer the serial approach, otherwise I think we'd see this same burst of failing connection attempts should anyone have a lot of listening addresses configured.

@robertsdotpm
Copy link
Contributor

I'll take this issue

@sangaman
Copy link
Collaborator Author

That would be great, I haven't started any work on it yet. Let me know if you have any questions at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2p Peer to peer networking
Projects
None yet
Development

No branches or pull requests

4 participants