Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (51)
📝 WalkthroughWalkthroughThis PR adds many {.used.} pragmas, renames generic hash constants to algorithm-prefixed names (Sha256_, RMD160_), removes unused imports/locals, tightens several static size assertions, and disables or trims multiple tests and experimental backend imports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the overall code quality and maintainability by systematically addressing compilation warnings and removing dead code. The changes primarily involve refining imports, updating constant references for clarity, and explicitly marking elements as 'used' to satisfy the compiler. A notable functional change is the removal of problematic SHA3 tests that relied on inconsistent OpenSSL features, which improves the stability of the test environment. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a number of compilation warnings and hints across the codebase. The changes primarily involve removing unused imports and variables, adding {.used.} pragmas to prevent incorrect unused symbol warnings, and updating code to align with modern Nim practices (e.g., proc =copy for disabling copies). These modifications improve code hygiene and maintainability without introducing any apparent issues. The approach taken is sound and the changes are well-contained.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/t_hash_keccak_sha3_vs_openssl.nim (1)
206-214:⚠️ Potential issue | 🟡 MinorFix the skip log label mismatch.
Line 213 prints “SHA256” in a SHA3-256 test file, which can mislead CI/debug output.
✏️ Proposed fix
- echo "SHA256 - Differential testing vs OpenSSL - [SKIPPED]" + echo "SHA3-256 - Differential testing vs OpenSSL - [SKIPPED]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/t_hash_keccak_sha3_vs_openssl.nim` around lines 206 - 214, The else branch prints the wrong algorithm label—change the echo that currently outputs "SHA256 - Differential testing vs OpenSSL - [SKIPPED]" to use "SHA3-256" so the skip message matches the test file; locate the else branch echo in tests/t_hash_keccak_sha3_vs_openssl.nim (the echo in the when/else block) and update the string to "SHA3-256 - Differential testing vs OpenSSL - [SKIPPED]".constantine/math_compiler/impl_fields_ops.nim (1)
105-116:⚠️ Potential issue | 🟠 MajorFix wrong identifier in
mulCheckSparsefallback branch.At Line 115,
x *= bshould use the template parametery. Current code can bind to an unintended outerbsymbol and compute the wrong result.🔧 Proposed fix
else: - x *= b + x *= y🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@constantine/math_compiler/impl_fields_ops.nim` around lines 105 - 116, The fallback branch in the template mulCheckSparse uses the wrong identifier `b`, which can bind to an outer symbol and produce incorrect results; change the multiplication to use the template parameter `y` instead (replace `x *= b` with `x *= y`) so the method always multiplies by the intended argument; update any related comments if needed and run tests that exercise mulCheckSparse to confirm correct behavior.
🧹 Nitpick comments (2)
constantine/math_compiler/pub_fields.nim (1)
9-12: Top-of-file NOTE appears stale after removingasm_nvidia.The NOTE still describes a direct
asm_nvidiadependency, but the import now excludes it. Consider updating/removing the comment to avoid misleading maintenance context.Suggested comment-only cleanup
-# NOTE: We probably don't want the explicit `asm_nvidia` dependency here, I imagine? Currently it's -# for direct usage of `slct` in `neg`. +# NOTE: Explicit `asm_nvidia` dependency was removed from this module as part of warning/import cleanup. +# Keep backend-specific operations encapsulated in lower-level implementation modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@constantine/math_compiler/pub_fields.nim` around lines 9 - 12, The top-of-file NOTE about an explicit asm_nvidia dependency is stale: update or remove that comment so it no longer claims a direct asm_nvidia import; either delete the NOTE or reword it to reflect the current import of constantine/platforms/llvm/llvm and the fact that usage of slct in neg no longer requires asm_nvidia. Ensure the comment references the symbols asm_nvidia, slct, and neg so future readers understand why the NOTE was changed.tests/t_ethereum_eip4844_deneb_kzg_parallel.nim (1)
176-191: Consider removing the large commented-out test body.Lines 176-191 keep dead code inline. A short comment pointing to the non-parallel test location is easier to maintain than a fully commented implementation.
🧹 Suggested cleanup
-# testGen(verify_kzg_proof, testVector): -# ... -# else: -# doAssert testVector["output"].content == "null" +# `verify_kzg_proof` is intentionally omitted in this parallel suite +# because the implementation under test is non-parallel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/t_ethereum_eip4844_deneb_kzg_parallel.nim` around lines 176 - 191, Remove the large commented-out test body that includes the testGen/verify_kzg_proof scaffold and parseAssign lines (commitment, z, y, proof) and the ctx.verify_kzg_proof status-checking block; replace it with a short one-line comment that points to the canonical non-parallel test location (or references the existing non-parallel test) so the file no longer contains the dead inline implementation while preserving a pointer for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@constantine/math_compiler/impl_fields_ops.nim`:
- Around line 105-116: The fallback branch in the template mulCheckSparse uses
the wrong identifier `b`, which can bind to an outer symbol and produce
incorrect results; change the multiplication to use the template parameter `y`
instead (replace `x *= b` with `x *= y`) so the method always multiplies by the
intended argument; update any related comments if needed and run tests that
exercise mulCheckSparse to confirm correct behavior.
In `@tests/t_hash_keccak_sha3_vs_openssl.nim`:
- Around line 206-214: The else branch prints the wrong algorithm label—change
the echo that currently outputs "SHA256 - Differential testing vs OpenSSL -
[SKIPPED]" to use "SHA3-256" so the skip message matches the test file; locate
the else branch echo in tests/t_hash_keccak_sha3_vs_openssl.nim (the echo in the
when/else block) and update the string to "SHA3-256 - Differential testing vs
OpenSSL - [SKIPPED]".
---
Nitpick comments:
In `@constantine/math_compiler/pub_fields.nim`:
- Around line 9-12: The top-of-file NOTE about an explicit asm_nvidia dependency
is stale: update or remove that comment so it no longer claims a direct
asm_nvidia import; either delete the NOTE or reword it to reflect the current
import of constantine/platforms/llvm/llvm and the fact that usage of slct in neg
no longer requires asm_nvidia. Ensure the comment references the symbols
asm_nvidia, slct, and neg so future readers understand why the NOTE was changed.
In `@tests/t_ethereum_eip4844_deneb_kzg_parallel.nim`:
- Around line 176-191: Remove the large commented-out test body that includes
the testGen/verify_kzg_proof scaffold and parseAssign lines (commitment, z, y,
proof) and the ctx.verify_kzg_proof status-checking block; replace it with a
short one-line comment that points to the canonical non-parallel test location
(or references the existing non-parallel test) so the file no longer contains
the dead inline implementation while preserving a pointer for future
maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c93bd435-d291-41b3-a31d-b98ec0e3b777
📒 Files selected for processing (50)
bindings/lib_hashes.nimconstantine/ethereum_bls_signatures.nimconstantine/ethereum_evm_precompiles.nimconstantine/ethereum_verkle_ipa.nimconstantine/hash_to_curve/h2c_hash_to_field.nimconstantine/hashes/h_ripemd160.nimconstantine/hashes/h_sha256.nimconstantine/hashes/ripemd160/ripemd160_generic.nimconstantine/hashes/sha256/sha256_generic.nimconstantine/hashes/sha256/sha256_x86_sha.nimconstantine/hashes/sha256/sha256_x86_ssse3.nimconstantine/math/arithmetic/assembly/limbs_asm_mul_mont_x86.nimconstantine/math/arithmetic/assembly/limbs_asm_mul_mont_x86_adx_bmi2.nimconstantine/math/arithmetic/assembly/limbs_asm_redc_mont_x86_adx_bmi2.nimconstantine/math/arithmetic/limbs_crandall.nimconstantine/math/elliptic/ec_multi_scalar_mul_parallel.nimconstantine/math/elliptic/ec_scalar_mul.nimconstantine/math/elliptic/ec_scalar_mul_vartime.nimconstantine/math/polynomials/polynomials.nimconstantine/math/polynomials/polynomials_parallel.nimconstantine/math_compiler/experimental/backends/cuda.nimconstantine/math_compiler/experimental/backends/wgsl.nimconstantine/math_compiler/experimental/cuda_execute_dsl.nimconstantine/math_compiler/experimental/gpu_compiler.nimconstantine/math_compiler/experimental/nim_to_gpu.nimconstantine/math_compiler/experimental/runtime_compile.nimconstantine/math_compiler/impl_curves_ops_affine.nimconstantine/math_compiler/impl_curves_ops_jacobian.nimconstantine/math_compiler/impl_fields_ops.nimconstantine/math_compiler/impl_msm_nvidia.nimconstantine/math_compiler/ir.nimconstantine/math_compiler/pub_curves_jacobian.nimconstantine/math_compiler/pub_fields.nimconstantine/named/constants/bandersnatch_endomorphisms.nimconstantine/named/constants/banderwagon_endomorphisms.nimconstantine/named/constants/secp256k1_endomorphisms.nimconstantine/named/properties_fields.nimconstantine/platforms/constant_time/ct_routines.nimconstantine/platforms/loadtime_functions.nimconstantine/proof_systems/constraint_systems/r1cs_circom_parser.nimconstantine/signatures/bls_signatures.nimconstantine/signatures/ecc_sig_ops.nimconstantine/signatures/ecdsa.nimconstantine/threadpool/crossthread/scoped_barriers.nimconstantine/threadpool/instrumentation.nimconstantine/threadpool/parallel_offloading.nimconstantine/threadpool/primitives/topology_linux.nimtests/gpu/t_msm.nimtests/t_ethereum_eip4844_deneb_kzg_parallel.nimtests/t_hash_keccak_sha3_vs_openssl.nim
💤 Files with no reviewable changes (7)
- constantine/ethereum_bls_signatures.nim
- constantine/ethereum_evm_precompiles.nim
- constantine/math/polynomials/polynomials_parallel.nim
- constantine/math/polynomials/polynomials.nim
- constantine/hash_to_curve/h2c_hash_to_field.nim
- constantine/math/elliptic/ec_multi_scalar_mul_parallel.nim
- constantine/math/elliptic/ec_scalar_mul_vartime.nim
mratsim
force-pushed
the
fix-compilation-warnings
branch
from
6fe834b to
f4a5271
Compare
…_kzg_parallel.nim
mratsim
force-pushed
the
fix-compilation-warnings
branch
from
f4a5271 to
e0de081
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes most compilation warnings and hints.
Tested on:
Has leftover:
I've also removed SHA3 tests against OpenSSL as
EVP_Q_digestsupport is very spotty: not on Windows, MacOS (libressl) or my dev machine (Archlinux).Summary by CodeRabbit
Refactoring
Code Cleanup
Chores
Tests