zcash / halo2

The Halo2 zero-knowledge proving system
https://zcash.github.io/halo2/
Other
694 stars 479 forks source link

Establish workflow for developing features in-situ #707

Open str4d opened 1 year ago

str4d commented 1 year ago

In the Halo 2 Dev Call today, one of the issues raised was the workflow for getting features into the codebase. Currently, features that land in this repository are subject to ECC's review policy, which is more strict than is necessarily appropriate for experimental features that are being developed. Meanwhile, experimenting with the union of a large set of in-development features is currently only practical in downstream forks, which makes the process of upstreaming more laborious. We decided to set up a workflow to enable in-situ development of experimental features.

This issue can be closed once we have a section of the Halo 2 book describing the feature development workflow, that dev teams can use to determine how to contribute features to the repository.

str4d commented 1 year ago

I suggested in the call that we follow a "nightly -> beta -> stable" workflow inspired by the Rust development workflow. We would add a beta feature flag, and a nightly feature flag that enables beta (names subject to finalization). Here's my first sketch of the feature workflow:

CPerezz commented 1 year ago

I think this is probably the best approach we can take. It will also be nice to doc this criteria in the repo so that it is very clear how it will work.

Will we have also nightly and beta branches and cherry-pick to main directly? Or which is the procedure you purpose? As merging directly into main can cause a significant reivew overhead for the ECC team and also a significant increase on the codebase overall complexity, introducing two new features that conditionally compile lots and lots of code.

The biggest caveat I see to this is that for upstreaming things like the Challenge API or even, the Traitified-PCS, the ammount of cfg flags needed to be introduced will almost duplicate the code that this codebase holds. (Which might be OK, but it's worth noticing and discussing even).

str4d commented 1 year ago

716 adds the initial harness corresponding to https://github.com/zcash/halo2/issues/707#issuecomment-1344870500. It notes that we might augment it with a branching approach later (more specifically in concert with a feature that needs it, to work out the workflow details), but for now just getting the feature-based harness in will enable some existing PRs to get reviewed and merged more quickly.