-
Notifications
You must be signed in to change notification settings - Fork 4
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 module #14
base: master
Are you sure you want to change the base?
Conversation
a37083c
to
d6c271b
Compare
aa14da7
to
d4f796b
Compare
Thanks for putting this together, it's really cool if we can get this stuff in the lib. A few more thoughts I thought it might be an idea to add here:
.. on review I think you addressed all my other comments already, either in code or in TODO Do you envisage having the dleq stuff separate at some point, given it may be useful outside of the ecdsa adaptor case? |
Yeah, that's important. I fixed that right after you mentioned it, so unless I'm missing something, that fix should be included in the current version.
Perhaps the dleq stuff could be its own module and automatically turned on when you enable ecdsa-adaptors or whatever else. |
Oh sorry, yes, I see it now. Looks good.
Yes. Was thinking of coinjoin/dual funding type use cases ('scarcity token' things). |
if (buf33[0] > 1) { | ||
return 0; | ||
} | ||
secp256k1_ge_set_xo_var(p, &x, buf33[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secp256k1_ge_set_xo_var
can fail https://github.com/bitcoin-core/secp256k1/blob/6034a04fb1afe7d78dd367ec719d3ced9db2b05e/src/group_impl.h#L234 I think you should check the return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, great catch. Pushed fix. There could be more issues like that because this PR doesn't include a complete test suite yet (as mentioned in the TODO list of secp256k1_ecdsa_adaptor.h
.
return 1; | ||
} | ||
|
||
int secp256k1_ecdsa_adaptor_extract_secret(const secp256k1_context* ctx, unsigned char *adaptor_secret32, const secp256k1_ecdsa_signature *sig, const unsigned char *adaptor_sig65, const secp256k1_pubkey *adaptor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about enforcing the logic that if the correct secret is extracted then this always implies the signature is correct (for both ECDSA and Schnorr). This is what I went for here: https://github.com/LLFourn/secp256kfun/blob/master/ecdsa_fun/src/adaptor/mod.rs#L256. I check that the R_x in the ciphertext is the same as the R_x in the final signature.
This is to protect against the weird but I think not unimaginable situation where an attacker sends you a sig
that has a correct s
value but a bogus R
value off-chain. Since you successfully extract the secret the programmer may think that this implies the signature was valid too. If they try to use it later they may end up in a pickle.
This is not normally an issue from a protocol standpoint since it is usually the creator of the encrypted signature that is trying to extract the secret so the R value is not interesting. I'm not sure this is always the case though. I think there is no case where you want to extract the secret from an invalid signature.
It is also just faster to extract/verify at the same time rather than verify then extract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea and very cheap to do. Added a fix that compares the R values. I haven't thought through all the edge cases to guarantee that the signature is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I will put this in the DLC specification then. I think as long as you are not enforcing low_s in verification then making sure that the R_x values are the same is sufficient as long as the encrypted signature is valid and you successfully recover the correct key.
This module implements single signer ECDSA adaptor signatures following "One-Time Verifiably Encrypted Signatures A.K.A. Adaptor Signatures" by Lloyd Fournier https://github.com/LLFourn/one-time-VES/blob/master/main.pdf).
Note that at this module is currently a work in progress. It's not secure nor stable. Let me repeat: IT IS EXTREMELY DANGEROUS AND RECKLESS TO USE HIS MODULE IN PRODUCTION. DON'T!