Skip to content

repo-sync-2025-04-22T17:16:18+0800 - #519

Merged
shaojian-ant merged 3 commits into
secretflow:mainfrom
shaojian-ant:main
Apr 23, 2025
Merged

shaojian-ant merged 3 commits into
secretflow:mainfrom
shaojian-ant:main

Conversation

@shaojian-ant

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello @shaojian-ant, 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!

Summary of Changes

This pull request introduces several key updates and enhancements to the YACL library. It adds support for SHAKE128 and SHAKE256 hash algorithms, includes a new BigInt implementation with GMP support, and refactors the BigInt library structure to allow for different underlying implementations (OpenSSL, TomMath, and GMP). The changes also include updates to build files, documentation, and test cases to accommodate the new features.

Highlights

  • SHAKE128 and SHAKE256 Support: Implements SHAKE128 and SHAKE256 extendable output functions (XOFs) using OpenSSL. This includes adding new functions to hash_utils.h and hash_utils.cc, creating ssl_hash_xof.h and ssl_hash_xof.cc for the OpenSSL implementation, and adding corresponding test cases in ssl_hash_xof_test.cc.
  • BigInt Implementation with GMP Support: Introduces a new BigInt implementation that supports GMP (GNU Multiple Precision Arithmetic Library) as one of the underlying arithmetic libraries. This involves creating new files under yacl/math/bigint including bigint.h, bigint.cc, bigint_spi.h, bigint_spi.cc, bigint_lib.h, mont_space.h, mont_space.cc, bigint_var.h, and a gmp subdirectory with GMP-specific implementations.
  • BigInt Library Refactoring: Refactors the BigInt library to allow for different underlying implementations (OpenSSL, TomMath, and GMP). This includes introducing a service provider interface (SPI) for BigInt libraries and updating the build files to conditionally include GMP support.
  • Build and Documentation Updates: Updates the .bazelrc file to include a flag for building with GMP, modifies BUILD.bazel files to include new source files and dependencies, and updates STANDARDS.md to reflect the added cryptographic algorithms.

Changelog

Click here to see the changelog
  • .bazelrc
    • Added a build flag --define BIGNUM_WITH_GMP=true for gmp.
  • STANDARDS.md
    • Updated the list of supported cryptography specifications to include SHAKE128 and SHAKE256.
  • yacl/crypto/hash/BUILD.bazel
    • Added ssl_hash_xof library and ssl_hash_xof_test test.
    • Added ssl_hash_xof as a dependency to hash_utils.
  • yacl/crypto/hash/hash_interface.h
    • Added SHAKE128 and SHAKE256 to the HashAlgorithm enum.
    • Updated ToString function to support SHAKE128 and SHAKE256.
  • yacl/crypto/hash/hash_utils.cc
    • Included ssl_hash_xof.h.
    • Added Shake128 and Shake256 functions.
  • yacl/crypto/hash/hash_utils.h
    • Included ssl_hash_xof.h.
    • Added declarations for Shake128 and Shake256 functions.
  • yacl/crypto/hash/ssl_hash_xof.cc
    • Created new file implementing SslHashXof class for SHAKE128 and SHAKE256.
  • yacl/crypto/hash/ssl_hash_xof.h
    • Created new file defining SslHashXof class and related classes.
  • yacl/crypto/hash/ssl_hash_xof_test.cc
    • Created new file containing test cases for SslHashXof class.
  • yacl/math/BUILD.bazel
    • Added common library for common math headers.
  • yacl/math/bigint/BUILD.bazel
    • Created new file defining build rules for the BigInt library, including conditional GMP support.
  • yacl/math/bigint/bigint.cc
    • Created new file implementing the BigInt class, which acts as a wrapper around different underlying implementations.
  • yacl/math/bigint/bigint.h
    • Created new file defining the BigInt class and its public interface.
  • yacl/math/bigint/bigint_lib.h
    • Created new file defining the IBigIntLib interface for different BigInt implementations.
  • yacl/math/bigint/bigint_spi.cc
    • Created new file implementing the service provider interface (SPI) for BigInt libraries.
  • yacl/math/bigint/bigint_spi.h
    • Created new file defining the BigIntSpiFactoryBase and BigIntLibFactory classes.
  • yacl/math/bigint/bigint_test.cc
    • Created new file containing test cases for the BigInt class.
  • yacl/math/bigint/bigint_var.h
    • Created new file defining the BigIntVar variant type.
  • yacl/math/bigint/gmp/BUILD.bazel
    • Created new file defining build rules for the GMP-specific BigInt implementation.
  • yacl/math/bigint/gmp/gmp_int.cc
    • Created new file implementing the GMP-specific BigInt class.
  • yacl/math/bigint/gmp/gmp_int.h
    • Created new file defining the GMP-specific BigInt class.
  • yacl/math/bigint/gmp/gmp_lib.cc
    • Created new file implementing the GmpLib class, which provides the IBigIntLib interface for GMP.
  • yacl/math/bigint/gmp/gmp_lib.h
    • Created new file defining the GmpLib class.
  • yacl/math/bigint/gmp/gmp_loader.cc
    • Created new file implementing the GMPLoader class, which dynamically loads the GMP library.
  • yacl/math/bigint/gmp/gmp_loader.h
    • Created new file defining the GMPLoader class.
  • yacl/math/bigint/gmp/mont_space.cc
    • Created new file implementing the GMP-specific Montgomery space.
  • yacl/math/bigint/gmp/mont_space.h
    • Created new file defining the GMP-specific Montgomery space.
  • yacl/math/bigint/openssl/BUILD.bazel
    • Created new file defining build rules for the OpenSSL-specific BigInt implementation.
  • yacl/math/bigint/openssl/bignum.cc
    • Created new file implementing the OpenSSL-specific BigInt class.
  • yacl/math/bigint/openssl/bignum.h
    • Created new file defining the OpenSSL-specific BigInt class.
  • yacl/math/bigint/openssl/mont_space.cc
    • Created new file implementing the OpenSSL-specific Montgomery space.
  • yacl/math/bigint/openssl/mont_space.h
    • Created new file defining the OpenSSL-specific Montgomery space.
  • yacl/math/bigint/tommath/BUILD.bazel
    • Created new file defining build rules for the TomMath-specific BigInt implementation.
  • yacl/math/bigint/tommath/mont_space.cc
    • Created new file implementing the TomMath-specific Montgomery space.
  • yacl/math/bigint/tommath/mont_space.h
    • Created new file defining the TomMath-specific Montgomery space.
  • yacl/math/bigint/tommath/tommath_lib.cc
    • Created new file implementing the TomMathLib class, which provides the IBigIntLib interface for TomMath.
  • yacl/math/bigint/tommath/tommath_lib.h
    • Created new file defining the TomMathLib class.
  • yacl/math/common.h
    • Added PrimeType enum
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

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.


A BigInt's a wondrous thing,
Numbers large, they make code sing.
With GMP's aid,
Calculations are made,
Crypto tasks, new heights they bring!

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several changes, including adding a build definition for GMP, updating the NIST standards document, incorporating SHAKE128 and SHAKE256 hash algorithms, and adding a BigInt implementation with GMP support. The changes seem well-structured and introduce new cryptographic functionalities.

Summary of Findings

  • Missing Error Handling in Montgomery Reduction: The MontgomeryReduce functions in gmp_int.cc and bignum.cc lack explicit error handling for potential failures during the Montgomery reduction process. This could lead to incorrect results or unexpected behavior if the reduction fails.
  • Inconsistent Use of YACL_ENFORCE: There are inconsistencies in the use of YACL_ENFORCE across the codebase. For example, some functions enforce conditions using YACL_ENFORCE, while others rely on OpenSSL's internal error handling mechanisms. Consistent error handling practices should be adopted.
  • Potential Performance Bottlenecks in Montgomery Reduction: The MontgomeryReduceComba function in gmp_int.cc is marked as exhibiting slower performance. This could be a performance bottleneck for certain cryptographic operations. Further optimization or alternative implementations should be considered.

Merge Readiness

The pull request introduces significant new functionality, particularly with the addition of GMP support and SHAKE algorithms. However, the identified issues regarding error handling and potential performance bottlenecks should be addressed before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the HIGH severity comments should be addressed before merging.

Comment thread yacl/math/bigint/gmp/gmp_int.cc
Comment thread yacl/math/bigint/openssl/bignum.cc
Comment thread yacl/math/bigint/gmp/gmp_int.cc
@shaojian-ant
shaojian-ant requested review from tongke6 and usafchn April 22, 2025 10:07

@tongke6 tongke6 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@shaojian-ant
shaojian-ant merged commit 64bca9a into secretflow:main Apr 23, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants