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

Update public examples #2572

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
11db74f
prelim idea
qwang98 Mar 17, 2025
041a879
pass stage-0 values to backend with print statements
qwang98 Mar 19, 2025
910da15
clippy
qwang98 Mar 19, 2025
77c6e7e
remove println
qwang98 Mar 19, 2025
6e26390
remove more println
qwang98 Mar 19, 2025
0f67e0a
clippy
qwang98 Mar 19, 2025
5b4d910
clippy
qwang98 Mar 19, 2025
f5b059b
fix test error for set witness
qwang98 Mar 19, 2025
e33b47e
use btreemap and revert a bit
qwang98 Mar 19, 2025
04ecf57
fix ci
qwang98 Mar 19, 2025
f2d01a8
fix build
qwang98 Mar 19, 2025
521f0e3
fix ci
qwang98 Mar 20, 2025
0e5712d
revert unwanted
qwang98 Mar 20, 2025
61609f2
first commit
qwang98 Mar 20, 2025
1ce0a97
revert some unwanted
qwang98 Mar 20, 2025
a66328a
fix dynamic machine
qwang98 Mar 20, 2025
e449004
Merge branch 'stage-1-public-reference' into stage-2-public-reference
qwang98 Mar 20, 2025
e989f3b
first commit after debugging
qwang98 Mar 21, 2025
ccfbceb
second commit with most test changes
qwang98 Mar 21, 2025
dcbfa1e
updated pipeline and debugged
qwang98 Mar 21, 2025
bc8175b
unify witness and publics
qwang98 Mar 24, 2025
a466877
Merge branch 'main' into stage-1-public-reference
qwang98 Mar 24, 2025
f28b39c
fix build
qwang98 Mar 24, 2025
b5d2f7a
Merge branch 'stage-1-public-reference' into stage-2-public-reference
qwang98 Mar 24, 2025
bbb9334
Merge branch 'stage-2-public-reference' into update-public-examples
qwang98 Mar 24, 2025
f6eb6e9
fix build
qwang98 Mar 24, 2025
7798d19
Merge branch 'stage-1-public-reference' into stage-2-public-reference
qwang98 Mar 24, 2025
5cc7e1d
Merge branch 'stage-2-public-reference' into update-public-examples
qwang98 Mar 24, 2025
b11d553
ci
qwang98 Mar 24, 2025
0b34bb4
fix riscv bugs with printout
qwang98 Mar 26, 2025
57cec98
remove println and fix clippy
qwang98 Mar 26, 2025
9a58980
ci
qwang98 Mar 26, 2025
c78890e
defer stwo error to future PR
qwang98 Mar 26, 2025
3fc9910
Merge branch 'main' into stage-2-public-reference
qwang98 Mar 26, 2025
d92168e
Merge branch 'stage-2-public-reference' into update-public-examples
qwang98 Mar 26, 2025
50038b4
ci
qwang98 Mar 26, 2025
823d04d
ci
qwang98 Mar 26, 2025
86971c5
Merge branch 'stage-2-public-reference' into update-public-examples
qwang98 Mar 26, 2025
346c952
ci again'
qwang98 Mar 26, 2025
5d623f4
delete unwanted
qwang98 Mar 26, 2025
cf87630
fix ci
qwang98 Mar 26, 2025
189d4c8
revert halo2 example and defer to future PR
qwang98 Mar 26, 2025
e6a6f44
Merge branch 'main' into update-public-examples
qwang98 Mar 27, 2025
6d993c6
some fixes
qwang98 Mar 27, 2025
d81a962
added TODO comment
qwang98 Mar 27, 2025
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
3 changes: 3 additions & 0 deletions backend/src/composite/sub_prover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ pub struct SubProver<'s, F: FieldElement> {

impl<'s, F: FieldElement> SubProver<'s, F> {
pub fn resume(self, response: Vec<(String, Vec<F>)>) -> RunStatus<'s, F> {
// `response_sender` has to send `(witness, publics)` to match `response_receiver`.
// TODO: currently publics is set to empty, but we should update this once composite backend
// uses public reference values instead of witness values in the future.
self.response_sender
.send((response, BTreeMap::new()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I'm always a bit confused by the sender and receiver here, but why do we need to send a map and then always send an empty map?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This no longer appears after rebase as it's added in a prior PR #2567.

However, to answer this question, this is actually inferred by the compiler:

  1. The witgen callback function now returns both witness and publics, as needed for stage 1 publics: https://github.com/powdr-labs/powdr/blob/main/backend/src/composite/sub_prover.rs#L32-L42
  • The returned value response_receiver.recv().unwrap() therefore has to be (witness, publics).
  1. It's also required that the sender and receive have the same underlying type (I think somewhere inside this package): https://github.com/powdr-labs/powdr/blob/main/backend/src/composite/sub_prover.rs#L25
  2. Therefore, the compiler forces that the response_sender always send a (witness, publics).

I left this out so that #2567 don't grow too large, but I think one potential effect of this is that composite backends won't work for stage 1 publics, but I was planning to work on other backends in future PRs regardless. (It currently isn't an issue because all backends except Mock still use the witness for exposing publics after this PR).

To make sure this isn't forgotten, however, I'm adding a TODO comment here.

.unwrap();
Expand Down
6 changes: 3 additions & 3 deletions backend/src/mock/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ impl<'a, F: FieldElement> Machine<'a, F> {
pub fn try_new(
machine_name: String,
witness: &'a [(String, Vec<F>)],
publics: &'a BTreeMap<String, Option<F>>,
fixed: &'a [(String, VariablySizedColumn<F>)],
pil: &'a Analyzed<F>,
witgen_callback: &WitgenCallback<F>,
Expand Down Expand Up @@ -55,9 +56,8 @@ impl<'a, F: FieldElement> Machine<'a, F> {

let intermediate_definitions = pil.intermediate_definitions();

// TODO: Supports publics.
let values =
OwnedTerminalValues::new(pil, witness, fixed).with_challenges(challenges.clone());
let values = OwnedTerminalValues::new(pil, witness, publics.clone(), fixed)
.with_challenges(challenges.clone());

Some(Self {
machine_name,
Expand Down
3 changes: 2 additions & 1 deletion backend/src/mock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl<F: FieldElement> Backend<F> for MockBackend<F> {
fn prove(
&self,
witness: &[(String, Vec<F>)],
_publics: &BTreeMap<String, Option<F>>,
publics: &BTreeMap<String, Option<F>>,
prev_proof: Option<Proof>,
witgen_callback: WitgenCallback<F>,
) -> Result<Proof, Error> {
Expand Down Expand Up @@ -111,6 +111,7 @@ impl<F: FieldElement> Backend<F> for MockBackend<F> {
Machine::try_new(
machine_name.clone(),
witness,
publics,
&self.fixed,
pil,
&witgen_callback,
Expand Down
18 changes: 17 additions & 1 deletion backend/src/plonky3/stark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,12 +375,22 @@ mod tests {
pol fixed FIRST = [1] + [0]*;
pol witness x;
pol witness y;
y * y = y;
y = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change necessary? It seems like before y was not uniquely constraint, but why did it work before then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are some witgen errors after I added in the public reference constraints. Previously, y is simply set to 0 because it can't be uniquely solved and thus passes the constraint "by chance". I think non unique constraints of the type y * y = y isn't something we are explicitly testing in this public_values() test and therefore I just made this unique to avoid the witgen error.

However, happy to look deeper into this or defer to another PR for a fix if we wonder why y is simply set to 0 before but creates a witgen error now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I think it's fine, just curious :) Thanks!

x' = (1 - FIRST') * (x + 1);
public out0 = x(6);
public out1 = x(7);
public out2 = y(3);
public out3 = y(5);

pol fixed IS_6 = [0, 0, 0, 0, 0, 0, 1, 0];
pol fixed IS_7 = [0, 0, 0, 0, 0, 0, 0, 1];
pol fixed IS_3 = [0, 0, 0, 1, 0, 0, 0, 0];
pol fixed IS_5 = [0, 0, 0, 0, 0, 1, 0, 0];

IS_6 * (x - out0) = 0;
IS_7 * (x - out1) = 0;
IS_3 * (y - out2) = 0;
IS_5 * (y - out3) = 0;
";
run_test(content);
}
Expand Down Expand Up @@ -412,6 +422,9 @@ mod tests {
x + y = z;

public outz = z(7);

col fixed IS_LAST = [0]* + [1];
IS_LAST * (z - outz) = 0;
"#;
let malicious_publics = Some(vec![0]);
run_test_publics(content, &malicious_publics);
Expand Down Expand Up @@ -486,6 +499,9 @@ mod tests {
x = y + beta * alpha;

public out = y(N - 1);

col fixed IS_LAST = [0]* + [1];
IS_LAST * (y - out) = 0;
"#;
run_test(content);
}
Expand Down
11 changes: 8 additions & 3 deletions executor-utils/src/expression_evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,11 @@ pub struct RowValues<'a, F> {
row: usize,
}

impl<F: std::fmt::Debug> OwnedTerminalValues<F> {
impl<F: std::fmt::Debug + Clone> OwnedTerminalValues<F> {
pub fn new(
pil: &Analyzed<F>,
witness_columns: Vec<(String, Vec<F>)>,
publics: BTreeMap<String, Option<F>>,
fixed_columns: Vec<(String, Vec<F>)>,
) -> Self {
let mut columns_by_name = witness_columns
Expand All @@ -57,9 +58,13 @@ impl<F: std::fmt::Debug> OwnedTerminalValues<F> {
.map(|column| (poly_id, column))
})
.collect();
let publics = publics
.iter()
.map(|(name, value)| (name.clone(), value.clone().unwrap()))
.collect();
Comment on lines +61 to +64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let publics = publics
.iter()
.map(|(name, value)| (name.clone(), value.clone().unwrap()))
.collect();
let publics = publics
.into_iter()
.map(|(name, value)| (name, value.unwrap()))
.collect();

Then you don't need the + Clone above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I'm a bit confused, why does it expect an Option<F> and then unwraps it? What's the difference to not having the key in the map in the first place?

Self {
trace,
public_values: Default::default(),
public_values: publics,
challenge_values: Default::default(),
}
}
Expand Down Expand Up @@ -123,7 +128,7 @@ impl<F: FieldElement, T: From<F>> TerminalAccess<T> for RowValues<'_, F> {

/// Evaluates an algebraic expression to a value.
pub struct ExpressionEvaluator<'a, T, Expr, TA> {
terminal_access: TA,
pub terminal_access: TA,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub terminal_access: TA,
terminal_access: TA,

Seems like that change is not needed?

intermediate_definitions: &'a BTreeMap<AlgebraicReferenceThin, Expression<T>>,
/// Maps intermediate reference to their evaluation. Updated throughout the lifetime of the
/// ExpressionEvaluator.
Expand Down
1 change: 1 addition & 0 deletions executor/src/witgen/bus_accumulator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ impl<'a, T: FieldElement, Ext: ExtensionField<T> + Sync> BusAccumulatorGenerator
let values = OwnedTerminalValues::new(
pil,
witness_columns.to_vec(),
BTreeMap::new(),
fixed_columns
.map(|(name, values)| (name, values.to_vec()))
.collect(),
Expand Down
59 changes: 55 additions & 4 deletions executor/src/witgen/machines/write_once_memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ use std::collections::{BTreeMap, HashMap};
use itertools::{Either, Itertools};

use num_traits::One;
use powdr_ast::analyzed::{PolyID, PolynomialType};
use powdr_ast::analyzed::{AlgebraicExpression, PolyID, PolynomialType};
use powdr_ast::parsed::visitor::{AllChildren, Children};
use powdr_number::{DegreeType, FieldElement};

use crate::witgen::data_structures::identity::BusReceive;
Expand Down Expand Up @@ -42,6 +43,7 @@ pub struct WriteOnceMemory<'a, T: FieldElement> {
/// The memory content
data: BTreeMap<DegreeType, Vec<Option<T>>>,
name: String,
public_names: Vec<String>,
}

impl<'a, T: FieldElement> WriteOnceMemory<'a, T> {
Expand All @@ -50,7 +52,13 @@ impl<'a, T: FieldElement> WriteOnceMemory<'a, T> {
fixed_data: &'a FixedData<'a, T>,
parts: &MachineParts<'a, T>,
) -> Option<Self> {
if !parts.identities.is_empty() {
// All identities should have a public reference
if !parts.identities.iter().all(|id| {
(*id).children().any(|c| {
c.all_children()
.any(|c| matches!(c, AlgebraicExpression::PublicReference(_)))
})
}) {
Comment on lines +55 to +61
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// All identities should have a public reference
if !parts.identities.iter().all(|id| {
(*id).children().any(|c| {
c.all_children()
.any(|c| matches!(c, AlgebraicExpression::PublicReference(_)))
})
}) {
// The only identities we'd expect would be to expose public values.
if !parts.identities.iter().all(|id| {
id.all_children()
.any(|c| matches!(c, AlgebraicExpression::PublicReference(_)))
}) {

return None;
}

Expand Down Expand Up @@ -135,6 +143,12 @@ impl<'a, T: FieldElement> WriteOnceMemory<'a, T> {
value_polys,
key_to_index,
data: BTreeMap::new(),
public_names: parts
.fixed_data
.analyzed
.public_declarations_in_source_order()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fragile, because this would include publics of other machines. I opened #2600 as a suggested fix, it also contains the other suggestions to this file.

.map(|(name, _)| name.clone())
.collect(),
})
}

Expand Down Expand Up @@ -263,7 +277,8 @@ impl<'a, T: FieldElement> Machine<'a, T> for WriteOnceMemory<'a, T> {
&mut self,
_mutable_state: &'b MutableState<'a, T, Q>,
) -> HashMap<String, Vec<T>> {
self.value_polys
let witness = self
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this change is not necessary?

.value_polys
.iter()
.enumerate()
.map(|(value_index, poly)| {
Expand All @@ -285,6 +300,42 @@ impl<'a, T: FieldElement> Machine<'a, T> for WriteOnceMemory<'a, T> {
(*poly, column)
})
.map(|(poly_id, column)| (self.fixed_data.column_name(&poly_id).to_string(), column))
.collect()
.collect();
witness
}

fn take_public_values(&mut self) -> BTreeMap<String, T> {
if self.public_names.is_empty() {
BTreeMap::new()
} else {
let public_values: Vec<_> = self
.value_polys
.iter()
.enumerate()
.flat_map(|(value_index, poly)| {
self.fixed_data.witness_cols[poly]
.external_values
.cloned()
.map(|mut external_values| {
// External witness values might only be provided partially.
external_values.resize(self.degree as usize, T::zero());
external_values
})
.unwrap_or_else(|| {
let mut column = vec![T::zero(); 8]; // 8 public values
for (row, values) in self.data.iter() {
column[*row as usize] = values[value_index].unwrap_or_default();
}
column
})
})
.collect();

self.public_names
.clone()
.into_iter()
.zip(public_values)
.collect()
}
}
}
23 changes: 9 additions & 14 deletions executor/src/witgen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,11 @@ 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, _publics) =
let (columns, publics) =
MutableState::new(machines.into_iter(), &self.query_callback).run();

let publics = extract_publics(&columns, self.analyzed);
let publics = extract_publics(publics, self.analyzed);

Comment on lines -250 to +254
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The key change is that publics are now assembled from public reference witgen and passed off to the backend.

if !publics.is_empty() {
log::debug!("Publics:");
}
Expand Down Expand Up @@ -291,22 +292,16 @@ impl<'a, 'b, T: FieldElement> WitnessGenerator<'a, 'b, T> {
}
}

pub fn extract_publics<'a, T, I>(witness: I, pil: &Analyzed<T>) -> BTreeMap<String, Option<T>>
pub fn extract_publics<T>(
public_references: BTreeMap<String, T>,
pil: &Analyzed<T>,
) -> BTreeMap<String, Option<T>>
Comment on lines +295 to +298
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this do exactly? I think the inputs are publics already, but then this makes sure that there is an entry for each public declaration? What would happen if we removed the function and just return the publics as returned from MutableState::run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly, it makes sure that we have a value for each public declaration, which might not be the case if we have stage-1 publics during stage-0 witgen, in which case extract_publics will assign a None value to the stage-1 public declaration. In the end, it's checked in all backend testing functions that all values in the publics map are Some, for example: https://github.com/powdr-labs/powdr/blob/main/pipeline/src/test_util.rs#L170-L174

where
T: FieldElement,
I: IntoIterator<Item = (&'a String, &'a Vec<T>)>,
{
let witness = witness
.into_iter()
.map(|(name, col)| (name.clone(), col))
.collect::<BTreeMap<_, _>>();
pil.public_declarations_in_source_order()
.map(|(name, public_declaration)| {
let poly_name = &public_declaration.referenced_poly_name();
let poly_row = public_declaration.row();
let value = witness
.get(poly_name)
.map(|column| column[poly_row as usize]);
.map(|(name, _)| {
let value = public_references.get(name).cloned();
((*name).clone(), value)
})
.collect()
Expand Down
3 changes: 2 additions & 1 deletion parser/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,8 @@ namespace Fibonacci(N);
[x + 2, y'] in [ISLAST, 7];
y $ [x + 2, y'] is ISLAST $ [ISLAST, 7];
(x - 2) * y = 8;
public out = y(%last_row);"#;
public out = y(%last_row);
ISLAST * (y - out) = 0;"#;
let printed = format!("{}", parse(Some("input"), input).unwrap());
assert_eq!(input.trim(), printed.trim());
}
Expand Down
2 changes: 2 additions & 0 deletions pil-analyzer/tests/parse_display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ namespace std::prover(65536);
namespace std::convert(65536);
let int = [];
namespace T(65536);
col fixed IS_2 = [0_fe, 0_fe, 1_fe] + [0_fe]*;
T::IS_2 * (P - T::pc) = 0;
col fixed first_step = [1_fe] + [0_fe]*;
col fixed line(i) { i };
let ops: int -> bool = |i| i < 7_int && 6_int >= -i;
Expand Down
33 changes: 26 additions & 7 deletions pipeline/src/pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -541,16 +541,19 @@ impl<T: FieldElement> Pipeline<T> {
}

/// Sets the witness to the provided value.
pub fn set_witness(mut self, witness: Vec<(String, Vec<T>)>) -> Self {
pub fn set_witness_and_publics(
mut self,
witness: Vec<(String, Vec<T>)>,
publics: BTreeMap<String, Option<T>>,
) -> Self {
if self.output_dir.is_some() {
// Some future steps require the witness to be persisted.
let fixed_cols = self.compute_fixed_cols().unwrap();
self.maybe_write_witness(&fixed_cols, &witness).unwrap();
}
Pipeline {
artifact: Artifacts {
// need to set publics to Some, or `compute_witness` will run auto witgen
witness_and_publics: Some(Arc::new((witness, BTreeMap::new()))),
witness_and_publics: Some(Arc::new((witness, publics))),
// we're changing the witness, clear the current proof
proof: None,
..self.artifact
Expand Down Expand Up @@ -708,7 +711,12 @@ impl<T: FieldElement> Pipeline<T> {
// but give it different witnesses and generate different proofs.
// The previous alternative to this was cloning the entire pipeline.
pub fn rollback_from_witness(&mut self) {
self.artifact.witness_and_publics = None;
if let Some(old_arc) = &self.artifact.witness_and_publics {
// Clone the public part
let public = old_arc.1.clone();
// Replace with an empty witness and the preserved public
self.artifact.witness_and_publics = Some(Arc::new((Vec::new(), public)));
Comment on lines +715 to +718
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we keep the publics?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this causes a few other changes in the file. In my mind, scalar publics could almost be considered part of the witness (you solve for them, you need them to evaluate constraints, ...), so I don't see why they are treated differently.

}
self.artifact.proof = None;
self.arguments.external_witness_values.clear();
}
Expand Down Expand Up @@ -1047,7 +1055,10 @@ impl<T: FieldElement> Pipeline<T> {

pub fn compute_witness(&mut self) -> WitgenResult<T> {
if let Some(arc) = &self.artifact.witness_and_publics {
return Ok(arc.clone());
// If exists, witness will always be non-empty, but not necessarily publics
if !arc.0.is_empty() {
return Ok(arc.clone());
}
}

self.host_context.clear();
Expand Down Expand Up @@ -1079,8 +1090,16 @@ impl<T: FieldElement> Pipeline<T> {
.all(|name| external_witness_values.iter().any(|(e, _)| e == name))
{
self.log("All witness columns externally provided, skipping witness generation.");
self.artifact.witness_and_publics =
Some(Arc::new((external_witness_values, BTreeMap::new())));
if let Some(old_arc) = &self.artifact.witness_and_publics {
// Clone the public part
let public = old_arc.1.clone();
// Replace with an empty witness and the preserved public
self.artifact.witness_and_publics =
Some(Arc::new((external_witness_values, public)));
} else {
self.artifact.witness_and_publics =
Some(Arc::new((external_witness_values, BTreeMap::new())));
}
} else {
self.log("Deducing witness columns...");
let start = Instant::now();
Expand Down
Loading
Loading