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

Test generators for kzg-4844 libraries #3274

Merged
merged 49 commits into from
Mar 2, 2023
Merged

Test generators for kzg-4844 libraries #3274

merged 49 commits into from
Mar 2, 2023

Conversation

dankrad
Copy link
Contributor

@dankrad dankrad commented Feb 25, 2023

Based on #3272.

Tests to be written for

  • compute_kzg_proof
  • verify_blob_kzg_proof
  • verify_blob_kzg_proof_batch
  • blob_to_kzg_commitment
  • verify_kzg_proof
  • compute_blob_kzg_proof

@hwwhww
Copy link
Contributor

hwwhww commented Feb 26, 2023

Great work!

Some installation errors:

Linux (CircleCI)

Operating System: Ubuntu 20.04.5 LTS
OSType: linux
Architecture: x86_64
Python: CPython 3.8

Collecting py_arkworks_bls12381==0.3.1
  Downloading py_arkworks_bls12381-0.3.1.tar.gz (11 kB)
  Installing build dependencies ... -� �\� �|� �/� �done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... -� �error
    ERROR: Command errored out with exit status 1:
     command: /home/circleci/specs-repo/venv/bin/python3 /home/circleci/specs-repo/venv/lib/python3.8/site-packages/pip/_vendor/pep517/in_process/_in_process.py prepare_metadata_for_build_wheel /tmp/tmpomv4ghkj
         cwd: /tmp/pip-install-3wfzp7my/py-arkworks-bls12381_1a72aa3f75004b0cad637ad55afd6093
    Complete output (6 lines):
    
    Cargo, the Rust package manager, is not installed or is not on PATH.
    This package requires Rust and Cargo to compile extensions. Install it through
    the system's package manager or via https://rustup.rs/
    
    Checking for Rust toolchain....
    ----------------------------------------
WARNING: Discarding https://files.pythonhosted.org/packages/80/36/e237fbcf36960097892686f34414cef260132f04299619e7e6d4d0f4858b/py_arkworks_bls12381-0.3.1.tar.gz#sha256=6eb0f01d4545747626753fe5baf4c2a1ff014b03a7eb6531a6941ee5e6d5bce6 (from https://pypi.org/simple/py-arkworks-bls12381/) (requires-python:>=3.7). Command errored out with exit status 1: /home/circleci/specs-repo/venv/bin/python3 /home/circleci/specs-repo/venv/lib/python3.8/site-packages/pip/_vendor/pep517/in_process/_in_process.py prepare_metadata_for_build_wheel /tmp/tmpomv4ghkj Check the logs for full command output.
ERROR: Could not find a version that satisfies the requirement py_arkworks_bls12381==0.3.1 (from eth2spec[lint]) (from versions: 0.1.0, 0.1.1, 0.1.2, 0.1.3, 0.1.4, 0.1.5, 0.2.0, 0.2.1, 0.3.0, 0.3.1)
ERROR: No matching distribution found for py_arkworks_bls12381==0.3.1

macOS

CPU: M1 pro
Python: pypy3.9

Building wheels for collected packages: py_arkworks_bls12381
  Building wheel for py_arkworks_bls12381 (pyproject.toml) ... error
  error: subprocess-exited-with-error

  × Building wheel for py_arkworks_bls12381 (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [9 lines of output]
      Running `maturin pep517 build-wheel -i /Users/hww/dev/consensus-specs/venv/bin/python3 --compatibility off`
      🔗 Found pyo3 bindings
      💻 Using `MACOSX_DEPLOYMENT_TARGET=11.0` for aarch64-apple-darwin by default
      error: package `ark-ff v0.4.1` cannot be built because it requires rustc 1.63 or newer, while the currently active rustc version is 1.61.0
      💥 maturin failed
        Caused by: Failed to build a native library through cargo
        Caused by: Cargo build finished with "exit status: 101": `"cargo" "rustc" "--release" "--manifest-path" "/private/var/folders/zf/7y9f1tzn2y33y9601hsthq_40000gn/T/pip-install-m_okx_w7/py-arkworks-bls12381_0cc61e4af6c84c97824e35b118764cfd/Cargo.toml" "--message-format" "json" "--lib" "--" "-C" "link-arg=-undefined" "-C" "link-arg=dynamic_lookup" "-C" "link-args=-Wl,-install_name,@rpath/py_arkworks_bls12381.pypy39-pp73-darwin.so"`
      🐍 Found PyPy 3.9 at /Users/hww/dev/consensus-specs/venv/bin/python3
      Error: command ['maturin', 'pep517', 'build-wheel', '-i', '/Users/hww/dev/consensus-specs/venv/bin/python3', '--compatibility', 'off'] returned non-zero exit status 1
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for py_arkworks_bls12381
Failed to build py_arkworks_bls12381
ERROR: Could not build wheels for py_arkworks_bls12381, which is required to install pyproject.toml-based projects
WARNING: You are using pip version 22.0.4; however, version 23.0.1 is available.
You should consider upgrading via the '/Users/hww/dev/consensus-specs/venv/bin/python3 -m pip install --upgrade pip' command.
make: *** [install_test] Error 1

IIUC, milagro_bls_binding also uses maturin. It looks like py-arkworks-bls12381 distributions only support CPython 3.11+arm64 now: https://pypi.org/project/py-arkworks-bls12381/#files

@kevaundray could you make py-arkworks-bls12381 support Python >=3.8 (CPython & PyPy) and different platforms by uploading more distributions rather than adding the requirements of installing Rust? 🙏

@hwwhww
Copy link
Contributor

hwwhww commented Feb 26, 2023

@kevaundray do you have some py-ecc v.s. arkworks benchmarks of the pairing? Once we make CircleCI pass, we can try to see if this PR also makes #3255 CircleCI pass.

@kevaundray
Copy link
Contributor

@hwwhww I will prepare benchmark numbers + a script to verify and make the modifications that you mentioned above :)

@kevaundray
Copy link
Contributor

kevaundray commented Feb 26, 2023

@hwwhww

Ran benchmarks on my laptop MacBook Pro M1.

Reproduce

To reproduce from source, you can do the following:

  1. Checkout commit
  2. Setup virtual environment
  3. python3 -m examples.benches.bench

To reproduce from pypi:

  1. Download a version >0.3.2

Py-ecc benchmarks

pairingsGen 172.97867500001303 ms
pairings 176.04457500001445 ms
multiplyG1 0.8747412499906204 ms
multiplyG2 3.522136249994219 ms
addG1 0.018061249993479578 ms
addG2 0.07142667000152869 ms
G1_to_bytes48 0.001760420000209706 ms
G2_to_bytes96 0.0599379200139083 ms

Arkworks benchmarks

pairingsGen 1.298829099869181 ms (~133x)
pairings 1.2997874999200576 ms (~135x)
multiplyG1 0.021608329989248887 ms (~40x)
multiplyG2 0.06284999999479624 ms (~58x)
addG1 0.0006941699939488899 ms (~28x)
addG2 0.0021008399926358834 ms (~35x)
G1_to_bytes48 0.00037416999475681223 ms (~4.5x)
G2_to_bytes96 0.0005658299960487057 ms (~104x)

Notes

pairings vs pairingsGen

I store the points in Projective coordinates, but arkworks requires pairing input to be in Affine coordinates. This will incur a cost.

pairingsGen is just the generators being used to which arkworks can optimise as the conversion from Projective to Affine since z=1.

pairingsGen will replace one of the pairings with two points which don't have z=1, to see if there is a noticeable cost.

MultiExp

Having a multi-exponentation algorithm is quite common for ECC libraries so py-arkworks also exposes this. It uses pippenger so would be alot faster than g1_lincomb. We won't be able to use it in the specs but it may be useful for test generation in the future

@kevaundray
Copy link
Contributor

I've added a lot more distributions and have updated setup.py here to use 0.3.2

See here for added distributions

@hwwhww
Copy link
Contributor

hwwhww commented Feb 26, 2023

@kevaundray Thank you! CircleCI (Linux) installation succeeds now.

But for macOS, I only see Python 3.10 and Python 3.11 distributions:

  • py_arkworks_bls12381-0.3.2-cp311-cp311-macosx_11_0_arm64.whl
  • py_arkworks_bls12381-0.3.2-cp311-cp311-macosx_10_7_x86_64.whl
  • py_arkworks_bls12381-0.3.2-cp310-cp310-macosx_11_0_arm64.whl
  • py_arkworks_bls12381-0.3.2-cp310-cp310-macosx_10_7_x86_64.whl

