Aatif's Personal Hit List

[0xaatif]

In no particular order...

• [x] Cargo.lock We definitely want this for reproducibility, especially when we're not pinned to an alloy revision. No-one wants to bisect versions. See also Change in Guidance on Committing Lockfiles from the Cargo Team.
• [ ] One crate trace_decoder etc should really be modules in a single crate.
• [x] One repo zero-bin should live in zk_evm
• [ ] One branch Just one main, no develop, or long lived feat branches
• [ ] No nightly rustc #[feature(..)]s.
• [ ] No cargo --features for conditional compilation.
• [ ] Configurable difficulty so we exercise the same codepaths in test code. (No --feature test_only)
• [ ] Stable toolchain
  • [ ] Depends on https://github.com/0xPolygonZero/plonky2/issues/417
• [ ] Publish rustdoc
• [ ] Remove mpt_trie
• [ ] sane nibbles structure
• [ ] use bitvec instead of Bits in smt_trie
• [ ] robust smt_trie library
• [ ] link checker

[Nashtare]

Hey @0xaatif, could you expand on some of these items for clarity (like the One crate / One branch / Configurable difficulty ones)?

Additionally, here are some comments for the others:

• Cargo.lock: I'm not sure I understand the point of having one here, as this is a library and as such would be ignored in applications like zero-bin anyway.
• No --features: this one can probably be elided indeed, as we most likely will not run the zkEVM on WASM, and I don't recall any scenario where we had to switch off one default feature.
• Stable toolchain: this is going to require getting plonky2 to compile on stable as well, as it currently relies on specialization (see https://github.com/0xPolygonZero/plonky2/issues/417) and stdarch_x86_avx512. Removing the latter would incur performance loss as disabling AVX512 support.

[Nashtare]

So to comment on the updated list:

Cargo.lock: We definitely want this for reproducibility, especially when we're not pinned to an alloy revision.

Agreed, though the problem here is the alloy revision not being pinned I believe. We should have fixed the version we use on zero-bin side instead of leaving it floating around. Note that, related to #229, we cannot have alloy here or we won't be able to make new releases, at least until alloy gets its own versions on crates.io.

One crate: trace_decoder etc should really be modules in a single crate.

I disagree here. These crates could independently be used by external projects, and grouping everything seems messy to me. Instead, I'd suggest

• combining trace_decoder & proof_gen together. If the trace_decoder refactoring yields significant codelines savings, then it would make sense merging it into another crate, as decoding module of zero-bin for instance
• getting rid of mpt_trie if we can find an already existing alternative that does what we want, is maintained regularly enough. Note that we have an smt_trie crate pending for the type2 prover in feat/type2, for which there most likely isn't any Rust alternative.

One repo: zero-bin should live in zk_evm

There was a long discussion around combining or not libraries and binaries built on top, we ended up keeping them separate. cc @cpubot

One branch: Just one main, no develop, or long lived feat branches

I don't understand how we would do without feat/ branches for some projects. The current ones have been in progress for several months and some are still not complete. Work like the cancun upgrade can't be easily made incremental without breaking both the current Shanghai compat' and also not being Cancun compatible (until feature completeness).

No cargo --features for conditional compilation.

While not the case at the moment, the plan was to enable conditionally either the type-1 or the type-2 prover, through feature flags. This would have the benefit of helping long-term maintenance and reducing code duplication. Having both provers live together would have an impact on these points.

Stable toolchain

I don't really see the need, for now, to go away from nightly, especially as this would incur performance hindrance. There are even projects in production still requiring nightly, usually through a pinned version of the compiler.

[BGluth]

Interesting about the official stance on Cargo.lock in libraries changing, thanks for linking that!

One branch: Just one main, no develop, or long lived feat branches

Hmm... Despite merging directly to main in the past, I really don't think we should go back to this setup. My core thought is it's really nice to have a "buffer" branch (develop) to merge PRs into to protect against bugs introduced by PRs. I think it's worthwhile doing this to:

• Keep main more stable.
• Make it less of a big deal to break large PRs into smaller separate ones (I'm a lot less comfortable doing this against main vs. develop).

Stable toolchain

While this is nice, IIRC plonky2 relies on nightly and I think this is going to remain the case for quite some time due to us wanting to get as much runtime performance out as possible.

One crate: trace_decoder etc should really be modules in a single crate.

Just going to add a bit more onto what Nashtare already wrote.

Part of the initial motivation for having trace_decoder as it's own crate was so other projects can link against it and use our trace_protocol in their own implementations since the plan was to create a proof protocol that other chains could also use.

There's also a bit of a preference I think (still debated a bit) in the Rust community to prefer smaller crates and break separate concerns into crates instead of sub-modules when it's possible that other projects could also use the logic.

No cargo --features for conditional compilation.

While I think this something we probably want in the long term, we absolutely can not do this in the short term. Specifically, SMT support in plonky2 makes changes that break MPT support. Eventually the plan is to abstract the changes in the feat/type2 branch back into main, but this is going to take a while. The current plan is to use feature gating to link against different versions of evm_arithmetization until this abstraction is added.

Configurable difficulty so we exercise the same codepaths in test code. (No --feature test_only)

Yeah maybe... Can we do this nicely without using a feature flag though?

We pretty much can not be generating full proofs during cargo test (ie. massive memory requirements that will trigger the OOM killer on most people's machines). We want to be able to test proof gen during testing, but it probably should be made explicit to the user that they are testing proof gen and not just witness generation (eg. by invoking a script or explicitly opting-in during cargo test).

[atanmarko]

Just one main, no develop, or long lived feat branches

This could work on some smaller project in maintenance mode, not on the type of project zk_evm is

Configurable difficulty so we exercise the same codepaths in test code. (No --feature test_only)

Circuit sizes are very fragile parameter, proof generation could easily break if you try to make them "less difficult".

No cargo --features for conditional compilation.

Why not? Most of the non-trivial mainstream Rust projects use features.