feat!: Log Directive support for traces #137
Conversation
There was a problem hiding this comment.
I think ACIR generation should produce only one log directive, so that we do not try to solve multiple times the same witness.
Then this solve directive would have a vector of 'field or witness', depending on whether the requested log was solved during acir generation.
PWG can completely ignore this directive, because the caller knowing the solved witness and the directive can figure out on its own.
This keeps ACVM simpler.
| /// evaluation, we can form an output string during ACIR generation. | ||
| /// Otherwise, we must store witnesses whose values will | ||
| /// be fetched during the PWG stage. | ||
| pub enum LogOutputInfo { |
There was a problem hiding this comment.
I don't understand why it is one or the other, why can't you have some of the witness solved during ACIR generation and some of them not solved?
There was a problem hiding this comment.
There is certain situations where ACIR generation does not solve the witness. For example, when implementing the log directive for the first time it was discovered that attempting to print the output of blackbox functions required waiting until PWG to fetch the witness value.
|
|
||
| pub enum SolvedLogOutputInfo { | ||
| FinalizedOutput(String), | ||
| WitnessValues(Vec<FieldElement>), |
There was a problem hiding this comment.
If the log is solved, why do you keep the values and not just have a string?
There was a problem hiding this comment.
I didn't want to leave any decoding of the field elements in the ACVM. Transforming Vec<FieldElement> into a string right away would also require the extra logic that currently exists in the ACVM where we check whether we have a singular FieldElement or multiple (as the string looks different based on whether we have a singular field element or an array).
acvm/src/lib.rs
Outdated
| @@ -74,7 +79,11 @@ pub trait PartialWitnessGenerator { | |||
| Self::solve_black_box_function_call(initial_witness, bb_func) | |||
| } | |||
| Opcode::Directive(directive) => { | |||
| Self::solve_directives(initial_witness, directive) | |||
| Self::solve_directives(initial_witness, directive).map(|possible_log| { | |||
There was a problem hiding this comment.
I don't think the solve should take care of the logs, this should be handled internally by the log directive.
There was a problem hiding this comment.
I can move this logic to be internal to solve_directives
I see what you mean about how the caller knowing the solved witness and the directive can figure out what to call on its own. This is something I hadn't considered and will look at making this change here and the upstream Noir PR. I have a couple clarifying questions: When you refer to solve directive do you mean If we were to only have a vector of fields and witnesses, the log directive would then require some extra information such as whether the vector of fields/witnesses is a string or a normal array of fields. You can see how I currently handle this in the Noir PR when evaluating array logs. I foresee if I made this change that One more thing to consider is how the caller will only handle logs that have been solved before any resolution error in the PWG. We don't want to handle a log that is known at compile time but was not reached by the program execution. It looks like the caller should still be able handle this though so I do think your requested changes are the best way forward. |
|
The VM has diverged a lot since this was opened, and these changes are small. Closing in favor of a fresh PR. |
Related issue(s)
Resolves #95
Description
Directive::Logno longer prints the evaluated log while it is being evaluating during partial witness generation. This enables us to have both trace functionality and move logic for displaying logs to the code that calls the PWG rather than keeping it within the solver itself.Summary of changes
solve under the PWG now takes in a vector of
SolvedLogstructs. These solved logs contain either be a finalized string (calculated during compilation) or a vector of field elements that should be handled by the caller ofsolve.Several more changes showing how to handle logs correctly will be supplied in nargo (where we call
solve)Dependency additions / changes
(If applicable.)
Test additions / changes
As this reduces the functionality of the log directive, implementation of tests can be seen in the noir/nargo PR that implements these changes.
Checklist
cargo fmtwith default settings.Additional context
(If applicable.)