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

feat: support alpha3 testnet #91

Open
wants to merge 23 commits into
base: taiko-pi-test
Choose a base branch
from
Open

feat: support alpha3 testnet #91

wants to merge 23 commits into from

Conversation

johntaiko
Copy link

No description provided.

Copy link

@Brechtpd Brechtpd left a comment

Choose a reason for hiding this comment

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

Still have to go through some parts of the circuits, but looking good!

bus-mapping/Cargo.toml Show resolved Hide resolved
external-tracer/src/lib.rs Outdated Show resolved Hide resolved
geth-utils/build.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit2.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit2.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit2.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit2.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit2.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit2.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit2.rs Outdated Show resolved Hide resolved
Comment on lines 571 to 577
let rpi_rlc_acc_cell = rpi_rlc_acc_cell.unwrap();
rpi_rlc_acc_cell.copy_advice(
|| "keccak(rpi)_input",
region,
self.rpi,
keccak_row,
)?;
Copy link

Choose a reason for hiding this comment

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

Seems like this logic with the cells and stuff can can be simplified a bit because hashing is always done at the same position, so just enable the keccak lookup on the last row which already contains all the necessary data and good to go?

Copy link
Author

Choose a reason for hiding this comment

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

I need use two cell to store the keccak_input_rlc and keccak_output_rlc, so I reuse the self.rpi and self.rpi_rlc_acc which previously used to store rpi and rpi_rlc, the rpi_rlc here as the keccak_input_rlc for keccak lookup
So, if I don't copy from the rpi_rlc column into the rpi column, I need allocate an new column for keccak_output_rlc

zkevm-circuits/src/pi_circuit2.rs Outdated Show resolved Hide resolved
@johntaiko johntaiko linked an issue May 11, 2023 that may be closed by this pull request
@johntaiko
Copy link
Author

johntaiko commented May 15, 2023 via email

@Brechtpd
Copy link

Can you run formatting and clippy, github shows a lot of clippy warnings/errors.

@johntaiko
Copy link
Author

Can you run formatting and clippy, github shows a lot of clippy warnings/errors.

Yeah, but some warnings/errors have existed before. e.g. clippy warnings in bus-mapping.
The CI always checks all code instead of changed code.
https://github.com/taikoxyz/zkevm-circuits/pull/91/checks?check_run_id=13536737282

@johntaiko
Copy link
Author

johntaiko commented May 17, 2023

after the discussion with @smtmfft , because this PR is based on the OLD branch from PSE, we just ignore some warnings/errors. Next testnet, we will use the newest code and let all CI passed

Is this OK?

@Brechtpd
Copy link

Ah yeah makes sense.

Copy link

@ggkitsas ggkitsas left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM on a first, shallow pass. I am missing some context though, is there a list of changes we need for alpha3?

xiaodino pushed a commit that referenced this pull request Aug 10, 2023
### Description

Remove geth-utils and examples.

### Type of change

New feature (non-breaking change which adds functionality)

### Rationale

- Assembly was introduced in #91 to build examples to use geth-util. It
was not used elsewhere in the codebase.
- The examples are removed as we currently don't use geth-util like the
way the examples show. (Note that the examples also fail to run now. The
transactions in the examples would fail because the trace config
defaults to a 0 block gas limit. It runs after adding a block gas
limit.)
- I found those examples unused when reviewing how we use the tracing
API. It is much easier to review the tracing code after removing the
Assembly.
- The Readme mentioned a TODO CLI, which we never built. I think it
might be interesting to create a good first issue for people to extend
testool(https://github.com/privacy-scaling-explorations/zkevm-circuits/blob/690b2f143a94b7474f0c99493a99b0f2c53d93b8/testool/README.md#L38)
for it.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update public input circuit for new protocol release
3 participants