feat: interface pubsub #60

vasco-santos · 2020-08-16T15:32:58Z

This PR adds pubsub-interface, which will replace libp2p/js-libp2p-pubsub per #53 . This is also an important work in the context of dropping the pubsub abstraction layer in js-libp2p per libp2p/js-libp2p-pubsub#68

The interface is closely in line with the base pubsub spec and also got some inspiration from go-libp2p-pubsub during the gossipsub refactor in preparation for 1.1.

This PR includes:

move libp2p-pubsub content into src/pubsub and test/pubsub.
refactor of libp2p-pubsub codebase regarding constructor and to better track streams
test suite that pubsub routers should use for testing

TODO:

routers test suite
final refactor on pending libp2p-gossipsub
base implementation test suite refactor
improve docs on proper usage, what to override and what not to override

Unblocks:

vasco-santos · 2020-08-18T16:54:52Z

@wemeetagain can you give a pass through this PR?
It is gigantic, but it is mostly for moving pieces around, both from libp2p-pubsub and your refactor from gossipsub. The main changes I needed to do are in the constructor, subscription handlers and some improvements on the coordination of overrides.

I did these changes in parallel with the routers update libp2p/js-libp2p-floodsub#108 and ChainSafe/js-libp2p-gossipsub#122 which are successfully using this code now

The main things left for me to finish tomorrow are improving the docs for usage and the refactor of the tests to include more than we had in libp2p-pubsub

vasco-santos · 2020-08-18T17:18:32Z

src/pubsub/index.js

+   * @param {InMessage} msg
+   * @returns {void}
+   */
+  _publishFrom (msg) {


I need to remove this according to the gossipsub last PR merged!

wemeetagain · 2020-08-18T17:27:26Z

src/pubsub/README.md

+| Name | Type | Description |
+|------|------|-------------|
+| topics | `Array<string>|string` | set of pubsub topics |
+| [handler] | `function (msg)` | handler for messages received in the given topics |


I think the interface should only allow subscription/unsubscription of a single topic at a time.

Attaching a handler to multiple topics at once feels like an anti-pattern, or at least a less primary interface, in the same way we were previously attaching multiple topics to multiple messages at once in publish.

I think this will simplify some of the underlying implementations, too. The body of our subscription code looks roughly like: for (const topic of topics) { /* doSubscribe(topic) */ }. We would be able to remove this outer loop and better support extended behavior.

Also, go-libp2p-pubsub's interface for subscribing is for a single topic at a time, for what its worth.

I agree that the API would look better with one topic. Taking into account that we are doing several breaking changes in pubsub, I think we should also change this.

The libp2p API doc also says a single topic, so we were supporting it below in the stack but we were not mentioning it. However, this should be pointed in the migration guide

What about publish? Should we also just accept one topic per API call? That's how we have it in libp2p main docs as well

👍 for publish also accepting a single topic per API call

I will update it

wemeetagain · 2020-08-18T17:32:56Z

src/pubsub/index.js

+   * @returns {Uint8Array}
+   */
+  _encodeRpc (rpc) {
+    throw errcode(new Error('_encodeRpc must be implemented by the subclass'), 'ERR_NOT_IMPLEMENTED')


In the same way that _acceptFrom returns a default implementation, we should consider providing default encoding/decoding using the base protobuf.

wemeetagain · 2020-08-18T17:34:19Z

src/pubsub/index.js

+      seqno: utils.randomSeqno(),
+      topicIDs: topics
+    }
+


pls copy the latest edits from gossipsub here, so we can have the signature already attached in _publish

It looks like this wasn't copied exactly, there was a reason it was as it was before:

we want to emit the message object with the optional signature (since other messages will be emitted with the signature and we shouldn't be treating self-published messages differently)

_publish currently expects an "InMessage" message transformed using utils.normalizeInRpcMessage. It should be standardized bc this function is called in different contexts: self-publish and peer-publish. Also, all overridable methods that operate on messages (_publish, _processRpcMessage, etc) receive an "InMessage". It's nice to rely on that and not require overriders to call 'utils.normalizeInRpcMessage'.

We had a test before stating: "should emit non normalized messages on publish" and that's why I put it like this.

I will revisit this in a bit

Yeah, the code is a little screwy, I think a proper solution would be to allow an InMessage (plus private key) to be signed (and have signature/key attached).

What we were doing to get around all this was:

create msg: InMessage

normalize/sign outMsg = await this.buildMessage(msg)

set msg to the non-normalized msg, but including the signature (msg = utils.normalizeInRpcMsg(outMsg))

then emit msg

_publish(msg)

I guess we can emit it normalized and signed for now. I don't see any reason against it. I changed to what you had before for now

wemeetagain · 2020-08-18T17:40:58Z

src/pubsub/index.js

+   * @param {function} [handler]
+   * @returns {void}
+   */
+  subscribe (topics, handler) {


Previously, we had this broken into two levels subscribe and _subscribe.
Perhaps this can be better named, eg: subscribe and _newSubscription (or some other).
But the outer layer, subscribe normalized input and removed existing subscriptions, so that _newSubscription was able to be extended in a way that didn't need to renormalize + remove existing subscriptions, but rather acted on known new subscriptions.
Eg: In gossipsub, we want to call join only on new subscriptions, so overriding just using this method becomes somewhat more cumbersome, than overriding a lower level method for "subscribing to new, previously unsubscribed topic".

I went with not decoupling it because all these operations seem mandatory to do. For the gossipsub example, I changed it to:

subscribe (topics: string[], handler: (msg: any) => void): void { super.subscribe(topics, handler) this.join(topics) }

We should only be extending its behaviour, not changing what it currently is implementing it.
Any reason you foresee to not simplify this?

I think this is fine, especially considering we'll only be operating on a single topic.

wemeetagain

It would be nice for typescript router implementers to have a typescript interface

vasco-santos · 2020-08-19T18:13:44Z

It would be nice for typescript router implementers to have a typescript interface

Ideally, we should move forward with the support via js doc + aegir soon

jacobheun

I still need to run through the tests but adding my initial comments on the source for now.

also, test/pubsub/utils/emit-self.spec.js is an empty file.

package.json

src/pubsub/README.md

src/pubsub/utils.js

jacobheun · 2020-08-20T17:03:08Z

src/pubsub/message/sign.js

+  const baseMessage = { ...message }
+  delete baseMessage.signature
+  delete baseMessage.key
+  if (typeof baseMessage.from === 'string') {


Won't this always be a string as this takes an InMessage? The message is run through the normalizeIn function by this point. If this is not a string there's an issue.

src/pubsub/message/sign.js

jacobheun · 2020-08-20T17:29:02Z

src/pubsub/index.js

+          this.log('received message we didn\'t subscribe to. Dropping.')
+          return
+        }
+        const msg = utils.normalizeInRpcMessage(message, PeerId.createFromB58String(idB58Str))


The PeerId passed to normalizeInRpcMessage is converted to its string form for use, and I only see this called with a PeerId here. Perhaps this should just change to taking and using the string to avoid the double conversion.

src/pubsub/index.js

Co-authored-by: Jacob Heun <[email protected]>

vasco-santos · 2020-08-21T09:52:31Z

also, test/pubsub/utils/emit-self.spec.js is an empty file.

yeah lol, I created it inside utils by mistake and it moved away from my radar. Just added the base tests for it and the review is addressed

src/pubsub/message/sign.js

src/pubsub/utils.js

jacobheun

Just a few minor things left

src/pubsub/tests/multiple-nodes.js

test/pubsub/sign.spec.js

test/pubsub/topic-validators.spec.js

Co-authored-by: Jacob Heun <[email protected]>

…es into feat/interface-pubsub

jacobheun

LGTM

vasco-santos mentioned this pull request Aug 17, 2020

feat: gossipsub 1.1 libp2p/js-libp2p#733

Merged

4 tasks

vasco-santos force-pushed the feat/interface-pubsub branch 5 times, most recently from db97b73 to b8cca2d Compare August 17, 2020 09:40

feat: interface pubsub

af3487a

vasco-santos force-pushed the feat/interface-pubsub branch 3 times, most recently from 603aea4 to 1093890 Compare August 17, 2020 17:12

chore: pubsub router tests

7d5b3c4

vasco-santos force-pushed the feat/interface-pubsub branch from 1093890 to 7d5b3c4 Compare August 17, 2020 17:25

vasco-santos mentioned this pull request Aug 17, 2020

chore: use pubsub interface ChainSafe/js-libp2p-gossipsub#122

Merged

2 tasks

vasco-santos force-pushed the feat/interface-pubsub branch 10 times, most recently from 7c975af to fe6329f Compare August 18, 2020 15:58

chore: move pubsub abstractions from gossipsub

d8671f9

vasco-santos force-pushed the feat/interface-pubsub branch from fe6329f to d8671f9 Compare August 18, 2020 16:46

vasco-santos commented Aug 18, 2020

View reviewed changes

wemeetagain reviewed Aug 18, 2020

View reviewed changes

vasco-santos force-pushed the feat/interface-pubsub branch from e266b17 to 3173755 Compare August 19, 2020 08:13

chore: normalize msg before emit

ec5ae25

vasco-santos force-pushed the feat/interface-pubsub branch from e905864 to ec5ae25 Compare August 19, 2020 16:19

chore: do not reset inbound stream

f209d04

vasco-santos marked this pull request as ready for review August 19, 2020 17:02

vasco-santos requested a review from jacobheun August 19, 2020 17:03

vasco-santos mentioned this pull request Aug 19, 2020

chore: update pubsub libp2p/js-libp2p-floodsub#108

Merged

1 task

wemeetagain reviewed Aug 19, 2020

View reviewed changes

jacobheun reviewed Aug 20, 2020

View reviewed changes

vasco-santos force-pushed the feat/interface-pubsub branch from 9c7dd34 to bc8ed8b Compare August 21, 2020 09:10

chore: apply suggestions from code review

d5ffc49

Co-authored-by: Jacob Heun <[email protected]>

vasco-santos force-pushed the feat/interface-pubsub branch from bc8ed8b to d5ffc49 Compare August 21, 2020 09:48

jacobheun reviewed Aug 21, 2020

View reviewed changes

src/pubsub/message/sign.js Outdated Show resolved Hide resolved

jacobheun reviewed Aug 21, 2020

View reviewed changes

src/pubsub/utils.js Outdated Show resolved Hide resolved

vasco-santos added 2 commits August 21, 2020 14:46

chore: address review

a45ff8d

fix: remove subscribe handler

94c1806

vasco-santos force-pushed the feat/interface-pubsub branch from 6a967fd to 94c1806 Compare August 21, 2020 13:00

vasco-santos requested a review from jacobheun August 21, 2020 13:42

jacobheun reviewed Aug 24, 2020

View reviewed changes

src/pubsub/tests/multiple-nodes.js Show resolved Hide resolved

test/pubsub/sign.spec.js Outdated Show resolved Hide resolved

test/pubsub/topic-validators.spec.js Outdated Show resolved Hide resolved

vasco-santos and others added 2 commits August 24, 2020 14:28

chore: remove bits from create peerId

956067a

Co-authored-by: Jacob Heun <[email protected]>

chore: remove delay from topic validators tests

05491ac

vasco-santos mentioned this pull request Aug 24, 2020

[pubsub] add partial network subscribe tests #63

Closed

Merge branch 'feat/interface-pubsub' of github.com:libp2p/js-interfac…

2152105

…es into feat/interface-pubsub

vasco-santos force-pushed the feat/interface-pubsub branch from 00104ec to 2152105 Compare August 24, 2020 13:27

vasco-santos added 2 commits August 25, 2020 10:17

chore: add event emitter information

f389446

fix: topic validator docs

34e69ed

jacobheun approved these changes Aug 25, 2020

View reviewed changes

jacobheun merged commit ba15a48 into master Aug 25, 2020

jacobheun deleted the feat/interface-pubsub branch August 25, 2020 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: interface pubsub #60

feat: interface pubsub #60

vasco-santos commented Aug 16, 2020 •

edited

Loading

vasco-santos commented Aug 18, 2020 •

edited

Loading

vasco-santos Aug 18, 2020

wemeetagain Aug 18, 2020

vasco-santos Aug 19, 2020

vasco-santos Aug 19, 2020

wemeetagain Aug 19, 2020

vasco-santos Aug 19, 2020

wemeetagain Aug 18, 2020

wemeetagain Aug 18, 2020

wemeetagain Aug 19, 2020 •

edited

Loading

vasco-santos Aug 19, 2020

wemeetagain Aug 19, 2020 •

edited

Loading

vasco-santos Aug 19, 2020

wemeetagain Aug 18, 2020

vasco-santos Aug 19, 2020 •

edited

Loading

wemeetagain Aug 19, 2020

wemeetagain left a comment

vasco-santos commented Aug 19, 2020

jacobheun left a comment

jacobheun Aug 20, 2020

jacobheun Aug 20, 2020

vasco-santos commented Aug 21, 2020 •

edited

Loading

jacobheun left a comment

jacobheun left a comment

feat: interface pubsub #60

feat: interface pubsub #60

Conversation

vasco-santos commented Aug 16, 2020 • edited Loading

vasco-santos commented Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wemeetagain Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wemeetagain Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wemeetagain left a comment

Choose a reason for hiding this comment

vasco-santos commented Aug 19, 2020

jacobheun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Aug 21, 2020 • edited Loading

jacobheun left a comment

Choose a reason for hiding this comment

jacobheun left a comment

Choose a reason for hiding this comment

vasco-santos commented Aug 16, 2020 •

edited

Loading

vasco-santos commented Aug 18, 2020 •

edited

Loading

wemeetagain Aug 19, 2020 •

edited

Loading

wemeetagain Aug 19, 2020 •

edited

Loading

vasco-santos Aug 19, 2020 •

edited

Loading

vasco-santos commented Aug 21, 2020 •

edited

Loading