Add some CKZG property tests

[jtraglia]

PR Description

I've been reviewing EIP-4844 code today & wrote some fuzzing tests for CKZG methods. I ran into a segmentation fault in libckzg4844jni, so I thought I might as well make a PR with my work. I haven't run into problems with the other methods.

Things worth mentioning:

• Limit tests to 100 runs because they are relatively slow.
• I didn't make property tests for CKZG4844Utils because most of the methods touch the filesystem.
  • There are two that don't but they are pretty basic.
• I didn't make property tests for ZKGCommitment and ZKGProof.
  • They are also pretty basic and it doesn't seem worth it.

The problem

Spoiler: I'm fairly certain it's because there's a zero-length "blob" in blobs.

The following List<Bytes> blobs passed to computeAggregateKzgProof:

[0x0d2024ece3e004271319699b8b00cc010628b6bc0be5457f031fb1db0afd3ff8, 0x, 0x925668a49d06f4, 0xc7f6113f6000d68002dcc507fce7f30016e4db02e2febf126aee04d6012409f3041700f20e1ae9000544ef1a120080204a0b87f09ce87fd8f4b76905e2b40619ee070f75c20a77e820c0bed807f5d922120bfbcad9f626e01f0008241021ccd0ed253d6f00b71b900ae8edf8eecd, 0xae187ff07a635607ab25200c1a3424fe0058fbdde6ee0addf9fbac, 0xf2fb99f9dc615577250cb2f9077fd9f41037030c, 0x442115345d0307cf0525d905efe0f90aaa82c94de304fde97e061004b5dcca, 0xeecaf6f91f264402fbdb1f0a02bba2faf808282dc0c7e1b1468dfb1929, 0x1f33f7ede6001a15201deadf82f9f3, 0xe00e0581f3cafd7f9aef0743f027fe9b01ec00f7bc, 0x26c6f70a5aa1053420f309450baaef0001, 0x44fdf30fa9fce46419fc6cc38119907fdb80ddf61f770b3003df1ae0, 0xa3f86d419c9fcbfcf30c28ece6011ff5, 0x04e3f205f60ef47af609ee0afcd601affa1704047f1a1e7eef2080e728, 0x41, 0x5807a9d7efe509b766f7f31000f80b0412efe6, 0x0c0a0a084180f77b134626877ac4661c0d5c4f3445021769, 0xeef1f3f8f8dc627de37634dd7415eab74d93ea8547637efcfc3df20f302f729c7429d67ef5e503fbdc372f, 0x6dacd9ff03fcf50cd8392338058f080bcafcc5f208d9e4450051f7da2ae4143b901ff41deffc1622fff41065e4f9e10efcd8ed1153fffe060bae18ed0af74cca05661c24f3ff2dfa7200088efff58082257dd064e9cbf6c803080aaaed9ce7e10dfbe4f90089ed1810051cee399db20a24f622fec7594e16270fb361ee1608bf81cfc4fa7528fe7d0a8fefe90425df2f1f379c05f2dcfbea09f21f08f201d787fad6599320fffa00a2f278ee3ce6e8, 0x2af96e02647b0511229607, 0xfe11, 0xfd6b3bff9425fe08f48dfaf5b31de298e108d7fffefb2effff70042f16f4ff3588e9f604f9f62e27e9e17e0e1f80ee22d91dbe976d06f3fc1572500bf3f41c22fe362d020bb425fc07f928b8f80081b300f5cbf01ffadbb50df85578c713031f03971cc41efcf2bfe469f4f70cbae94127e605e1d836f5ac0f290220f321e544015d17d857, 0xeafd72dc94f838d40202e6d3673d04e8f6f823fff81adffbe5f6f20c09ecc4e08511f4f6b622fc0194f75ff81817cff8fb165e12df16100a0420dbfaf9009afbf3afff6773fd6c0d3a4cc5df84e5e3f50881dffdc5fdd67ff57bd60cf4f9f4c6f71f0e0d289902170971d2de020aece9fe760d8503ff64da8f061fdd0610fee1e448f9acfa080c25dfeb93ff212a0b62d0beeb350714b2330afac466290924f914f80a8002ee710bfaaa05020ee40708e1047f7f623b21117a0dfbd9fffe23fe0de17e9225e69d18fee9afd61af7c36731f805fb7edaf7e5018b0a011b07a3dffef22b21b42da3e883085e24e7240256b907fcbb84e7131f650070f661e9fc, 0x7ef7b56501507ff80c002b001726f190a760062602ee2318a3182e0ac4e28fdead69e0bc811b3ee403e5f280e011e3169bc5c326b422bf0c42df4bb8396813ad1d061f10, 0x19a780fcfe3953f3fa79f503fe39e1, 0x54f5046cfbf4fd02f5aeb8fdf4b3ede9, 0x3c0fece50573e905b03ecaf3daff52ee00850b096c0118f8f2, 0x02dbe0daf103200b02e6f3fffe]

