whisper: new protocol-level message codes #15802
Conversation
There was a problem hiding this comment.
That was a first pass. For instance, I have not yet checked if the test coverage was good. I suggest that:
- You use a bloom filters with all 1s for full nodes
- You add more comments to make it easier for others (i.e. status, parity and me) to comprehend what this does
- You break the filter test into separate tests, so that the immediate cause of the problem can be found.
whisper/whisperv6/api.go
Outdated
| if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
can be rewritten: return (err != nil), err. Does this need to be part of this changelist, though?
There was a problem hiding this comment.
my style is easier to read, it's more explicit.
whisper/whisperv6/api.go
Outdated
| if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil |
whisper/whisperv6/doc.go
Outdated
| @@ -57,6 +56,7 @@ const ( | |||
| aesKeyLength = 32 | |||
| AESNonceLength = 12 | |||
| keyIdSize = 32 | |||
| bloomFilterSize = 64 | |||
There was a problem hiding this comment.
nitpick: I would like to see a unit for that "size" Bytes? Bits? String length? A comment like the one when you define DefaultSyncAllowance below.
whisper/whisperv6/envelope.go
Outdated
| return e.bloom | ||
| } | ||
|
|
||
| func TopicToBloom(topic TopicType) []byte { |
whisper/whisperv6/envelope.go
Outdated
| var index [3]int | ||
| for j := 0; j < 3; j++ { | ||
| index[j] = int(topic[j]) | ||
| if (topic[3] & powers[j]) != 0 { |
There was a problem hiding this comment.
imho, it would be more readable to write it: topic[3] & (1 << j). Creating powers on the stack for just this purpose is wasteful.
There was a problem hiding this comment.
thanx, it was good one!
whisper/whisperv6/whisper.go
Outdated
| return val.(float64) | ||
| } | ||
|
|
||
| func (w *Whisper) BloomFilter() []byte { |
whisper/whisperv6/whisper.go
Outdated
| return val.([]byte) | ||
| } | ||
|
|
||
| func (w *Whisper) BloomFilterTolerance() []byte { |
| for _, p := range arr { | ||
| err := p.notifyAboutBloomFilterChange(bloom) | ||
| if err != nil { | ||
| // allow one retry |
There was a problem hiding this comment.
I'm curious: why only one?
Also, since this is a network-related function, shouldn't you wait a little before retrying?
Finally, since there's a network call in that loop, why not start a goroutine for each iteration and be done with that function? You're not waiting for the result anyway, beyond reporting that something went wrong.
There was a problem hiding this comment.
this is only used in one api function, and it should wait till all the peers are notified, IMHO.
or maybe not, in which case the entire function should be called as a go routine.
but you are right, we should refactor this function later, i just don't want to do it in haste.
There was a problem hiding this comment.
having the entire function as a goroutine would have the same issue. fine, let's do that later
whisper/whisperv6/whisper.go
Outdated
| for i := 0; i < bloomFilterSize; i++ { | ||
| f := filter[i] | ||
| s := sample[i] | ||
| if ((f | s) ^ f) != 0 { |
There was a problem hiding this comment.
Not sure what you're trying to do, but wouldn't this be more understandable as f & s == f ?
whisper/whisperv6/peer_test.go
Outdated
|
|
||
| if cnt == 0 { | ||
| t.Fatalf("no matching peers found.") | ||
| func isBloomFilterEqual(a, b []byte) bool { |
There was a problem hiding this comment.
This function should be available in the general api, not just in a test file.
whisper/whisperv6/doc.go
Outdated
| TopicLength = 4 // in bytes | ||
| signatureLength = 65 // in bytes | ||
| aesKeyLength = 32 // in bytes | ||
| AESNonceLength = 12 // in bytes |
There was a problem hiding this comment.
I really just meant for the Size constants. Length are usually in bytes.
whisper/whisperv6/envelope.go
Outdated
| @@ -51,6 +51,7 @@ type Envelope struct { | |||
|
|
|||
| // size returns the size of envelope as it is sent (i.e. public fields only) | |||
| func (e *Envelope) size() int { | |||
| const EnvelopeHeaderLength = 20 | |||
There was a problem hiding this comment.
Why isn't this constant declared in doc.go ?
There was a problem hiding this comment.
because it is used only once in this function, and never again. we don't need a global constant if it can be local.
There was a problem hiding this comment.
I disagree: in spite of momentarily only being used in this specific function, this constant is a characteristic of the envelope format, and therefore its scope is the whole envelope code. So it should definitely be a top-level constant, if not documented in doc.go.
There was a problem hiding this comment.
To press the point further, there are two reasons this should be defined globally:
- Anybody who would need it will not be scanning the code to find if it's already defined. They will look at
doc.goand the first lines ofenvelope.go. - documentation purposes: once again, not having to scan the entire code to get some info about the format.
whisper/whisperv6/api.go
Outdated
| if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
If the error is non-nil, the caller will not see the return value. You can use return true, err.
whisper/whisperv6/peer_test.go
Outdated
| node.shh.SetMinimumPowTest(0.00000001) | ||
| node.shh.SetMinimumPoW(masterPow) | ||
| node.shh.SetBloomFilter(b) | ||
| if !isBloomFilterEqual(node.shh.BloomFilter(), masterBloomFilter) { |
There was a problem hiding this comment.
Please use bytes.Equal and delete isBloomFilterEqual
This PR contains complete functionality of PoW and Bloom Filter exchange protocol.
This is the main feature of v.6.