-
Notifications
You must be signed in to change notification settings - Fork 125
feat: support alpha3 testnet #91
base: taiko-pi-test
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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!
zkevm-circuits/src/pi_circuit2.rs
Outdated
let rpi_rlc_acc_cell = rpi_rlc_acc_cell.unwrap(); | ||
rpi_rlc_acc_cell.copy_advice( | ||
|| "keccak(rpi)_input", | ||
region, | ||
self.rpi, | ||
keccak_row, | ||
)?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
I think this part can be reverted, the new layouter of pi doesn't need
these changes
Brecht Devos ***@***.***> 于 2023年5月15日周一 19:03写道:
… ***@***.**** commented on this pull request.
------------------------------
In geth-utils/build.rs
<#91 (comment)>
:
> @@ -24,12 +24,7 @@ fn main() {
}
// Files the lib depends on that should recompile the lib
- let dep_files = vec![
But still, why so many changes related to this code? Also code in
CreateTrace being commented out, which probably breaks a lot of tests.
Are these changes temporary?
—
Reply to this email directly, view it on GitHub
<#91 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF7FCZSTPVYYOFS72RNCWQTXGIEPRANCNFSM6AAAAAAX2EM7KM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Can you run formatting and clippy, github shows a lot of clippy warnings/errors. |
Yeah, but some warnings/errors have existed before. e.g. |
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? |
Ah yeah makes sense. |
There was a problem hiding this 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?
### 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.
No description provided.