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

Runtime config #1728

Merged
merged 20 commits into from
Jan 31, 2024
Merged

Runtime config #1728

merged 20 commits into from
Jan 31, 2024

Conversation

ChihChengLiang
Copy link
Collaborator

@ChihChengLiang ChihChengLiang commented Jan 10, 2024 •
edited
Loading

Description

We add an initial structure for runtime config. In this PR, we plan to add only the invalid tx configuration for the starter.

Issue Link

#1636

Type of change

New feature (non-breaking change which adds functionality)

Decision

  • Allow Geth to take invalid tx. The config doesn't affect geth util

TODO

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Jan 10, 2024
@ChihChengLiang ChihChengLiang force-pushed the runtime-config branch 2 times, most recently from 95675e2 to bf59a9b Compare January 17, 2024 16:56
@ed255 ed255 linked an issue Jan 18, 2024 that may be closed by this pull request
Add runtime feature set #1636
Closed
@ChihChengLiang ChihChengLiang marked this pull request as ready for review January 22, 2024 18:16
@ChihChengLiang ChihChengLiang mentioned this pull request Jan 22, 2024
Open
ed255
ed255 approved these changes Jan 24, 2024
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 to me! I think the way the runtime config is implemented is very nice; in particular it's not very intrusive! And the features are now available anywhere where they are needed; to allow the implementation to take different paths as necessary without making thinks too complex :)

I'm approving already even though I left some comments and suggestions; as I think they are minor and don't affect the current implementation. The most serious one is the state height map discussion.

bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit.rs Show resolved Hide resolved
zkevm-circuits/src/evm_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit.rs Outdated
@@ -457,7 +478,7 @@ mod evm_circuit_stats {
fn evm_circuit_unusable_rows() {
assert_eq!(
EvmCircuit::<Fr>::unusable_rows(),
unusable_rows::<Fr, EvmCircuit::<Fr>>(()),
unusable_rows::<Fr, EvmCircuit::<Fr>>(FeatureConfig::default()),
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if different feature configs could lead to different unusable rows 🤔

zkevm-circuits/src/evm_circuit/param.rs Outdated
&mut meta,
FeatureConfig {
// Enable invalid_tx to get ExecutionState height
invalid_tx: true,
Copy link
Member

Choose a reason for hiding this comment

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

What happens when we use this step_height_map with the feature invalid_tx: false? Does any state height change? Or is it only that we have an extra one for the InvalidTx state?
I think we could have the situation where a a state height changes, maybe not now?

I think it's worth adding a check to make sure that all common states between step_height_map built out of different FeatureConfigs have the same height, to detect bugs as soon as possible.

As an example. with the invalid_tx feature, the EndTx constraints change a bit. Imagine this change causes an extra cell to be allocated, and then we have that height[EndTx]=5 with invalid_tx=false, and height[EndTx]=6 with invalid_tx=true! Provably this is not the case right now, but I imagine this situation could happen with other features.

zkevm-circuits/src/super_circuit.rs Show resolved Hide resolved
zkevm-circuits/src/test_util.rs Show resolved Hide resolved
Brechtpd
Brechtpd approved these changes Jan 25, 2024
View reviewed changes
Copy link
Collaborator

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

Very nice! Only some really unimportant comments from me, the step heights issue pointed out by Edu I think indeed potentially problematic.

bus-mapping/src/circuit_input_builder.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/evm_circuit/execution.rs Outdated Show resolved Hide resolved
ed255
ed255 approved these changes Jan 31, 2024
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.

I've reviewed the latest changes on this PR and all looks good to me!

@ChihChengLiang ChihChengLiang added this pull request to the merge queue Jan 31, 2024
Merged via the queue into main with commit fc4964c Jan 31, 2024
15 checks passed
@ChihChengLiang ChihChengLiang deleted the runtime-config branch January 31, 2024 13:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ed255 ed255 ed255 approved these changes

@Brechtpd Brechtpd Brechtpd approved these changes

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member 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.

Add runtime feature set
3 participants
@ChihChengLiang @Brechtpd @ed255