Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Awesome Endeavour: DHT Part II #856

Merged
merged 11 commits into from
Feb 8, 2019
Merged

Awesome Endeavour: DHT Part II #856

merged 11 commits into from
Feb 8, 2019

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented May 19, 2017

This PR is the second part of the DHT integration in js-ipfs

Remaining tasks

Main work tracker: libp2p/js-libp2p-kad-dht#1

Needs the following PRs:

Blocked PRs:

@daviddias
Copy link
Member Author

daviddias commented Jul 20, 2017

@daviddias
Copy link
Member Author

It might actually be good to enable the DHT if it proves to not break things and then figure out the interop of the DHT with go-ipfs as we go.

@daviddias
Copy link
Member Author

Updated deps including all the latest changes in Bitswap. DHT tests in JS land are fine. Still no love between JS and Go 😢

@daviddias
Copy link
Member Author

CI is still not happy as well


  1) interface-ipfs-core tests .dht callback API .findpeer finds other peers:
     Uncaught AssertionError: expected [Error] to not exist
      at nodeA.dht.findpeer (node_modules/interface-ipfs-core/src/dht.js:90:32)
      at self._libp2pNode.peerRouting.findPeer (src/core/components/dht.js:81:18)
      at node_modules/async/internal/once.js:12:16
      at next (node_modules/async/waterfall.js:21:29)
      at node_modules/async/internal/onlyOnce.js:12:16
      at waterfall (node_modules/libp2p-kad-dht/src/index.js:449:20)
      at nextTask (node_modules/async/waterfall.js:16:14)
      at next (node_modules/async/waterfall.js:23:9)
      at node_modules/async/internal/onlyOnce.js:12:16
      at setImmediate (node_modules/multihashing-async/src/utils.js:8:7)
      at Immediate.<anonymous> (node_modules/async/internal/setImmediate.js:27:16)

  2) interface-ipfs-core tests .dht callback API .query returns the other node in the query:
     Uncaught AssertionError: expected [] to include 'QmS72d5n39h2zWXHQFFQs2Xub7GADbYimayvJYkocJv98D'
      at nodeA.dht.query (node_modules/interface-ipfs-core/src/dht.js:139:47)
      at self._libp2pNode._dht.getClosestPeers (src/core/components/dht.js:157:9)
      at q.run (node_modules/libp2p-kad-dht/src/index.js:172:18)
      at Query.run (node_modules/libp2p-kad-dht/src/query.js:47:14)
      at utils.convertBuffer (node_modules/libp2p-kad-dht/src/index.js:166:9)
      at setImmediate (node_modules/multihashing-async/src/utils.js:8:7)
      at Immediate.<anonymous> (node_modules/async/internal/setImmediate.js:27:16)

@daviddias daviddias added P0 Critical: Tackled by core team ASAP status/ready Ready to be worked and removed status/ready Ready to be worked labels Oct 17, 2017
@hoffmabc
Copy link

Is this PR still up to date?

@daviddias
Copy link
Member Author

@hoffmabc it is as with regards to status, DHT is still a WIP. Lot's of work happening on js-libp2p land itself -- https://github.com/libp2p/js-libp2p --. Currently focused on libp2p/js-libp2p#159

@dryajov dryajov mentioned this pull request Mar 26, 2018
@alanshaw
Copy link
Member

Is there any prior docs I can read about the technical challenges involved in go <-> js DHT interop?

@daviddias
Copy link
Member Author

Let's unblock this endeavour with the delegated routing as soon as 0.29.0 gets released.

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Additionally the CLI command handlers may soon need to be updated to use getIpfs instead of ipfs. See #1860 for details.

@@ -319,7 +314,6 @@ Enable and configure experimental features.
- `pubsub` (boolean): Enable libp2p pub-sub. (Default: `false`)
- `ipnsPubsub` (boolean): Enable pub-sub on IPNS. (Default: `false`)
- `sharding` (boolean): Enable directory sharding. Directories that have many child objects will be represented by multiple DAG nodes instead of just one. It can improve lookup performance when a directory has several thousand files or more. (Default: `false`)
- `dht` (boolean): Enable KadDHT. **This is currently not interoperable with `go-ipfs`.**
Copy link
Member

Choose a reason for hiding this comment

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

OK, so, I'm half inclined to just enable it in the browser by default, expose ourselves to the issues and get them fixed asap. That said, since this is such a big feature perhaps not having it enabled by default in the browser initially will give us some scope to test it out first and fix the bigger issues, so that we don't expose all our users to the bugs and make everyone mad.

We do need a way to enable/disable it though - if the dht config option is being removed from EXPERIMENTAL, is it being added to the top level?

const peers = await ipfs.dht.findPeer(peerID)
const addresses = peers.multiaddrs.toArray().map((ma) => ma.toString())

print(addresses)
Copy link
Member

Choose a reason for hiding this comment

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

print is just console.log which will give you output like:

[ '/ip4/127.0.0.1/tcp/4001',
  '/ip4/169.254.83.88/tcp/4001',
  '/ip4/192.168.1.95/tcp/4001' ]

In comparison go-ipfs outputs:

$ ipfs dht findpeer QmZMxNdpMkewiVZLMRxaNxUeZpDUb34pWjZ1kZvsd16Zic
/ip6/2001:19f0:5:66d1::/tcp/4001
/ip4/149.28.228.162/tcp/33154
/ip4/45.77.150.82/tcp/4001
/ip6/::1/tcp/4001
/ip6/2001:19f0:5:5d40:5400:1ff:fec1:ce94/tcp/4001
/ip4/127.0.0.1/tcp/4001
/ip4/149.28.228.162/tcp/4001
/ip4/127.0.0.1/tcp/8081/ws

},

handler ({ ipfs, key, resolve }) {
// TODO add recursive option
Copy link
Member

Choose a reason for hiding this comment

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

Please pass this option to ipfs.dht.provide so it can throw and inform the user the option is not implemented yet.

@@ -76,6 +76,9 @@ function defaultBundle ({ datastore, peerInfo, peerBook, options, config }) {
}
},
dht: {
kBucketSize: get(options, 'dht.kBucketSize', 20),
enabled: get(options, 'dht.enabled', true) && !(get(options, 'local', false)),
Copy link
Member

Choose a reason for hiding this comment

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

"local" is now "offline"?

const some = require('async/some')
const eachOf = require('async/eachOf')
const someSeries = require('async/someSeries')
const eachOfSeries = require('async/eachOfSeries')
Copy link
Member

Choose a reason for hiding this comment

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

What does this change do?

Copy link
Member

Choose a reason for hiding this comment

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

Using some is making a lot of parallel dht find and connect requests.

So, everything will get queried even if we find the target node, as the find takes longer than initiating the requests.

In the long run, we should refactor the pin-set, in order to try to parallelize some operations, but this is the safest option currently.

arg: Joi.string().required()
}).unknown()
},
handler: (request, reply) => {
Copy link
Member

Choose a reason for hiding this comment

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

These handlers now need to change for Hapi 18 - please shout if you have any questions!

@vasco-santos vasco-santos force-pushed the feat/dht-part-ii branch 2 times, most recently from 999ebf2 to 2b548be Compare February 6, 2019 11:31
@alanshaw alanshaw mentioned this pull request Feb 6, 2019
@vasco-santos vasco-santos force-pushed the feat/dht-part-ii branch 2 times, most recently from 9a4083e to c16bdc4 Compare February 6, 2019 15:32
@ghost ghost assigned alanshaw Feb 7, 2019
@vasco-santos vasco-santos force-pushed the feat/dht-part-ii branch 4 times, most recently from 0c73e99 to 147b100 Compare February 7, 2019 22:32
@vasco-santos vasco-santos mentioned this pull request Feb 8, 2019
4 tasks
@alanshaw alanshaw merged commit 77a0957 into master Feb 8, 2019
@alanshaw alanshaw deleted the feat/dht-part-ii branch February 8, 2019 11:01
@ghost ghost removed the status/in-progress In progress label Feb 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awesome endeavour exp/wizard Extensive knowledge (implications, ramifications) required P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants