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

Integrate PeerDAS cryptography library #62

Merged
merged 18 commits into from
Jul 8, 2024

Conversation

kevaundray
Copy link

PR Description

On top of c-kzg, we plan to have a Rust library that compiles to Java and all of the other targets. A preliminary release has been made, this PR is being made to show the end to end integration of the library and possibly surface any issues.

Fixed Issue(s)

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.

@kevaundray kevaundray changed the title Chore: Integrate PeerDAS library Chore: Integrate PeerDAS cryptography library Jun 26, 2024
@kevaundray kevaundray changed the title Chore: Integrate PeerDAS cryptography library Integrate PeerDAS cryptography library Jun 26, 2024
@kevaundray kevaundray changed the base branch from master to das June 26, 2024 21:09
@kevaundray
Copy link
Author

Note: Things like namespaces will change in the future

@kevaundray
Copy link
Author

One thing to note:

  • This repo currently uses an old API for c-kzg where you are able to recover the Blob. The new consensus-specs does not allow this, it has become encapsulated in a method called recoverCellsAndproofs which returns all of the cells and the proofs for them.

@kevaundray
Copy link
Author

Noting the use of BeforeEach for the setup method in tests. This will implicitly test that loading the same trusted setup does not throw or break. This is in addition to the explicit testKzgLoadSameTrustedSetupTwice_shouldNotThrowException function.

See here:

*BeforeEach as opposed to BeforeAll

- change namespace and ensres that the library is loaded once
@kevaundray
Copy link
Author

Currently to run tests with the rust lib, you can do ./gradlew :infrastructure:kzg:testWithPeerDas , the end solution may look something like ./gradlew test -D use.peer.das not really sure what the teku convention is here, so open to whatever is best

@kevaundray kevaundray marked this pull request as ready for review June 27, 2024 14:23
@kevaundray
Copy link
Author

Something I noticed, some methods have try-catch logic while others (mainly the new peerdas methods) do not -- Not blocking for this PR, though it looks undesirable since you are not wrapping the potentially arbitrary exceptions from the underlying crypto library into a KZGException

@kevaundray
Copy link
Author

We could also modify BlobToKZGCommitment to:

@Override
  public KZGCommitment blobToKzgCommitment(final Bytes blob) throws KZGException {

    try {
      byte[] commitmentBytes;

      if (USE_PEER_DAS) {
        commitmentBytes = peerDASinstance.blobToKZGCommitment(blob.toArrayUnsafe());
      } else {
        commitmentBytes = CKZG4844JNI.blobToKzgCommitment(blob.toArrayUnsafe());
      }

      return KZGCommitment.fromArray(commitmentBytes);
    } catch (final Exception ex) {
      throw new KZGException("Failed to produce KZG commitment from blob", ex);
    }
  }

But there are CKZG specific errors in the tests so that would require more changes than I am hoping for

zilm13 and others added 4 commits July 8, 2024 11:15
- update FLAG name from USE_PEER_DAS to `USE_RUST_KZG_LIB`
- We now initialize and free the rust kzg lib when the flag is enabled
@kevaundray
Copy link
Author

Changed the flag name, tests can now be ran with ./gradlew :infrastructure:kzg:testWithRustKZG

@zilm13
Copy link
Collaborator

zilm13 commented Jul 8, 2024

I've finished with all changes I wanted, happy to merge.
So at the end we have --Xrust-kzg-enabled command line option to enable library and separate tests for both implementation and rust with fallback variant, which will be used with flag.
I've tested with flag enabled in Kurtosis, no issues detected.

Copy link
Collaborator

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

LGTM

@kevaundray
Copy link
Author

kevaundray commented Jul 8, 2024

I've finished with all changes I wanted, happy to merge. So at the end we have --Xrust-kzg-enabled command line option to enable library and separate tests for both implementation and rust with fallback variant, which will be used with flag. I've tested with flag enabled in Kurtosis, no issues detected.

Thanks for doing the heavy lifting to separate the two libraries into two different files and adding a flag to the command line, and for adding 4844 fallback methods for the Rust lib!

@zilm13 zilm13 merged commit df8605a into Nashatyrev:das Jul 8, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants