search

ethereum / c-kzg-4844

A minimal implementation of the Polynomial Commitments API for EIP-4844 and EIP-7594, written in C.
Apache License 2.0
118 stars 109 forks source link

[rust] Add compute_* verify_* functions for KzgSettings #413

Closed mattsse closed 3 months ago

mattsse commented 7 months ago

currently, all KzgProof functions take KzgSettings as argument:

https://github.com/ethereum/c-kzg-4844/blob/1ef67c17bf659547749750e6ebaaf09563e0149d/bindings/rust/src/bindings/mod.rs#L321-L325

for convenience, all of those could also be implemented for KzgSettings directly that delegate with &self

Since these are new functions, this would make it possible to change the API and accept fixed arrays directly instead of the c-kzg Bytes wrapper types, which would be even more convenient.

jtraglia commented 7 months ago

I think this is a good idea. @asn-d6 @pawanjay176 what do y'all think about this?

mattsse commented 7 months ago

perhaps a bit more context re the last part, building the c-kzg crate can be a problem on certain platforms, hence in alloy we'd like to use alloy-primitives, which are also just array wrappers:

https://github.com/alloy-rs/alloy/blob/1fc24a7545ad51f26b09747a3c7c2b204a56c869/crates/consensus/src/transaction/eip4844.rs#L963-L970

so we can use the type without needing c-kzg, they have the same array repr and could be casted to the c-kzg, but ideally we can move this step behind the KzgSettings API so that we can verify types that are AsRef<[]> for example.

asn-d6 commented 7 months ago

currently, all KzgProof functions take KzgSettings as argument:

https://github.com/ethereum/c-kzg-4844/blob/1ef67c17bf659547749750e6ebaaf09563e0149d/bindings/rust/src/bindings/mod.rs#L321-L325

for convenience, all of those could also be implemented for KzgSettings directly that delegate with &self

No strong opinion about this change.

Gotta say that it seems a bit arbitrary to me to consider compute_kzg_proof() a method of KZGSettings. Because in a similar way, I could think of it as a method of Blob.

That said, if this makes people happy, I don't have a strong opinion about it. I think Justin also likes it because it will inadvertently also group all the public functions together.

Since these are new functions, this would make it possible to change the API and accept fixed arrays directly instead of the c-kzg Bytes wrapper types, which would be even more convenient.

Hm, this seems like an unrelated and more complicated change. I would make a separate issue for it.

The Bytes32 struct right now is part of bindgen's generated code, and we have also written a bunch of helper methods on top of it. How would it look like to make the API accept fixed arrays? I cannot easily visualize the change and understand if it simplifies or makes things more complicated.