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

Review KZG Interface & Integration / Integrate pure JS KZG #3662

Closed
holgerd77 opened this issue Sep 12, 2024 · 16 comments
Closed

Review KZG Interface & Integration / Integrate pure JS KZG #3662

holgerd77 opened this issue Sep 12, 2024 · 16 comments

Comments

@holgerd77
Copy link
Member

Thanks to @paulmillr there is now a pure JS KZG implementation available here within the micro-eth-signer library! 🎉

We should take this occasion and generally review our interface definition from Util which we use to type the CustomCrypto input in Common (for re-use e.g. in the EVM) or also the 4844 txs.

Guess this (the Kzg interface) was very much designed with our own code (respectively the code we had/have from kzg-wasm) in mind. We should see if all makes sense there + is necessary. I wonder e.g. if loadTrustedSetup() really needs to be in the interface? Or is this a left-over from when we called this ourselves?

Another thing is that we might e.g. want to make this verifyBlobKzgProofBatch() batch function optional and rather adjust our implementation so that if the method is not available the single verification method is called.

Generally we likely want to coordinate with @paulmillr here so that we get to a practical and matching interface. Not sure if we still need to provide some small wrapper here at the end (because the APIs remain slightly different), but that would also be ok I guess.

At the end we should also adjust our KZG documentation like the Setup section in the tx library and generalize for both the JS and WASM versions.

And lastly we should also give testing some additional thought (might be a separate PR/also someone else from the team), and see that both the JS and WASM versions are tested for both the tx library and the EVM in our CI.

Will assign @acolytec3 here since he has done most of the KZG work and is likely best suited to work on this integration.

@paulmillr
Copy link
Member

@acolytec3
Copy link
Contributor

I'll pick this up either today or next week. Will try and look at some benchmarks to see if there's much difference between the pure JS version and wasm along the way.

@paulmillr
Copy link
Member

Trusted setups are available in #3670

You can test them out using this:

git clone git@github.com:paulmillr/trusted-setups.git
cd trusted-setups
npm install && npm run build && npm pack
mv trusted-setups-0.1.0.tgz <repository>
cd <repository>
npm install ./trusted-setups-0.1.0.tgz

@acolytec3
Copy link
Contributor

acolytec3 commented Sep 16, 2024

@paulmillr Is there a bug in how the fast setup is constructed? I followed the above instructions as far as building and then installed locally via npm link and then using this code to import:

import { trustedSetup as fast } from 'trusted-setups/fast.js'

const fastKZG = new KZG(fast)

and it's giving this error:

Error: pointHex must be valid hex string, got "00413c0dcafec6dbc9f47d66785cf1e8c981044f7d13cfe3e4fcbb71b5408dfde6312493cb3c1d30516cb3ca88c03654 1690c1ade165e7c0b1fbdd0dc7ce71a8cfccbb16708de5164b32f31166b7a6bed225d39038457e05214cfda6f567b61c". Cause: Error: padded hex string expected, got unpadded hex of length 193
 ❯ ensureBytes ../../node_modules/@noble/curves/src/abstract/utils.ts:128:13
 ❯ Function.fromHex ../../node_modules/@noble/curves/src/abstract/weierstrass.ts:415:44
 ❯ parseG1 ../../node_modules/micro-eth-signer/src/kzg.ts:144:39
 ❯ new KZG ../../node_modules/micro-eth-signer/src/kzg.ts:135:35

The "slow" setup seems to load. Going to start testing outputs against kzg-wasm today.

@paulmillr
Copy link
Member

«fast” var must have the property

encoding === 'fast_v1'

https://github.com/paulmillr/micro-eth-signer/blob/17183a114ab581a11710f197345a02a655ee540c/src/kzg.ts#L132

you can add it manually to fast.js for now

@acolytec3
Copy link
Contributor

#3674 Here's a start. Looks like everything works (pending full CI run)

@paulmillr is it necessary to use strings for all of the inputs for the kzg library? I don't know enough about the inner workings of the kzg code in micro-eth-signer to know if it's more/less efficient for your library to use strings versus Uint8Arrays. In our code, we generally have all the fields in Uint8Arrays and pass that to the kzg code directly.

Because your library uses strings for the inputs/outputs, we do conversions from bytes to strings for each input for your kzg library and then back from strings to bytes for the outputs in our code.

I don't have a strong opinion other than the fact that we already use Uint8Arrays and it'd be nice to not have to do conversions coming and going since that will have a performance drag (though I suspect not major since we don't generate/verify KZG proofs as a major bottleneck in our code).

@paulmillr
Copy link
Member

The strings are converted to bigint, which is fast. Converting Uint8Arrays to bigints would in any case be done via strings.

The question here what's more correct. E.g. spec-wise or something like that. How are you building such Uint8Arrays in your library? Are you just "passing" data around, without building it?

@acolytec3
Copy link
Contributor

We generally deal with everything as Uint8Arrays since we treat the kzg-wasm package as a black box and just pass bytes in and get the correct sized responses back as bytes.

@acolytec3
Copy link
Contributor

We will build the blob from whatever underlying data a tx submitter provides but everything else from there on is just bytes. The kzg-wasm library does all of the serializing and deserializing of data under the hood and just gives us the proofs/commitments and we really on their results for proof verification

@acolytec3
Copy link
Contributor

We will build the blob from whatever underlying data a tx submitter provides but everything else from there on is just bytes. The kzg-wasm library does all of the serializing and deserializing of data under the hood and just gives us the proofs/commitments and we really on the results for proof verification.

@paulmillr
Copy link
Member

Uint8arrays can't be passed via network, they need to be serialized first into some format.

@acolytec3
Copy link
Contributor

Correct, so everything sent over the wire is sent as hex strings. So fair point to us. @holgerd77 I will look through our codebase see if there's ever a place where we "need" the KZG values (blobs/commitments/versionedHashes/proofs) as bytes or whether we can just handle them as strings.

@paulmillr
Copy link
Member

I of course can make everything U8A, just want to get to the bottom of this, whether the types they've picked out of necessity are "correct ones".

@paulmillr
Copy link
Member

The package is now live on NPM at @paulmillr/trusted-setups.

@holgerd77
Copy link
Member Author

@holgerd77
Copy link
Member Author

Closed by #3674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants