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

SHA3 opcode | Keccak support in copy circuit#235

Merged
roynalnaruto merged 16 commits into
privacy-ethereum:masterfrom
scroll-tech:support/copy-circuit-keccak
Jul 28, 2022
Merged

SHA3 opcode | Keccak support in copy circuit#235
roynalnaruto merged 16 commits into
privacy-ethereum:masterfrom
scroll-tech:support/copy-circuit-keccak

Conversation

@roynalnaruto
Copy link
Copy Markdown
Collaborator

@roynalnaruto roynalnaruto commented Jul 22, 2022

This PR adds a RlcAcc type in the CopyDataTypeTag enum.

This tag is reserved for copy destination where instead of copying byte to byte (read to write), we're accumulating an RLC value in the destination. That is:

write_val::cur == (write_val::prev * r) + read_val::cur

This variant of the copy circuit will be used for SHA3 opcode execution gadget, where the goal is to find a final accumulator value over each byte from the input. The final RLC accumulator will be used to lookup to the Keccak table and find the SHA3 of the input bytes.

The PR also implements the SHA3 opcode.

Closes #228
Closes #223

@roynalnaruto roynalnaruto mentioned this pull request Jul 22, 2022
@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Jul 22, 2022

This tag is reserved for copy destination where instead of copying byte to byte (read to write), we're accumulating an RLC value in the destination. That is:

write_val::cur == write_val::prev + read_val::cur

This is interesting. While working on the Super Circuit I noticed that for RLC with dynamic-length inputs, it's easier (or I should say, more convenient) to calculate the RLC(reversed(input)).
This is assuming this definition of RLC:
RLC(v) = v[0] + r * v[1] + r^2 * v[2] + r^3 * v[3] + ... + r^n * v[n]
And this is because it's easy to lay out the inputs in order in a circuit, and then calculate an RLC forward (meaning that you get the RLC at the bottom row) like this:
acc_rlc[i] = acc_rlc[i-1] * r + v[i]
which makes acc_rlc[n-1] = RLC(reversed(v))

The bytecode circuit already does the lookups to keccak like this (using the reverse of the input).

Maybe we can agree to always use RLC(reversed(input)) when the input length is dynamic.

@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

This is assuming this definition of RLC:
RLC(v) = v[0] + r * v[1] + r^2 * v[2] + r^3 * v[3] + ... + r^n * v[n]

I did know about this definition from the bytecode circuit. I made a mistake by only adding up every value while ignoring the r^i multipliers.

Based on what I've pushed in the recent fix, it turns out to be this, which is in fact the RLC(reverse(input))

rlc_acc(i) = v[i] + v[i-1]*r + v[i-2]*r^2 + ... + v[0]*r^i

@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Jul 25, 2022

Before this feature, a copy_table lookup looked like this:

                is_first: 1,
                src_id,
                src_tag,
                dst_id,
                dst_tag,
                src_addr,
                src_addr_end,
                dst_addr,
                length,
                rw_counter,
                rwc_inc,
  1. How will the lookup table look like after this feature?
  2. Which values will be used for an AccRlc lookup? The accumulated rlc is found in the row with is_last = 1, but the input arguments (src_addr, rw_counter) are found in the row with is_first = 1.

@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

roynalnaruto commented Jul 25, 2022

@ed255 I've handled the above points in this PR: scroll-tech#46

I haven't opened it yet against upstream because I was waiting for this PR to be merged first. Do you prefer if I opened a single PR for this change as well as implementing the SHA3 opcode?

How will the lookup table look like after this feature?

Defined here: https://github.com/scroll-tech/zkevm-specs/blob/feat/sha3/src/zkevm_specs/evm/table.py#L411-L423

Which values will be used for an AccRlc lookup? ...

Handled here: https://github.com/scroll-tech/zkevm-specs/blob/feat/sha3/src/zkevm_specs/evm/table.py#L467-L496

In the PR: scroll-tech#46 I already have a basic test case for SHA3 opcode execution, for is_root=True transaction, with the SHA3 implemented here: https://github.com/scroll-tech/zkevm-specs/blob/feat/sha3/src/zkevm_specs/evm/execution/sha3.py

@ed255
Copy link
Copy Markdown
Contributor

ed255 commented Jul 25, 2022

@ed255 I've handled the above points in this PR: scroll-tech#46

I haven't opened it yet against upstream because I was waiting for this PR to be merged first. Do you prefer if I opened a single PR for this change as well as implementing the SHA3 opcode?

Ah I see. Considering that this PR is quite small I would say that I'd prefer if the SHA3 opcode spec was added to this PR as well :) That is, merging scroll-tech#46 into this PR. I think this way the review will be more clear, because the same PR will contain the new copy circuit functionality, and the SHA3 opcode using such functionality, with the corresponding tests.

How will the lookup table look like after this feature?

Defined here: https://github.com/scroll-tech/zkevm-specs/blob/feat/sha3/src/zkevm_specs/evm/table.py#L411-L423

Which values will be used for an AccRlc lookup? ...

Handled here: https://github.com/scroll-tech/zkevm-specs/blob/feat/sha3/src/zkevm_specs/evm/table.py#L467-L496

In the PR: scroll-tech#46 I already have a basic test case for SHA3 opcode execution, for is_root=True transaction, with the SHA3 implemented here: https://github.com/scroll-tech/zkevm-specs/blob/feat/sha3/src/zkevm_specs/evm/execution/sha3.py

Thanks for the pointers!

* feat: initial changes

* fix: rebase and fix return_ opcode as per new copy lookup api

* feat: add copy and keccak lookup

* tests: WIP test for sha3

* fix: sha3 tests for root tx
@roynalnaruto roynalnaruto changed the title Keccak support in copy circuit SHA3 opcode | Keccak support in copy circuit Jul 25, 2022
@roynalnaruto
Copy link
Copy Markdown
Collaborator Author

@ed255 Merged! So this PR now is a full implementation of the SHA3 opcode.

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.

I've left comments to discuss some stuff.

Comment thread src/zkevm_specs/evm/table.py Outdated
Comment thread src/zkevm_specs/evm/table.py Outdated
Comment thread src/zkevm_specs/evm/typing.py Outdated
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! Great work!

Comment thread tests/evm/test_sha3.py Outdated
Copy link
Copy Markdown
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 added some questions

Comment thread src/zkevm_specs/evm/execution/sha3.py Outdated
Comment thread src/zkevm_specs/util/param.py Outdated
Comment thread src/zkevm_specs/evm/typing.py Outdated
Copy link
Copy Markdown
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 now. Great PR. Thank you for the contribution!

Comment thread src/zkevm_specs/evm/table.py
@roynalnaruto roynalnaruto merged commit f1c9719 into privacy-ethereum:master Jul 28, 2022
@roynalnaruto roynalnaruto deleted the support/copy-circuit-keccak branch July 28, 2022 13:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add keccak support in the copy circuit SHA3 opcode

4 participants