Consensys / corset

4 stars 10 forks source link

Compute Index Out-Of-Bounds Error #106

Closed DavePearce closed 1 month ago

DavePearce commented 1 month ago

I'm not sure whether this is a bug, or I have made a mistake formulating the input. But, the following command fails:

> corset compute --trace test.trace --out test.out test.lisp
thread 'main' panicked at src/compiler/generator.rs:997:67:
byte index 2 is out of bounds of `0`
...

Where we have:

test.lisp

(module test)
(defcolumns (A :binary@prove) (B))
(defconstraint test () (vanishes! (* A B)))

test.trace

{"test": { "A": [0,1], "B": [0,0] } }

I should add that this passes corset check --trace test.trace test.lisp

DavePearce commented 1 month ago

@letypequividelespoubelles Did I formulate the trace file correctly?

DavePearce commented 1 month ago

So, the offending chunk of code is this (from src/compiler/generator.rs:

 while let Some(x) = value.next() {
          out.write_all(
               cache
                   .cache_get_or_set_with(x.to_owned(), || {
                         format!("\"0x0{}\"", x.to_string()[2..].trim_start_matches('0')) // <--- HERE
           })

Specifically, x.to_string()[2..] appears to be assuming at least two characters in the original string (hex anyone)?

letypequividelespoubelles commented 1 month ago

(defcolumns (A :binary@prove) B)

Is it working like this ?

DavePearce commented 1 month ago

Is it working like this ?

Well, it doesn't matter whether we have :binary@prove or not --- it crashes eitherway. To be honest, its just a bug in that piece of code. But, I'm confused why that hasn't been caught already. Is it because we are using corset convert now instead of corset compute ?

delehef commented 1 month ago

Yes, we never use compute anymore, and very seldom convert (maybe a couple times for advanced debugging).

DavePearce commented 1 month ago

Thanks!! So what is used now instead of compute / convert?

delehef commented 1 month ago

What's the use case?

DavePearce commented 1 month ago

To do the trace expansion? Or is that not supported from the command line anymore? I'm just playing around with it getting a feel for how it works.

delehef commented 1 month ago

To do the trace expansion?

If we want to do that from the CLI rather than through the FFI/lib, then the better course of actions would probably be to leverage code from lib.rs to backport it into corset compute

DavePearce commented 1 month ago

Ok, thanks --- I think that has cleared up what's going on here. Ultimately, it seems the FFI is the only reliable way at this time to do trace expansion. I can fix this if I need to at some point.

DavePearce commented 1 month ago

Yup, so removing that slice on the to_string() allows corset compute to work again --- thanks. As far as I can tell actually its doing exactly the same thing as the FFI (aside from printing out the result to a file).