-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
792473e
to
97f10fd
Compare
4b43129
to
835f166
Compare
You'll need to start the daemon in offline mode for that to work (pass |
The repos are currently not compatible. Go supports complex datastore configs and, as far as I know, javascript doesn't (yet). |
How are you storing the IPNS record currently? go-ipfs does will store the IPNS record in a libp2p "record" (https://github.com/libp2p/go-libp2p-record/blob/master/pb/record.proto) under |
Yes, I already addressed that problem in ipfs/interop#29 and I am waiting for feedback.
Regarding this, I tried to reach you yesterday for discussing this. I have been debugging the go implementation and I think that I understood the problem. Basically, as go-ipfs have a local + dht implementation, after publishing a record, you will have 3 distinct entries in your datastore. For instance, consider the following ones:
As I am doing the IPNS local implementation first, I only have the last one on js repo. Accordingly, when I run the following command in go:
I expect that it only cares about the last entry and it does not happen in fact. At first, it tries to get the PublicKey here (https://github.com/ipfs/go-ipfs/blob/master/namesys/routing.go#L76), which fails. I commented that block, since it only checks if the public key exists, and proceeded, in order to understand if it was the only problem. However, I got another problem in here (https://github.com/ipfs/go-ipfs/blob/master/namesys/routing.go#L86). Basically, both problems try to get from the datastore what was stored in the routing context: https://github.com/ipfs/go-ipfs/blob/master/namesys/publisher.go#L225 As it was the routing context, I didn’t implement that function, but it will be quick to add it if it is supposed. This way, I only want to ask you if this is in fact the correct approach, because if we run locally, we will immediately have the public key and the local record. Am I right, or missing something? Moreover, I found another interop problem that I will work on today. I only noticed during this debug that you also publish an IPNS record during the |
That last one isn't actually a part of the "routing" system, we put raw IPNS records there for republishing (both to remember the last-used sequence number and to remember which records we need to periodically republish). The first record in that list is the IPNS record as stored in the routing system. That's the one we care about here. The second record is the public key record. Now that we (a) embed the public key in the IPNS record and (b) don't sign the outer "routing" record, that's less important. |
See the comment above that line. We need the public key to validate the record. Note: once everyone starts just embedding the public key in their IPNS record, we won't need that. Really, we might just want to not fail if that lookup fails and try getting the IPNS record anyways. |
We publish the empty directory but that's not really part of the IPNS spec. That's just something that go-ipfs does. We probably do this because we support mounting user's IPNS entry using fuse (under
You'll have to make sure to republish IPNS entries once every ~4hr. |
ade1db4
to
4483871
Compare
PR was refactored, and the interop was also achieved!! 🙌 The first message of the PR was updated with the list of PRs needed for merging this one. |
4483871
to
47d521c
Compare
A working version of **IPNS working locally** is here! 🚀 😎 💪 Steps: - [x] Create a new repo (**js-ipns**) like it was made with go in the last week ([go-ipns](https://github.com/ipfs/go-ipns)) and port the related code from this PR to there - [x] Resolve IPNS names in publish, in order to verify if they exist (as it is being done for regular files) before being published - [x] Handle remaining parameters in publish and resolve (except ttl). - [x] Test interface core spec [interface-ipfs-core#327](ipfs-inactive/interface-js-ipfs-core#327) - [x] Test interoperability with go. [interop#26](ipfs/interop#26) - [x] Integrate logging - [x] Write unit tests - [x] Add support for the lifetime with nanoseconds precision - [x] Add Cache - [x] Add initializeKeyspace - [x] Republish Some notes, regarding the previous steps: - There is an optional parameter not implemented in this PR, which is `ttl`, since it is still experimental, we can add it in a separate PR. Finally, thanks @Stebalien for your help and time answering all my questions regarding the IPNS implementation in GO. Moreover, since there are no specs, and not that much documentation, I have been writing a document with all the IPNS workflow. It is a WIP by now, but it is currently available [here](ipfs/specs#184). Related PRs: - [x] [js-ipns#4](ipfs/js-ipns#4) - [x] [js-ipfs-repo#173](ipfs/js-ipfs-repo#173) - [x] [js-ipfs#1496](#1496) - [x] [interface-ipfs-core#327](ipfs-inactive/interface-js-ipfs-core#327) - [x] enable `interface-ipfs-core` tests for IPNS in `js-ipfs`
47d521c
to
8b5d67e
Compare
8b5d67e
to
248c4a2
Compare
45b91f8
to
962ee23
Compare
package.json
Outdated
@@ -51,7 +51,7 @@ | |||
"form-data": "^2.3.2", | |||
"go-ipfs-dep": "~0.4.17", | |||
"hat": "0.0.3", | |||
"ipfs": "~0.31.6", | |||
"ipfs": "github:ipfs/js-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.
This has been released, you can :D
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.
Yeah! Just found a problem blocking the interop to work 😞
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.
@vasco-santos ipfs/js-ipfs#1570 now merged and released, can this now be updated so we can merge? ignore, I just noticed you already updated. I'm going to review and re-run the tests
1a2504d
to
c374b7f
Compare
test/ipns.js
Outdated
.spawn({ | ||
repoPath: dir, | ||
disposable: false, | ||
initOptions: { bits: 512 } |
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.
How come no args: ['--offline']
for the js daemon?
test/ipns.js
Outdated
const stopPublisherAndStartResolverDaemon = (callback) => { | ||
if (sameDaemon) { | ||
return callback() | ||
} |
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.
I'd move this check out of this function and into your series
below and pass the two daemons in instead. I think it's unexpected for this function to not do any stopping or starting.
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.
I agree!
test/ipns.js
Outdated
this.timeout(120 * 1000) | ||
const dir = path.join(os.tmpdir(), hat()) | ||
|
||
parallel([ |
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.
This has to be series
for now, ipfsd-ctl
will occasionally error for parallel spawns because of races for ports.
test/ipns.js
Outdated
], callback) | ||
} | ||
|
||
describe('ipns', () => { |
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.
This could be named better - these are IPNS repo interop tests, when I first started reviewing I had incorrectly assumed they were testing that IPNS records published on js could be resolved on go and vice versa over the network.
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.
I changed it to: ipns locally using the same repo across implementations
. What do you think?
test/ipns.js
Outdated
this.timeout(160 * 1000) | ||
const dir = path.join(os.tmpdir(), hat()) | ||
|
||
parallel([ |
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.
Also series
:(
664da27
to
992cdbf
Compare
992cdbf
to
634e937
Compare
In the context of the
js-ipfs
PR for theIPNS working locally
implementation, I made this PR.PR's needed:
js-ipfs@0.32.0
withIPNS
and replace the package.json dependency (actually pointing to master)