Implement a wrapper around c-kzg jni bindings #6520
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| .loadBytes(path) | ||
| .orElseThrow(() -> new FileNotFoundException("Not found")); | ||
|
|
||
| File temp = File.createTempFile("resource", ".tmp"); |
Check warning
Code scanning / CodeQL
Local information disclosure in a temporary directory
| * @param path a path to a trusted setup file in filesystem | ||
| * @throws KzgException if file not found or arguments from file are incorrect | ||
| */ | ||
| void loadTrustedSetup(final String path) throws KzgException; |
There was a problem hiding this comment.
we don't use final in interfaces
| * @param blobs Blobs | ||
| * @return the aggregated proof | ||
| */ | ||
| KZGProof computeAggregateKzgProof(List<Bytes> blobs); |
There was a problem hiding this comment.
shouldn't we have throws KzgException; on all methods here?
| return kzgImpl.verifyKzgProof(kzgCommitment, z, y, kzgProof); | ||
| } | ||
|
|
||
| private static String copyResourceToTempFile(final String path) throws IOException { |
There was a problem hiding this comment.
could we pass-through the file if it is a local path?
There was a problem hiding this comment.
yes, but I guess we will need absolute path
also how could I recognize it?
There was a problem hiding this comment.
is you switch path from String to URI and then getScheme().equalsIgnoreCase("file") ?
There was a problem hiding this comment.
Thank you
also I decided to pass URL there instead of String, so it's clear that it comes with scheme and not just path
| KZG.loadTrustedSetup(resourcePath); | ||
| KZG.resetTrustedSetup(); | ||
| } | ||
|
|
There was a problem hiding this comment.
We could add another test named like testKzgLoadTrustedSetupTwice to verify that we cannot load a setup without resetting the previous one
|
Worth also checking what happens if we call the kzg functions without having previously loaded any setup |
| throws KzgException { | ||
| try { | ||
| return CKzg4844JNI.verifyAggregateKzgProof( | ||
| blobs.stream().reduce(Bytes::wrap).orElseThrow().toArray(), |
There was a problem hiding this comment.
empty blobs and commitments are actually a thing: ethereum/consensus-specs#3093
I think we should cover the case here and in tests
There was a problem hiding this comment.
yeah, doing it already, nice catch!
PR Description
I'm pretty unsure on everything here, but need to start from something
Some notes:
KZGLoaderhereminimalsetup anywhereFixed Issue(s)
Fixes #6470
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog