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

gossipsub #5373

Merged
merged 8 commits into from
Aug 15, 2018
Merged

gossipsub #5373

merged 8 commits into from
Aug 15, 2018

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Aug 12, 2018

Adds the option to use gossipsub as the pubsub protocol.

This is handled by adding an Pubusub.Router config option, which allows the user to specify the pubsub router to use.
The possible values are

  • floodsub (or empty), for the default floodsub protocol
  • gossipsub for the gossipsub protocol

Requires:

Blocked:

@vyzo vyzo requested a review from Kubuxu as a code owner August 12, 2018 10:53
@ghost ghost assigned vyzo Aug 12, 2018
@ghost ghost added the status/in-progress In progress label Aug 12, 2018
@vyzo
Copy link
Contributor Author

vyzo commented Aug 12, 2018

Ugh, it seems that the configuration repo has been extracted from go-ipfs in the last day, which breaks my patch.
The option will have to be added to the extracted package ...

@vyzo
Copy link
Contributor Author

vyzo commented Aug 12, 2018

Updated go-ipfs-config to include the necessary option and rebased.

package.json Outdated
"name": "go-libp2p-pubsub-router",
"version": "0.3.9"
"version": "0.3.10"
Copy link
Member

Choose a reason for hiding this comment

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

FYI, I had to re-publish so this'll have to be re-updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this pulled a bunch of new deps, so lots of things need to be updated recursively... sigh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, this update touches too many things -- we'll have to wait for #5375 I think, as there are also code changes involved.


test_expect_success "enable gossipsub" '
for x in $(seq 0 4); do
ipfsi 0 config Pubsub.Router gossipsub
Copy link
Member

Choose a reason for hiding this comment

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

Should be ipfsi $x .... FYI, you can also use iptb for-each ipfs config ....

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit odd that the tests are still passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, good catch!
It's not odd that the test still pass, gossipsub is backwards compatible with floodsub.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Good to know.

@vyzo
Copy link
Contributor Author

vyzo commented Aug 13, 2018

We are blocked on #5375 as it interleaves a big update with a competing update to go-libp2p-pubsub-router.

We will have to wait for those updates to get merged (and rebase) before we can proceed, as it's out of scope to include a cascade of irrelevant package touches in this pr, let alone that it would create conflicts.

@Stebalien
Copy link
Member

@vyzo update merged.

@vyzo
Copy link
Contributor Author

vyzo commented Aug 13, 2018

rebased.

@vyzo
Copy link
Contributor Author

vyzo commented Aug 13, 2018

hrm, some merge errors -- fixing.

@vyzo
Copy link
Contributor Author

vyzo commented Aug 13, 2018

should be ok now.

@ghost ghost assigned Stebalien Aug 13, 2018
@Stebalien
Copy link
Member

Rebased.

@Stebalien Stebalien added RFM and removed status/in-progress In progress labels Aug 14, 2018
@Stebalien Stebalien requested a review from magik6k August 14, 2018 21:32
'

# this is just a copy of t0180-pubsub; smell.
startup_cluster $NUM_NODES --enable-pubsub-experiment
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to wrap this into a function in the pubsub test and call it with different setup, like https://github.com/ipfs/go-ipfs/blob/master/test/sharness/t0170-dht.sh#L10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of wanted to keep the two tests separate, but maybe they should be one.
We could also have a library script that contains the test logic in a function, and then source that from the two tests...

Copy link
Member

Choose a reason for hiding this comment

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

Up to you. I'm usually not a stickler on DRYing up tests, especially integration tests. We should never have to change these anyways ("don't break userspace").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, me neither... there is some merit in having a separate test for gossipsub so that the two can vary and perhaps have tests for protocol specific behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so if anyone has strong feelings about it, i can merge into a single test -- but i am otherwise inclined to keep it as is.

@@ -43,6 +43,11 @@ experimental, default-disabled.
run your daemon with the `--enable-pubsub-experiment` flag. Then use the
`ipfs pubsub` commands.

### gossipsub

You can enable the new experimental gossipsub protocol via configuration:
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a short line explaining the difference between the default floodsub and gossipsub.

Copy link
Contributor Author

@vyzo vyzo Aug 15, 2018

Choose a reason for hiding this comment

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

hrm, maybe, although that's a topic in itself. There will be a gossipsub spec soon [TM] and perhaps we could link to that instead.

Copy link
Member

Choose a reason for hiding this comment

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

For now, I'd just go with:

Gossipsub is a new, experimental routing protocol for pubsub that should waste less bandwidth than the current default routing protocol (floodsub). It's backwards compatible with floodsub so enabling this feature shouldn't break compatibility with existing IPFS nodes.

That should tell users everything they need to know (for now).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that sounds good -- will add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

License: MIT
Signed-off-by: vyzo <[email protected]>
License: MIT
Signed-off-by: vyzo <[email protected]>
vyzo and others added 5 commits August 15, 2018 10:02
in particular, this enabls gossipsub when configured so.

License: MIT
Signed-off-by: vyzo <[email protected]>
just a copy of t0180-pubsub with gossipsub enabled for now

License: MIT
Signed-off-by: vyzo <[email protected]>
License: MIT
Signed-off-by: vyzo <[email protected]>
License: MIT
Signed-off-by: Steven Allen <[email protected]>
@ghost ghost added the status/in-progress In progress label Aug 15, 2018
@Stebalien Stebalien added need/author-input Needs input from the original author and removed RFM labels Aug 15, 2018
License: MIT
Signed-off-by: vyzo <[email protected]>
@Stebalien Stebalien added RFM and removed status/in-progress In progress need/author-input Needs input from the original author labels Aug 15, 2018
@Stebalien Stebalien merged commit 99df463 into master Aug 15, 2018
@Stebalien Stebalien deleted the feat/gossipsub branch August 15, 2018 18:45
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants