Skip to content

Fix: Update noir-r1cs with Skyscraper v2#142

Merged
Bisht13 merged 10 commits into
mainfrom
sl/skyscraper-refactor
Aug 27, 2025
Merged

Fix: Update noir-r1cs with Skyscraper v2#142
Bisht13 merged 10 commits into
mainfrom
sl/skyscraper-refactor

Conversation

@shreyas-londhe
Copy link
Copy Markdown
Collaborator

This PR:

  • Refactors the code by moving block_multiplier, block_multiplier_codegen, fp_rounding and hla into /skyscraper and moves /skyscraper to /skyscraper/core.
  • Updates /noir-r1cs/skyscraper to be compatible with Skyscraper v2.

Impact:

  • No behavior change expected; Merkle, sponge, and proofs continue to work via externalized Skyscraper
  • Clear separation of concerns: generic performance code in skyscraper/, noir-r1cs keeps only protocol glue
  • Easier maintenance and reuse of skyscraper/ components across projects

Copy link
Copy Markdown
Contributor

Copilot AI left a 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 refactors the codebase to align with Skyscraper v2 by moving core components into a centralized skyscraper/ directory structure and updating noir-r1cs to be compatible with the new version. The refactoring maintains behavioral compatibility while improving code organization.

  • Moves block_multiplier, block_multiplier_codegen, fp_rounding, and hla into skyscraper/ subdirectory
  • Migrates existing /skyscraper to /skyscraper/core for better organization
  • Updates noir-r1cs/skyscraper implementation to use Skyscraper v2 APIs

Reviewed Changes

Copilot reviewed 23 out of 88 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
noir-r1cs/src/skyscraper/whir.rs Updates type aliases from Field256 to FieldElement and replaces local compress function with v2 API
noir-r1cs/src/skyscraper/sponge.rs New sponge implementation using FieldElement and skyscraper::reference::permute
noir-r1cs/src/skyscraper/skyscraper_impl.rs Removes old implementation file (171 lines deleted)
noir-r1cs/src/skyscraper/mod.rs Updates module structure to match new organization
noir-r1cs/Cargo.toml Adds ark-bn254 dependency for field element conversions
gnark-whir/whir_utilities.go Updates function calls from Compress to CompressV2
gnark-whir/go.mod Updates dependency versions for gnark-nimue and gnark-skyscraper
Cargo.toml Restructures workspace members to use new skyscraper/ directory layout
cm31_ntt/* Code style improvements including import organization and formatting consistency
.github/workflows/benchmark.yml Enhances benchmark workflow to automatically discover packages with benchmarks

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread cm31_ntt/src/rm31.rs
Comment thread cm31_ntt/src/rm31.rs
Comment thread .github/workflows/benchmark.yml Outdated
Comment thread noir-r1cs/src/skyscraper/pow.rs
Comment thread noir-r1cs/src/skyscraper/sponge.rs
@worldfnd worldfnd deleted a comment from codspeed-hq Bot Aug 21, 2025
@shreyas-londhe shreyas-londhe force-pushed the sl/skyscraper-refactor branch from c9b8ae2 to c0a5084 Compare August 27, 2025 14:13
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Aug 27, 2025

CodSpeed WallTime Performance Report

Merging #142 will improve performances by ×52

Comparing sl/skyscraper-refactor (c0a5084) with main (490264f)

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

Summary

⚡ 1 improvements
✅ 3 untouched benchmarks
🆕 31 new benchmarks
⁉️ 31 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
⁉️ ark_ff 98 ns N/A N/A
⁉️ block_mul 127 ns N/A N/A
⁉️ montgomery_interleaved_3 113 ns N/A N/A
⁉️ montgomery_interleaved_4 192 ns N/A N/A
⁉️ scalar_mul 95 ns N/A N/A
⁉️ simd_mul 112 ns N/A N/A
⁉️ ark_ff 81 ns N/A N/A
⁉️ block_sqr 91 ns N/A N/A
⁉️ montgomery_square_interleaved_3 97 ns N/A N/A
⁉️ montgomery_square_interleaved_4 157 ns N/A N/A
⁉️ montgomery_square_log_interleaved_3 97 ns N/A N/A
⁉️ montgomery_square_log_interleaved_4 157 ns N/A N/A
⁉️ scalar_sqr 78 ns N/A N/A
⁉️ simd_sqr 85 ns N/A N/A
⁉️ wrm_overhead 14 ns N/A N/A
verify_poseidon_1000 118.8 ms 2.3 ms ×52
⁉️ block3 162.5 µs N/A N/A
⁉️ block4 196.6 µs N/A N/A
⁉️ reference 1.3 ms N/A N/A
⁉️ simple 381.5 µs N/A N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@shreyas-londhe shreyas-londhe requested a review from Bisht13 August 27, 2025 15:29
@xrvdg
Copy link
Copy Markdown
Collaborator

xrvdg commented Sep 1, 2025

These crates are not skyscraper specific. Grouping them separate from skyscraper under montgomery would be better. It will be used by the NTT as well.

dcbuild3r pushed a commit that referenced this pull request May 16, 2026
Fix: Update `noir-r1cs` with Skyscraper v2
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.

4 participants