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.

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

fix: revm bn254 mul issue + cancun support + misc issues #222

Merged
merged 6 commits into from
May 19, 2024
Merged

Conversation

Brechtpd
Copy link
Contributor

@Brechtpd Brechtpd commented May 19, 2024

Some fixes:

  1. Our revm incorrectly set the bn 254 mul precompiles in revm which made them not work correctly, it seems this was introduced only sometime last week when switching to the feat/zk-op revm branch. This is now fixed in the v35_taiko_v2 branch: taikoxyz/revm@ec20417
  2. Cancun support did not always work correctly because the point precompile is only enabled for cancun when the c-kzg feature is enabled, so each block containing a call to this precompile would not work correctly. For cancun support in non-native provers we will also have to make sure that KzgProof::verify_kzg_proof (https://github.com/taikoxyz/revm/blob/a5c01f345f4362f4d5d8bbddf37e9d8aeb27b0d3/crates/precompile/src/kzg_point_evaluation.rs#L73) works correctly without file IO. I haven't tested it yet, but I think it should be okay because revm also avoids using file io to load in the trusted setup data: https://github.com/taikoxyz/revm/blob/a5c01f345f4362f4d5d8bbddf37e9d8aeb27b0d3/crates/primitives/src/kzg/env_settings.rs#L51. In any case, Taiko doesn't support cancun yet so we don't really need this to work for all provers yet.
  3. Fixes MPT issue introduced in chore: pedantic lints #213: 59dc5fd
  4. Added check that the passed in chain spec is correct when a known chain id is used: c1cbfb8
  5. Switched to a "paid" beacon api RPC node so that the tests can run again

Issues 1/2 can be triggered in ethereum block 19899808 at tx 65, so that block can be used for testing.

@Brechtpd Brechtpd changed the title fix: revm bn254 mul/mod issue + cancun support fix: revm bn254 mul issue + cancun support May 19, 2024
@CeciliaZ030
Copy link
Contributor

Should we start doing PR in revm main instead of using branches?

@Brechtpd
Copy link
Contributor Author

Brechtpd commented May 19, 2024

The difficulties of having different repos. We should do PRs but not sure what mechanism would be best. Maybe we should do PRs to the main branch on revm side and use specific revs on the raiko side, each revm update also requires a PR in raiko to update the revs which I guess is fine.

@Brechtpd Brechtpd changed the title fix: revm bn254 mul issue + cancun support fix: revm bn254 mul issue + cancun support + misc issues May 19, 2024
@@ -56,7 +56,7 @@
"l1_contract": null,
"l2_contract": null,
"rpc": "https://ethereum-holesky-rpc.publicnode.com",
"beacon_rpc": "https://api.holesky.blobscan.com",
"beacon_rpc": "https://fabled-weathered-cherry.ethereum-holesky.quiknode.pro/8f1c66935fa5f9afbda0db43318fe3c9e7b061e1/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we hide this one into maybe CI secrets to prevent unauthorized accesses from others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should have a dedicated account for our CI. I'm not paying for this one yet because I just picked the free tier but can buy more capacity if needed. I guess we can see if it's a problem or not, hopefully there's enough free rpcs out there that people won't abuse this one.

// TODO: we should probably split things up in critical and non-critical parts
// in the chain spec itself so we don't have to manually all the ones we have to care about.
if let Some(verified_chain_spec) =
SupportedChainSpecs::default().get_chain_spec_with_chain_id(input.chain_spec.chain_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems it also relies on fixed image_id checking on-chain (in sgx it's mrenclave, in r0/sp1 it's image_id), as the default value can be override by user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! But the config is injected in the code, so it's part of the program itself for both SGX and zkVMs so it will already be part of whatever system is used to ensure the correct code is executed.

Cargo.toml Show resolved Hide resolved
@Brechtpd Brechtpd added this pull request to the merge queue May 19, 2024
Merged via the queue into main with commit d90acd0 May 19, 2024
13 checks passed
@Brechtpd Brechtpd deleted the fix-revm branch May 19, 2024 16:18
@Brechtpd Brechtpd mentioned this pull request May 20, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants