paulmillr / noble-secp256k1

Fastest 4KB JS implementation of secp256k1 signatures and ECDH
https://paulmillr.com/noble
MIT License
757 stars 114 forks source link

1.7.1: `Point.add` is not throwing for "1 + -1 == 0/Infinity" #91

Closed landabaso closed 1 year ago

landabaso commented 1 year ago

I'm the developer of https://github.com/bitcoinerlab/secp256k1.

I just released it on npm and then noticed there was a new 1.7.1 release, so I decided to upgrade. However, there is now a test on my lib that is failing that was previously passing with version 1.7.0.

I am running a large number of tests that I took from https://github.com/bitcoinjs/tiny-secp256k1. The specific test that is failing is shown below:

{
  "description": "1 + -1 == 0/Infinity",
  "P": "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798",
  "Q": "0379be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798",
  "expected": null
},

I've isolated the problem and prepared a minimum example on RunKit for you to verify/test.

You can edit it to switch between versions 1.7.0 and 1.7.1 (and click on the green icon to refresh) to see the difference:

https://runkit.com/landabaso/63bc3f88bd43770008177bfd

I believe the behavior in 1.7.0 is the correct one.

paulmillr commented 1 year ago

I believe the behavior in 1.7.0 is the correct one.

Why?

paulmillr commented 1 year ago

I suggest to update your definition of tweak / publicAdd functions. They need to do the validation. P - P is valid point 0. It may not be valid in your context, but that's not applicable to everyone.

landabaso commented 1 year ago

I suggest to update your definition of tweak / publicAdd functions. They need to do the validation. P - P is valid point 0. It may not be valid in your context, but that's not applicable to everyone.

Thanks. I catch this case on my side now.

Why

Previous behavior up to version 1.7.0 did not return a point, which matched the behavior in tiny-secp256k1. I simply assumed that this was the typical output for secp256k1 libraries and that this minor breaking change had not been intentional.