-
-
Notifications
You must be signed in to change notification settings - Fork 289
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: swap blst for napi library #6350
Conversation
73fea83
to
1f2d67e
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6350 +/- ##
=========================================
Coverage 60.13% 60.14%
=========================================
Files 407 407
Lines 46451 46462 +11
Branches 1534 1534
=========================================
+ Hits 27935 27944 +9
- Misses 18486 18488 +2
Partials 30 30 |
Performance Report✔️ no performance regression detected Full benchmark results
|
if (this.blsPoolType === BlsPoolType.workers) { | ||
await this.runJobWorkerPool(prepared); | ||
} else { | ||
await this.runJobLibuv(prepared); |
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.
if possible I'd like to refactor the current implementation to have BaseBlsMultiThreadPool
to move all common code there, have BlsMultiThreadWorkerPool
for worker specific implementation, and have this new work in another implementation, so there would be multiple steps required to make this happens similar to n_historical_states
work I've worked on
that would make a smooth transition to new bls implementation, it's also easier to review and maintain. What's your opinions on this approach @matthewkeil @wemeetagain ?
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.
Sounds good. I can take a look at this in the updated PR #6362 once the metrics look good. Going to try and limit the complexity until I can get it running smoothly though @tuyennhv
the test mainnet node does not look great in the last 6h, may worth to look into this @matthewkeil vs unstable |
Closing this in favor of #6362 |
Motivation
DRAFT PR. Not ready for full review.
NOTE: most of the diff is the blst-ts (with the full supranational/bls) library itself. It has not been published so all that code is brought into lodestar to test the code
Branch is deployed to
feat2
to collect metricsDescription
Swaps all references to
@chainsafe/bls
and@chainsafe/blst
with the new library@chainsafe/blst-ts