Skip to content

Add secp256k1-zkp and schnorrsig modules #1

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

Merged
merged 5 commits into from
May 28, 2019
Merged

Add secp256k1-zkp and schnorrsig modules #1

merged 5 commits into from
May 28, 2019

Conversation

jonasnick
Copy link
Collaborator

@jonasnick jonasnick commented May 9, 2019

WIP because this requires a new rust-secp256k1 release (with rust-bitcoin/rust-secp256k1#105 and rust-bitcoin/rust-secp256k1#101).

This PR adds 3 crates, a no_std and low-level zkp-sys, higher level zkp and a crate -dev for dev dependencies.

@jonasnick jonasnick force-pushed the schnorrsig branch 2 times, most recently from 459b005 to 8be2b45 Compare May 21, 2019 21:07
@jonasnick jonasnick changed the title WIP: Add secp256k1-zkp and schnorrsig modules Add secp256k1-zkp and schnorrsig modules May 21, 2019
@jonasnick
Copy link
Collaborator Author

un-WIP'ed because rust-secp256k1 0.13 is released

}

trait New {
fn new<R: Rng>(rng: &mut R) -> SecretKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want impl Distribution<SecretKey> for Standard rather than this trait.

See the second example in https://docs.rs/rand/0.6.5/rand/distributions/index.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm getting E0117 ("impl doesn't use types inside crate") because SecretKey isn't defined in this crate. This looks ergonomic but a downside is that copy & pasted code starts diverging from rust-secp.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you PR to rust-secp? Or then do we run into the problem with dev-dependencies features?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, yes we'd run into that problem.

.define("USE_NUM_NONE", Some("1"))
.define("USE_FIELD_INV_BUILTIN", Some("1"))
.define("USE_SCALAR_INV_BUILTIN", Some("1"))
.define("USE_ENDOMORPHISM", Some("1"))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should drop this for now because gmax is concerned about patent aggression related to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just copied it from rust-secp. Is it a concern there also?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I thought we'd removed it from rust-secp.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this took me way to long to figure out but we can't remove USE_ENDOMORPHISM here without also removing it in rust-secp because the context objects become incompatible.

@apoelstra
Copy link
Contributor

ACK

@jonasnick
Copy link
Collaborator Author

squashed

@jonasnick jonasnick merged commit 2f8adae into master May 28, 2019
@jonasnick jonasnick deleted the schnorrsig branch May 28, 2019 12:56
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.

2 participants