VM: Replace BN.js with bigints, Util helpers

[acolytec3]

Replaces BN.js with bigints across the VM and updated compilation targets for VM and util to ES2020
Implementation details:

• Replaces BN.js with bigints in all calculations across all VM internals (evm/StateManager/etc.).
• All VM exposed APIs now require bigint inputs instead of BN.js and numeric outputs in execResult and similar VM outputs are passed as bigints instead of BNs
• Adds new helpers in util to convert BNs to bigints, buffers, and vice-versa.
• Upgrades libraries to target ES2020 to support bigint usage
• All BNs are currently converted to bigints when passed from other packages into VM API (e.g. from tx and block objects in the State Manager)

[ryanio]

cool, could we just do 2n ** 256n here?

[holgerd77]

Hmm, is this wise if this always have to be recalculated? 🤔

[ryanio]

that's a good question, if the calculation is run every time the constant is imported or if it is calculated once on init of the package.

To test:

constants.js

const TWO_POW256_BIGINT_HEX =
  BigInt(0x10000000000000000000000000000000000000000000000000000000000000000);

const TWO_POW256_BIGINT_EXP = 2n ** 256n;

exports = [TWO_POW256_BIGINT_HEX, TWO_POW256_BIGINT_EXP];

index.js

const Benchmark = require("benchmark");
const BN = require("bn.js");

const suite = new Benchmark.Suite();

suite
  .add("bigint init with hex", function () {
    const { TWO_POW256_BIGINT_HEX } = require("./constants");
    if (100n > TWO_POW256_BIGINT_HEX) return true;
  })
  .add("bigint init with exp", function () {
    const { TWO_POW256_BIGINT_EXP } = require("./constants");
    if (100n > TWO_POW256_BIGINT_EXP) return true;
  })
  .on("cycle", function (event) {
    console.log(String(event.target));
  })
  .on("complete", function () {
    console.log("Fastest is " + this.filter("fastest").map("name"));
  })
  .run({ async: true });

On my Macbook Air (M1, 2020) my results are always about even:

bigint init with hex x 6,561,811 ops/sec ±0.37% (97 runs sampled)
bigint init with exp x 6,573,137 ops/sec ±0.10% (99 runs sampled)
Fastest is bigint init with exp,bigint init with hex

I'm not quite sure if this exactly replicates our util setup, but thought I'd try it.

Let's compare this to inline definitions and not importing (snippet):

const TWO_POW256_BIGINT_HEX =
  BigInt(0x10000000000000000000000000000000000000000000000000000000000000000);
const TWO_POW256_BIGINT_EXP = 2n ** 256n;

suite
  .add("bigint init with hex", function () {
    if (100n > TWO_POW256_BIGINT_HEX) return true;
  })
  .add("bigint init with exp", function () {
    if (100n > TWO_POW256_BIGINT_EXP) return true;
  })

Much faster 😄

bigint init with hex x 110,702,702 ops/sec ±0.12% (101 runs sampled)
bigint init with exp x 110,732,705 ops/sec ±0.12% (94 runs sampled)
Fastest is bigint init with exp,bigint init with hex

One last comparison, with requiring but only once in the file header (snippet):

const { TWO_POW256_BIGINT_HEX, TWO_POW256_BIGINT_EXP } = require("./constants");

suite
  .add("bigint init with hex", function () {
    if (100n > TWO_POW256_BIGINT_HEX) return true;
  })
  .add("bigint init with exp", function () {
    if (100n > TWO_POW256_BIGINT_EXP) return true;
  })

Results:

bigint init with hex x 85,243,817 ops/sec ±0.41% (90 runs sampled)
bigint init with exp x 86,028,773 ops/sec ±0.19% (97 runs sampled)
Fastest is bigint init with exp

[acolytec3]

One thing that is definitely broken is the EXP function handler. It seems to work in most instances but in some cases it's producing values out of range. Since I don't really understand the math that bn.js uses to do exponentiation, I can't comment on how the bigint version is different or why it is producing incorrect results for some state tests like some of these VMTests/vmArithmeticTest/exp.json

[ryanio]

I was curious about BigInt conversion performance with the varying ways to do it, so I put together a small benchmarking setup, my results in the first comment: https://gist.github.com/ryanio/89cc842a7573dd077cbb7cfa4469f8ec

[jochem-brouwer]

Some small stuff I noticed, which we should look at (will edit this if I find more)

• npm i --workspaces fails on build
• Browser build are failing on some packages
• Coding style should be consistent, for instance gas = gas + 2n and gas += 2n are used mixed

[codecov]

Codecov Report

Merging #1671 (1ddcc23) into develop (3627197) will increase coverage by 0.73%.
The diff coverage is 92.70%.

Flag	Coverage Δ
block	85.33% <ø> (+0.31%)	⬆️
blockchain	83.55% <ø> (+0.26%)	⬆️
client	72.76% <33.33%> (+1.63%)	⬆️
common	93.40% <ø> (+0.28%)	⬆️
devp2p	82.90% <ø> (+0.39%)	⬆️
ethash	90.76% <ø> (ø)
rlp	?
trie	86.53% <ø> (+0.34%)	⬆️
tx	90.97% <ø> (+1.02%)	⬆️
util	89.91% <86.66%> (-0.09%)	⬇️
vm	82.10% <93.16%> (+0.77%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[acolytec3]

Okay, I think this bigint PR is now ready for review at long last. All the tests are passing (except Hardhat which isn't surprising). Question for the group, do you want me to figure out which values Hardhat is failing on (looks like there are VM bigints that it's trying to reference) and put a BN wrapper around them or do we want to review as is, potentially merge, and then continue on with a bigint conversion across the whole monorepo.

[ryanio]

it's okay if hardhat isn't passing on develop since it's the breaking changes, which is expected

[ryanio]

would suggest leaving the default values out when unneeded, i.e.

Suggested change

	const gasUsed = new BN(this.gasUsed.toString(10), 10)
	const gasUsed = new BN(this.gasUsed.toString())

[ryanio]

Suggested change

	st.equal(true, totalGasUsed === BigInt(test.totalGasUsed) + 21000n) // Add tx upfront cost.
	st.ok(totalGasUsed === BigInt(test.totalGasUsed) + 21000n, 'totalGasUsed should be correct') // Add tx upfront cost.

[ryanio]

awesome, thanks for doing that!

let's move the three functions you added to types.ts over to bytes.ts, all of them there sounds good to me

edit: I'm still seeing a handful of places where these new functions aren't used (big pr 😄) do you want me add a commit to convert more of them over, or point them out in review comments, or you got them?

[acolytec3]

edit: I'm still seeing a handful of places where these new functions aren't used (big pr smile) do you want me add a commit to convert more of them over, or point them out in review comments, or you got them?

I'll take another skim through it. I was just searching with VS Code so probably missed a couple of spots. Will just go line by line.

[acolytec3]

let's move the three functions you added to types.ts over to bytes.ts, all of them there sounds good to me

I'm not sure I follow the logic on this one. These three are type conversion utilities between non Buffer types and we already have similar type converters for intToHex/bnToHex etc in the types section. Happy to move but it seems like it's more fitting for them to stay in types unless we're also going to move all the other type converters to bytes too (and I could make a case for that I think.

[ryanio]

good point, sounds good!

[holgerd77]

I'm not sure I follow the logic on this one. These three are type conversion utilities between non Buffer types and we already have similar type converters for intToHex/bnToHex etc in the types section. Happy to move but it seems like it's more fitting for them to stay in types unless we're also going to move all the other type converters to bytes too (and I could make a case for that I think.

Yes, these conversion function like bnToHex() in types.ts are totally misplaced and we should move, wonder if I have at some point already opened an issue on that (or at least mentioned somewhere some time ago)?

If you do please make sure to clearly document somewhere, otherwise this will likely be forgotten on the release notes at some point.

[holgerd77]

Ok, since this got another review from Paul I will merge this in now, then the subsequent BigInt (and other) work can be done on top (please: do by rebase 🙂).

Thanks again @acolytec3 for this great work here and @jochem-brouwer and @paulmillr for the assistance and guidance on some things.

Really cool! 🎉
Let's go BigInt. 🚀

[holgerd77]

LGTM.