-
-
Notifications
You must be signed in to change notification settings - Fork 312
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
Update libp2p to ESM version #4114
Conversation
81e9924
to
c0168a2
Compare
@wemeetagain : is there an estimate on when this will be completed? I'm asking because am hoping Lodestar can use the "canary" version of js-libp2p to provide feedback on the resource protection mechanisms that have recently been introduced. |
c0168a2
to
687b0ae
Compare
687b0ae
to
764ecad
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
This is ready for initial review, the last remaining items to fix are related to updating stream limits per protocol. |
Thanks for sharing @wemeetagain . No expectation to respond as don't want to create extra work, but if there's a place can point to, I'd be interested in knowing what the remaining work is and if there's anything needed from js-libp2p at this point. |
I don't think there's anything remaining on js-libp2p's side that we need. Performance is now mostly on-par or better than our unstable and stable branches. We've redeployed 67da674 across a group in our fleet (roughly 24 hrs ago) and are monitoring metrics to iron out the last remaining things. The big thing we're working through:
|
packages/beacon-node/package.json
Outdated
"libp2p-tcp": "^0.17.2", | ||
"multiaddr": "^10.0.1", | ||
"peer-id": "^0.16.0", | ||
"libp2p": "0.37.3-509e56a", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wemeetagain are there newer versions?
} | ||
|
||
// create metric if undefined | ||
if (metrics[protocol] === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we just add protocol
in labelNames
and simplify this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not, bc they split out the direction. eg: would be helpful to see if we're spammed with identify requests, but not responding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, just add protocol
as an additional label. Yes good idea. Lets tackle metrics cleanup in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! metrics could be refactored in followup
cc @achingbrain we finally did it buddy |
Some different gossipsub metrics I notice after 6h-7h (in a 12h timeframe):
|
"@types/tmp": "^0.2.0", | ||
"@types/varint": "^6.0.0", | ||
"eventsource": "^2.0.2", | ||
"leveldown": "^6.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, why is leveldown required? For libp2p datastore? If so, @wemeetagain can you motivate upstream dep to move to level >= 8.0.0?
Motivation
We want to get the latest libp2p, gossipsub, etc updates.
Description
TODO:
Closes #4140
Closes #4043