Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update libp2p-pubsub #110

Merged
merged 7 commits into from
Jul 21, 2020
Merged

chore: update libp2p-pubsub #110

merged 7 commits into from
Jul 21, 2020

Conversation

wemeetagain
Copy link
Member

Update libp2p-pubsub to v0.6.x (passes tests with libp2p/js-libp2p-pubsub#65 and libp2p/js-libp2p-pubsub#66 applied)

  • Update InMessage object to include receivedFrom peer-id string
    • Update methods that previously took (id: string, msg: InMessage) to just be (msg: InMessage) since InMessage now contains the receivedFrom peer id
  • Update validate to throw on invalid messages
    • Remove _processTopicValidatorResult in favor of overriding validate in order to apply validation penalties
  • Update publish/_publish for publishing a single message
  • Update tests as needed, remove now irrelevant tests
  • Replace peer/Peer with peerStreams/PeerStreams in many places
    • (in a future PR, will refactor gossipsub maps/internal methods to use peer id string identifiers instead of peerStreams in many places)
  • Remove special handling from fix: send subscriptions once ready #39 which isn't needed. If a subscription is triggered after a peer has opened an inbound stream but before a peer has opened an outbound stream, we don't need special handling. The subscription will simply be sent when the peer opens the outbound stream (via _onPeerConnected).

Copy link
Collaborator

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Things are getting cleaner now 👍

ts/pubsub.js Outdated Show resolved Hide resolved
subs.forEach((subOpt) => this._processRpcSubOpt(peer, subOpt))
this.emit('pubsub:subscription-change', peer.id, peer.topics, subs)
subs.forEach((subOpt) => this._processRpcSubOpt(idB58Str, subOpt))
this.emit('pubsub:subscription-change', peerStreams.id, subs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any strong reason to remove the topics the peer is subscribed from the event?

Copy link
Member Author

Choose a reason for hiding this comment

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

We aren't indexing "peer => subscribed topics" anymore.
Previously the Peer maintained a list of topics - Peer#topics
Now we just have this.topics - topic => subscribed peers

we could:

  1. leave this as is, keeping this event modified
  2. loop thru all this.topics, collecting subscribed peers
  3. create another index, eg: this.peerTopics - peer => subscribed topics

I've got a preference for 1 or 3

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's leave it for now. If we have something built on top of this information, we can reconsider what to do

ts/pubsub.js Show resolved Hide resolved
ts/index.ts Outdated Show resolved Hide resolved
ts/pubsub.js Outdated Show resolved Hide resolved
@vasco-santos
Copy link
Collaborator

CI seems broken. Any cache related issue?

@wemeetagain
Copy link
Member Author

Yeah, just cleared the cache and rerunning now

@wemeetagain
Copy link
Member Author

It seems the tests are failing w floodsub interop. Floodsub seems to be using the 0.6.x branch of libp2p-pubsub 🤔

@wemeetagain
Copy link
Member Author

I'll put in a PR to update floodsub, link to that branch here

Copy link
Collaborator

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM!

@wemeetagain wemeetagain merged commit 6a792de into gsv1.1 Jul 21, 2020
@wemeetagain wemeetagain deleted the cayman/gs1.1-update-pubsub branch July 21, 2020 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants