-
Notifications
You must be signed in to change notification settings - Fork 102
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
Stage 0 public reference #2556
Stage 0 public reference #2556
Changes from all commits
11db74f
041a879
910da15
77c6e7e
6e26390
0f67e0a
5b4d910
f5b059b
e33b47e
04ecf57
f2d01a8
521f0e3
0e5712d
a66328a
bc8175b
a466877
f28b39c
f6eb6e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,9 @@ mod vm_processor; | |
pub use affine_expression::{AffineExpression, AffineResult, AlgebraicVariable}; | ||
pub use evaluators::partial_expression_evaluator::{PartialExpressionEvaluator, SymbolicVariables}; | ||
|
||
pub type Witness<T> = Vec<(String, Vec<T>)>; | ||
pub type Publics<T> = BTreeMap<String, Option<T>>; | ||
|
||
static OUTER_CODE_NAME: &str = "witgen (outer code)"; | ||
|
||
// TODO change this so that it has functions | ||
|
@@ -141,6 +144,7 @@ impl<T: FieldElement> WitgenCallbackContext<T> { | |
.with_external_witness_values(current_witness) | ||
.with_challenges(stage, challenges) | ||
.generate() | ||
.0 | ||
} | ||
} | ||
} | ||
|
@@ -202,7 +206,7 @@ impl<'a, 'b, T: FieldElement> WitnessGenerator<'a, 'b, T> { | |
|
||
/// Generates the committed polynomial values | ||
/// @returns the values (in source order) and the degree of the polynomials. | ||
pub fn generate(self) -> Vec<(String, Vec<T>)> { | ||
pub fn generate(self) -> (Witness<T>, Publics<T>) { | ||
record_start(OUTER_CODE_NAME); | ||
let fixed = FixedData::new( | ||
self.analyzed, | ||
|
@@ -241,7 +245,8 @@ impl<'a, 'b, T: FieldElement> WitnessGenerator<'a, 'b, T> { | |
let machines = MachineExtractor::new(&fixed).split_out_machines(); | ||
|
||
// Run main machine and extract columns from all machines. | ||
let columns = MutableState::new(machines.into_iter(), &self.query_callback).run(); | ||
let (columns, _publics) = | ||
MutableState::new(machines.into_iter(), &self.query_callback).run(); | ||
Comment on lines
247
to
+249
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, this confuses me still: Is it because at this point we'd expect them to be the same? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually intentional. This PR technically still uses witness columns to derive public values as you identified in In this other PR, This PR focuses more on making the pipeline work. |
||
|
||
let publics = extract_publics(&columns, self.analyzed); | ||
if !publics.is_empty() { | ||
|
@@ -258,7 +263,7 @@ impl<'a, 'b, T: FieldElement> WitnessGenerator<'a, 'b, T> { | |
|
||
let mut columns = if self.stage == 0 { | ||
// Multiplicities should be computed in the first stage | ||
MultiplicityColumnGenerator::new(&fixed).generate(columns, publics) | ||
MultiplicityColumnGenerator::new(&fixed).generate(columns, publics.clone()) | ||
} else { | ||
columns | ||
}; | ||
|
@@ -279,7 +284,8 @@ impl<'a, 'b, T: FieldElement> WitnessGenerator<'a, 'b, T> { | |
(name, column) | ||
}) | ||
.collect::<Vec<_>>(); | ||
witness_cols | ||
|
||
(witness_cols, publics) | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it would make sense to introduce a struct for both? Whenever you need the witness, you'll also need the publics. So then the pipeline would have a field
witness_and_publics: Option<Arc<WitnessAndPublics<T>>>
instead of two fields.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Took a slightly different approach by always returning (witness, publics) in the main APIs. Kept them separate so that in helper functions we can return only witness or only publics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pipeline now has
witness_and_publics
as a single field.