Stage 0 public reference #2556

qwang98 · 2025-03-17T08:39:03Z

This PR is a step towards passing stage-0 public witgen to the backend. It focuses on modifying the pipeline to separate witness values from public values. Additional changes to pass stage-1 public witgen is needed before public values are fully passed to the backend.

georgwiese · 2025-03-17T13:16:34Z

I think the reason is that the second-stage witgen is run by SecondStageMachine which currently ignores updated_data.publics? If you put a panic here and run the test with RUST_BACKTRACE=1, you'll see that it finds the public while running SecondStageMachine::process. So the solution would be to collect the public there and return it in take_public_values I think, like is already done for block machine and dynamic machine.

But one other thing to note is that this code is only run in this path, when we use automatic witgen for the second phase. In practice (when we use the bus linker and are on Goldilocks or BabyBear), it would use the hand-written bus witgen, which would have to implement extracting any second-stage publics again.

So maybe the first step would be to pass stage-0 publics to the backend first?

qwang98 · 2025-03-18T07:39:54Z

this path

I implemented passing stage-0 publics to backend in #2502, which unfortunately can't pass CI due to some stage-1 publics tests.

For extracting second stage publics in the non-bus linker case, I assume that I would need to:

Append hand-written bus witgen to stage-0 witness (as is already done).
Pass this new witness to witness generator.
Generate the publics using the witness generator. (There should be no new stage-1 witgen as it's just done by the hand-written bus witgen).

Am I understanding it correctly?

georgwiese · 2025-03-18T11:54:50Z

I'm a bit confused. Do you mean "second stage publics in he bus linker case" (not non-bus)?

There are two paths see this switch:

We are using the bus linker and are not on BN254: This is the "hand-written" bus witgen. I'm not sure if we currently even have publics in that case. But in principle, in that case, we should be returning the last row of the accumulator column as a second-stage public (so that the verifier can check whether they sum to 0 across machines). So, I think in that path we'd just add to the hand-written witgen to also return the publics.
For the other path, I think you're on a good track with this PR, but you'll need to do something similar to what's currently happening in block machines (for example) in SecondStageMachine.

Let me know if you think a call would be helpful :)

qwang98 · 2025-03-18T13:11:05Z

see this switch

Ahh yes. Sorry I meant the opposite. Your description makes sense so I'll take a stab tomorrow and request a call if I get stuck. :) Thanks!

qwang98 · 2025-03-19T09:02:01Z

executor/src/witgen/mod.rs

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


Note that _publics is not passed to the backend yet till I also implement stage-1 publics. However, the pipeline for passing publics to the backend is ready (rest of the PR).

Right, this confuses me still: run() returns some publics, but then they are ignored and instead we use extract_publics(&columns, self.analyzed).

Is it because at this point we'd expect them to be the same?

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: `f2d01a8`	Previous: `f5b059b`	Ratio
`executor-benchmark/keccak`	`25304196723` ns/iter (`± 340555011`)	`20837156242` ns/iter (`± 342822186`)	`1.21`

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

georgwiese

Cool! Seems like a bit of an intermediate step, but I think it makes sense and looks like we're quite close to making the switch to scalar publics everywhere.

georgwiese · 2025-03-21T10:44:55Z

executor/src/witgen/machines/mod.rs

+        match self {
+            KnownMachine::BlockMachine(m) => m.take_public_values(),
+            KnownMachine::DynamicMachine(m) => m.take_public_values(),
+            _ => BTreeMap::new(),


I think we want to enumerate all machines here? And maybe change the default implementation to return an empty map?

georgwiese · 2025-03-21T10:48:49Z

pipeline/src/pipeline.rs

@@ -538,6 +545,8 @@ impl<T: FieldElement> Pipeline<T> {
        Pipeline {
            artifact: Artifacts {
                witness: Some(Arc::new(witness)),
+                // need to set publics to Some, or `compute_witness` will run auto witgen
+                publics: Some(Arc::new(BTreeMap::new())),


I think here we can change the function so that both needs to be provided, right? Seems reasonable that anyone who knows the witness should also know the publics.

georgwiese · 2025-03-21T10:49:46Z

pipeline/src/pipeline.rs

@@ -520,6 +525,8 @@ impl<T: FieldElement> Pipeline<T> {
        Ok(Pipeline {
            artifact: Artifacts {
                witness: Some(Arc::new(witness)),
+                // need to set publics to Some, or `compute_witness` will run auto witgen
+                publics: Some(Arc::new(BTreeMap::new())),


Hmm, I guess we need to export publics along with the witness then? Problem for a later time, but good to keep in mind.

georgwiese · 2025-03-21T10:54:19Z

executor/src/witgen/mod.rs

+pub type Witness<T> = Vec<(String, Vec<T>)>;
+pub type Publics<T> = BTreeMap<String, Option<T>>;


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.

georgwiese · 2025-03-21T10:59:19Z

executor/src/witgen/mod.rs

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


Right, this confuses me still: run() returns some publics, but then they are ignored and instead we use extract_publics(&columns, self.analyzed).

Is it because at this point we'd expect them to be the same?

georgwiese · 2025-03-21T11:14:38Z

executor/src/witgen/data_structures/mutable_state.rs

+                        acc_witness
+                            .entry(key)
+                            .or_insert_with(Vec::new)
+                            .append(&mut vec_vals);


I don't think we want to concatenate columns of different machines! I think this should be a assert!(acc_witness.set(key, vec_vals).is_none()).

georgwiese · 2025-03-21T11:20:10Z

executor/src/witgen/data_structures/mutable_state.rs

-                    })
-                    .unwrap();
-                let columns = machine.take_witness_col_values(&self).into_iter();
+        let (witness_columns, public_values, _processed) = self.machines.iter().fold(


BTW, might be a matter of taste, but I find the fold API a bit confusing, and it seems like this works as well:

let witness_columns = { // We keep the already processed machines mutably borrowed so that // "later" machines do not try to create new rows in already processed // machines. let mut processed = vec![]; self.machines .iter() .flat_map(|machine| { let mut machine = machine.try_borrow_mut().unwrap_or_else(|_| { panic!("Recursive machine dependencies while finishing machines."); }); let columns = machine.take_witness_col_values(&self).into_iter(); processed.push(machine); columns }) .collect() }; let public_values = self .machines .iter() .flat_map(|machine| { let mut machine = machine.try_borrow_mut().unwrap_or_else(|_| { panic!("Recursive machine dependencies while finishing machines."); }); machine.take_public_values().into_iter() }) .collect();

It's less of a change to before and keeps the processed shenanigans local to the call to take_witness_col_values (which can in principle call other machines, whereas take_public_values can't, because it doesn't have access to MutableState).

prelim idea

11db74f

qwang98 requested a review from georgwiese March 17, 2025 08:57

qwang98 added 8 commits March 19, 2025 13:53

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

qwang98 commented Mar 19, 2025

View reviewed changes

qwang98 added 2 commits March 19, 2025 17:25

fix ci

04ecf57

fix build

f2d01a8

github-actions bot reviewed Mar 19, 2025

View reviewed changes

fix ci

521f0e3

qwang98 changed the title ~~Stage 1 public reference~~ Stage 0 public reference Mar 20, 2025

revert unwanted

0e5712d

qwang98 marked this pull request as ready for review March 20, 2025 03:35

fix dynamic machine

a66328a

qwang98 mentioned this pull request Mar 20, 2025

Stage 1 public reference #2567

Open

georgwiese reviewed Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stage 0 public reference #2556

Stage 0 public reference #2556

qwang98 commented Mar 17, 2025 •

edited

Loading

georgwiese commented Mar 17, 2025

qwang98 commented Mar 18, 2025

georgwiese commented Mar 18, 2025

qwang98 commented Mar 18, 2025

qwang98 Mar 19, 2025 •

edited

Loading

georgwiese Mar 21, 2025

github-actions bot left a comment

georgwiese left a comment

georgwiese Mar 21, 2025

georgwiese Mar 21, 2025

georgwiese Mar 21, 2025

georgwiese Mar 21, 2025

georgwiese Mar 21, 2025

georgwiese Mar 21, 2025

georgwiese Mar 21, 2025

		pub type Witness<T> = Vec<(String, Vec<T>)>;
		pub type Publics<T> = BTreeMap<String, Option<T>>;

Stage 0 public reference #2556

Are you sure you want to change the base?

Stage 0 public reference #2556

Conversation

qwang98 commented Mar 17, 2025 • edited Loading

georgwiese commented Mar 17, 2025

qwang98 commented Mar 18, 2025

georgwiese commented Mar 18, 2025

qwang98 commented Mar 18, 2025

qwang98 Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

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

qwang98 commented Mar 17, 2025 •

edited

Loading

qwang98 Mar 19, 2025 •

edited

Loading