Different return types of `assign_advice` and `assign_advice_from_constant`

[mmagician]

As per title. While this might be intentional design, causes some confusion and inconsistencies when trying to use both in our codebase.

Some context: we're trying to change Scroll's poseidon-circuit library to point to Axiom's halo2 backend. Later, we'd like to make a custom builder that combines halo2 & halo2-lib circuit creations, inspired by zkevm-keccak (as discussed offline with @nyunyunyunyu). Then, whenever there's hashing in our circuit, use the optimized cell assignment from Scroll's adaptation that we're working on, rather than the Chip currently in community-edition that uses vertical gates under the hood. Here's our current attempt with @antonio95: https://github.com/HungryCatsStudio/poseidon-circuit/tree/axiom-backend.

We have run into the following issue: Axiom's region.assign_advice_from_constant has the following prototype

pub fn assign_advice_from_constant<VR, A, AR>(
    &mut self,
    annotation: A,
    column: Column<Advice>,
    offset: usize,
    constant: VR,
) -> Result<AssignedCell<VR, F>, Error>
where
    for<'vr> Assigned<F>: From<&'vr VR>,
    A: Fn() -> AR,
    AR: Into<String>,

This is in contrast to analogous Region methods such as

pub fn assign_advice<'v>(
  ...
) -> AssignedCell<&'v Assigned<F>, F>;

The return type of assign_advice_from_constant is causing an inconsistency in our code: we would like to be able to call assign_advice and assign_advice_from_constant and get the same return type. See: https://github.com/HungryCatsStudio/poseidon-circuit/blob/88fc90c714453f9fcc5272d3ae9fe9e84c0f37b7/src/poseidon/pow5.rs#L358

We have thought of a couple of solutions:

• Since the assign_advice_from_constant is generic on VR, we can try to produce convert the caller's parameter from type F to &'v Assigned<F>, but I'm not sure that's possible.
• Alternatively, we could leave the call to assign_advice_from_constant as is, and try to transform its output from type AssignedCell<F, F> into AssignedCell<&'v Assigned<F>, F>. Again, we're not sure how we could achieve that, direct construction isn't possible since fields of AssignedCell are private (cf. https://github.com/axiom-crypto/halo2/blob/main/halo2_proofs/src/circuit.rs#L110C1-L114C2).

In view of this, do you guys see a way to make things work? Either with one of the above approaches or a slight redesign of assign_advice_from_constant's prototype? It could also be that we're completely missing the point here and barking at the wrong tree.

See also discussion on a PR in halo2-lib.

[nyunyunyunyu]

Hmm I remember I also ran into this in the past. This conversion is not an issue for AssignedCell<Assigned<F>, F> because it provides .evaluate(self) (link)). Then I found we changed the return value from AssignedCell<Assigned<F>, F> to AssignedCell<&'v Assigned<F>, F>. @jonathanpwang What was the rationale behind this change?

[jonathanpwang]

Rationale: when you assign a value, it is stored in some vector in the backend. In order for it to return the value again, it is copy/cloning the value. This is unnecessary since the true value is available in the backend, so you should only return a pointer.

In fact for our purposes we could have it now return nothing, since we track all values in virtual regions and only assign at the end.

[jonathanpwang]

Btw you will run into a bigger issue: Scroll Poseidon uses layouter extensively while we intentionally turned off this support

[mmagician]

Thanks @jonathanpwang. How this will affect us exactly? It seems that zkevm-keccak also uses the Layouter: https://github.com/axiom-crypto/halo2-lib/blob/195aa8ce38bc8283538a309abc9ff8a05489bf30/hashes/zkevm-keccak/src/keccak_packed_multi/tests.rs#L52 and similarly in the builder: https://github.com/axiom-crypto/axiom-eth/blob/993a0529e723225531b2b345a7ad11cbb6cdcee0/axiom-eth/src/keccak/builder.rs#L246