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

CompositeBackend: Create proofs for each machine (working with Halo2) #1470

Merged
merged 19 commits into from
Jul 3, 2024

Conversation

georgwiese
Copy link
Collaborator

@georgwiese georgwiese commented Jun 26, 2024

Another step towards VadCoP (#1495)

With this PR, the CompositeBackend splits the given PIL into multiple machines and creates a proof for each machine.

The rough algorithm is as follows:

  1. The PIL is split into namespaces
  2. Any lookups or permutations that reference multiple namespaces are removed.
  3. Any other constraints that reference multiple namespaces lead to the two namespaces being merged.

This is an example:

$ RUST_LOG=debug cargo run pil test_data/asm/block_to_block.asm -o output -f --prove-with halo2-composite --field bn254
...
Skipping connecting identity: main.instr_add { main.x, main.y, main.z } in 1 { main_arith.x, main_arith.y, main_arith.z };
== Proving machine: main
PIL:
namespace main(8);
    col fixed x(i) { i / 4 };
    col fixed y(i) { i / 4 + 1 };
    col witness z;
    col witness res;
    col fixed latch = [0, 0, 0, 1]*;
    main.res' = (1 - main.latch) * (main.res + main.z);
    (1 - main.instr_add) * (main.x + main.y - main.z) = 0;
    col fixed instr_add = [0, 1]*;

Starting proof generation...
Generating PK for snark...
Generating proof...
Time taken: 149.459458ms
Proof generation done.
== Proving machine: main_arith
PIL:
namespace main_arith(8);
    col witness x;
    col witness y;
    col witness z;
    main_arith.z = main_arith.x + main_arith.y;

Starting proof generation...
Generating PK for snark...
Generating proof...
Time taken: 150.752125ms
Proof generation done.
Writing output/block_to_block_proof.bin.

Seems seems to work across the entire codebase and allows us to create Halo2 proofs by machine!

For STARK backends, we typically expect that IDs (e.g. Polynomial IDs, constraint IDs, ...) are re-assigned, which is not yet happening in this implementation. As mentioned in the comment, the easiest way to fix that would be to fix #1488 and re-parse the PIL file.

@georgwiese georgwiese changed the title Add split_pil() function [WIP] CompositeBackend: Create proofs for each machine Jun 26, 2024
@georgwiese georgwiese force-pushed the composite-wrapper3 branch 2 times, most recently from c58c848 to 352696b Compare June 27, 2024 07:33
Base automatically changed from composite-wrapper2 to main June 27, 2024 10:11
@georgwiese georgwiese force-pushed the composite-wrapper3 branch 2 times, most recently from b502273 to 86c2ee7 Compare June 28, 2024 07:21
@georgwiese georgwiese changed the base branch from main to backend-use-arc June 28, 2024 07:44
github-merge-queue bot pushed a commit that referenced this pull request Jun 28, 2024
Pulled out of #1470.

Receiving references, the `CompositeBackend` has no chance of changing
the PIL or fixed columns before passing it to the wrapped backend. Using
`Arc` fixed it, and is quite compatible with what is kept by `Pipeline`
already.
Base automatically changed from backend-use-arc to main June 28, 2024 08:40
Comment on lines -77 to -83
// Also test composite backend:
pipeline
.clone()
.with_backend(powdr_backend::BackendType::EStarkStarkyComposite, None)
.compute_proof()
.unwrap();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed for now, until the re-parsing is happening.

@georgwiese georgwiese changed the title [WIP] CompositeBackend: Create proofs for each machine CompositeBackend: Create proofs for each machine (working with Halo2) Jun 28, 2024
@georgwiese georgwiese marked this pull request as ready for review June 28, 2024 16:54
@Schaeff Schaeff self-requested a review July 1, 2024 11:54
github-merge-queue bot pushed a commit that referenced this pull request Jul 2, 2024
Includes #1511, but also removes the declarations of the challenges in
the `permutation` and `lookup` modules.

With these changes, we never declare a column or challenge outside a
machine namespace. This greatly simplifies #1470.

---------

Co-authored-by: schaeff <thibaut@schaeff.fr>
Co-authored-by: chriseth <chris@ethereum.org>
Copy link
Member

@leonardoalt leonardoalt left a comment

Choose a reason for hiding this comment

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

Awesome! Some small comments that don't necessarily need to be fixed. Can this PR be merged already or do we need to wait?

backend/src/composite/mod.rs Outdated Show resolved Hide resolved
backend/src/composite/mod.rs Outdated Show resolved Hide resolved
backend/src/composite/split.rs Outdated Show resolved Hide resolved
fn get_namespace(name: &str) -> String {
let mut namespace = AbsoluteSymbolPath::default().join(SymbolPath::from_str(name).unwrap());
namespace.pop().unwrap();
namespace.relative_to(&Default::default()).to_string()
Copy link
Member

Choose a reason for hiding this comment

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

Is this always correct? I don't have a counterexample, but wondering if there are cases that would break this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so! The code is inspired by our implementation of Display:

let name = namespace.pop().unwrap();
if namespace != current_namespace {
current_namespace = namespace;
writeln!(
f,
"namespace {}({degree});",
current_namespace.relative_to(&Default::default())
)?;
};

pil: Analyzed<F>,
statements: Vec<StatementIdentifier>,
) -> Option<Analyzed<F>> {
// TODO: After #1488 is fixed, we can implement this like so:
Copy link
Member

Choose a reason for hiding this comment

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

Is this PR going to wait for that, or should it be merged before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be merged before!

backend/src/composite/split.rs Show resolved Hide resolved
@georgwiese
Copy link
Collaborator Author

Yeah, I think it can be merged!

leonardoalt
leonardoalt previously approved these changes Jul 3, 2024
backend/src/composite/merged_machines.rs Outdated Show resolved Hide resolved
backend/src/composite/mod.rs Outdated Show resolved Hide resolved
backend/src/composite/mod.rs Outdated Show resolved Hide resolved
backend/src/composite/split.rs Outdated Show resolved Hide resolved
executor/src/witgen/mod.rs Outdated Show resolved Hide resolved
backend/src/composite/split.rs Outdated Show resolved Hide resolved
backend/src/composite/split.rs Outdated Show resolved Hide resolved
backend/src/composite/split.rs Outdated Show resolved Hide resolved
backend/src/composite/split.rs Show resolved Hide resolved
backend/src/composite/mod.rs Show resolved Hide resolved
@georgwiese georgwiese enabled auto-merge July 3, 2024 13:49
@georgwiese georgwiese added this pull request to the merge queue Jul 3, 2024
Merged via the queue into main with commit c26ca45 Jul 3, 2024
6 checks passed
@georgwiese georgwiese deleted the composite-wrapper3 branch July 3, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-parsing optimized PIL files does not always work
3 participants