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 support #510

Closed
likern opened this issue May 8, 2021 · 37 comments
Closed

BigInt support #510

likern opened this issue May 8, 2021 · 37 comments
Labels
enhancement New feature or request

Comments

@likern
Copy link

likern commented May 8, 2021

Problem

Hello βœ‹πŸ» I've looked at documentation and source code and couldn't find BigInt type.
@tmikov Is it possible to add BigInt support?

@likern likern added the enhancement New feature or request label May 8, 2021
@tmikov
Copy link
Contributor

tmikov commented May 8, 2021

@likern Yes, it is technically possible, though non-trivial. Unfortunately it is not on our short term roadmap since BigInt isn't used internally by our apps :-(

@ehsan6sha
Copy link

Unfortunately it is not on our short term roadmap sin

Can I ask what is used instead for internal apps? Or for internal apps there was no need for larger than nomral numbers?

@jzxchiang1
Copy link

Bump, this is quite important for all crypto-related apps, which is the future anyways.

@jzxchiang1
Copy link

Another friendly bump.

Facebook's recent rebranding to Meta underscores how important crypto-native mobile apps are and will continue to be. BigInt is an essential component of all cryptographic libraries, and mobile apps that create and validate signatures will need this support.

Flutter already has BigInt support in Dart. Given that React Native is actively maintained by Meta, I assume they will need BigInt support in RN sooner rather than later. (Maybe not now but very soon...)

Right now, the workaround is to use JSC (limited to only iOS 14 and doesn't work on Android) or v8 (Android only). It's not ideal. Especially if Hermes is becoming the default JS engine.

@mrousavy
Copy link
Contributor

mrousavy commented Dec 9, 2021

Hey! BigInt support is something I'm dealing with right now. I am using JS-based libraries such as BigNumber, but those aren't standardized and not very solid (in my opinion).

I've played around with trying to expose BigInt functionality through JSI to use C++ based big-number types, as well as using the JSBI polyfill - none of those support the BigInt literal notation (with the n suffix) though.

Would be great to have that baked into Hermes, I would love to have a shot at this myself if you can give me any pointers with relevant lines of code in this repo cc @tmikov

@tmikov
Copy link
Contributor

tmikov commented Dec 10, 2021

@mrousavy adding BigInt would touch a lot of components:

  • Parser support for BigInt literals.
  • BigInt literals and instructions have to be added to the IR and the type system (for type inference).
  • The bytecode file format has to be extended with a bigint table and instructions to load from it.
  • HermesValue has to be extended with a new type tag and new "first-class" VM object has to be added. This is assuming that BigInt-s will always be kept on the heap. If small ones will be kept inline, it gets even more complicated.
  • The interpreter has to support the new instructions.
  • All internal operations have to support the new type (like add, etc).
  • It is not totally clear whether interpreter fast paths will be affected. Presumably if BigInt-s are kept only on the heap, they will be relatively slow so probably should be kept out of the fast paths.
  • Lastly, JSLib (the standard JS library) has to extended.

Overall, I would say that it is a pretty involved project, but is certainly doable.

I am not actively working on Hermes anymore (I am currently focused on Juno), so I will let members of the Hermes team chime in here. cc: @neildhar

@neildhar
Copy link
Contributor

Unfortunately, implementing BigInt is still not on our immediate roadmap.

@jzxchiang1
Copy link

If it's possible to bring up BigInt with the team and possibly prioritize it, that would be very much appreciated.

@likern
Copy link
Author

likern commented Jan 16, 2022

I want to point out that coming Temporal proposal (as I understand) is also using bigint, at least in polyfill.

@jzxchiang1
Copy link

jzxchiang1 commented Jan 31, 2022

Here's another reason to support BigInt sooner rather than later:

The widely used web3.js library is rewriting their library using modern TS and also replacing their bignumber.js dependency with native BigInt. This will be released in v4 soon (I believe this year, please correct me if I'm wrong). They are perhaps the most popular client-side library for connecting to Ethereum nodes.

@jzxchiang1
Copy link

Is there any update on this? Thanks.

@krpeacock
Copy link

krpeacock commented May 2, 2022

We have many heard complaints about the lack of bigint, and may have to refactor away from the bigint primitive in our crypto packages because of this lack of support. Meta plans to be a crypto company - it seems fair that anyone who needs to manage satoshis, gwei, or other high-precision currencies should be able to do so using a native language feature that has been out since ES2020

@jpporto
Copy link
Contributor

jpporto commented May 2, 2022

Hi everyone,

First of all, apologies for the delay in providing updates. Second, I am happy to say that we have been working on BigInts internally :). There should be some news around this soon! Please bear with us a little longer...

John Paul

@jzxchiang1
Copy link

Looking forward to it.

@jzxchiang1
Copy link

@jpporto Any news on this?

@jpporto
Copy link
Contributor

jpporto commented Jun 14, 2022

Hi @jzxchiang1,

We're still (actively) implementing BigInts. We're currently trying to minimize the overall impact of adding BigInt to the VM. It is our goal to ensure that adding BigInt doesn't cause any meaningful performance regressions in non-BigInt usage (e.g., Numbers).

Please bear with us a little longer.
John Paul

@brunomacf
Copy link

Any updates on this? Anxious to have BigInt in hermes in order to allow crypto packages to work correctly in react native.

@jpporto
Copy link
Contributor

jpporto commented Jun 23, 2022

Hi,

We're working to minimize the impact adding BigInt has in the VM. I want to point out, however, that js BigInts are not suitable to crypto workloads. Your best option is to use a native crypto library, add js bindings, and call those from your JS app.

John Paul

@jzxchiang1
Copy link

I hate to ask this, but do you have any idea of a timeline for when BigInt will be launched?

@jpporto
Copy link
Contributor

jpporto commented Jun 28, 2022

I wish I had something more specific to say, but at this point I don't have any ETAs on when this support will land. :-(

@jpporto
Copy link
Contributor

jpporto commented Jul 8, 2022

Hi,

We have started the process of merging BigInt to hermes! It is a relatively large change (about 50 diffs), so it will be a while before everything makes to github. Hopefully everything will be ready by next week. :)

Thank you for your patience!

@jpporto
Copy link
Contributor

jpporto commented Jul 11, 2022

Good morning,

All commits have been merged, and BigInt should be available on hermes starting in aa83f8bd6f536d4d0fc45fa092eecb360230dc7f. Here are the pre-built cli binaries for linux(sha256), macos(sha256), and windows(sha256).

I'll close this issue as the feature is now implemented. Please report any problems in new issues.

John Paul

@jpporto jpporto closed this as completed Jul 11, 2022
@jzxchiang1
Copy link

Incredible thank you. Will it be released with the next version of Hermes/RN?

@kelset
Copy link

kelset commented Jul 12, 2022

@jzxchiang1 it will be present with RN 0.70

@paulmillr
Copy link

paulmillr commented Jul 19, 2022

For anyone's reading @jpporto 's comment that "bigints are unsuitable for crypto":

We're working to minimize the impact adding BigInt has in the VM. I want to point out, however, that js BigInts are not suitable to crypto workloads. Your best option is to use a native crypto library, add js bindings, and call those from your JS app.

The advice is uninformed.

First of all, using native bindings does not guarantee any resistance against timing attacks. Because, JS runtime would still be there. JS is garbage-collected language. And there are no guarantees your uint8arrays or any other data structure would be cleaned properly at some point. If you want 100% constant-time code, your best bet is using low-level languages directly, but even they are not the 100% solution: LLVM & other compilers could "optimize out" constant timeness. Also, you will need to not use branches (if differing conditions take different time to execute, you're in trouble). Also you will need to ensure secret data is not used as array indexes. Don't forget about division: It takes variable time on some cpus.

Second, "Using a native crypto library" means users would need to run unsigned wasm binaries that may as well contain malware & trojans. Using pure javascript code allows every JS developer to verify the js code is indeed the one published on GitHub. Compiling C / Rust code to wasm produces huge binaries, commonly without using reproducible builds. The binaries may, or may not contain some dangerous garbage.

NPM got hacked. GitHub got hacked. Developer accounts can and will get hacked. Binaries will be replaced with malware without account owner's knowledge. So using bigints js library is not worse than a bytebuffer-based native solution. I think, it's better.

In fact, i've developed some bigint crypto libraries, which already received security audits.

@jpporto
Copy link
Contributor

jpporto commented Jul 19, 2022

Hi Paul,

Thank you for your feedback. I just wanted to point out that that's not my advice. Rather, I just quoted the JS spec, ipsis litteris.

@jpporto jpporto closed this as not planned Won't fix, can't repro, duplicate, stale Jul 19, 2022
@shea256
Copy link

shea256 commented Jul 19, 2022

@jpporto can you link to where it says that?

@ljharb
Copy link

ljharb commented Jul 19, 2022

to clarify: MDN is not the spec, and not authoritative in any way.

@shea256
Copy link

shea256 commented Jul 19, 2022

All I see is the MDN page (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#cryptography) which says the following:

Cryptography
The operations supported on BigInt values are not constant-time, and are thus open to timing attacks. JavaScript BigInts are therefore not well-suited for use in cryptography.

This statement has limited scope and isn’t relevant to the debate at hand, plus as @ljharb mentioned this is not authoritative.

@jpporto
Copy link
Contributor

jpporto commented Jul 19, 2022

(Apologies for the MDN vs. spec confusion. My bad.)

I should have started by saying I am no crypto expert. Far from it, I have no technical knowledge about the area as I prefer working on compilers and runtimes.

While ecma262 doesn't mention crypto, the bigint proposal (which is arguably what I should have quoted before) does:

The operations supported on BigInts are not constant time. BigInt is therefore unsuitable for use in cryptography.

I strongly believe in being transparent, so after having told krpeacock that BigInts were coming after he specifically asked about the feature in the crypto context, I deemed prudent to come on the record with this pitfall. This doesn't mean I am telling anyone not to use BigInts for crypto -- I added the feature for the community to use it after all! :) --, just for those who do, to do so carefully.

@landabaso
Copy link

landabaso commented Jul 20, 2022

it will be present with RN 0.70

I just read react-native blog post about 0.70 and Hermes integration and they say that BigInt support is "Ongoing work".

I'm not 100% sure what that means. @kelset do you know if it finally made it (to the 0.70 rc)?

@kelset
Copy link

kelset commented Jul 20, 2022

as far as I can tell, in 0.70 we have the BigInt commits - so it should work.

@jzxchiang1
Copy link

Thanks for getting this over the finish line. Although it may not seem like it, this is a big deal.

@roryabraham
Copy link

Hi πŸ‘‹

Quick question: Does the new BigInt implementation in Hermes support the 12345n syntax, or just the BigInt(12345) syntax?

@jpporto
Copy link
Contributor

jpporto commented Aug 5, 2022

Hi,

Hermes supports bigint literals, so you should be able to use 12345n. :)

John Paul

@paulmillr
Copy link

I think you have some bugs with regards to bigint exponentiation 123n ** 256n. Has this been tested thoroughly?

@jpporto
Copy link
Contributor

jpporto commented Aug 5, 2022

Hi,

I am not sure if the testing that's been done would be considered extensively by you, so I'll refrain from commenting on that. However, I don't see any issues with 123n ** 456n (hermes and v8 agree on what the result). If you have an issue, please report it on a new bug. I'll lock this one as this thread is too old, too long, and possibly include way too many people.

John Paul

@facebook facebook locked as resolved and limited conversation to collaborators Aug 5, 2022
ChALkeR referenced this issue in tmikov/hermes Nov 9, 2024
Summary:
The BigInt exponentiation code currently truncates the first digit of a
BigInt from 64 to 32 bits. This means that for a single digit BigInt
exponent that has a value larger than `UINT32_MAX`, we end up
truncating the exponent and producing an incorrect value. To fix this,
do not truncate the exponent until after we have guaranteed that it
will fit in 32 bits.

Reviewed By: fbmal7

Differential Revision: D47275000

fbshipit-source-id: 881b7653d0394b88aad9db11fb6f99e675a7e38f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests