Conversation
adr1anh
left a comment
There was a problem hiding this comment.
Had a quick sneak peek, but this is great already! Left two small comments about the codegen.
adr1anh
left a comment
There was a problem hiding this comment.
Looks really good! Just left some minor comments which can be addressed in a follow-up PR.
| pub trait AirBuilderWithPeriodicColumns: AirBuilder { | ||
| type PeriodicColumnsVar: Field + Into<Self::Expr>; | ||
|
|
||
| fn periodic_columns(&self) -> Vec<Self::PeriodicColumnsVar> { |
There was a problem hiding this comment.
We should also return a slice here which we can cast to an array.
There was a problem hiding this comment.
I think this is more complicated issue though, since I see below that we're using this to make testing easier.
| { | ||
| type PeriodicColumnsVar = F; | ||
|
|
||
| fn periodic_columns(&self) -> Vec<Self::PeriodicColumnsVar> { |
There was a problem hiding this comment.
While maybe not for this PR, I think we can represent periodic_columns in the DebugConstraintBuilderWithPeriodicColumns struct a bit differently
- Pad all periodic columns to the maximum length by periodic repetition
- Transpose the columns so that we have a vector of rows.
- We just return
&self.periodic_columns[self.row_index % col.len()]
There was a problem hiding this comment.
Hi! I've created an issue to do this in a followup: #492
Al-Kindi-0
left a comment
There was a problem hiding this comment.
Looks great!
Support and testing for periodic columns and buses will come in a follow up PR once we agree on how our version of P3 backend will look like.
huitseeker
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward. The Plonky3 backend work looks like the right direction.
One architectural concern before this is something we would want to land on next: we should avoid baking MassaLabs forks into the long-term air-script dependency story. If this backend is the forward path, I think we should aim for it to integrate against the official 0xMiden stack rather than rely on forks.
Concretely, I think there are two follow-ups we should plan for:
- Move the Plonky3 integration off fork-specific VM/crypto dependencies and onto the official Miden stack.
- Isolate the Plonky3 backend dependency surface from default
air-scriptconsumers.
Right now these forked dependencies are unconditional normal dependencies of theair-scriptcrate. It would be better to depend on miden-crypto Plonky3 re-exports into a separate crate/module so the core language/compiler stays clean.
That would give us a cleaner path from “working branch” to “something we can comfortably maintain upstream.”
/cc @Al-Kindi-0
| format!("AB::ExprEF::from(periodic_values[{index}].clone().into())") | ||
| }, | ||
| ElemType::ExtFieldElem => { | ||
| format!("AB::EF::from(periodic_values[{index}].clone())") |
There was a problem hiding this comment.
This branch uses ElemType::ExtFieldElem both inside eval() and inside the generated bus helpers. In eval() the AB::EF name exists, but in buses_initial_values() and buses_transitions() there is no AB in scope, so any bus expression that references a periodic column will generate Rust that does not compile. I think this arm needs to emit EF::from(periodic_values[idx].clone()) instead.
There was a problem hiding this comment.
Indeed current tests don't use this codepath so I did not catch this, addressed in d363738
| } | ||
| let col_matrix = ColMatrix::new(felt_columns_vec); | ||
| let last_program_row = main.height().into(); | ||
| let main_trace = MainTrace::new(col_matrix, last_program_row); |
There was a problem hiding this comment.
MainTrace::new expects the last valid program row index, but main.height() is the row count, so helpers that later read main_trace.last_program_row() can step one past the trace. In the MassaLabs fork, BlockHashTableRow::table_init() reads decoder_hasher_state_first_half(main_trace.last_program_row()), so this can go out of bounds on padded traces. I think this needs num_rows - 1 at minimum, or the actual halt row if that is available.
There was a problem hiding this comment.
Nice catch! I've addressed this in a6f53cb in the same way I saw it handled in miden-vm
Hi! Indeed, we should be able to improve the dependencies (both avoid forks and depend on released versions instead of git revisions). Note that currently the dependency to a massalabs maintained miden-vm is only used for aux trace generation, but the current implementation for
|
Describe your changes
This PR aims to close #450.
It became the tracking branch for Plonky3 Codegen related features.
Tasklist:
AirBuilderWithPeriodicColumnsandBaseAirWithPeriodicColumnsDebugConstraintBuilderWithPeriodicColumnscheck_constraints_with_periodic_columnsto check the constraints are valid for a given traceimplement the traits for upstream structs needed formoved to a followup: feat: prove/verify and sync plonky3 #523proveandverify