feat: allow usage of custom codecs #288

D4nte · 2022-06-23T00:16:16Z

The gossip codecs are now available on GossipSub.multicodecs.
Hence, the check can use this field instead of a function that uses
the constants.

This enables library consumers to set their own codecs by assigning a
different value to GossipSub.multicodecs.

D4nte · 2022-06-23T04:20:36Z

The CI errors I am encountering are not related to the changes introduced by this PR:

Error: test/compliance.spec.ts(19,25): error TS2345: Argument of type 'import("/home/runner/work/js-libp2p-gossipsub/js-libp2p-gossipsub/node_modules/@libp2p/interface-pubsub-compliance-tests/node_modules/@libp2p/components/dist/src/index").Components' is not assignable to parameter of type 'import("/home/runner/work/js-libp2p-gossipsub/js-libp2p-gossipsub/node_modules/@libp2p/components/dist/src/index").Components'.
  Types have separate declarations of a private property 'peerId'.
Error: test/utils/create-pubsub.ts(23,5): error TS2322: Type 'MockConnectionManager' is not assignable to type 'ConnectionManager'.
  The types returned by 'getConnections(...)' are incompatible between these types.
    Type 'import("/home/runner/work/js-libp2p-gossipsub/js-libp2p-gossipsub/node_modules/@libp2p/interface-connection/dist/src/index").Connection[]' is not assignable to type 'import("/home/runner/work/js-libp2p-gossipsub/js-libp2p-gossipsub/node_modules/@libp2p/interface-pubsub/node_modules/@libp2p/interface-connection/dist/src/index").Connection[]'.
      Type 'import("/home/runner/work/js-libp2p-gossipsub/js-libp2p-gossipsub/node_modules/@libp2p/interface-connection/dist/src/index").Connection' is not assignable to type 'import("/home/runner/work/js-libp2p-gossipsub/js-libp2p-gossipsub/node_modules/@libp2p/interface-pubsub/node_modules/@libp2p/interface-connection/dist/src/index").Connection'.
        Types of property 'streams' are incompatible.
          Type 'import("/home/runner/work/js-libp2p-gossipsub/js-libp2p-gossipsub/node_modules/@libp2p/interface-connection/dist/src/index").Stream[]' is not assignable to type 'import("/home/runner/work/js-libp2p-gossipsub/js-libp2p-gossipsub/node_modules/@libp2p/interface-pubsub/node_modules/@libp2p/interface-connection/dist/src/index").Stream[]'.
            Type 'Stream' is missing the following properties from type 'Stream': stat, metadata

wemeetagain

Lgtm

dapplion · 2022-06-23T06:15:19Z

src/index.ts

-          if (peerStreams && hasGossipProtocol(peerStreams.protocol) && !peers.has(id) && !this.direct.has(id)) {
+          if (
+            peerStreams &&
+            this.multicodecs.includes(peerStreams.protocol) &&


includes loops over an array, would it be better to use a Set? Otherwise it's a quadratic loop inside shuffledPeers

The multicodecs field comes from the PubSub interface in @libp2p/interface-pubsub: https://github.com/libp2p/js-libp2p-interfaces/blob/9f8d5f45647e3bd8943acfbfb77424b0f83c27cf/packages/interface-pubsub/src/index.ts#L74

I see 3 solutions:

Modify the pubsub interface and then gossipsub to change multicodecs to a Set<string>.

Add another field on GossipSub that is a Set and implement multicodecs as a getter to conform to the PubSub interface,

Leave it as it is.

Happy to proceed with either.

~~In this instance, I do not expect a library consumer to set a high number of value in the array so I am not sure 1 & 2 are worth it~~

I can see that the check is done inside a loop of a loop (this.mesh.forEach and then shuffledPeers) so indeed, it may be worth doing 1 or 2.

Lets try to upstream the change in the interface (1)

Lets try to upstream the change in the interface (1)

Done: libp2p/js-libp2p-interfaces#260

Waiting for it to be reviewed/merged/released to update this PR.

To move forward with this, I need to do a run of the test suite with a Set and an Array to check if there is any performance gain: libp2p/js-libp2p-interfaces#260 (comment)

Unfortunately, the CI is currently broken on this repo due to the lack of lockfile and discrepancy in the dependencies being used. I am looking if I can fix that myself.

Getting there, I hit a road block trying to bump several libp2p libraries, some help would be appreciated :) #289

Running benchmarks on a droplet with multicodecs as Array and Set.

Note that the Set does not seem to help with performance: libp2p/js-libp2p-interfaces#260 (comment)

Would it be acceptable to merge the PR with just 4b873a3 ?

The `multicodecs` values are the protocol codecs for the given pubsub implementation. They are likely to be set once (at instantiation) but accessed multiple time: everytime it needs to check whether a remote peer supports the same pubsub protocol. Hence, the usage of `Set` over `Array` is more adapted as it will improve access performance by using `Set.prototype.has` O(1) instead of `Array.prototype.includes` O(N). Refs: ChainSafe/js-libp2p-gossipsub#288

D4nte · 2022-06-27T06:12:51Z

Blocked on CI failure:

Need to merge chore(deps)!: bump @libp2p/interface-connection from 1.0.1 to 2.1.0 libp2p/js-libp2p-utils#51
Need to release @libp2p/utils and use it @ChainSafe/libp2p-gossipsub

The gossip codecs are now available on `GossipSub.multicodecs`. Hence, the check can use this field instead of a function that uses the constants. This enables library consumers to set their own codecs by assigning a different value to `GossipSub.multicodecs`.

codecov-commenter · 2022-07-13T03:54:02Z

Codecov Report

Merging #288 (b21ee49) into master (489355c) will decrease coverage by 1.03%.
The diff coverage is 50.00%.

❗ Current head b21ee49 differs from pull request most recent head 4b873a3. Consider uploading reports for the commit 4b873a3 to get more accurate results

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
- Coverage   81.48%   80.44%   -1.04%     
==========================================
  Files          44       41       -3     
  Lines        9447     9104     -343     
  Branches      917      826      -91     
==========================================
- Hits         7698     7324     -374     
- Misses       1749     1780      +31

Impacted Files	Coverage Δ
src/utils/index.ts	`100.00% <ø> (ø)`
src/index.ts	`69.44% <50.00%> (-1.20%)`	⬇️
src/utils/buildRawMessage.ts	`74.46% <0.00%> (-9.93%)`	⬇️
src/utils/msgIdFn.ts	`90.47% <0.00%> (-9.53%)`	⬇️
src/utils/publishConfig.ts	`78.26% <0.00%> (-6.53%)`	⬇️
test/utils/create-pubsub.ts	`76.47% <0.00%> (-1.72%)`	⬇️
src/score/peer-score.ts	`87.66% <0.00%> (ø)`
test/signature-policy.spec.ts
src/stream.ts
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 489355c...4b873a3. Read the comment docs.

wemeetagain · 2022-07-14T14:22:43Z

Can you force push the right version here? looks like it still has the commit to change to a Set

D4nte · 2022-07-19T02:04:21Z

Can you force push the right version here? looks like it still has the commit to change to a Set

Done! Ready.

D4nte requested a review from a team as a code owner June 23, 2022 00:16

D4nte mentioned this pull request Jun 23, 2022

Upgrade to latest libp2p libraries waku-org/js-waku#803

Merged

3 tasks

D4nte force-pushed the use-multicodecs branch from 954046f to 71d5906 Compare June 23, 2022 03:59

D4nte changed the title ~~Remove hasGossipProtocols~~ feat: allow usage of custom codecs Jun 23, 2022

D4nte force-pushed the use-multicodecs branch from 71d5906 to 0f65368 Compare June 23, 2022 04:13

wemeetagain previously approved these changes Jun 23, 2022

View reviewed changes

dapplion reviewed Jun 23, 2022

View reviewed changes

D4nte mentioned this pull request Jun 24, 2022

fix!: use Set for improved performance when searching in multicodecs libp2p/js-libp2p-interfaces#260

Closed

D4nte dismissed wemeetagain’s stale review via b21ee49 June 30, 2022 00:55

D4nte force-pushed the use-multicodecs branch from 0f65368 to b21ee49 Compare June 30, 2022 00:55

D4nte force-pushed the use-multicodecs branch from 6e18cd4 to e4ff157 Compare July 13, 2022 03:53

D4nte force-pushed the use-multicodecs branch from e4ff157 to 4b873a3 Compare July 19, 2022 02:04

wemeetagain approved these changes Jul 19, 2022

View reviewed changes

wemeetagain merged commit 0e76f49 into ChainSafe:master Jul 19, 2022

github-actions bot mentioned this pull request Jul 19, 2022

chore(master): release 3.2.0 #302

Merged

D4nte deleted the use-multicodecs branch July 20, 2022 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow usage of custom codecs #288

feat: allow usage of custom codecs #288

D4nte commented Jun 23, 2022

D4nte commented Jun 23, 2022

wemeetagain left a comment

dapplion Jun 23, 2022 •

edited

D4nte Jun 23, 2022 •

edited

wemeetagain Jun 23, 2022

D4nte Jun 24, 2022

D4nte Jun 27, 2022

D4nte Jun 28, 2022

D4nte Jun 30, 2022

D4nte Jul 13, 2022

D4nte Jul 14, 2022

wemeetagain Jul 14, 2022

D4nte commented Jun 27, 2022

codecov-commenter commented Jul 13, 2022 •

edited

wemeetagain commented Jul 14, 2022

D4nte commented Jul 19, 2022

feat: allow usage of custom codecs #288

feat: allow usage of custom codecs #288

Conversation

D4nte commented Jun 23, 2022

D4nte commented Jun 23, 2022

wemeetagain left a comment

Choose a reason for hiding this comment

dapplion Jun 23, 2022 • edited

Choose a reason for hiding this comment

D4nte Jun 23, 2022 • edited

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

Choose a reason for hiding this comment

D4nte commented Jun 27, 2022

codecov-commenter commented Jul 13, 2022 • edited

Codecov Report

wemeetagain commented Jul 14, 2022

D4nte commented Jul 19, 2022

dapplion Jun 23, 2022 •

edited

D4nte Jun 23, 2022 •

edited

codecov-commenter commented Jul 13, 2022 •

edited