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

fix tag byte overlap issue #520#523

Merged
icemelon merged 12 commits into
privacy-ethereum:mainfrom
scroll-tech:fix_state_orderbyte
May 30, 2022
Merged

fix tag byte overlap issue #520#523
icemelon merged 12 commits into
privacy-ethereum:mainfrom
scroll-tech:fix_state_orderbyte

Conversation

@DreamWuGit
Copy link
Copy Markdown
Collaborator

this PR try to fix issue #520

@DreamWuGit DreamWuGit requested a review from miha-stopar as a code owner May 20, 2022 03:16
@github-actions github-actions Bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label May 20, 2022
@lispc
Copy link
Copy Markdown
Collaborator

lispc commented May 20, 2022

@DreamWuGit if you change field tag to 5 bits, the total bits will not be 480 bits? 481 bits? Or reduce 1 bit from some other key?

@z2trillion
Copy link
Copy Markdown
Contributor

For the proofs to verify and the tests to pass, you need to update packed_tags in line 282.

As written, the extra bit comes from the second most significant byte of the id. The maximum value of id we can handle after this change will only be 2^16 - 1.

@DreamWuGit DreamWuGit marked this pull request as draft May 20, 2022 05:57
@DreamWuGit
Copy link
Copy Markdown
Collaborator Author

@DreamWuGit if you change field tag to 5 bits, the total bits will not be 480 bits? 481 bits? Or reduce 1 bit from some other key?

481 bits seem ok to me , will check with Mason if he has another idea !

@lispc
Copy link
Copy Markdown
Collaborator

lispc commented May 20, 2022

481 bits need 31 16bit limbs. And much refactor needed... Better keep 480bits i think

@z2trillion
Copy link
Copy Markdown
Contributor

this still keeps it to 480 bits, so 2 field elements.

@DreamWuGit DreamWuGit mentioned this pull request May 22, 2022
8 tasks
@DreamWuGit DreamWuGit marked this pull request as ready for review May 23, 2022 01:26
@DreamWuGit
Copy link
Copy Markdown
Collaborator Author

@miha-stopar could you have a look ?

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.

Looks cool to me, I would just perhaps add a couple of comments to make it more easy to understand the idea.

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
Copy link
Copy Markdown
Collaborator

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

LGTM. minor revise on the comment.

Comment thread zkevm-circuits/src/state_circuit/lexicographic_ordering.rs Outdated
DreamWuGit and others added 3 commits May 27, 2022 08:46
@icemelon icemelon merged commit 323a4f2 into privacy-ethereum:main May 30, 2022
@icemelon icemelon deleted the fix_state_orderbyte branch May 30, 2022 07:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants