chore: Integrate PeerDASKZG library #6396
Conversation
build_peerdas_lib.sh
Outdated
| @@ -0,0 +1,57 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
In the rust library, we call ./scripts/compile.sh and it will compile the rust code for the host platform and copy the static libs into the nim directory. From thereon, you can call the nim code as if it were regular nim code. This script:
- clones the rust code
- calls ./scripts/compile.sh
- Copies the nim directory to the destination folder
- deletes the rust code since we no longer need it
| @@ -12,6 +12,7 @@ import | |||
| std/json, | |||
| yaml, | |||
| kzg4844/kzg_ex, | |||
| ../../../vendor/nimpeerdaskzg/nim_peerdas_kzg/nim_peerdas_kzg, | |||
There was a problem hiding this comment.
A hack right now, to reference the peerdas-kzg lib
| @@ -60,12 +64,13 @@ proc runBlobToKzgCommitmentTest(suiteName, suitePath, path: string) = | |||
| if blob.isNone: | |||
| check output.kind == JNull | |||
| else: | |||
| let commitment = blobToKzgCommitment(blob.get) | |||
| var blobObject = Blob(bytes: blob.get) | |||
There was a problem hiding this comment.
Not happy with needing this; will modify the API so that this can take types like array
There was a problem hiding this comment.
Seems that the c-kzg library also needed this: #6403
Not sure why it errors on the rust, pre gcc-14 but not on the c-kzg lib pre gcc-14
| check: | ||
| if commitment.isErr: | ||
| output.kind == JNull | ||
| else: | ||
| commitment.get == fromHex[48](output.getStr).get | ||
| commitment.get.bytes == fromHex[48](output.getStr).get |
There was a problem hiding this comment.
same comment here, re https://github.com/status-im/nimbus-eth2/pull/6396/files#r1661590547
| @@ -60,12 +64,13 @@ proc runBlobToKzgCommitmentTest(suiteName, suitePath, path: string) = | |||
| if blob.isNone: | |||
| check output.kind == JNull | |||
| else: | |||
| let commitment = blobToKzgCommitment(blob.get) | |||
| var blobObject = Blob(bytes: blob.get) | |||
| let commitment = ctx.blobToKzgCommitment(blobObject) | |||
There was a problem hiding this comment.
Just as a reference, if this branch is to be merged into stable, we probably do not want to modify this method since its used in 4844
| block: | ||
| ctx = newKZGCtx() |
There was a problem hiding this comment.
This will be modified in the future to allow one to say how many threads the library should use
There was a problem hiding this comment.
In general, Nimbus except for a couple specific cryptography libraries doesn't use threads -- would these threads be wholly contained within the KZG library?
There was a problem hiding this comment.
Yep they would be wholly contained in the cryptography library, you can set the number to 1 to use a single thread -- I can also look into making it a compilation flag in the future
There was a problem hiding this comment.
That's fine, then -- we also have
nimbus-eth2/beacon_chain/conf.nim
Lines 264 to 267 in cac63a3
already to configure the number of threads to be modified by the end-user, so could potentially hook into that.
It's already used for BLS signature verification threads, though, so one would have to consider how to keep it from becoming a misleading or double-counted setting
|
After running the build script, kzg tests are currently being ran with |
|
Most of the skeleton is now here, setting to ready to get feedback |
|
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 |
|
Currently going to work on a different strategy to allow an easier way to integrate and release the library |
|
@tersec can you re-run CI please? |
done |
|
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 |
Co-authored-by: Agnish Ghosh <[email protected]>
Co-authored-by: Agnish Ghosh <[email protected]>
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: crate-crypto/rust-eth-kzg#44