poolscript: make sure negation works with btcec v2 #363
Conversation
| secp.ScalarBaseMultNonConst(new(secp.ModNScalar).SetInt(1), &g) | ||
|
|
||
| // Get -G by negating the Y axis. | ||
| // Get -G by negating the Y axis. We normalize first, so we can negate |
There was a problem hiding this comment.
Are we doing this anywhere else across our packages? Agree that it's safer to first map to affine coordinate, and one always needs to normalize after modifying any of the field elements.
One way we can try to trigger a failure here would be using a quickcheck test which may eventually hit an edge case that causes an issue to be triggered here.
There was a problem hiding this comment.
Are we doing this anywhere else across our packages?
Not that I'm aware of. But I need to check the lnd codebase that we always to the normalize after modifying field elements.
I added some quickcheck tests and also increased the number of iterations. Both works fine with and without this diff. So I guess it can't hurt to add it anyway?
There was a problem hiding this comment.
Yeah agreed, in the future we should spin this out into methods in the btcec package to make things generally easy to use.
Something I noticed when taking a closer look at how the btcec V2 library works.
Not sure if this is really necessary or not, any thoughts, @Roasbeef?
The unit tests all passed fine without this, but maybe this could be an edge case?