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

Add ecdsa adaptor #11

Merged

Conversation

Tibo-lg
Copy link
Contributor

@Tibo-lg Tibo-lg commented Apr 7, 2021

As ECDSA adaptor signatures PR has been merged upstream (BlockstreamResearch/secp256k1-zkp#117) I tried to port the code I had for them in a fork of rust-secp256k1 here (hoping that it could be useful to others).

@Tibo-lg Tibo-lg force-pushed the add-ecdsa-adaptor branch 3 times, most recently from 3becc42 to 9658de2 Compare April 7, 2021 12:19
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @Tibo-lg!
Definitely most welcome :)

I've left some comments regarding naming, unsafe-block size and very importantly: panic behaviour.

src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
@Tibo-lg Tibo-lg force-pushed the add-ecdsa-adaptor branch from 9658de2 to 2202709 Compare April 8, 2021 06:26
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks for working in the comments and adding all these tests!

I've left some more feedback. Everything related to the tests is more of a nit though !

@jonasnick Can you also have a look at this please?

src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Confirmed that 7637c2e is the result of vendoring BlockstreamResearch/secp256k1-zkp@f3708a1.

@Tibo-lg Tibo-lg force-pushed the add-ecdsa-adaptor branch 3 times, most recently from 55df7d2 to 0229236 Compare April 12, 2021 02:47
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thank you!

Just one more typo in the docs!
Otherwise happy to merge this.

I'll give @jonasnick a couple of days in case he wants to have a look as well!

src/lib.rs Outdated Show resolved Hide resolved
@Tibo-lg Tibo-lg force-pushed the add-ecdsa-adaptor branch from 0229236 to 1830ec6 Compare April 12, 2021 04:17
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

utACK 1830ec6

Copy link
Collaborator

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

Very nice!

ACK mod nits

src/lib.rs Outdated Show resolved Hide resolved
src/zkp/ecdsa_adaptor.rs Show resolved Hide resolved
@Tibo-lg Tibo-lg force-pushed the add-ecdsa-adaptor branch from 1830ec6 to e009a51 Compare April 19, 2021 00:31
@Tibo-lg Tibo-lg force-pushed the add-ecdsa-adaptor branch from e009a51 to 309bbb4 Compare April 19, 2021 00:33
@Tibo-lg
Copy link
Contributor Author

Tibo-lg commented Apr 19, 2021

Rebased on a7409c9

@thomaseizinger
Copy link
Contributor

tACK 309bbb4

Verified locally that f9b8dc7 just vendors BlockstreamResearch/secp256k1-zkp@f3708a1 and does nothing else.

@jonasnick
Copy link
Collaborator

ACK 309bbb4

@jonasnick jonasnick merged commit 8baed9d into BlockstreamResearch:master Apr 19, 2021
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.

3 participants