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

p2p gossip #211

Merged
merged 15 commits into from Nov 12, 2018
Merged

p2p gossip #211

merged 15 commits into from Nov 12, 2018

Conversation

@zalmen
Copy link
Contributor

@zalmen zalmen commented Oct 18, 2018

No description provided.

@y0sher y0sher mentioned this pull request Nov 5, 2018
@@ -186,11 +186,18 @@ func (app *SpacemeshApp) startSpacemesh(cmd *cobra.Command, args []string) {
// start p2p services
log.Info("Initializing P2P services")
swarm, err := p2p.New(app.Config.P2P)
if err != nil {
log.Error("Error starting p2p services, err: %v", err)

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

we should have different error prints than the one below, otherwise it will be hard to know what really failed

@@ -64,6 +71,7 @@ BOOTLOOP:
case <-timeout.C:
return ErrFailedToBoot
case err := <-reschan:
i++
if err == nil {
return ErrFoundOurself

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

why is it implicit? Why not return ErrFoundOurself instead of nil (over the channel)?

This comment has been minimized.

@y0sher

y0sher Nov 11, 2018
Contributor

because this isn't really an error from this side. we tried to lookup a node, and we found it. hence error is nil.
this is an error only in the bootstrap context.

This comment has been minimized.

@zalmen

zalmen Nov 12, 2018
Author Contributor

as we talked offline - it is a valid case that a Lookup call on the local id will return the local node, it can happen if the local node is trying to bootstrap for the second time (in the case where it got killed after the first lookup). It is also possible to get itself in the first time the bootstrap is running. Anyways, in the case where the lookup outputs the local node we should continue with the bootstrap process and not fail it.

break BOOTLOOP
}
d.local.Warning("%d lookup didn't bootstrap the routing table", i)

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

merge the two log prints

time.Sleep(LookupIntervals)
}
}
return nil // succeed
}

func (d *KadDHT) healthLoop() {

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

not used - remove

}
}

func (d *KadDHT) refresh() error {

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

not used - remove

@@ -37,6 +39,12 @@ func (pm protocolMessage) Data() []byte {
}

type swarm struct {
started uint32

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

You should add comments to all of the members below - hard to understand their purpose just from their names


peer, err := s.dht.Lookup(peerPubKey) // blocking, might issue a network lookup that'll take time.
peer, conn = s.gossip.Peer(peerPubKey) // check if he's a neighbor

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

IMO this shouldn't concern swarm

// ErrBadFormat1 could'nt deserialize the payload
ErrBadFormat1 = errors.New("bad msg format, could'nt deserialize 1")
// ErrBadFormat2 could'nt deserialize the protocol message payload
ErrBadFormat2 = errors.New("bad msg format, could'nt deserialize 2")

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

why do we need two errors that look the same?

func PrepareMessage(ns net.NetworkSession, data []byte) ([]byte, error) {
encPayload, err := ns.Encrypt(data)
if err != nil {
return nil, fmt.Errorf("aborting send - failed to encrypt payload: %v", err)

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

this log print is no longer relevant, change it


final, err := proto.Marshal(cmd)
if err != nil {
e := fmt.Errorf("aborting send - invalid msg format %v", err)

This comment has been minimized.

@zalmen

zalmen Nov 11, 2018
Author Contributor

this log print is no longer relevant, change it

func (s *Neighborhood) Broadcast(msg []byte) error {

if len(s.peers) == 0 {
panic("WHOHO")

This comment has been minimized.

@y0sher

y0sher Nov 12, 2018
Contributor

use an error instead

@y0sher
Copy link
Contributor

@y0sher y0sher commented Nov 12, 2018

Finally this PR is ready to merge, the comments mentioned here and some more are going to be implemented soon in a another PR.

@y0sher
y0sher approved these changes Nov 12, 2018
@y0sher y0sher merged commit 19631f8 into develop Nov 12, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@y0sher y0sher deleted the p2p-gossip branch Nov 25, 2018
@y0sher y0sher mentioned this pull request Dec 6, 2018
@y0sher y0sher restored the p2p-gossip branch May 29, 2019
@y0sher y0sher deleted the p2p-gossip branch May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants