Improve halo2 query calls

[CeciliaZ030]

#144

[leolara]

Perhaps cur is a more clear name for this method?

[Brechtpd]

Yeah probably a good idea to at least also expose it as an option. I like the option for expr as well because a lot of other data types already use that to convert something to an expression.

[leolara]

It is not obvious what a column.expr() should return, what does it mean to convert a column to a expression, in how many ways can be that interpreted? This method is doing a current row rotation and that is what it should say in my opinion

[leolara]

Also, we have a next and prev method, it is obvious in my opinion that the cur one is missing.

[Brechtpd]

Good points. For me it would be natural to assume that expr() is kind of the default call, like in most languages you can do things like expr(rot: i32 = 0) where you don't have to specify the default value. But if it's confusing for other people better to drop it for sure.

[CeciliaZ030]

Should I add both expr() and cur() so that they can be used interchangeably?

[leolara]

my opinion is to have only cur() for the current rotation. if there is another way to convert a column to an expression that it does not include a rotation, my opinion is that in that case expr could be useful but I cannot think of any rotation-less expression from a column

[CPerezz]

I have to agree with @leolara here.

expr() just doesn't tell you anything about the fact that you're querying nor at which rotation.

I'd maybe swap names for query_cur(), query_next(), query_prev() and query_n().

Another topic is why do we need to have Option<> for the query index. Why can't we have the raw num?

[leolara]

I agree with @CPerezz also in the option issue. We should put our halo2 user hat in order to design external interfaces.

I am a user, I want to query a column:

For cur rotation: c.cur() is more readable than c.expr() or c.expr(None)

For arbitrary rotation: c.rot(n), c.rotation(n) or c.query(n) is more readable than c.expr(Some(n))

Also, it seems than c.next() and c.prev() is more readable than these alternatives.

I would even include a c.nextnext() and c.prevprev() although not very important.

[Brechtpd]

I'd maybe swap names for query_cur(), query_next(), query_prev() and query_n().

Indeed more clear, but also more verbose. Not sure what would be best, I tend to prefer shorter when something is used a lot so there's less stuff to read and more code fits on a single line.

Another topic is why do we need to have Option<> for the query index. Why can't we have the raw num?

An Expression can now exist without a valid query_index so it seemed safer to make this very clear by using an Option. But if we make sure the query index is always only accessible by halo2 internally we could also just use some dummy value instead.

[leolara]

I don't understand `An Expression can now exist without a valid query_index, I guess a query_index is a rotation? I don't know.

I think that can be one of the problems as a user of halo2, the programmer interface seems to come from internals that I don't know.

As I user of halo2, I would like to have .cur, .next, .prev and .rot. That should not be influenced by how halo2 works internally. It is about user ergonomics, in my opinion.

We should design the language that the zk circuits programmers need, for his/her experience, and then translate it to how halo2 works internally. Otherwise it is akin of programming in Assembly, where you need to understand the computer internals to code a text editor.

For a zk circuit developer halo2 is the "hardware" on which to run zk circuits, and the zk circuit language to describe it should make sense from his/her point of view and then being translated to the "hardware" machine code language.

[Brechtpd]

I don't understand `An Expression can now exist without a valid query_index, I guess a query_index is a rotation? I don't know.

It's just an index in an array of all (column, rotation) pairs. So only for internal use, and I checked and doesn't seem to be accessible by users.

As I user of halo2, I would like to have .cur, .next, .prev and .rot. That should not be influenced by how halo2 works internally. It is about user ergonomics, in my opinion.

Agreed, that's why the current query interface where people have to manually query expressions from meta is not great because users indeed mostly do not really need to care about that, they just want to use some data from a column at some rotation.

We should design the language that the zk circuits programmers need, for his/her experience, and then translate it to how halo2 works internally. Otherwise it is akin of programming in Assembly, where you need to understand the computer internals to code a text editor.

I think this PR is a small step forward for that, in that it moves the querying from the user's responsibility to something halo2 will do for you.

For a zk circuit developer halo2 is the "hardware" on which to run zk circuits, and the zk circuit language to describe it should make sense from his/her point of view and then being translated to the "hardware" machine code language.

Sure, but is that something halo2 itself needs to do? I think halo2 just needs to expose the basic stuff in a nice way and I think we're good, as long as the API for that is reasonable people can build abstractions on top of it as they would see fit without having to do modify halo2 itself. That is the reason I wanted to have this change, not because I'm necessarily looking forward to directly use this API, but mainly because it will make abstractions on top the halo2 easier to write. Most of this querying should probably still be abstracted away to something higher level (like the Cells of a CellManager).

[Brechtpd]

This PR also solves the problem found here in a more general way: privacy-scaling-explorations/zkevm-circuits#852 (comment). Querying a cell from a column already added the query to the internal halo2 structures, even if you didn't actually end up using it! The change it this PR makes it so the cost for the query is only paid when it is actually used.

[CPerezz]

