-
Notifications
You must be signed in to change notification settings - Fork 37
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
Use aux rand for ecdsa adaptor encrypt #15
Use aux rand for ecdsa adaptor encrypt #15
Conversation
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.
Not a big deal since ECDSA signers don't expect to get the same kind of synthetic nonces as BIP-340 signers do. Your fix looks good to me.
But I think we should mention somewhere what this auxiliary randomness is because otherwise it seems rather confusing to someone who has never seen this as it's also not clear where to look upw hat this means. Perhaps somehere, e.g. in the doc for encrypt
, we could say This function derives a nonce using a similar process as described in BIP-340. Therefore, the nonce derivation process can be strengthened against side channel attacks by providing auxiliary randomness.
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.
ack 065fd19
I like this approach. Agree with Jonas that we could probably document this better so that nostd users know that it's perfectly safe to use encrypt_no_aux_rand
.
src/zkp/ecdsa_adaptor.rs
Outdated
) -> EcdsaAdaptorSignature { | ||
let mut aux = [0u8; 32]; | ||
rng.fill_bytes(&mut aux); | ||
encrypt_helper( |
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.
This can forward to encrypt_with_aux_rand
instead.
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 point fixed
src/zkp/ecdsa_adaptor.rs
Outdated
@@ -84,31 +88,116 @@ impl EcdsaAdaptorSignature { | |||
} | |||
} | |||
|
|||
fn encrypt_helper<C: Signing>( |
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.
If we have such kind of helper functions, I would prefer them to be defined further down from where they are used. Makes it easier to scan through the code if the public API is defined first and followed by code that is only an implementation detail.
In this case though, I don't think we need a helper function at all. Instead, I think it is clearer if we prefer forwarding to other public functions (see other comment) plus for the no_aux usecase, just duplicate the invocation. DRY has a price in the form of indirection and in this case, I don't think it is worth it :)
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.
Agreed, updated as suggested. Another solution I thought of would be to have a single function encrypt_no_std
that takes an Option<&[u8; 32]>
as pasted below but I don't know if the match would have some negative performance impact.
pub fn encrypt_no_std<C: Signing>(
secp: &Secp256k1<C>,
msg: &Message,
sk: &SecretKey,
enckey: &PublicKey,
aux_rand: Option<&[u8; 32]>,
) -> EcdsaAdaptorSignature {
let mut adaptor_sig = ffi::EcdsaAdaptorSignature::new();
let aux_rand_ptr = match aux_rand {
Some(aux) => aux.as_c_ptr() as *mut ffi::types::c_void,
None => ptr::null_mut(),
};
unsafe {
debug_assert!(
ffi::secp256k1_ecdsa_adaptor_encrypt(
*secp.ctx(),
&mut adaptor_sig,
sk.as_c_ptr(),
enckey.as_c_ptr(),
msg.as_c_ptr(),
ffi::secp256k1_nonce_function_ecdsa_adaptor,
aux_rand_ptr,
) == 1
);
};
EcdsaAdaptorSignature(adaptor_sig)
}
065fd19
to
5a68fb3
Compare
@jonasnick @apoelstra thanks for the review. I updated the docs adapting a bit the suggestion I hope it's fine. |
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.
utACK 5a68fb3
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.
ACK 5a68fb3
@nkohen pointed out to me that the latest version of the ECDSA adaptor signatures use auxiliary random data to generate the adaptor signature similarly to what is done for Schnorr signatures in BIP340.
Unfortunately I realized that when porting my code from the old version to the new one I just used null values for the nonce data and function, which makes it impossible to pass auxiliary random data at the moment. In this PR I mimicked what I had done in rust-bitcoin/rust-secp256k1#237 when adding Schnorr signatures to rust-secp256k1. Again unfortunately, while I think that this is the best way to implement things, it is also a breaking change AFAICT since now the simple
EcdsaAdaptorSignature::encrypt
function requires compilation withrand-std
feature.Sorry for missing that! Let me know what you think would be the best way to fix it.