feat(levm): implement precompile ecpairing #1560
Conversation
…ass/ethrex into precompiles_support_scaffolding
crates/vm/levm/src/gas_cost.rs
Outdated
| .checked_mul(34000) | ||
| .ok_or(OutOfGasError::GasCostOverflow)?; | ||
| groups_cost | ||
| .checked_add(45000) |
There was a problem hiding this comment.
Quick question Maxi, could these values be put into constants?
Would that make sense in this context?
There was a problem hiding this comment.
Done in Add constants in gas cost!
|
|
||
| #[test] | ||
| fn ecpairing_test() { | ||
| let calldata = hex::decode("2cf44499d5d27bb186308b7af7af02ac5bc9eeb6a3d147c186b21fb1b76e18da2c0f001f52110ccfe69108924926e45f0b0c868df0e7bde1fe16d3242dc715f61fb19bb476f6b9e44e2a32234da8212f61cd63919354bc06aef31e3cfaff3ebc22606845ff186793914e03e21df544c34ffe2f2f3504de8a79d9159eca2d98d92bd368e28381e8eccb5fa81fc26cf3f048eea9abfdd85d7ed3ab3698d63e4f902fe02e47887507adf0ff1743cbac6ba291e66f59be6bd763950bb16041a0a85e000000000000000000000000000000000000000000000000000000000000000130644e72e131a029b85045b68181585d97816a916871ca8d3c208c16d87cfd451971ff0471b09fa93caaf13cbf443c1aede09cc4328f5a62aad45f40ec133eb4091058a3141822985733cbdddfed0fd8d6c104e9e9eff40bf5abfef9ab163bc72a23af9a5ce2ba2796c1f4e453a370eb0af8c212d9dc9acd8fc02c2e907baea223a8eb0b0996252cb548a4487da97b02422ebc0e834613f954de6c7e0afdc1fc").unwrap(); |
There was a problem hiding this comment.
Could there be some comment / link explaining what this bytecode means/is trying to test?
There was a problem hiding this comment.
Done in Add comment explaining in test
crates/vm/levm/src/precompiles.rs
Outdated
| Ok(Bytes::from([0u8; 64].to_vec())) | ||
| } else if first_point_is_zero { | ||
| // If first point is zero, return is second point | ||
| // If first point is zero, response is second point |
There was a problem hiding this comment.
crates/vm/levm/src/precompiles.rs
Outdated
| @@ -421,27 +428,20 @@ pub fn ecadd( | |||
| .concat(); | |||
| Ok(Bytes::from(res)) | |||
| } else if second_point_is_zero { | |||
| // If second point is zero, return is first point | |||
| // If second point is zero, response is first point | |||
There was a problem hiding this comment.
crates/vm/levm/src/precompiles.rs
Outdated
| let first_point = BN254Curve::create_point_from_affine(first_point_x, first_point_y) | ||
| .map_err(|_| PrecompileError::ParsingInputError)?; | ||
| let res = [first_point.x().to_bytes_be(), first_point.y().to_bytes_be()].concat(); | ||
| Ok(Bytes::from(res)) | ||
| } else { | ||
| // If none of the points is zero, return is the sum of both in the EC | ||
| // If none of the points is zero, response is the sum of both in the EC |
There was a problem hiding this comment.
crates/vm/levm/src/precompiles.rs
Outdated
| return Err(VMError::PrecompileError(PrecompileError::ParsingInputError)); | ||
| } | ||
|
|
||
| let groups_number = calldata.len() / 192; |
There was a problem hiding this comment.
I think this variable name can be improved. Maybe something like "quantity of inputs" or "amount of inputs" would be more accurate.
There was a problem hiding this comment.
Maybe groups_amount? inputs_amount is good also.
There was a problem hiding this comment.
crates/vm/levm/src/precompiles.rs
Outdated
| increase_precompile_consumed_gas(gas_for_call, gas_cost, consumed_gas)?; | ||
|
|
||
| let mut mul: FieldElement<Degree12ExtensionField> = QuadraticExtensionFieldElement::one(); | ||
| for group_number in 0..groups_number { |
There was a problem hiding this comment.
And this would be like input index, right?
There was a problem hiding this comment.
Yes, done in Replace group terminology, use input instead
crates/vm/levm/src/precompiles.rs
Outdated
| // Infinite is defined by (0,0). Any other zero-combination is invalid | ||
| if (U256::from_big_endian(first_point_x) == U256::zero() | ||
| && U256::from_big_endian(first_point_y) != U256::zero()) | ||
| || (U256::from_big_endian(first_point_x) != U256::zero() | ||
| && U256::from_big_endian(first_point_y) == U256::zero()) |
There was a problem hiding this comment.
Isn't this like an exclusive or?
I don't really know if this is equivalent but it might be:
if (U256::from_big_endian(first_point_x) == U256::zero())
^ (U256::from_big_endian(first_point_y) == U256::zero())There was a problem hiding this comment.
crates/vm/levm/src/precompiles.rs
Outdated
| if (U256::from_big_endian(second_point_x_first_part) == U256::zero() | ||
| && U256::from_big_endian(second_point_x_second_part) != U256::zero()) | ||
| || (U256::from_big_endian(second_point_x_first_part) != U256::zero() | ||
| && U256::from_big_endian(second_point_x_second_part) == U256::zero()) |
There was a problem hiding this comment.
crates/vm/levm/src/precompiles.rs
Outdated
| let second_point_x_first_part = | ||
| group_data.get(96..128).ok_or(InternalError::SlicingError)?; | ||
| let second_point_x_second_part = | ||
| group_data.get(64..96).ok_or(InternalError::SlicingError)?; |
There was a problem hiding this comment.
This got me confused. From 64..96 we have x2 and from 96..128 we have y2 according to evm codes. Why does this look different?
There was a problem hiding this comment.
I gave it that name because of the place they occupy in the vector, maybe it is not very clear. I could give it another name, but I focused on the fact that, after all, it is a component of what will be the second point.
crates/vm/levm/src/precompiles.rs
Outdated
| if (U256::from_big_endian(second_point_y_first_part) == U256::zero() | ||
| && U256::from_big_endian(second_point_y_second_part) != U256::zero()) | ||
| || (U256::from_big_endian(second_point_y_first_part) != U256::zero() | ||
| && U256::from_big_endian(second_point_y_second_part) == U256::zero()) |
There was a problem hiding this comment.
crates/vm/levm/src/precompiles.rs
Outdated
| ) | ||
| .map_err(|_| InternalError::ConversionError)?; | ||
|
|
||
| // Check if the curve belongs to the curve (this happens if it's lower than the prime) |
There was a problem hiding this comment.
This comment is not clear enough, at least for me that I'm not very knowledgeable about this precompile. "The curve belongs to the curve"?
There was a problem hiding this comment.
Motivation
The goal is to implement the ecpairing precompile.
Description
I put a clippy allow for a *= in a math type because it is not handled like conventional multiplication (it has its own implementation).
The implementation makes pass all EF tests related.