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

[word lo/hi] retype table, common gadget and util design #1414

Closed
wants to merge 10 commits into from
Closed

[word lo/hi] retype table, common gadget and util design #1414

wants to merge 10 commits into from

Conversation

hero78119
Copy link
Member

@hero78119 hero78119 commented May 17, 2023 •
edited
Loading

Description

Few fix to iterating utilities api.

Issue Link

#1388

Type of change

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

Contents

  • more documentations
  • word limbs add assign_lo_hi, while lo, hi part support assign with different length of bytes respectively. Refer the use case in evm_circuit/execution/address.rs, evm_circuit/execution/caller.rs.
  • rwtable.aux2 change to Word type to fit committed value
  • lt_word gadget usage fix
  • fix MemoryAddressGadget to fit EVM spec, for length = 0 then offset can be arbitrary value still legal (even larger than 5 bytes, since it will be ignore).
  • fix SstoreGasGadget unnecessary assignment. For the cells/word passed from constructor should be assigned by caller, not callee.

How to test

  • based on reviewing/fixing compiling error in evm execution gadgets

@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label May 17, 2023
@hero78119 hero78119 changed the title documentation and generalize word lo/hi assignment [word lo/hi WIP] documentation and generalize word lo/hi assignment May 17, 2023
@hero78119 hero78119 changed the title [word lo/hi WIP] documentation and generalize word lo/hi assignment [WIP word lo/hi] iterating common gadget and util May 17, 2023
@hero78119 hero78119 changed the title [WIP word lo/hi] iterating common gadget and util [WIP word lo/hi] retype table, common gadget and util design May 17, 2023
@hero78119 hero78119 changed the title [WIP word lo/hi] retype table, common gadget and util design [word lo/hi] retype table, common gadget and util design May 17, 2023
ed255
ed255 reviewed May 18, 2023
View reviewed changes
Copy link
Member

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

Overall looks good! The only issue I found is that from the spec, an address in the CallContext is not split, so a conversion is needed when taking an address from CallContext and writing it to the Stack.

The other point is that I think it's convenient to have a Word assign method that takes an eth_types::Word

zkevm-circuits/src/evm_circuit/execution/address.rs Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution/caller.rs Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/util/common_gadget.rs Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/util/math_gadget/lt_word.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/util/word.rs Show resolved Hide resolved
@hero78119 hero78119 mentioned this pull request May 19, 2023
Closed
@hero78119 hero78119 mentioned this pull request May 22, 2023
Merged
4 tasks
@ed255 ed255 mentioned this pull request May 22, 2023
Closed
@adria0
Copy link
Member

adria0 commented May 22, 2023

@ed255 @hero78119 at this moment we have 3 equivalent types:

  • primitive_types::U256
  • eth_types::Word
  • utils::word::Word<F>

I think that it will be nice if we have a rule when using this types.

I personally find it confusing the type eth_types::Word, so I would prefer always use U256 and utils::word::Word<F> when we need to use to split into Fs

WDYT?

@hero78119
Copy link
Member Author

@ed255 @hero78119 at this moment we have 3 equivalent types:

  • primitive_types::U256
  • eth_types::Word
  • utils::word::Word<F>

I think that it will be nice if we have a rule when using this types.

I personally find it confusing the type eth_types::Word, so I would prefer always use U256 and utils::word::Word<F> when we need to use to split into Fs

WDYT?

@adria0 it sounds good to me! I think we can do this change at once in another PR :)

@hero78119
Copy link
Member Author

Hi @ed255 @adria0 Do you think it's good we merge this pr first once no other concern, since it's util related and shared for all circuit.

@hero78119
Copy link
Member Author

Update: support assign_h160 on Word type for address assignment

@hero78119 hero78119 mentioned this pull request May 23, 2023
Closed
5 tasks
@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
Copy link
Member Author

closed, as word partially migrate to #1441

@hero78119 hero78119 closed this Jun 1, 2023
@hero78119 hero78119 mentioned this pull request Jun 5, 2023
Open
hero78119 added a commit that referenced this pull request Jun 27, 2023
@hero78119
[word lo/hi] support u16 range lookup (#1427)
1264bfc 
### Description

[_PR description_]

### Issue Link

Close
#1426
Depends by
#1414

### Type of change

- [x] Breaking change (fix or feature that would cause existing
functionality to not work as expected)

### Contents

- [x] constraint builder in evm circuit support u16 lookup
- [x] rename `byte_table` to `u8_table` and `query_byte` to `query_u8`
so it align with `query_u16` and so on in the future
- [x] refactor state circuit and evm circuit to reuse range table.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ed255 ed255 ed255 left review comments

@adria0 adria0 Awaiting requested review from adria0

@KimiWu123 KimiWu123 Awaiting requested review from KimiWu123

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.

Write Word utilities, types and abstractions for word lo/hi
3 participants
@hero78119 @adria0 @ed255