feat: use @chainsafe/blst directly #6706
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #6706 +/- ##
============================================
- Coverage 61.87% 61.87% -0.01%
============================================
Files 557 562 +5
Lines 59209 59305 +96
Branches 1915 1916 +1
============================================
+ Hits 36635 36694 +59
- Misses 22531 22568 +37
Partials 43 43 |
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
matthewkeil
reviewed
May 1, 2024
| @@ -31,7 +40,11 @@ export const testFnByType: Record<string, "skip" | ((data: any) => any)> = { | |||
| */ | |||
| function aggregate_verify(input: {pubkeys: string[]; messages: string[]; signature: string}): boolean { | |||
| const {pubkeys, messages, signature} = input; | |||
| return bls.verifyMultiple(pubkeys.map(fromHexString), messages.map(fromHexString), fromHexString(signature)); | |||
| try { | |||
There was a problem hiding this comment.
@wemeetagain I added this try/catch for the spec tests but am updating the base lib to not throw for this so it better meets the spec.
matthewkeil
reviewed
May 1, 2024
Comment on lines
+149
to
+153
| try { | ||
| return _verify(fromHexString(message), fromHexString(pubkey), fromHexString(signature)); | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
@wemeetagain I added this try/catch for the spec tests but am updating the base lib to not throw for this so it better meets the spec.
matthewkeil
reviewed
May 1, 2024
Comment on lines
+92
to
+96
| try { | ||
| return aggregateVerify(messages.map(fromHexString), pubkeys.map(fromHexString), fromHexString(signature)); | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
@wemeetagain I added this try/catch for the spec tests but am updating the base lib to not throw for this so it better meets the spec.
matthewkeil
reviewed
May 1, 2024
Comment on lines
+192
to
+196
| try { | ||
| return _verify(fromHexString(message), fromHexString(pubkey), fromHexString(signature)); | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
@wemeetagain I added this try/catch for the spec tests but am updating the base lib to not throw for this so it better meets the spec.
|
Metrics on feat2-mainnet do not look great. They should be equal to unstable/stable but seems to be performing worse for some reason... Will try and swap for another feat group tomorrow and see if its the machine |
|
After running for a while it seems like this branch is actually fine. @tuyennhv would love your opinion as well but I'm going to approve |
twoeths
reviewed
May 8, 2024
twoeths
previously approved these changes
May 8, 2024
Added them both to |
|
@twoeths @philknows Spoke with @wemeetagain and we will merge this one after interop. Should not be an issue but why add more complexity to an already moving target. Can sit for the week and get merged when we get back. All is ready and just waiting for reapproval after the last update for the comment @twoeths had |
twoeths
reviewed
May 9, 2024
| let isAllValid = true; | ||
| // validate signature = true | ||
| const signatures = sets.map((set) => { | ||
| try { | ||
| return bls.Signature.fromBytes(set.signature, CoordType.affine, true); | ||
| const sig = Signature.deserialize(set.signature, CoordType.affine); | ||
| sig.sigValidate(); |
There was a problem hiding this comment.
use signatureFromBytes() here
twoeths
reviewed
May 9, 2024
| pubkeys.map((hex) => bls.PublicKey.fromBytes(fromHexString(hex), CoordType.jacobian, true)), | ||
| fromHexString(message) | ||
| const sig = Signature.deserialize(fromHexString(signature), undefined); | ||
| sig.sigValidate(); |
There was a problem hiding this comment.
need to search for all sigValidate() and replace with signatureFromBytes()
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
In order to do some optimizations, we want to remove the
@chainsafe/blsabstraction layer. We know we won't be running the beacon node or validator in node only. In the future, we may relax this restriction if a usecase presents itself.Description
@chainsafe/blsthroughout the monorepo, with the exception of the light client package.toBytes()->.serialize().keyValidate()or.sigValidate().fromBytes()->.deserialize()Pubkey.aggregate,bls.aggregatePubkeys->aggregatePubkeysSignature.verifyAggregate->fastVerifyAggregate