Open adr1anh opened 6 months ago
The inputs
here should not be an Option
. The None
variant is only used to facilitate the generation of public parameters. However, it is easy to add a dummy
method to NovaAugmentedCircuitInputs
that generates an appropriate set of inputs (even if most of these are None, and if the inputs are not actually valid). The setup
function the public parameters would simply create a set of dummy inputs with the following function:
impl<E: Engine> NovaAugmentedCircuitInputs<E> {
/// Create dummy inputs/witness for the verification circuit, to be used for constraint synthesis
pub fn dummy(
arity: usize
) -> Self {
Self {
params: E::Base::ZERO,
I: E::Base::ZERO,
z0: vec![E::Base::ZERO; arity],
zi: None,
U: None,
u: None,
T: None,
}
}
}
In general for circuits, we should be able to generate "dummy" input which allow the circuit to be called even for constraint generation. These are often cheap to generate, and avoid having to call self.inputs.get()?
over every item in the struct.
For gadgets, it would be nice to have equivalent alloc_infallible
methods that take as input a value rather than an Option wrapped one.
One of the issues with this is conveying that the gadget is not populated with a valid value. The posterchild for this issue is https://github.com/lurk-lab/bellpepper/pull/88
A circuit/gadget needs to be written so that it can be run in many different configurations (constraint, witness, shape synthesis). In some cases, it may be impractical to generate a valid set of inputs in order to actually call the
synthesize
function.To facilitate synthesis when we are not interested in computing any witness data, it may be practical to wrap the input value with
Option
so that allocated variables can be created by passingNone
as the native value being allocated. Another reason why gadgets may chose to acceptOption
inputs is to facilitate the allocation of variables whose value depends on an existing allocated variable, which may not have a value because theConstraintSystem
is explicitly not allocating values.This unfortunately leads to a lot of boilerplate and unnecessary
clone
s when handling potentially empty witness values and checking if a value is indeedSome
. Moreover, it leads to an awkward pattern where each input needs to be checked not to beNone
when in theory, either all the inputs should beNone
, or all should beSome
.For some circuits, it can actually be very straight-forward to generate valid default inputs without much overhead. It also makes circuits easier to read and can prevent non-uniformity bugs from the implicit branches involved in unwrapping an
Option
.For example in the
CyclefoldCircuit
, we can make the following observations:Option
.false
) to pass to thesynthesize
method, even if they are ignored by the allocation closures.Another example would be the
SuperNovaAugmentedCircuitInputs
.z0
andzi
are allocated asE::Base::ZERO
,U
is just a "default" where all values are zero/point at infinityT
is the point at infinity (valid if allU
s are trivial)Option
inSuperNovaAugmentedCircuit
.Overall, we should be more careful about wrapping all circuit inputs in
Option
, and prefer allowing default or easy-to-generate "dummy" values, as this removes boilerplate, and prevents the risk of non-uniformity bugs.