@Brechtpd @CeciliaZ030 after re-reading (I've been quite bussy, apologies),
The biggest concern I have is that this will change the syntax of all the circuits breaking all compatibility with upstream circuits/chips code. And this is something I want @han0110 @kilic @ed255 @ChihChengLiang and @davidnevadoc to aggre with.

If they agree, can we go for cur(), prev(), next() & n() for the query and try to avoid the Option and use a dummy val for the query_index that doesn't exist??

[leolara]

My understanding is that the meta.query_* will still work. won't?

[han0110]

First pass and focused on backward compatibility.

[ed255]

Looks really good to me! I like this change and I vote to accept this proposal! This feature will not require changes in our existing circuit code, which is really nice. It may break some tests, as Han pointed out, from these two facts:

• Order of gate.queried_cells may change, which means MockProver can reports failures in a different order.
• query index is now an Option, which changes the default Debug implementation.

Both issues have solutions already proposed by Han; which I think should be considered.

Regarding the naming of the new query methods, I also agree with Leo and Carlos, I think cur is much less confusing than expr for column query at rotation 0. I would like to see these methods: cur(), next(), prev() and rot(i32).

I understand the reasoning behind wanting an expr() method, but I think it would only make sense if there was an intermediate Query type, so then we would have:

col.cur().expr();
col.next().expr();
col.prev().expr();
col.rot(4).expr();

Nevertheless I'm not proposing this option because it's very verbose.

I think it's good to keep the index: Option<usize> because it will cause a panic if the lazy cell query has not been performed when the index is needed. Using a default value would produce a silent error for such a bug.

[ed255]

Following Han's comment here https://github.com/privacy-scaling-explorations/halo2/pull/154/files#r1127741332
We could wrap this over if query.index.is_none() { ... }.
The same applies for Advice and Instance.

[CeciliaZ030]

@ed255

Without the change you request, the only breaking change would be that whenever the MockProver returns a list of failures, the order of such failures will change right? Is there any other change?

Now that I have reverted changes in meta.query_X as opposed to col.cur(), so for old code which uses VirtualCells the order will stay the same.

Are you thinking maybe on tests that produce MockProver failures and then do assert_eq on the string-formatted error?

For error format, I changed the Debug impl following @han0110 's comment, right now the failures should print out plain old information.

[han0110]

LGTM! Some leftover need to be remove and also need a extra fmt.

[han0110]

Need one extra format to pass cargo clippy --all-features --all-targets, see:

shuffle.rs.diff

diff --git a/halo2_proofs/examples/shuffle.rs b/halo2_proofs/examples/shuffle.rs
index c482b7b0..9e2a36d3 100644
--- a/halo2_proofs/examples/shuffle.rs
+++ b/halo2_proofs/examples/shuffle.rs
@@ -12,7 +12,7 @@ use halo2_proofs::{
            multiopen::{ProverIPA, VerifierIPA},
            strategy::AccumulatorStrategy,
        },
-        Rotation, VerificationStrategy,
+        VerificationStrategy,
    },
    transcript::{
        Blake2bRead, Blake2bWrite, Challenge255, TranscriptReadBuffer, TranscriptWriterBuffer,
@@ -63,19 +63,19 @@ impl<const W: usize> MyConfig<W> {
        // Second phase
        let z = meta.advice_column_in(SecondPhase);

-        meta.create_gate("z should start with 1", |meta| {
+        meta.create_gate("z should start with 1", |_| {
            let one = Expression::Constant(F::ONE);

            vec![q_first.expr() * (one - z.cur())]
        });

-        meta.create_gate("z should end with 1", |meta| {
+        meta.create_gate("z should end with 1", |_| {
            let one = Expression::Constant(F::ONE);

            vec![q_last.expr() * (one - z.cur())]
        });

-        meta.create_gate("z should have valid transition", |meta| {
+        meta.create_gate("z should have valid transition", |_| {
            let q_shuffle = q_shuffle.expr();
            let original = original.map(|advice| advice.cur());
            let shuffled = shuffled.map(|advice| advice.cur());

[CeciliaZ030]

@han0110 Just did clippy but it was with cargo clippy --features halo2curves/reexport --all-targets
Not sure if it's my problem or not but the halo2curves we use in this version has something like this that cause my --all-features fail:

#[cfg(feature = "reexport")]
pub use group;
#[cfg(not(feature = "reexport"))]
use group;

https://github.com/privacy-scaling-explorations/halo2curves/blob/c4dce16378e7b1d4d38e563c1669cdbee1647fb2/src/lib.rs#L21
If I want to have --all-features passed then I had to add halo2curves = { git = 'https://github.com/privacy-scaling-explorations/halo2curves', branch = "main", features = ['reexport']}

Please anyone let me know if I did something wrong. 😥

[han0110]

If I want to have --all-features passed then I had to add halo2curves = { git = 'https://github.com/privacy-scaling-explorations/halo2curves', branch = "main", features = ['reexport']}

yeah it's because we have unpinned the halo2curves version recently accidentally I think, and there is a update in halo2curves that have the reexport feature disabled by default, so some imports are missing without adding that feature.

After privacy-scaling-explorations/halo2curves#31 merged it should work without adding that.

[CeciliaZ030]

@han0110 Thanks for merging my #156 :) Can anyone see if this PR can be merged as well?

[CPerezz]

Will take a look today for sure @CeciliaZ030

[CPerezz]

LGTM. I really don't like the introduction of the unwraps TBH. But I don't see a way arround.

If others are Ok with this, I'm fine if it gets in. But need @han0110 and @kilic to look at it.

[CPerezz]

These unwraps still bother me TBH.
But the overall solution is much better I agree.

[han0110]

Just final nitpick, others LGTM!

[han0110]

LGTM! Thanks for addressing all the comments.