Skip to content

Conversation

@yelhousni
Copy link
Contributor

@yelhousni yelhousni commented Jan 20, 2023

Use gnark-crypto instead of geth/crypto for secp256k1 and ecdsa.
This needs Consensys/gnark-crypto#310 to be merged first.

@yelhousni yelhousni requested a review from ivokub January 20, 2023 11:52
@yelhousni yelhousni marked this pull request as draft January 20, 2023 12:00
@gbotrel gbotrel added this to the v0.8.0 milestone Jan 23, 2023
@yelhousni yelhousni marked this pull request as ready for review January 24, 2023 13:58
@yelhousni
Copy link
Contributor Author

needs to be updated after Consensys/gnark-crypto#313 and Consensys/gnark-crypto#314 get merged in gnark-cypto.

@ivokub
Copy link
Collaborator

ivokub commented Feb 1, 2023

So, right now there is a slight problem - when we sign with gnark-crypto, as a signature we get a []byte, but it is difficult to get R and S from the signature. We would have to instantiate fr-specific ecdsa.Signature, unmarshal it from bytes and assign from it. It involves a type-switch, but I think it is ok.

I have some ideas and will try to push the changes.

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Rebased on top of develop, added a few more changes (avoid explicit conversion to *big.Int, rename package import, update go.mod).

Previously I totally overthought it. I think the simple approach is better (read signature directly into secp256k1 ECDSA signature struct and get R and S from there).

As far as it seems then the failing tests are due to MiMC hashing. Imo are unrelated to this PR.

@gbotrel gbotrel merged commit 1097a17 into develop Feb 6, 2023
@gbotrel gbotrel deleted the refactor/geth-dependency branch February 6, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants