Skip to content
This repository has been archived by the owner on Aug 1, 2023. It is now read-only.

test: add ipns pubsub tests #39

Merged
merged 2 commits into from
May 13, 2019
Merged

test: add ipns pubsub tests #39

merged 2 commits into from
May 13, 2019

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Oct 27, 2018

PR containing a set of tests aimimg to test the IPNS over Pubsub interoperability between go-ipfs and js-ipfs.

It needs the following:

cc @bigs

@ghost ghost assigned vasco-santos Oct 27, 2018
@ghost ghost added the status/in-progress In progress label Oct 27, 2018
@vasco-santos
Copy link
Member Author

Currently, we are facing a problem regarding the interop of the pubsub topic.

go-ipfs sets the topic as /ipns/BINARY_PEER_ID. In Golang strings are a slice of bytes, and consequently, do not have a specific encoding. On the other side, strings in Javascript must have an encoding format associated. This way, when decoding /ipns/BINARY_PEER_ID to the topic string and sending the message to a go peer, it cannot parse the topic correctly.

Considering all this and that IPNS over Pubsub is still an experimental feature, I have been talking with @Stebalien about changing the topic format to a standard encoding, such as base64 for both implementations.

@Stebalien
Copy link
Member

So, some history: we used to use /ipns/Qm... but I changed it to /ipns/BINARY_ID to match what we do in the DHT. This allows us to treat pubsub as just another (kind of funky) record store (see https://github.com/libp2p/go-libp2p-pubsub-router). This, in turn, means we don't need to duplicate all the IPNS logic for both the DHT and Pubsub (or any other "record stores" we make in the future.

However, given that record-store keys are binary (arguably, they should also just be strings...), we could use /rs/base32(key) (or base64url). In this case, that would be /record/$base64url(/ipns/BINARY_KEY).

However, I'd also like to consider just making pubsub topics arbitrary binary strings. We already do this for the DHT and forcing users to use UTF-8 for pubsub topics will likely cause issues like this in the future as well (some implementations will enforce it, others won't).

@Stebalien
Copy link
Member

I've hashed this out with @vyzo on IRC. He agrees we should probably be nice and use UTF8.

So, the only remaining question is: How? Personally, I'd vote for either /record/base32(...) or /record/base64url(...). Thoughts?

@vasco-santos
Copy link
Member Author

Thanks for your analysis @Stebalien .

I would go with /record/base64url(/ipns/BINARY_KEY) since it will need fewer bytes and the topic will go with every single message. In addition, with ipns.pubsub.subs we can get the decoded topic.

@vasco-santos vasco-santos added the status/blocked Unable to be worked further until needs are met label Nov 4, 2018
Stebalien added a commit to libp2p/go-libp2p-pubsub-router that referenced this pull request Nov 15, 2018
Instead of using bare keys (binary), use `/record/base64url(key)`. This:

1. Pubsub keys must be UTF-8. Record-store keys definitely are not.
2. The prefix may help with validators in the future (namespace *everything*).

See: ipfs/interop#39
@Stebalien
Copy link
Member

@vyzo suggested base32 to be consistent with everything else we do. However, I really don't mind either way. To get this ball rolling: libp2p/go-libp2p-pubsub-router#17

@magik6k
Copy link
Member

magik6k commented Nov 15, 2018

I'd use base64 since this is internal, and we send these topics to all peers we are pubsubbing with, so b64 saves us a few bytes each time we connect to someone.

@vasco-santos
Copy link
Member Author

Interop between go-ipfs and js-ipfs is green locally using:

JS: ipfs/js-ipfs#1559 with ipfs/js-datastore-pubsub/pull/9 for fixing the interop

Go: go-ipfs#master with libp2p/go-libp2p-pubsub-router#17 (binary go-ipfs can be obtained with: https://ipfs.io/ipfs/QmXQDCKdsEaeDyejWn2ziXfTSjBd5YEZ6zoHTdrpMJWyvf)

@Stebalien do you have any eta for the new go-ipfs release with this?

@alanshaw maybe we can add this to v0.34.0 RELEASE 🚀

@vasco-santos vasco-santos changed the title [WIP] test: add ipns pubsub tests test: add ipns pubsub tests Nov 26, 2018
@vasco-santos vasco-santos mentioned this pull request Dec 3, 2018
1 task
test/utils/wait-for.js Outdated Show resolved Hide resolved
before(function (done) {
this.timeout(100 * 1000)

parallel([
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: async/each

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is better to use async/parallel here, since using async/each here would require an extra array to keep the data with all the ids, which I feel unnecessary just for 2 elements.

nodeBId = ids[1]

nodes[0].api.swarm.connect(ids[1].addresses[0], () => {
setTimeout(done, 60000) // wait for republish as we can receive the republish message first
Copy link
Member

Choose a reason for hiding this comment

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

This is such a long wait, it would be great if this was somehow configurable...

@Stebalien is it possible in go-ipfs?

@vasco-santos perhaps add a console.log line here to inform the user this pause is happening...?

Copy link
Member Author

Choose a reason for hiding this comment

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

go-ipfs already accepts the republish configuration through the node config. For JS Land, it is one of the things that I want to integrate during this quarter.

Meanwhile, I can add a console.log here!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wait, go-ipfs allows the RepublishPeriod and RecordLifetime, but not the time for the first InitialRebroadcastDelay, afaik!

test/ipns-pubsub.js Outdated Show resolved Hide resolved
test/ipns-pubsub.js Outdated Show resolved Hide resolved
test/ipns-pubsub.js Outdated Show resolved Hide resolved
test/ipns-pubsub.js Outdated Show resolved Hide resolved
@daviddias
Copy link
Member

@vasco-santos mind rebasing master onto this PR and test it with js-ipfs master?

@daviddias daviddias changed the title test: add ipns pubsub tests [WIP] test: add ipns pubsub tests Feb 13, 2019
@alanshaw
Copy link
Member

alanshaw commented Apr 15, 2019

@vasco-santos what's the status of this? Can we get it merged plz 🙏?

@vasco-santos vasco-santos force-pushed the add-ipns-pubsub-tests branch 3 times, most recently from 48f1bc6 to 2353dbc Compare April 17, 2019 10:21
@vasco-santos
Copy link
Member Author

Subscribe in js and Publishing in go is currently failing. It seems that js node is not receiving the pubsub message once go publishes.

I just tested this using the latest js-ipfs and go-ipfs CLIs and everything worked fine, so I will debug this once I have some time

@vasco-santos vasco-santos changed the title [WIP] test: add ipns pubsub tests test: add ipns pubsub tests May 13, 2019
@vasco-santos
Copy link
Member Author

@alanshaw it is 💚 🎊

@alanshaw alanshaw merged commit 3e89ad4 into master May 13, 2019
@alanshaw alanshaw deleted the add-ipns-pubsub-tests branch May 13, 2019 11:35
@whyrusleeping whyrusleeping removed the status/in-progress In progress label May 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/blocked Unable to be worked further until needs are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants