Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geb generates incorrect function for the identity lambda-abstraction #93

Closed
lukaszcz opened this issue Apr 18, 2023 · 31 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@lukaszcz
Copy link

I compile the following

(defpackage #:gg
  (:shadowing-import-from :geb.lambda.spec #:func #:pair)
  (:shadowing-import-from :geb.spec #:case)
  (:use #:common-lisp #:geb.lambda.spec #:geb))

(in-package :gg)

(defparameter *entry*
  (typed
    (lamb
      (coprod
        so1
        so1)
      (coprod
        so1
        so1)
      (index 0))
    (!->
      (coprod
        so1
        so1)
      (coprod
        so1
        so1))))

with

geb.image -i gg.lisp -e "gg::*entry*" -l -v -o gg.pir

The output is:

(...)
def entry x = {
  2 * (((1 * ((mod32 (2 * ((pwless32 (1 * (x + 0)) 1 ((1 * (x + 0)) / 1) (((1 * (x + 0)) - 1) / 1)) + (pwless32 (1 * (x + 0)) 1 (mod32 (1 * (x + 0)) 1) (1 + (mod32 ((1 * (x + 0)) - 1) 1))))) 2) + ((2 * ((pwless32 (1 * (x + 0)) 1 ((1 * (x + 0)) / 1) (((1 * (x + 0)) - 1) / 1)) + (pwless32 (1 * (x + 0)) 1 (mod32 (1 * (x + 0)) 1) (1 + (mod32 ((1 * (x + 0)) - 1) 1))))) / 1))) / 2) + ((1 * ((mod32 (2 * ((pwless32 (1 + (1 * (x + 0))) 1 ((1 + (1 * (x + 0))) / 1) (((1 + (1 * (x + 0))) - 1) / 1)) + (pwless32 (1 + (1 * (x + 0))) 1 (mod32 (1 + (1 * (x + 0))) 1) (1 + (mod32 ((1 + (1 * (x + 0))) - 1) 1))))) 2) + ((2 * ((pwless32 (1 + (1 * (x + 0))) 1 ((1 + (1 * (x + 0))) / 1) (((1 + (1 * (x + 0))) - 1) / 1)) + (pwless32 (1 + (1 * (x + 0))) 1 (mod32 (1 + (1 * (x + 0))) 1) (1 + (mod32 ((1 + (1 * (x + 0))) - 1) 1))))) / 1))) / 2))
};

At the end I add a constraint to test this:

entry x = x;

I run:

% vamp-ir plonk setup -m 14 -o params.pp
% vamp-ir plonk compile -u params.pp -s gg.pir -o circuit.plonk
% vamp-ir plonk prove -u params.pp -c circuit.plonk -o proof.plonk
> [input 0 for x here]
% vamp-ir plonk verify -u params.pp -c circuit.plonk -p proof.plonk

I get:

* Result from verifier: Err(ProofVerificationError)

By the way, the encoding of the identity function needs 2^14 parameters which seems a bit too much.

@lukaszcz lukaszcz added the bug Something isn't working label Apr 18, 2023
@agureev
Copy link
Collaborator

agureev commented Apr 19, 2023

For reference, here is the current syntax taking the above term to Vamp-IR under the name "name":

(to-circuit (list (coprod so1 so1)) (lamb (coprod so1 so1) (index 0)) :name)
given that you are in the translation package.

The current output is

def name x = { 2 * (((2 * ((mod32 (2 * ((pwless32 (1 * (x + 0)) 2 ((1 * (x + 0)) / 2) (((1 * (x + 0)) - 2) / 2)) + (pwless32 (1 * (x + 0)) 2 (mod32 (1 * (x + 0)) 1) (1 + (mod32 ((1 * (x + 0)) - 2) 1))))) 2) + ((2 * ((pwless32 (1 * (x + 0)) 2 ((1 * (x + 0)) / 2) (((1 * (x + 0)) - 2) / 2)) + (pwless32 (1 * (x + 0)) 2 (mod32 (1 * (x + 0)) 1) (1 + (mod32 ((1 * (x + 0)) - 2) 1))))) / 2))) / 2) + ((2 * ((mod32 (2 * ((pwless32 (2 + (1 * (x + 0))) 2 ((2 + (1 * (x + 0))) / 2) (((2 + (1 * (x + 0))) - 2) / 2)) + (pwless32 (2 + (1 * (x + 0))) 2 (mod32 (2 + (1 * (x + 0))) 1) (1 + (mod32 ((2 + (1 * (x + 0))) - 2) 1))))) 2) + ((2 * ((pwless32 (2 + (1 * (x + 0))) 2 ((2 + (1 * (x + 0))) / 2) (((2 + (1 * (x + 0))) - 2) / 2)) + (pwless32 (2 + (1 * (x + 0))) 2 (mod32 (2 + (1 * (x + 0))) 1) (1 + (mod32 ((2 + (1 * (x + 0))) - 2) 1))))) / 2))) / 2)) };

@rokopt
Copy link
Member

rokopt commented Apr 21, 2023

Although the current output differs from the original, I still get a proof verification error from it during the verify phase. So far I'm not sure how to determine what the computation did actually return -- I tried both the constraint = 0 and the constraint = 1 and verification failed for both. For a function bool -> bool, we should definitely not be generating any code that returns anything other than 0 or 1!

@mariari
Copy link
Member

mariari commented Apr 21, 2023

Although the current output differs from the original, I still get a proof verification error from it during the verify phase. So far I'm not sure how to determine what the computation did actually return -- I tried both the constraint = 0 and the constraint = 1 and verification failed for both. For a function bool -> bool, we should definitely not be generating any code that returns anything other than 0 or 1!

You'll need to send in -m n where n is greater than 14

@lukaszcz
Copy link
Author

Although the current output differs from the original, I still get a proof verification error from it during the verify phase. So far I'm not sure how to determine what the computation did actually return -- I tried both the constraint = 0 and the constraint = 1 and verification failed for both. For a function bool -> bool, we should definitely not be generating any code that returns anything other than 0 or 1!

You'll need to send in -m n where n is greater than 14

You mean give -m n to what? vamp-ir plonk setup? Because geb.image doesn't support this option. With 14 PLONK parameters the circuit compiles, but you get verification errors, also with the new output.

@lukaszcz
Copy link
Author

It doesn't work with vamp-ir plonk setup -m 15 either. Still getting a verification error. It takes ages (or rather minutes) to compile, prove and verify with 15 parameters. I don't have enough patience to try 16.

@lopeetall
Copy link

You can speed up a setup using the --unchecked flag, but you must remember to continue to use --unchecked on the compile, prove, and verify commands.

Or better yet, skip the setup altogether and use halo2 :)

@lopeetall
Copy link

Also, if the setup size is too small you will get an error about the trimming degree rather than proof verification. A proof verification error should only mean the circuit is not satisfied.

@rokopt
Copy link
Member

rokopt commented Apr 24, 2023

This problem still exists, but the fix in #99 does change the output circuit, so for reference, here's the new one:

  (2 * (((2 * (mod32 ((2 * (pwless32 ((1 * x) + 0) 2 (((1 * x) + 0) / 2) ((((1 * x) + 0) - 2) / 2))) + (pwless32 ((1 * x) + 0) 2 (mod32 ((1 * x) + 0) 1) (1 + (mod32 (((1 * x) + 0) - 2) 1)))) 2)) + (((2 * (pwless32 ((1 * x) + 0) 2 (((1 * x) + 0) / 2) ((((1 * x) + 0) - 2) / 2))) + (pwless32 ((1 * x) + 0) 2 (mod32 ((1 * x) + 0) 1) (1 + (mod32 (((1 * x) + 0) - 2) 1)))) / 2)) / 2)) + (((2 * (mod32 ((2 * (pwless32 (2 + ((1 * x) + 0)) 2 ((2 + ((1 * x) + 0)) / 2) (((2 + ((1 * x) + 0)) - 2) / 2))) + (pwless32 (2 + ((1 * x) + 0)) 2 (mod32 (2 + ((1 * x) + 0)) 1) (1 + (mod32 ((2 + ((1 * x) + 0)) - 2) 1)))) 2)) + (((2 * (pwless32 (2 + ((1 * x) + 0)) 2 ((2 + ((1 * x) + 0)) / 2) (((2 + ((1 * x) + 0)) - 2) / 2))) + (pwless32 (2 + ((1 * x) + 0)) 2 (mod32 (2 + ((1 * x) + 0)) 1) (1 + (mod32 ((2 + ((1 * x) + 0)) - 2) 1)))) / 2)) / 2)
};

I've tried several constraints and none of them has worked, so I don't know what this circuit is actually outputting yet. @lopeetall is working on debug features that should make this easy to determine, and @AHartNtkn is doing some refactoring for using multivariate polynomials which I suspect might also make it straightforward (particularly in conjunction with my forthcoming cat-spec API) for us to write an interpreter for the subset of VampIR that we use in Geb itself, which would not only help with debugging issues such as this, but would also help with formally verifying the whole pipeline throughout Geb.

@rokopt
Copy link
Member

rokopt commented Apr 24, 2023

Here's the new to-poly output:

(to-poly (list (coprod so1 so1)) (lamb (coprod so1 so1) (index 0)))
(GEB.POLY.SPEC:+ (GEB.POLY.SPEC:* 2 (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:/ #<IDENT> 2) (GEB.POLY.SPEC:+ (GEB.POLY.SPEC:* 2 (GEB.POLY.SPEC:MOD #<IDENT> 2)) (GEB.POLY.SPEC:/ #<IDENT> 2))) (GEB.POLY.SPEC:+ (GEB.POLY.SPEC:* 2 (GEB.POLY.SPEC:IF-LT #<IDENT> 2 (GEB.POLY.SPEC:/ #<IDENT> 2) (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:/ #<IDENT> 2) (GEB.POLY.SPEC:- #<IDENT> 2)))) (GEB.POLY.SPEC:IF-LT #<IDENT> 2 (GEB.POLY.SPEC:COMPOSE #<IDENT> (GEB.POLY.SPEC:MOD #<IDENT> 1)) (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:+ 1 #<IDENT>) (GEB.POLY.SPEC:MOD #<IDENT> 1)) (GEB.POLY.SPEC:- #<IDENT> 2))))) #<IDENT>) (GEB.POLY.SPEC:+ (GEB.POLY.SPEC:* 1 #<IDENT>) 0))) (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:/ #<IDENT> 2) (GEB.POLY.SPEC:+ (GEB.POLY.SPEC:* 2 (GEB.POLY.SPEC:MOD #<IDENT> 2)) (GEB.POLY.SPEC:/ #<IDENT> 2))) (GEB.POLY.SPEC:+ (GEB.POLY.SPEC:* 2 (GEB.POLY.SPEC:IF-LT #<IDENT> 2 (GEB.POLY.SPEC:/ #<IDENT> 2) (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:/ #<IDENT> 2) (GEB.POLY.SPEC:- #<IDENT> 2)))) (GEB.POLY.SPEC:IF-LT #<IDENT> 2 (GEB.POLY.SPEC:COMPOSE #<IDENT> (GEB.POLY.SPEC:MOD #<IDENT> 1)) (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:COMPOSE (GEB.POLY.SPEC:+ 1 #<IDENT>) (GEB.POLY.SPEC:MOD #<IDENT> 1)) (GEB.POLY.SPEC:- #<IDENT> 2))))) (GEB.POLY.SPEC:+ 2 #<IDENT>)) (GEB.POLY.SPEC:+ (GEB.POLY.SPEC:* 1 #<IDENT>) 0)))

@mariari
Copy link
Member

mariari commented Apr 24, 2023

I can probably write the interpreter for the parts of vampir we use, it'd be woefully incomplete, but I could just quickly draft up an interpreter for the vampir we use like #98 does for the other categories we have

@rokopt
Copy link
Member

rokopt commented Apr 24, 2023

I can probably write the interpreter for the parts of vampir we use, it'd be woefully incomplete, but I could just quickly draft up an interpreter for the vampir we use like #98 does for the other categories we have

That'd certainly be great if it's not too much work! We could port it (along with everything else) to the new, modular cat-spec API once I have that in (and itself ported).

I think this bug must be in to-poly, though, unless I'm misunderstanding how this works. I tried merging mariari/interpreter along with the fix for #99 and did some gapplys and got the following:

(gapply (to-poly (list (coprod so1 so1)) (lamb (coprod so1 so1) (index 0))) 0)
5/4
(gapply (to-poly (list (coprod so1 so1)) (lamb (coprod so1 so1) (index 0))) 1)
3
(gapply (to-poly (list (coprod so1 so1)) (lamb (coprod so1 so1) (index 0))) 2)
17/4
*

I guess the divisions aren't reducing yet? Even if they were, though, these wouldn't be the right answers. So I think that this particular bug is happening before we get to the translation to VampIR. (Hence we don't need any VampIR-level debug features to address this specific issue.)

@mariari
Copy link
Member

mariari commented Apr 24, 2023

@rokopt would make sense given the lambda refactor has changed meaning, @rokopt can you try code before @agureev's changes. either way you can test 1 layer at a time, by compiling any of our geb terms to poly, and using gapply there

@rokopt
Copy link
Member

rokopt commented Apr 24, 2023

I tried to figure out how to get gapply to run on the substmorph term compiled from the STLC (using the merge of main with mariari/interpreter), but I haven't figured out how to get a result I understand. I tried this:

(gapply (compile-checked-term (list (coprod so1 so1)) (lamb (coprod so1 so1) (index 0))) (->left so1 so1))
((left s-1) (right s-1))

I was trying to apply the checked term (which ought to be equivalent to the identity) to false, so I expected something with the signature so1 -> bool, but that output doesn't look like that to me (is it even well-formed?). I may have misunderstood the current syntax, or gapply, or there might be a conflict between how the interpreter based off main works and the lambda refactoring. I'll try the stock interpreter branch and see if I can figure out the right thing to apply with the old syntax.

@mariari
Copy link
Member

mariari commented Apr 24, 2023

(gapply (compile-checked-term (list (coprod so1 so1)) (lamb (coprod so1 so1) (index 0))) (->left so1 so1))

what is the (->left so1 so1) here, are you trying to inject left so1?

If so, then actually inject-left

(left so1)

is all you should need.

We use realized-objects here. See this example

GEB> (gapply (pair (<-left so1 geb-bool:bool)
                   (<-right so1 geb-bool:bool))
             (list so1 (left geb-bool:true-obj)))
(s-1 (left GEB-BOOL:TRUE))

@agureev
Copy link
Collaborator

agureev commented Apr 24, 2023

Just checked: previous STLC logic also does not work. Working in https://github.com/anoma/geb/tree/mariari/interpreter and changing the pair case for to-poly as in #99 one gets the output:

TRANS> (gapply (to-poly nil (!-> (coprod so1 so1) (coprod so1 so1)) (lamb (coprod so1 so1) (coprod so1 so1) (index 0))) 0) 1

and

TRANS> (gapply (to-poly nil (!-> (coprod so1 so1) (coprod so1 so1)) (lamb (coprod so1 so1) (coprod so1 so1) (index 0))) 1) 4

@rokopt
Copy link
Member

rokopt commented Apr 24, 2023

I just realized something -- this is generating a function that returns a lambda abstraction. It's not returning a function that applies the lambda abstraction. What it's returning is the quoted value of the identity function.

@rokopt
Copy link
Member

rokopt commented Apr 24, 2023

I'm not sure what typed does -- or, rather, did; I think that's gone. For (perhaps only my) reference, here's some Idris which applies a lambda abstraction to a term from the context:

idb : SubstMorph (SubstBool !* Subst1) SubstBool
idb = compileCheckedTerm {ctx=([SubstBool])} {ty=SubstBool} $
  Checked_STLC_App {ctx=[SubstBool]} {dom=SubstBool} {cod=SubstBool}
    (Checked_STLC_Lambda {ctx=[SubstBool]} {vty=SubstBool} {tty=SubstBool}
      (Checked_STLC_Var {i=Z}))
    (Checked_STLC_Var {i=Z})

idbFunc : Integer -> Integer
idbFunc = substMorphToFunc idb

polyIdb0 : Assertion
polyIdb0 = Assert $ idbFunc 0 == 0

polyIdb1 : Assertion
polyIdb1 = Assert $ idbFunc 1 == 1

This does what I'd expect.

The way the Idris is written is that the STLC context is where the inputs to the circuit come from; if the STLC context is empty then you get a constant value. The constant value of a lambda abstraction, in particular, is just the quote of the function. This allows you to manipulate quoted functions -- it doesn't assume that a return value that happens to be a function type means that you want to evaluate the function. If you want to evaluate it, you can explicitly apply it to a parameter from the context, as in the above Idris.

I'm not sure what we're doing in the Lisp yet with the latest STLC code.

@rokopt
Copy link
Member

rokopt commented Apr 24, 2023

Assigning to @agureev as he knows what to do. :)

@agureev
Copy link
Collaborator

agureev commented Apr 27, 2023

Update: apologies for a very silly error. The new API does not change the value of the function we feed it in. I simply supplied a context when it did not need to be supplied. In particular, the appropriate code to reproducing Lukasz's original entry/output is.

to-circuit nil (lamb (coprod so1 so1) (index 0)) :name)

Note the empty context here. Adding in a boolean to the context is what caused the incremental change in the Vamp-IR code.

Moreover, using the uncurry API as well as @mariari interpreter one can check that the STLC -> Geb compilation actually works well, i.e. pointwise produces an identity function.

TRANS> (geb.generics:gapply (uncurry (coprod so1 so1) (coprod so1 so1) (geb.lambda.trans:compile-checked-term nil (geb.lambda:lamb (coprod so1 so1) (geb.lambda:index 0)) )) (list so1 (geb:left so1)))
(left s-1)
TRANS> (geb.generics:gapply (uncurry (coprod so1 so1) (coprod so1 so1) (geb.lambda.trans:compile-checked-term nil (geb.lambda:lamb (coprod so1 so1) (geb.lambda:index 0)) )) (list so1 (geb:right so1)))
(right s-1)

In other words, the mistake seems to linger in the polynomial part of the compilation.

@rokopt
Copy link
Member

rokopt commented Apr 27, 2023

If I understand the above comment correctly, then #107 will fix this specific bug because the stlc -> subst part of the translation will do the right thing, and the subst -> poly compilation happens to work with the subst code that then results from this case, but there will remain some bug in subst -> poly which affects the code that is currently (incorrectly) being generated in this case. Is that right?

@agureev
Copy link
Collaborator

agureev commented Apr 27, 2023

No, even in this case after uncurrying, the to-poly function seems to be generating an incorrect result.

@rokopt
Copy link
Member

rokopt commented Apr 27, 2023

No, even in this case after uncurrying, the to-poly function seems to be generating an incorrect result.

Okay, that makes sense. In that case, would you please file a separate issue for the to-poly bug, to document the code for which it produces the incorrect result, and what the correct result should be, and to track its investigation and fixing? That way we'll still have that for a reference once #107 changes the code that will be generated by this specific STLC code, and we'll be able to close this issue when this specific case works as a way of signaling that.

@mariari
Copy link
Member

mariari commented Apr 28, 2023

With #108 the code seems to work

GEB-TEST> (bitc:to-circuit (geb:to-bitc (geb.lambda:compile-checked-term
                                         (list (coprod so1 so1))
                                         (coprod so1 so1)
                                         (geb.lambda:lamb (coprod so1 so1)
                                                          (coprod so1 so1)
                                                          (geb.lambda:index 1))))
                           :foo)
def foo x1 = {
  ((1 - 0) * x1) + (0 * x1);
  ((1 - 1) * x1) + (1 * x1)
};

if handed 1 it should return 1, if handed 0, it'd return 0

@lukaszcz
Copy link
Author

Are de Bruijn indices in the GEB STLC front-end 0-based or 1-based? I thought they're 0-based and that's what we generate.

@mariari
Copy link
Member

mariari commented Apr 28, 2023

Are de Bruijn indices in the GEB STLC front-end 0-based or 1-based? I thought they're 0-based and that's what we generate.

they are 0 based, however, due to the way stlc->geb was originally designed it was odd semantically. @agureev 's work it should work as expected

@lukaszcz
Copy link
Author

I'm asking because you have index 1 in your last example, instead of index 0. So it seems to point outside of the term, not at the lambda.

@mariari
Copy link
Member

mariari commented Apr 28, 2023

I'm asking because you have index 1 in your last example, instead of index 0. So it seems to point outside of the term, not at the lambda.

it points to the top context, which is non ideal. Once the code changed it will be properly index 0 with a null starting context

@rokopt
Copy link
Member

rokopt commented May 3, 2023

This should be fixed by a combination of #108 , #115 , and the fix for #111 once we have one.

@rokopt
Copy link
Member

rokopt commented May 3, 2023

This should be fixed by a combination of #108 , #115 , and the fix for #111 once we have one.

Actually, I should note that #111 will no longer be relevant with the to-bitc compilation having replaced the to-poly compilation, so #108 and #115 should suffice to fix this.

@rokopt
Copy link
Member

rokopt commented May 9, 2023

Can this be closed as fixed by #108 and #115 ?

@rokopt
Copy link
Member

rokopt commented May 10, 2023

Can this be closed as fixed by #108 and #115 ?

Yes, this works now and is tested by the automated tests. (The indexing did change.)

@rokopt rokopt closed this as completed May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants