Refactor with a backend trait #36

katat · 2024-04-16T09:42:36Z

No description provided.

Please enter the commit message for your changes. Lines starting

mimoo

Beautiful, I left you some comments but I think we can merge this :)

mimoo · 2024-04-16T09:53:10Z

src/backends/mod.rs

+// TODO: should it be cloneable? It is now so because FnInfo needs to be cloneable.
+pub trait Backend: Clone {
+    /// The circuit field / scalar field that the circuit is written on.
+    type Field: Field + PrettyField;


nit: to avoid lines like Field: Field I often qualify, so that it looks like Field: ark_ff::Field

mimoo · 2024-04-16T09:57:17Z

src/lib.rs

@@ -4,6 +4,7 @@

 pub mod asm;
 pub mod circuit_writer;
+pub mod backends;


btw it dawns on me that we're not really supporting multiple backends here, but rather multiple constraint systems. Maybe we should rename backends to constraint systems? wdyt? (let's not do this in this PR though, not urgent, more of a philosophical discussion)

mimoo · 2024-04-16T09:59:30Z

src/stdlib/crypto.rs

    let ctx = &mut ParserCtx::default();
    let mut tokens = Token::parse(0, name).unwrap();
    let sig = FnSig::parse(ctx, &mut tokens).unwrap();

    let fn_handle = match name {
-        POSEIDON_FN => poseidon,
+        POSEIDON_FN => B::poseidon(),


btw I think a better idea here would be to have a default implementation if B::poseidon() returns None (maybe I can create an issue for that)

src/backends/kimchi/mod.rs

mimoo · 2024-04-16T10:02:07Z

src/circuit_writer/mod.rs

+    /// The constraint backend for the circuit.
+    /// For now, this needs to be exposed for the kimchi prover for kimchi specific low level data.
+    /// So we might make this private if the prover facilities can be deprecated.
+    pub backend: B,


pub(crate)?

mimoo · 2024-04-16T10:44:36Z

src/backends/kimchi/prover.rs

+impl KimchiVesta {
+    pub fn compile_to_indexes(
+        &self,
+        public_input_size: usize,


I still find it weird that we have to pass this, maybe it should live in the backend as well?

This seems to only related to the proving function that we might want to deprecate?
Maybe hold this decision until we decided if the proving function is to be removed.

mimoo · 2024-04-16T10:45:54Z

src/circuit_writer/writer.rs

-        if !self.finalized {
-            unreachable!();
-        }
-        &self.gates


I'm still wondering if we should still have a function like this, to make sure that someone is not using the constraint system directly when it's not finalized

It is checked before generating witness: https://github.com/katat/noname/blob/d48d95f76c7a7bcca5e6219379026a7c3ec181a8/src/backends/kimchi/mod.rs#L466

mimoo · 2024-04-16T10:48:48Z

src/circuit_writer/mod.rs

+        let returned_cells = circuit_writer.compile_main_function(fn_env, &function)?;
+        let main_span = circuit_writer.main_info().unwrap().span;
+        let private_input_indices = circuit_writer.private_input_indices.clone();
+        let public_output = circuit_writer.public_output.clone();


why the clones here instead of passing as a ref?

mimoo · 2024-04-16T11:08:58Z

src/circuit_writer/mod.rs

-        circuit_writer.compile_main_function(fn_env, &function)?;
+        let returned_cells = circuit_writer.compile_main_function(fn_env, &function)?;
+        let main_span = circuit_writer.main_info().unwrap().span;
+        let private_input_indices = circuit_writer.private_input_indices.clone();


I think private_input_indices should keep track of CellVars instead of indices, as indices is a backend implementation detail.

Maybe we should remove the pub in CellVar { pub index: usize and see what breaks

mimoo · 2024-04-16T11:10:58Z

src/circuit_writer/writer.rs

@@ -57,11 +57,11 @@ pub struct Gate

    /// Coefficients
    #[serde(skip)]
-    pub coeffs: Vec<Field>,
+    pub coeffs: Vec<VestaField>,
 }

 impl Gate {


all this kimchi-specific code should be moved to the backend stuff

mimoo · 2024-04-16T11:53:56Z

src/backends/kimchi/mod.rs

+            for (col, var) in row_of_vars.iter().enumerate() {
+                let val = if let Some(var) = var {
+                    // if it's a public output, defer it's computation
+                    if matches!(self.witness_vars()[&var.index], Value::PublicOutput(_)) {


instead: self.witness_vars[&var.index]

mimoo · 2024-04-16T11:55:48Z

src/backends/mod.rs

+
+    /// This provides a standard way to access to all the internal vars.
+    /// Different backends should be accessible in the same way by the variable index.
+    fn witness_vars(&self) -> &HashMap<usize, Value<Self>>;


ok so I think that we should not do that, instead we should expose this:

fn witness_vars(&self, var: &CellVar) -> Value<Self>;

and probably rename this to get_cellvar_value

mimoo · 2024-04-16T11:58:08Z

src/backends/mod.rs

+        span: Span,
+    ) -> CellVar;
+
+    fn debug_info(&self) -> &[DebugInfo];


yeah so I believe we could easily remove this

katat added 26 commits April 16, 2024 12:04

gitignore .vscode

5d32c1b

refactor: add backend type parameter

9a06998

move kimchi poseidon impl to kimchi folder

d188678

impl poseidon for kimchi

e4d43e4

add backend to circuit writer

446f352

move new_internal_var to backend

a978f7b

need to make Value cloneable because Backend is cloneable

8f43fb9

change to Arc from Box for hint

d596603

use backend's internal_new_var

207e051

remove dead code

8f667aa

add witness_vars to backend interface

7d88c04

move DebugInfo to backend

5050b86

Please enter the commit message for your changes. Lines starting

move add_gate to backend

fe482b0

refactor: use finalize_circuit

c7d95b4

move add_constants to backend

947b07d

move compute_var to backend

48227d3

move generate_witness to backend

446e8e0

move asm to backend

1ded0a9

move generate_asm to backend

4946a35

move to_kimchi_witness to backend

d8701a5

move prover to backend

d301f24

refactor: impl prover for kimchi vesta backend

10573ba

fix: missing finalize_circuit call

bdf5989

add init function for KimchiVesta struct

6bb1255

move kimchi field type to backend

f5c475d

remove unused imports

1a99e83

katat requested a review from mimoo April 16, 2024 09:42

katat mentioned this pull request Apr 16, 2024

Refactor using backend trait #35

Closed

mimoo approved these changes Apr 16, 2024

View reviewed changes

mimoo reviewed Apr 16, 2024

View reviewed changes

katat added 4 commits April 17, 2024 10:03

fmt

06290e5

remove debug_info backend interface

881a78c

doc: GeneratedWitness

9c98efe

remove dead code

3a07d2a

katat merged commit 754e029 into zksecurity:main Apr 17, 2024
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor with a backend trait #36

Refactor with a backend trait #36

katat commented Apr 16, 2024

mimoo left a comment

mimoo Apr 16, 2024

mimoo Apr 16, 2024

mimoo Apr 16, 2024

mimoo Apr 16, 2024

mimoo Apr 16, 2024

mimoo Apr 16, 2024

katat Apr 17, 2024

mimoo Apr 16, 2024

katat Apr 17, 2024

mimoo Apr 16, 2024

mimoo Apr 16, 2024 •

edited

Loading

mimoo Apr 16, 2024

mimoo Apr 16, 2024

mimoo Apr 16, 2024 •

edited

Loading

mimoo Apr 16, 2024 •

edited

Loading

mimoo Apr 16, 2024

mimoo Apr 16, 2024 •

edited

Loading

mimoo Apr 16, 2024

Refactor with a backend trait #36

Refactor with a backend trait #36

Conversation

katat commented Apr 16, 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

mimoo Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

mimoo Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimoo Apr 16, 2024 •

edited

Loading

mimoo Apr 16, 2024 •

edited

Loading

mimoo Apr 16, 2024 •

edited

Loading

mimoo Apr 16, 2024 •

edited

Loading