Will cause a segmentation fault:

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x0000000103059330, pid=53565, tid=5891
#
# JRE version: OpenJDK Runtime Environment Homebrew (11.0.16.1) (build 11.0.16.1+0)
# Java VM: OpenJDK 64-Bit Server VM Homebrew (11.0.16.1+0, mixed mode, tiered, compressed oops, g1 gc, bsd-aarch64)
# Problematic frame:
# C  [libckzg4844jni.dylib+0x21330]  blst_scalar_from_lendian+0x8
#

More information: hs_err_pid53565.log

Documentation

• I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

• I thought about adding a changelog entry, and added one if I deemed necessary.

[jtraglia]

Maybe we should change the blob argument type from Bytes to Bytes32.

[StefanBratanov]

Maybe we should change the blob argument type from Bytes to Bytes32.

Thanks for the PR. It would be very useful to have those tests. Potentially can create a Blob object similar to KZGCommitment and KZGProof which can encapsulate the logic for handling the bytes.

@jtraglia Ignore the above. Blob object is a bad idea, since it will add more boxing/unboxing. The compute proof must be fixed now in the latest commit. I modified the flatten methods and also updated the trusted setup.

[kevaundray]

Adding a note to change the trusted_setup being used here to the one being used in the consensus-specs. It won't produce any errors, but it makes life easier if everyone is using the same trusted setup file.

Assosciated PR to change it in the c-kzg repo: ethereum/c-kzg-4844#33

[jtraglia]

@jtraglia Ignore the above. Blob object is a bad idea, since it will add more boxing/unboxing. The compute proof must be fixed now in the latest commit. I modified the flatten methods and also updated the trusted setup.

No worries, I understand. I can verify that it is indeed fixed. Also, I moved some things around before I noticed your other PR which made some difficult conflicts but I fixed those. I think it makes sense to put the trusted setups in test fixtures so both source trees can use them. I added the setup for minimal too, in case we want to test that later.

[jtraglia]

So why is :ethereum:spec an illegal dependency? What can I do about this?

[ajsutton]

So why is :ethereum:spec an illegal dependency? What can I do about this?

Because infrastructure modules are the bottom layer of the stack so they can only depend on other infrastructure modules ore external dependencies. ethereum sits on top of that so ethereum can depend on infrastructure but not the other way around. There's more rearranging to be done to get the whole stack tidied up but having a clear "stack" like this makes it much easier to avoid circular dependencies and other weird interactions between modules.

[jtraglia]

Okay thanks for explaining. That makes sense. I'll see what I can do to fix it tomorrow.

[StefanBratanov]

Hi LGTM. Got one nit. Why are the tests suffixed throwsExpected? In my mind it's a mix of successful and failed calls. (since the kzg library checks if the passed parameters are canonical, so very likely most will fail, but some may pass).

[jtraglia]

Thanks. The ThrowsUnexpected suffix is due to me not really knowing what to call them. The test will only fail if the method throws an exception that isn't in that list.

How about using the "fuzz" keyword instead? One of these:

• fuzzVerifyKZGCommitmentsAgainstTransactions
• fuzz_verifyKZGCommitmentsAgainstTransactions

[StefanBratanov]

Thanks. The ThrowsUnexpected suffix is due to me not really knowing what to call them. The test will only fail if the method throws an exception that isn't in that list.

How about using the "fuzz" keyword instead? One of these:

• fuzzVerifyKZGCommitmentsAgainstTransactions
• fuzz_verifyKZGCommitmentsAgainstTransactions

I like fuzz. :) The camel case looks good.

[jtraglia]

Good call, I like that better too. Thanks!

[StefanBratanov]

Good call, I like that better too. Thanks!

(y) Thanks for the PR again. The property tests bring more confidence in the code. Gonna merge now.