Use bls.Scalar as the base class for BLSFieldElement

[jtraglia]

The goal of this PR is to simplify the spec and speed up test execution. One semi-easy way to do this is to use the arkworks_Scalar type instead of the remerkleable:uint type as the underlying type for BLSFieldElement. The arkworks type is much faster and provides useful helper methods which improve spec readability. Tested locally, this PR resulted in a 300% performance improvement for KZG operations. These changes only affect internal code and not any of the public API methods; those still take bytes and convert to internal types. This PR makes the following notable changes:

• Update py_arkworks_bls12381 to get the updated Scalar type.
  • It can be initialized with integers greater than u128 now.
  • There are also some new operations, to support our spec usage.
  • With the subclass feature, it can be used as a base type now.
• Move definitions for the following types to the spec builder classes:
  • BLSFieldElement, Polynomial, PolynomialCoeff, Coset, CosetEvals.
  • In the future, we hope to add a special section in the spec for this.
• Remove two helper functions: bls_modular_inverse and div.
  • These operations can be performed with Scalar::inverse and / now.
• Add eip7594 to the list of executable specs and fix pylint errors.
• Add a new py_ecc_Scalar class which can be used instead of arkworks_Scalar.

[ralexstokes]

I like performance improvement!

I don't like exposing the bls.Scalar type in the specs though as it implicitly brings in a dependency on some external library when the specs should be self-contained.

One way to handle this would be to leave the spec as-is and rebind the names to more efficient implementations when testing. We do this with the BLS implementation and should be able to do something similar here without it affecting the spec itself.

[mratsim]

Ideally I'd prefer to use an audited backend, I don't know if arkworks was cc @Pratyush

[jtraglia]

Ideally I'd prefer to use an audited backend, I don't know if arkworks was cc @Pratyush

To be fair, neither py_ecc nor milagro_bls have been audited. Both have disclaimers on their READMEs.

We've been using arkworks as the backend for MSMs, pairing checks, multi_exp, etc for a while too:

consensus-specs/tests/core/pyspec/eth2spec/utils/bls.py

Lines 87 to 92 in 2b4d94a

	def use_fastest():
	"""
	Shortcut to use Milagro for signatures and Arkworks for other BLS operations
	"""
	global bls
	bls = fastest_bls

[asn-d6]

Hmm, is it possible to make this change a bit less wide?

I feel like the removal of those types and functions affect both the readability of the spec (types are nice) and of the libraries who might be using those types (and now they will be dangling).

Would the performance remain bad if we just typedefed bls.Scalar to BLSFieldElement? This way, Alex's concern that we are pinning a particular dependency would still be true, but at least it would be a more isolated dependency.

Finally, just curious: what improvement does this do to the test execution time?

[jtraglia]

Would the performance remain bad if we just typedefed bls.Scalar to BLSFieldElement?

Where would this typedef exist? It cannot go in the "custom types" table because it is not an SSZ type. Would we make typedefs for the other deleted types too? Would those go in the same place?

Finally, just curious: what improvement does this do to the test execution time?

Hard to tell exactly, but locally it was 3x faster. To know for sure, we need to push a new curdleproofs release to fix CI tests.

[ralexstokes]

yeah, after looking at this some I think the better way to do this is with the SpecBuilder infra we have

it looks like we will need to extend the code to handle type substitutions (where you can swap BLSFieldElement == int to BLSFieldElement == arkworks_Scalar

and if the other functions are need to be updated for the performance improvements we can use the SpecBuilder.implement_optimizations infra

another caveat is that we will likely want a way to run both versions of the spec (as-is, and with the optimizations) so will want some way to generate both via the SpecBuilder

[jtraglia]

it looks like we will need to extend the code to handle type substitutions (where you can swap BLSFieldElement == int to BLSFieldElement == arkworks_Scalar

So we could keep the BLSFieldElement name, but it couldn't be int anymore. It wouldn't behave like a field element; it would allow values greater than or equal to BLS_MODULUS. In this PR, I removed a lot of the mod operations. Part of the reason I wanted to do this was so the spec was easier to read. For example:

What do you think about the two commits that I just pushed? They add a new py_ecc_Scalar class which is based on FQ. I just needed to set field_modulus and define two functions match arkworks_Scalar. With --bls-type=py_ecc it will use py_ecc_Scalar (which is pretty much just a wrapper around int).

Also here's my local test performance:

• origin/dev -- 11m08s
• --bls-type=fastest with arkworks_Scalar -- 3m12s
• --bls-type=fastest with py_ecc_Scalar -- 12m24s

[jtraglia]

Here's a more concrete performance comparison. It's ~2x faster.

Before: 30m4s (CircleCI) & 15m57s (GitHub)

Screenshots

After: 17m22s (CircleCI) & 6m51s (GitHub)

Screenshots

[hwwhww]

I love the significant performance improvement! 👍
The scope of this change looks fine to me since the polynomial-commitment docs are for crypto libs only.

[ppopth]

The scope of this change looks fine to me since the polynomial-commitment docs are for crypto libs only.

I think this is a good point. We can treat polynomial-commitments docs differently from others.

[asn-d6]

I think this is a plausible approach. IMO spec readability is still slightly decreased, but I think it's now at reasonable areas of the tradeoff space.

I left some comments as well.

[asn-d6]

LGTM! Thanks a lot!