Add in basic libp2p host, and commands for establishing connectivity #34

whyrusleeping · 2018-01-31T07:16:48Z

Testing still TBD

dignifiedquire · 2018-01-31T10:28:08Z

commands/daemon.go

@@ -19,6 +21,9 @@ var daemonCmd = &cmds.Command{
 	Helptext: cmdkit.HelpText{
 		Tagline: "start the filecoin daemon",
 	},
+	Options: []cmdkit.Option{
+		cmdkit.StringOption("swarmlisten").WithDefault("/ip4/127.0.0.1/tcp/6000"),


as noted in the other PR, lets make the option name a constant

Also I wonder if swarmaddr or swarmlistenaddr wouldn't be more clear of what this option does.

Yeah, I don't even think we will want this option once we have the config file working, What do you think?

Yeah, that's true. Mark it with a todo to move to the config file, and I am happy.

Its already marked as a todo, look a few lines below where we pull the option out.

dignifiedquire · 2018-01-31T10:31:40Z

commands/id.go

+				output = strings.Replace(output, "<addrs>", strings.Join(val.Addresses, "\n"), -1)
+				output = strings.Replace(output, "\\n", "\n", -1)
+				output = strings.Replace(output, "\\t", "\t", -1)
+				_, err := fmt.Fprint(w, output)


Lets put the formatting above into its own function, that can be easily tested

Its copy-pasted from go-ipfs. I'm thinking that we can do some sort of extraction of common commands at some point.

Still want this in its own function?

I suspect that the id command will evolve in Filecoin to a degree that will not be a 1-1 match with IPFS in the long run, so pulling that into a shared space might not be worth it.

phritz

OK the main thing I want to ask is whether we are shooting for a world with clean separation between how triggers enter the system (via command, via peer network) and the things they use to get their work done. It would be great if there were loose coupling. That is, if there were a set of abstractions -- call them model objects though that's not quite right -- that manage and manipulate all our state (chain, orderbook, storage, etc). The model objects do work and are separate from the network/command endpoints that receive triggers and encode responses. The models don't really know about the network or commands, the network/command endpoints hide the specifics of what's happening from the models. This as opposed to close coupling where for example the thing that stores our state also embeds the network and knows a lot about it. One advantage of the loose coupling is that we have a seam that we can use to test. For example we were kicking around the idea of presenting a uniform RPC interface to the node. Peer network endpoints and command handlers pass messages in as RPC requests and messages out are just RPC responses (both structured datatypes). The thing receiving the RPC would have no idea about whether it's doing the work for a peer node, a user command, or an automated test; it just knows how to manipulate models and stuff RPC responses full and return them to the caller. Having this seam also makes it easier uniformly enforce access control, rate limiting, to do logging and tracing, etc. It obviously donest have to use RPC as a model, that's just an example, it could be a straight set of Go interfaces. Does that make any sense?

ps You seem to have misplaced your tests.

pps This is presumably in service of #19?

EDIT: spoke with why in person about this. Answer is basically yes, we're shooting for a reasonably clean separation.

phritz · 2018-02-01T05:42:42Z

node/node.go

 	"fmt"
+
+	"gx/ipfs/QmT68EgyFbnSs5rbHkNkFZQwjdHfqrJiGr3R6rwcwnzYLc/go-libp2p"
+	"gx/ipfs/QmfCtHMCd9xFvehvHeVxtKVXJTMVTuHhyPRVHEXetn87vL/go-libp2p-host"
 )

 // Node represents a full Filecoin node.


Could you add some more comments about what a node is an its responsibilities? Same with api server? For example it looks like a node is the interface to the rest of the filecoin network and the api server is strictly user-command-facing. Presumably all filecoin network traffic comes through the node. Does the node also contain all the blockchain and associated data structures or do they live elsewhere?

phritz · 2018-02-01T05:43:56Z

node/node.go

+// Build instantiates a filecoin Node from the settings specified in the
+// config.
+func (nc *NodeConfig) Build(ctx context.Context) (*Node, error) {
+	host, err := libp2p.New(ctx, nc.Libp2pOpts...)


Can you instantiate a node if you dont want to enable networking? I guess it only matters if the node is the thingy containing all the important data structures (maybe it is?), but still curious.

Yes. We should be able to instantiate a node without networking.

phritz · 2018-02-01T05:45:26Z

commands/id.go

+	cmdkit "gx/ipfs/QmceUdzxkimdYsgtX733uNgzf1DLHyBKN6ehGSp85ayppM/go-ipfs-cmdkit"
+)
+
+type idOutput struct {


can you comment what this is?

phritz · 2018-02-01T05:46:19Z

commands/id.go

+	PublicKey       string
+}
+
+var IdCmd = &cmds.Command{


Does embedding implementations in these structures make them hard to test?

possibly, In go-ipfs land we've been moving towards pulling everything out into a separate "core api" thingy. Can start doing that here too.

phritz · 2018-02-01T20:05:32Z

commands/daemon.go

+	}()
+
+	// TODO: this should be passed in from a config file, not an api flag
+	libp2pOpts := node.Libp2pOptions(


I dunno if it's true that this should come in via config file. We talked about having a signal to reload the server config, presumably it needs to reset everything that is in the config file. Networking seems like something that might be a PITA to re-initialize while the node is running. Command line flags are fine for stuff we don't want to change while the server is running. Config file (in the model we talked about) is for stuff we do want to change while the server is running, for example what asks to place.

cc @dignifiedquire

I think we have some different thoughts on what a configuration file should be... The networking configuration will definitely be in the config file. and "what bids and asks to place" does not feel like anything that belongs in a config file. Thats process state, not process configuration.

I will file a config file issue actually... #45

phritz · 2018-02-01T20:07:02Z

commands/daemon.go

+	}
+
+	cfg := cmdhttp.NewServerConfig()
+	cfg.APIPath = "/api"


Should pull "/api" out into a constant, it's used two places. Also I think I suggested scoping it to /cmdapi or similar to not preclude some other api.

I'll pull it out, going to leave it as /api for consistency though.

phritz · 2018-02-01T20:07:59Z

commands/id.go

+)
+
+// idOutput is the output of the /id api endpoint
+type idOutput struct {


As for naming, is the pattern generally to call things FooInput and FooOutput when it comes to the api? I like the consistency.

Yeah, we tend to do that. Sometimes multiple commands can share an output type so it can get weird.

That said, there is something to having them be nicer user facing types that get returned by an api client. I'm interested in your thoughts on the current coreapi approach in go-ipfs land:

https://github.com/ipfs/go-ipfs/tree/master/core/coreapi

coreapi: Pin API ipfs/kubo#4575

coreapi: Block API ipfs/kubo#4548

For consistency let's use FooInput and FooOutput. It's easy enough to declare that type BarInput is FooInput and then you can always separate them if need be.

I'll take a note to look at the apis, likely not before vacation though.

phritz · 2018-02-01T20:13:05Z

commands/env.go

+}
+
+// GetNode returns the Filecoin node of the environment.
+func GetNode(env cmds.Environment) *node.Node {


Is there a reason to have what are essentially global variables like this? (As in, is there a reason we couldn't have something that does the command work that has a handle on the node?)

Or just because it's easy?
edit: added ?

What do you mean?

is there a reason we couldn't have something that does the command work that has a handle on the node?

Thats essentially what this is. We get a handle to the node this way for the commands work to operate on

I meant why not have a thingy that implements commands and is instantiated with a Node. That way a command thingy has-a node, instead of all commands necessarily sharing the same node. The node is a global variable with all the normal baggage. For example you can't have two command thingies that use different nodes in a test. Everyone gets the same node. Globals often give rise to concurrency or deadlock issues because of unclear access patterns, globals tend to accumulate more globals, etc. Guess I'm asking whether there is a reason for having a global variable other than that it's an easy thing to do...

Nevermind w/the globals question ^, I got it. The context is provided to the command handler.

phritz · 2018-02-01T20:17:16Z

utils/lock.go

+}
+
+// Lock creates a lockfile if necessary under filename.
+func Lock(filename string) error {


You should say somewhere that this lock is re-entrant. And I don't think it should be: if you have a binary that is locking the same locking file multiple times to ensure that it is running once that likely means an error -- that the binary is being initialized more than once.

Also from @dignifiedquire's PR. I think he has since removed the locking (for now). I'll rebase in a bit.

phritz · 2018-02-01T20:18:35Z

utils/lock_test.go

+	err = utils.Lock(lockFile)
+	Expect(err).To(BeNil())
+
+	locked, err = utils.IsLocked(lockFile)


Should also test the intended behavior if the file is already locked (and you call Lock on it again).

This is from @dignifiedquire's PR, i'm just building on top of it to avoid reimplementing.

phritz

Are unit tests forthcoming? There are something like 6 new go files with ~700 new lines of code and no unit tests for them.

phritz · 2018-02-02T01:42:10Z

commands/main.go

+var rootSubcmds = map[string]*cmds.Command{
+	"daemon":  daemonCmd,
+	"version": versionCmd,
+	"swarm":   SwarmCmd,


Sorry I missed this first time around: if the command doesnt require the daemon to be running how can i list a node's peers this way? Or are you thinking that "list a node's peers" comes later or we list the peers of an ephemeral invocation of the command (rather than what the daemon sees)?

I think in the case of filecoin, we should basically require the daemon to be running for all commands. With ipfs its nice to be able to run things locally without the daemon, but here it just adds extra complexity.

I think I can get behind this, given the following setup

all CLI commands are eventually modeled as RPC commands (in whatever RPC definition/framework we decide on)

there is a separate configuration that controls enable/disable network connections

Currently ipfs-commands seems to mix the logic of client and server in the same packages/files. I wonder if we could change this such that the cli client is its own small package, which effectively only wraps RPC calls.

I am not sure how to best approach this right now though. @whyrusleeping do you have an idea on how to do this, given you have been using the commands library for some time now and have a better understanding of it.

dignifiedquire · 2018-02-02T21:22:28Z

@whyrusleeping can you rebase on master, now that the previous branch was merged, currently hard to see the changes

whyrusleeping · 2018-02-02T21:44:49Z

cmd/daemon.go

@@ -1,14 +1,16 @@
-package cmd
+package commands


I was moving things from cmd to commands. (cmd should be reserved for binaries, commands is a better place for all the commands) but git made that really ugly and hard to read the diff on.

@whyrusleeping I pushed the rename to master, so the git history is clean

whyrusleeping · 2018-02-02T21:49:16Z

cmd/id.go

+	},
+	Type: idOutput{},
+	Encoders: cmds.EncoderMap{
+		cmds.Text: cmds.MakeTypedEncoder(func(req *cmds.Request, w io.Writer, val *idOutput) error {


In this weeks episode of "making it easier to write commands" we have "REFLECTION!"

whyrusleeping · 2018-02-02T21:50:29Z

cmd/main.go

@@ -26,31 +26,36 @@ var (
 	ErrMissingDaemon = errors.New("daemon must be started before using this command")
 )

+func defaultApiAddr() string {
+	// Until we have a config file, we need an easy way to influence the API
+	// address for testing


injecting "--cmdapiaddr=foobar" into every command invocation gets annoying.

dignifiedquire · 2018-02-02T21:55:04Z

cmd/env.go


 import (
 	"context"

-	cmds "gx/ipfs/Qmc5paX4ECBARnAKkcAmUYHBGor228Tkfxeya3Nu2KRL46/go-ipfs-cmds"
+	cmds "gx/ipfs/QmWGgKRz5S24SqaAapF5PPCfYfLT7MexJZewN5M82CQTzs/go-ipfs-cmds"


@whyrusleeping should we gx-go uw these before commiting like we do for go-ipfs?

But we don't do that for go-ipfs

hmm, okay I am confused, so it is not done for go-ipfs, but for libp2p (e.g. https://github.com/libp2p/go-libp2p-secio/blob/master/protocol.go) and ipfs packages (e.g. https://github.com/ipfs/go-ipfs-cmds/blob/master/command.go) could you explain why?

phritz · 2018-02-05T01:36:18Z

cmd/id.go

+)
+
+// idOutput is the output of the /id api endpoint
+type idOutput struct {


Mmmm, it's dumping a bunch of stuff that's not used. Strategy-wise we'd talked about only adding that which is actually used so as to move fast and avoid clutter and to make it clear what is used and what isn't (the policy being if it is implemented it actually does something, even if that thing is not fully functional). For example the person who implements the public key functionality can add it here and wherever else it should go. From the perspective of our strategy, it's just clutter that we now have to talk about. I guess don't rip it out but let's talk about the strategy going forward if it's not one you'd like to embrace (or maybe we didn't talk about it enough?).

phritz · 2018-02-05T01:43:36Z

cmd/id.go

@@ -0,0 +1,78 @@
+package commands


Should probably have a simple unit test for this

phritz · 2018-02-05T01:43:53Z

cmd/main.go

 	"daemon":  daemonCmd,
 	"version": versionCmd,
+	"swarm":   SwarmCmd,


we should decide if the commands are either exported or not, but be consistent about it.

phritz · 2018-02-05T01:45:19Z

cmd/swarm.go

@@ -0,0 +1,269 @@
+package commands


Do we have any tests for this code?

dignifiedquire · 2018-02-05T10:59:43Z

cmd/daemon.go

-	node := node.New()
-	if err := startNode(node, api); err != nil {
+	// TODO: this should be passed in from a config file, not an api flag
+	libp2pOpts := node.Libp2pOptions(


I would rather see the creation of something as specific as libp2p options be encapsulated inside the node, rather than leaking that outside unless we have to.

whyrusleeping · 2018-02-05T17:52:40Z

commands/id.go

@@ -40,7 +40,9 @@ var idCmd = &cmds.Command{

 		out := idOutputFromNode(fcn)

-		re.Emit(out)
+		if err := re.Emit(out); err != nil {


no. This is wrong.

Why/could you explain what the right way is to handle the error?

dignifiedquire suggested changes Jan 31, 2018

View reviewed changes

phritz reviewed Feb 1, 2018

View reviewed changes

dignifiedquire force-pushed the daemon branch 3 times, most recently from f0d524c to 9aaa818 Compare February 1, 2018 11:01

phritz reviewed Feb 1, 2018

View reviewed changes

phritz reviewed Feb 2, 2018

View reviewed changes

whyrusleeping force-pushed the feat/networking branch from 00769d8 to 7947a5e Compare February 2, 2018 21:35

whyrusleeping changed the base branch from daemon to master February 2, 2018 21:37

whyrusleeping force-pushed the feat/networking branch from 7947a5e to 944a094 Compare February 2, 2018 21:43

whyrusleeping commented Feb 2, 2018

View reviewed changes

dignifiedquire reviewed Feb 2, 2018

View reviewed changes

phritz suggested changes Feb 5, 2018

View reviewed changes

dignifiedquire reviewed Feb 5, 2018

View reviewed changes

whyrusleeping added 2 commits February 5, 2018 09:27

Add in basic libp2p host, and commands for establishing connectivity

8cf6730

add test for connecting two nodes

6d200e4

whyrusleeping force-pushed the feat/networking branch 3 times, most recently from 4305ab2 to 481edd8 Compare February 5, 2018 17:41

whyrusleeping commented Feb 5, 2018

View reviewed changes

linting

67c5228

whyrusleeping force-pushed the feat/networking branch from 7ea14d4 to 67c5228 Compare February 5, 2018 17:54

whyrusleeping merged commit 08bfead into master Feb 5, 2018

Add in basic libp2p host, and commands for establishing connectivity #34

Add in basic libp2p host, and commands for establishing connectivity #34

Conversation

whyrusleeping commented Jan 31, 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

Choose a reason for hiding this comment

phritz left a comment • edited Loading

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

phritz Feb 1, 2018 • edited Loading

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

phritz Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phritz Feb 2, 2018 • edited Loading

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

phritz Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phritz 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

dignifiedquire commented Feb 2, 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

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

phritz left a comment •

edited

Loading

phritz Feb 1, 2018 •

edited

Loading

phritz Feb 1, 2018 •

edited

Loading

phritz Feb 2, 2018 •

edited

Loading

phritz Feb 1, 2018 •

edited

Loading