Skip to content
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

Replace kilic/bls12-381 with gnark-crypto/ecc/bls12-381 #2629

Open
wants to merge 24 commits into
base: gligneul/fix-update-gethpin-v1.14.0
Choose a base branch
from

Conversation

amsanghi
Copy link
Contributor

@amsanghi amsanghi commented Sep 3, 2024

Fixes : NIT-2721

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Sep 3, 2024
@amsanghi amsanghi marked this pull request as ready for review September 3, 2024 14:48
@amsanghi amsanghi changed the title Replace bls Replace kilic/bls12-381 with gnark-crypto/ecc/bls12-381 Sep 3, 2024
blsSignatures/blsSignatures.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me, but I have a few recommended simplifications and test cases

blsSignatures/blsSignatures.go Outdated Show resolved Hide resolved
blsSignatures/blsSignatures.go Outdated Show resolved Hide resolved
blsSignatures/blsSignatures.go Show resolved Hide resolved
blsSignatures/blsSignatures.go Outdated Show resolved Hide resolved
blsSignatures/blsSignatures.go Outdated Show resolved Hide resolved
blsSignatures/blsSignatures.go Outdated Show resolved Hide resolved
blsSignatures/blsSignatures.go Show resolved Hide resolved
blsSignatures/blsSignatures.go Outdated Show resolved Hide resolved
blsSignatures/blsSignatures.go Outdated Show resolved Hide resolved
blsSignatures/blsSignatures.go Outdated Show resolved Hide resolved
@amsanghi amsanghi changed the base branch from update-gethpin-v1.14.0 to gligneul/fix-update-gethpin-v1.14.0 October 11, 2024 13:05
@PlasmaPower
Copy link
Collaborator

I pushed up a fuzzer for the new impl vs the old impl to the new-bls-impl-fuzzer branch. It's currently failing, I believe because the new impl accepts compressed points but the old impl does not. For the public key and signature deserialization in the new impl we should ensure the compressed bit (I think the most significant bit from the docs) is not set. Private key deserialization isn't relevant to this though, as it's only done with local trusted input.

@amsanghi
Copy link
Contributor Author

I pushed up a fuzzer for the new impl vs the old impl to the new-bls-impl-fuzzer branch. It's currently failing, I believe because the new impl accepts compressed points but the old impl does not. For the public key and signature deserialization in the new impl we should ensure the compressed bit (I think the most significant bit from the docs) is not set. Private key deserialization isn't relevant to this though, as it's only done with local trusted input.

Thanks for running the fuzzer on this, updated!

@PlasmaPower
Copy link
Collaborator

The new impl is now panicking with panic: runtime error: index out of range [0] with length 0 if passed an empty signature or public key bytes. I think your new if statements should be prefixed with e.g. if len(in) == 0 || .

You can also run the fuzzer yourself locally by checking out the new-bls-impl-fuzzer branch and running go test ./blsSignaturesNew -v -fuzz FuzzSignatureSerialization . The fuzzer also catches this new panic.

@amsanghi
Copy link
Contributor Author

FuzzSignatureSerialization

Thanks for catching this, updated! Also started running fuzzer to see if it catches something new.

@amsanghi
Copy link
Contributor Author

FuzzSignatureSerialization

Thanks for catching this, updated! Also started running fuzzer to see if it catches something new.

Its failing with
new_vs_old_fuzz_test.go:25: signature mismatch: 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 vs 400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

It cause the first byte is being set to mUncompressedInfinity in the new setup, any thoughts on how to handle this?

@PlasmaPower
Copy link
Collaborator

I think that's a cofactor issue.. Maybe try calling ClearCofactor after deserializing points in the new impl? I'm also getting a different fuzz failure locally with the input 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000030 . I think we need to require it's exactly 96 bytes in the new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants