-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
chore: upgrade to js-libp2p 2.0 #7077
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #7077 +/- ##
============================================
- Coverage 49.19% 49.18% -0.01%
============================================
Files 598 598
Lines 39797 39801 +4
Branches 2094 2087 -7
============================================
- Hits 19577 19576 -1
- Misses 20179 20184 +5
Partials 41 41 |
just deployed to feat3 |
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
@wemeetagain is this what's the consequence of that and do we need to tweak params for them? |
Yes
When we receive new (non-duplicate) messages greater than We likely want to avoid having to send "a lot" of IDONTWANTs and make sure we're just sending them for messages that are "larger". Avoid sending for attestations, ensure sending for blocks and blobs. We can bump |
redeployed with tweaked idontwantMinDataSize |
// Only send IDONTWANT messages if the message size is larger than this | ||
// This should be large enough to not send IDONTWANT for "small" messages | ||
// See https://github.com/ChainSafe/lodestar/pull/7077#issuecomment-2383679472 | ||
idontwantMinDataSize: 16829, |
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.
Regarding #7077 (comment), isn't the maxSize
inflated because the list limits are way too high but in reality can't happen because those are just aggregates over committees while the limit is the block attestation max.
Anyhow doesn't matter much as long as this is smaller than block / blob size
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.
Yeah, its way higher than needed, but it should suffice for now.
* chore: upgrade to js-libp2p 2.0 * chore: bump libp2p versions * chore: fix up yarn lock * chore: fix some tests * chore: fix connection map * feat: gossipsub v1.2 * chore: bump libp2p * chore: tweak idontwantMinDataSize * chore: bump libp2p deps
This reverts commit d37bdb0.
we received too many |
🎉 This PR is included in v1.23.0 🎉 |
Description