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

State Circuit: Refactor word_rlc into word lo/hi #1423

Merged
merged 12 commits into from
Jun 8, 2023
Merged

State Circuit: Refactor word_rlc into word lo/hi #1423

merged 12 commits into from
Jun 8, 2023

Conversation

adria0
Copy link
Member

@adria0 adria0 commented May 19, 2023 •
edited
Loading

Description

State Circuit: Refactor word_rlc into word lo/hi

This PR does not compile for itself, anyway the changes introduced in this PR passes the state circuit tests in another branch

Issue Link

Contents

  • As expected, changing rlc encoded values into word::Word, as defined in rw_table specs
  • Add helper methods to ConstraintBuilder (like require_word_zero )
  • Make MPI able to manage multi-field values (to be able to use U256's)
  • Remove random_linear_combination.rs because Word uses MPI instead rlc
  • Removed test focused on test rlc
  • Removed unused field aux1 and renamed aux2 to init_val
  • Added helper methods to word::Word
  • Also updated mpt witness to pass the state tests
  • Apply Split Address when stored in Word  zkevm-specs#424

Rationale

The lexicographic ordering, used word_rlc randomness for another use. Since this randomess is going to be removed, we have three options here:

  • Use another method to build the constraints that does not uses rlc at all
  • Create another challenge value
  • Use another existing randomness

In order to change only the code concerning hi/lo refactor, the choice is to use the keccak randomness instead

@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label May 19, 2023
@adria0 adria0 changed the title WIP Statecircuit lo hi State Circuit: Refactor word_rlc into word lo/hi May 25, 2023
@adria0 adria0 marked this pull request as ready for review May 25, 2023 15:26
@adria0 adria0 marked this pull request as draft May 26, 2023 06:16
@adria0 adria0 marked this pull request as ready for review May 26, 2023 14:55
@ChihChengLiang
Copy link
Collaborator

Can we do a Clippy round before the review?

@ChihChengLiang ChihChengLiang self-requested a review May 31, 2023 09:35
@adria0
Copy link
Member Author

adria0 commented May 31, 2023

8aa2148

Sure @ChihChengLiang! done in 8aa2148

ChihChengLiang
ChihChengLiang reviewed Jun 1, 2023
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

I did first pass.
Would be great if we can fix the Rust fmt.

zkevm-circuits/src/state_circuit.rs Show resolved Hide resolved
zkevm-circuits/src/state_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/state_circuit/constraint_builder.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/state_circuit/test.rs Show resolved Hide resolved
@ed255 ed255 linked an issue Jun 1, 2023 that may be closed by this pull request
State Circuit: Refactor word_rlc into word lo/hi #1382
Closed
@ed255 ed255 self-requested a review June 1, 2023 08:19
This was referenced Jun 1, 2023
Merged
Closed
@hero78119 hero78119 self-requested a review June 1, 2023 09:45
ChihChengLiang
ChihChengLiang reviewed Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

I figured out the address_change function. I added some proposals for the rw_table.address field.

zkevm-circuits/src/table.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/state_circuit/constraint_builder.rs Outdated Show resolved Hide resolved
hero78119
hero78119 reviewed Jun 5, 2023
View reviewed changes
Copy link
Member

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

First round quick pass, it looks really nice to clean up many randomness in state circuit. Beside also replace aux1 aux2 with better naming! few other comments

zkevm-circuits/src/state_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/state_circuit.rs Show resolved Hide resolved
zkevm-circuits/src/state_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/state_circuit/constraint_builder.rs Show resolved Hide resolved
zkevm-circuits/src/state_circuit/multiple_precision_integer.rs Show resolved Hide resolved
zkevm-circuits/src/table.rs Outdated Show resolved Hide resolved
@hero78119 hero78119 mentioned this pull request Jun 5, 2023
Merged
1 task
@ChihChengLiang ChihChengLiang removed the request for review from ed255 June 5, 2023 13:07
@ChihChengLiang
Copy link
Collaborator

I removed the review request for @ed255 since we have two reviewers here already.

ChihChengLiang
ChihChengLiang reviewed Jun 6, 2023
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

I finished reading all the diffs and added more comments.

zkevm-circuits/src/state_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/state_circuit/multiple_precision_integer.rs
}

impl ToLimbs<N_LIMBS_RW_COUNTER> for u32 {
fn to_limbs(&self) -> [u16; 2] {
fn to_limbs(&self) -> [u16; N_LIMBS_RW_COUNTER] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should rename N_LIMBS_RW_COUNTER to N_LIMBS_U32. But probably not in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, why not 👍

zkevm-circuits/src/table.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/table.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/table.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/util/word.rs Outdated Show resolved Hide resolved
ChihChengLiang
ChihChengLiang reviewed Jun 6, 2023
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

I finished reading all the diffs and added more comments.

ChihChengLiang
ChihChengLiang approved these changes Jun 7, 2023
View reviewed changes
Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

LGTM after addressing @hero78119's comments. Great word adding lo-hi and cleaning up this big chunk of the codebase.

hero78119
hero78119 approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

All LGTM!! Few discussion can be moved to discuss in separate issue

@adria0 adria0 merged commit adac146 into word-lo-hi Jun 8, 2023
@adria0 adria0 deleted the statecircuit-lo-hi branch June 8, 2023 06:34
@ChihChengLiang ChihChengLiang mentioned this pull request Jun 8, 2023
Closed
This was referenced Jun 15, 2023
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hero78119 hero78119 hero78119 approved these changes

@ChihChengLiang ChihChengLiang ChihChengLiang approved these changes

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.

State Circuit: Refactor word_rlc into word lo/hi
3 participants
@adria0 @ChihChengLiang @hero78119