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

feat: fallback to floodsub #34

Merged
merged 7 commits into from
Jul 11, 2019

Conversation

vasco-santos
Copy link
Collaborator

Added a fallback to dial with other peers running floodsub, behind an option.

For this context, the _dialPeer function was override from libp2p-pubsub to extend the dial behavior with the selection of the appropriate protocol to dial .

@ChainSafeSystems ChainSafeSystems requested a review from a team July 8, 2019 15:34
@codecov-io
Copy link

codecov-io commented Jul 8, 2019

Codecov Report

Merging #34 into master will increase coverage by 1.64%.
The diff coverage is 94.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   83.22%   84.86%   +1.64%     
==========================================
  Files           8        8              
  Lines         453      489      +36     
==========================================
+ Hits          377      415      +38     
+ Misses         76       74       -2
Impacted Files Coverage Δ
src/index.js 80.66% <100%> (+1.88%) ⬆️
src/pubsub.js 92.99% <94.73%> (+0.43%) ⬆️

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 cbf8015...21240f1. Read the comment docs.

Copy link
Collaborator

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

Added some minor language changes, but the major thing is updating the dial logic so we don't accidentally use floodsub if gossip is usable.

README.md Outdated Show resolved Hide resolved
src/index.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
src/pubsub.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
* @returns {void}
*/
_dialPeer (peerInfo, callback) {
const onDial = (idB58Str, pubsubStopped, conn) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand the purpose of this function wrapper for _onDial

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved the logic to the _onDial function.

jacobheun
jacobheun previously approved these changes Jul 10, 2019
Copy link
Collaborator

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

This LGTM. We can improve the dialPeer logic in a later PR once we add support for dialBest or similar in libp2p.

@vasco-santos
Copy link
Collaborator Author

@wemeetagain @Mikerah @GregTheGreek can you have a look and merge this PR if you approve it?

@GregTheGreek
Copy link
Member

@wemeetagain @Mikerah @GregTheGreek can you have a look and merge this PR if you approve it?

Hey sorry, I'm on it, I know @wemeetagain Is a bit under the weather

src/pubsub.js Show resolved Hide resolved
src/pubsub.js Show resolved Hide resolved
wemeetagain
wemeetagain previously approved these changes Jul 11, 2019
src/pubsub.js Outdated
@@ -186,6 +269,10 @@ class BasicPubSub extends Pubsub {
if (err) {
return callback(err)
}
// if fallback to floodsub enabled, we need to listen to its protocol
if (this._options.fallbackToFloodsub) {
this.libp2p.handle('/floodsub/1.0.0', this._onConnection)
Copy link
Member

Choose a reason for hiding this comment

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

consider pulling out the string into a constant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wait sorry! I will change it, I already have the constant 😅

@vasco-santos
Copy link
Collaborator Author

Should be ready now @wemeetagain
Let's just wait for CI

@wemeetagain wemeetagain merged commit c800992 into ChainSafe:master Jul 11, 2019
@vasco-santos vasco-santos deleted the feat/fallback-to-floodsub branch July 11, 2019 15:43
@vasco-santos
Copy link
Collaborator Author

Thanks ❤️
Can I have a new release?

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.

5 participants