Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify query_instance API to take an absolute row rather than a relative Rotation #768

Open
enricobottazzi opened this issue Apr 13, 2023 · 2 comments

Comments

@enricobottazzi
Copy link

enricobottazzi commented Apr 13, 2023

As already discussed with @therealyingtong on Discord, we think that the query_instance() method should take an absolute row: usize instead of a relative Rotation as input parameters. Instance values do not interact with regions / the layouter.
This would also make query_instance() consistent with assign_advice_from_instance(), which already requires the caller to provide the absolute row of the specified instance column.

An example where this would be required:

This is the current configuration of a chip

        // configure lt chip 
        let lt_config = LtChip::configure(
            meta,
            |meta| meta.query_selector(lt_selector),
            |meta| meta.query_advice(col_a, Rotation::cur()), // lhs
            |meta| meta.query_advice(col_b, Rotation::cur()), // rhs
        );

rhs is a public input passed in the instance column at row 3. Right now, in the assignment function, the rhs value is copied from the instance column to the advice column.

    pub fn enforce_less_than {

        layouter.assign_region(
            || "enforce sum to be less than total assets",
            |mut region| {
                
                ...

                // copy the total assets from instance column at row 3
                region.assign_advice_from_instance(
                    || "copy total assets",
                    self.config.instance,
                    3,
                    self.config.advice[1], 
                    0
                )?;

              ...

            },
        )?;
    }

A more convenient way would be configuring the chip by passing directly an expression from the instance column. This will remove the need for an extra copy constraint

        // configure lt chip 
        let lt_config = LtChip::configure(
            meta,
            |meta| meta.query_selector(lt_selector),
            |meta| meta.query_advice(col_a, Rotation::cur()), // lhs
            |meta| meta.query_instance(instance, Rotation::cur()), // rhs
        );

In this case, it is unclear how to use the Rotation to query the instance column at a specific row. A more intuitive API would be

        // configure lt chip 
        let lt_config = LtChip::configure(
            meta,
            |meta| meta.query_selector(lt_selector),
            |meta| meta.query_advice(col_a, Rotation::cur()), // lhs
            |meta| meta.query_instance(instance, row: 3), // rhs
        );
@daira
Copy link
Contributor

daira commented Apr 13, 2023

Why is there a query_instance method at all? With the other query_* methods, the aim is to be able to construct a gate polynomial that references the given column at the specified offset. These references are necessarily relative, because that's just how gate polynomials work. But unless you have complete control of layout (which would potentially allow you to arrange gates on absolute rows next to the relevant instance cells), that doesn't work for instance columns. So the only practical way to reference an instance cell in a way that works with the layouter abstraction is to use an equality constraint, which is what assign_advice_from_instance does.

We might want to retain a way for a circuit to use absolute layout, but that shouldn't be mixed in with the layouter-based API, I think. In any case I don't think that retaining query_instance but making the second argument an absolute row number is the right approach. (If we did do this, it's incompatible with any existing correct use of query_instance and inconsistent with the other query_* methods, so we should at least change the method name.)

@enricobottazzi
Copy link
Author

I agree that the current query_instance method is confusing and difficult to use.
Every idea that I have of adding an API to support such a feature adds unnecessary complexity at the (small) benefit of saving a copy constraint. I will close the issue unless you, or any other member of the community, decide it is worth it.

Also, back to the previous scenario, querying the instance column at absolute row 3 could create a huge region since the cell in the instance column might be very "far" from the cells in the advice columns. Would that increase the prover cost?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants