Manual Bus Witgen: Output folded columns only if they exist #2428

georgwiese · 2025-02-03T00:57:22Z

Depends on #2426.

With this PR, we use the annotations added in #2412 and #2426 to find the columns being generated. This allows us to detect when the folded payload is not persisted as a witness column.

The background is that @Schaeff is working on #2425, which would undo persisting the folded payloads in some cases, allowing us to spend fewer witness columns per bus interaction. With this PR, this should be fine: The column type will change from witness to intermediate, which means that the bus witgen will not output any folded columns.

It can be tested by changing the materialize_folded bool to false, e.g. here and running:

cargo run pil test_data/asm/block_to_block.asm \
    -o output -f --linker-mode bus --prove-with mock --field gl

This used to fail before this PR.

qwang98 · 2025-02-03T10:33:10Z

executor/src/witgen/bus_accumulator/mod.rs

+        .iter()
+        .zip_eq(folded)
+        .filter_map(|(expr, column)| match expr {
+            AlgebraicExpression::Reference(col_reference) if col_reference.is_witness() => {


From the is_witness function, it looks like there are Committed, Constant, and Intermediate enums for PolynomialType. If the folded column has been "optimized away" as you mentioned, will it become Intermediate and thus filtered away?

qwang98 · 2025-02-03T10:39:30Z

executor/src/witgen/bus_accumulator/mod.rs

    pub fn generate(&self) -> Vec<(String, Vec<T>)> {
-        let accumulators = self
+        let mut columns = self
            .bus_interactions
            .par_iter()
-            .flat_map(|bus_interaction| self.interaction_columns(bus_interaction))
-            .collect::<Vec<_>>();
+            .flat_map(|bus_interaction| {
+                let (folded, acc) = self.interaction_columns(bus_interaction);
+                collect_folded_columns(bus_interaction, folded)
+                    .chain(collect_acc_columns(bus_interaction, acc))
+                    .collect::<Vec<_>>()
+            })
+            .collect::<BTreeMap<_, _>>();

        self.pil
            .committed_polys_in_source_order()
            .filter(|(symbol, _)| symbol.stage == Some(1))
-            .flat_map(|(symbol, _)| symbol.array_elements().map(|(name, _)| name))
-            .zip_eq(accumulators)
+            .flat_map(|(symbol, _)| symbol.array_elements())
+            .map(|(name, poly_id)| (name, columns.remove(&poly_id).unwrap()))
            .collect()
    }


Looks like the key of this PR is on this function. We first generate the witness values which filters out columns that are already optimized away. Then, we find the intersection between these witness columns and stage 1 columns. I just wonder why we need this "stage 1" step. Are all accumulator and folded columns "stage 1" in our protocol?

Yeah, this code makes the assumption that (1) the folded and accumulator columns are in stage 1 (they couldn't be in stage 0, as they depend on a challenge, and we never use higher stages) and (2) that all stage-1 columns are either folded or acc columns (otherwise the remove(...).unwrap() would fail). Actually, let me improve the panic messages in both cases.

qwang98 · 2025-02-03T10:45:21Z

I think you mentioned it in some other PR, but multiple bus interactions can use the same accumulator. Is it already supported in this PR? It looks like interaction_columns is invoked for each bus interaction, but accumulator_columns are algebraic references in PhantomBusInteractionIdentity, so does it mean that the reference supports using the same accumulator columns for different bus interactions, but the accumulator columns are still calculated in the executor each time it appears in a bus interaction?

qwang98

Left some comments but LGTM :)

georgwiese · 2025-02-03T10:54:49Z

I think you mentioned it in some other PR, but multiple bus interactions can use the same accumulator. Is it already supported in this PR?

No, but that's the plan for the next step ;) The idea is that you can mention the same accumulator columns in multiple bus interactions, and they just get added. Right now, this would fail, because we're collecting all (PolyID, Vec<T>) into a map, which would only keep one of the keys.

Depends on #2427 (because otherwise a referenced column might be removed by the optimizer). This PR is completely analogous to #2412: We add a list of expressions that evaluate to the folded payload to `PhantomBusInteraction`. This lets us easily find the referenced columns in the manual bus witgen, see #2428.

The base branch was changed.

qwang98 · 2025-02-04T06:40:52Z

here

Because I'm encountering a similar issue when testing with materialize_folded = false v.s. materialize_foldd = true in #2435, I wonder if the test here we are running which manually changes materialize_folded = false for gl might not be valid.

According to this chunk

powdr/std/protocols/bus.asm

Lines 52 to 56 in e77d380

    
                   // The case above triggers our hand-written witness generation, but on Bn254, we'd not be 
        
                   // on the extension field and use the automatic witness generation. 
        
                   // However, it does not work with a materialized folded payload. At the same time, Halo2 
        
                   // (the only prover that supports BN254) does not have a hard degree bound. So, we can 
        
                   // in-line the expression here.

, setting materialize_folded = false means hand written witgen isn't triggered, but here we should be testing hand-written witgen? Am I missing something?

georgwiese · 2025-02-04T09:37:27Z

Rebased!

qwang98

lgtm :)

qwang98 · 2025-02-04T13:57:26Z

here

Because I'm encountering a similar issue when testing with materialize_folded = false v.s. materialize_foldd = true in #2435, I wonder if the test here we are running which manually changes materialize_folded = false for gl might not be valid.

According to this chunk

powdr/std/protocols/bus.asm

Lines 52 to 56 in e77d380

// The case above triggers our hand-written witness generation, but on Bn254, we'd not be

// on the extension field and use the automatic witness generation.

// However, it does not work with a materialized folded payload. At the same time, Halo2

// (the only prover that supports BN254) does not have a hard degree bound. So, we can

// in-line the expression here.

, setting materialize_folded = false means hand written witgen isn't triggered, but here we should be testing hand-written witgen? Am I missing something?

After your comments to my other PR, this comment is partially stale, but there's still the question on what this comment means:

powdr/std/protocols/bus.asm

Lines 52 to 56 in e77d380

    
                   // The case above triggers our hand-written witness generation, but on Bn254, we'd not be 
        
                   // on the extension field and use the automatic witness generation. 
        
                   // However, it does not work with a materialized folded payload. At the same time, Halo2 
        
                   // (the only prover that supports BN254) does not have a hard degree bound. So, we can 
        
                   // in-line the expression here.

.

Does changing materialize_folded = false disable the manual witgen and therefore doesn't really test the executor code in this PR?

georgwiese · 2025-02-04T18:29:48Z

Manual witgen (which only exists for performance reasons, BTW) currently runs on all fields except BN254 that use bus interactions:

powdr/executor/src/witgen/mod.rs

Lines 112 to 137 in 2215b9e

    
           let has_phantom_bus_sends = pil 
        
               .identities 
        
               .iter() 
        
               .any(|identity| matches!(identity, AnalyzedIdentity::PhantomBusInteraction(_))); 
        
           let supports_field = match T::known_field().unwrap() { 
        
               KnownField::GoldilocksField 
        
               | KnownField::BabyBearField 
        
               | KnownField::KoalaBearField 
        
               | KnownField::Mersenne31Field => true, 
        
               KnownField::Bn254Field => false, 
        
           }; 
        
           if has_phantom_bus_sends && supports_field { 
        
               log::debug!("Using hand-written bus witgen."); 
        
               assert_eq!(stage, 1); 
        
               let bus_columns = generate_bus_accumulator_columns( 
        
                   pil, 
        
                   current_witness, 
        
                   &self.fixed_col_values, 
        
                   challenges, 
        
               ); 
        
               current_witness.iter().cloned().chain(bus_columns).collect() 
        
           } else { 
        
               log::debug!("Using automatic stage-1 witgen.");

Hopefully, with automatic witgen becoming faster and the number of bus interactions becoming fewer, we can remove this path again entirely soon :) It also shouldn't be too complicated to make it work for BN254 as well if needed.

But to answer your question, if I set materialize_folded = false for Goldilocks, it still should run with the manual witgen.

) Depends on #2428 and implements the second bullet point from #2337 (comment). **Test Coverage** Not sure why CI isn't running but currently `static_bus_multi.asm` passes test with `cargo run pil test_data/asm/static_bus_multi.asm --force --linker-mode bus --prove-with mock --field bn254` but NOT if `--field gl` with all else equal. See bottom for error from `--field gl`, which are obviously field extension constraints not passing for `folded` columns. **Questions** 1. Probably the biggest problem is with the degree-3 bound of Plonky3. The non batched version already has a degree of 3 (https://github.com/powdr-labs/powdr/blob/main/std/protocols/bus.asm#L86-L89). Therefore, this batched version has a degree of 4 and therefore doesn't work with Plonky3. The degree bound issue is also mentioned in #2337. 2. This comment (https://github.com/powdr-labs/powdr/blob/main/std/protocols/bus.asm#L52-L56) seems to say that `--field bn254` uses auto witgen and therefore manual witgen (which I'm trying to test here) isn't working. 3. According to this chunk (https://github.com/powdr-labs/powdr/blob/main/std/protocols/bus.asm#L60-L69) Does `--field bn254` correspond to no constraints for line 65 or whatsoever? With `--field bn254`, it seems that the accumulator adds up the folded columns but there's no constraint that the folded columns are correctly calculated from payload, id, and challenges. Am I missing something? (I think this might also explain why my `--field bn254` test has no constraint errors for `folded` columns, because they don't exist?) 4. I checked the following constraint errors against our field extension APIs and seems that `mul_ext`, `finger_print_inter_with_id`, `add_ext`, and `constrain_eq_ext` etc. are applied correctly. Any insights on whether `mock` works with `gl` and/or field extensions to start with? I still have a last resort of simply hand computing the values from the error below to see why it's not working with field extension... ``` ➜ powdr git:(bus-multi-interaction) ✗ cargo run pil test_data/asm/static_bus_multi.asm --force --linker-mode bus --prove-with mock --field gl [00:00:05 (ETA: 00:00:00)] ████████████████████ 100% - Starting... Witness generation took 5.538085s Writing ./commits.bin. Backend setup for mock... Setup took 0.0644265s Generating later-stage witnesses took 0.00s Machine main has 64 errors Error: Identity fails on row 0: main::folded_0 = std::prelude::challenge(0, 5) - (123 + (std::prelude::challenge(0, 1) * main::intermediate_fingerprint_0_2 + 11 * std::prelude::challenge(0, 2) * main::intermediate_fingerprint_1_2)); main::intermediate_fingerprint_0_2 = 16683533738167355631 main::intermediate_fingerprint_1_2 = 8619433688316392780 main::folded_0 = 14379784368020248175 std::prelude::challenge(0, 1) = 2206609067086327257 std::prelude::challenge(0, 2) = 11876854719037224982 std::prelude::challenge(0, 5) = 15794382300316794652 Error: Identity fails on row 0: main::folded_0_1 = std::prelude::challenge(0, 6) - (std::prelude::challenge(0, 2) * main::intermediate_fingerprint_0_2 + std::prelude::challenge(0, 1) * main::intermediate_fingerprint_1_2); main::intermediate_fingerprint_0_2 = 16683533738167355631 main::intermediate_fingerprint_1_2 = 8619433688316392780 main::folded_0_1 = 3590326197943317962 std::prelude::challenge(0, 1) = 2206609067086327257 std::prelude::challenge(0, 2) = 11876854719037224982 std::prelude::challenge(0, 6) = 18147521187885925800 Error: Identity fails on row 0: main::folded_1 = std::prelude::challenge(0, 7) - (456 + (std::prelude::challenge(0, 3) * main::intermediate_fingerprint_0_5 + 11 * std::prelude::challenge(0, 4) * main::intermediate_fingerprint_1_5)); main::intermediate_fingerprint_0_5 = 9393166848595961660 main::intermediate_fingerprint_1_5 = 14353807143801496692 main::folded_1 = 4794846700896775308 std::prelude::challenge(0, 3) = 18270091135093349626 std::prelude::challenge(0, 4) = 6185506036438099345 std::prelude::challenge(0, 7) = 7364705619221056123 Error: Identity fails on row 0: main::folded_1_1 = std::prelude::challenge(0, 8) - (std::prelude::challenge(0, 4) * main::intermediate_fingerprint_0_5 + std::prelude::challenge(0, 3) * main::intermediate_fingerprint_1_5); main::intermediate_fingerprint_0_5 = 9393166848595961660 main::intermediate_fingerprint_1_5 = 14353807143801496692 main::folded_1_1 = 16858051030421639712 std::prelude::challenge(0, 3) = 18270091135093349626 std::prelude::challenge(0, 4) = 6185506036438099345 std::prelude::challenge(0, 8) = 2404222719611925354 Error: Identity fails on row 0: main::folded_0_2 = std::prelude::challenge(0, 5) - (456 + (std::prelude::challenge(0, 1) * main::intermediate_fingerprint_0_8 + 11 * std::prelude::challenge(0, 2) * main::intermediate_fingerprint_1_8)); main::intermediate_fingerprint_0_8 = 16683533738167355631 main::intermediate_fingerprint_1_8 = 8619433688316392780 main::folded_0_2 = 14379784368020247842 std::prelude::challenge(0, 1) = 2206609067086327257 std::prelude::challenge(0, 2) = 11876854719037224982 std::prelude::challenge(0, 5) = 15794382300316794652 ... and 59 more errors ```

This was referenced Feb 3, 2025

Refactor manual bus witgen #2420

Closed

Add folded payload columns to PhantomBusInteraction #2426

Merged

georgwiese marked this pull request as ready for review February 3, 2025 01:18

leonardoalt previously approved these changes Feb 3, 2025

View reviewed changes

qwang98 reviewed Feb 3, 2025

View reviewed changes

qwang98 previously approved these changes Feb 3, 2025

View reviewed changes

georgwiese force-pushed the bus-interaction-add-folded branch from cb0ebfb to 5e0443f Compare February 3, 2025 11:26

Base automatically changed from bus-interaction-add-folded to main February 4, 2025 02:28

qwang98 mentioned this pull request Feb 4, 2025

Batch two bus interactions to the same set of accumulator columns #2435

Merged

georgwiese added 3 commits February 4, 2025 10:36

Return folded and acc seperately

f55271a

Collect folded and acc columns

dd5bd50

Improve panic messages

c13ccbc

georgwiese force-pushed the refactor-bus-witgen2 branch from 559a7fb to c13ccbc Compare February 4, 2025 09:36

georgwiese requested a review from qwang98 February 4, 2025 12:28

qwang98 approved these changes Feb 4, 2025

View reviewed changes

georgwiese added this pull request to the merge queue Feb 4, 2025

Merged via the queue into main with commit 718f802 Feb 4, 2025
16 checks passed

georgwiese deleted the refactor-bus-witgen2 branch February 4, 2025 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manual Bus Witgen: Output folded columns only if they exist #2428

Manual Bus Witgen: Output folded columns only if they exist #2428

georgwiese commented Feb 3, 2025 •

edited

Loading

qwang98 Feb 3, 2025 •

edited

Loading

georgwiese Feb 3, 2025

qwang98 Feb 3, 2025

georgwiese Feb 3, 2025

qwang98 commented Feb 3, 2025

qwang98 left a comment

georgwiese commented Feb 3, 2025

qwang98 commented Feb 4, 2025 •

edited

Loading

georgwiese commented Feb 4, 2025

qwang98 left a comment

qwang98 commented Feb 4, 2025

georgwiese commented Feb 4, 2025

Manual Bus Witgen: Output folded columns only if they exist #2428

Manual Bus Witgen: Output folded columns only if they exist #2428

Conversation

georgwiese commented Feb 3, 2025 • edited Loading

qwang98 Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

georgwiese Feb 3, 2025

Choose a reason for hiding this comment

qwang98 Feb 3, 2025

Choose a reason for hiding this comment

georgwiese Feb 3, 2025

Choose a reason for hiding this comment

qwang98 commented Feb 3, 2025

qwang98 left a comment

Choose a reason for hiding this comment

georgwiese commented Feb 3, 2025

qwang98 commented Feb 4, 2025 • edited Loading

georgwiese commented Feb 4, 2025

qwang98 left a comment

Choose a reason for hiding this comment

qwang98 commented Feb 4, 2025

georgwiese commented Feb 4, 2025

georgwiese commented Feb 3, 2025 •

edited

Loading

qwang98 Feb 3, 2025 •

edited

Loading

qwang98 commented Feb 4, 2025 •

edited

Loading