Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add feature-gating in prover code based on target network #598

Merged
merged 19 commits into from
Sep 6, 2024
Merged

feat: Add feature-gating in prover code based on target network #598

merged 19 commits into from
Sep 6, 2024

Conversation

Nashtare
Copy link
Collaborator

@Nashtare Nashtare commented Sep 5, 2024 •
edited
Loading

We were previously mixing bits of eth mainnet, cdk-erigon, etc...

Now the prover (i.e. evm_arithmetization) supports 3 network features:

  • eth_mainnet (by default for now): full Ethereum equivalence, Cancun HF
  • polygon_pos: Prunes the ignored bits of Cancun from Napoli upgrade1 and supports incoming PIP-302
  • cdk_erigon: Prunes the ignored bits of Cancun irrelevant for CDK, includes LxLy pre-state execution3 and (yet unimplemented) post-state execution4, as well as the EIP-1559 variant for L2s.

Notes:

  • Removed the parallel feature (as we will always want it enabled, and this allows to not mess up when calling --no-default-features to change network)
  • Commented out the bls12 Kernel file as being unused at the moment
  • Tweaked all integration tests (apart from global_exit_root) to run under eth_mainnet feature flag.

Based on #594

closes #575
closes #563

Footnotes

  1. https://forum.polygon.technology/t/pip-33-napoli-upgrade/13405 ↩

  2. https://forum.polygon.technology/t/pip-37-ahmedabad-hardfork/13885 ↩

  3. cdk-erigon: Update LxLy pre-block processing logic (post Etrog) #436 ↩

  4. cdk-erigon: Implement post-block execution LxLy logic  #437 ↩

@Nashtare Nashtare added this to the System strengthening milestone Sep 5, 2024
@Nashtare Nashtare self-assigned this Sep 5, 2024
@github-actions github-actions bot added crate: trace_decoder Anything related to the trace_decoder crate. crate: evm_arithmetization Anything related to the evm_arithmetization crate. labels Sep 5, 2024
@Nashtare Nashtare changed the base branch from develop to feat/pre_state_cdk September 5, 2024 23:50
@github-actions github-actions bot removed the crate: trace_decoder Anything related to the trace_decoder crate. label Sep 5, 2024
@Nashtare Nashtare removed the request for review from 0xaatif September 5, 2024 23:53
muursh
muursh approved these changes Sep 6, 2024
View reviewed changes
Copy link
Member

@muursh muursh left a comment

Choose a reason for hiding this comment

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

Really nice job

atanmarko
atanmarko approved these changes Sep 6, 2024
View reviewed changes
evm_arithmetization/src/cpu/kernel/aggregator.rs Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@hratoanina hratoanina self-requested a review September 6, 2024 14:36
@Nashtare Nashtare mentioned this pull request Sep 6, 2024
Merged
Base automatically changed from feat/pre_state_cdk to develop September 6, 2024 16:23
@github-actions github-actions bot added crate: proof_gen Anything related to the proof_gen crate. crate: zero_bin Anything related to the zero-bin subcrates. labels Sep 6, 2024
@github-actions github-actions bot added the crate: trace_decoder Anything related to the trace_decoder crate. label Sep 6, 2024
hratoanina
hratoanina approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@hratoanina hratoanina left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

evm_arithmetization/src/cpu/kernel/asm/core/exception.asm
@@ -327,8 +327,16 @@ min_stack_len_for_opcode:
BYTES 0 // 0x46, CHAINID
BYTES 0 // 0x47, SELFBALANCE
BYTES 0 // 0x48, BASEFEE
BYTES 1 // 0x49, BLOBHASH
BYTES 0 // 0x4a, BLOBBASEFEE
#[cfg(feature = eth_mainnet)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think an invalid instruction will be caught elsewhere anyway, so we don't need to make a distinction here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems weird to me to have BYTES 1 lying around for an instruction that doesn't exist. Even though as you point this would get caught elsewhere, I think we'd rather not have invalid Kernel bits if we have workarounds

evm_arithmetization/src/cpu/kernel/aggregator.rs
} else {
158
// unreachable
0
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is there no way to panic here in a const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can't compile if you do not have a usize (like if you were to rely on unreachable!() for instance), and I didn't want to have the else only be equivalent to the last network feature in case we were to add another chain later on (as it would easily mess things up)

evm_arithmetization/src/cpu/kernel/asm/transactions/router.asm Outdated Show resolved Hide resolved
evm_arithmetization/src/lib.rs Show resolved Hide resolved
evm_arithmetization/src/proof.rs
@@ -968,59 +968,45 @@ impl BurnAddrTarget {
}
}

#[cfg(feature = "cdk_erigon")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just put the entire section concerning the burn address behind this cfg?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would require to conditionally remove this field from PublicValuesTarget but I think we'd prefer keep it consistent with the non target version

evm_arithmetization/src/recursive_verifier.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/get_challenges.rs Show resolved Hide resolved
zero_bin/common/Cargo.toml Outdated Show resolved Hide resolved
evm_arithmetization/src/cpu/kernel/tests/mod.rs Show resolved Hide resolved
@Nashtare Nashtare merged commit 223a0d9 into develop Sep 6, 2024
17 checks passed
@Nashtare Nashtare deleted the feat/feature_gate_chain branch September 6, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atanmarko atanmarko atanmarko approved these changes

@muursh muursh muursh approved these changes

@hratoanina hratoanina hratoanina approved these changes

@LindaGuiga LindaGuiga Awaiting requested review from LindaGuiga LindaGuiga is a code owner

Assignees

@Nashtare Nashtare

Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate. crate: proof_gen Anything related to the proof_gen crate. crate: trace_decoder Anything related to the trace_decoder crate. crate: zero_bin Anything related to the zero-bin subcrates.
Projects
Zero EVM
Status: Done
Milestone
Type 1 - Polygon PoS / shadow proving...
4 participants
@Nashtare @muursh @atanmarko @hratoanina