Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat!: Log Directive support for traces #105

Closed
wants to merge 5 commits into from
Closed

feat!: Log Directive support for traces #105

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
980e2cc
initial commit changing log directive to accept traces and move away …
vezenovm Feb 17, 2023
eddc64a
Merge branch 'master' into mv/trace
TomAFrench Feb 22, 2023
f7b98fa
chore: add comment on the assumption in printing witnesses
TomAFrench Feb 22, 2023
a7227ee
chore: remove dbg! call
TomAFrench Feb 22, 2023
def272e
Merge branch 'master' into mv/trace
TomAFrench Feb 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 22 additions & 15 deletions acir/src/circuit/directives.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ pub enum Directive {
bits: Vec<Witness>, // control bits of the network which permutes the inputs into its sorted version
sort_by: Vec<u32>, // specify primary index to sort by, then the secondary,... For instance, if tuple is 2 and sort_by is [1,0], then a=[(a0,b0),..] is sorted by bi and then ai.
},
Log(LogInfo),
Log {
is_trace: bool, // This field states whether the log should be further manipulated or simply displayed to standard output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have two separate process, and not just one which basically notifies when the 'log witness' are solved? Then the caller can decide to print to standard output or to further manipulate them. This logic should not be in ACIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed as without this flag the caller does not know whether to print to standard output or to further manipulate the logs. I was trying to have the same directive for the builtins std::println and std::trace. For example, if solve simply returns a finalized string to be logged, but both std::println and std::trace exist, the caller will not know whether to print to std out or manipulate the trace how it sees fit without more info. The flag would be set during the evaluator, and essentially passed along until solve is called. Only the caller of solve would implement the logic of how to handle a log according to the flag.

Copy link
Contributor

@guipublic guipublic Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not suggesting to remove the flag, sorry for the mis-understanding, the location of my comment is confusing. This is because my first reaction was to ask for a clarification on the flag meaning. But at the end, I think it is better to not handle it in the solve, and just solve the witness.
With the flag (from the directive), the caller should be able to handle it correctly, shouldn't he?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is the case. We only pass the flag along with the solved witness info so that the caller can determine how they should handle the solved log directive. The process in solve_directives for Directive::Log remains the same for both traces and printlns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flag is also used during the solve, that's why I thought we were implementing its logic.
The solve is handling the flag and also behaves differently when there is only one witness. Could you explain why we have these 2 special cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So currently we only support logs of individual fields, arrays, or strings (formatting and handling of complex types can come in a separate PR). In the case of one witness we don't have to construct a list of hex elements. We can simply format the respective witness value and return that the log has been solved. In the case of multiple witnesses this means we have an array, where we have to fetch a list of hex elements, and format them into one finalized string output. Both cases will form a LogOutputInfo::FinalizedOutput(String), which the caller of solve will then handle. In the case of a string this is formatted during acir gen and should already be a LogOutputInfo::FinalizedOutput. If Directive::Log already has finalized output we can just return the string stored in the enum.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation. For me you are processing the logs here and you should not. The Log directive should just hold the parameters (for now just the flag), and the witness to log.
When the gate is solved, i.e when all the witness are known, you can send the parameters and witness values to the caller. That's it. How/where to print, which language to use, etc... all this should be handled by the caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I see now. Ok I will refactor this

output_info: LogOutputInfo,
},
}

impl Directive {
Expand Down Expand Up @@ -143,17 +146,20 @@ impl Directive {
write_u32(&mut writer, *i)?;
}
}
Directive::Log(info) => match info {
LogInfo::FinalizedOutput(output_string) => {
write_bytes(&mut writer, output_string.as_bytes())?;
}
LogInfo::WitnessOutput(witnesses) => {
write_u32(&mut writer, witnesses.len() as u32)?;
for w in witnesses {
write_u32(&mut writer, w.witness_index())?;
Directive::Log { is_trace, output_info } => {
write_u32(&mut writer, *is_trace as u32)?;
match output_info {
LogOutputInfo::FinalizedOutput(output_string) => {
write_bytes(&mut writer, output_string.as_bytes())?;
}
LogOutputInfo::WitnessOutput(witnesses) => {
write_u32(&mut writer, witnesses.len() as u32)?;
for w in witnesses {
write_u32(&mut writer, w.witness_index())?;
}
}
}
},
}
};

Ok(())
Expand Down Expand Up @@ -242,11 +248,12 @@ impl Directive {
}

#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
// If values are compile time and/or known during
// 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 LogInfo {
/// This info is used when solving the initial witness
/// If values are compile time and/or known during
/// 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 {
FinalizedOutput(String),
WitnessOutput(Vec<Witness>),
}
Expand Down
25 changes: 15 additions & 10 deletions acir/src/circuit/opcodes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::io::{Read, Write};

use super::directives::{Directive, LogInfo};
use super::directives::{Directive, LogOutputInfo};
use crate::native_types::{Expression, Witness};
use crate::serialization::{read_n, read_u16, read_u32, write_bytes, write_u16, write_u32};
use crate::BlackBoxFunc;
Expand Down Expand Up @@ -166,15 +166,20 @@ impl std::fmt::Display for Opcode {
bits.last().unwrap().witness_index(),
)
}
Opcode::Directive(Directive::Log(info)) => match info {
LogInfo::FinalizedOutput(output_string) => write!(f, "Log: {output_string}"),
LogInfo::WitnessOutput(witnesses) => write!(
f,
"Log: _{}..._{}",
witnesses.first().unwrap().witness_index(),
witnesses.last().unwrap().witness_index()
),
},
Opcode::Directive(Directive::Log { is_trace, output_info }) => {
let is_trace_display = if *is_trace { "trace" } else { "println" };
match output_info {
LogOutputInfo::FinalizedOutput(output_string) => {
write!(f, "Log: {output_string}, log type {is_trace_display}")
jfecher marked this conversation as resolved.
Show resolved Hide resolved
}
LogOutputInfo::WitnessOutput(witnesses) => write!(
f,
"Log: _{}..._{}, log type: {is_trace_display}",
witnesses.first().unwrap().witness_index(),
witnesses.last().unwrap().witness_index(),
),
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
}
Expand Down
16 changes: 11 additions & 5 deletions acvm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub use acir::FieldElement;
// TODO: ExpressionHasTooManyUnknowns is specific for arithmetic expressions
// TODO: we could have a error enum for arithmetic failure cases in that module
// TODO that can be converted into an OpcodeNotSolvable or OpcodeResolutionError enum
#[derive(PartialEq, Eq, Debug, Error)]
#[derive(PartialEq, Eq, Debug, Error, Clone)]
pub enum OpcodeNotSolvable {
#[error("missing assignment for witness index {0}")]
MissingAssignment(u32),
Expand All @@ -35,7 +35,7 @@ pub enum OpcodeNotSolvable {
UnreachableCode,
}

#[derive(PartialEq, Eq, Debug, Error)]
#[derive(PartialEq, Eq, Debug, Error, Clone)]
pub enum OpcodeResolutionError {
#[error("cannot solve opcode: {0}")]
OpcodeNotSolvable(OpcodeNotSolvable),
Expand All @@ -59,6 +59,7 @@ pub trait PartialWitnessGenerator {
&self,
initial_witness: &mut BTreeMap<Witness, FieldElement>,
opcodes: Vec<Opcode>,
logs: &mut Vec<Directive>,
) -> Result<(), OpcodeResolutionError> {
if opcodes.is_empty() {
return Ok(());
Expand All @@ -71,7 +72,12 @@ pub trait PartialWitnessGenerator {
Opcode::BlackBoxFuncCall(bb_func) => {
Self::solve_black_box_function_call(initial_witness, bb_func)
}
Opcode::Directive(directive) => Self::solve_directives(initial_witness, directive),
Opcode::Directive(directive) => Self::solve_directives(initial_witness, directive)
.map(|possible_log| {
if let Some(solved_log) = possible_log {
logs.push(solved_log)
}
}),
};

match resolution {
Expand All @@ -87,7 +93,7 @@ pub trait PartialWitnessGenerator {
Err(err) => return Err(err),
}
}
self.solve(initial_witness, unsolved_opcodes)
self.solve(initial_witness, unsolved_opcodes, logs)
}

fn solve_black_box_function_call(
Expand All @@ -109,7 +115,7 @@ pub trait PartialWitnessGenerator {
fn solve_directives(
initial_witness: &mut BTreeMap<Witness, FieldElement>,
directive: &Directive,
) -> Result<(), OpcodeResolutionError> {
) -> Result<Option<Directive>, OpcodeResolutionError> {
pwg::directives::solve_directives(initial_witness, directive)
}
}
Expand Down
44 changes: 25 additions & 19 deletions acvm/src/pwg/directives.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{cmp::Ordering, collections::BTreeMap};

use acir::{
circuit::directives::{Directive, LogInfo},
circuit::directives::{Directive, LogOutputInfo},
native_types::Witness,
FieldElement,
};
Expand All @@ -15,13 +15,13 @@ use super::{get_value, insert_value, sorting::route, witness_to_value};
pub fn solve_directives(
initial_witness: &mut BTreeMap<Witness, FieldElement>,
directive: &Directive,
) -> Result<(), OpcodeResolutionError> {
) -> Result<Option<Directive>, OpcodeResolutionError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the solve should return an optional directive. The directive is either solved or not. If it is solved, then you can get the witness values from the witness map.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - If a directive is not solved, then it means we did not find a witness that it needed, this will return an Error

Copy link
Contributor Author

@vezenovm vezenovm Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unsure on whether I should do this but I went with returning a directive mainly to remove duplication of code as the logging information needed by whomever is calling solve is very similar to the Log directive itself. Whomever is calling solve must check that we have a finalized string output and whether the log is a trace or a println. This can be seen in this draft PR: noir-lang/noir#872. I will look into changing this as I agree it confuses the idea of whether the directive has been solved or not.

match directive {
Directive::Invert { x, result } => {
let val = witness_to_value(initial_witness, *x)?;
let inverse = val.inverse();
initial_witness.insert(*result, inverse);
Ok(())
Ok(None)
}
Directive::Quotient { a, b, q, r, predicate } => {
let val_a = get_value(a, initial_witness)?;
Expand Down Expand Up @@ -54,7 +54,7 @@ pub fn solve_directives(
initial_witness,
)?;

Ok(())
Ok(None)
}
Directive::Truncate { a, b, c, bit_size } => {
let val_a = get_value(a, initial_witness)?;
Expand All @@ -76,7 +76,7 @@ pub fn solve_directives(
initial_witness,
)?;

Ok(())
Ok(None)
}
Directive::ToRadix { a, b, radix, is_little_endian } => {
let value_a = get_value(a, initial_witness)?;
Expand Down Expand Up @@ -127,7 +127,7 @@ pub fn solve_directives(
}
}

Ok(())
Ok(None)
}
Directive::OddRange { a, b, r, bit_size } => {
let val_a = witness_to_value(initial_witness, *a)?;
Expand All @@ -153,7 +153,7 @@ pub fn solve_directives(
initial_witness,
)?;

Ok(())
Ok(None)
}
Directive::PermutationSort { inputs: a, tuple, bits, sort_by } => {
let mut val_a = Vec::new();
Expand Down Expand Up @@ -186,23 +186,27 @@ pub fn solve_directives(
let value = if value { FieldElement::one() } else { FieldElement::zero() };
insert_witness(*w, value, initial_witness)?;
}
Ok(())
Ok(None)
}
Directive::Log(info) => {
let witnesses = match info {
LogInfo::FinalizedOutput(output_string) => {
println!("{output_string}");
return Ok(());
Directive::Log { is_trace, output_info } => {
let witnesses = match output_info {
LogOutputInfo::FinalizedOutput(_) => {
return Ok(Some(directive.clone()));
}
LogInfo::WitnessOutput(witnesses) => witnesses,
LogOutputInfo::WitnessOutput(witnesses) => witnesses,
};

if witnesses.len() == 1 {
dbg!(witnesses.clone());
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved

let witness = &witnesses[0];
let log_value = witness_to_value(initial_witness, *witness)?;
println!("{}", format_field_string(*log_value));

return Ok(());
let solved_log_directive = Directive::Log {
is_trace: *is_trace,
output_info: LogOutputInfo::FinalizedOutput(format_field_string(*log_value)),
};
return Ok(Some(solved_log_directive));
}

// If multiple witnesses are to be fetched for a log directive,
Expand All @@ -221,9 +225,11 @@ pub fn solve_directives(

let output_witnesses_string = "[".to_owned() + &comma_separated_elements + "]";

println!("{output_witnesses_string}");

Ok(())
let solved_log_directive = Directive::Log {
is_trace: *is_trace,
output_info: LogOutputInfo::FinalizedOutput(output_witnesses_string),
};
Ok(Some(solved_log_directive))
}
}
}
Expand Down