feat: implement getProof verifier and testing prover#521
feat: implement getProof verifier and testing prover#521antoniolocascio wants to merge 8 commits intodevfrom
Conversation
antoniolocascio
force-pushed
the
alocascio-get-proof
branch
4 times, most recently
from
92eb500 to
0e71c0d
Compare
basic_system/src/system_implementation/flat_storage_model/get_proof.rs
Outdated
Show resolved
Hide resolved
basic_system/src/system_implementation/flat_storage_model/get_proof.rs
Outdated
Show resolved
Hide resolved
basic_system/src/system_implementation/flat_storage_model/get_proof.rs
Outdated
Show resolved
Hide resolved
| let commitment = | ||
| compute_state_commitment(&state_root, &response.state_commitment_preimage); | ||
| if &commitment != expected_batch_hash { |
There was a problem hiding this comment.
Nit: Are extra hash operations not a concern? This fragment could be reworked to call compute_state_commitment once and then just compare recovered state root hashes on subsequent iterations. Then again, if the number of hashing operations a concern, having separate Merkle paths for storage slots is suboptimal on its own 🙃
There was a problem hiding this comment.
Doubt that this extra hashing will be a concern, but I've implemented your suggestion. I think this method will largely be used for single proofs, so I guess I didn't consider the batching optimization too much
antoniolocascio
force-pushed
the
alocascio-get-proof
branch
from
0e71c0d to
4628c3e
Compare
antoniolocascio
force-pushed
the
alocascio-get-proof
branch
from
e8c204f to
fb3b939
Compare
Benchmark report
|
| #[cfg_attr(feature = "serde", serde(with = "serde_hex::u64"))] | ||
| index: u64, | ||
| #[cfg_attr(feature = "serde", serde(with = "serde_hex::bytes32"))] | ||
| value: [u8; 32], | ||
| #[cfg_attr(feature = "serde", serde(rename = "nextIndex"))] | ||
| #[cfg_attr(feature = "serde", serde(with = "serde_hex::u64"))] | ||
| next_index: u64, | ||
| #[cfg_attr(feature = "serde", serde(with = "serde_hex::vec_bytes32"))] | ||
| siblings: Vec<[u8; 32]>, |
There was a problem hiding this comment.
It's possible to reuse these 4 fields with LeafWithProof via #[serde(flatten)]. I've (temporarily) modeled API types like this in my PR.
| let mut index = index; | ||
| for path in path.iter() { | ||
| let path: &[u8; 32] = path; | ||
| let (left, right) = if index & 1 == 0 { |
There was a problem hiding this comment.
Bikeshedding: Don't really have time to play much with the disaassembler / Godbolt, but AFAIU, index % 2 == 0 (and index /= 2 below) would be transformed to binary and / shift right anyway, and they would look more human-readable.
| return Ok(alloc::vec![]); | ||
| } | ||
|
|
||
| let empty_hashes = compute_empty_hashes::<N, H>(hasher); |
There was a problem hiding this comment.
JFYI, the server implementation goes one step further and has empty subtree hashes precomputed once on program initialization. (It could be possible to go even further and embed the hash values as a constant, e.g. covering its validity via a dedicated unit test.) Doesn't matter much from the server integration perspective.
| pub trait ZksGetProofHasher { | ||
| fn update(&mut self, input: impl AsRef<[u8]>); | ||
| fn finalize_reset(&mut self) -> [u8; 32]; | ||
| } |
There was a problem hiding this comment.
- JFYI, the server also has a hasher trait, and it's more high-level. Subjectively, it's easier to work with, but I'm obviously biased as the author 🙃
- Bikeshedding:
impl AsReflooks overly complicated; justinput: &[u8]would suffice (and make the trait dyn-compatible, although that's not a requirement).
There was a problem hiding this comment.
I basically copied the one in our crypto crate. Will take a look at it tomorrow, thanks!
2 participants
What ❔
Add verifier for zks_getProof to the API and a testing prover. Main prover will live in the server.
Why ❔
Is this a breaking change?
Checklist