Any chance you could also upload CPython 3.8 and 3.9 support? Python3.8 is the minimum requirement of eth2spec.

I guess it's still early for PyPy+macOS+arm64 distribution so you can skip it for now. (Also, PyPy only provides 3.8 and 3.9 support and started to support M1 last year. 🥲)


I managed to install it with a CPython 3.10 env on the same machine. The test case that used to take me ~30s to run with py-ecc, now is only 6.03s with --bls-type=fastest! 🚀 🚀

@kevaundray
Copy link
Contributor

@hwwhww I've pushed a 0.3.3 which now uses 3.8 :)

@kevaundray
Copy link
Contributor

@hwwhww if tests are generated using py-arkworks specifically and not via the bls interface, we can probably reduce the six seconds further via py-arkworks specific functions!

@hwwhww
Copy link
Contributor

hwwhww commented Feb 27, 2023

@kevaundray Great!

FYI, just to record my troubleshooting for PyPy, I did something like what I needed for milagro binding:

  1. Upgrade my rustc:

    rustup update
    
  2. Install py_arkworks_bls12381 via building PyPy wheel:

    pip install --no-binary py_arkworks_bls12381 py_arkworks_bls12381
    

    Output:

    Collecting py_arkworks_bls12381
    Using cached py_arkworks_bls12381-0.3.2.tar.gz (13 kB)
    Installing build dependencies ... done
    Getting requirements to build wheel ... done
    Preparing metadata (pyproject.toml) ... done
    Building wheels for collected packages: py_arkworks_bls12381
    Building wheel for py_arkworks_bls12381 (pyproject.toml) ... done
    Created wheel for py_arkworks_bls12381: filename=py_arkworks_bls12381-0.3.2-pp39-pypy39_pp73-macosx_11_0_arm64.whl size=461544 sha256=89d7fec7125eaf8bd7ba85ce17f02abbca817f1392fc3db7df98eecd6d01b6ec
    Stored in directory: /Users/hww/Library/Caches/pip/wheels/4a/c6/f3/3630a2e3deb887d1506a2eaca9de8d6d4e91e8d7d4ddafb559
    Successfully built py_arkworks_bls12381
    Installing collected packages: py_arkworks_bls12381
    Successfully installed py_arkworks_bls12381-0.3.2
    

The same test case I mentioned (~30s with py-ecc) is now only 3.70s with PyPy3.9 with --bls-type=fastest. 😎

@@ -0,0 +1,288 @@
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be split into many files, like one file per one kzg-4844 function? I think doing it similarly to the one in https://github.com/ethereum/consensus-specs/tree/dev/tests/generators/ssz_generic is more organized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion on this one. @hwwhww what would you prefer? I copied from the bls generators which seem to have this structure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slightly prefer separate files, but I can see there are some shared helpers/constants. ~500 lines look acceptable so far, but if you expect to extend the test cases, refactoring them early seems better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided on the call today to leave for now and split when the file becomes too large.

dankrad and others added 3 commits February 28, 2023 12:27
Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two minor typos that need to be fixed here:

tests/generators/kzg_4844/main.py Outdated Show resolved Hide resolved
tests/generators/kzg_4844/main.py Outdated Show resolved Hide resolved
tests/formats/kzg/blob_to_kzg_commitment.md Outdated Show resolved Hide resolved
tests/formats/kzg/compute_blob_kzg_proof.md Outdated Show resolved Hide resolved
tests/formats/kzg/compute_kzg_proof.md Outdated Show resolved Hide resolved
tests/formats/kzg/verify_blob_kzg_proof.md Outdated Show resolved Hide resolved
tests/formats/kzg/verify_blob_kzg_proof_batch.md Outdated Show resolved Hide resolved
tests/formats/kzg/verify_kzg_proof.md Outdated Show resolved Hide resolved

## Condition

The `verify_blob_kzg_proof` handler should verify that `commitment` is a correct KZG commitment to `blob` by using the blob KZG proof `proof`, and the result should match the expected `output`. If the commitment or proof are invalid (e.g. not on the curve or not in the G1 subgroup of the BLS curve) or `blob` is invalid (e.g. incorrect length or one of the 32 byte blocks does not represent a BLS field element), it should error, i.e. the output should be `None`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the commitment or proof is invalid (e.g. not on the curve or not in the G1 subgroup of the BLS curve) or blob is invalid (e.g. incorrect length or one of the 32-byte blocks does not represent a BLS field element), it should error, i.e. the output should be null.

Note that it's not as same as the pyspec BLS signature verify_* methods: in BLS signature API implementation, we catch the exception and return False.

I'm not suggesting adding error handling in polynomial-commitments.md, I think that's too Pythonic. Just note that the clients have to deal with the error handling themselves.

btw, does c-kzg also throw exceptions in these cases? /cc @jtraglia

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure what cases you're referring to, but the C library in c-kzg-4844 will return C_KZG_BADARGS if given invalid inputs. Generally, the bindings will throw an exception if the library doesn't return C_KZG_OK (success).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, the case that the input is in the correct length but doesn't pass the subgroup check. I suppose it's not C_KZG_OK there so will throw an exception?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtraglia
Copy link
Member

jtraglia commented Mar 1, 2023

So either there's something wrong with the tests or something wrong with ckzg. When tested against ckzg, there are failures in compute_blob_kzg_proof, verify_blob_kzg_proof, and verify_blob_kzg_proof_batch, such as:

AssertionError: ../../newtests/compute_blob_kzg_proof/small/compute_blob_kzg_proof_case_valid_blob_94b2a33de83f0ee5/data.yaml
proof.hex()='97425e513a683f1d90a8ebadf84b8f60576e41270272a5561714c450fc904e9eb44f15d8ea37f7672739e82a8f5eb671'
expected_proof.hex()='b1f94037a955facd0540d16dc23533d7f6e6fbe0573af591ca6a2a641edbb8fe3f7feb1df3d82d739f08d99a4abca36c'
AssertionError: ../../newtests/verify_blob_kzg_proof/small/verify_blob_kzg_proof_case_correct_proof_5caa8bb4962217cb/data.yaml
valid=False
expected_valid=True
AssertionError: ../../newtests/verify_blob_kzg_proof_batch/small/verify_blob_kzg_proof_batch_case_9176f3148be71345/data.yaml
valid=False
expected_valid=True

All of the test cases for the other functions pass.

dankrad and others added 6 commits March 1, 2023 22:09
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to merge it now and then @kevaundray can add more bls.py fixes later. Please use squash and merge for the 45 commits. 🙏

But also see @jtraglia pointed out that the new test vector found inconsistency between pyspec or c-kzg, should we address that in this PR? (or the issue may be in c-kzg?)

@asn-d6
Copy link
Contributor

asn-d6 commented Mar 2, 2023

So either there's something wrong with the tests or something wrong with ckzg. When tested against ckzg, there are failures in compute_blob_kzg_proof, verify_blob_kzg_proof, and verify_blob_kzg_proof_batch, such as:

AssertionError: ../../newtests/compute_blob_kzg_proof/small/compute_blob_kzg_proof_case_valid_blob_94b2a33de83f0ee5/data.yaml
proof.hex()='97425e513a683f1d90a8ebadf84b8f60576e41270272a5561714c450fc904e9eb44f15d8ea37f7672739e82a8f5eb671'
expected_proof.hex()='b1f94037a955facd0540d16dc23533d7f6e6fbe0573af591ca6a2a641edbb8fe3f7feb1df3d82d739f08d99a4abca36c'
AssertionError: ../../newtests/verify_blob_kzg_proof/small/verify_blob_kzg_proof_case_correct_proof_5caa8bb4962217cb/data.yaml
valid=False
expected_valid=True
AssertionError: ../../newtests/verify_blob_kzg_proof_batch/small/verify_blob_kzg_proof_batch_case_9176f3148be71345/data.yaml
valid=False
expected_valid=True

All of the test cases for the other functions pass.

Seems like a bug was found -- ckzg was not following the spec! See ethereum/c-kzg-4844#168 for a fix!

With that PR I'm not getting the errors above anymore (using the python bindings).

@asn-d6
Copy link
Contributor

asn-d6 commented Mar 2, 2023

That was a c

I think it's fine to merge it now and then @kevaundray can add more bls.py fixes later. Please use squash and merge for the 45 commits. pray

But also see @jtraglia pointed out that the new test vector found inconsistency between pyspec or c-kzg, should we address that in this PR? (or the issue may be in c-kzg?)

that was a c-kzg bug after all!

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.

None yet

7 participants