Circuits naming convention

[arnaucube]

Currently we have (using the merkletree gadget as example):

struct MerkleProofGadget { // contains the config params
  max_depth: usize,
}

impl MerkleProofGadget {
  fn eval(&self, builder) -> Result<MerkleProofTarget> {
    // logic of the gadget
  }
}

struct MerkleProofTarget {
    max_depth: usize,
    enabled: BoolTarget,
    root: HashOutTarget,
    key: ValueTarget,
    value: ValueTarget,
    existence: BoolTarget,
    siblings: Vec<HashOutTarget>,
    case_ii_selector: BoolTarget,
    other_key: ValueTarget,
    other_value: ValueTarget,
}
impl MerkleProofTarget{
  fn set_targets(&self, partial_witness, inputs) -> Result<()> {
    // assign inputs to the targets
  }
}

Then to use it we have to do:

let targets = MerkleProofGadget { max_depth }.eval(&mut builder)?;
targets.set_targets( &mut pw, true, true, tree2.root(), proof.clone(), key, value)?;

I find the eval naming confusing, since it makes it look like the method is "evaluating", when actually it is setting the targets and the gadget logic (ie. building the gadget/circuit logic).
Since the other method is set_targets, maybe an option would be using add_targets instead of eval, but not convinced; maybe something else like build_circuit (although in some cases will not be a 'circuit' but a 'gadget'). Then maybe something like build_logic or targets_logic or similar. Or, since we're already in the SomethingGadget context, can be directly SomethingGadget::build().

Also we can group the two structs to simplify a bit the interfaces, something like:

struct MerkleProofGadget {
    max_depth: usize,
    enabled: BoolTarget,
    root: HashOutTarget,
    key: ValueTarget,
    value: ValueTarget,
    existence: BoolTarget,
    siblings: Vec<HashOutTarget>,
    case_ii_selector: BoolTarget,
    other_key: ValueTarget,
    other_value: ValueTarget,
}

impl MerkleProofGadget {
  // NOTE: here the name 'build' could be 'build_logic', 'define_logic', 'define_gadget', etc
  fn build(builder, params) -> Result<Self> {
    // logic of the gadget
  }

  fn set_targets(&self, partial_witness, inputs) -> Result<()> {
    // assign inputs to the targets
  }
}

Then we would use it as

let targets = MerkleProofGadget::build(&mut builder, max_depth)?;
targets.set_targets( &mut pw, true, true, tree2.root(), proof.clone(), key, value)?;

Opening this issue to have a separate thread to discuss it, hoping to include it
in the refactor (#171).

[ed255]

• I like the name build instead of eval.
• From your proposal I would do MerkleProofTarget::build -> Result<MerkleProofTarget> instead of MerkleProofGadget::build -> Result<MerkleProofGadget>. So there would not be a FooGadget type.
• About removing the type that holds the parameters. If we only have a single parameter value (max_depth in the MerkleTree or params in other gadgets) then I'm OK with this change. A question on the order of arguments: would it be like this? CircuitBuilder, params, input targets...)?

[arnaucube]

1. cool.

2. ok, so you're suggesting renaming XXGadget by XXTarget. Then we would not have anymore the concept of 'Gadget'. But our XXTarget would not be a 'Target' but a 'collection of targets' (which is fine also).
   How would you see on having the concept of Gadget, which represents the 'collection of targets of a gadget'? so that we keep the concept of 'gadget' in our reasoning? if not, I'm happy also with XXTarget.

3. yes, if there are >1 param, then the params is used as input. On the order, agree with having input targets at the end, since is the one that in some cases will not exist, and in other cases will be multiple inputs.

[ed255]

2. ok, so you're suggesting renaming `XXGadget` by `XXTarget`. Then we would not have anymore the concept of 'Gadget'. But our `XXTarget` would not be a 'Target' but a 'collection of targets' (which is fine also).
   How would you see on having the concept of `Gadget`, which represents the 'collection of targets of a gadget'? so that we keep the concept of 'gadget' in our reasoning? if not, I'm happy also with `XXTarget`.

You're right that we would not have XXGadget types anymore.
It's true that the XXTarget would be a collection of targets, and this is following the convention from plonky2. For example see HashOutTarget: https://github.com/0xPolygonZero/plonky2/blob/6a2c1b47b7b00864c2b65a44c18099b0152f550e/plonky2/src/hash/hash_types.rs#L119

Also semantically I would claim that the gadget is the code in the build but not the returned type, which are just pointers to cells of the gadget.

[arnaucube]

Right, agree then, happy with it ^^

[arnaucube]

Been thinking a bit more on this, and I find it more natural to use XXGadget instead of XXTarget, the reasoning is that:

1. when referring to the SignatureTarget we would still informally use "the signature gadget" wording, since we refer to the circuit/gadget that performs the "logic of the signature". (this can be seen in many issues, and also in the meetings when we refer to the circuits/gadgets (never to the targets)).
   We can then just assume that any "Gadget" contains the "Targets" (as in the initial proposal from Circuits naming convention #181 (comment) ).
2. This also builds from the definition of Target at the plonky2 docs
   https://github.com/0xPolygonZero/plonky2/tree/6a2c1b47b7b00864c2b65a44c18099b0152f550e/plonky2/src/iop/target.rs#L11:

   A location in the witness.
   Targets can either be placed at a specific location, or be "floating" around,
   serving as intermediary value holders, and copied to other locations whenever needed.

So with this, I'm more inclined towards having the MerkleProofGadget as the struct that

• has the methods to build (.build()) the circuit(gadget) logic, and to assign the values to its targets (.set_targets())
• and that internally contains the Targets (which are needed for the build & set_targets methods), but kind of an internal state of the gadget

[ed255]

My concern about having a type called XXGadget that contains the Target types is that it we'd be reusing terms that plonky2 uses but with with very different meanings.
My interpretation is that the convention in plonky2 is to call XXTarget anything that contains targets, and that's a common output of functions that generate some constraints and return a result that contains targets.
Example of XXTarget https://github.com/0xPolygonZero/plonky2-ecdsa/blob/bdb6504bca250db1548cdcce2407d4e334990c33/src/gadgets/biguint.rs#L18-L20
Example of a function that constraints and returns XXTarget https://github.com/0xPolygonZero/plonky2-ecdsa/blob/bdb6504bca250db1548cdcce2407d4e334990c33/src/gadgets/biguint.rs#L183
In the plonky2-ecdsa codebase there is no type called XXGadget.

I've checked other repositories built by other developers and they also use XXTarget to name a struct that holds targets and is returned after building the constraints:

• https://github.com/polymerdao/plonky2-sha256/blob/cf88eae66b5cdfbd0f4b49a0472eb5d87b450428/src/circuit.rs#L36-L39
• https://github.com/Sladuca/plonky2-ecgfp5/blob/8c5d9c42ffb5e6eda6335e8c960432dacc4db415/src/gadgets/curve.rs#L20

Those repositories don't have a XXGadget type. Most of them don't even have a XXGate; instead they extend the CircuitBuilder with methods that take some XXTarget, constraint and return XXTarget types.

For this reason (consistency with the conventions that plonky2 developers and others are using), I'd prefer to keep the type that holds Targets called XXTarget. That's why my original proposal was to have a XXGate type that holds the configuration with a method eval that returns XXTarget, where the name Gate and eval could be changed for Gadget and build. But I know you'd prefer not having 2 types for each "gadget".

Here's another proposal: instead of having a type that has the build method, we could have just functions. Like this:

fn merkle_proof_build(builder: &mut CircuitBuilder<F, D>, max_depth: usize) -> Result<MerkleClaimAndProofTarget>
fn merkle_proof_existence_build(builder: &mut CircuitBuilder<F, D>, max_depth: usize) -> Result<MerkleProofExistenceTarget>
fn signature_verify_build(builder: &mut CircuitBuilder<F, D>) -> Result<SignatureVerifyTarget>
fn signed_pod_verify_build(builder: &mut CircuitBuilder<F, D>, params: &Params) -> Result<SignedPodVerifyTarget>

[arnaucube]

we'd be reusing terms that plonky2 uses but with with very different meanings

True, agree. Although on the same time, the codebase of plonky2 is not much active and there is not much community around it using those terms, so we would not be diverging from an active community. Still I agree that better if we can keep using those conventions.

But if we follow the plonky2 convention, then we would not have the XXTarget::build(builer...)/XXTarget::add_targets(builder...) nor the signed_pod_verify_build(builder... methods, but we would use an extension of the CircuitBuilder like CircuitBuilder.add_signed_pod_targets(). But I find this less intuitive to use (there would not be an object for each circuit that is used to build the targets (XXTargets::build()), since to build the circuit you would need to import the trait that extends the CircuitBuilder and call some method from it (CircuitBuilderExtended.add_xx_targets()), which requires more code to define the same things (need to extend the CircuitBuilder trait with another trait in our code and then implement it).

So with this, I think that we might not follow 100% the plonky2 convention. Once we assume we're not completely consistent with plonky2 convention, the matter is how far or close we are from that 😺

From all the options we've explored, I think that the proposal of #181 (comment) together with what you say in #181 (comment) (2d point) on replacing Gadget by Target might be the more comfortable to use while on the same time not going too much far from plonky2's conventions. That is

struct MerkleProofTargets {
    max_depth: usize, /* or Params if needed more */
    root: HashOutTarget,
    //... (rest of Targets)
}

impl MerkleProofTargets {
  fn build(builder: &mut CircuitBuilder<F, D>, max_depth: usize /* or Params if needed more */)
       -> Result<Self> {
    // logic of the circuit
    return Self{...} // returns `MerkleProofTargets`
  }

  fn set_targets(&self, pw: &mut PartialWitness<F>, inputs: ...) -> Result<()> {
    // assign inputs to the targets
  }
}

Probably we will still refer (informally in meetings and also in the code inline comments) to the MerkleProofTargets as the merkleproof verification circuit; but at least for the code we then have the convention on using the XXTargets naming, which also we can understand as it groups the targets of the circuit.

[arnaucube]

*I used XXTargets (plural, since it groups many targets), but probably it follows best the convention using XXTarget (singular)