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

[word lo/hi] support u16 range lookup #1427

Merged
merged 6 commits into from
Jun 27, 2023
Merged

[word lo/hi] support u16 range lookup #1427

merged 6 commits into from
Jun 27, 2023

Conversation

hero78119
Copy link
Member

@hero78119 hero78119 commented May 22, 2023 •
edited
Loading

Description

[PR description]

Issue Link

Close #1426
Depends by #1414

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Contents

  • constraint builder in evm circuit support u16 lookup
  • rename byte_table to u8_table and query_byte to query_u8 so it align with query_u16 and so on in the future
  • refactor state circuit and evm circuit to reuse range table.

@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label May 22, 2023
@hero78119 hero78119 marked this pull request as draft May 22, 2023 09:13
@hero78119 hero78119 force-pushed the feat/u16_rangecheck branch 5 times, most recently from 57507f1 to 4274bb8 Compare May 22, 2023 09:23
@hero78119 hero78119 marked this pull request as ready for review May 22, 2023 16:16
@hero78119 hero78119 changed the title support u16 range lookup [word-lo-hi] support u16 range lookup May 22, 2023
@hero78119 hero78119 changed the title [word-lo-hi] support u16 range lookup [word lo/hi] support u16 range lookup May 22, 2023
@ed255 ed255 linked an issue May 31, 2023 that may be closed by this pull request
Write Word utilities, types and abstractions for word lo/hi #1388
Closed
@hero78119 hero78119 force-pushed the feat/u16_rangecheck branch 2 times, most recently from a10abca to 1774d5e Compare June 1, 2023 04:13
@ed255
Copy link
Member

ed255 commented Jun 1, 2023

This PR touches the state circuit, we will give priority to merging #1423 first and then update this PR as necessary.

@ChihChengLiang
Copy link
Collaborator

#1423 Is merged, so this is unblocked.

@ChihChengLiang
Copy link
Collaborator

@hero78119 Can we resolve conflicts before reviewing it?

@hero78119
Copy link
Member Author

@hero78119 Can we resolve conflicts before reviewing it?

Done! Rebase onto the latest word-lo-hi and resolved conflict

@ChihChengLiang ChihChengLiang mentioned this pull request Jun 20, 2023
Closed
@hero78119 hero78119 mentioned this pull request Jun 21, 2023
Closed
10 tasks
@ChihChengLiang ChihChengLiang linked an issue Jun 21, 2023 that may be closed by this pull request
[word lo/hi] optimisation and improvements. #1487
Closed
10 tasks
@ChihChengLiang ChihChengLiang removed a link to an issue Jun 21, 2023
Write Word utilities, types and abstractions for word lo/hi #1388
Closed
@ChihChengLiang ChihChengLiang linked an issue Jun 22, 2023 that may be closed by this pull request
support byte16 range check #1426
Closed
1 task
@ChihChengLiang ChihChengLiang self-requested a review June 22, 2023 06:42
@adria0 adria0 self-requested a review June 23, 2023 06:52
adria0
adria0 approved these changes Jun 23, 2023
View reviewed changes
Copy link
Member

@adria0 adria0 left a comment

Choose a reason for hiding this comment

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

Nice and clean refactor! 🥇

zkevm-circuits/src/evm_circuit/util.rs
@@ -294,7 +294,9 @@ pub(crate) enum CellType {
StoragePhase1,
StoragePhase2,
StoragePermutation,
LookupByte,
// TODO combine LookupU8, LookupU16 with Lookup(Table::U8 | Table::U16)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, why not, maybe creating a good-first-issue ?

Copy link
Member Author

@hero78119 hero78119 Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

y let create good-first-issue once this feature sync back to main branch :)

ChihChengLiang
ChihChengLiang approved these changes Jun 23, 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 the feedback is addressed.

zkevm-circuits/src/evm_circuit/execution.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/state_circuit/lookups.rs Show resolved Hide resolved
@hero78119
evm circuit u16_lookup and rename byte_table to u8_table
7a64b87 
refactor evm/state circuit to share range check

fix build error
@hero78119
Copy link
Member Author

hero78119 commented Jun 23, 2023 •
edited
Loading

Found 2 issues after pr re-trigger ci check (since now we have light test :) )

  1. N_U8_LOOKUPS will complain cell not enough, due to I changed 24 -> 12, and move half column 12/2=6 to N_U16_LOOKUPS. However we do not cell move to U16 lookup yet. so I will tune it in later PR. For now I set N_U16_LOOKUPS=0, and clippy will complain, therefore adding #[allow(clippy::reversed_empty_ranges)] just to silent it.
  2. need to adjust test degree for evm circuit, so I hardcode 1<<16 to claim minimum row for evm circuit unittest (otherwise it will failed)

Please review again 🙏 tks

ChihChengLiang
ChihChengLiang approved these changes Jun 26, 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. Added a nitpick

zkevm-circuits/src/table/ux_table.rs Outdated Show resolved Hide resolved
@adria0
Copy link
Member

adria0 commented Jun 26, 2023

👍 LGTM

@hero78119 hero78119 merged commit 1264bfc into privacy-scaling-explorations:word-lo-hi Jun 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adria0 adria0 adria0 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.

[word lo/hi] optimisation and improvements. support byte16 range check
4 participants
@hero78119 @ed255 @ChihChengLiang @adria0