-
Notifications
You must be signed in to change notification settings - Fork 36
Target 0xMiden Plonky3 repo and use AirScriptAir and AirScriptBuilder traits #508
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
Conversation
|
@adr1anh here I assumed that the randomness was given by: Value::RandomValue(idx) => {
if *idx == 0 {
format!("beta.into()")
} else {
format!("alpha_challenges[{}].into()", idx - 1)
}
},But maybe it should only be alpha_challenges for the constraints, beta being used internally in the prover/verifier? |
adr1anh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great just left some small comments
Al-Kindi-0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Would be great to think of a way to compare the outputs of Winterfell against P3. Haven't spent time thinking about this so not sure how realistic and straightforward this is.
|
From the P3 side, we should be able to define an air builder which evaluates the constraints at a set of random points (there's already something super close), but not sure how we can do with with Winterfell |
|
For Winterfell, we might be able to use the verifier constraints evaluator (or a small modification of it) in order to evaluate the constraints on an OOD evaluation frame. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for the 0xMiden Plonky3 repository by implementing AirScriptAir and AirScriptBuilder traits for improved code generation. The changes add support for auxiliary trace constraints in Plonky3, refactor the code generation to use new traits, and reorganize test structure to use a centralized test utilities module.
Key changes:
- Introduced
AirScriptAirandAirScriptBuildertraits for Plonky3 code generation - Added auxiliary trace constraint generation support
- Reorganized test structure with new
test_utilsmodule replacing scattered helper files
Reviewed Changes
Copilot reviewed 128 out of 208 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/generate_all_e2e_tests.sh | New shell script for generating all end-to-end tests for both Winterfell and Plonky3 backends |
| codegen/plonky3/src/imports.rs | Updated imports to use new AirScript traits instead of custom helper traits |
| codegen/plonky3/src/air/mod.rs | Refactored AIR generation to use new trait structure with separate constants, main AIR implementation, and AirScriptAir trait |
| codegen/plonky3/src/air/integrity_constraints.rs | Added support for auxiliary integrity constraints alongside main constraints |
| codegen/plonky3/src/air/graph.rs | Enhanced constraint string generation to support extension field types and determine field type requirements |
| codegen/plonky3/src/air/boundary_constraints.rs | Added auxiliary boundary constraint generation alongside main boundary constraints |
| air/src/passes/expand_buses.rs | Changed constraint domain from EveryRow to EveryFrame(2) for bus expansion |
| air-script/tests/* (multiple files) | Removed old test files using deprecated helper structure |
| air-script/src/tests/* (multiple files) | Added new test files using AirScriptAir trait and reorganized into src/tests directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Value::Constant(0) => match elem_type { | ||
| ElemType::Base => format!("AB::Expr::ZERO"), | ||
| ElemType::Ext => format!("AB::ExprEF::ZERO"), | ||
| }, | ||
| Value::Constant(1) => match elem_type { | ||
| ElemType::Base => format!("AB::Expr::ONE"), | ||
| ElemType::Ext => format!("AB::ExprEF::ONE"), | ||
| }, |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repeated match statements for Value::Constant(0) and Value::Constant(1) could be consolidated using a helper function to reduce duplication and improve maintainability.
|
|
||
| p.insert(2, b) when 1 - s1; | ||
| p.remove(2, a) when 1 - s2; | ||
|
|
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Lines 37-38 insert the same value a at the same position (3) with different conditions (s3 and s4). Consider adding a comment explaining why this duplication is intentional or if it should be different values/positions.
| // Intentionally insert the same value `a` at position 3 under both `s3` and `s4` conditions. | |
| // This ensures that the bus `q` receives `a` regardless of which condition is met. |
Hi, I've tested a few things out, and was able to adapt Winterfell's It seems to work with a few caveats (for now, the code is specific to the air and a given row, it's a bit messy, and it's on a simple case with no public inputs, no periodic columns, no aux trace. I think I'll put it in another PR if I find a way to clean it up. |
Al-Kindi-0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great stuff!
I haven't checked every line of the codegen-ed code but I am assuming it should not be different from the Winterfell one. Also, I believe there is some work going to test consistency between the P3 and Winterfell backends
| fn $test_name() { | ||
| type Val = Goldilocks; | ||
| type Challenge = BinomialExtensionField<Val, 3>; | ||
| type Challenge = BinomialExtensionField<Val, 5>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it is probably enough to use just degree 2 here making things lighter, though it shouldn't matter really
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 5f2f351
| /// Number of beta challenges used in the AIR. | ||
| fn num_beta_challenges(&self) -> usize { | ||
| 0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would add a comment, and change the name of this method though not sure how easy it is, explaining that the number of beta challenge powers is equal to the width of the largest/widest message sent through all buses in an AIR instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to max_beta_challenge_power and added a comment on 871058c
| { | ||
| /// EF evaluations of periodic columns at the AIR’s random point (z). Order defined by | ||
| /// AirScript. | ||
| fn periodic_evals(&self) -> &[<Self as ExtensionBuilder>::VarEF]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: shouldn't this take z as input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment does seem to insinuate that, but I dont think periodic evals depend on the random point, they're always 1 or 0 for the same row, no matter the input.
Im leaving it as is for now, Ill sync with @Leo-Besancon about this to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caller which creates the Builder will have already evaluated the columns at z, to avoid doing it at every row.
| /// AirScript. | ||
| fn periodic_evals(&self) -> &[<Self as ExtensionBuilder>::VarEF]; | ||
|
|
||
| /// Global challenges in EF. (We can provide defaults; details not important here.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the parenthesis and what is between them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| fn beta(&self) -> <Self as ExtensionBuilder>::VarEF; | ||
| fn beta_powers(&self) -> &[<Self as ExtensionBuilder>::VarEF]; | ||
|
|
||
| /// Aux bus boundary values: EF finals, one per aux/bus column, carried in the proof. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would write this as:
Auxiliary buses boundary values, one per bus column, attached to the proof.or something better
| /// Used in conjunction with [`check_constraints`] to simulate | ||
| /// an execution trace and verify that the AIR logic enforces all constraints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: alignment of the first line
| let logup_current = EF::from(aux_current[1].clone()); | ||
| let logup_next = (a * logup_current + b + c - e) * d.inverse(); | ||
|
|
||
| // Dummy implementation for illustration purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Should we create a TODO to remove this dummy implementation with something more solid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be removed anyway once we have full support of auxiliary traces, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a TODO
I think this will be removed anyway once we have full support of auxiliary traces, right?
I think so but not certain, Ill sync with @Leo-Besancon on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! @Al-Kindi-0 This was used to build the aux trace for the bus complex air (that only worked with this specific air), but in #515 it has already been replaced by a dynamic building of the aux trace that works with any Air (in particular the buses_transitions function added in the Codegen here: https://github.com/0xMiden/air-script/pull/515/files#diff-ed5104fe9ad8f138c0530924ab07f8c45e2ed62332f7475757ec7bf657d8dd77R227-R262)
This way, we now have two ways to build the Aux trace, either with the codegen, or with the manual implementations given by the MidenVM repo.
Just so it is clear, the Plonky3 PRs are:
- Add plonky3 backend #461 that outlines the base for the main trace codegen
- Target 0xMiden Plonky3 repo and use AirScriptAir and AirScriptBuilder traits #508 (this one), based on Add plonky3 backend #461, that adds manual aux trace handling and periodic columns
- Use Plonky3 super trait #515, based on Target 0xMiden Plonky3 repo and use AirScriptAir and AirScriptBuilder traits #508, that uses the super traits and adds automatic aux trace building.
It would be great to merge these PRs in a Plonky3 codegen tracking branch once they are ready :)
| let height = main.height(); | ||
|
|
||
| let aux_bus_boundary_values: Vec<_> = (0..air.aux_width()).map(|_| EF::GENERATOR).collect(); | ||
| let alpha_f: Vec<F> = (0..5).map(|i| F::from_u64(123456789 * i)).collect(); // Dummy alpha in F |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can't we just use EF directly and hence avoid leaking the degree extension (i.e., 5)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 1d14475
| let aux_bus_boundary_values: Vec<_> = (0..air.aux_width()).map(|_| EF::GENERATOR).collect(); | ||
| let alpha_f: Vec<F> = (0..5).map(|i| F::from_u64(123456789 * i)).collect(); // Dummy alpha in F | ||
| let alpha = EF::from_basis_coefficients_iter(alpha_f.iter().cloned()).unwrap(); | ||
| let beta = EF::from_u64(987654321); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this about #508 (comment) ?
| use std::marker::PhantomData; | ||
|
|
||
| use p3_challenger::{HashChallenger, SerializingChallenger64}; | ||
| use p3_circle::CirclePcs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has to do with the macro generate_air_plonky3_test_with_airscript_traits that seems to use it.
Not sure why we're using this one tho, Ill see with Leo so we can use the proper imports.
In any case, the macro should use full path names so we do not need to import those dependencies explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe your changes
The PR introduces:
AirScriptAirandAirScriptBuildertraits proposed in AirScript integration Plonky3#5 (for now, in the AirScript repo, but will latter be imported from 0xMiden/Plonky3 once our testing is done)Checklist before requesting a review