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: allow configuring stream limits #293

Merged
merged 4 commits into from
Jul 22, 2022

Conversation

achingbrain
Copy link
Collaborator

The registrar defaults to only allowing 1x inbound and 1x outbound stream per protocol so allow configuring more than that.

I've plucked some defaults out of the air, these will need tuning to make sure they're not too restrictive or permissive.

The registrar defaults to only allowing 1x inbound and 1x outbound
stream per protocol so allow configuring more than that.

I've plucked some defaults out of the air, these will need tuning
to make sure they're not too restrictive or permissive.
@achingbrain achingbrain requested a review from a team as a code owner June 29, 2022 17:37
@wemeetagain
Copy link
Member

Isn't the max incoming/outgoing streams per-connection? So you'd only need 1 in both direction?

@wemeetagain
Copy link
Member

probably need to change the comments here: https://github.com/libp2p/js-libp2p-interfaces/blob/master/packages/interface-registrar/src/index.ts#L15

I've been so confused by the errors I'm seeing running lodestar. Definitely related to this.

@wemeetagain
Copy link
Member

I think the incoming and outgoing limits should be the same no?

We're doing some funky thing where we open two streams (one inbound, one outbound) per peer, and we read on one and write to the other. I can't find anything in the spec that suggests that this is necessary though...
Since streams are duplexes, we should be able to handle both inbound and outbound traffic on one?

@achingbrain
Copy link
Collaborator Author

achingbrain commented Jun 29, 2022

Since streams are duplexes, we should be able to handle both inbound and outbound traffic on one?

Yes, in this case one stream should suffice.

I think the incoming and outgoing limits should be the same no?

I'm not sure - my thinking was you wouldn't want a remote peer to connect to you then open a ton of streams and potentially fill up the incoming message buffer for each one, but you might want to open a ton of streams to a remote peer which is why I set it to be asymmetric.

Isn't the max incoming/outgoing streams per-connection? So you'd only need 1 in both direction?

They are per-connection limits, yes but an application might have some weird requirement that needs multiple simultaneous streams? I was definitely seeing IPFS test failures around IPNS over pubsub that were failing until I exposed the stream limits locally and upped the values.

probably need to change the comments here

If they are confusing people then yes, most likely.

@wemeetagain
Copy link
Member

I think these defaults are too permissive. Gossipsub is a little different than many other protocols in that streams between peers are long-lived, and you only really need a single one. I'm not even sure what it means to have multiple gossipsub inbound or outbound streams for a single peer. (would published messages be pushed out to all streams? only the most recent?)

Current behavior is we only currently 'track' two steams per peer (one inbound, one outbound).
If more inbound streams are created, they will overwrite the prior inbound stream:
https://github.com/ChainSafe/js-libp2p-gossipsub/blob/master/src/index.ts#L609 and
https://github.com/libp2p/js-libp2p-pubsub/blob/master/src/peer-streams.ts#L87

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

I think the incoming and outgoing limits should be the same no?

We're doing some funky thing where we open two streams (one inbound, one outbound) per peer, and we read on one and write to the other. I can't find anything in the spec that suggests that this is necessary though... Since streams are duplexes, we should be able to handle both inbound and outbound traffic on one?

Here's a reference to why we're using unidirectional streams
libp2p/js-libp2p-pubsub#45

achingbrain and others added 2 commits July 22, 2022 11:46
Co-authored-by: Cayman <caymannava@gmail.com>
@achingbrain
Copy link
Collaborator Author

libp2p/js-libp2p#1301 increases the default stream limits so we can go with these for now, we can always tune further at a later date.

Talking to some of the libp2p folk, single, full-duplex or long-lived half-duplex gossipsub streams remove a bit of flexibility around things like low-power devices which may not be able to hold a connection open forever and may just want to periodically connect to send a message so I guess we should handle multiple incoming streams, also ones that only exist for the duration of one message, etc.

@wemeetagain wemeetagain mentioned this pull request Jul 22, 2022
wemeetagain
wemeetagain previously approved these changes Jul 22, 2022
src/index.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2022

Codecov Report

Merging #293 (ef0e1a3) into master (8486c39) will increase coverage by 1.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
+ Coverage   80.47%   81.53%   +1.06%     
==========================================
  Files          42       43       +1     
  Lines        9104     9501     +397     
  Branches      826      918      +92     
==========================================
+ Hits         7326     7747     +421     
+ Misses       1778     1754      -24     
Impacted Files Coverage Δ
src/index.ts 70.79% <100.00%> (+1.34%) ⬆️
test/gossip.spec.ts 95.53% <100.00%> (+0.75%) ⬆️
src/utils/index.ts 100.00% <0.00%> (ø)
src/score/peer-score.ts 87.66% <0.00%> (ø)
src/utils/has-gossip-protocol.ts
test/signature-policy.spec.ts 99.06% <0.00%> (ø)
src/stream.ts 100.00% <0.00%> (ø)
test/utils/create-pubsub.ts 78.18% <0.00%> (+1.71%) ⬆️
... and 3 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 8486c39...ef0e1a3. Read the comment docs.

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.

3 participants