-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add blst bindings #18
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
base: main
Are you sure you want to change the base?
Conversation
726a8c4 to
062fc28
Compare
| aggregateWithRandomness( | ||
| sets.concat({ | ||
| pk: sets[0].pk, | ||
| //TODO: (@matthewkeil) this throws error "Public key is infinity" not signature because there is only one blst error |
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.
what does this TODO mean, is this still relevant? @matthewkeil
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.
yes its still relevant. Check out the error type https://github.com/ChainSafe/blst-z/pull/49/files#diff-31ebad738598504e01f7313f21e76c7eb1c128f9cc636dca0fa8957b7ec15748R22 which wraps c.BLST_PK_IS_INFINITY, but no corresponding c.BLST_SIG_IS_INFINITY exists. The way the error is used is that it is returned whenever either p1 or p2 is infinity.
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.
So it shouldn't be a TODO, but a note?
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
|
I just added a few benchmarks to sanity check, somehow looks quite bad? |
|
looks like we did not ultilize multi-threads? also need to make sure a |
|
@twoeths is right, the port did not contain the mt version. I've since ported it over in ChainSafe/blst-z#56, and is using it in a branch that uses this as base: #21 The aggregation numbers look better but is still underperforming even under before: after:
|
This missing c flag as well as the macro were causing some regressions in performance within lodestar-bun versus production blst-ts in ChainSafe/lodestar-bun#18.
0effe9e to
361b6a2
Compare
* use new aggregateverify * update blst * remove unnecessary bytes.bench.ts from bench cmd * Update blst aggregateVerify * (change me) temporary changes to bench blst aggregateVerify only * bist: fix init
|
latest numbers with proper thread pool/memory pool impl. On M2 Mac it looks bad still: On Linux it's more reasonable, and outperforms at smaller sizes: |
022f861 to
81f6198
Compare
| // }, | ||
| // }); | ||
|
|
||
| bench({ |
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.
can this code be deduped a bit?
even just a for loop with the various number of signatures/messages
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.
For sure, this was supposed to be temporary so I quickly did this. The version before was pretty concise, I'll switch to that
| "init": { | ||
| "args": [], | ||
| "returns": "u32" | ||
| }, | ||
| "deinit": { | ||
| "args": [], | ||
| "returns": "void" | ||
| }, |
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.
these function names probably need to be qualified.
This PR implements the blst bun bindings as in ChainSafe/blst-z#49
What this PR does:
eth_c_abi.zigwithin that PR here asblst.zigWhat this PR does NOT do:
test/perffor a separate PR