Seperate crate for blob crypto #28
Conversation
| @@ -0,0 +1,3 @@ | |||
| # `cryptography` | |||
|
|
|||
| This crate contains bn254 logics for generating kzg proof for either client or host. This crate uses STD. | |||
There was a problem hiding this comment.
doesn't client not have access to std? or do some zkvms have access to the stdlib?
There was a problem hiding this comment.
Why not call this crate kzg-proof or something more explicit?
There was a problem hiding this comment.
crates dir contains a list of crates, some are std some are not
it does both prove and verify,. But sg.
There was a problem hiding this comment.
call it kzg-crypto
| @@ -100,27 +100,4 @@ impl<T: CommsClient + Sync + Send> EigenDABlobProvider for OracleEigenDAProvider | |||
|
|
|||
| Ok(blob.into()) | |||
| } | |||
|
|
|||
| async fn get_element(&mut self, cert: &Bytes, element: &Bytes) -> Result<Bytes, Self::Error> { | |||
There was a problem hiding this comment.
we don't need this. By having the cache module, we don't need this additional interface
| /// that provides efficient verification between the commitment and the blob | ||
| /// without actually commiting the blob | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct EigenDABlobWitness { |
There was a problem hiding this comment.
since this is the only struct in this crate cryptography sounds like not such a good name. it sounds too low level.
crates/cryptography/src/lib.rs
Outdated
| #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] | ||
| #![cfg_attr(not(test), warn(unused_crate_dependencies))] | ||
|
|
||
| pub mod witness; |
There was a problem hiding this comment.
how about exporting EigenDABlobWitness given its the only struct in here?
|
|
||
| /// This function computes a witness for a eigenDA blob | ||
| /// nitro code <https://github.com/Layr-Labs/nitro/blob/14f09745b74321f91d1f702c3e7bb5eb7d0e49ce/arbitrator/prover/src/kzgbn254.rs#L141> | ||
| /// could refactor in the future, such that both host and client can compute the proof |
There was a problem hiding this comment.
?? you mention elsewhere that this crate is meant to be used by both host and client. guess its only for host right now? good to add more details, explain why not, how it can be refactored, etc. Even you will forget these details.
| if let Err(e) = witness.push_witness(&blob, &self.srs) { | ||
| return Err(OracleProviderError::Preimage(PreimageOracleError::Other( | ||
| e.to_string(), | ||
| ))); |
There was a problem hiding this comment.
you use different ways to deal with errors in multiple places: match / if let Err(). Prob best to be consistent, and use same method everywhere. map_err(...)? is prob best
| } | ||
| }; | ||
|
|
||
| let mut witness = self.witness.lock().unwrap(); |
There was a problem hiding this comment.
propagate error instead of unwrapping which panics
| let last_commitment = witness.commitments.last().unwrap(); | ||
|
|
||
| // make sure locally computed proof equals to returned proof from the provider | ||
| if last_commitment[..32] != cert_blob_info.blob_header.commitment.x[..] | ||
| || last_commitment[32..64] != cert_blob_info.blob_header.commitment.y[..] |
There was a problem hiding this comment.
these abstractions are super leaky. Better to create abstractions that only require local reasoning. Should in principle never have to index into a struct's fields like this for validation. That should all be handled by the struct itself. Maybe pass the commitment to the push_witness function and let it validate and return an error if it doesnt validate. Think that's what austin is doing in the golang lib
bin/client/src/lib.rs
Outdated
| @@ -51,6 +57,14 @@ where | |||
| let beacon = OracleBlobProvider::new(oracle.clone()); | |||
| let eigenda_blob_provider = OracleEigenDAProvider::new(oracle.clone()); | |||
|
|
|||
| let eigenda_blob_witness = Arc::new(Mutex::new(EigenDABlobWitness::new())); | |||
| let srs = SRS::new("resources/g1.point", 268435456, 1024).unwrap(); | |||
There was a problem hiding this comment.
Why load 256MiB? Is this really needed? can we make this a configurable flag? Or is this going to change soon anyways?
There was a problem hiding this comment.
Actually now I'm confused why do we need this many points? Don't we only support up to 16MiB blobs?
| if !witness.batch_verify() { | ||
| panic!("batch verify wrong"); | ||
| } |
There was a problem hiding this comment.
rust has asserts. But why do we panic? prob should not panic and unwrap everywhere, and instead propagate errors?
bxue-l2
force-pushed
the
seperate-crate-for-blob-crypto
branch
from
df8e963 to
14aad44
Compare
This PR creates a cryptography crate that contains std library. The crate
TODO in the next PR