Support hint function #220

katat · 2024-11-04T08:23:22Z

This is a POC to support native hint function based on circ's IR and API.

The integration is quite straightforward. The major changes are listed below. Most of the code in circuit_writer/ir.rs remain the same as the circuit synthesizer:

circ IR can be composed using its term API, which can be used independently. (doesn't have to couple with other parts in circ)
creates a IR translator based on the template of synthesizer, to translate the mast to IR if it is a hint function.
one major change is to replace the ConstOrCell in the Var with the circ IR term, as a term can represent either constant field or a variable.
eg:

pub struct Var {
    pub cvars: Vec<Term>,
    pub span: Span,
}

So we may want to consider refactor the var::Var to cover both ConstOrCell and Term, in order to avoid the code duplication in the ir.rs.

another major change is to translate the binary operations or conditional branch.
eg:

Op2::Division => {
    let t: Term = term![
        Op::PfNaryOp(PfNaryOp::Mul); lhs.cvars[0].clone(), 
        term![Op::PfUnOp(PfUnOp::Recip); rhs.cvars[0].clone()]
    ];
    Var::new_cvar(t, expr.span)
}

atm, everything is defined as field type at low level using the circ IR
create a Value::HintIR to represent an IR generated from a hint function, and compute it via compute_val using circ's PreComp API.

TODOs:

support accepting other noname argument types and return types for hint functions.

StefanosChaliasos

The PR lgtm, I have a few questions wrt the type transformations.

src/backends/mod.rs

src/circuit_writer/ir.rs

StefanosChaliasos · 2024-11-06T08:04:15Z

src/circuit_writer/ir.rs

+        }
+    }
+
+    pub fn new_constant_typ<F: BackendField>(cst_info: &ConstInfo<F>, span: Span) -> Self {


it is the coding style :D

src/circuit_writer/mod.rs

src/parser/types.rs

src/backends/mod.rs

src/circuit_writer/ir.rs

mimoo · 2024-11-12T11:27:01Z

src/circuit_writer/ir.rs

+                                ErrorKind::InvalidFnCall("only hint functions allowed"),
+                                expr.span,
+                            ));
+                        }


btw what is the rationale here?

Ok, solves my previous comment :D. I guess here we check that a hint can call only another hint which makes sense

katat · 2024-11-12T13:11:44Z

The hint functions shouldn’t be allowed to call constrained functions, right? Otherwise it will end up some constraints created as an orphan circuit.

…

On Tue, Nov 12, 2024 at 6:27 PM David Wong ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/circuit_writer/ir.rs <#220 (comment)>: > @@ -660,6 +660,14 @@ impl<B: Backend> IRWriter<B> { FnKind::Native(func) => { // module::fn_name(args) // ^^^^^^ + // only allow calling hint functions + if !func.is_hint { + return Err(self.error( + ErrorKind::InvalidFnCall("only hint functions allowed"), + expr.span, + )); + } btw what is the rationale here? — Reply to this email directly, view it on GitHub <#220 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMDRGFA5FYBQJ7IHHRNZB32AHQZXAVCNFSM6AAAAABRDW6KYSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRZGMZDMMBVGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

mimoo · 2024-11-12T16:46:11Z

you could think that constraint functions wouldn't produce constraints if called by hints, but yeah I don't think it's a big deal :o was just wondering

mimoo · 2024-11-12T16:47:38Z

LGTM but @StefanosChaliasos I'll let you approve

mimoo · 2024-11-13T03:19:33Z

src/circuit_writer/ir.rs

@@ -0,0 +1,1119 @@
+use ark_ff::Zero;


btw as most of this file will be useful in the future to have an IR pass, I think it should be moved to an circ_ir folder or something, and moved out of the circuit_writer

mimoo

approving for the sake of getting this moved, let's add a disclaimer that we're still experimental in the README

StefanosChaliasos · 2024-11-13T03:47:56Z

src/backends/mod.rs

@@ -199,6 +202,48 @@ pub trait Backend: Clone {

                Ok(res)
            }
+            Value::HintIR(t, named_vars) => {


This lgtm, but I would suggest adding some unit tests here to check corner cases (0, p, p-1, p+1).

StefanosChaliasos · 2024-11-13T03:50:31Z

src/circuit_writer/ir.rs

@@ -0,0 +1,1119 @@
+use ark_ff::Zero;


StefanosChaliasos · 2024-11-13T03:51:27Z

src/circuit_writer/ir.rs

+        if idx < self.cvars.len() {
+            Some(&self.cvars[idx])
+        } else {
+            None


Do you handle that? Should throw an error here?

I think it would be cleaner instead of an option to return an error here

yeah, it is currently handled by callers.

Maybe we should add an issue to change that? Or do you think it's better to handle that in the callers? In any case even if you return a result it will be basically handled by callers but it would be more explicit and it will probably print a nicer error message?

StefanosChaliasos · 2024-11-13T03:51:39Z

src/circuit_writer/ir.rs

+                .as_value_opt()
+                .map(|v| BigUint::from(v.as_pf().i().to_u64_wrapping()))
+        } else {
+            None


Similarly here

StefanosChaliasos · 2024-11-13T03:57:10Z

src/circuit_writer/ir.rs

+                body,
+            } => {
+                match argument {
+                    ForLoopArgument::Range(range) => {


Is the range supposed to be known at compile time? No, right?

yes, it should be known at compiler time.

Why is that? I guess when we are in a hint, there isn't any reason why this should be known at compile time as we are out-of-circuit, right? Do I miss something?

oh yeah, this is a good point. I created an issue for that: #234

oh wait, I misunderstood your question.
so yeah, this is an interesting insight. It should allow variables as the range.

I guess lets keep this way for now, and will revisit this when someone raises the scenarios they need the range to be variables.

we don't do TDD here, we do user-complaint driven development

StefanosChaliasos · 2024-11-13T03:57:56Z

src/circuit_writer/ir.rs

+                            .expect("expected constant")
+                            .into();
+                        let start: u32 = start_bg.try_into().map_err(|_| {
+                            self.error(ErrorKind::InvalidRangeSize, range.start.span)


Why do we have this error?

atm, the size of the number is restricted to u32, so it throws error if it is bigger.

StefanosChaliasos · 2024-11-13T04:01:52Z

src/circuit_writer/ir.rs

+        function: &FunctionDef,
+        args: Vec<crate::circuit_writer::fn_env::VarInfo<B::Field, B::Var>>,
+    ) -> Result<Vec<crate::var::Value<B>>> {
+        assert!(!function.is_main());


Do we actually check that we are calling a hint function and not a non hint one?

Also, a hint should only be able to call hint function, right?

yes, only hint functions are allowed.

StefanosChaliasos · 2024-11-13T04:05:29Z

src/circuit_writer/ir.rs

+                                ErrorKind::InvalidFnCall("only hint functions allowed"),
+                                expr.span,
+                            ));
+                        }


Ok, solves my previous comment :D. I guess here we check that a hint can call only another hint which makes sense

katat added 7 commits November 4, 2024 15:30

add IR translator

af422ea

add Value::HintIR for hint calculations

7636d1a

enable parsing hint function body

34af420

add cargo deps

f004a33

circ require nightly rust toolchain

d64281d

add hint test

a38bba6

fmt

e30e563

katat requested review from StefanosChaliasos and mimoo November 4, 2024 11:22

katat added 3 commits November 5, 2024 16:35

support complex inputs and outputs

34162a7

Merge remote-tracking branch 'origin/main' into feat/ir2

a11e5c2

update asm

5d05284

katat force-pushed the feat/ir2 branch from bb2db8b to 5d05284 Compare November 5, 2024 08:40

StefanosChaliasos reviewed Nov 6, 2024

View reviewed changes

mimoo reviewed Nov 6, 2024

View reviewed changes

src/backends/mod.rs Outdated Show resolved Hide resolved

mimoo reviewed Nov 6, 2024

View reviewed changes

src/backends/mod.rs Outdated Show resolved Hide resolved

mimoo reviewed Nov 6, 2024

View reviewed changes

src/circuit_writer/ir.rs Outdated Show resolved Hide resolved

katat added 11 commits November 12, 2024 13:55

map backend fields to circ fields

427475b

better error for calling builtins in hint function

5b91bd1

refine error

190df51

more arithmetic operations

ed9dd4d

support boolean operations

99498fa

update tests

3dc4c71

update test

2cbc07e

only allow calling hint functions in a hint function

881ff1d

format

255b1e3

comment

f2c0e3a

Merge remote-tracking branch 'origin/main' into feat/ir2

3ab7525

katat changed the title ~~POC for supporting hint function~~ Support hint function Nov 12, 2024

mimoo reviewed Nov 12, 2024

View reviewed changes

mimoo requested a review from StefanosChaliasos November 12, 2024 16:46

mimoo reviewed Nov 13, 2024

View reviewed changes

mimoo approved these changes Nov 13, 2024

View reviewed changes

mimoo merged commit 8c0ca9d into main Nov 13, 2024
1 check passed

StefanosChaliasos reviewed Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support hint function #220

Support hint function #220

katat commented Nov 4, 2024 •

edited

Loading

StefanosChaliasos left a comment

StefanosChaliasos Nov 6, 2024

katat Nov 12, 2024

mimoo Nov 12, 2024

StefanosChaliasos Nov 13, 2024

katat commented Nov 12, 2024 via email

mimoo commented Nov 12, 2024

mimoo commented Nov 12, 2024

mimoo Nov 13, 2024

StefanosChaliasos Nov 13, 2024

mimoo left a comment

StefanosChaliasos Nov 13, 2024

StefanosChaliasos Nov 13, 2024

StefanosChaliasos Nov 13, 2024

StefanosChaliasos Nov 13, 2024

katat Nov 13, 2024

StefanosChaliasos Nov 13, 2024

StefanosChaliasos Nov 13, 2024

StefanosChaliasos Nov 13, 2024

katat Nov 13, 2024 •

edited

Loading

StefanosChaliasos Nov 13, 2024

katat Nov 13, 2024

katat Nov 13, 2024

katat Nov 13, 2024

mimoo Nov 14, 2024 •

edited

Loading

StefanosChaliasos Nov 13, 2024

katat Nov 13, 2024

StefanosChaliasos Nov 13, 2024

StefanosChaliasos Nov 13, 2024

katat Nov 13, 2024

StefanosChaliasos Nov 13, 2024

Support hint function #220

Support hint function #220

Conversation

katat commented Nov 4, 2024 • edited Loading

StefanosChaliasos 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

katat commented Nov 12, 2024 via email

mimoo commented Nov 12, 2024

mimoo commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

katat Nov 13, 2024 • 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

mimoo Nov 14, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katat commented Nov 4, 2024 •

edited

Loading

katat Nov 13, 2024 •

edited

Loading

mimoo Nov 14, 2024 •

edited

Loading