-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
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. |
6a29c16
to
df424e5
Compare
Updated deps including all the latest changes in Bitswap. DHT tests in JS land are fine. Still no love between JS and Go 😢 |
CI is still not happy as well
|
Is this PR still up to date? |
@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 |
Is there any prior docs I can read about the technical challenges involved in go <-> js DHT interop? |
Let's unblock this endeavour with the delegated routing as soon as 0.29.0 gets released. |
df424e5
to
5347ffd
Compare
b0acc71
to
c4cacf8
Compare
There was a problem hiding this 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`.** |
There was a problem hiding this comment.
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?
src/cli/commands/dht/find-peer.js
Outdated
const peers = await ipfs.dht.findPeer(peerID) | ||
const addresses = peers.multiaddrs.toArray().map((ma) => ma.toString()) | ||
|
||
print(addresses) |
There was a problem hiding this comment.
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
src/cli/commands/dht/provide.js
Outdated
}, | ||
|
||
handler ({ ipfs, key, resolve }) { | ||
// TODO add recursive option |
There was a problem hiding this comment.
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.
src/core/components/libp2p.js
Outdated
@@ -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)), |
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/http/api/resources/dht.js
Outdated
arg: Joi.string().required() | ||
}).unknown() | ||
}, | ||
handler: (request, reply) => { |
There was a problem hiding this comment.
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!
999ebf2
to
2b548be
Compare
9a4083e
to
c16bdc4
Compare
c16bdc4
to
2956637
Compare
0c73e99
to
147b100
Compare
147b100
to
02d1704
Compare
This PR is the second part of the DHT integration in js-ipfs
Remaining tasks
Main work tracker: libp2p/js-libp2p-kad-dht#1
Integrate libp2p-ipfs-browserwill only add DHT there after Relay is inNeeds the following PRs:
[email protected]
libp2p
new releaseBlocked PRs: