Do real secp256k1 point->curve checking

[brandonblack]

• This is a breaking change, as it requires the JS environment to have BigInt (all supported releases that I can find do).
• This check may prevent loss of funds by eliminating a category of unspendable addresses from being created.
• Performance is almost as fast as tiny-secp256k1 39-42us vs 33-35us.

[brandonblack]

A note on performance: tiny-secp256k1 builds a full point to check the curve equation, which requires performing a modular square root. This check avoids performing the square root by using the Jacobi Symbol to check that it can be done, which is sufficient to say that the Y coordinate exists, and therefore the X coordinate is on the curve.

[brandonblack]

Also of note: I confirmed the accuracy of this function by comparing it to tiny-secp256k1.isXOnlyPoint over > 1 million random 32-byte arrays.

[junderw]

1. Needs careful consideration.
2. Needs tests.

[junderw]

3. If we're going to use bigint, a notable addition that should also be considered is the satoshi unit. This would also be a breaking change, but allow for forks of bitcoin that have extremely high amounts (ie. doge) to be handled with just a network object like litecoin.

[brandonblack]

1. Agreed, figured opening a proper top-level PR was the place for that consideration, once I found I was able to bring the performance in line with what is proposed for point checking in the Taproot branch.
2. Done -- LMK if there are other particular cases you'd like to see covered.
3. You know that we at BitGo wouldn't complain about making Doge support easier :-D

[junderw]

Also, this exposes the methods from the public API via crypto. (we export crypto directly as is)

I don't want to export these. (which is why types is not exported)

[brandonblack]

Would you like them in a new file, in types, or another file?

…

On Wed, Mar 23, 2022, 21:36 Jonathan Underwood ***@***.***> wrote: Also, this exposes the methods from the public API via crypto. (we export crypto directly as is) I don't want to export these. (which is why types is not exported) — Reply to this email directly, view it on GitHub <#1786 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AVTUYXDF7BOVORO7NEFLZJDVBPWOPANCNFSM5RPIU6AQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[junderw]

In types is fine.

[motorina0]

NACK on:

This is a breaking change, as it requires the JS environment to have BigInt

• this change will make bitcoinjs-lib unusable as-is in some environments (I'm thinking of React Native on mobile)
  • disclaimer: I work at Exodus, this would be indeed a breaking change :(

[motorina0]

This check may prevent loss of funds by eliminating a category of unspendable addresses from being created.

• I totally agree with this stricter check, the question is only if this logic should be part of bitcoinjs-lib
• the hole point of having TinySecp256k1Interface was to have the ECC logic (imported lib) decoupled from this lib and to allow the users to provide their own implementation

My Suggestion:

• add isPoint() to TinySecp256k1Interface
  • note: the current types.isPoint() is used in all payments (p2pk, p2pkh, p2wpkh ...)
• once we have initEccLib in place:
  • the payments will use the isPoint() of the eccLib (if provided)
  • or will default to the existing (less strict) isPoint() otherwise
• this way the user of the lib can enforce stricter checks and there is no backwards compability issue

[brandonblack]

this change will make bitcoinjs-lib unusable as-is in some environments (I'm thinking of React Native on mobile)

Interesting, what version of which JS engine is that?

IIUC React Native uses JavaScriptCore, which appears to have BigInt since Safari iOS 14, which was released 1.5 years ago. Looks like the very last version that didn't have BigInt was released with iOS 13.4.1 in April, 2020 and is not supported with security updates. Ah, but iOS 12 had its last release as recently as September 2021.

I see that Exodus supports iOS 12+. OK.

[brandonblack]

once we have initEccLib in place

I'm still trying to avoid that in favor of passing just tweakFn to p2tr when it's needed. In particular, my next goal after Taproot is merged is to work on MuSig support, which will be best accomplished by passing the MuSig tweak function, specific to the aggregate key being tweaked, in order to properly record the parity for signing. It is possible to derive the necessary parity from the parity+x of the aggregate key and the parity+x of the tweaked key, but involves working backward.

Because the MuSig tweak function will be per-input, a cached eccLib is an impediment.

[junderw]

this change will make bitcoinjs-lib unusable as-is in some environments (I'm thinking of React Native on mobile)

Interesting, what version of which JS engine is that?

React: aka FacebookJS, aka ES5-or-bust

[brandonblack]

And now that I've spelunked into the odd world of mobile device support cycles, I see that a version of iOS that didn't have BigInt in its JS engine was released as recently as September, 2021, and so using BigInt in bitcoinjs-lib should probably wait until that's aged out a bit more. 😢

[junderw]

Not sure if I want to ask for a rebase (we merged taproot yay!!!) or just close this...

If we're gonna jump to BigInt requirements, might as well compile everything to WASM from Rust and rewrite the entire library in Rust! (can you tell I've become a Rust evangelist since the beginning of this year??? lol)

[paulmillr]

React Native supported bigints since august. It doesn't support bigint literals 42n, so this PR is ok.

@brandonblack is jacobi symbol faster than sqrt bit-by-bit unroll?

[junderw]

I noticed RN support for BigInt recently when I was checking for support when merging noble into bip32.

It's been almost half a year, but notably BlueWallet (one of our biggest/most popular dependents) is on an old version of RN that doesn't support it.

Once BlueWallet supports BigInt, I'd support merging this, in addition to switching the concept of "amount" (currently type number) to use bigint type. (Would have to change some of the runtime type checks to check for 0 <= x < 2**63.)

re: @Overtorment mentioned needing to bump to RN70 in October.

BlueWallet/BlueWallet#5073 (comment)

[Overtorment]

Can confirm, BlueWallet is working on bumping RN to 70+. Not easy as we have some native dependencies lagging behind/unmaintained, so we are looking into workarounds

cc @marcosrdz

[Overtorment]

btw BW migrated to RN70+ and theoretically has BigInt now