Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Simplify IsZero and BatchedIsZero #1401

Open
ChihChengLiang opened this issue May 10, 2023 · 2 comments
Open

Simplify IsZero and BatchedIsZero #1401

ChihChengLiang opened this issue May 10, 2023 · 2 comments

Comments

@ChihChengLiang
Copy link
Collaborator

For IsZero gadget, we defined IsZeroConfig, IsZeroChip, and IsZeroInstruction. For BatchedIsZero gadget, we defined BatchedIsZeroChip and BatchedIsZeroConfig.

For each case, we can collect all relevant methods and keep only XConfig struct in the gadget. The rests are redundant abstractions.

So that eventually we will have only IsZeroConfig and BatchedIsZeroConfig

  • For the IsZero gadget. The Chip trait did nothing really interesting.
  • For the BatchIsZero gadget. We don't even implement Chip trait for it.

Originally posted by @ChihChengLiang in #1397 (comment)

@KimiWu123
Copy link
Contributor

I didn't make it clear in my #1397 (comment).
What I mean is I couldn't find any xxxCircuitConfig struct having xxxChip defined member fields. Take StateCircuitConfig as example,

pub struct StateCircuitConfig<F> {
    ...
    sort_keys: SortKeysConfig,
    is_non_exist: BatchedIsZeroConfig,
    lexicographic_ordering: LexicographicOrderingConfig,
    lookups: LookupsConfig,
}

and we could also see in SortKeysConfig, tag is BinaryNumberConfig not BinaryNumberChip

pub struct SortKeysConfig {
    tag: BinaryNumberConfig<RwTableTag, 4>,
     ...
}

So, if we declare push_data_left_is_zero in IsZeroConfig type in #1397, it's more consistent to me but it's not a big issue. Besides, I guess we need to decide if we need xxxChip first.


As for this issue, found something interesting while I went through all the gadgets,

  1. under gadgets/src/, except evm_word.rs, other gadgets implement both xxxChip and xxxConfig (evm_word.rs only has WordConfig). WordConfig has configure and assign_word, but those methods are only available in xxxChip in other files.
  2. some of gadgets in zkevm-circuits/src/state_circuit implement both xxxConfig and xxxChip but LexicographicOrderingConfig doesn't have xxxChip.

I think we need a rule for gadgets implementation, either having both xxxChip and xxxConfig (follow Halo2's practice) or make it simple, just having xxxConfig. I don't have any strong opions on either one.

@ChihChengLiang
Copy link
Collaborator Author

We should keep it simple unless we really need a feature from a Chip implementation.

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

No branches or pull requests

2 participants