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

feat: ipns over dht #1725

Merged
merged 9 commits into from
Dec 6, 2018
Merged

feat: ipns over dht #1725

merged 9 commits into from
Dec 6, 2018

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Nov 20, 2018

Here is an initial implementation of IPNS over DHT. Until the dht is enabled by default in js-ipfs we need to use the --enable-dht-experiment experimental flag, or it will only use the local repo as datastore.

Needs:

Unblocks:

@ghost ghost assigned vasco-santos Nov 20, 2018
@ghost ghost added the status/in-progress In progress label Nov 20, 2018
@dirkmc
Copy link
Contributor

dirkmc commented Nov 20, 2018

@vasco-santos it's great to see you moving ahead with this work

It looks like the code only gets a single IPNS value from the DHT, whereas in the go version it retrieves multiple versions of the IPNS value (I believe the default is 16).

The go version provides a streaming API. Do you plan to support streaming in JS?

@vasco-santos
Copy link
Member Author

hey @dirkmc

So the stream was added recently to go-ipfs, in the latest [email protected]iirc!

We should add support for it in js-ipfs, as well, but I believe the main focus now should be to have this initial implementation as go had before and have interop between go and js. After that, I think it will be reasonable to add a stream API for this. We have been discussing adding stream's in the API for other things, and probably we should look into this too.

Just other detail, we currently do the dht.getMany with 16 occurences too, in the js-libp2p-kad-dht libp2p/js-libp2p-kad-dhtsrc/private.js#L271. However, we select the best record afterwards (using a select function provided), and return the "best" value.

@dirkmc
Copy link
Contributor

dirkmc commented Nov 22, 2018

@vasco-santos I agree about the streaming interface, it can wait until a later version. The reason I bring it up is because in the go version of IPNS, retrieving those 16 records takes so long that it almost always times out, so IPNS is painfully slow. I added an option to retrieve less than 16 records to mitigate this problem, but the real solution is streaming.

I think a lot of people were turned away from IPNS and even IPFS because of IPNS slowness so it would be great to get ahead of the issue as soon as possible

@vasco-santos vasco-santos force-pushed the feat/ipns-over-dht branch 4 times, most recently from 7f51f8b to a2ccc0a Compare November 29, 2018 22:54
@vasco-santos vasco-santos force-pushed the feat/ipns-over-dht branch 2 times, most recently from 84cee67 to c513b6e Compare December 1, 2018 14:54
@vasco-santos vasco-santos changed the title [WIP] feat: ipns over dht feat: ipns over dht Dec 1, 2018
@vasco-santos vasco-santos force-pushed the feat/ipns-over-dht branch 2 times, most recently from 95804d1 to 02071b1 Compare December 3, 2018 11:59
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

This looks good. Interop tests for ipns are also passing. Ci failures there are unrelated to the ipns tests.

src/core/ipns/publisher.js Outdated Show resolved Hide resolved
src/core/ipns/publisher.js Outdated Show resolved Hide resolved
src/core/ipns/publisher.js Outdated Show resolved Hide resolved
src/core/ipns/publisher.js Outdated Show resolved Hide resolved
@alanshaw alanshaw mentioned this pull request Dec 3, 2018
49 tasks
@vasco-santos vasco-santos force-pushed the feat/ipns-over-dht branch 2 times, most recently from 79ddf2a to cf0bbb7 Compare December 3, 2018 22:33
@alanshaw
Copy link
Member

alanshaw commented Dec 4, 2018

@vasco-santos would you mind rebasing this now that the IPNS over pubsub work is merged? 🙏

@vasco-santos vasco-santos force-pushed the feat/ipns-over-dht branch 11 times, most recently from 712e3cf to 6551fcc Compare December 4, 2018 21:14
src/core/ipns/publisher.js Outdated Show resolved Hide resolved
src/core/ipns/publisher.js Outdated Show resolved Hide resolved
src/core/ipns/publisher.js Outdated Show resolved Hide resolved
src/core/ipns/resolver.js Outdated Show resolved Hide resolved
src/core/ipns/resolver.js Outdated Show resolved Hide resolved
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.

Could you please also look at

(cb) => embedPublicKeyRecord ? this._publishPublicKey(keys.routingPubKey, publicKey, peerId, cb) : cb()

I think it is currently the wrong way round, but we should always publish it for the time being until there's no more go-ipfs nodes that require it.

src/core/ipns/publisher.js Outdated Show resolved Hide resolved
src/core/ipns/publisher.js Outdated Show resolved Hide resolved
src/core/ipns/publisher.js Outdated Show resolved Hide resolved
src/core/ipns/resolver.js Outdated Show resolved Hide resolved
@alanshaw alanshaw merged commit 1a943f8 into master Dec 6, 2018
@ghost ghost removed the status/in-progress In progress label Dec 6, 2018
@alanshaw alanshaw deleted the feat/ipns-over-dht branch December 6, 2018 15:23
@dirkmc
Copy link
Contributor

dirkmc commented Dec 6, 2018

W00t 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants