first hack at a nice libp2p constructor #241

whyrusleeping · 2017-11-04T19:07:13Z

Trying to make using go-libp2p as simple as possible

Stebalien

Nice!

Stebalien · 2017-11-04T19:28:36Z

libp2p.go

+
+func DefaultConfig() *Config {
+	// Create a multiaddress
+	addr, err := ma.NewMultiaddr("/ip4/127.0.0.1/tcp/0")


Should this not be 0.0.0.0? Maybe, to just accept the address as a parameter?

I feel very uneasy about having defaults which open your ports to the world. AWS warns about it, systemd gets shitstorms about it, and anyone building something on top should probably not use the default anyway.

Anyone building something on top should probably not use the default anyway.

Then, IMO, it should be called "testing" or something like that. In this case, you're setting up a service that's supposed to be accessible from the internet. Users will be very confused if it doesn't listen on a public address by default. However, IMO, using a default address probably isn't the best idea anyways as there's no reasonable default port.

Stebalien · 2017-11-04T19:30:36Z

libp2p.go

+	yamux "github.com/whyrusleeping/go-smux-yamux"
+)
+
+type Config struct {


It would be kind of nice if this (or some subset with the key etc.) could be serialized/deserialized easily.

Yeah... that would be nice. Definitely something that can be done later though, As i think it deserves a bit more thought.

dignifiedquire · 2017-11-04T20:09:37Z

libp2p.go

+	Reporter    metrics.Reporter
+}
+
+func Construct(ctx context.Context, cfg *Config) (host.Host, error) {


I wonder if having New(ctx) and NewWithConfig(ctx, cfg) would be nicer. At least for me as a new user it would be more straightforward to understand what those methods do anf how they work

hsanjuan

This is kind of useful but note it introduces duality for "defaults" that will be harder to maintain. swarm.NewNetwork* provides a way of getting a default swarm. This provides a second parallel way.

hsanjuan · 2017-11-07T16:20:10Z

libp2p.go

+	// Set up stream multiplexer
+	tpt := msmux.NewBlankTransport()
+
+	// By default, support yamux and multiplex


It strikes me that here the supported transports are slightly different than here: https://github.com/libp2p/go-libp2p-swarm/blob/master/swarm.go

I thought that swarm.NewNetworkWithProtector would be the default way of doing things so any updates to that should happen in swarm?

At first sight, it seems that this function is just re-doing what PSTransport provides:

// PSTransport is the default peerstream transport that will be used by // any libp2p swarms. var PSTransport pst.Transport

Good catch, I'm actually thinking the best path forward here is to remove that logic from the swarm code, and make this place be the default. The reason being that swarm is much deeper in the dependency tree, and making it depend on extra packages makes it harder to update those packages.

That sounds good to me 👍

whyrusleeping · 2017-12-14T20:17:30Z

I want to try and pull some wisdom from this blog post into this PR: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

whyrusleeping · 2017-12-14T21:01:54Z

I'm just not sure what to make the functions operate on

dignifiedquire · 2017-12-14T21:24:54Z

I'm just not sure what to make the functions operate on

From my understanding this would be something like this

// current version in the example
basicHost, err := libp2p.NewWithCfg(ctx, &libp2p.Config{
	ListenAddrs: []ma.Multiaddr{addr},
})

// functional version 
basicHost, err := libp2p.New(ctx, libp2p.ListenAddrs(addr))

// functional config - default config
basicHost, err := libp2p.New(ctx)

with impl being something like this

func ListenAddrs(addrs ...ma.Multiaddr) func (*Config) error {
  return func (c *Config) error {
    c.ListenAddrs = addrs
  }
}

func New(ctx context.Context, options ...func(*Config) error) (host, error) {
  cfg := Config{}
  for _, option := range options {
    err := option(cfg)
    // handle err
  }
  return NewWithCfg(ctx, cfg)
}

whyrusleeping · 2017-12-14T21:28:27Z

@dignifiedquire yeah, having things operate on the config seems like the most reasonable way forward. I'll start on that.

What do you think about still having the option to use the config directly? I'm not really sure because it adds extra things for the user to have to think about.

dignifiedquire · 2017-12-14T21:31:06Z

What do you think about still having the option to use the config directly

Yeah I would drop that option for users to avoid confusion. We can use NewWithCfg and make it private, but having it exposed would be just confusing for users

whyrusleeping · 2017-12-15T00:09:19Z

There, how does that look?

dignifiedquire · 2017-12-15T13:41:54Z

libp2p.go

+
+type Option func(cfg *Config) error
+
+func Transports(tpts []transport.Transport) Option {


The function signatures are inconsistent, would be nice if they all either take an array or varargs, as currently two of them (Transports and ListenAddrs) take an array but ListenAddrStrings takes varargs.

I may have been a little tired writing this... >.>

dignifiedquire · 2017-12-15T13:42:49Z

libp2p.go

+	}
+}
+
+func NoSecio(cfg *Config) error {


I think it would be good to have a Secio or WithSecio one as well, such that you can make it explicit to always use it, independent of the defaults

dignifiedquire · 2017-12-15T13:43:37Z

libp2p.go

+	return nil
+}
+
+func WithMuxer(m mux.Transport) Option {


why are these called With<> and Transports not WithTransports?

Because we overload the term Transports. Transports also applies to things like tcp, quic, websockets, etc

ghost · 2017-12-15T16:28:28Z

You might like my ideas for this constructor API, but they're pretty different.
Generally I'd like to be closer js-libp2p, and "leak" fewer of swarm's/host's
details across the surface (in particular we should not hardcode secio in
the interface).

Most of the option functions can be simplified as loaders for certain kinds of
modules (as in libp2p modules): transport, muxer, encryption, private networking,
discovery. At a later point, when we have transport capabilities, many of them
can simply be collapsed into the transport module category.

Identity(privkey)
ListenAddrs(string...)
AnnounceAddrs(string...)
Transports(tpt.Transport...)
Muxer(msmux.Transport...)
Encryption(...) / NoEncryption()
PrivateNetworking(protector)
Discovery(disc.Discovery...) (the discovery interface is still todo in go-libp2p)
As for the naming pattern, e.g. WithFoo, FooOpt, etc., I don't have a strong opinion at the moment.

About the default config: why not make it a separate package? We can have
packages for various configurations, while go-libp2p itself depends "mostly"
on interfaces. (The muxers should keep being configurable right here.)

import (
    libp2p "github.com/libp2p/go-libp2p"
    cfg "github.com/libp2p/go-libp2p-default"
)

func main() {
  h, err := libp2p.New(context.TODO(), cfg.Config()...)
}

Also at a later point, I'd like to be able to construct a node simply by
passing a bunch of protocol identifiers, e.g.:

StrTransports("/ws/1.0.0", "/secio/1.0.0", "/mplex/6.3.0")
StrDiscovery("/mdns/1.0.0", "/udp/1.0.0")
// libp2p modules register themselves similar to how multiaddr protocols do it.

But that's really not neccessary at the moment.

whyrusleeping · 2017-12-16T00:50:32Z

@lgierth there, updated with some of your input

hsanjuan · 2017-12-16T00:58:06Z

Is this go-ish? I don't think I've seen these constructs in Go before. I find it innovative, kind of javascrip-ty.

It's fancy, I might like it, but I find the NewWithCfg initialization way more natural and straightforward in the end (and require less code). Perhaps I'm old fashioned... but let's keep that method exported in any case.

hsanjuan · 2017-12-16T00:58:50Z

oh, just saw the dave cheney post, I guess it's go-ish then :)

ghost

:):):)

whyrusleeping · 2018-01-07T20:41:55Z

alright, gonna merge this for now. The next step is to try out using it in go-ipfs to construct our libp2p node.

* bump go.mod to Go 1.17 and run go fix * update .github/workflows/automerge.yml * update .github/workflows/go-test.yml * update .github/workflows/go-check.yml * remove unneeded nil check in ECDSAPublicKey.Verify Co-authored-by: web3-bot <web3-bot@users.noreply.github.com> Co-authored-by: Marten Seemann <martenseemann@gmail.com>

ghost assigned whyrusleeping Nov 4, 2017

ghost added the status/in-progress In progress label Nov 4, 2017

Stebalien reviewed Nov 4, 2017

View reviewed changes

dignifiedquire reviewed Nov 4, 2017

View reviewed changes

hsanjuan requested changes Nov 7, 2017

View reviewed changes

daviddias mentioned this pull request Nov 10, 2017

Implement top-level Node or "Host" libp2p/rust-libp2p#13

Closed

dignifiedquire reviewed Dec 15, 2017

View reviewed changes

ghost self-requested a review December 17, 2017 21:51

ghost approved these changes Dec 17, 2017

View reviewed changes

whyrusleeping force-pushed the feat/builder branch from 0f5ab37 to dabae69 Compare January 7, 2018 19:03

whyrusleeping added 6 commits January 7, 2018 12:22

first hack at a nice libp2p constructor

f34163e

review feedback

c937b88

use new constructor for examples

d042680

functional parameters

8d84ecd

incorporate lgierths opinions

bbbc040

import go-smux-multiplex

5824305

whyrusleeping force-pushed the feat/builder branch from dabae69 to 5824305 Compare January 7, 2018 20:23

whyrusleeping merged commit 7db6e54 into master Jan 7, 2018

ghost removed the status/in-progress In progress label Jan 7, 2018

whyrusleeping deleted the feat/builder branch January 7, 2018 20:42

Stebalien mentioned this pull request May 11, 2021

Release v0.14.0 #1090

Closed

27 tasks

marten-seemann pushed a commit that referenced this pull request Apr 21, 2022

fix swarm transient conn (#241)

953af40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first hack at a nice libp2p constructor #241

first hack at a nice libp2p constructor #241

whyrusleeping commented Nov 4, 2017

Stebalien left a comment

Stebalien Nov 4, 2017

hsanjuan Nov 7, 2017

Stebalien Nov 7, 2017

Stebalien Nov 4, 2017

whyrusleeping Nov 5, 2017

dignifiedquire Nov 4, 2017

hsanjuan left a comment

hsanjuan Nov 7, 2017

whyrusleeping Nov 8, 2017

hsanjuan Nov 9, 2017

whyrusleeping commented Dec 14, 2017

whyrusleeping commented Dec 14, 2017

dignifiedquire commented Dec 14, 2017

whyrusleeping commented Dec 14, 2017

dignifiedquire commented Dec 14, 2017

whyrusleeping commented Dec 15, 2017

dignifiedquire Dec 15, 2017

whyrusleeping Dec 15, 2017

dignifiedquire Dec 15, 2017

dignifiedquire Dec 15, 2017

whyrusleeping Dec 15, 2017

ghost commented Dec 15, 2017

whyrusleeping commented Dec 16, 2017

hsanjuan commented Dec 16, 2017

hsanjuan commented Dec 16, 2017

ghost left a comment

whyrusleeping commented Jan 7, 2018


		type Option func(cfg *Config) error

		func Transports(tpts []transport.Transport) Option {

first hack at a nice libp2p constructor #241

first hack at a nice libp2p constructor #241

Conversation

whyrusleeping commented Nov 4, 2017

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

hsanjuan 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

whyrusleeping commented Dec 14, 2017

whyrusleeping commented Dec 14, 2017

dignifiedquire commented Dec 14, 2017

whyrusleeping commented Dec 14, 2017

dignifiedquire commented Dec 14, 2017

whyrusleeping commented Dec 15, 2017

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

ghost commented Dec 15, 2017

whyrusleeping commented Dec 16, 2017

hsanjuan commented Dec 16, 2017

hsanjuan commented Dec 16, 2017

ghost left a comment

Choose a reason for hiding this comment

whyrusleeping commented Jan 7, 2018