DHT API #11

vyzo · 2018-09-30T12:36:08Z

also, bootstrap to have a working DHT and LIST_PEERS to see our peers.

…strap

vyzo · 2018-10-04T07:58:28Z

Rebased on master.

raulk

Some initial feedback.

raulk · 2018-10-05T10:48:53Z

bootstrap.go

+	ma "github.com/multiformats/go-multiaddr"
+)
+
+var BootstrapPeers = []string{


What if the application wanted a DHT separate from ours? I'm sure that'll be the case of Ethereum. The Ethereum Foundation runs their own bootstrap nodes per network:

https://github.com/ethereum/go-ethereum/blob/461291882edce0ac4a28f64c4e8725b7f57cbeae/params/bootnodes.go

On a related note, what if we had more than one app behind this daemon, and each wanted a different DHT?

Yeah, we should be able to support different bootstrap peers.
I think what we want is a configuration file of sorts for the daemon profile, and there we can list bootstrap peers there.
But I left that for a subsequent pr (we can open follow up issue).

Personally I'd like to see an application "opening a session" with a daemon, and providing any initial seed configuration as part of that handshake.

I think of the daemon as a service provided by the environment/OS for applications to use, versus a sidecar for applications. As such, multi-tenancy and app-specific configuration should be part of the protocol.

For now, it's good that it's on our radar. An issue would be great, and perhaps a note in the specs.

i think we can borrow the Options style configuration from go-libp2p and have a default bootstrap configuration with our peers

Yeah, that's not a bad idea at all.
Upon further thought I'll add a -bootstrapPeers flag so that we can pass a comma separated list of peers in the command line, so that we can punt on the configuration.

Added a command-line flag in bd604c4

raulk · 2018-10-05T10:53:59Z

bootstrap.go

+		<-ticker.C
+
+		conns := d.host.Network().Conns()
+		if len(conns) >= BootstrapConnections {


This is a bit confusing to me, as it makes me think this goroutine is going to "pin/keep connections to bootstrap peers". But in practice we don't care if the active connections are to bootstrap peers or not.

If this is intended as a low watermark check for DHT, we should also verify that the returned conns support DHT?

it's a low watermark check mostly for the dht -- but you may want to keep bootstrap connections (say for relay, observed address identification, etc) without it.

perhaps we should earmark this as something that could/should be made into a connection manager?

either that or move it into the dht itself, as it seems useful

We could make a connection manager, but for now it's well isolated in its corner of the code :)

raulk · 2018-10-05T10:57:02Z

bootstrap.go

+	defer cancel()
+
+	for _, pi := range pis {
+		if d.host.Network().Connectedness(pi.ID) == inet.Connected {


I think this check is redundant, because Host#Connect() already performs it as part of its contract.

it's not -- we want to skip already connected peers otherwise we will connect to fewer peers than intended.

Ah, right, my bad!

raulk · 2018-10-05T10:58:42Z

bootstrap.go

+	return nil
+}
+
+func (d *Daemon) connectBootstrapPeers(pis []*pstore.PeerInfo, toconnect int) int {


Maybe validate that len(pis) >= toconnect? Just a sanity check.

Nah, if it is less we'll just connect to all of them.

Oh yeah, the loop is a range loop over peers 👍

raulk · 2018-10-05T11:34:12Z

pb/p2pd.proto

+  optional int64 timeout = 7;
+}
+
+message DHTResponse {


I think we need a token that identifies the DHT request uniquely. Then all streaming responses would refer to that token, and the END response would close the life of the request. Currently it seems that responses for parallel DHT requests could get intermixed on the wire.

I thought about it too and concluded that request multiplexing is more trouble than it's worth.
If we want parallel queries, we can open another connection and issue the query there.
unix sockets are relatively cheap, just a transient file descriptor.

I guess that makes sense, although it complicates the client because it'll either need to sync to allow only 1 concurrent request (bad), or it'll need to maintain a pool of client instances (too complicated), because we don't want each request to create a new client...

yeah, i think the client can handle this with multiple connections. we definitely don't want to reinvent stream multiplexing at another layer in the stack.

We can create a new socket for each request, that's simple enough.

bigs · 2018-10-05T14:25:04Z

bootstrap.go

+	ma "github.com/multiformats/go-multiaddr"
+)
+
+var BootstrapPeers = []string{


i think we can borrow the Options style configuration from go-libp2p and have a default bootstrap configuration with our peers

bigs · 2018-10-05T14:32:12Z

bootstrap.go

+		<-ticker.C
+
+		conns := d.host.Network().Conns()
+		if len(conns) >= BootstrapConnections {


perhaps we should earmark this as something that could/should be made into a connection manager?

bigs · 2018-10-05T14:32:33Z

bootstrap.go

+		<-ticker.C
+
+		conns := d.host.Network().Conns()
+		if len(conns) >= BootstrapConnections {


either that or move it into the dht itself, as it seems useful

bigs · 2018-10-05T14:37:10Z

pb/p2pd.proto

+  optional int64 timeout = 7;
+}
+
+message DHTResponse {


yeah, i think the client can handle this with multiple connections. we definitely don't want to reinvent stream multiplexing at another layer in the stack.

bigs · 2018-10-05T14:43:35Z

conn.go

+			}
+
+			if ch != nil {
+				for res := range ch {


do we want to block on this response? is there any sane way it could be async? i suppose that would require per client connection locking?

No easy way to make this async without reinventing multiplexing.

bigs · 2018-10-05T14:48:37Z

conn.go

@@ -189,6 +224,21 @@ func (d *Daemon) doStreamHandler(req *pb.Request) *pb.Response {
 	return okResponse()
 }

+func (d *Daemon) doListPeers(req *pb.Request) *pb.Response {


bigs · 2018-10-05T15:02:17Z

p2pd/main.go

 )

 func main() {
+	identify.ClientVersion = "p2pd"


hmm should we get a version string in there? something like fmt.Sprintf("/p2pd/%d", DaemonVersion)

Yeah, that's probably a good idea. But what is our current version?

bigs · 2018-10-05T15:35:26Z

agree w/ all your responses, just wanted to earmark some stuff for later perhaps

ghost assigned vyzo Sep 30, 2018

ghost added the in progress label Sep 30, 2018

vyzo requested a review from bigs September 30, 2018 12:36

vyzo mentioned this pull request Oct 2, 2018

DHT API vyzo/gerbil-libp2p#1

Merged

vyzo added 18 commits October 4, 2018 10:57

extend protobuf for dht api

5e720e5

gx import dht, update libp2p and simplify deps to just the essentials

b13ae19

DHT request handling scaffolding

b1258bc

refactor errorResponseString

14296fa

implement dht FindPeer

30110e2

implement dht FindPeersClosestToPeer

4647341

implement dht FindProviders

cd177ac

implement dht GetClosestPeers

900aa28

implement dht GetPublicKey

f5e74ac

implement dht GetValue SearchValue PutValue

533a8e7

implement dht Provide

ab07018

enable dht hook

29e500c

daemon bootstrap

812e5a1

add bootstrap and dht options to p2pd main program

f2d26a4

add LIST_PEERS to daemon api

37ac1a1

periodic bootstrap process to ensure some connections when using boot…

eea9c45

…strap

check connectedness to nodes in periodic bootstrap process

55e194d

identify the daemon as p2pd

e1a93a8

vyzo force-pushed the feat/dht branch from e08273d to e1a93a8 Compare October 4, 2018 07:58

raulk reviewed Oct 5, 2018

View reviewed changes

bigs approved these changes Oct 5, 2018

View reviewed changes

vyzo added 3 commits October 5, 2018 18:09

add bootstrapPeers command line flag

bd604c4

add empty file in test dir so that go install ./... doesn't complain

d6a1cea

version the client string

4d7e846

vyzo force-pushed the feat/dht branch from 79be031 to 4d7e846 Compare October 5, 2018 15:35

vyzo merged commit 2b89ad5 into master Oct 5, 2018

ghost removed the in progress label Oct 5, 2018

vyzo deleted the feat/dht branch October 5, 2018 15:37

raulk mentioned this pull request Oct 11, 2018

Eth2.0 Implementers Call 5 Agenda ethereum/eth2.0-pm#11

Closed

DHT API #11

DHT API #11

Conversation

vyzo commented Sep 30, 2018 • edited Loading

vyzo commented Oct 4, 2018

raulk left a comment

Choose a reason for hiding this comment

raulk Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo Oct 5, 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

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

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

bigs commented Oct 5, 2018

vyzo commented Sep 30, 2018 •

edited

Loading

raulk Oct 5, 2018 •

edited

Loading

vyzo Oct 5, 2018 •

edited

Loading