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

Fix EIP2537 fixtures #3388

Closed
wants to merge 1 commit into from
Closed

Fix EIP2537 fixtures #3388

wants to merge 1 commit into from

Conversation

jochem-brouwer
Copy link
Member

This PR fixes the EIP 2537 fixtures from https://github.com/ethereum/execution-spec-tests/files/15099569/fixtures_develop.tar.gz

To run, see #3387

Before, 164 failing tests.

After the "fixed gas schedule" there are only 10 failing tests which correspond to:

tests/prague/eip2537_bls_12_381_precompiles/test_bls12_g1add.py::test_valid[fork_Prague-blockchain_test-bls_g1add_g1_wrong_order+g1-]
tests/prague/eip2537_bls_12_381_precompiles/test_bls12_g1add.py::test_valid[fork_Prague-blockchain_test-not_in_subgroup_1-]
tests/prague/eip2537_bls_12_381_precompiles/test_bls12_g1add.py::test_valid[fork_Prague-blockchain_test-not_in_subgroup_2-]
tests/prague/eip2537_bls_12_381_precompiles/test_bls12_g2add.py::test_valid[fork_Prague-blockchain_test-bls_g2add_g2_wrong_order+g2-]
tests/prague/eip2537_bls_12_381_precompiles/test_bls12_g2add.py::test_valid[fork_Prague-blockchain_test-not_in_subgroup-]

These individual tests can be fixed by disabling these checks in ./packages/evm/src/precompiles/util/bls12_381.ts:

  if (G1.isValidOrder() === false) {
    throw new EvmError(ERROR.BLS_12_381_POINT_NOT_ON_CURVE)
  }

  // Check if these coordinates are actually on the curve.
  if (G1.isValid() === false) {
    throw new EvmError(ERROR.BLS_12_381_POINT_NOT_ON_CURVE)
  }

and

  if (mclPoint.isValidOrder() === false) {
    throw new EvmError(ERROR.BLS_12_381_POINT_NOT_ON_CURVE)
  }

  if (mclPoint.isValid() === false) {
    throw new EvmError(ERROR.BLS_12_381_POINT_NOT_ON_CURVE)
  }

(In the BLS12_381_ToG1Point and BLS12_381_ToG2Point methods)

However, disabling those now fails other tests, so not sure how to proceed here.

@acolytec3
Copy link
Contributor

We should definitely cherry-pick the gas changes into a new PR (or else rebase this on master so we can continue triaging the test failures here).

@jochem-brouwer
Copy link
Member Author

Commit got cherry-picked into #3400

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants