Conversation
| inputs.extend_from_slice(&p2.x.components); | ||
| inputs.extend_from_slice(&p2.u.components); | ||
| let mut outputs = ECAddHomog::apply(self, &inputs); | ||
| // plonky2 expects all gate constraints to be satisfied by the zero vector. |
There was a problem hiding this comment.
This is very interesting. Could you share some plonky2 code reference so that I can learn more about this?
Do you know how they handle this case in the Poseidon gate?
There was a problem hiding this comment.
I found some references about this. Could you add this information as comments?
Each Gate can do multiple operations, and in that case the gate will register one generator per operation. When a gate operation is used, the CircuitBuilder tracks this information via the current_slots field. Finally when building the circuit the generators associated to unused operations are removed! https://github.com/0xPolygonZero/plonky2/blob/82791c4809d6275682c34b926390ecdbdc2a5297/plonky2/src/plonk/circuit_builder.rs#L1210
Since the generator for the unused operation is removed, no witness value will be calculated for it, and then it will be filled with the default value which is zero: https://github.com/0xPolygonZero/plonky2/blob/82791c4809d6275682c34b926390ecdbdc2a5297/plonky2/src/iop/witness.rs#L377
So a gate with multiple operations needs to pass the constraints for a single operation when all it's witness wire values are zero.
It would be nice if gates could define padding values so that unused operations can have their witness wires assigned to those values instead of zero, allowing for more flexibility in multi-operation gates...
| let u1 = QuinticTensor::from_base(wires[5..10].try_into().unwrap()); | ||
| let x2 = QuinticTensor::from_base(wires[10..15].try_into().unwrap()); | ||
| let u2 = QuinticTensor::from_base(wires[15..20].try_into().unwrap()); | ||
| let out = add_homog_offset(x1, u1, x2, u2); |
There was a problem hiding this comment.
The gate is named ECAddHomog but the implementation does an addition with an offset (that needs to be corrected). Could you document this behavior?
Co-authored-by: Eduard S. <eduardsanou@posteo.net>
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
I've completed my review of all the changes
Co-authored-by: Eduard S. <eduardsanou@posteo.net>
Co-authored-by: Eduard S. <eduardsanou@posteo.net>
ed255
left a comment
ed255
left a comment
There was a problem hiding this comment.
LGTM! Thanks for addressing all the requests I made!
I'll approve the PR but please could you add some documentation about the offset in the ECAddHomog? See #236 (comment)
3 participants
No description provided.