added padding advice column in Exp Circuit

[rrzhang139]

There is no column to indicate whether a row is padding or not.

[ed255]

Hi @rrzhang139 , is this PR related to this one? privacy-scaling-explorations/zkevm-circuits#1062

[rrzhang139]

@ed255 Yes, trying to push this PR before working on that one

[CPerezz]

@rrzhang139 thanks for working in this order. As @roynalnaruto suggested, we should use some mechanism similar to the one used in privacy-scaling-explorations/zkevm-circuits#1064 for the CopyCircuit padding.

I think the PR can serve you as inspiration on how to deal with this.

[rrzhang139]

Thanks! @CPerezz. Will check it out.

[CPerezz]

Can't we re-use is_step column and remove padding?
On this way, if is_step is false then we know it's padding.

Also see the comment for the final constraint.

[ed255]

I would like to discuss the possibility of a different approach:

• The exp circuit structure works following a repeated pattern of steps
• If we could define a fixed number of steps no matter what the input is, we would have constant fixed columns
• I see that an exponentiation like 2^1 takes 1 step
• Why not define max_exp_steps and then assign all the meaningful steps and fill the rest (until max_exp_steps) is reached with the steps corresponding to 2^1 repeatedly? This way we don't need extra columns or constraints.

Edit: I originally meant to say 1^2 instead of 2^1 to be used for padding, which is a case that requires a single step (I think 2^1 requires no steps)

[roynalnaruto]

@ed255 Great idea! This was also proposed by @z2trillion today and I was gonna post it here for comments. This is totally doable, removes the need for padding and simplifies constraints as all the "dummy" steps would be 2 ^ 1 == 2 (mod 2^256).

I was thinking about this today and I am completely supportive of this approach! @rrzhang139 would you be willing to drop the current padding approach and instead implement this? Please ask any questions if you are not completely sure how this would work.

[ed255]

Sorry, I pressed the button Close with comment unintentionally (when I just wanted to cancel writing a comment 😓 )

[CPerezz]

I would like to discuss the possibility of a different approach:

* The exp circuit structure works following a repeated pattern of steps

* If we could define a fixed number of steps no matter what the input is, we would have constant fixed columns

* I see that an exponentiation like `2^1` takes 1 step

* Why not define `max_exp_steps` and then assign all the meaningful steps and fill the rest (until `max_exp_steps`) is reached with the steps corresponding to `2^1` repeatedly?  This way we don't need extra columns or constraints.

Edit: I originally meant to say 1^2 instead of 2^1 to be used for padding, which is a case that requires a single step (I think 2^1 requires no steps)

Won't this require padding anyway?? Maybe I'm missing something but I don't see how padding is not needed here. As "filling-up" is the same as padding isn't it??

[rrzhang139]

@CPerezz yes I think you are right. Padding is involved, just how do we implement it to show that it is padding? I think exp_circuit does a good job delineating what is padding by creating a separate function, but I am open to some other design ideas.

[z2trillion]

Won't this require padding anyway?? Maybe I'm missing something but I don't see how padding is not needed here. As "filling-up" is the same as padding isn't it??

"Padding" would still be needed in the sense that some rows in the exp circuit will not be used by anything in the EVM circuit. the benefit (imo) would be that we wouldn't need to add an additional is_padding advice column and the only constraint we would need to add would be one saying that q_usable is always 1.

[CPerezz]

@z2trillion sounds good! Let's go for this then!

[roynalnaruto]

Good progress so far, I have a few suggestions :)

[rrzhang139]

@ed255 @roynalnaruto @CPerezz can we merge this PR since its zkevm-circuits PR is already merged? It sufficiently reflects the padding behavior exhibited in the circuits. Because it oversimplifies the number of rows in a step (I think it only uses two rows per step), I don't think we need to include the specificities of inserting unusable rows like in the circuits.

A few more changes I made to the latest commit were converting the steps -> rows by multiplying with OFFSET_INCREMENT, and removing pad_rows since we don't really pad but fill in dummy events.

[CPerezz]

Leaving the review to @ed255 as he was involved in the merge of the PR that solved this in the circuits.

[ed255]

LGTM! Sorry for the delay on reviewing this 😅

[ed255]

There's currently a type error in src/zkevm_specs/evm/typing.py, I think changing the max_exp_steps type like this should fix it:

class ExpCircuit:
    # [...]

    def __init__(self, max_exp_steps: int = 100) -> None:
        #[...]

We can merge once all the tests pass.

[CPerezz]

LGTM once @ed255 's comments are applied!

[ed255]

@rrzhang139 Could you take a look at this and fix the typing issue so that we can merge this? Thanks!

[rrzhang139]

Sorry for the delay! Just pushed!

[ed255]

@rrzhang139 sorry we've been away for some weeks, can you resolve the conflicts and we merge this?

[rrzhang139]

This is weird. I'm not getting any merge conflicts on my side so I can't even push a new commit.

[roynalnaruto]

Overall looks good, except the one addition which is not required for the specs. I'll go through it again once you update your PR

[rrzhang139]

@roynalnaruto I have resolved. But why is this conflict always appearing?

[lispc]

i dont know, what about squashing commits into one and forcing push again?

[aguzmant103]

@rrzhang139 just following up, did you figure out how to solve the conflicts to get this merged? Need any help from PSE?

[ed255]

@rrzhang139 we can take this PR and replicate it with the conflicts solved and merge it (we can't do it on the current PR because it's on a scroll branch); let us know if you'd prefer to work on it yourself, otherwise we'll take it in a few days :)

[CPerezz]

LGTM

[ed255]

Superseded by #419