status-im / nimbus-eth2

Nim implementation of the Ethereum Beacon Chain
https://nimbus.guide
Other
523 stars 227 forks source link

chore: Integrate PeerDASKZG library #6396

Open kevaundray opened 2 months ago

kevaundray commented 2 months ago

Currently in DRAFT as theres quite a bit of experimentation happening.

This is a PR to integrate the rust peerdas-kzg library into nimbus. The branch being used to make quick changes is here: https://github.com/crate-crypto/peerdas-kzg/pull/44

kevaundray commented 2 months ago

After running the build script, kzg tests are currently being ran with ./env.sh nim c -r tests/consensus_spec/test_fixture_kzg.nim

kevaundray commented 2 months ago

Most of the skeleton is now here, setting to ready to get feedback

github-actions[bot] commented 2 months ago

Unit Test Results

         9 files    1 337 suites   25m 57s :stopwatch:   5 111 tests   4 763 :heavy_check_mark: 348 :zzz: 0 :x: 21 456 runs  21 052 :heavy_check_mark: 404 :zzz: 0 :x:

Results for commit 1a3ea468.

:recycle: This comment has been updated with latest results.

kevaundray commented 2 months ago

Updated the script to not build the java bindings package -- this requires javac to generate a java header file

For windows, it seems to be having trouble picking up the linker -- will try to reproduce this environment

kevaundray commented 2 months ago

Currently going to work on a different strategy to allow an easier way to integrate and release the library

kevaundray commented 2 months ago

@tersec can you re-run CI please?

tersec commented 2 months ago

@tersec can you re-run CI please?

done

kevaundray commented 2 months ago

The error on linux seems to stem from the fact that linux has a different stack limit than the others -- The place where it is happening is not somewhere this PR has modified, so unclear why it did not pop up in the parent branch.

The reason why it is happening, is possibly due to ComputeCellsAndProofs method computing 819232 + 12848 = 268KB onto the stack for each call. If this is the case, I don't think this should be fixed in this PR as its already present on the parent branch and not related to integration