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

Finished an initial version of GossipSub #15

Merged
merged 131 commits into from
May 16, 2019
Merged

Conversation

Mikerah
Copy link
Contributor

@Mikerah Mikerah commented Feb 6, 2019

I completed an initial version of gossipsub (I think). The code is based on this specification and the reference implementation in Go. The code most likely can't run due to several bugs that I have eyeballed and some missing functionality in other libp2p modules that I plan on working on.

I have started writing tests and I will be submitting PRs for these tests as I go along. In the meantime, any extra cleanup and bug fixes will be greatly appreciated.

Current blockers:

  • Publish doesn't work. In the 2 node tests, the node that receives the message from a topic that it is subscribed to does receive a graft message but it doesn't send a I want message. As a result, it doesn't get the actual message.

…plementing gossipsub. Will do a refactoring in the future
…ater to determine the correct return type in JS
…Sub in order to take advantage of a lot of the functionality when dealing directly with floodsub peers.
@raulk
Copy link

raulk commented Feb 7, 2019

@Mikerah – would you like @vasco-santos @jacobheun to have a peek as well?

@Mikerah
Copy link
Contributor Author

Mikerah commented Feb 7, 2019

@raulk If they have the time, that would be great!

@vasco-santos
Copy link
Collaborator

Great!
@Mikerah I will have a look early next week! 😀

index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/message/index.js Show resolved Hide resolved
@wemeetagain
Copy link
Member

@vasco-santos
Added all of the above tests minus the floodsub interop test.
In working through that test, I believe that floodsub interop functionality will require some sort of (potentially minor) change to pubsub.
Specifically, even if we add this.libp2p.handle(constants.FloodSubID, this._onConnection) to the appropriate section of the gossipsub start method, the connection to a floodsub peer will error here because of the way pubsub only dials a single protocol here.

@Mikerah
Copy link
Contributor Author

Mikerah commented May 11, 2019

@wemeetagain For this test, what needs to be done is to make sure that floodsub peers added to a gossipsub peer's mesh receives a subscription as if it were a floodsub peer, not a gossipsub peer. So, from the floodsub peer's perspective, it cannot treat a gossipsub peer as a floodsub peer so a gossipsub peer has to treat the floodsub peer as it is.

So, instead of handling floodsub peers in the start method, what you can do is once a peer is connected, determine what protocol is it using. It's peerInfo object should have this.protocols populated with protocols that it supports. If that peer is a floodsub peer, just send a message to it as per the floodsub spec.

@wemeetagain
Copy link
Member

@Mikerah
The problem is that a stream is never properly created. See the links in my previous comment.

I'm not talking about handling, in a general sense, the floodsub peers in the start method. I'm referring to the libp2p.handle method that gets called in the pubsub start method, which sets up a connection callback when a peer of a certain protocol is dialed. See here

The logic to maintain subscriptions and send messages appropriately to floodsub peers is already present (though untested).
The "issue" is that the pubsub module assumes the router has only a single protocol, and this is causing floodsub peers to not be properly connected, and this can only be fixed by refactoring pubsub or doing an unreasonable amount of method specialization in gossipsub.

@vasco-santos
Copy link
Collaborator

@wemeetagain that is indeed a problem! Can you PR js-libp2p-pubsub changing the libp2p.dialProtocol?

@wemeetagain
Copy link
Member

@vasco-santos
How would you feel about merging in our current work and finishing out floodsub/golang/etc interop in other PRs?

@wemeetagain wemeetagain dismissed their stale review May 13, 2019 14:12

stale review

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.

Thanks for the updates @wemeetagain

Can you check my latest comments? I will try to review the tests tomorrow, so that we can merge this PR and add the remaining tests in a new one


## Install

## Usage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please complete the README.md first?

References:

Copy link
Member

Choose a reason for hiding this comment

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

it looks like the API documentation was autogenerated in both cases.
I left the API section blank for now.

src/heartbeat.js Outdated Show resolved Hide resolved
src/heartbeat.js Outdated Show resolved Hide resolved
src/heartbeat.js Outdated Show resolved Hide resolved
src/pubsub.js Outdated Show resolved Hide resolved
src/pubsub.js Outdated Show resolved Hide resolved
src/pubsub.js Outdated Show resolved Hide resolved
test/gossip.js Outdated Show resolved Hide resolved
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! 🚀 It is time to merge this PR and iterate on it!

Awesome work @Mikerah and @wemeetagain 🎉

When you merge this PR, I will create a few issues in the repo with the remaining things we discussed here.

@wemeetagain wemeetagain merged commit bf99d47 into master May 16, 2019
@Mikerah Mikerah deleted the gossip_implementation branch May 27, 2019 13:25
fryorcraken pushed a commit to fryorcraken/js-libp2p-gossipsub that referenced this pull request Aug 2, 2022
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.

None yet

6 participants