Conversation
|
Not a review, just a general comment about the approach. This PR leaks information about the prover into the executor, which, in my opinion, results in a worse separation of concerns. Are we happy to do this? |
This is necessary to run the prover with openvm precompile circuit. no matter merge or not, we are need to use this. |
# Conflicts: # src/precompile/blake2.rs # src/precompile/bn128.rs # src/precompile/hash.rs # src/precompile/modexp.rs
I had the same concern. @lispc agreed but he said currently it's not easy to do, so to avoid delaying the upgrade, we should merge this, and maybe refactor later. |
I agree with your proposal. Let's merge as is and then look to refactor in the future. One way we can approach this is by extracting |
frisitano
left a comment
frisitano
left a comment
There was a problem hiding this comment.
Looks good. I'm mindful that this feature is light on testing. Do you have any ideas about how we can test the different implementations (feature flag configurations) to ensure all implementations are consistent and correct?
|
oh @frisitano Peter just suggested another solution for this. He suggest inside zk guest program, we just replace the "handler" of Precompiles struct. So using that way, it is more easy to maintain. We are trying that method. So let us hold this PR for a while. Likely we don't need to add the openvm feature and codepath here. |
I think this is the cleanest approach, but you may need to make changes to the reth |
yes. cc @lightsing |
| if input.len() < FQ_LEN { | ||
| Err(PrecompileError::Bn128FieldPointNotAMember) | ||
| } else { | ||
| Fp::from_be_bytes(&input[..32]).ok_or(PrecompileError::Bn128FieldPointNotAMember) | ||
| } |
There was a problem hiding this comment.
| if input.len() < FQ_LEN { | |
| Err(PrecompileError::Bn128FieldPointNotAMember) | |
| } else { | |
| Fp::from_be_bytes(&input[..32]).ok_or(PrecompileError::Bn128FieldPointNotAMember) | |
| } | |
| if input.len() < FQ_LEN { | |
| Err(PrecompileError::Bn128FieldPointNotAMember) | |
| } | |
| Fp::from_be_bytes(&input[..32]).ok_or(PrecompileError::Bn128FieldPointNotAMember) |
| Bn254::msm(&[fr], &[p]) | ||
| } | ||
|
|
||
| /// pairing_check performs a pairing check on a list of G1 and G2 point pairs and |
There was a problem hiding this comment.
| /// pairing_check performs a pairing check on a list of G1 and G2 point pairs and | |
| /// Performs a pairing check on a list of G1 and G2 point pairs and |
| if pairs.is_empty() { | ||
| return true; | ||
| } | ||
| let (g1_points, g2_points): (Vec<_>, Vec<_>) = pairs |
There was a problem hiding this comment.
| let (g1_points, g2_points): (Vec<_>, Vec<_>) = pairs | |
| let (g1_points, g2_points) = pairs |
| p * fr | ||
| } | ||
|
|
||
| /// pairing_check performs a pairing check on a list of G1 and G2 point pairs and |
There was a problem hiding this comment.
| /// pairing_check performs a pairing check on a list of G1 and G2 point pairs and | |
| /// Performs a pairing check on a list of G1 and G2 point pairs and |
| result.into_affine() | ||
| } | ||
|
|
||
| /// pairing_check performs a pairing check on a list of G1 and G2 point pairs and |
There was a problem hiding this comment.
| /// pairing_check performs a pairing check on a list of G1 and G2 point pairs and | |
| /// Performs a pairing check on a list of G1 and G2 point pairs and |
|
@lightsing is this still relevant or can we close this now? |
5 participants
This pr adds openvm precompile support behind a
openvmgate.Some files are copied from
revmwith almost no modification sincerevmdid expose those as public, should be easy to update.openvmsupport are copied fromaxiom-crypto/revm.Also refactors some constant definition avoiding magic number.