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

fix: increasing maxInboundStreams for lightpush from 32 to 100 #2021

Merged
merged 2 commits into from
May 30, 2024

Conversation

gabrielmer
Copy link
Contributor

Problem

Solution for waku-org/nwaku#2478 and #1790

Solution

The root cause seems to be in js-libp2p, in the DEFAULT_MAX_INBOUND_STREAMS const defined here

When running the test, got the following log

  libp2p:mplex:stream:receiver:36 abort with error CodeError: Too many inbound protocol streams for protocol "/vac/waku/filter-push/2.0.0-beta1" - limit 32
    at file:///Users/gabrielmer/status/workspace2/js-waku/node_modules/libp2p/src/upgrader.ts:388:29
    at processTicksAndRejections (node:internal/process/task_queues:95:5) {
  code: 'ERR_TOO_MANY_INBOUND_PROTOCOL_STREAMS',
  props: {}
} +0ms

If we configure a higher limit of input streams in js-libp2p, the bug is fixed

Note

Feel free to recommend cleaner ways to implement the solution. Probably better to have a constant instead of a hardcoded 100, but waiting for the recommendations of the js-waku experts 😁

Copy link

github-actions bot commented May 22, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 181.41 KB (+0.02% 🔺) 3.7 s (+0.02% 🔺) 922 ms (+105.82% 🔺) 4.6 s
Waku Simple Light Node 181.43 KB (-0.06% 🔽) 3.7 s (-0.06% 🔽) 661 ms (-4.25% 🔽) 4.3 s
ECIES encryption 23.12 KB (0%) 463 ms (0%) 244 ms (+159.64% 🔺) 707 ms
Symmetric encryption 22.58 KB (0%) 452 ms (0%) 209 ms (+80.54% 🔺) 660 ms
DNS discovery 72.49 KB (0%) 1.5 s (0%) 338 ms (-36.99% 🔽) 1.8 s
Peer Exchange discovery 74.15 KB (0%) 1.5 s (0%) 452 ms (+97.61% 🔺) 2 s
Local Peer Cache Discovery 67.68 KB (0%) 1.4 s (0%) 678 ms (+178.23% 🔺) 2.1 s
Privacy preserving protocols 38.87 KB (0%) 778 ms (0%) 279 ms (-23.41% 🔽) 1.1 s
Waku Filter 111.92 KB (-0.1% 🔽) 2.3 s (-0.1% 🔽) 473 ms (+18.34% 🔺) 2.8 s
Waku LightPush 110.39 KB (0%) 2.3 s (0%) 506 ms (-14.47% 🔽) 2.8 s
History retrieval protocols 110.91 KB (0%) 2.3 s (0%) 368 ms (-23.69% 🔽) 2.6 s
Deterministic Message Hashing 7.29 KB (0%) 146 ms (0%) 73 ms (+85.42% 🔺) 219 ms

@gabrielmer gabrielmer changed the title increasing maxInboundStreams for lightpush from 32 to 100 fix: increasing maxInboundStreams for lightpush from 32 to 100 May 22, 2024
@weboko
Copy link
Collaborator

weboko commented May 28, 2024

Thank you @gabrielmer for root causing the issue!

I think this config should be set here -

return createLibp2p({

If you don't mind I can commit to this PR

@danisharora099
Copy link
Collaborator

Thank you @gabrielmer for root causing the issue!

I think this config should be set here -

return createLibp2p({

If you don't mind I can commit to this PR

I wouldn't set it for all the connections by the node -- when we set it as done by @gabrielmer, we are only changing the value for that particular registrar handler/connection

Copy link
Collaborator

@danisharora099 danisharora099 left a comment

Choose a reason for hiding this comment

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

LGTM

@gabrielmer
Copy link
Contributor Author

@weboko @danisharora099 thanks so much! Feel free to modify and commit to the PR in whatever way you feel it improves the solution.
If there's anything I can help me just let me know anytime :))

@danisharora099
Copy link
Collaborator

@weboko merging now, let me know!

@danisharora099 danisharora099 merged commit 2311a59 into master May 30, 2024
11 checks passed
@danisharora099 danisharora099 deleted the fix-disconnection-beyond-32-lightpush-messages branch May 30, 2024 08:28
@weboko weboko mentioned this pull request May 14, 2024
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