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

whisper: new protocol-level message codes #15802

Merged
merged 6 commits into from
Jan 12, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions whisper/whisperv6/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,15 +113,32 @@ func (api *PublicWhisperAPI) Info(ctx context.Context) Info {
// SetMaxMessageSize sets the maximum message size that is accepted.
// Upper limit is defined by MaxMessageSize.
func (api *PublicWhisperAPI) SetMaxMessageSize(ctx context.Context, size uint32) (bool, error) {
return true, api.w.SetMaxMessageSize(size)
err := api.w.SetMaxMessageSize(size)
if err != nil {
return false, err
}
return true, nil
Copy link
Member

Choose a reason for hiding this comment

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

can be rewritten: return (err != nil), err. Does this need to be part of this changelist, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my style is easier to read, it's more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the error is non-nil, the caller will not see the return value. You can use return true, err.

}

// SetMinPow sets the minimum PoW for a message before it is accepted.
// SetMinPow sets the minimum PoW, and notifies the peers.
func (api *PublicWhisperAPI) SetMinPoW(ctx context.Context, pow float64) (bool, error) {
return true, api.w.SetMinimumPoW(pow)
err := api.w.SetMinimumPoW(pow)
if err != nil {
return false, err
}
return true, nil
Copy link
Member

Choose a reason for hiding this comment

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

Same thing as above

}

// SetBloomFilter sets the new value of bloom filter, and notifies the peers.
func (api *PublicWhisperAPI) SetBloomFilter(ctx context.Context, bloom hexutil.Bytes) (bool, error) {
err := api.w.SetBloomFilter(bloom)
if err != nil {
return false, err
}
return true, nil
}

// MarkTrustedPeer marks a peer trusted. , which will allow it to send historic (expired) messages.
// MarkTrustedPeer marks a peer trusted, which will allow it to send historic (expired) messages.
// Note: This function is not adding new nodes, the node needs to exists as a peer.
func (api *PublicWhisperAPI) MarkTrustedPeer(ctx context.Context, enode string) (bool, error) {
n, err := discover.ParseNode(enode)
Expand Down
18 changes: 8 additions & 10 deletions whisper/whisperv6/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
)

const (
EnvelopeVersion = uint64(0)
ProtocolVersion = uint64(6)
ProtocolVersionStr = "6.0"
ProtocolName = "shh"
Expand All @@ -52,11 +51,12 @@ const (
paddingMask = byte(3)
signatureFlag = byte(4)

TopicLength = 4
signatureLength = 65
aesKeyLength = 32
AESNonceLength = 12
keyIdSize = 32
TopicLength = 4 // in bytes
signatureLength = 65 // in bytes
aesKeyLength = 32 // in bytes
AESNonceLength = 12 // in bytes
Copy link
Member

Choose a reason for hiding this comment

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

I really just meant for the Size constants. Length are usually in bytes.

keyIdSize = 32 // in bytes
bloomFilterSize = 64 // in bytes

MaxMessageSize = uint32(10 * 1024 * 1024) // maximum accepted size of a message.
DefaultMaxMessageSize = uint32(1024 * 1024)
Expand All @@ -68,10 +68,8 @@ const (
expirationCycle = time.Second
transmissionCycle = 300 * time.Millisecond

DefaultTTL = 50 // seconds
SynchAllowance = 10 // seconds

EnvelopeHeaderLength = 20
DefaultTTL = 50 // seconds
DefaultSyncAllowance = 10 // seconds
)

type unknownVersionError uint64
Expand Down
36 changes: 33 additions & 3 deletions whisper/whisperv6/envelope.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,16 @@ type Envelope struct {
Data []byte
Nonce uint64

pow float64 // Message-specific PoW as described in the Whisper specification.
hash common.Hash // Cached hash of the envelope to avoid rehashing every time.
// Don't access hash directly, use Hash() function instead.
pow float64 // Message-specific PoW as described in the Whisper specification.

// the following variables should not be accessed directly, use the corresponding function instead: Hash(), Bloom()
hash common.Hash // Cached hash of the envelope to avoid rehashing every time.
bloom []byte
}

// size returns the size of envelope as it is sent (i.e. public fields only)
func (e *Envelope) size() int {
const EnvelopeHeaderLength = 20
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't this constant declared in doc.go ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it is used only once in this function, and never again. we don't need a global constant if it can be local.

Copy link
Member

@gballet gballet Jan 8, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.go and the first lines of envelope.go.
  • documentation purposes: once again, not having to scan the entire code to get some info about the format.

return EnvelopeHeaderLength + len(e.Data)
}

Expand Down Expand Up @@ -227,3 +230,30 @@ func (e *Envelope) Open(watcher *Filter) (msg *ReceivedMessage) {
}
return msg
}

// Bloom maps 4-bytes Topic into 64-byte bloom filter with 3 bits set (at most).
func (e *Envelope) Bloom() []byte {
if e.bloom == nil {
e.bloom = TopicToBloom(e.Topic)
}
return e.bloom
}

// TopicToBloom converts the topic (4 bytes) to the bloom filter (64 bytes)
func TopicToBloom(topic TopicType) []byte {
Copy link
Member

Choose a reason for hiding this comment

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

Public functions need a comment

b := make([]byte, bloomFilterSize)
Copy link
Member

Choose a reason for hiding this comment

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

a more descriptive name, maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is good enough. a comment is added as well.

var index [3]int
for j := 0; j < 3; j++ {
index[j] = int(topic[j])
if (topic[3] & (1 << uint(j))) != 0 {
index[j] += 256
}
}

for j := 0; j < 3; j++ {
byteIndex := index[j] / 8
bitIndex := index[j] % 8
b[byteIndex] = (1 << uint(bitIndex))
}
return b
}
54 changes: 50 additions & 4 deletions whisper/whisperv6/peer.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ type Peer struct {

trusted bool
powRequirement float64
bloomFilter []byte // may contain nil in case of full node
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of having nil all over the place, especially in things that amount to references. Old C reflex I guess. Wouldn't it make sense to have bloomFilter to be all 1s if this is a full node? Looking at the code, I see no reason to separate the case of a full node from the case where the some stuff is filtered. The full node case is essentially: filter nothing, and making the distinction is just adding confusion by adding more if bloomFilter == nils that is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought about productivity: most nodes will be full nodes, so comparing 64 bytes in a cycle would be an overkill. but i have to admit that you are absolutely right -- readability (and good engineering practices in gerenral) have higher priority over productivity.

however, my first priority today is to deliver the minimum viable product of v6. so, you are welcome to do the necessary refactoring after v6 is realeased.


known *set.Set // Messages already known by the peer to avoid wasting bandwidth

Expand Down Expand Up @@ -74,8 +75,12 @@ func (p *Peer) handshake() error {
// Send the handshake status message asynchronously
errc := make(chan error, 1)
go func() {
errc <- p2p.Send(p.ws, statusCode, ProtocolVersion)
pow := p.host.MinPow()
powConverted := math.Float64bits(pow)
bloom := p.host.BloomFilter()
errc <- p2p.SendItems(p.ws, statusCode, ProtocolVersion, powConverted, bloom)
}()

// Fetch the remote status packet and verify protocol match
packet, err := p.ws.ReadMsg()
if err != nil {
Expand All @@ -85,14 +90,42 @@ func (p *Peer) handshake() error {
return fmt.Errorf("peer [%x] sent packet %x before status packet", p.ID(), packet.Code)
}
s := rlp.NewStream(packet.Payload, uint64(packet.Size))
peerVersion, err := s.Uint()
_, err = s.List()
if err != nil {
return fmt.Errorf("peer [%x] sent bad status message: %v", p.ID(), err)
}
peerVersion, err := s.Uint()
if err != nil {
return fmt.Errorf("peer [%x] sent bad status message (unable to decode version): %v", p.ID(), err)
}
if peerVersion != ProtocolVersion {
return fmt.Errorf("peer [%x]: protocol version mismatch %d != %d", p.ID(), peerVersion, ProtocolVersion)
}
// Wait until out own status is consumed too

// only version is mandatory, subsequent parameters are optional
powRaw, err := s.Uint()
if err == nil {
pow := math.Float64frombits(powRaw)
if math.IsInf(pow, 0) || math.IsNaN(pow) || pow < 0.0 {
return fmt.Errorf("peer [%x] sent bad status message: invalid pow", p.ID())
}
p.powRequirement = pow

var bloom []byte
err = s.Decode(&bloom)
if err == nil {
sz := len(bloom)
if sz != bloomFilterSize && sz != 0 {
return fmt.Errorf("peer [%x] sent bad status message: wrong bloom filter size %d", p.ID(), sz)
}
if isFullNode(bloom) {
Copy link
Member

Choose a reason for hiding this comment

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

insisting on my previous comment: I think it makes no sense to distinguish between a full and "half" node, it just makes the code harder to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree, thanx for pointing out!

p.bloomFilter = nil
} else {
p.bloomFilter = bloom
}
}
}

if err := <-errc; err != nil {
return fmt.Errorf("peer [%x] failed to send status packet: %v", p.ID(), err)
}
Expand Down Expand Up @@ -156,7 +189,7 @@ func (p *Peer) broadcast() error {
envelopes := p.host.Envelopes()
bundle := make([]*Envelope, 0, len(envelopes))
for _, envelope := range envelopes {
if !p.marked(envelope) && envelope.PoW() >= p.powRequirement {
if !p.marked(envelope) && envelope.PoW() >= p.powRequirement && p.bloomMatch(envelope) {
bundle = append(bundle, envelope)
}
}
Expand Down Expand Up @@ -186,3 +219,16 @@ func (p *Peer) notifyAboutPowRequirementChange(pow float64) error {
i := math.Float64bits(pow)
return p2p.Send(p.ws, powRequirementCode, i)
}

func (p *Peer) notifyAboutBloomFilterChange(bloom []byte) error {
return p2p.Send(p.ws, bloomFilterExCode, bloom)
}

func (p *Peer) bloomMatch(env *Envelope) bool {
if p.bloomFilter == nil {
// no filter - full node, accepts all envelops
return true
}

return bloomFilterMatch(p.bloomFilter, env.Bloom())
Copy link
Member

Choose a reason for hiding this comment

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

as per my previous comment, bloomFilter should never be nil, so this function becomes return bloomFilterMatch(p.bloomFilter, env.Bloom())

}
105 changes: 85 additions & 20 deletions whisper/whisperv6/peer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"bytes"
"crypto/ecdsa"
"fmt"
mrand "math/rand"
"net"
"sync"
"testing"
Expand Down Expand Up @@ -87,6 +88,9 @@ var nodes [NumNodes]*TestNode
var sharedKey []byte = []byte("some arbitrary data here")
var sharedTopic TopicType = TopicType{0xF, 0x1, 0x2, 0}
var expectedMessage []byte = []byte("per rectum ad astra")
var masterBloomFilter []byte
var masterPow = 0.00000001
var round int = 1

func TestSimulation(t *testing.T) {
// create a chain of whisper nodes,
Expand All @@ -104,27 +108,79 @@ func TestSimulation(t *testing.T) {
// check if each node have received and decrypted exactly one message
checkPropagation(t, true)

// send protocol-level messages (powRequirementCode) and check the new PoW requirement values
powReqExchange(t)
// check if Status message was correctly decoded
checkBloomFilterExchange(t)
checkPowExchange(t)

// send new pow and bloom exchange messages
resetParams(t)
round++

// node #1 sends one expected (decryptable) message
sendMsg(t, true, 1)

// check if each node (except node #0) have received and decrypted exactly one message
checkPropagation(t, false)

for i := 1; i < NumNodes; i++ {
time.Sleep(20 * time.Millisecond)
sendMsg(t, true, i)
}

// check if corresponding protocol-level messages were correctly decoded
checkPowExchangeForNodeZero(t)
checkBloomFilterExchange(t)

stopServers()
}

func resetParams(t *testing.T) {
// change pow only for node zero
masterPow = 7777777.0
nodes[0].shh.SetMinimumPoW(masterPow)

// change bloom for all nodes
masterBloomFilter = TopicToBloom(sharedTopic)
for i := 0; i < NumNodes; i++ {
nodes[i].shh.SetBloomFilter(masterBloomFilter)
}
}

func initBloom(t *testing.T) {
masterBloomFilter = make([]byte, bloomFilterSize)
_, err := mrand.Read(masterBloomFilter)
if err != nil {
t.Fatalf("rand failed: %s.", err)
}

msgBloom := TopicToBloom(sharedTopic)
masterBloomFilter = addBloom(masterBloomFilter, msgBloom)
for i := 0; i < 32; i++ {
masterBloomFilter[i] = 0xFF
}

if !bloomFilterMatch(masterBloomFilter, msgBloom) {
t.Fatalf("bloom mismatch on initBloom.")
}
}

func initialize(t *testing.T) {
initBloom(t)

var err error
ip := net.IPv4(127, 0, 0, 1)
port0 := 30303

for i := 0; i < NumNodes; i++ {
var node TestNode
b := make([]byte, bloomFilterSize)
copy(b, masterBloomFilter)
node.shh = New(&DefaultConfig)
node.shh.SetMinimumPowTest(0.00000001)
node.shh.SetMinimumPoW(masterPow)
node.shh.SetBloomFilter(b)
if !isBloomFilterEqual(node.shh.BloomFilter(), masterBloomFilter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use bytes.Equal and delete isBloomFilterEqual

t.Fatalf("bloom mismatch on init.")
}
node.shh.Start(nil)
topics := make([]TopicType, 0)
topics = append(topics, sharedTopic)
Expand Down Expand Up @@ -206,7 +262,7 @@ func checkPropagation(t *testing.T, includingNodeZero bool) {
for i := first; i < NumNodes; i++ {
f := nodes[i].shh.GetFilter(nodes[i].filerId)
if f == nil {
t.Fatalf("failed to get filterId %s from node %d.", nodes[i].filerId, i)
t.Fatalf("failed to get filterId %s from node %d, round %d.", nodes[i].filerId, i, round)
}

mail := f.Retrieve()
Expand Down Expand Up @@ -332,34 +388,43 @@ func TestPeerBasic(t *testing.T) {
}
}

func powReqExchange(t *testing.T) {
func checkPowExchangeForNodeZero(t *testing.T) {
cnt := 0
for i, node := range nodes {
for peer := range node.shh.peers {
if peer.powRequirement > 1000.0 {
t.Fatalf("node %d: one of the peers' pow requirement is too big (%f).", i, peer.powRequirement)
if peer.peer.ID() == discover.PubkeyID(&nodes[0].id.PublicKey) {
cnt++
if peer.powRequirement != masterPow {
t.Fatalf("node %d: failed to set the new pow requirement.", i)
}
}
}
}
if cnt == 0 {
t.Fatalf("no matching peers found.")
}
}

const pow float64 = 7777777.0
nodes[0].shh.SetMinimumPoW(pow)

// wait until all the messages are delivered
time.Sleep(64 * time.Millisecond)

cnt := 0
func checkPowExchange(t *testing.T) {
for i, node := range nodes {
for peer := range node.shh.peers {
if peer.peer.ID() == discover.PubkeyID(&nodes[0].id.PublicKey) {
cnt++
if peer.powRequirement != pow {
t.Fatalf("node %d: failed to set the new pow requirement.", i)
if peer.peer.ID() != discover.PubkeyID(&nodes[0].id.PublicKey) {
if peer.powRequirement != masterPow {
t.Fatalf("node %d: failed to exchange pow requirement in round %d; expected %f, got %f",
i, round, masterPow, peer.powRequirement)
}
}
}
}
}

if cnt == 0 {
t.Fatalf("no matching peers found.")
func checkBloomFilterExchange(t *testing.T) {
for i, node := range nodes {
for peer := range node.shh.peers {
if !isBloomFilterEqual(peer.bloomFilter, masterBloomFilter) {
t.Fatalf("node %d: failed to exchange bloom filter requirement in round %d. \n%x expected \n%x got",
i, round, masterBloomFilter, peer.bloomFilter)
}
}
}
}
Loading