-
Notifications
You must be signed in to change notification settings - Fork 0
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
b204c71
to
d0fcd71
Compare
5123e7f
to
fd9c69a
Compare
9374881
to
726a8c4
Compare
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.
This PR implements the blst bun bindings as in ChainSafe/blst-z#49
What this PR does:
eth_c_abi.zig
within that PR here asblst.zig
What this PR does NOT do:
test/perf
for a separate PR