feat: add remaining constraints in aggchain proof #84
Conversation
There was a problem hiding this comment.
Not a full review yet. The parts I went through look good. The main point is how we integrate the vkey hash constant with #83.
| /// Hardcoded hash of the "aggregation vkey". | ||
| /// NOTE: Format being `hash_u32()` of the `SP1StarkVerifyingKey`. | ||
| #[cfg(target_os = "zkvm")] | ||
| pub const AGGREGATION_VKEY_HASH: Vkey = [0u32; 8]; // TODO: to put the right value |
There was a problem hiding this comment.
In order to check the vkey hash before the proof checking (as done in #83), this constant would have to be available publicly. Either by exporting it from this crate or by moving it into some sort of -common crate. Also, it may be worth considering making the constant of the type VKeyHash, also introduced in #83.
I appreciate that if merging this PR is time sensitive, just exporting it from the crate as is might be the most viable option for now.
| if !self.l1_head_inclusion_proof.verify( | ||
| self.l1_info_tree_leaf.1.hash(), | ||
| self.l1_info_tree_leaf.0, | ||
| self.l1_info_root, | ||
| ) { |
There was a problem hiding this comment.
Tiny style nit. I prefer to avoid multi-line if conditions.
| if !self.l1_head_inclusion_proof.verify( | |
| self.l1_info_tree_leaf.1.hash(), | |
| self.l1_info_tree_leaf.0, | |
| self.l1_info_root, | |
| ) { | |
| let inclusion_proof_valid = self.l1_head_inclusion_proof.verify( | |
| self.l1_info_tree_leaf.1.hash(), | |
| self.l1_info_tree_leaf.0, | |
| self.l1_info_root, | |
| ); | |
| if !inclusion_proof_valid { |
| /// Verify the claimed global indexes extracting the unset global indexes | ||
| /// are equal to the Constrained global indexes. |
There was a problem hiding this comment.
| /// Verify the claimed global indexes extracting the unset global indexes | |
| /// are equal to the Constrained global indexes. | |
| /// Verify that the claimed global indexes extracting the unset global indexes | |
| /// are equal to the Constrained global indexes. |
| .inserted_gers | ||
| .iter() | ||
| .map(|inserted_ger| inserted_ger.ger()) | ||
| .collect(); |
There was a problem hiding this comment.
Are they always in the same order, do you need to sort them here for comparison?
|
|
||
| impl StaticCallWithContext { | ||
| /// Returns the decoded output values of a static call. | ||
| pub fn execute_static_call<C: SolCall>( |
There was a problem hiding this comment.
I think this may now just be called execute since StaticCall is already a part the type name.
| self.verify_inserted_gers() | ||
| } | ||
| } | ||
|
|
||
| /// Check that the rebuilt hash chain is equal to the new hash chain. | ||
| fn rebuild_hash_chain( |
There was a problem hiding this comment.
I am not completely sold on the rebuild_ part of the name since this also does verification.
| // Iterate over values and remove (skip) one occurrence for each removed value | ||
| values | ||
| .iter() | ||
| .cloned() |
There was a problem hiding this comment.
I think this may be demoted to copied
| .cloned() | |
| .copied() |
| .cloned() | ||
| .filter(|value| { |
There was a problem hiding this comment.
Probably does not make much difference since the value is Copy anyway so cloning it is supposed to be cheap. However in general, I'd suggest filtering first and then applying map-like operations if practical.
Description
optimisticModeaggchain_paramsexpressionFixes #76
PR Checklist: