mimblewimble / secp256k1-zkp

Fork of secp256k1-zkp for the Grin/MimbleWimble project
MIT License
32 stars 42 forks source link

Aggsig compatibility with BIP-schnorr #22

Closed jaspervdm closed 6 years ago

jaspervdm commented 6 years ago

Optionally add a public key to the aggsig challenge in both the creation and verification of the signature. Compatibility with BIP-schnorr (under "key prefixing")

Also fixes https://github.com/mimblewimble/secp256k1-zkp/pull/21 and fixes https://github.com/mimblewimble/secp256k1-zkp/pull/23

ignopeverell commented 6 years ago

Can't we just rely on the upstream musig code instead?

jaspervdm commented 6 years ago

@ignopeverell could you point me to where to find this code? I couldnt find any references to aggsig or musig in the ElementsProject fork or the original secp256k1 repository

ignopeverell commented 6 years ago

Mmmh looks like you're right. I was assuming the release of the BIP came with code but looks like I was wrong, sorry about that.

jaspervdm commented 6 years ago

Updated the tests to reflect the changes, however 1 is failing (L245, validating combined signature, even though separate signatures work). I don't understand why but will research it more

garyyu commented 6 years ago

One more comment:
change the title of this PR to change aggsig to bip-schnorr, something like that. and put this link in this PR: https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr.mediawiki

yeastplume commented 6 years ago

You guys have been all over this already and it all looks good to me. (Also very glad to have had more pairs of eyes on this code). Happy to merge when ready (it won't affect Grin builds until it's explicitly tagged)

jaspervdm commented 6 years ago

If there are no further comments, I think the PR is ready to be merged in

jaspervdm commented 6 years ago

Final small change on suggestion of @garyyu: write the full 33 bytes of the public key, as in the BIP-schnorr reference code

yeastplume commented 6 years ago

Great, merging! Thanks for all your work on this