Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BigInt syntax errors in unsupported environments #12

Closed
jacogr opened this issue Nov 25, 2021 · 19 comments
Closed

BigInt syntax errors in unsupported environments #12

jacogr opened this issue Nov 25, 2021 · 19 comments
Labels
enhancement New feature or request

Comments

@jacogr
Copy link
Contributor

jacogr commented Nov 25, 2021

... you are not going to like this one, bear with me :)

Ok, I started pulling this in. Basically in my case I still need to support some ancient environments as well, which is not that much of an issue, basically on my hashing wrappers I do the following -

import { blake2b as blake2bJs } from '@noble/hashes/lib/blake2b';

// wasm
import { blake2b as blake2bWasm } from '@polkadot/wasm-crypto'; 

export function blake2 (u8a: Uint8Array): Uint8Array {
  return typeof BigInt !== 'undefined'
    ? blake2bJs(u8a)
    : blake2bWasm(u8a);
}

So effectively there is some fallback in there. (The above is typed from memory as illustration, but shows the point. Normally when wasm is initialized, it actually is the preferred route in my case. The same applies to eg fallback to blake2-js when BigInt is not available)

Anyway, all good. Now however the issue is the following - on some older environments, such as for instance React Native, it even throws up when trying to parse the modules, e.g. this is problematic (although not used at runtime) https://github.com/paulmillr/noble-hashes/blob/main/src/_u64.ts#L1

I did a quick search for the 32n e.g. /\dn/ and there are actually not that many. So for instance, we could get around it with the following ...

// _64.ts 
// suggested adjustments as an example, some files will need adjusting 
// yes, it is, well ... horrid :(
export const _BigInt: (value: string | number | bigint | boolean) => bigint =
  typeof BigInt !== 'undefined'
    ? BigInt
    : () => undefined as unknown as bigint;

const U32_MASK64 = _BigInt(2 ** 32 - 1);

const _32n = _BigInt(32);

export function fromBig(n: bigint, le = false) {
  if (le) return { h: Number(n & U32_MASK64), l: Number((n >> _32n) & U32_MASK64) };
  return { h: Number((n >> _32n) & U32_MASK64) | 0, l: Number(n & U32_MASK64) | 0 };
}

The above is problematic, obviously since it will give undefined results in non-BigInt environments. In BigInt environments there is no extra overhead, well, maybe slightly at library load. However for non-BigInt-envs inside functions it does use BigInt which would throw up at runtime.

At this point the TL;DR is -

  1. I'm making sure that at runtime, the BigInt libs are not used (if not available)
  2. I still have support issue where library users now complain that it doesn't compile in their environments

So my options are -

  1. Having to debug all user environments to figure out why
  2. Documenting all of it
  3. Making a PR here with "some" hacks like above

None of the options are fun atm :) Other suggestions welcome.

@jacogr
Copy link
Contributor Author

jacogr commented Nov 25, 2021

EDIT: I actually have this issue as it stands in the current version of my libs, i.e. had this (it is not used anywhere, only declared in a file that has a function exported) -

const SQRT_MAX_SAFE_INTEGER = 94906265n;

Took a similar "hack" approach now as laid out above (to not spend all day on issues) to get around it with a _BigInt(94906265) definition where it is defined. At runtime, if called and BigInt is not available, the function will fail since BigInt(<some-number>) is used internally to it.

@jacogr
Copy link
Contributor Author

jacogr commented Nov 25, 2021

Made a PR here since I had a small gap and always prefer seeing things in code - #13

However, feel free to close this and that after some thought it is absolutely horrid in your eyes, it is just an approach.

@paulmillr
Copy link
Owner

If there are no bigints, we should absolutely NOT fall back to something else, and instead an error should be thrown.

It's like falling back to "math random" when crypto.secureRandom is not available. Very bad idea.

@jacogr
Copy link
Contributor Author

jacogr commented Nov 25, 2021

Agreed. So in this case, there is detection -

return typeof BigInt !== 'undefined'
  ? somethingWithBi(...)
  : somethingWithNoBi(...)

The issue is the build steps - it pulls in all code (even unused libs) and then fails on things like 32n.

So there are def. 2 parts -

  • ensuring that old env can actually import the code (the issue is that this cannot be really done at build time with stuff like export maps)
  • ensuring that at runtime the sane things is done (here not sure if that means sprinkling in asserts top-level)

@paulmillr
Copy link
Owner

when wasm is initialized, it actually is the preferred route in my case

Can I ask, why? WASM:

  1. Cannot be verified. You cannot be sure you're not running a malware. The binaries are unsigned. You're downloading them from NPM which was allowing anyone to upload a new version of any package without auth for a few years.
  2. Eats more storage / network than JS
  3. Is in some cases faster, but it's very rare that you need huge hashing speed in JS apps. Huge meaning 1M ops/sec instead of 200k ops/sec. Not instead of 2k ops/sec.

ensuring that old env can actually import the code

If the old env is able to import the code, but then fails immediately, what's the point here? I think the environments must update to be compliant with modern ECMAScript.

Leaving bigint code as-is would increase the pressure on build tools to be compliant. E.g. if we merge the pull request, less people would know about the problem, and less people would report to react-native.

Anyway i'll leave the PR open for now until we get more environment examples. If enough people report this, we'll prob go ahead. Note that it's not just noble-hashes that needs to be adjusted, it's also all other noble libraries.

@paulmillr
Copy link
Owner

Another question. How would your env handle an error that would be thrown if the bigints do not exist, during the runtime Can the error be catched?

@paulmillr paulmillr changed the title Pre-historic parsing BigInt syntax errors in unsupported environments Nov 25, 2021
@paulmillr paulmillr added the enhancement New feature or request label Nov 25, 2021
@paulmillr
Copy link
Owner

Also, it seems like the most recent react-native supports bigints. The problem only resides in devices with ios 13.7 or earlier. (current is v15)

@jacogr
Copy link
Contributor Author

jacogr commented Nov 25, 2021

100% - however, I'm already this week flooded with support and all I did was adding unused bi utils :) The plan is to swap to noble in the next major release (which I want to do soon), however I cannot force everybody to upgrade their environments, but a lot of times I do have to force them to upgrade my libs and if the two don't quite work as expected, great unhappiness.

(In the case of RN, according to the Ledger Live developers, it may be solved by an upgrade from Expo to Hermes, but it is not a small undertaking. Current version of my libs, even before @noble/hashes integration is an issue for this team as it stands, so have applied the same kind of to-be-released fixes for the bigint utils included there... it is what I could come up with...)

So I will point you to actual (in master branch) code using @noble/hashes -

https://github.com/polkadot-js/common/blob/master/packages/util-crypto/src/blake2/asU8a.ts#L33-L35

The Rust libs used for WASM is part of the audit for the Polkadot runtime, i.e. use the same versions as there. So yes, I agree, it is not readable, e.g. https://github.com/polkadot-js/wasm/tree/master/packages/wasm-crypto/src/rs and it brings a host of other issues with it (i.e. for React Native it also needs asm.js since it doesn't do Webassembly... but at least in this case it now provides a fallback for that env)

So, in my case, I guess for the time being I'll revert @noble/hashes (or include it with these changes as a fork), since all hell will break loose in my life... it already has :)

Sadly I deal with a lot of different environments using the libs and, well, I would love to tell them to just not use it, but the ecosystem has no other options apart from what is provided by my libs - so that needs to cater for a wide spectrum. I am trying my best to drive forward bit by bit (and not get held back).

As for other libs, I am aware of it, i.e. the @noble/secp256k1 I have a PR for, but it is very problematic - since I don't have a WASM fallback, so it really is all-or-nothing. Luckily it is only a small part, i.e. most keys are sr25519 (https://github.com/polkadot-js/wasm/blob/master/packages/wasm-crypto/src/rs/sr25519.rs) or ed25519 (tweetnacl, I have not attempted @noble/ed25519 as of yet)

@paulmillr
Copy link
Owner

So, if we merge the BigInt changes to secp and hashes, would that help your users with old React Natives? Does RN support BigInts, just not syntax?

@jacogr
Copy link
Contributor Author

jacogr commented Nov 26, 2021

Yes, it would allow me to swap over completely. On some environments, e.g. iOS, will have 100% support - on some older RN the fallbacks would be used. However, it does mean it can be available for 99% of end-users with no issues (i.e. have JS hashing available, the remainder is on fallback) and available for 100% of all library users with no issues in their tooling.

In the end it needs to make 100% sense here, if not it should not go in.

Sometimes I wish it was 2025 already and 2020-era new is everywhere... :)

@paulmillr
Copy link
Owner

The question is still in the error thrown on import. Would you be able to handle it?

@jacogr
Copy link
Contributor Author

jacogr commented Nov 26, 2021

On use of a non-compliant function, indeed. An example of this is the sr25519 pair functions, they are only available in WASM (or asm.js under RN - only included in this environment, too big otherwise). If there is no WASM support, they do fail and it is up to the caller to handle.

So on my side (others may do it differently), each hashing function has a check for typeof BigInt !== 'undefined' to determine the path to follow. If it then calls into the BigInt-using function and that does fail for whatever reason, it won't use another fallback attempt, rather it is up to the library user to sort out. So basically need to ensure I do the BigInt-available check before calling into anything.

(secp, when I get to it, will be different, with no BigInt it will just fail outright without even trying to call into @noble/secp256k1. In my case it is a tradeoff than can be made since 99.9% of keys are all ed25519 or sr25519)

On import itself, I don't have lazy imports, so if I do -

import { blake2b } from '@noble/hashes/lib/blake2b';

and that fails, there is an issue. However when calling into blake2b above, it is fine since I better make sure the env can handle it.

Does that make sense?

@paulmillr
Copy link
Owner

I'm thinking about throwing an error on import. What users of yours would be affected by it?

@jacogr
Copy link
Contributor Author

jacogr commented Nov 26, 2021

Really not 100%. Will need to see what it looks like code-wise at the end to determine impact.

It is probably mostly my RN users. It is more tricky to determine capability paths (for me downstream) at import. Obviously at runtime things like this (random example) would work without issues -

class BLAKE2b extends blake2.BLAKE2<BLAKE2b> {
  // Same as SHA-512, but LE
  // ... snipped ...
  constructor(opts: blake2.BlakeOpts = {}) {
    if (typeof BigInt ==='undefined') throw new Error('BigInt is not available');

But it does have a slight runtime impact.

At file level it would be trickier (for me), but I'm guessing it is the 100% clear solution here for this.

@paulmillr
Copy link
Owner

yeah, I don't want runtime impacts for stuff that's used by 1% of users. I don't see big disadvantages in changing 1n to BigInt(1) tho.

@aulneau
Copy link
Sponsor Contributor

aulneau commented Dec 3, 2021

Just as a side note, this is something i have also ran into trying to support react-native usage of my library micro-stacks. the n helper (while not the nicest to look at), would make a huge impact in being able to support more environments. I spent some time trying to build out a babel plugin that would transform native bigint usage to something like jsbi, but with a change like this in both the hashes and secp256k1 it would be much more realistic.

@paulmillr
Copy link
Owner

Thanks @jacogr, i've added this, but without _n util. You'll need to replace BigInt on your own in your app.

@jacogr
Copy link
Contributor Author

jacogr commented Dec 3, 2021

Thank you. I think that is a sane approach.

@jacogr
Copy link
Contributor Author

jacogr commented Dec 3, 2021

Edit to the above: I have something similar for secp256k1 (in this case also BigInt(…) wrappers), will contribute… once again if somebody doesn’t get there before me…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants