coreapi: DHT API by magik6k · Pull Request #4804 · ipfs/kubo

magik6k · 2018-03-10T18:35:28Z

Based on #4802

Replaces #4774

TODOs:

Merge coreapi: Split the interface into multiple files #4802
Tests
Review

magik6k · 2018-03-24T22:26:32Z

#4807 should get merged first

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

@magik6k is this ready for review?

magik6k · 2018-03-30T07:37:25Z

Rebased, should be ready for a review

Kubuxu · 2018-03-30T08:39:26Z

CI is red

magik6k · 2018-03-30T11:01:00Z

I think there is a random test fail, will try to fix it first

Stebalien

Any chance we could get the DHT commands converted over to use this API before merging? I understand if that would be too much too fast but this is a lot of unused code at the moment.

Stebalien · 2018-03-30T23:05:37Z

core/coreapi/dht.go

+	dht, ok := api.node.Routing.(*ipdht.IpfsDHT)
+	if !ok {
+		return nil, ErrNotDHT
+	}


Why not just call FindPeer directly on ipfs.node.Routing? The interface requires that method.

Stebalien · 2018-03-30T23:11:47Z

core/coreapi/dht.go

+			return
+		}
+
+		notif.PublishQueryEvent(ctx, &notif.QueryEvent{


Why not just call FindPeer, get the result, return it to the user, and be done with it? We're throwing away all the other events anyways.

Stebalien · 2018-03-30T23:13:49Z

core/coreapi/dht.go

+		return nil, err
+	}
+
+	dht, ok := api.node.Routing.(*ipdht.IpfsDHT)


Same here. We shouldn't be casting this. If we need some new functionality, we should add that to the interface.

Stebalien · 2018-03-30T23:18:47Z

core/coreapi/dht.go

+		defer close(events)
+		for p := range pchan {
+			np := p
+			notif.PublishQueryEvent(ctx, &notif.QueryEvent{


Again, maybe I'm missing something but this seems like pointless complexity. The caller of this method (FindProviders) can't even use these events because we're registering for (and consuming) them.

Stebalien · 2018-03-30T23:24:35Z

core/coreapi/dht.go

+		return errors.New("cannot provide in offline mode")
+	}
+
+	if len(api.node.PeerHost.Network().Conns()) == 0 {


Oof. This is going to hurt. It's fine to do this from the command as we amortize over the request and multiple arguments but listing all the connections just to see if we have some connections is going to be very expensive. Ideally, the DHT would detect this and return an error. Does it not do that?

This was it the dht command.

Looking at provide code in kad-dht, it invokes dht.GetClosestPeers, which I think will error in that case (correct me if I'm wrong) -> https://github.com/libp2p/go-libp2p-kad-dht/blob/2bab49ef85b2d8547e95ca2a699b5be562cc78b6/lookup.go#L60

Stebalien · 2018-03-30T23:25:50Z

core/coreapi/dht.go

+
+	//defer close(events)
+	if settings.Recursive {
+		err = provideKeysRec(ctx, api.node.Routing, api.node.DAG, []*cid.Cid{c})


I'm pretty sure this needs to be an offline DAG.

Stebalien · 2018-03-30T23:30:05Z

core/coreapi/coreapi.go

 }

+// Dht returns the DhtAPI interface implementation backed by the go-ipfs node
+func (api *CoreAPI) Dht() coreiface.DhtAPI {


High level comment: I'd rather not have a DHT API. The DHT is just a backend for other stuff (the routing interface). While I know we currently have a command, I'd prefer not to bake this into the CoreAPI. Is there a design discussion for the CoreAPI anywhere?

Is there a design discussion for the CoreAPI anywhere?

I'm trying to follow the js coreapi spec in most places - for DHT https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/DHT.md.

I did only include the basic functions which may be useful in some cases.

👍

I've submitted an issue to discuss this (https://github.com/ipfs/interface-ipfs-core/issues/249). We can probably move forward discuss this in parallel. It's not the end of the world, just a bit unfortunate.

Stebalien · 2018-03-30T23:31:43Z

core/coreapi/dht.go

+}
+
+func provideKeysRec(ctx context.Context, r routing.IpfsRouting, dserv ipld.DAGService, cids []*cid.Cid) error {
+	provided := cid.NewSet()


This is just a copy of the existing functionality but... it's going to hurt regardless. Could we get a TODO: Use a bloom filter?

ipfs/go-cid#57

Stebalien · 2018-03-30T23:32:14Z

core/coreapi/dht.go

+	for _, c := range cids {
+		kset := cid.NewSet()
+
+		err := dag.EnumerateChildrenAsync(ctx, dag.GetLinksDirect(dserv), c, kset.Visit)


Same, could we get a "TODO: provide and enumerate in parallel with a small buffer".

Will get that with #4333

Stebalien · 2018-03-30T23:33:20Z

core/coreapi/interface/dht.go

+)
+
+// DhtAPI specifies the interface to the DHT
+type DhtAPI interface {


I'd rather use the interface defined in https://github.com/libp2p/go-libp2p-routing/ (although we'd probably want to change it to return channels). Alternatively, we could do this for now with a big "we're going to change this" todo.

magik6k · 2018-03-31T00:36:59Z

Thanks for the review and pointers.

Any chance we could get the DHT commands converted over to use this API before merging

I guess it's probably better to get this done sooner than later. This is going to be at least a slightly breaking change. Currently when you run some dht command with --enc=json you get a really verbose output of everything the dht is doing. Imo it's weird to produce output that verbose in API, so I think it's fine to clean that up. Is there anything that depends on this output?

magik6k · 2018-08-15T17:37:04Z

(should be ready to merge, though we might want one more review)

used in ipfs/kubo#4804

Stebalien

Two nits but nothing that we can't fix later (well, except the With prefix but I don't feel strongly about that).

Stebalien · 2018-08-23T00:42:01Z

core/coreapi/dht.go

+	errCh := make(chan error)
+	go func() {
+		for _, c := range cids {
+			dserv := dag.NewDAGService(blockservice.New(bs, offline.Exchange(bs)))


Let's construct this once.

Stebalien · 2018-08-23T00:43:39Z

core/coreapi/interface/options/dht.go

+
+// WithRecursive is an option for Dht.Provide which specifies whether to provide
+// the given path recursively
+func (dhtOpts) WithRecursive(recursive bool) DhtProvideOption {


What's the rational behind the With prefix?

Options before refractor used it, this didn't get updated with rebase. Will fix

Stebalien · 2018-08-23T00:46:06Z

Hm. Actually, is there any way we could get some better test coverage or will that come when we switch commands over to using this interface?

magik6k · 2018-09-01T22:34:28Z

coverage++ (73% on coreapi/dht.go)

(Circle appears to be borked/annoyingly slow - #5389 may or may not be related)

Kubuxu

SGWM

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k requested a review from Kubuxu as a code owner March 10, 2018 18:35

ghost assigned magik6k Mar 10, 2018

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

magik6k force-pushed the feat/coreapi/dht branch from 6be1c4e to 1516f2e Compare March 10, 2018 18:59

magik6k added topic/core-api Topic core-api status/blocked Unable to be worked further until needs are met labels Mar 11, 2018

magik6k mentioned this pull request Mar 12, 2018

CoreAPI progress tracking issue #4498

Open

51 tasks

magik6k removed the status/blocked Unable to be worked further until needs are met label Mar 23, 2018

magik6k force-pushed the feat/coreapi/dht branch from 1516f2e to 1d5d05f Compare March 24, 2018 22:16

magik6k requested review from a user and Stebalien March 24, 2018 22:23

magik6k added the status/blocked Unable to be worked further until needs are met label Mar 24, 2018

This was referenced Mar 24, 2018

coreapi: Swarm API #4803

Merged

coreapi: PubSub API #4805

Merged

magik6k force-pushed the feat/coreapi/dht branch from 1d5d05f to d4b6cee Compare March 30, 2018 07:26

magik6k removed the status/blocked Unable to be worked further until needs are met label Mar 30, 2018

magik6k force-pushed the feat/coreapi/dht branch from 8aaf2e1 to 5f75628 Compare March 30, 2018 09:07

magik6k added the status/blocked Unable to be worked further until needs are met label Mar 30, 2018

magik6k force-pushed the feat/coreapi/dht branch from ae14365 to 77f6604 Compare March 30, 2018 11:16

Stebalien requested changes Mar 30, 2018

View reviewed changes

magik6k force-pushed the feat/coreapi/dht branch 3 times, most recently from 36dec57 to 6665373 Compare March 31, 2018 00:29

magik6k force-pushed the feat/coreapi/dht branch from 6665373 to e326b7f Compare March 31, 2018 00:42

Stebalien force-pushed the feat/coreapi/dht branch from 49033ec to c0e2f54 Compare August 15, 2018 17:15

ghost assigned Stebalien Aug 15, 2018

kevina pushed a commit to ipfs/go-cidutil that referenced this pull request Aug 16, 2018

add a streaming CID set

4de263a

used in ipfs/kubo#4804

Stebalien force-pushed the feat/coreapi/dht branch from 5b87efc to c7ad317 Compare August 22, 2018 02:24

Stebalien approved these changes Aug 23, 2018

View reviewed changes

magik6k mentioned this pull request Aug 28, 2018

add go-dev meeting, 2018-08-27 ipfs/team-mgmt#687

Merged

magik6k force-pushed the feat/coreapi/dht branch 2 times, most recently from 15fcac3 to c1c8b4d Compare September 1, 2018 21:00

Kubuxu approved these changes Sep 7, 2018

View reviewed changes

magik6k and others added 13 commits September 10, 2018 17:33

coreapi: dht interface

a63e224

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: implement dht api

3616854

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: test using mock swarm

d2e6a61

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: dht tests

6cc86c1

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: update dht imports after rebase

bda76a1

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: dht: simplify the implementation

906c747

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: dht: refactor options after rebase

77ceb75

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi dht: add a note on name change

9fe843f

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

move streaming set to thirdparty

e1bdf6c

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: dht: use shared set in provideKeysRec

4b252a2

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: dht: make tests less falky

db646d0

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

switch to StreamingSet in cidutil

7139b83

License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>

coreapi: dht: remove option prefix

86f9eb7

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

Stebalien force-pushed the feat/coreapi/dht branch from c1c8b4d to 86f9eb7 Compare September 11, 2018 00:40

Stebalien added the RFM label Sep 11, 2018

Stebalien merged commit eb45644 into master Sep 11, 2018

ghost removed the status/in-progress In progress label Sep 11, 2018

Stebalien deleted the feat/coreapi/dht branch February 28, 2019 22:47

Uh oh!

Conversation

magik6k commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magik6k commented Mar 24, 2018

Uh oh!

Stebalien commented Mar 30, 2018

Uh oh!

magik6k commented Mar 30, 2018

Uh oh!

Kubuxu commented Mar 30, 2018

Uh oh!

magik6k commented Mar 30, 2018

Uh oh!

Stebalien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magik6k Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magik6k Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magik6k commented Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magik6k commented Aug 15, 2018

Uh oh!

Stebalien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stebalien commented Aug 23, 2018

Uh oh!

magik6k commented Sep 1, 2018

Uh oh!

Kubuxu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

magik6k commented Mar 10, 2018 •

edited

Loading

magik6k Mar 31, 2018 •

edited

Loading

magik6k Jul 19, 2018 •

edited

Loading

magik6k commented Mar 31, 2018 •

edited

Loading