refactor for transport changes #299

Stebalien · 2018-03-13T04:25:04Z

Also, make the libp2p constructor fully useful. There should now be no need to manually construct a swarm/host. That's actually where most of the new code comes from (hence the +450 lines of (actual) code). This allows me to actually use the libp2p constructor in go-ipfs.

DO NOT MERGE (yet)
Part of: #297

Stebalien

While I usually don't like dynamic typing or duck typing, I figured that the usability benefit of duck typing in the libp2p constructor significantly outweighs the drawbacks. Anyways, errors should be caught as soon as the application starts.

Stebalien · 2018-03-13T04:29:44Z

config/config.go

+type AddrsFactory = bhost.AddrsFactory
+
+// NATManagerC is a NATManager constructor.
+type NATManagerC func(inet.Network) bhost.NATManager


Could also call this NATManagerConstructor (hence the C) or NATManagerFactory. However, while technically exported, users should never need to actually use this type (or any of the other *C types).

Still better than in Java world.

Stebalien · 2018-03-13T04:30:19Z

config/config.go

@@ -0,0 +1,173 @@
+package config


I moved this logic into a separate package for organization (it was growing a bit large). I left all the actual options in the main libp2p package.

Stebalien · 2018-03-13T04:30:38Z

config/config.go

+type Config struct {
+	Transports         []TptC
+	Muxers             []MsMuxC
+	SecurityTransports []MsSecC


One can now easily plug in new security transports.

Stebalien · 2018-03-13T04:31:30Z

config/config.go

+// Config describes a set of settings for a libp2p node
+type Config struct {
+	Transports         []TptC
+	Muxers             []MsMuxC


Instead of taking a single transport, we now take an array of named multiplexer transports. Why not a map? We need the order (priority).

Stebalien · 2018-03-13T04:51:54Z

libp2p.go

-	if err != nil {
-		return nil, err
+// EnableRelay configures libp2p to enable the relay transport.
+func EnableRelay(options ...circuit.RelayOpt) Option {


This isn't the most user-friendly interface. It also kind of sucks that one has to import the relay package to call this.

Stebalien · 2018-03-13T04:55:14Z

p2p/host/basic/basic_host.go

@@ -270,10 +243,6 @@ func (h *BasicHost) newStreamHandler(s inet.Stream) {
 	}

 	s.SetProtocol(protocol.ID(protoID))
-
-	if h.bwc != nil {
-		s = mstream.WrapStream(s, h.bwc)


The swarm now takes care of this before it even hits the identify service.

Stebalien · 2018-03-13T04:55:53Z

p2p/test/reconnects/reconnect_test.go

 )

-func init() {
-	// change the garbage collect timeout for testing.
-	ps.GarbageCollectTimeout = 10 * time.Millisecond


Stebalien · 2018-03-13T05:04:43Z

p2p/host/basic/basic_host.go


 	// ConnManager is a libp2p connection manager
 	ConnManager ifconnmgr.ConnManager
-
-	// Relay indicates whether the host should use circuit relay transport
-	EnableRelay bool


Handled by the libp2p constructor.

Stebalien · 2018-03-13T05:05:45Z

p2p/host/basic/basic_host.go

-
-	// BandwidthReporter is used for collecting aggregate metrics of the
-	// bandwidth used by various protocols.
-	BandwidthReporter metrics.Reporter


Was only needed by the identity service because the old swarm didn't enable the bandwidth reporter early enough. That's no longer the case so the host doesn't need this now.

Stebalien · 2018-03-13T05:07:23Z

p2p/host/basic/basic_host.go

@@ -536,11 +497,6 @@ func (h *BasicHost) Close() error {
 	return h.proc.Close()
 }

-// GetBandwidthReporter exposes the Host's bandiwth metrics reporter
-func (h *BasicHost) GetBandwidthReporter() metrics.Reporter {


This isn't a part of the Host interface and I can't find anything that actually uses it. Really, users should just hang onto the bandwidth reporter that they passed in.

vyzo

this overall looks good, but i skimmed through the reflection details.

vyzo · 2018-03-13T06:13:26Z

libp2p.go

-
-	return tpt
+// NATPortMap configures libp2p to use the default NATManager. The default
+// NATManager will attempt to punch holes in your NAT.


this comment is technically incorrect -- doesn't the nat manager attempt to open ports using UPnP?
It doesn't really use hole punching.

Yes, good point.

vyzo · 2018-03-13T06:20:36Z

config/config.go

+	if ps == nil {
+		ps = pstore.NewPeerstore()
+	}
+	if !cfg.Insecure {


Why do we keep lugging around the insecure option?

We use it for debugging. It's actually really useful. However, it's designed to be an all or nothing "disable all security" option.

fair enough! I can see it being useful in analysing wire protocols without encryption.

whyrusleeping

LGTM (reviewed in a video call together)

Stebalien · 2018-03-13T23:41:06Z

@whyrusleeping massively reduced the code duplication for the reflection stuff. It turned out to be much easier than I had thought. I left the optional error in the transport constructor return type but broke the code int a bunch of smaller, readable functions.

hsanjuan · 2018-03-15T13:42:02Z

I was looking to bubble go-reuseport with a fixed go-log that does not panic randomly. Or do you prefer to keep libp2p frozen until all these changes come in?

Stebalien · 2018-03-15T18:10:24Z

Feel free (that's why I'm merging these all at the same time).

Kubuxu · 2018-03-27T18:28:02Z

config/config.go

+	ConnManager        ifconnmgr.ConnManager
+	AddrsFactory       bhost.AddrsFactory
+	Filters            *filter.Filters
+	NATManager         NATManagerC


Some spacing ques would be good here. Like Relay and RelayOpts can have their own section.

Kubuxu · 2018-03-27T18:28:47Z

config/config.go

+func (cfg *Config) NewNode(ctx context.Context) (host.Host, error) {
+	// Check this early. Prevents us from even *starting* without verifying this.
+	if pnet.ForcePrivateNetwork && cfg.Protector == nil {
+		log.Error("tried to dial with no Private Network Protector but usage" +


dial is not accurate here

Kubuxu · 2018-03-27T18:29:19Z

config/config.go

+		return nil, err
+	}
+
+	// Create a new blank peerstore if none was passed in


Comment not accurate, either change behaviour or the comment.

Good catch. I moved the defaults to a special FallbackDefaults option.

Kubuxu · 2018-03-27T18:31:54Z

config/muxer.go

+		}, nil
+	}
+
+	fn, err := makeConstructor(m, muxType, muxArgTypes)


IMO s/fn/ctor/

Kubuxu · 2018-03-27T18:34:38Z

package.json

+    {
+      "author": "whyrusleeping",
+      "hash": "QmYkxWtBWqkj6SWCiFN2a3xnnk5womAQYd9Kh68LJJR7jz",
+      "name": "go-ws-transport",


do we have to depend on a transport?

It's a default transport so yes unless we want to make it not a default transport. Note: we used to depend on it in go-libp2p-swarm but I moved all default transport construction here.

Kubuxu

One invalid comment, and a nitpick.

I am not a fan of voodoo magic but I don't think there is a better way.

Stebalien · 2018-03-27T23:06:03Z

I am not a fan of voodoo magic but I don't think there is a better way.

I completely agree. My first attempt involved defining some nice interfaces but it turned into a mess.

Kubuxu

SGWM

Also, make the libp2p constructor fully useful. There should now be no need to manually construct a swarm/host.

ghost assigned Stebalien Mar 13, 2018

ghost added the status/in-progress In progress label Mar 13, 2018

Stebalien commented Mar 13, 2018

View reviewed changes

Stebalien requested review from vyzo and whyrusleeping March 13, 2018 05:08

Stebalien force-pushed the feat/refactor branch from ae4c193 to df17003 Compare March 13, 2018 05:09

Stebalien mentioned this pull request Mar 13, 2018

Transport + Swarm refactor #297

Closed

72 tasks

vyzo reviewed Mar 13, 2018

View reviewed changes

whyrusleeping approved these changes Mar 13, 2018

View reviewed changes

Stebalien force-pushed the feat/refactor branch 5 times, most recently from 12d3618 to fd3f3d9 Compare March 15, 2018 00:37

Stebalien force-pushed the feat/refactor branch 4 times, most recently from 99b1900 to 29228f2 Compare March 26, 2018 22:52

Stebalien requested a review from Kubuxu March 27, 2018 18:16

Kubuxu reviewed Mar 27, 2018

View reviewed changes

config/muxer.go Outdated

}, nil

}

fn, err := makeConstructor(m, muxType, muxArgTypes)

Copy link

Member

Kubuxu Mar 27, 2018

Choose a reason for hiding this comment

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

IMO s/fn/ctor/

Kubuxu reviewed Mar 27, 2018

View reviewed changes

Kubuxu approved these changes Mar 28, 2018

View reviewed changes

Stebalien force-pushed the feat/refactor branch 3 times, most recently from e3579af to cfe6ab5 Compare March 29, 2018 09:41

Stebalien added 2 commits June 4, 2018 21:22

refactor for transport changes

41c6834

Also, make the libp2p constructor fully useful. There should now be no need to manually construct a swarm/host.

gx publish 6.0.0

a612c7d

Stebalien force-pushed the feat/refactor branch from cfe6ab5 to a612c7d Compare June 5, 2018 04:23

Stebalien merged commit 8bdf9d0 into master Jun 6, 2018

ghost removed the status/in-progress In progress label Jun 6, 2018

Stebalien deleted the feat/refactor branch June 6, 2018 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor for transport changes #299

refactor for transport changes #299

Stebalien commented Mar 13, 2018 •

edited

Loading

Stebalien left a comment

Stebalien Mar 13, 2018

Kubuxu Mar 27, 2018

Stebalien Mar 13, 2018

Stebalien Mar 13, 2018

Stebalien Mar 13, 2018

Stebalien Mar 13, 2018

Stebalien Mar 13, 2018

Stebalien Mar 13, 2018

Stebalien Mar 13, 2018

Stebalien Mar 13, 2018

Stebalien Mar 13, 2018

vyzo left a comment

vyzo Mar 13, 2018 •

edited

Loading

Stebalien Mar 13, 2018

vyzo Mar 13, 2018

Stebalien Mar 13, 2018 •

edited

Loading

vyzo Mar 13, 2018

whyrusleeping left a comment

Stebalien commented Mar 13, 2018

hsanjuan commented Mar 15, 2018

Stebalien commented Mar 15, 2018

Kubuxu Mar 27, 2018

Kubuxu Mar 27, 2018

Kubuxu Mar 27, 2018

Stebalien Mar 27, 2018

Kubuxu Mar 27, 2018

Kubuxu Mar 27, 2018

Stebalien Mar 27, 2018

Kubuxu left a comment

Stebalien commented Mar 27, 2018

Kubuxu left a comment

refactor for transport changes #299

refactor for transport changes #299

Conversation

Stebalien commented Mar 13, 2018 • edited Loading

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo left a comment

Choose a reason for hiding this comment

vyzo Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping left a comment

Choose a reason for hiding this comment

Stebalien commented Mar 13, 2018

hsanjuan commented Mar 15, 2018

Stebalien commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu left a comment

Choose a reason for hiding this comment

Stebalien commented Mar 27, 2018

Kubuxu left a comment

Choose a reason for hiding this comment

Stebalien commented Mar 13, 2018 •

edited

Loading

vyzo Mar 13, 2018 •

edited

Loading

Stebalien Mar 13, 2018 •

edited

Loading