Backend unification #417

lvella · 2023-07-24T15:41:21Z

Originally, Backend was an enum, whose the crate's user had to match, and then use the specific API for each of the backend, with specific concrete types and functions.

This my attempt to make the Backend API generic, so that the user doesn't need to know the specifics of the selected backend. This led to the runtime checks being moved from the CLI to the Backend, and in case the user tries to use a functionality not available in some backend, the error is raised from inside the backend implementation.

leonardoalt

Overall I like the direction of this refactoring, simplifying the backend's API to a single entry point. I also like some of the changes to the halo2 crate with the Halo2 struct.

However, before this PR, the backend API made distinctions between provers that need setup and provers that don't in the traits, disallowing bad paths by construction. This PR makes all that runtime dependent in a way that I find kinda hacky and personally don't like. Maybe I'm overreacting and it's fine, but we should discuss it.

@Schaeff should also take a look

backend/src/halo2_structs.rs

backend/src/lib.rs

backend/src/pilcom_cli/mod.rs

leonardoalt · 2023-08-02T08:47:49Z

backend/src/pilcom_cli/mod.rs

+    }
+}
+
+fn write_constants_to_fs<T: FieldElement>(


I think the FS stuff should all go to CLI instead of backend

leonardoalt · 2023-08-02T08:48:56Z

halo2/src/prover.rs


 /// Create a halo2 proof for a given PIL, fixed column values and witness column values
 /// We use KZG ([GWC variant](https://eprint.iacr.org/2019/953)) and Keccak256
+pub struct Halo2<F: FieldElement> {


Halo2Prover maybe

leonardoalt · 2023-08-02T08:49:06Z

halo2/src/prover.rs

-    let params = ParamsKZG::<Bn256>::read(&mut params).unwrap();
-    prove_ast(pil, fixed, witness, params)
-}
+    pub fn read(input: &mut impl io::Read) -> Result<Self, io::Error> {


new_with_params maybe

I still think this should be applied

forgot this one

Schaeff

I like that it has a simple API so I'm not against having it now.
But I can't help but think that this would be better structured with a bunch of traits like these.

As I understand it, this PR uses dynamic dispatch to go from a user-defined backend argument to specific implementations of a single trait. Dynamic dispatch has to happen somehow anyway and I like the way it's done here. But what is stopping us from having many traits instead of one here? So for example when running a setup:

pub trait SetupBackendBuilder<T: FieldElement> {
    fn setup_from_params(&self, size: DegreeType) -> Result<Box<dyn SetupBackend<T>>, NotApplicable>;
}

Then backends which don't have that functionality can return an error here, and we can simplify the actual implementations as the underlying traits are simpler, for example AggregationBackend can have an API geared towards aggregation, etc.

Schaeff · 2023-08-02T17:15:36Z

halo2/src/circuit_builder.rs

@@ -36,7 +36,7 @@ pub(crate) fn analyzed_to_circuit<T: FieldElement>(

    let query = |column, rotation| Expr::Var(PlonkVar::Query(ColumnQuery { column, rotation }));

-    let mut cd = CircuitData::from(fixed, witness, &analyzed.constants);
+    let mut cd = CircuitData::from(fixed.to_owned(), witness, &analyzed.constants);


This change seems unrelated to this PR, but doesn't this do a lot of copying now? We could go all the way and make CircuitData take fixed as reference, the extra selectors which prevent that can be stored separately.

I tried, but CircuitData needs a Vec as it has to append to it.

If a Vec is needed inside a function, I have a preference on taking it directly as the argument.

That's my point with the selectors: CircuitData needs to introduce 2 selectors __enable_cur and __enable_next. Instead of adding them to the fixed columns, we can just store them in another struct field in CircuitData, then fixed doesn't need to be mutable. Then when hitting halo2 we can add these selectors: fixed: cd.fixed.iter().chain(once(cd.sel0)).chain(cd.sel1). It's sad to clone the whole fixed data for 2 selectors.

backend/src/halo2_structs.rs

lvella · 2023-08-03T12:10:12Z

So, @Schaeff and @leonardoalt prefer I separate the backends in trait categories. There are two dimensions:

Serializable and non-serializable (for slow setups, the setup can be written to disk);
Aggregation-able and non-aggregation-able;

The cross product gives 4 traits.

Is this a good split? Currently we have 3 backends.

leonardoalt · 2023-08-03T12:58:45Z

I wouldn't called it Serializable/non-serializable because it's not clear what's being serialized, and also it's not directly related to serialization but rather pre-setup. I would call it something like "ProverWithSetup"/"ProverWithoutSetup" like we had before.

The aggregation one, I'm fine with having an Option<Vec<Proof>> as an argument and that decides whether it's an aggregated proof or not, so we'd only have 2 traits for now.

leonardoalt

comments for the new backend stuff

backend/src/lib.rs

leonardoalt · 2023-08-04T17:01:13Z

backend/src/lib.rs

+        witness: Option<&[(&str, Vec<F>)]>,
+        prev_proof: Option<Proof>,
+        output_dir: &Path,
+    ) -> io::Result<()>;


why doesn't it return a proof?

See this: #417 (comment)

leonardoalt · 2023-08-04T17:01:47Z

backend/src/lib.rs

+        witness: Option<&[(&str, Vec<F>)]>,
+        prev_proof: Option<Proof>,
+        output_dir: &Path,
+    ) -> io::Result<()>;


should return proof?

See this: #417 (comment)

Actually thinking about it now I disagree with it. The client should receive the proof and decide what to do with it. The way we're doing this now, no one will be able to use the backend crate via powdr for any needs if the proofs always get written to disk, especially in cases of recursion.

I think it should return option proof

leonardoalt · 2023-08-04T17:03:57Z

backend/src/lib.rs

+impl BackendType {
+    pub fn build<T: FieldElement>(&self) -> &'static dyn BackendFactory<T> {
+        #[cfg(feature = "halo2")]
+        const HALO2_FACTORY: WithSetupFactory<halo2::Halo2Prover> = WithSetupFactory(PhantomData);


why does this one use halo2::... and Mock uses halo2_structs?

Because Halo2Prover is defined in the halo2 crate, and the mock is defined in the module halo2_structs of the backend crate that makes use of the halo2 crate.

leonardoalt · 2023-08-07T14:28:02Z

Had a call with @lvella, missing items until merge:

Re-swap "proof.bin" and "proof_aggr.bin" file names
rename factory build to something else
prove should return Result<Option<Proof>, ...>
Rename halo2_structs.rs to halo2_impl.rs
pilcom_cli/mod.rs::prove should return error instead of unimplemented

Schaeff · 2023-08-07T17:25:59Z

backend/src/halo2_impl.rs

        };

-        {
+        if let Some(output_dir) = output_dir {


Now that the proof is returned, we don't need to write it to disk. In general the only crate which has fs access should be powdr_cli.

This in turn should simplify the function signatures as output_dir shouldn't be required?

Me and Leo talked earlier and we considered all that. I think he prefers your way, but hear my reasons:

In our current model we have an output dir and dump all the compilation artifacts there, with predefined names. The way I propose, we keep this model, and let the backend control the naming of the artifacts it can generate.

If we are to keep the pilcom as a backend, it needs to know the output dir, because all it does is write files.

In every place we call the backend to write files (there are currently two), we want the writing of files to be consistent. So, the natural place for this function to be is in the backend.

I think the main culprit here is the pilcom backend. It doesn't return a proof and creates a non-proof file that needs to be written to disk in our current workflow.

In general the only crate which has fs access should be powdr_cli.

I agree with this, but I don't know how to solve it for the pilcom artifact because

... So, the natural place for this function to be is in the backend.

I also agree with this.

I would still prefer the proof not to be written here, but that doesn't solve pilcom.

backend/src/halo2_impl.rs

leonardoalt · 2023-08-07T22:19:24Z

halo2/src/prover.rs

-        params.downsize(degree);
-        params
-    };
+    pub fn write(&self, output: &mut impl io::Write) -> Result<(), io::Error> {


Is this supposed to be write_setup or just write? Either fine with me

yes, it was supposed to be write_setup

lvella marked this pull request as draft July 24, 2023 15:41

Mostly done.

165dd57

lvella force-pushed the backend-unification branch from e355bd1 to 165dd57 Compare July 28, 2023 19:03

lvella added 6 commits July 31, 2023 18:05

With all features enabled, it at least compiles.

36cc2e4

Fixed some bugs.

ce2fbbc

Being explicit about degree type.

5d4a5f6

More tests passing.

3604afe

Even more tests passing.

a10732a

Restoring optional Halo2 compilation.

397fd82

lvella marked this pull request as ready for review August 1, 2023 21:18

Merge branch 'main' into backend-unification

a745483

leonardoalt reviewed Aug 2, 2023

View reviewed changes

lvella added 3 commits August 2, 2023 15:52

Fixing bad merge.

9c58166

Fixing lint errors.

2a773b3

More lint fixes.

1709b1c

Schaeff reviewed Aug 2, 2023

View reviewed changes

Merge branch 'main' into backend-unification

7a9111a

Split of the backend implementations into trait categories.

3ead294

lvella requested review from Schaeff and leonardoalt August 3, 2023 18:28

leonardoalt reviewed Aug 3, 2023

View reviewed changes

leonardoalt mentioned this pull request Aug 4, 2023

Nova backend in Supernova R1CS #453

Closed

8 tasks

lvella added 2 commits August 4, 2023 17:23

Some renamings.

2a86f70

Much better function names.

3d6f36d

leonardoalt reviewed Aug 4, 2023

View reviewed changes

Removed option from witness.

e3069a9

Pleasing the lint.

594601d

Returning the proof.

c6343b9

lvella requested a review from leonardoalt August 7, 2023 16:41

Schaeff reviewed Aug 7, 2023

View reviewed changes

backend/src/halo2_impl.rs Outdated Show resolved Hide resolved

Removing filesystem stuff from backend.

e89924a

leonardoalt previously approved these changes Aug 7, 2023

View reviewed changes

lvella added 2 commits August 7, 2023 23:11

Renaming function.

366f5d0

Merge remote-tracking branch 'origin/main' into backend-unification

ee0aaf9

lvella dismissed leonardoalt’s stale review via ee0aaf9 August 7, 2023 22:17

leonardoalt reviewed Aug 7, 2023

View reviewed changes

leonardoalt previously approved these changes Aug 7, 2023

View reviewed changes

Another function rename.

b5870d4

lvella dismissed leonardoalt’s stale review via b5870d4 August 8, 2023 11:22

Merge branch 'main' into backend-unification

4a55617

leonardoalt approved these changes Aug 8, 2023

View reviewed changes

leonardoalt enabled auto-merge (squash) August 8, 2023 11:56

leonardoalt merged commit c817571 into main Aug 8, 2023
2 checks passed

leonardoalt deleted the backend-unification branch August 8, 2023 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend unification #417

Backend unification #417

lvella commented Jul 24, 2023 •

edited

Loading

leonardoalt left a comment

leonardoalt Aug 2, 2023

leonardoalt Aug 2, 2023

lvella Aug 4, 2023

leonardoalt Aug 2, 2023

leonardoalt Aug 7, 2023

lvella Aug 7, 2023

Schaeff left a comment •

edited

Loading

Schaeff Aug 2, 2023

lvella Aug 3, 2023

Schaeff Aug 4, 2023 •

edited

Loading

lvella commented Aug 3, 2023

leonardoalt commented Aug 3, 2023

leonardoalt left a comment

leonardoalt Aug 4, 2023

lvella Aug 4, 2023

leonardoalt Aug 4, 2023

lvella Aug 4, 2023

leonardoalt Aug 4, 2023 •

edited

Loading

leonardoalt Aug 4, 2023

lvella Aug 7, 2023

leonardoalt Aug 4, 2023

lvella Aug 4, 2023

leonardoalt commented Aug 7, 2023 •

edited by lvella

Loading

Schaeff Aug 7, 2023

Schaeff Aug 7, 2023

lvella Aug 7, 2023 •

edited

Loading

leonardoalt Aug 7, 2023

leonardoalt Aug 7, 2023

lvella Aug 8, 2023

Backend unification #417

Backend unification #417

Conversation

lvella commented Jul 24, 2023 • edited Loading

leonardoalt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Schaeff left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Schaeff Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

lvella commented Aug 3, 2023

leonardoalt commented Aug 3, 2023

leonardoalt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardoalt Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardoalt commented Aug 7, 2023 • edited by lvella Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvella Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvella commented Jul 24, 2023 •

edited

Loading

Schaeff left a comment •

edited

Loading

Schaeff Aug 4, 2023 •

edited

Loading

leonardoalt Aug 4, 2023 •

edited

Loading

leonardoalt commented Aug 7, 2023 •

edited by lvella

Loading

lvella Aug 7, 2023 •

edited

Loading