Skip to content

Fix missing public data mix#352

Merged
ClementWalter merged 1 commit into
mainfrom
cw/fix-missing-public-data-mix
Nov 3, 2025
Merged

Fix missing public data mix#352
ClementWalter merged 1 commit into
mainfrom
cw/fix-missing-public-data-mix

Conversation

@ClementWalter

@ClementWalter ClementWalter commented Oct 31, 2025

Copy link
Copy Markdown
Member

Summary

  • Add mix_into implementation and mix in prover and verifier
  • Fix import ordering across the codebase to follow Rust conventions

Test plan

  • Verify all tests pass
  • Ensure code compiles without warnings
  • Check that the mix functionality works correctly in both prover and verifier

🤖 Generated with Claude Code


Note

Add mix_into for public data and wire it into prover and verifier channel setup.

  • Public Data:
    • Add PublicEntries::mix_into to serialize and mix program/input/output entries into Channel.
    • Add PublicData::mix_into to mix registers, clock, memory roots, and delegate to public_memory.
  • Prover (crates/prover/src/prover.rs):
    • Construct PublicData and call public_data.mix_into(channel) before committing traces.
  • Verifier (crates/prover/src/verifier.rs):
    • Call proof.public_data.mix_into(channel) during setup before commitments and checks.

Written by Cursor Bugbot for commit 92b6740. This will update automatically on new commits. Configure here.

@claude

claude Bot commented Oct 31, 2025

Copy link
Copy Markdown

Claude finished @ClementWalter's task —— View job


Code Review Complete ✅

TLDR: High-quality implementation that correctly addresses the missing public data mix in the channel setup. The code follows Cairo-M conventions well and the channel mixing implementation is cryptographically sound. Minor performance improvements and test coverage could be enhanced.

Todo List:

  • Read and analyze changed files
  • Review mix_into implementation in public_data.rs
  • Check integration in prover.rs and verifier.rs
  • Assess code quality and best practices
  • Check for potential bugs and security concerns
  • Evaluate performance implications
  • Review test coverage
  • Provide comprehensive feedback

📋 Code Quality & Best Practices

Excellent:

  • ✅ Follows Rust documentation conventions with ## Arguments sections
  • ✅ Consistent with existing Cairo-M codebase patterns
  • ✅ Uses generic impl Channel trait bounds correctly
  • ✅ Proper error handling with unwrap_or_else() where appropriate
  • ✅ Clean separation of concerns between PublicEntries and PublicData

Good:

  • ✅ Import ordering follows Rust conventions (std, external crates, local modules)
  • ✅ Method signatures are well-designed and type-safe

🔍 Implementation Analysis

PublicEntries::mix_into (public_data.rs:132-189):

  • Correctly mixes lengths first, then data
  • Properly flattens Option values and extracts M31/QM31 components
  • Handles the complex QM31 structure (4 M31 components: value.0.0.0, value.0.1.0, value.1.0.0, value.1.1.0)

PublicData::mix_into (public_data.rs:401-412):

  • Mixes all critical VM state: registers, clock, memory roots
  • Delegates to public_memory.mix_into() for consistency

Integration (prover.rs:66, verifier.rs:31):

  • Correctly positioned after pcs_config.mix_into() and before trace commitments
  • Ensures prover and verifier have identical channel state

🚨 Potential Issues

Minor Performance Concerns:

  • collect::<Vec<_>>() in three locations creates intermediate allocations
  • Suggestion: Use iterator chaining to pass directly to mix_u32s():
    // Instead of collecting, consider if mix_u32s can accept an iterator
    channel.mix_u32s(&self.program.iter().flatten().flat_map(...).collect());

Code Duplication:

  • The flat_map logic is repeated 3 times with identical patterns
  • Suggestion: Extract into a helper method:
    fn mix_entries(channel: &mut impl Channel, entries: &[Option<(M31, QM31, M31)>]) {
        // shared implementation
    }

🔒 Security Analysis

