Skip to content

feat: Expose AIR column names#2771

Merged
georgwiese merged 23 commits into
openvm-org:develop-v2.0.0-rc.1from
powdr-labs:columns-air
May 13, 2026
Merged

feat: Expose AIR column names#2771
georgwiese merged 23 commits into
openvm-org:develop-v2.0.0-rc.1from
powdr-labs:columns-air

Conversation

@georgwiese
Copy link
Copy Markdown

This PR introduces the ColumnsAir trait and makes it required by all AIRs. Via this trait, AIRs can (optionally) provide a list of column names. At powdr, we use this to generate (somewhat) human-readable autoprecompiles for OpenVM (example).

Unfortunately, this touches all AIRs and therefore a lot of files, but the changes are small and now change to stark-backend are needed.

This change represents most of our diff to OpenVM.

Defines `ColumnsAir<F>` in `openvm-circuit-primitives` for column-name
introspection. Wraps it with stark-backend's `AnyAir` into
`AnyAirWithColumns` (and `AirRefWithColumns`), which `AirInventory` and
`SystemAirInventory::into_airs` now use; trait upcasting (Rust 1.86+)
handles the boundary back to `AirRef<SC>` at the few call sites that need
a stark-backend-shaped `AirRef`.

Implements `ColumnsAir` (and adds `StructReflection` derives, via the
crates.io `struct-reflection` crate) on every AIR and column struct in
the workspace. With the trait owned by OpenVM, stark-backend no longer
needs to define it; the dependency is repointed to the
`v2-powdr-beta.2-remove-columns-air` fork branch.

Collects the ColumnsAir portion of original commits 449db83, 9a25af6,
and 8a5d23a so the trait introduction can be cherry-picked upstream
cleanly.
Each side independently falls back to synthesized `adapter[i]` /
`core[i]` placeholders when its `columns()` returns None, then the
two are concatenated. The previous match on (adapter, core) had four
branches that did the same thing in a less direct way.

impl<F: Field> BaseAirWithPublicValues<F> for FieldExpr {}
impl<F: Field> PartitionedBaseAir<F> for FieldExpr {}
impl<F: Field> ColumnsAir<F> for FieldExpr {}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Note that we didn't bother implementing it for AIRs we don't build autopecompiles for. This keeps the default implementation, which returns None.

}

impl<const CHUNK: usize, F: Field> PartitionedBaseAir<F> for MemoryMerkleAir<CHUNK> {}
impl<const CHUNK: usize, F: Field> ColumnsAir<F> for MemoryMerkleAir<CHUNK> {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Note that it would be possible to reduce these types of changes using a macro, but it becomes a bit ugly, especially in the presence of const generics.

See this discussion: powdr-labs#60 (comment)

@georgwiese georgwiese marked this pull request as ready for review May 2, 2026 21:45
Comment thread crates/circuits/primitives/src/range_tuple/mod.rs Outdated
@georgwiese georgwiese changed the title Introduce ColumnsAir trait and implement it everywhere feat: Expose AIR column names May 6, 2026
georgwiese and others added 4 commits May 6, 2026 19:02
Replaces hand-written `impl ColumnsAir<F>` blocks that delegate to
`StructReflectionHelper::struct_reflection` with `#[derive(ColumnsAir)]`
plus `#[columns_via(ColsTy<F, ...>)]`. Net -143 lines across 38 sites.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Updates the derive macro to skip injecting `F: Field` when the struct
already has an `F` type generic, then converts ~75 hand-written empty
`impl ColumnsAir<F> for X {}` blocks to `#[derive(ColumnsAir)]`.

Skips:
- VmAirWrapper (custom body delegating to adapter/core)
- WhirFoldingAir (uses an `F` type alias rather than a generic)
- ExpBitsLenAir (`BaseAir<F>` impl requires `F: PrimeField32`,
  not implied by the macro's default `F: Field`)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds #[columns_via(...)] to the 6 #[derive(ColumnsAir)] sites whose
matching XxxCols struct already implements StructReflectionHelper
(directly or via #[derive(StructReflection)]). Also adds
#[derive(StructReflection)] to CanonicityAuxCols and
DeferralCallCoreCols so the deferral call core air can use it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The derive macro now errors at compile time if `#[columns_via(...)]`
is missing. Each AIR using the derive declares its column struct;
AIRs without a usable column struct keep a one-line manual impl.

Cascading `#[derive(StructReflection)]` was added to several PVs/cols
types (`MerkleTreeCols`, `VerifierBasePvs`, `VmPvs`, `DeferralPvs`,
`DeferralCircuitPvs`, `DeferralAggregationPvs`, `VkCommit`,
`VerifierDefPvs`, `DeferralCallAdapterCols`). Cols whose contents
include external types without reflection (e.g. wrappers around
`Poseidon2SubCols`) get a manual `StructReflectionHelper` returning
`None` so `columns_via` still applies uniformly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@georgwiese georgwiese marked this pull request as draft May 6, 2026 19:06
@georgwiese
Copy link
Copy Markdown
Author

I implemented the macro per powdr-labs#60 (comment), but Claude has been annoying to navigate on this large diff. I'll do a review pass myself and am marking the PR as draft until then.

@georgwiese georgwiese marked this pull request as ready for review May 6, 2026 21:30
@georgwiese
Copy link
Copy Markdown
Author

OK, should be clean now!

@jonathanpwang
Copy link
Copy Markdown
Contributor

Please mention in PR description / docs that default implementation is always safe, since this is just for labeling purposes.

Going to wait till @shuklaayush is back to review it, but overall I feel like the new macro makes it pretty clean

@jonathanpwang jonathanpwang requested a review from shuklaayush May 7, 2026 04:33
@socket-security
Copy link
Copy Markdown

socket-security Bot commented May 7, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedcargo/​struct-reflection@​0.1.010010093100100

View full report

@shuklaayush
Copy link
Copy Markdown
Collaborator

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 12, 2026

Claude finished @shuklaayush's task in 2m 34s —— View job


PR Review: ColumnsAir trait

  • Read core trait definition and derive macro
  • Review key implementation patterns across AIRs
  • Check integration with VM architecture (extensions, integration API)
  • Review consistency across all changed files
  • Post review feedback

Overall Assessment

This is a clean, well-structured PR. The ColumnsAir trait design is sound — the default None return makes it safe for all existing AIRs, and the derive macro with #[columns_via(...)] keeps the boilerplate minimal. The AnyAirWithColumns super-trait with trait upcasting is an elegant way to thread column introspection through the VM without touching the stark-backend API.

Positive aspects

  1. Safe default — The trait provides a default fn columns(&self) -> Option<Vec<String>> { None }, so AIRs that don't have a corresponding Cols struct can just write impl ColumnsAir for X {}.

  2. Derive macro is well done — The #[derive(ColumnsAir)] + #[columns_via(ColsTy<u8>)] pattern is ergonomic and catches missing attributes at compile time with a clear error message.

  3. Good comments — Each manual impl ColumnsAir for X {} includes a brief explanation of why columns aren't provided (e.g., external p3-poseidon2-air cols, dynamic FieldExpr layout, slice-borrowing ref structs). This makes future maintenance much easier.

  4. VmAirWrapper composition — The ColumnsAir impl for VmAirWrapper<A, M> correctly chains adapter + core columns, matching the BaseAir::width() composition. The short-circuit via ? on both is the right behavior: if either half lacks names, the whole thing returns None.

Issues and suggestions

1. Doc comment should mention the default impl is always safe (per @jonathanpwang's request)

The trait doc says column names are "for debugging purposes" but doesn't explicitly call out that the default implementation is always safe. Per Jonathan's request, this should be mentioned clearly — perhaps something like:

"The default implementation returns None and is always safe to use, as column names are purely informational and have no effect on proving or verification."

This helps downstream implementors feel confident using the default.

Fix this →

2. Allocation on every call

fn columns(&self) -> Option<Vec<String>> allocates a fresh Vec on every invocation. This is fine for debugging/tooling, but if anyone calls it in a hot path it could be surprising. Worth noting in the doc that this is intended for introspection, not performance-critical paths. Not a blocker.

3. struct-reflection dependency — new supply chain surface

The PR adds a new external crate struct-reflection = "0.1.0". Socket flagged it clean, but it's a 0.1.0 crate — worth pinning or vendoring if stability matters. The crate's purpose (generating field names from struct definitions) is well-scoped. Just calling it out for awareness.

4. Minor: AirRef import in sdk/keygen and continuations/tests

The .map(|a| a as AirRef<_>) coercion at call sites (crates/sdk/src/keygen/mod.rs:68, crates/continuations/src/tests/mod.rs:225) works via trait upcasting. This is clean and correct. One thing to note: trait upcasting is stable since Rust 1.86, and the toolchain is 1.91.1, so no concerns there.

5. Minor: unused StructReflection/StructReflectionHelper imports

Several files import StructReflection and StructReflectionHelper even though they only use them transitively through the #[derive(StructReflection)] macro (which generates the import itself). For example, in crates/vm/src/arch/execution.rs, StructReflection and StructReflectionHelper are imported but the file only adds StructReflection to derives — the derive macro should handle the trait import internally. This is minor and might be needed for some edge cases with extern crate self as, but worth double-checking if any are actually unused.

6. No runtime validation of columns().len() == width()

The doc comment says "if the result is Some(names), names.len() == air.width() should always be true" but this is not enforced. A debug assert in VmAirWrapper::columns() or in the ColumnsAir derive macro output could catch mismatches early. Not a blocker, but a nice-to-have for safety.

Fix this →

Summary

The PR is well-executed for its scope. The trait design, derive macro, and integration are all sound. The main actionable item is adding the safety note to the trait doc per Jonathan's review comment. Everything else is minor or informational.

Recommendation: Approve with the doc comment update.


Copy link
Copy Markdown
Collaborator

@shuklaayush shuklaayush left a comment

Choose a reason for hiding this comment

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

looks fine

@georgwiese georgwiese merged commit e11f786 into openvm-org:develop-v2.0.0-rc.1 May 13, 2026
67 of 75 checks passed
@georgwiese georgwiese deleted the columns-air branch May 13, 2026 09:44
shuklaayush added a commit that referenced this pull request May 13, 2026
…named files

Upstream #2771 (e11f786) introduced the ColumnsAir trait as required
for all AIRs and added StructReflection to corresponding Cols structs.
During the rebase, the -X theirs strategy dropped these additions for
files our rv64 branch had renamed (Rv32* -> Rv64*) or otherwise modified.

Re-applies the derive macros and #[columns_via(...)] attributes so the
workspace builds against develop-v2.0.0-rc.1.
shuklaayush added a commit that referenced this pull request May 14, 2026
…named files

Upstream #2771 (e11f786) introduced the ColumnsAir trait as required
for all AIRs and added StructReflection to corresponding Cols structs.
During the rebase, the -X theirs strategy dropped these additions for
files our rv64 branch had renamed (Rv32* -> Rv64*) or otherwise modified.

Re-applies the derive macros and #[columns_via(...)] attributes so the
workspace builds against develop-v2.0.0-rc.1.
shuklaayush added a commit that referenced this pull request May 15, 2026
…named files

Upstream #2771 (e11f786) introduced the ColumnsAir trait as required
for all AIRs and added StructReflection to corresponding Cols structs.
During the rebase, the -X theirs strategy dropped these additions for
files our rv64 branch had renamed (Rv32* -> Rv64*) or otherwise modified.

Re-applies the derive macros and #[columns_via(...)] attributes so the
workspace builds against develop-v2.0.0-rc.1.
jpw-axiom pushed a commit that referenced this pull request May 16, 2026
This PR introduces the `ColumnsAir` trait and makes it required by all
AIRs. Via this trait, AIRs can (optionally) provide a list of column
names. At powdr, we use this to generate (somewhat) human-readable
autoprecompiles for OpenVM
([example](https://github.com/powdr-labs/powdr/blob/main/openvm-riscv/tests/apc_snapshots/complex/rotate.txt)).

Unfortunately, this touches all AIRs and therefore a lot of files, but
the changes are small and now change to `stark-backend` are needed.

This change represents most of our [diff to
OpenVM](https://github.com/openvm-org/openvm/compare/v2.0.0-beta.2...powdr-labs:openvm:v2-powdr-beta.2-reorganized?expand=1).

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jpw-axiom pushed a commit that referenced this pull request May 18, 2026
This PR introduces the `ColumnsAir` trait and makes it required by all
AIRs. Via this trait, AIRs can (optionally) provide a list of column
names. At powdr, we use this to generate (somewhat) human-readable
autoprecompiles for OpenVM
([example](https://github.com/powdr-labs/powdr/blob/main/openvm-riscv/tests/apc_snapshots/complex/rotate.txt)).

Unfortunately, this touches all AIRs and therefore a lot of files, but
the changes are small and now change to `stark-backend` are needed.

This change represents most of our [diff to
OpenVM](https://github.com/openvm-org/openvm/compare/v2.0.0-beta.2...powdr-labs:openvm:v2-powdr-beta.2-reorganized?expand=1).

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

3 participants