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

Aatif's Personal Hit List #270

Open
4 of 18 tasks
0xaatif opened this issue Jun 9, 2024 · 4 comments
Open
4 of 18 tasks

Aatif's Personal Hit List #270

0xaatif opened this issue Jun 9, 2024 · 4 comments
Assignees
Milestone

Comments

@0xaatif
Copy link
Contributor

0xaatif commented Jun 9, 2024

In order:

  • Do a pass on mpt_trie #401
  • Refactor our scripts/*
  • Robust SMT library
    • use bitvec instead of Bits in smt_trie
  • Robust trace_decoder
  • zero_bin should be one crate with several binaries
  • One crate
    trace_decoder etc should really be modules in a single crate.
  • 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
  • Publish rustdoc
  • link checker

Done

  • 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 repo
    zero-bin should live in zk_evm
@Nashtare

This comment was marked as resolved.

@Nashtare
Copy link
Collaborator

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
Copy link
Contributor

BGluth commented Jun 11, 2024

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
Copy link
Member

atanmarko commented Jun 11, 2024

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.

@0xaatif 0xaatif self-assigned this Jun 12, 2024
@Nashtare Nashtare added this to the Misc. milestone Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

No branches or pull requests

4 participants