Skip to content

fix: deferral output_commit should be a true hash#2574

Merged
stephenh-axiom-xyz merged 5 commits into
develop-v2.0.0-betafrom
fix/deferral-output-hash
Mar 18, 2026
Merged

fix: deferral output_commit should be a true hash#2574
stephenh-axiom-xyz merged 5 commits into
develop-v2.0.0-betafrom
fix/deferral-output-hash

Conversation

@stephenh-axiom-xyz
Copy link
Copy Markdown
Contributor

Resolves INT-6725, INT-6727, INT-6709, INT-6784, INT-6723, INT-6724

INT-6725: Output commit must be a true sponge hash

INT-6709: output_len is not bound in DEFER_CALL

INT-6784: Zero-length outputs bypass the output AIR

Output commit is now the hash_slice of [def_idx, output_len, 0, ...] andoutput_raw. The first row in DeferralOutputAir is for the [def_idx, output_len, 0, ...] (i.e. skips a write), and DeferralOutputCommitAir does the same.

INT-6727: DeferralOutputCommitAir doesn't handle initial def_idx state

Updated DeferralOutputCommitAir to constrain the def_idx and output_len present on the first row. The former is compared against a constant, while output_len is constrained to be consistent with the number of valid rows.

INT-6723: DeferralOutputAir cleanup and degree reduction

Replaced the redundant constraints with is_transition, which is used throughout the AIR now.

INT-6724: DeferralOutputCommitAir cleanup

Removed next_f and the final invalid row by calling permute on the next row.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

Comment thread extensions/deferral/circuit/src/poseidon2/bus.rs Outdated
Comment thread extensions/deferral/circuit/src/output/air.rs
Comment thread crates/sdk/src/prover/deferral/mod.rs Outdated
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.

some small nits. lgtm, i didn't find any glaring issues

Comment thread extensions/deferral/circuit/src/output/air.rs Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

unfortunate that we need to duplicate all this

Comment thread guest-libs/verify-stark/circuit/src/output/air.rs Outdated
Comment thread guest-libs/verify-stark/circuit/src/output/air.rs Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 18, 2026

Claude finished @stephenh-axiom-xyz's task in 2m 47s —— View job


Code Review: PR #2574 — Deferral Output Hash Fixes

  • Gather context and read changed files
  • Review deferral extension changes (output AIR, poseidon2, def_fn, call)
  • Review verify-stark circuit changes (output commit AIR/trace)
  • Review SDK/prover/continuations changes
  • Post final review

This PR correctly converts the deferral output commit from a naive compression to a proper sponge hash of [def_idx, output_len, 0, ...] || output_raw, addressing critical security issues INT-6725, INT-6709, INT-6784, INT-6727, INT-6723, and INT-6724.

Security Fixes Verified

  • INT-6725 (output commit must be true sponge hash): hash_output_raw in def_fn.rs:76-110 now constructs a proper sponge: initial state [def_idx, output_len, 0...], permute, absorb chunks, squeeze. Both DeferralOutputAir and DeferralOutputCommitAir mirror this construction correctly.
  • INT-6709 (output_len not bound in DEFER_CALL): output_len is now embedded in the initial sponge state (initial_state[1] = output_len) and cross-checked against section_idx * DIGEST_SIZE on the last row (output/air.rs:185-188).
  • INT-6784 (zero-length outputs bypass AIR): num_rows = output_len / DIGEST_SIZE + 1 ensures at least 1 row (the initial state row), so the AIR is always populated.
  • INT-6727 (def_idx initial state unconstrained): DeferralOutputCommitAir now constrains input_vals[0] == def_idx as a constant on the first row (output/air.rs:94).
  • INT-6723/INT-6724 (cleanup): is_transition = next.is_valid - next.is_first pattern is used consistently. The next_f / final invalid row in DeferralOutputCommitAir has been removed.

Sponge Consistency Check

I verified that trace generation in output/trace.rs matches the AIR constraints in output/air.rs:

  1. Initial state: [def_idx, output_len, 0..., 0...] — matches initial_sponge_input in trace (line 298-300) and AIR (line 167-169).
  2. First permutation: perm(initial_state, zeros) with is_compress=false → capacity stored — matches AIR where rhs = is_transition * local.poseidon2_res is zero on the first row (line 197).
  3. Subsequent absorptions: perm(chunk, previous_capacity) with is_compress=false → new capacity — matches AIR transition rows.
  4. Final squeeze: last permutation uses is_compress=true to return left half as commitment — matches next.is_last flag in bus lookup (line 199).

The same pattern is correctly replicated in DeferralOutputCommitAir (verify-stark), using next_capacity zeroed on first rows via is_transition * local.res_right (output/air.rs:150).

def_idx Threading

The new get_def_idx() method on DeferralCircuitProver (continuations/src/prover/deferral/mod.rs:15) is correctly threaded through:

  • DeferredVerifyCircuitProver returns self.prover.circuit.def_idx (verify-stark prover/mod.rs:269)
  • DeferralProver::new asserts def_circuit_prover.get_def_idx() == 0 (sdk/prover/deferral/mod.rs:61)
  • with_prover asserts def_circuit_prover.get_def_idx() == self.single_circuit_provers.len() (line 114-117)

This prevents cross-circuit forgery.

Issues Found

1. DeferralOutputAir wrap-around row: implicit minimum height requirement

At extensions/deferral/circuit/src/output/air.rs:197-200, the Poseidon2 bus lookup uses next row columns with next.is_valid as multiplicity. This requires the padded trace height ≥ 2. Since num_rows ≥ 1 and trace heights are powers of two, the minimum padded height is 2 (rounding up from 1). This is correct but implicit — a comment documenting the invariant would help future maintainers.

2. DeferralOutputCommitAir: row_idx - 1 wraps in field on first row

At guest-libs/verify-stark/circuit/src/output/air.rs:141, local.row_idx - AB::Expr::ONE evaluates to P - 1 (BabyBear) when row_idx = 0. This is safe because the multiplicity local.is_valid - local.is_first is 0 on the first row. A brief comment explaining why the field wrap is benign would be helpful.

3. DeferralOutputCommitAir: is_first can only be set on first row — not just "at most once"

At guest-libs/verify-stark/circuit/src/output/air.rs:81, when_transition().assert_zero(next.is_first) prevents is_first from appearing on any row other than row 0. Combined with when_first_row().assert_one(local.is_first) at line 80, this means is_first is exactly 1 on row 0 and 0 elsewhere. However, is_first is never constrained to be boolean — it's only constrained to be 1 on the first row and 0 on transitions. Since is_valid IS constrained boolean, and is_last = is_valid - is_transition is derived, this means is_first could theoretically be non-boolean on the first row... except it's forced to 1 there. On subsequent rows it's forced to 0 by the transition constraint. So this is actually fine — just noting that the boolean constraint on is_first is implicit rather than explicit.

4. Reviewer nits from @shuklaayush — some appear unaddressed

Based on the review comments:

  • is_compress at end of bus key (poseidon2/bus.rs): The lookup sends left || right || output || is_compress, and the AIR sends inputs || output_half || is_compress. The ordering in the lookup matches the AIR — is_compress is already at the end in both. This seems fine as-is.
  • Magic number 10 should be a constant (output/air.rs): I don't see a magic 10 in the current code — this may have already been addressed or was in a previous revision.
  • zip_eq in sdk/prover/deferral/mod.rs: At line 136, zip_eq is already used (self.single_circuit_provers.iter().zip_eq(inputs)). This nit appears addressed.
  • capacity naming (verify-stark output/air.rs): The variable is next_capacity at line 150. The author responded that it's the capacity for the next row, so the naming seems intentional.

5. DeferralPoseidon2Chip::records.clear() in generate_proving_ctx — single-use footgun

At extensions/deferral/circuit/src/poseidon2/trace.rs:139, self.records.clear() means a second call to generate_proving_ctx would produce an empty trace. This is fine for the current usage pattern but could be a footgun if proving context generation is ever retried.

Summary

No blocking issues found. The sponge construction is correctly implemented in both the deferral extension and verify-stark circuits, with proper binding of def_idx and output_len. The constraint logic is sound across all changed AIRs. The def_idx threading through the prover hierarchy is correct and prevents cross-circuit forgery. Only minor documentation nits identified.

@stephenh-axiom-xyz stephenh-axiom-xyz merged commit 732eb66 into develop-v2.0.0-beta Mar 18, 2026
32 of 34 checks passed
@stephenh-axiom-xyz stephenh-axiom-xyz deleted the fix/deferral-output-hash branch March 18, 2026 18:29
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.

2 participants