bitcoinjs / ecpair

The ECPair module for bitcoinjs-lib
MIT License
15 stars 22 forks source link

Minor fixes for compatibility #11

Closed brandonblack closed 1 year ago

landabaso commented 1 year ago

You might want to take a look at the discussions below about removing cases with tweak = 0 in the tests in this PR:

junderw commented 1 year ago
  1. I don't understand the bech32 change.
  2. 1 is a valid private key. 0 is not. However, privateAdd args are (privateKey, scalar)... and scalar is [0, curve.n) so the only difference between private keys and scalars is the inclusion of 0.

This test was put there to test that privateAdd actually uses scalars for the second position, and it's doing its job.

brandonblack commented 1 year ago

Thanks @junderw and @landabaso!

I don't understand the bech32 change.

Many forkcoins don't have a bech32 prefix. So, when using bitcoinjs libraries to support forkcoins, the network parameter will often not have a meaningful value for the bech32 member. I think we have this change made to our fork of bitcoinjs-lib as well, but haven't upstreamed it yet.

paulmillr commented 1 year ago

@junderw

the only difference between private keys and scalars is the inclusion of 0

Private key is scalar. What's the difference? Why would you allow 0 in privateKeyAdd, if it enables non-contributing behavior?

paulmillr commented 1 year ago

for example, it was said curve25519 keys should not be validated, however, "zero" keys enabled some bad behavior: https://research.kudelskisecurity.com/2017/04/25/should-ecdh-keys-be-validated/

junderw commented 1 year ago

Private key is scalar. What's the difference? Why would you allow 0 in privateKeyAdd, if it enables non-contributing behavior?

In the bitcoin secp256k1 library, the secp256k1_ec_privkey_tweak_add method internally uses secp256k1_scalar to represent both seckey and tweak32... but only seckey is eventually checked against 0, both on input and right before output (it is modified in-place)

Whereas tweak32 is only compared as being less than the order of the curve N.

If we define TypeA as being a type that represents in bytes secp256k1_scalar that is validated to be in the range [1, N), and TypeB as being a type that represents in bytes secp256k1_scalar that is [0, N)

The call signature for this function is privateAdd(TypeA, TypeB): TypeA.

The goal of the tiny-secp256k1 interface is to be in line with the secp256k1 library, but with a smaller interface than node-secp256k1, and the behavior of these functions was designed to match up with libsecp256k1.

https://github.com/bitcoin-core/secp256k1/blob/6a873cc4a97cccf234b019c064386c82f2df2111/src/tests.c#L5623-L5625

paulmillr commented 1 year ago

If the goal is to match btc libsecp api, that makes sense.

junderw commented 1 year ago

Since the bech32 change is for non-Bitcoin assets, I will say no.

And the privateAdd part has been explained so I will close this.

Thanks for the PR.