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

net/p2p + secio: parallelize crypto handshake #1227

Merged
merged 1 commit into from
May 12, 2015
Merged

Conversation

jbenet
Copy link
Member

@jbenet jbenet commented May 12, 2015

We had a very nasty problem: handshakes were serial so incoming
dials would wait for each other to finish handshaking. this was
particularly problematic when handshakes hung-- nodes would not
recover quickly. This led to gateways not bootstrapping peers
fast enough.

The approach taken here is to do what crypto/tls does:
defer the handshake until Read/Write[0]. There are a number of
reasons why this is the right thing to do:

  • it delays handshaking until it is known to be necessary (doing io)
  • it "accepts" before the handshake, getting the handshake out of the
    critical path entirely.
  • it defers to the user's parallelization of conn handling. users
    must implement this in some way already so use that, instead of
    picking constants surely to be wrong (how many handshakes to run
    in parallel?)

[0] http://golang.org/src/crypto/tls/conn.go#L886

We had a very nasty problem: handshakes were serial so incoming
dials would wait for each other to finish handshaking. this was
particularly problematic when handshakes hung-- nodes would not
recover quickly. This led to gateways not bootstrapping peers
fast enough.

The approach taken here is to do what crypto/tls does:
defer the handshake until Read/Write[1]. There are a number of
reasons why this is _the right thing to do_:
- it delays handshaking until it is known to be necessary (doing io)
- it "accepts" before the handshake, getting the handshake out of the
  critical path entirely.
- it defers to the user's parallelization of conn handling. users
  must implement this in some way already so use that, instead of
  picking constants surely to be wrong (how many handshakes to run
  in parallel?)

[0] http://golang.org/src/crypto/tls/conn.go#L886
@jbenet jbenet added the status/in-progress In progress label May 12, 2015
@jbenet
Copy link
Member Author

jbenet commented May 12, 2015

This seems to pass. it's a very time sensitive push, so i'm going to merge it. Would appreciate CR anyway though cc @whyrusleeping @cryptix

(oh and the code in there is icky. one day we'll switch to a simple AEAD primitive agl gives us or something).

jbenet added a commit that referenced this pull request May 12, 2015
net/p2p + secio: parallelize crypto handshake
@jbenet jbenet merged commit 414ab63 into master May 12, 2015
@jbenet jbenet removed the status/in-progress In progress label May 12, 2015
@jbenet jbenet deleted the parallelize-handshake branch May 12, 2015 10:16
@whyrusleeping whyrusleeping mentioned this pull request May 12, 2015
30 tasks
@@ -79,15 +67,60 @@ func (s *secureSession) LocalPrivateKey() ci.PrivKey {

// RemotePeer retrieves the remote peer.
func (s *secureSession) RemotePeer() peer.ID {
if err := s.Handshake(); err != nil {
return ""
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd at least log.Warning these errors.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, something something throwing away errors is the worst thing in the world

@wking wking mentioned this pull request Jun 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants