EVM: Generic BLS Interface / Use JS Implementation (@noble/curves) as Default #3471

holgerd77 · 2024-06-24T11:44:03Z

Follow-up on #3350

Goal of this PR is to move over to use a JavaScript BLS implementation for the BLS precompiles (EIP-2537). For this the direct usage of the MCL library in the precompile files as well as the tight MCL library coupling (by adding as a property to the EVM) is replaced by using a generic EVMBLSInterface. It will likely be possible later on to fully allow switching the implementation, not sure if we want to go that far, we'll see.

This first push is for testing with the official test suite after setting up the base structures (as well as some temporary double structures like the doubled mcl code for gradual implementation which will go away at the end).

codecov · 2024-06-24T11:53:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.16%. Comparing base (d24ca11) to head (cbb8041).
Report is 42 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
blockchain	`?`
common	`?`
statemanager	`?`
tx	`80.16% <ø> (?)`
util	`?`

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

holgerd77 · 2024-06-28T11:38:04Z

I've done something wrong with my fork builds of @noble/curves from here https://github.com/holgerd77/noble-curves/tree/build-5fcd71a, I battled with this already the first 1 1/2 hours of the morning and ended up manually copying over the code to my node_modules folder.

This is a known issue though and should go away once @noble/curves gets an official update with the changes.

holgerd77 · 2024-06-28T11:39:29Z

So if one is running

npx vitest run test/precompiles/eip-2537-bls.spec.ts

from the evm folder locally these are the more meaningful test runs (currently 8/131 tests failing).

jochem-brouwer · 2024-07-02T06:44:30Z

Instructions on how to run the ethereum-tests BLS tests (please note, we had a discussion on that PR in April, see ethereum/tests#1366)

Ensure ethereum-tests is on v14:

cd ./packages/ethereum-tests && git checkout tags/v14.0

To run the state tests:

cd ./packages/vm && npm run test:state -- --fork=Prague --dir=../EIPTests/StateTests/stEIP2537

Note: blockchain tests cannot be ran, it throws some error about a header field missing, so it runs on partial Prague.

State tests output:

1..1403
# tests 1403
# pass  1217
# fail  186

The tests all pass on our current master (so with MCL)!

jochem-brouwer · 2024-07-02T06:46:24Z

Oh wait, nvm, I can of course test using mcl-wasm. If I re-run the tests with MCL, then all pass. So we definitely have bugs!

jochem-brouwer

Some comments, but this looks extremely good in general, especially how MCL / Noble is now decoupled, I love it!

packages/evm/src/evm.ts

packages/evm/src/index.ts

jochem-brouwer · 2024-07-02T07:08:15Z

packages/evm/src/precompiles/bls12_381/util.ts

+
+export const zeroByteCheck = (
+  opts: PrecompileInput,
+  zeroByteRanges: number[][],


Note: this will thus slice the array exclusive of zeroByteRanges[x][1]. So for instance the array [[0,1]] will only check the 0th byte, not the byte at position 1. Is this correct? (And can docs be added for this method?)

Yes, I think this should be all correct, actually this only copies your former code into a new function 😂, see this commit 62f15f2.

So this is a BLS specific zero byte check, applying this part of the EIP:

And specificall this "Due to the size of p, the top 16 bytes are always zeroes." part, see that the input subarrays always have a length of 16 bytes.

I'll give this and the other methods some documentation.

jochem-brouwer · 2024-07-02T07:09:03Z

packages/evm/src/precompiles/bls12_381/util.ts

+      zeroByteRanges[index][0] + pairStart,
+      zeroByteRanges[index][1] + pairStart
+    )
+    if (!(equalsBytes(slicedBuffer, ZERO_BYTES_16) === true)) {


Why is this constant ZERO_BYTES_16 here and not zeros(x) where x is the expected length of the zero byte array?

See answer above.

jochem-brouwer · 2024-07-02T07:10:45Z

packages/evm/src/types.ts

+   * An interface for the MCL WASM implementation https://github.com/herumi/mcl-wasm
+   * is shipped with this library, see the following test file for an example on
+   * how to use:
+   * https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/evm/test/precompiles/eip-2537-bls.spec.ts


This test file does not have tests which specifically use MCL. Did you intend to use a permalink here?

I think you intend to permalink to the test which specifically runs on this PR (and later thus likely on master). Could you also point to the specific lines in this permalink? Especially the mcl init and the mcl curve settings should be initialized before being passed into the EVM.

I initially wanted to just add a code example here (which I think is the better solution) and then I tried and the linter complained and I was just lazy and therfore added the test link. 😂 Will go back to the code version, this now worked this time anyhow (no idea why).

jochem-brouwer · 2024-07-02T07:13:27Z

packages/evm/test/precompiles/eip-2537-bls.spec.ts

+mcl.setMapToMode(mcl.IRTF) // set the right map mode; otherwise mapToG2 will return wrong values.
+mcl.verifyOrderG1(true) // subgroup checks for G1
+mcl.verifyOrderG2(true) // subgroup checks for G2
+const mclbls = new MCLBLS(mcl)


I think we can maybe improve here (I also now see why we export MCLBLS / NobleBLS). Can we add a static method to the interfaces which inits these BLSs? Then we can init the MCL using MCLBLS.init(mcl) and write those settings there

That's a great idea, will directly add! 👍 🙂

I now went for a non-static init() version, I find it a bit nicer if the out asny initialization (so await mcl.init(mcl.BLS12_381)) is not also wrapped and can be done separately by the user, guess both ways would work and be fine though, for this one I find the typing also a bit nicer. The optional init() method from the interface is the automatically called from within the EVM constructor if present.

import * as mcl from 'mcl-wasm' await mcl.init(mcl.BLS12_381) const evm = await EVM.create({ mcl: new MCLBLS(mcl) })

… folder structure

… as a work basis

…ted set of abstractions

…w VM evmOpts option

…r methods

…straction) -> Fixes VM state tests

jochem-brouwer

This review is on top of my previous review, got 2 small comments, LGTM in general 😄

jochem-brouwer · 2024-07-05T09:56:10Z

packages/evm/src/precompiles/bls12_381/util.ts

+ * @param pairStart
+ * @returns
+ */
+export const zeroByteCheck = (


Nit: this does zero byte checks, but only if the range of these zero bytes is 16 zero bytes, so the name is a bit misleading.

Good point, will rename to leading16ZeroBytesCheck

jochem-brouwer · 2024-07-05T09:58:55Z

packages/vm/test/tester/index.ts

@@ -45,6 +45,7 @@ import type { bn128 } from '@ethereumjs/evm'
 * --expected-test-amount: number.        If passed, check after tests are ran if at least this amount of tests have passed (inclusive)
 * --verify-test-amount-alltests: number. If passed, get the expected amount from tests and verify afterwards if this is the count of tests (expects tests are ran with default settings)
 * --reps: number.                        If passed, each test case will be run the number of times indicated
+ * --bls: string.                         Use MCL as a bls library if passed (default: Noble) (state tests only)


Could we also add this for blockchain tests?

Following on this, is there a reason we wouldn't want to use MCL by default in the test runner? I don't know how many BLS related tests there are but anything that slows down the tests seems undesirable to me since these are the main bottleneck in our CI right now, right?

Was a bit lazy and I feel that every new code is this messy test runner is a bit of "lost code" 😂, so I decided to skip. But I can very well add, makes sense for practical reasons and shouldn't be so much extra work.

@acolytec3 Yes, guess we can do that for the VM as long as we keep running tests for both implementations in the EVM.

acolytec3

Looks great overall. One item regarding comments in the noble.ts that should be clarified.

acolytec3 · 2024-07-05T10:52:19Z

packages/evm/src/precompiles/bls12_381/noble.ts

+  return G1
+}
+
+// input: a mcl G1 point


Do we need all these mcl references? I'm assuming these are obsolete in this file, right? Can they be replaced with something like noble/curves

Don't think that a noble/curves reference is needed here, will just completely remove

acolytec3 · 2024-07-05T10:56:41Z

packages/vm/test/tester/index.ts

@@ -45,6 +45,7 @@ import type { bn128 } from '@ethereumjs/evm'
 * --expected-test-amount: number.        If passed, check after tests are ran if at least this amount of tests have passed (inclusive)
 * --verify-test-amount-alltests: number. If passed, get the expected amount from tests and verify afterwards if this is the count of tests (expects tests are ran with default settings)
 * --reps: number.                        If passed, each test case will be run the number of times indicated
+ * --bls: string.                         Use MCL as a bls library if passed (default: Noble) (state tests only)


Following on this, is there a reason we wouldn't want to use MCL by default in the test runner? I don't know how many BLS related tests there are but anything that slows down the tests seems undesirable to me since these are the main bottleneck in our CI right now, right?

…fault

jochem-brouwer

LGTM!

jochem-brouwer

EVM lint failed, should pass now, still LGTM 😄 👍

All addressed.

holgerd77 added type: feature PR state: WIP type: refactor package: evm labels Jun 24, 2024

holgerd77 mentioned this pull request Jun 26, 2024

BLS12_381 G1 fromHex() Instantiation Bug paulmillr/noble-curves#143

Closed

holgerd77 force-pushed the evm-js-default-for-bls-precompiles branch from 75d14f1 to a3f2a1e Compare June 28, 2024 11:12

holgerd77 force-pushed the evm-js-default-for-bls-precompiles branch from a3f2a1e to 4190d5a Compare July 1, 2024 10:35

jochem-brouwer reviewed Jul 2, 2024

View reviewed changes

holgerd77 force-pushed the evm-js-default-for-bls-precompiles branch 2 times, most recently from 38563a2 to 3eab6a4 Compare July 2, 2024 12:03

holgerd77 added PR state: needs review and removed PR state: WIP labels Jul 3, 2024

holgerd77 added 13 commits July 3, 2024 12:57

Add @noble/curves dependency to EVM

4ad3341

Rebuild package-lock.json

020e7e0

Rename mcl specific util code file, prepare for more generic BLS util…

c2846ab

… folder structure

Add mcl util file copy as noble to new bls12_381 util folder to serve…

5499f6f

… as a work basis

Simplify folder structure

7105f75

Adopt file imports

f9e31cc

Start moving to a generically applicable bls interface with some shif…

445e127

…ted set of abstractions

Some modest Noble library switch

db40e41

Move generic constants to own file

4a3d256

First @noble/curves integration test (not yet working)

8e36f57

Add generalized zero byte check util method

f084e57

Add generalized gas check utility method

772d179

Add utility methods for equality and modulo length checks

89d4b26

holgerd77 added 13 commits July 3, 2024 12:57

Add some util function code docs

0afa955

Add mcl-wasm to client dependencies

1ccf2c3

Rebuild package-lock.json

2bce773

Integrate explicit MCL usage for BLS precompiles in the client via ne…

c27d0c2

…w VM evmOpts option

Some clean-up

0a2b9f0

Simplify, align and optimize infinity point checks in toG1/toG2 helpe…

670262c

…r methods

Simplify Noble fromG1/fromG2 point methods

ab626c4

Simplify MCL fromG1/fromG2 methods

30380e0

Move mcl-wasm depedency in VM from production to dev dependencies

eebee1b

Rebuild package-lock.json

bc01789

Add simple --bls CLI option to VM state test runner

d266a1a

Modulo your way down on Fr field order check (instead of a simple sub…

ffa7ca9

…straction) -> Fixes VM state tests

Some clean-ups and docs

cbb8041

holgerd77 force-pushed the evm-js-default-for-bls-precompiles branch from 1c997ed to cbb8041 Compare July 3, 2024 11:02

Merge branch 'master' into evm-js-default-for-bls-precompiles

95b47e4

jochem-brouwer reviewed Jul 5, 2024

View reviewed changes

acolytec3 previously requested changes Jul 5, 2024

View reviewed changes

holgerd77 added 4 commits July 5, 2024 14:03

Merge branch 'master' into evm-js-default-for-bls-precompiles

cdac208

Rename BLS util zeroByteCheck -> leading16ZeroBytesCheck

a4c7ad1

Expand VM test runner --bls option to blockchain tests, use MCL as de…

f698eb1

…fault

Some naming clean-up

d991389

jochem-brouwer previously approved these changes Jul 5, 2024

View reviewed changes

evm: lint

f37bea5

jochem-brouwer dismissed their stale review via f37bea5 July 5, 2024 12:48

jochem-brouwer approved these changes Jul 5, 2024

View reviewed changes

holgerd77 merged commit e08c229 into master Jul 5, 2024
34 checks passed

holgerd77 deleted the evm-js-default-for-bls-precompiles branch July 5, 2024 13:46

holgerd77 mentioned this pull request Aug 6, 2024

EVM: Generic BN254 (alt_BN128) Interface for Precompiles / Use @noble/curves By Default #3564

Merged

holgerd77 mentioned this pull request Sep 25, 2024

BLS: use noble-curves MSM #3626

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM: Generic BLS Interface / Use JS Implementation (@noble/curves) as Default #3471

EVM: Generic BLS Interface / Use JS Implementation (@noble/curves) as Default #3471

holgerd77 commented Jun 24, 2024

codecov bot commented Jun 24, 2024 •

edited

Loading

holgerd77 commented Jun 28, 2024

holgerd77 commented Jun 28, 2024

jochem-brouwer commented Jul 2, 2024 •

edited

Loading

jochem-brouwer commented Jul 2, 2024

jochem-brouwer left a comment

jochem-brouwer Jul 2, 2024

holgerd77 Jul 2, 2024

jochem-brouwer Jul 2, 2024

holgerd77 Jul 2, 2024

jochem-brouwer Jul 2, 2024

jochem-brouwer Jul 2, 2024

holgerd77 Jul 2, 2024

jochem-brouwer Jul 2, 2024

holgerd77 Jul 2, 2024

holgerd77 Jul 2, 2024

jochem-brouwer left a comment

jochem-brouwer Jul 5, 2024

holgerd77 Jul 5, 2024

jochem-brouwer Jul 5, 2024

acolytec3 Jul 5, 2024

holgerd77 Jul 5, 2024

holgerd77 Jul 5, 2024

acolytec3 left a comment

acolytec3 Jul 5, 2024

holgerd77 Jul 5, 2024

acolytec3 Jul 5, 2024

jochem-brouwer left a comment

jochem-brouwer left a comment

EVM: Generic BLS Interface / Use JS Implementation (@noble/curves) as Default #3471

EVM: Generic BLS Interface / Use JS Implementation (@noble/curves) as Default #3471

Conversation

holgerd77 commented Jun 24, 2024

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

holgerd77 commented Jun 28, 2024

holgerd77 commented Jun 28, 2024

jochem-brouwer commented Jul 2, 2024 • edited Loading

jochem-brouwer commented Jul 2, 2024

jochem-brouwer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolytec3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2024 •

edited

Loading

jochem-brouwer commented Jul 2, 2024 •

edited

Loading