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

Use BinaryNumberChip in LexicographicOrderingConfig to lower degree#566

Merged
ed255 merged 62 commits into
privacy-ethereum:mainfrom
scroll-tech:fix/lex
Jun 27, 2022
Merged

Use BinaryNumberChip in LexicographicOrderingConfig to lower degree#566
ed255 merged 62 commits into
privacy-ethereum:mainfrom
scroll-tech:fix/lex

Conversation

@z2trillion
Copy link
Copy Markdown
Contributor

@z2trillion z2trillion commented Jun 13, 2022 •
edited
Loading

fixes #561

@z2trillion z2trillion requested a review from miha-stopar as a code owner June 13, 2022 21:40
@z2trillion z2trillion marked this pull request as draft June 13, 2022 21:40
@github-actions github-actions Bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Jun 13, 2022
@z2trillion z2trillion mentioned this pull request Jun 14, 2022
Closed
15 tasks
DreamWuGit
DreamWuGit reviewed Jun 15, 2022
View reviewed changes
Comment thread zkevm-circuits/src/state_circuit.rs
DreamWuGit
DreamWuGit reviewed Jun 15, 2022
View reviewed changes
Comment thread zkevm-circuits/src/state_circuit/constraint_builder.rs
pub tag_bits: [Expression<F>; 4],
pub id: MpiQueries<F, N_LIMBS_ID>,
pub is_id_unchanged: Expression<F>,
pub is_tag_and_id_unchanged: Expression<F>,
Copy link
Copy Markdown
Collaborator

@DreamWuGit DreamWuGit Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also need is_tag_unchanged and is_id_unchanged is_field_tag_unchanged, I have used them in PR #552 , I believe others Op constraint will need in the near future .

Copy link
Copy Markdown
Contributor Author

@z2trillion z2trillion Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can add them later when they're needed.

z2trillion and others added 22 commits June 16, 2022 17:15
Add BinaryNumber chip
a7ecc3d
Cleanup
339eadc
Use binary columns to select based on Tag
67962c9
Clean up and rename to_bits to as_bits
dc462db
Add check that RwTableTag is in allowed range
ddf7ea9
Fix degree test and remove unused function
92890a4
Fix rebase and clean up invalid_values gate
c4ee256
Remove unused derive
9910424
Make requested changes
941b394
clippy
47c2e4a
Make requested changes
2a7ca30
Create and use SortKeysConfig
81a1819
Cleanup lexicographic ordering config
7ac83e6
Fix rebase and limb count errors
f55976a
Fix first_access expression and remove unused IsZero gadget
e5bd60b
Clean up invalid values gate
bc80250
Replace is_id_uncchanged with binary expression
725b53b
cargo fix and fix tests
4bfcca7
lower degree
8f3fe8f
clippy, fix, and fmt
65723be
clean up
2df49e7
@lispc
feat(evm-circuit): add dummy gadget (#563)
9666e11 
* feat(evm-circuit): add dummy gadget; make sha3 uses dummy gadget; enable erc20 integration tests

* some comments
@github-actions github-actions Bot added the CI Issues related to the Continuous Integration mechanisms of the repository. label Jun 16, 2022
@z2trillion z2trillion mentioned this pull request Jun 21, 2022
Closed
@z2trillion z2trillion requested a review from miha-stopar June 21, 2022 17:22
@z2trillion
Copy link
Copy Markdown
Contributor Author

z2trillion commented Jun 21, 2022

I left the shift description in the top of the file because I think it's much easier to understand. In my mind, the gadget is showing "one of C0, ..., C31 is non-zero and fits into 16 bits" by using an RLC to avoid overflowing.

miha-stopar
miha-stopar reviewed Jun 22, 2022
View reviewed changes
Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs
Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs Outdated
@miha-stopar
Copy link
Copy Markdown
Collaborator

miha-stopar commented Jun 22, 2022

I left the shift description in the top of the file because I think it's much easier to understand. In my mind, the gadget is showing "one of C0, ..., C31 is non-zero and fits into 16 bits" by using an RLC to avoid overflowing.

Just thought it might be a bit confusing if docs are different from the implementation. Perhaps leave the shifts in the comment, but mention that RLC is used to lower the degree?

BTW, a naive question: wouldn't be possible to implement the approach with RLC without BinaryNumberChip?

@z2trillion
Copy link
Copy Markdown
Contributor Author

z2trillion commented Jun 22, 2022 •
edited
Loading

No it wouldn't be possible because we need to use the BinaryNumberChip to represent first_different_limb. Without the BinaryNumberChip, we can't have low-degree constraints that are conditional on the value of first_different_limb.

@miha-stopar
Copy link
Copy Markdown
Collaborator

miha-stopar commented Jun 23, 2022

No it wouldn't be possible because we need to use the BinaryNumberChip to represent first_different_limb. Without the BinaryNumberChip, we can't have low-degree constraints that are conditional on the value of first_different_limb.

Of course, got it now. Thanks! Perhaps add a comment that there are two optimisations to reduce the degree: RLC and BinaryNumberChip. The PR looks cool to me! Just one consideration: seems like BinaryNumberChip could be useful elsewhere too, perhaps it could be put in evm_circuit/util?

ed255
ed255 reviewed Jun 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! Very interesting use of the RLC to verify the equality between a large window of values!

I've added some notes and comments with suggestions; all are small details. Please take a look!

Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs Outdated
Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs
Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs
Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs Outdated
Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs
Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs Outdated
Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs
@z2trillion
Copy link
Copy Markdown
Contributor Author

z2trillion commented Jun 24, 2022

Of course, got it now. Thanks! Perhaps add a comment that there are two optimisations to reduce the degree: RLC and BinaryNumberChip. The PR looks cool to me! Just one consideration: seems like BinaryNumberChip could be useful elsewhere too, perhaps it could be put in evm_circuit/util?

I don't think it makes sense to move it into evm_circuit/util right now, since it's only used in by the state circuit at the moment.

@z2trillion z2trillion requested review from ed255 and miha-stopar June 24, 2022 22:11
miha-stopar
miha-stopar approved these changes Jun 27, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

ed255
ed255 approved these changes Jun 27, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ed255 ed255 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ed255 ed255 merged commit e6dda06 into privacy-ethereum:main Jun 27, 2022
@z2trillion z2trillion deleted the fix/lex branch June 30, 2022 05:57
jonathanpwang pushed a commit to axiom-crypto/zkevm-circuits that referenced this pull request Aug 1, 2023
@kunxian-xia
Feat: constrain tx data gas cost in RLP & Tx circuits (privacy-ethere…
196db5e 
…um#566)

* stage

* support tx gas cost in RLP circuit

* lookup to RLP table for TxDataGasCost
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@DreamWuGit DreamWuGit DreamWuGit left review comments

@miha-stopar miha-stopar miha-stopar approved these changes

@ed255 ed255 ed255 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crate-zkevm-circuits Issues related to the zkevm-circuits workspace member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fix state circuit order and packing issue

5 participants

@z2trillion @miha-stopar @DreamWuGit @ed255 @lispc