Update public examples #2572

qwang98 · 2025-03-21T05:08:57Z

Depends on #2567.

Did a blanket search of public declarations and added public references in all such cases (except one halo2 and one Stwo example, which will require more backend refactoring). This will be required once public reference values instead of witness is solely used for proving.

To make all such updated public examples work, also make the following changes:

Updated mock backend to use public reference values (this is technically the first backend that's edited to use public reference witgen entirely).
Updated pipeline functions for soundness tests (set_witness --> set_witness_and_publics).

Schaeff · 2025-03-21T09:15:34Z

Out of curiosity: do I understand correctly that we still have a RHS on public declarations, but it is not enforced in the circuit? Is it used for witgen? Can it be omitted?

qwang98 · 2025-03-25T00:42:20Z

Out of curiosity: do I understand correctly that we still have a RHS on public declarations, but it is not enforced in the circuit? Is it used for witgen? Can it be omitted?

Yes we still have a RHS on public declarations and it will be gradually phased out as I implement each backend using our public references. It's not used for witgen.

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: `9a58980`	Previous: `4dc747a`	Ratio
`jit-witgen-benchmark/jit_witgen_benchmark`	`30250330310` ns/iter (`± 93445799`)	`10493155444` ns/iter (`± 118276385`)	`2.88`

This comment was automatically generated by workflow using github-action-benchmark.

qwang98 · 2025-03-26T08:44:27Z

executor/src/witgen/machines/write_once_memory.rs

+    fn take_public_values(&mut self) -> BTreeMap<String, T> {
+        if self.publics.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.publics
+                .clone()
+                .into_iter()
+                .zip(public_values)
+                .collect()
+        }
    }


This basically slightly modifies the take_witness_values code but instead only takes the first 8 values. The more proper way to do this should be to solve the public reference constraints using one of our processors, similar to how it's systematically done in dynamic_machine and block_machine. I think this is a good simplification, but a bit hacky.

But why 8? It could be any number?

Because there are 8 publics here: https://github.com/powdr-labs/powdr/blob/main/std/machines/write_once_memory_with_8_publics.asm#L24-L31

qwang98 · 2025-03-26T08:45:43Z

executor/src/witgen/mod.rs

-        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);
+


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

qwang98 · 2025-03-26T08:49:20Z

executor/src/witgen/machines/write_once_memory.rs

-        if !parts.identities.is_empty() {
-            return None;
-        }
-


write_once_memory_with_publics now contain public reference identities, so this can't be used as a filter.

You could check that all identities reference a public though.

georgwiese

I looked at the first half, but stopping now as I'm not 100% sure I'm looking at the right diff, maybe this needs a rebase?

I'm also pretty confused at this point, because now we also have have two types of publics and sometimes we just set the scalar publics to an empty map. I guess that's because we don't want to have a massive PR that changes everything at once, but I find it hard to track where we are on the road towards only having scalar publics.

georgwiese · 2025-03-26T14:27:32Z

backend/src/composite/sub_prover.rs

 }

 impl<'s, F: FieldElement> SubProver<'s, F> {
    pub fn resume(self, response: Vec<(String, Vec<F>)>) -> RunStatus<'s, F> {
-        self.response_sender.send(response).unwrap();
+        self.response_sender
+            .send((response, BTreeMap::new()))


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?

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:

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).

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

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.

backend/src/mock/machine.rs

backend/src/plonky3/stark.rs

georgwiese · 2025-03-26T14:35:46Z

backend/src/plonky3/stark.rs

@@ -375,12 +375,22 @@ mod tests {
            pol fixed FIRST = [1] + [0]*;
            pol witness x;
            pol witness y;
-            y * y = y;
+            y = 1;


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

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.

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

executor-utils/src/expression_evaluator.rs

executor/src/witgen/machines/mod.rs

georgwiese · 2025-03-26T14:49:08Z

executor/src/witgen/machines/write_once_memory.rs

-        if !parts.identities.is_empty() {
-            return None;
-        }
-


You could check that all identities reference a public though.

executor/src/witgen/machines/write_once_memory.rs

georgwiese · 2025-03-26T14:58:32Z

executor/src/witgen/machines/write_once_memory.rs

+    fn take_public_values(&mut self) -> BTreeMap<String, T> {
+        if self.publics.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.publics
+                .clone()
+                .into_iter()
+                .zip(public_values)
+                .collect()
+        }
    }


But why 8? It could be any number?

georgwiese · 2025-03-26T15:04:12Z

executor/src/witgen/mod.rs

+pub fn extract_publics<T>(
+    public_references: BTreeMap<String, T>,
+    pil: &Analyzed<T>,
+) -> BTreeMap<String, Option<T>>


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?

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

georgwiese · 2025-03-27T09:16:45Z

backend/src/plonky3/stark.rs

@@ -375,12 +375,22 @@ mod tests {
            pol fixed FIRST = [1] + [0]*;
            pol witness x;
            pol witness y;
-            y * y = y;
+            y = 1;


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

georgwiese · 2025-03-27T14:32:41Z

executor/src/witgen/machines/write_once_memory.rs

+        // 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(_)))
+            })
+        }) {


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(_)))

}) {

georgwiese · 2025-03-27T14:35:05Z

executor/src/witgen/machines/write_once_memory.rs

@@ -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


I think this change is not necessary?

georgwiese · 2025-03-27T17:41:42Z

executor/src/witgen/machines/write_once_memory.rs

+            public_names: parts
+                .fixed_data
+                .analyzed
+                .public_declarations_in_source_order()


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.

georgwiese · 2025-03-27T17:49:18Z

pipeline/src/pipeline.rs

+            // 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)));


Why do we keep the publics?

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.

georgwiese · 2025-03-27T17:56:36Z

riscv/tests/common/mod.rs

-    // Compute the witness once for all tests that follow.
-    // we will have to include a backend type here or compute_witness will panic
-    pipeline.compute_witness().unwrap();
-
-    test_mock_backend(pipeline.clone());
-
-    // verify with the mock prover
-    if T::known_field().unwrap() == KnownField::GoldilocksField {
-        let pipeline_gl: Pipeline<GoldilocksField> =
-            unsafe { std::mem::transmute(pipeline.clone()) };
-        test_mock_backend(pipeline_gl);
-    }
-


Is this change related to publics?

qwang98 added 19 commits March 17, 2025 16:38

prelim idea

11db74f

pass stage-0 values to backend with print statements

041a879

clippy

910da15

remove println

77c6e7e

remove more println

6e26390

clippy

0f67e0a

clippy

5b4d910

fix test error for set witness

f5b059b

use btreemap and revert a bit

e33b47e

fix ci

04ecf57

fix build

f2d01a8

fix ci

521f0e3

revert unwanted

0e5712d

first commit

61609f2

revert some unwanted

1ce0a97

fix dynamic machine

a66328a

Merge branch 'stage-1-public-reference' into stage-2-public-reference

e449004

first commit after debugging

e989f3b

second commit with most test changes

ccfbceb

qwang98 changed the base branch from main to stage-2-public-reference March 21, 2025 05:09

updated pipeline and debugged

dcbfa1e

qwang98 mentioned this pull request Mar 24, 2025

Stage 0 public reference #2556

Merged

qwang98 added 7 commits March 24, 2025 12:44

unify witness and publics

bc8175b

Merge branch 'main' into stage-1-public-reference

a466877

fix build

f28b39c

Merge branch 'stage-1-public-reference' into stage-2-public-reference

b5d2f7a

Merge branch 'stage-2-public-reference' into update-public-examples

bbb9334

fix build

f6eb6e9

Merge branch 'stage-1-public-reference' into stage-2-public-reference

7798d19

qwang98 added 2 commits March 24, 2025 13:21

Merge branch 'stage-2-public-reference' into update-public-examples

5cc7e1d

ci

b11d553

qwang98 added 6 commits March 26, 2025 12:31

fix riscv bugs with printout

0b34bb4

remove println and fix clippy

57cec98

ci

9a58980

defer stwo error to future PR

c78890e

Merge branch 'main' into stage-2-public-reference

3fc9910

Merge branch 'stage-2-public-reference' into update-public-examples

d92168e

github-actions bot reviewed Mar 26, 2025

View reviewed changes

qwang98 added 7 commits March 26, 2025 13:21

ci

50038b4

ci

823d04d

Merge branch 'stage-2-public-reference' into update-public-examples

86971c5

ci again'

346c952

delete unwanted

5d623f4

fix ci

cf87630

revert halo2 example and defer to future PR

189d4c8

qwang98 commented Mar 26, 2025

View reviewed changes

qwang98 requested a review from georgwiese March 26, 2025 08:53

qwang98 marked this pull request as ready for review March 26, 2025 08:56

Base automatically changed from stage-2-public-reference to main March 26, 2025 11:51

georgwiese reviewed Mar 26, 2025

View reviewed changes

qwang98 added 3 commits March 27, 2025 12:20

Merge branch 'main' into update-public-examples

e6a6f44

some fixes

6d993c6

added TODO comment

d81a962

qwang98 requested a review from georgwiese March 27, 2025 06:33

georgwiese mentioned this pull request Mar 27, 2025

More robust publics implementation for WriteOnceMemory #2600

Open

georgwiese reviewed Mar 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update public examples #2572

Update public examples #2572

qwang98 commented Mar 21, 2025 •

edited

Loading

Schaeff commented Mar 21, 2025

qwang98 commented Mar 25, 2025

github-actions bot left a comment

qwang98 Mar 26, 2025 •

edited

Loading

georgwiese Mar 26, 2025

qwang98 Mar 27, 2025

qwang98 Mar 26, 2025

qwang98 Mar 26, 2025

georgwiese Mar 26, 2025

qwang98 Mar 27, 2025

qwang98 Mar 27, 2025

georgwiese left a comment

georgwiese Mar 26, 2025

qwang98 Mar 27, 2025

georgwiese Mar 26, 2025

qwang98 Mar 27, 2025

georgwiese Mar 27, 2025

georgwiese Mar 26, 2025

georgwiese Mar 26, 2025

georgwiese Mar 26, 2025

qwang98 Mar 27, 2025

georgwiese Mar 27, 2025

georgwiese Mar 27, 2025

georgwiese Mar 27, 2025

georgwiese Mar 27, 2025

georgwiese Mar 27, 2025

georgwiese Mar 27, 2025

georgwiese Mar 27, 2025

Update public examples #2572

Are you sure you want to change the base?

Update public examples #2572

Conversation

qwang98 commented Mar 21, 2025 • edited Loading

Schaeff commented Mar 21, 2025

qwang98 commented Mar 25, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

qwang98 Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgwiese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwang98 commented Mar 21, 2025 •

edited

Loading

qwang98 Mar 26, 2025 •

edited

Loading