bitcoinjs / tiny-secp256k1

A tiny secp256k1 native/JS wrapper
MIT License
90 stars 55 forks source link

Feature: Schnorr #59

Closed junderw closed 2 years ago

junderw commented 2 years ago

Let me know if I need to add anything else.

junderw commented 2 years ago

Thoughts (still not sure):

junderw commented 2 years ago

I would be interested to know if the below is secure.

Possible solution

```diff diff --git a/src/lib.rs b/src/lib.rs index f22adbf..751249a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,9 +17,9 @@ use secp256k1_sys::{ secp256k1_ec_pubkey_tweak_add, secp256k1_ec_pubkey_tweak_mul, secp256k1_ec_seckey_negate, secp256k1_ec_seckey_tweak_add, secp256k1_ecdsa_sign, secp256k1_ecdsa_signature_normalize, secp256k1_ecdsa_signature_parse_compact, secp256k1_ecdsa_signature_serialize_compact, - secp256k1_ecdsa_verify, secp256k1_keypair_create, secp256k1_keypair_xonly_pub, - secp256k1_nonce_function_bip340, secp256k1_nonce_function_rfc6979, secp256k1_schnorrsig_sign, - secp256k1_schnorrsig_verify, secp256k1_xonly_pubkey_from_pubkey, secp256k1_xonly_pubkey_parse, + secp256k1_ecdsa_verify, secp256k1_keypair_create, secp256k1_nonce_function_bip340, + secp256k1_nonce_function_rfc6979, secp256k1_schnorrsig_sign, secp256k1_schnorrsig_verify, + secp256k1_xonly_pubkey_from_pubkey, secp256k1_xonly_pubkey_parse, secp256k1_xonly_pubkey_serialize, secp256k1_xonly_pubkey_tweak_add, secp256k1_xonly_pubkey_tweak_add_check, types::c_void, Context, KeyPair, PublicKey, Signature, XOnlyPublicKey, SECP256K1_SER_COMPRESSED, SECP256K1_SER_UNCOMPRESSED, SECP256K1_START_SIGN, @@ -52,7 +52,7 @@ const HASH_SIZE: usize = 32; const EXTRA_DATA_SIZE: usize = 32; const SIGNATURE_SIZE: usize = 64; -const ERROR_BAD_PRIVATE: usize = 0; +// const ERROR_BAD_PRIVATE: usize = 0; const ERROR_BAD_POINT: usize = 1; const ERROR_BAD_TWEAK: usize = 2; // const ERROR_BAD_HASH: usize = 3; @@ -128,25 +128,6 @@ fn get_context() -> *const Context { } } -unsafe fn create_keypair(input: *const u8) -> InvalidInputResult { - let mut kp = KeyPair::new(); - if secp256k1_keypair_create(get_context(), &mut kp, input) == 1 { - Ok(kp) - } else { - Err(ERROR_BAD_PRIVATE) - } -} - -unsafe fn x_only_pubkey_from_pubkey( - input: *const u8, - inputlen: usize, -) -> InvalidInputResult<(XOnlyPublicKey, i32)> { - let mut xonly_pk = XOnlyPublicKey::new(); - let mut parity: i32 = 0; - let pubkey = jstry!(pubkey_parse(input, inputlen), Ok((xonly_pk, parity))); - x_only_pubkey_from_pubkey_struct(&mut xonly_pk, &mut parity, &pubkey) -} - unsafe fn x_only_pubkey_from_pubkey_struct( xonly_pk: &mut XOnlyPublicKey, parity: &mut i32, @@ -332,35 +313,6 @@ pub extern "C" fn point_from_scalar(outputlen: usize) -> i32 { } } -#[no_mangle] -#[export_name = "xOnlyPointFromScalar"] -pub extern "C" fn x_only_point_from_scalar() -> i32 { - unsafe { - let keypair = jstry!(create_keypair(PRIVATE_INPUT.as_ptr()), 0); - let mut xonly_pk = XOnlyPublicKey::new(); - let mut parity: i32 = 0; // TODO: Should we return this somehow? - assert_eq!( - secp256k1_keypair_xonly_pub(get_context(), &mut xonly_pk, &mut parity, &keypair), - 1 - ); - x_only_pubkey_serialize(&xonly_pk, X_ONLY_PUBLIC_KEY_INPUT.as_mut_ptr()); - 1 - } -} - -#[no_mangle] -#[export_name = "xOnlyPointFromPoint"] -pub extern "C" fn x_only_point_from_point(inputlen: usize) -> i32 { - unsafe { - let (xonly_pk, _parity) = jstry!( - x_only_pubkey_from_pubkey(PUBLIC_KEY_INPUT.as_ptr(), inputlen), - 0 - ); - x_only_pubkey_serialize(&xonly_pk, X_ONLY_PUBLIC_KEY_INPUT.as_mut_ptr()); - 1 - } -} - #[no_mangle] #[export_name = "pointMultiply"] pub extern "C" fn point_multiply(inputlen: usize, outputlen: usize) -> i32 { diff --git a/src_ts/index.ts b/src_ts/index.ts index d463762..add09dc 100644 --- a/src_ts/index.ts +++ b/src_ts/index.ts @@ -75,6 +75,7 @@ export function __initializeContext(): void { } export function isPoint(p: Uint8Array): boolean { + // Note: isPoint is checking for DER point, for backwards compatibility return validate.isDERPoint(p) && _isPoint(p); } @@ -159,28 +160,15 @@ export function pointFromScalar( } } -export function xOnlyPointFromScalar(d: Uint8Array): Uint8Array { - validate.validatePrivate(d); - try { - PRIVATE_KEY_INPUT.set(d); - wasm.xOnlyPointFromScalar(); - return X_ONLY_PUBLIC_KEY_INPUT.slice(0, validate.X_ONLY_PUBLIC_KEY_SIZE); - } finally { - PRIVATE_KEY_INPUT.fill(0); - X_ONLY_PUBLIC_KEY_INPUT.fill(0); - } +export function xOnlyPointFromScalar(d: Uint8Array): Uint8Array | null { + const point = pointFromScalar(d, true); + return point ? point.subarray(1, 33) : null; } export function xOnlyPointFromPoint(p: Uint8Array): Uint8Array { - validate.validatePoint(p); - try { - PUBLIC_KEY_INPUT.set(p); - wasm.xOnlyPointFromPoint(p.length); - return X_ONLY_PUBLIC_KEY_INPUT.slice(0, validate.X_ONLY_PUBLIC_KEY_SIZE); - } finally { - PUBLIC_KEY_INPUT.fill(0); - X_ONLY_PUBLIC_KEY_INPUT.fill(0); - } + // validate the point is DER and exists on the curve + if (!isPoint(p)) validate.validatePoint(/* throw */ new Uint8Array()); + return p.slice(1, 33); } export function pointMultiply( diff --git a/src_ts/wasm_loader.ts b/src_ts/wasm_loader.ts index acd44ce..d5e5166 100644 --- a/src_ts/wasm_loader.ts +++ b/src_ts/wasm_loader.ts @@ -41,8 +41,6 @@ interface Secp256k1WASM { pointAddScalar: (p: number, outputlen: number) => number; pointCompress: (p: number, outputlen: number) => number; pointFromScalar: (outputlen: number) => number; - xOnlyPointFromScalar: () => number; - xOnlyPointFromPoint: (inputLen: number) => number; xOnlyPointAddTweak: () => 1 | 0; xOnlyPointAddTweakCheck: (parity: number) => number; pointMultiply: (p: number, outputlen: number) => number; ```

junderw commented 2 years ago

I implemented the simpler version here, not sure if we should use it:

https://github.com/bitcoinjs/tiny-secp256k1/compare/feature/schnorr...feature/schnorr-simpler

Looking at the above, though. Really, the only actual changes to the API are

  1. the introduction of the x_only_pubkey concept.
  2. addition of schnorr_sign and schnorr_verify

Suggestions on naming of functions in the API etc. are welcome.

Also, I made isPoint only work for DER points and not x_only points since the rest of the API expects them, besides the new methods.

Perhaps we should just allow for all methods to accept x_only and assume the parity bit is 0 as per the BIP.

junderw commented 2 years ago

Also, some of the addtweakcheck vectors I got from pointAddScalar vectors.

If point was odd, I converted the vector by negating the point to even, then negating the tweak, then negating the result if it existed.

P + tweak*G = R

if P y is odd

P = -P (change 03 to 02) tweak = n - tweak R = -R (flip 02/03) if R not null

fanatid commented 2 years ago

LGTM