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

Make debugging / logging variables in a circuit less error-prone #489

Closed
mitschabaude opened this issue Oct 14, 2022 · 9 comments · Fixed by #998
Closed

Make debugging / logging variables in a circuit less error-prone #489

mitschabaude opened this issue Oct 14, 2022 · 9 comments · Fixed by #998
Labels
error-messages Issues about making error messages better

Comments

@mitschabaude
Copy link
Contributor

mitschabaude commented Oct 14, 2022

feedback by @gretzke

- vielleicht sollte man toString/toJSON ganz entfernen (oder zumindest dem Entwickler nicht zugängig machen) damit man gar nicht in Versuchung kommt etwas zu loggen
- Eine dedicated log funktion anbieten (e.g., snarky.log()) die 
Circuit.asProver(() => {
  console.log(field.toString())
});

Paraphrasing:

  • remove functions like field.toString() / .toJSON() because they aren't allowed inside circuits
  • instead, expose a simple debug / log function which abstracts this away
    • example: field.debug(x => console.log(x)), where x is a string
    • or maybe: debug(log => { log(field); log(bool) }) which logs variables in a nice format
@gretzke
Copy link

gretzke commented Oct 14, 2022

I think such a wrapper would also work well:

function log(...args: any[]) {
  Circuit.asProver(() => {
    console.log(...args.map((x) => x.toString()));
  });
}

calling it like this:

snarky.log('field', Field.zero, 'bool', Bool(true), 'uint', new UInt64(2));

should result in

> field 0 bool true uint 2

It would allow developers to log values intuitively with the same syntax they are used from console.log without breaking the compilation. toString() methods should be protected so that the functions cannot be called by the dev directly, because the first intuition of every dev would be logging like this console.log(myField.toString());

Another option would be exposing a dedicated log function on the Field, and CircuitValue classes that also take an optional string for logging, e.g.,:

const myField = Field(5);
myField.log("the value of my field is:");
> the value of my field is: 5

@mitschabaude
Copy link
Contributor Author

I like the simplicity of your snarky.log, and similarity with console.log :D

I wonder whether the innermost function should not be x => x.toString(), but x => JSON.stringify(x) or x => JSON.parse(JSON.stringify(x)), because that will work on all circuit values, and print something nice for plain JS arrays / objects / primitive types as well

@mitschabaude mitschabaude self-assigned this Oct 14, 2022
@gretzke
Copy link

gretzke commented Oct 14, 2022

The json representation makes sense, I know people also like logging values like this:

console.log({a,b})
> { a: 1, b: 2 }

I feel like this would work great with the json parsing

@mitschabaude
Copy link
Contributor Author

I'm adding Circuit.log now. Leaving this open for discussion on whether to remove toString etc

@bkase
Copy link
Member

bkase commented Oct 24, 2022

unsafeToStringOutsideCircuit()

@bkase
Copy link
Member

bkase commented Oct 24, 2022

Todo: Propose something with reasoning

@gretzke
Copy link

gretzke commented Oct 24, 2022

Is there any other reason to convert a provable value to a string except for logging?

@mitschabaude
Copy link
Contributor Author

@gretzke yes, serializing for IO, & in general process it as a normal JS value outside the circuit. It's used all the time, it's just not supposed to be used in circuits

@mitschabaude
Copy link
Contributor Author

mitschabaude commented Oct 24, 2022

@bkase I propose to not change this for now, but instead change the error message to something that explains the matter very well, and to revisit this when we work on separating constants and variables.

The reasoning is that the constant-variable separation could mean we can keep a nice API, so it feels premature to introduce that ugly / dangerous-sounding API in the meantime

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error-messages Issues about making error messages better
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants