-
Notifications
You must be signed in to change notification settings - Fork 27
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
Implement simultaneous open extension #42
Conversation
ping @Stebalien @raulk |
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 did a quick read through this and it looks sane. Nice job! I want to do a more thorough review — this is mildly intricate in terms of possible branching and it’ll be on our critical path to everything. Can we add fuzzing tests?
@raulk what scenarios do you want to test? We can certainly add more tests. |
tok, err = ReadNextToken(rwc) | ||
|
||
if err == io.EOF { | ||
return "", ErrNotSupported |
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.
@vyzo When can this EOF happen ? I mean, why do we treat this error separately ?
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.
EOF will happen when the remote side gives up because we have no protocols in common. In general, it's best to avoid returning EOF for actual error cases anyways (usually, you want to either return ErrUnexpectedEOF, or a better error like we're doing here).
d06dbd1
to
b087b1b
Compare
Ping @Stebalien @marten-seemann for review. |
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.
The idea is sound. I'd actually go a bit further and break much of this out into helper functions that can be reused with the other functions.
- We seem to write and send multiple delimited strings at once quite frequently, that should have a helper.
- Some of this code now duplicates existing code.
client.go
Outdated
@@ -74,6 +78,244 @@ func SelectOneOf(protos []string, rwc io.ReadWriteCloser) (string, error) { | |||
return "", ErrNotSupported | |||
} | |||
|
|||
// Performs protocol negotiation with the simultaneous open extension; the returned boolean | |||
// indicator will be true if we should act as a server. | |||
func SelectWithSimopen(protos []string, rwc io.ReadWriteCloser) (string, bool, error) { |
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.
Nit: SelectProtoOrFailSimultanious
, or something like that.
- It's unclear that this is "select proto or fail".
- We're not paying by the letter.
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.
This is done.
client.go
Outdated
} | ||
|
||
func simOpen(protos []string, rwc io.ReadWriteCloser) (string, bool, error) { | ||
retries := 3 |
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.
There's really no point in re-trying, or even building that into the protocol. If we collide, our randomness sources are likely identical and we'll never finish.
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 agree, the likelihood is abysmal. Have removed this.
client.go
Outdated
|
||
// this skips pipelined protocol negoatiation | ||
// keep reading until the token starts with select: | ||
if strings.HasPrefix(tok, "select:") { |
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.
We should skip exactly one protocol. Otherwise, if someone names their protocol select:
, we'll have problems.
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.
@Stebalien I am not sure what you mean here.
Are you talking about the exact scenario where a peer sends "mutlsitream"|"iamclient"|"select:"|"select:nonce"
where that first "select:" is a security protocol the user wants to negotiate ? I agree this scenario will create problems.
But, is this the ONLY scenario you have in mind ? Does this really warrant fixing ?
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.
Yes, that's the scenario I have in mind. My concern isn't really "fixing a bug" but rather "simplifying the spec". If we go with the current approach, the multistream-select spec would need to say "protocols beginning with select: are reserved" and "iamclient" would become an integral part of the spec. If we simply skip exactly one protocol, we can define "iamclient" to be it's own protocol.
The "iamclient" (maybe /bootstrap/simultaneous
?) protocol:
- Send exactly one preferred protocol.
- Send exactly one
select:nonce
message to pick the winner. - The winner recursively uses multistream-select to pick the final protocol. NOTE: this would require sending a new multistream header as well.
The beauty of this approach is that there's nothing inherently special about the "iamclient" protocol from a spec standpoint. The implementation can take advantage of the multistream-select ambiguity and pipeline "iamclient" with the "preferred protocol" from step 1, but that's not required. A simpler implementation could just:
- Negotiate one of "iamclient" or a set of other security transports (one round-trip per step).
- If "iamclient" is selected, send the preferred protocol and a nonce.
- Once the winner is picked, the winner negotiates their chosen protocol, potentially using the "preferred" protocol.
This version takes an extra round-trip but can be implemented as two compossible protocols. Later, as the libp2p implementation matures, it can be re-implemented to remove the extra round-trip without changing the spec.
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.
TL;DR: it allows us to write the spec as follows:
- Multistream protocol.
- "iamclient" protocol for selecting a initiator when a simultaneous connect happens.
- Optional: oh, by the way, you can combine these as follows ... to save a round-trip.
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.
@Stebalien This is done.
client.go
Outdated
var mybig, peerbig big.Int | ||
var iamserver bool | ||
mybig.SetBytes(mynonce) | ||
peerbig.SetBytes(peernonce) | ||
|
||
switch mybig.Cmp(&peerbig) { | ||
case -1: | ||
// peer nonce bigger, he is client | ||
iamserver = true | ||
|
||
case 1: | ||
// my nonce bigger, i am client | ||
iamserver = false | ||
|
||
case 0: |
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.
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.
This is done.
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.
client.go
Outdated
@@ -74,6 +78,244 @@ func SelectOneOf(protos []string, rwc io.ReadWriteCloser) (string, error) { | |||
return "", ErrNotSupported | |||
} | |||
|
|||
// Performs protocol negotiation with the simultaneous open extension; the returned boolean | |||
// indicator will be true if we should act as a server. | |||
func SelectWithSimopen(protos []string, rwc io.ReadWriteCloser) (string, bool, error) { |
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.
This is done.
client.go
Outdated
} | ||
|
||
func simOpen(protos []string, rwc io.ReadWriteCloser) (string, bool, error) { | ||
retries := 3 |
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 agree, the likelihood is abysmal. Have removed this.
client.go
Outdated
var mybig, peerbig big.Int | ||
var iamserver bool | ||
mybig.SetBytes(mynonce) | ||
peerbig.SetBytes(peernonce) | ||
|
||
switch mybig.Cmp(&peerbig) { | ||
case -1: | ||
// peer nonce bigger, he is client | ||
iamserver = true | ||
|
||
case 1: | ||
// my nonce bigger, i am client | ||
iamserver = false | ||
|
||
case 0: |
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.
This is done.
client.go
Outdated
|
||
// this skips pipelined protocol negoatiation | ||
// keep reading until the token starts with select: | ||
if strings.HasPrefix(tok, "select:") { |
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.
@Stebalien I am not sure what you mean here.
Are you talking about the exact scenario where a peer sends "mutlsitream"|"iamclient"|"select:"|"select:nonce"
where that first "select:" is a security protocol the user wants to negotiate ? I agree this scenario will create problems.
But, is this the ONLY scenario you have in mind ? Does this really warrant fixing ?
@Stebalien Have addressed all your comments. Please can you take a look ? |
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.
Some nits here, but one more serious point: I really think we should reduce the size of the random value we send. A collision probability of 2^-32 should be good enough here, and would simplify things quite a bit.
client.go
Outdated
} | ||
|
||
var iamserver bool | ||
switch bytes.Compare(mynonce, peernonce) { |
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.
Will this yield the correct result if one peer sends a "2" and the other one a "0000001"?
On a more general note (and as I've argued on the specs PR), I still believe that using a 256-bit number here is excessive. We definitely can live with a collision probability of 2^-64 (for that matter, 2^-32 would be totally fine as well, and might simplify stuff in JS, where 64 bit numbers tend to be more problematic), which would allow us to use a simple uint64
/ uint32
comparison here.
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.
why? It doesn't cost anything to use bigger nonces, which makes the probability of collision astronomically low.
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.
Why? Because it simplifies the code. I'm still not sure if bytes.Compare
will give the correct result if the number is encoded with leading 0s.
With a 32 bit nonce the chance is already astronomically small. At this probability, you'd expect a collision every 2^31 connection attempts. Assuming you're performing 10 simultaneous connects per second (I don't think any node will ever do this), you'll have to wait for almost 7 years until you get the first collision. With a 64 bit nonce, you'd have to wait about 30 billion years, twice the age of the universe.
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 the idea of using a random uint64 is a reasonable one and have made the change in the PR. Please let me know if there are any strong objections against this and we can discuss this in depth.
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.
@marten-seemann the code was correct. It took a random byte string, encoded it, decoded it, then compared the bytes.
Honestly, that's a lot simpler than comparing a 64bit number as we're not relying on encoding order at all.
However, sending a raw number also works and I agree that 32 bytes was overkill for our purposes.
client.go
Outdated
return "", false, err | ||
} | ||
|
||
peernonce, err := base64.StdEncoding.DecodeString(tok[7:]) |
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.
peernonce, err := base64.StdEncoding.DecodeString(tok[7:]) | |
peernonce, err := base64.StdEncoding.DecodeString(tok[len(tieBreakerPrefix):]) |
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.
This is done.
client.go
Outdated
case 0: | ||
return "", false, errors.New("failed client selection; identical nonces!") | ||
|
||
default: |
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 don't think you need a default
when using bytes.Compare
.
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.
This has been removed.
client.go
Outdated
// peer nonce bigger, he is client | ||
iamserver = true | ||
|
||
case 1: |
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.
You actually don't need this case, iamserver
is already initialized to false
.
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 this helps readability of the code a bit, so keeping it around.
client.go
Outdated
if err != nil { | ||
return "", err | ||
} | ||
if tok != "initiator" { |
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.
Maybe make this a const
? I see you're using it a few times.
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.
This is done for both initiator and responder.
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.
Have addressed all your review comments. Thanks !
client.go
Outdated
// peer nonce bigger, he is client | ||
iamserver = true | ||
|
||
case 1: |
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 this helps readability of the code a bit, so keeping it around.
client.go
Outdated
case 0: | ||
return "", false, errors.New("failed client selection; identical nonces!") | ||
|
||
default: |
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.
This has been removed.
client.go
Outdated
if err != nil { | ||
return "", err | ||
} | ||
if tok != "initiator" { |
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.
This is done for both initiator and responder.
client.go
Outdated
return "", false, err | ||
} | ||
|
||
peernonce, err := base64.StdEncoding.DecodeString(tok[7:]) |
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.
This is done.
client.go
Outdated
} | ||
|
||
var iamserver bool | ||
switch bytes.Compare(mynonce, peernonce) { |
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 the idea of using a random uint64 is a reasonable one and have made the change in the PR. Please let me know if there are any strong objections against this and we can discuss this in depth.
client.go
Outdated
iamserver = true | ||
} else if peerNone < myNonce { | ||
// my nonce bigger, i am client | ||
iamserver = false |
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.
Probably nicer:
if peerNonce == myNonce {
return ...
}
iamserver := peerNonce > myNonce
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.
Done.
client.go
Outdated
return "", false, err | ||
} | ||
|
||
peerNone, err := strconv.ParseUint(tok[len(tieBreakerPrefix):], 10, 64) |
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.
Typo? peerNonce
?
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.
Done.
Have addressed all review comments so far. Please approve if happy and we can ship ! |
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.
feel free to ignore the nits.
} | ||
|
||
switch tok { | ||
case "iamclient": |
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.
nit: define a string constant.
if err != nil { | ||
return "", false, err | ||
} |
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.
nit: do we need this if err
? Or can we just return proto, false, err
.
case "na": | ||
return selectProtosOrFail(protos[1:], rwc) | ||
default: | ||
return "", errors.New("unexpected response: " + tok) |
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.
nit: fmt.Errorf("unexpected response: %s", tok)
is technically more idiomatic (although it really doesn't matter).
randBytes := make([]byte, 8) | ||
_, err := rand.Read(randBytes) | ||
if err != nil { | ||
return "", false, err | ||
} | ||
myNonce := binary.LittleEndian.Uint64(randBytes) |
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.
nit: it's probably slightly simpler to do:
var myNonce uint64
if err := binary.Read(rand.Reader, binary.LittleEndian, &myNonce); err != nil {
...
}
``
return "", false, err | ||
} | ||
|
||
var iamserver bool |
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.
nit: drop this line.
tok, err = ReadNextToken(rwc) | ||
|
||
if err == io.EOF { | ||
return "", ErrNotSupported |
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.
EOF will happen when the remote side gives up because we have no protocols in common. In general, it's best to avoid returning EOF for actual error cases anyways (usually, you want to either return ErrUnexpectedEOF, or a better error like we're doing here).
For the record, I find the use of a uint strictly worse than the original spec from a protocol standpoint; but then again it doesn't matter much at the end of the day. |
The Golang implementation does not retry. See multiformats/go-multistream#42 (comment).
For libp2p/go-libp2p#1039.
Implements this Spec: libp2p/specs#196
TBD: