bitcoinjs / ecpair

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

New testEcc tests in 2.1.0 break @noble/secp256k1 #13

Closed landabaso closed 1 year ago

landabaso commented 1 year ago

Ecpair 2.1.0 testEcc has additional new tests with code like this:

ecc.privateAdd(
        h('0000000000000000000000000000000000000000000000000000000000000001'),
        h('0000000000000000000000000000000000000000000000000000000000000000'),
      )

which breaks noble-secp256k1, because noble expects the private key > 0.

See: https://github.com/paulmillr/noble-secp256k1/blob/1.7.0/index.ts#L1024

I understand this is not strictly a bug. Or is it? Anyway, I thought about sharing this with you in case you agree on tweaking the tests to avoid 0 priv keys.

junderw commented 1 year ago

I don't see any code for privateAdd, so I assume this is a bug in the wrapper code you created/use.

privateAdd args are (privkey, scalar) so the second arg can be from 0 to curve order -1.

Please file a bug report with whoever maintains the wrapper.

edit: curve order -1, not 2^256-1

landabaso commented 1 year ago

You're right. I just did. Thanks!

landabaso commented 1 year ago

I don't want to open another issue because I somehow feel this is somewhat related.

I'm still working to make noble & ecpair work together after new versions of both libraries.

There's this other test in ecpair:

  // -3 + 3 == 0
  assert(
    ecc.privateAdd(
      h('fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd036413e'),
      h('0000000000000000000000000000000000000000000000000000000000000003'),
    ) === null,
  );

I end up having a point that is zero but not a null.

Does it need to return null because this operation is invalid (& noble should throw)?

EDIT: Yeah, I guess this is the reason. I updated my wrapper to validate returned values from noble-ecc and everything runs smooth.

junderw commented 1 year ago

Yes. The output of privateAdd is a privateKey. And 0 is an invalid privateKey which should return null.