Strong Points:

  • ✅ All public data is properly committed to the channel
  • ✅ No sensitive information leakage
  • ✅ Deterministic mixing order prevents manipulation
  • ✅ Channel state consistency between prover/verifier

Cryptographic Soundness:

  • ✅ Mixing occurs at the correct protocol stage
  • ✅ Public data binding prevents proof malleability
  • ✅ Covers all required VM state components

Performance Considerations

Current Implementation:

  • Memory allocations from collect() calls could be optimized
  • QM31 field element expansion is necessary but creates 4x data overhead

Benchmarks Show: Minimal impact (1.00-1.01x ratio) - excellent!

🧪 Test Coverage Assessment

Current State:

  • ❌ No dedicated tests for mix_into methods found
  • ❌ No unit tests for PublicEntries::mix_into serialization format
  • ❌ No integration tests verifying channel state consistency

Recommendations:

  1. Add unit tests for mix_into methods to verify:
    • Correct serialization order
    • Handling of None values in public entries
    • Channel state consistency between prover/verifier
  2. Property-based tests for different input sizes
  3. Integration test ensuring identical channel states

🎯 Specific Recommendations

  1. Extract Helper Method (public_data.rs:132-189):

    fn mix_memory_entries(channel: &mut impl Channel, entries: &[Option<(M31, QM31, M31)>]) {
        // Shared implementation for all three entry types
    }
  2. Performance Optimization:

    • Consider if Channel::mix_u32s can accept iterators directly
    • Profile memory allocations in hot paths
  3. Add Tests:

    #[cfg(test)]
    mod tests {
        #[test] 
        fn test_public_data_mix_into_consistency() { ... }
        
        #[test]
        fn test_public_entries_handles_none_values() { ... }
    }
  4. Documentation Enhancement:

    • Add example usage to doc comments
    • Document the specific channel mixing order requirements

Overall Assessment

This is a well-implemented solution that correctly addresses the missing public data mix. The code quality is high, follows project conventions, and the cryptographic integration is sound. The minor performance optimizations and test additions suggested above would make this even stronger, but the current implementation is solid and ready for production use.

The PR successfully implements the required functionality with proper channel integration and maintains the security properties of the STARK proof system.

@enitrat

enitrat commented Oct 31, 2025

Copy link
Copy Markdown
Member

@codex review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cairo-M VM Benchmarks

Details
Benchmark suite Current: 2737b1c Previous: 4be219f Ratio
fibonacci_1m/execution_only 289133638 ns/iter (± 2745452) 286377007 ns/iter (± 3052103) 1.01

This comment was automatically generated by workflow using github-action-benchmark.

@ClementWalter ClementWalter linked an issue Oct 31, 2025 that may be closed by this pull request
3 tasks
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cairo-M Prover Speed Benchmarks

Details
Benchmark suite Current: 92b6740 Previous: c017402 Ratio
prover_fibonacci/prove 2883242371 ns/iter (± 11489965) 2824850497 ns/iter (± 32178653) 1.02

This comment was automatically generated by workflow using github-action-benchmark.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cairo-M Prover Memory Benchmarks

Details
Benchmark suite Current: 92b6740 Previous: c017402 Ratio
fibonacci_prove_peak_mem 2422514599 bytes 2422514085 bytes 1.00
sha256_1kb_prove_peak_mem 2184172038 bytes 2184172036 bytes 1.00

This comment was automatically generated by workflow using github-action-benchmark.

zmalatrax
zmalatrax previously approved these changes Oct 31, 2025
@ClementWalter ClementWalter dismissed zmalatrax’s stale review October 31, 2025 16:03

The merge-base changed after approval.

@ClementWalter ClementWalter force-pushed the cw/fix-missing-public-data-mix branch from 2737b1c to 92b6740 Compare October 31, 2025 16:03
@ClementWalter ClementWalter merged commit 79c1b1e into main Nov 3, 2025
19 checks passed
@ClementWalter ClementWalter deleted the cw/fix-missing-public-data-mix branch November 3, 2025 10:03
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.

[vulnerability] The public data is not mixed into the channel

4 participants