WIP: Modularize backends #30

katat · 2024-03-28T06:58:34Z

Refactor with ark_ff::Field
Modularize backends
Existing tests passed
Add R1CS backend

katat · 2024-03-28T08:23:17Z

The changes from the actual pallas field type to trait ark_ff::Field propagate to the variables such as:

These variables are const or static, so they won't be able to support using generic type without make some complex tricks using keywords such as Box or dyn.

I am wondering if it makes more sense to have the NAST struct to encapsulate these variables, so they can inherit the generic type from their container.

mimoo

First of all this looks great, I think this is a great step towards what we want to achieve. I've left a number of comments, and I will continue to do so, but I wanted to get the big ones now rather than later. To summarize:

can you create a separate PR that we can merge first for the refactor of the builtins?
there's a number of changes that are unrelated to this PR, could you also move them to another PR? This would make reviewing this PR easier as I can focus on the main changes :)
I've noted a number of places where I think things should be moved to the backend, let me know if these make sense, otherwise we can have a convo on that :o

mimoo · 2024-03-28T17:50:28Z

src/constants.rs

@@ -5,7 +5,8 @@
 use serde::{Deserialize, Serialize};

 /// We use the scalar field of Vesta as our circuit field.
-pub type Field = kimchi::mina_curves::pasta::Fp;
+// pub type Field = kimchi::mina_curves::pasta::Fp;
+pub use ark_ff::Field;


(in the final PR this should be imported in relevant file instead, but great trick to avoid too many changes now!)

mimoo · 2024-04-11T01:35:36Z

src/asm.rs


-impl CircuitWriter {


so, now I'm wondering if the ASM functions should be implemented directly on each specific Backend, or each specific Backend::CompileCircuit type. I don't think it makes sense to have a generic implementation here

mimoo · 2024-04-11T01:36:44Z

src/backends/kimchi/fp_kimchi.rs

+
+use std::str::FromStr;
+
+pub fn params<F>() -> ArithmeticSpongeParams<F>


can't we just import the one from mina?

This is used by the generic poseidon function, that forces this params function to have generic field.

So looks like this poseidon builtin function should be implemented for each backend, then we can just import the params from the kimchi.

my question was, why not import https://github.com/o1-labs/proof-systems/blob/master/poseidon/src/pasta/fp_kimchi.rs#L9

but I guess the problem here is that this crate is not published on crates.io :D

oh and yeah this should not have a generic parameter here, it should be specific for a backend that has a specific field.

mimoo · 2024-04-11T01:38:02Z

src/backends/kimchi/mod.rs

+pub const NUM_REGISTERS: usize = kimchi::circuits::wires::COLUMNS;
+
+#[derive(Debug, Clone, Default)]
+pub struct Kimchi {


perhaps we should call this backend kimchi_vesta and KimchiVesta respectively (as it is defined on the scalar field of Vesta)

mimoo · 2024-04-11T01:38:40Z

src/backends/kimchi/mod.rs

+        }
+    }
+
+    fn add_constraint(


I would suggest keeping add_generic_gate for now so you don't have to rename it everywhere else

mimoo · 2024-04-11T04:12:56Z

src/witness.rs

    /// kimchi uses a transposed witness
-    pub fn to_kimchi_witness(&self) -> [Vec<Field>; NUM_REGISTERS] {
-        let transposed = vec![Vec::with_capacity(self.0.len()); NUM_REGISTERS];
+    pub fn to_kimchi_witness(&self) -> [Vec<F>; NUM_REGISTERS] {


sounds like this should be moved to the kimchi backend module as well

mimoo · 2024-04-11T04:13:20Z

src/witness.rs

+    pub fn to_kimchi_witness(&self) -> [Vec<F>; NUM_REGISTERS] {
+        let transposed = (0..NUM_REGISTERS)
+            .map(|_| Vec::with_capacity(self.0.len()))
+            .collect::<Vec<_>>();


why this change?

mimoo · 2024-04-11T04:16:30Z

src/compiler.rs

+pub struct GeneratedWitness<B: Backend> {
+    pub all_witness: Witness<B::Field>,
+    pub full_public_inputs: Vec<B::Field>,
+    pub public_outputs: Vec<B::Field>,


need documentation

all_witness is the entire witness vector, comprising the public input and the private input

full_public_inputs is the public input part of the previous vector, including the public output at the end

public_outputs is only the public output part of the previous vector

to be honest I don't remember why I returned that :)

(seems kinda inefficient)

mimoo · 2024-04-11T04:20:08Z

src/witness.rs

@@ -196,12 +201,15 @@ impl CompiledCircuit {
        // compute each rows' vars, except for the deferred ones (public output)


so I think what comes after should be part of the backend as well, this is very specific to kimchi/plonk

so perhaps the backend should have a fn generate_witness() which we could call at this point

mimoo · 2024-04-11T04:31:49Z

src/stdlib/mod.rs

+
+pub fn has_builtin_fn(name: &str) -> bool {
+    BUILTIN_FNS_SIGS.iter().any(|(_, s)| s.name.value == name)
+}


can you create a PR that we could merge first that refactors the builtins? This would simplify this PR :o

mimoo · 2024-04-11T04:50:08Z

src/cli/cmd_build_and_check.rs

    // produce all TASTs and stop here
-    produce_all_asts(&curr_dir)?;
+    produce_all_asts::<Kimchi>(&curr_dir)?;


I think this is the right approach, we can have another PR later to introduce different choices of backend 👍

katat · 2024-04-17T03:05:27Z

The other PR with same purpose was merged.

katat changed the title ~~WIP: refactor using ark_ff::Field~~ WIP: Modularize backends Mar 28, 2024

katat force-pushed the backend/r1cs branch from 8274a28 to e3477bc Compare April 10, 2024 07:48

katat added 2 commits April 10, 2024 16:41

refactor: modularize circuit backend

4b87026

chore: clippy

502a6a5

katat force-pushed the backend/r1cs branch from e3477bc to 502a6a5 Compare April 10, 2024 08:41

mimoo reviewed Apr 11, 2024

View reviewed changes

katat mentioned this pull request Apr 13, 2024

Refactor using backend trait #35

Closed

katat closed this Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Modularize backends #30

WIP: Modularize backends #30

katat commented Mar 28, 2024 •

edited

Loading

katat commented Mar 28, 2024

mimoo left a comment

mimoo Mar 28, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

katat Apr 11, 2024

mimoo Apr 12, 2024

mimoo Apr 12, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

mimoo Apr 11, 2024

katat commented Apr 17, 2024


		use std::str::FromStr;

		pub fn params<F>() -> ArithmeticSpongeParams<F>

		@@ -196,12 +201,15 @@ impl CompiledCircuit {
		// compute each rows' vars, except for the deferred ones (public output)

WIP: Modularize backends #30

WIP: Modularize backends #30

Conversation

katat commented Mar 28, 2024 • edited Loading

katat commented Mar 28, 2024

mimoo 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

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

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

katat commented Apr 17, 2024

katat commented Mar 28, 2024 •

edited

Loading