[Fix] Avoid ejecting the scalar value when loading and storing a circuit. #2534
Conversation
| @@ -169,6 +169,25 @@ pub struct CastOperation<N: Network, const VARIANT: u8> { | |||
| } | |||
|
|
|||
| impl<N: Network, const VARIANT: u8> CastOperation<N, VARIANT> { | |||
| /// Initializes a new `cast` instruction. | |||
| #[inline] | |||
| pub fn new(operands: Vec<Operand<N>>, destination: Register<N>, cast_type: CastType<N>) -> Result<Self> { | |||
There was a problem hiding this comment.
Is this new method only use for testing purposes?
If so, I would advise as an independent reviewer that this method be denoted as CastOperation::new_testing_only with a #[cfg(test)] guard on it.
| // We do a special check for scalar values, as there is a possibility of an overflow via | ||
| // field to scalar conversion in deployment verification. | ||
| match register_type_is_scalar && circuit_value_is_scalar { | ||
| true => {} |
There was a problem hiding this comment.
The comment above states that We do a special check for scalar values however there is no check here on L174 for the scalar case. Can you clarify what exactly should be checked here?
| // We do a special check for scalar values, as there is a possibility of an overflow via | ||
| // field to scalar conversion in deployment verification. | ||
| match register_type_is_scalar && circuit_value_is_scalar { | ||
| true => {} |
There was a problem hiding this comment.
The comment above states that We do a special check for scalar values however there is no check here on L107 for the scalar case. Can you clarify what exactly should be checked here?
| match register_type_is_scalar && circuit_value_is_scalar { | ||
| true => {} | ||
| false => { | ||
| stack.matches_register_type(&circuit::Eject::eject_value(&circuit_value), ®ister_type)? |
There was a problem hiding this comment.
What about other types that might have scalars internally?
e.g. Structs that have scalar elements or arrays of scalars?
This PR avoids ejecting the scalar value when loading and storing a circuit.
Failing Case
This issue was found in a deployed program.
Suppose that we have a program
castoperation from afieldto ascalar.Suppose that
r0is a field element that is larger than the scalar modulus.When the
castinstruction is executed, constraints are added to check that the casted scalar value is correct.During deployment, these constraints are not checked for satisfaction (by design), and an invalid scalar element is stored into the registers.
store_circuitejects the stored value and checks for type equality.However, the implementation of
EjectforScalarrequires that the element is a valid a scalar and panics otherwise.Solution
The eject is done to check that the type of the stored value is correct.
To avoid this failing case, we directly check for type equality against the circuit value.
Testing
This PR includes an integration test that enumerates all
LiteralTypecombinations ofcastandcast.lossyand tests them in a program context.CI is running in this branch.
Considerations
We considered alternate solutions including:
castoperation onscalarsScalar::ejectdoes not failhowever, these solutions would either require a migration or impact a large portion of the codebase.