feat(field): add KoalaBear prime field and degree 4 extension - #1136
Conversation
Add KoalaBear field (p = 2^31 - 2^24 + 1 = 2130706433) following Plonky3's
implementation. Includes:
- Base field with TWO_ADICITY=24 and full FFT support
- Degree 4 extension using irreducible polynomial x^4 - 3
- Optimized mul_by_beta using additions (3x = x.double() + x)
- Generic inversion algorithm in U32MontgomeryBackendPrimeField
BETA = 3 is both a quadratic and quartic non-residue, making it the correct
choice for the extension field (matching Plonky3's W=3).
The extension field supports basic arithmetic (add, mul, inv) but not FFT
operations, as computing the primitive 2^26-th root of unity in F_{p^4}
requires finding a multiplicative generator.
Acknowledgments: Plonky3 (Polygon) for field definition, RISC Zero for
degree 4 inversion algorithm, lambdaworks BabyBear for implementation patterns.
Kimi AI ReviewReview Feedback
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Correctness
- The implementation appears mostly correct; however, thorough checks on the underlying
U32MontgomeryBackendPrimeFieldclass would be needed to ensure proper modular arithmetic operations, especially given the emphasis on FFT operations, which are very sensitive to arithmetic correctness.
Security
- Timing Side-Channels: Ensure all arithmetic operations involving field elements, particularly inversions, are constant time to prevent timing attacks. The code currently does not specify if the operations provided by
U32MontgomeryBackendPrimeFieldhandle this. - Hash Function Domain Separation: No evidence of domain separation in hashing is evident, though it's likely not in the reviewed snippet. Check elsewhere if the code interacts with hash functions.
- Proper Zeroization: The code does not show explicit zeroization of sensitive data. Consider using libraries or mechanisms to zeroize secret data properly after use to prevent data leaks.
Performance
- Unnecessary Allocations: The function
get_powers_of_primitive_rootmight be generating temporary vectors that could be eliminated or reduced. - Redundant Field Inversions: Field inversions may already be optimized by the library; it's essential to verify that they aren't called unnecessarily since they are expensive operations.
Bugs & Errors
- Panics or Unwraps:
expectonunwrap/expectusage exists in tests, though it's acceptable here, it might be risky if similar patterns exist in production code. - Integer Overflow/Underflow: There is a theoretical risk of implicit overflows in arithmetic operations; a dense test around these operations should include checks for overflows.
Code Simplicity
- The code currently seems complex with a significant amount of test boilerplate. Abstractions for repetitive test patterns could improve test maintainability and readability.
- Could consider reducing complexity by modularizing test functions where repetitive logic patterns (e.g., FFT evaluations, or different interpolation methods).
Overall, while the changes seem promising, ensure comprehensive correctness validation, constant-time assurance, and avoidance of any risk of timing side-channels before merging.
| pub mod quartic_koalabear; | ||
| /// Implementation of the prime field used in [Stark101](https://starkware.co/stark-101/) tutorial, p = 3 * 2^30 + 1 | ||
| pub mod stark_101_prime_field; | ||
| /// Implementation of two-adic prime field over 256 bit unsigned integers. |
There was a problem hiding this comment.
Correctness
- Edge Cases: Ensure the new KoalaBear prime field correctly handles all edge cases, similar to Babybear, especially for zero-initialization and identity elements.
- Modular Arithmetic: Double-check the arithmetic operations for the KoalaBear prime field to verify correct modular reduction with the specified prime.
Security
- Timing Side-Channels: It is crucial that all field operations and sensitive procedures in both Babybear and KoalaBear implementations are conducted in constant-time to prevent timing attacks.
- Cryptographically Secure Randomness: When generating elements from these fields, ensure that randomness is generated using a cryptographically secure RNG where applicable.
- Zeroization: Any sensitive data should be zeroized appropriately after its use, especially in extended field operations.
Performance
- Redundant Field Inversions: Review the code for any unnecessary field inversions in both Babybear and KoalaBear that could be avoided through alternative algorithms.
- MSM and FFT Efficiency: If MSM or FFT are implemented within these modules, profile and optimize for performance, focusing on avoiding unnecessary data allocations.
Bugs & Errors
- Potential Panics or Unwraps: Ensure no operations in these modules use
unwrap()or could lead to a panic under legitimate inputs. Consider returning Result types where failures are possible. - Memory Safety: Validate that all memory operations are safe and free from potential overflows or unchecked indexing.
Code Simplicity
- Duplicated Code: If KoalaBear shares much logic with Babybear, consider abstracting common operations to avoid code duplication and improve maintainability.
Overall, thorough testing is recommended, both unit tests and property-based tests, to ensure the operations adhere to expected behaviors across these critical cryptographic modules.
| assert_eq!(Fp4E::from_bytes_le(&bytes).expect("valid bytes").to_bytes_le(), bytes); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Correctness
- Polynomial Arithmetic: The code for polynomials modulo x^4 - 3 appears mathematically sound, and tests are provided to back operations.
- Inversion Logic: The inversion algorithm looks properly implemented for the irreducible polynomial x^4 - 3.
- Edge Cases: There are tests for zero and identity elements but ensure thorough edge cases like the inverse of zero are checked.
Security
- Timing Side-channels: There are no evident constant-time guarantees, especially in arithmetic operations like
inv. Consider using a constant-time algorithm for inversion, especially when handling sensitive data. - Zeroization: Sensitive data zeroization is not evident, which is important for security in cryptographic operations.
- Branching: Ensure no secret-dependent branching is happening unintentionally.
- Randomness:
get_random_field_element_from_rnguses rejection sampling correctly, but ensure the RNG is cryptographically secure. - Hash Function Domain Separation: Not evaluated in the provided code. Ensure domain separation in associated cryptographic hash function use.
Performance
- Redundant Operation Checks:
doublecallsadd, which may induce unnecessary overhead. Direct implementation might be optimized. - FFT & MSM: Since FFT support isn't implemented yet for this degree 4 field, this limits potential use cases for optimization discussions.
- Unnecessary Allocations: The
to_bytes_beandto_bytes_lecould aggregate bytes without extending them repeatedly, thus reducing allocation overhead.
Bugs & Errors
- Memory Safety: No memory safety issues identified in provided code, but it heavily depends on proper handling in called methods.
- Panics or Unwraps: Some unwraps in tests must ensure no panics on valid field operations.
- Integer Overflow/Underflow: Rust's
FieldElementlikely provides safety against integer overflows, but double-check any unchecked regions missing exploits. - Off-by-One Errors: None detected in this code review.
Code Simplicity
- Complexity: The
invmethod's complex nature could be a candidate for refactoring into simpler components or utilizing more comments. - Duplicated Code: Byte conversion methods present similar patterns. Consider refactoring for less duplication.
- Poor Abstractions: The library efficiently defines
IsFieldandIsSubFieldOftraits to handle field extensions, enhancing code maintainability.
| Ok(result) | ||
| } | ||
|
|
||
| #[inline(always)] |
There was a problem hiding this comment.
Correctness
- Mathematical Operations: The use of a generic square-and-multiply algorithm for computing the multiplicative inverse via Fermat's Little Theorem is correct theoretically, but it's crucial to ensure that this algorithm operates correctly for all valid inputs. Specifically, ensure that
MODULUSis indeed a prime as assumed.
Security
- Timing Side-Channels: The new implementation employs a basic square-and-multiply algorithm, which may not be constant-time due to the secret-dependent loop based on the exponent bits. This could lead to timing side-channel vulnerabilities when operating on secret data. Consider using a constant-time algorithm like Montgomery ladder for more secure exponentiation.
- Zeroization of Sensitive Data: There is no explicit mention of any zeroization of the variables used in this computation (e.g.,
result,base). While they are on the stack, ensure zeroization of sensitive data when feasible.
Bugs & Errors
- Potential Panics: Ensure that the subtraction
MODULUS - 2does not panic due to potential underflows. AlthoughMODULUSshould be greater than 2 for primes, handling edge cases or verifying constraints is crucial.
Performance
- Efficiency: The new algorithm is likely to be more efficient than the previous unrolled method, especially for large
MODULUS, due to its reduced complexity dependency on the modulus size.
Code Simplicity
- Complexity: The current implementation is significantly simpler and more maintainable than the previous, albeit needing attention on the timing side-channel front.
Recommendations
- Implement constant-time exponentiation for secure computations.
- Add checks or assertions to ensure that
MODULUSmeets the required properties (i.e., being a prime greater than 2). - Consider adding test cases for edge cases such as minimal non-zero values and maximal values just below
MODULUS.
Greptile OverviewGreptile SummaryThis PR adds the KoalaBear prime field (p = 2^31 - 2^24 + 1) and its degree-4 extension, following Plonky3's implementation. The implementation includes:
The PR correctly sets Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| crates/math/src/field/fields/fft_friendly/koalabear.rs | New KoalaBear prime field implementation with comprehensive tests and FFT support |
| crates/math/src/field/fields/fft_friendly/quartic_koalabear.rs | Degree-4 extension field using x^4 - 3 with optimized mul_by_beta and comprehensive tests (BYTES_PER_FIELD correctly set to 4) |
| crates/math/src/field/fields/u32_montgomery_backend_prime_field.rs | Refactored inv() to use generic Fermat's Little Theorem instead of BabyBear-specific optimization |
| crates/math/src/field/fields/fft_friendly/mod.rs | Module registration for koalabear and quartic_koalabear modules |
Sequence Diagram
sequenceDiagram
participant User
participant KoalaBear as Koalabear31PrimeField
participant Quartic as Degree4KoalaBearExtensionField
participant Montgomery as U32MontgomeryBackendPrimeField
participant FFT as FFT Operations
Note over KoalaBear: Base Field (p = 2^31 - 2^24 + 1)
User->>KoalaBear: Create field element
KoalaBear->>Montgomery: Use Montgomery backend
Montgomery-->>KoalaBear: Element in Montgomery form
User->>KoalaBear: Field operations (add, mul, inv)
KoalaBear->>Montgomery: Generic Fermat's Little Theorem inv()
Montgomery-->>KoalaBear: Result
User->>KoalaBear: FFT operations
KoalaBear->>FFT: Use TWO_ADICITY=24, root=1791270792
FFT-->>User: FFT result
Note over Quartic: Extension Field (x^4 - 3)
User->>Quartic: Create extension element [a0,a1,a2,a3]
Quartic->>Quartic: mul_by_beta(x) optimized as x.double() + x
User->>Quartic: Extension operations (mul, inv)
Quartic->>Quartic: Adapted RISC Zero algorithm for x^4 - β
Quartic->>KoalaBear: Base field operations
KoalaBear-->>Quartic: Base field results
Quartic-->>User: Extension result
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| where | ||
| Self: Sized, | ||
| { | ||
| const BYTES_PER_FIELD: usize = 32; |
There was a problem hiding this comment.
BYTES_PER_FIELD should be 4, not 32. KoalaBear is a u32 field, so each element is 4 bytes.
| const BYTES_PER_FIELD: usize = 32; | |
| const BYTES_PER_FIELD: usize = 4; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/math/src/field/fields/fft_friendly/quartic_koalabear.rs
Line: 273:273
Comment:
`BYTES_PER_FIELD` should be 4, not 32. KoalaBear is a u32 field, so each element is 4 bytes.
```suggestion
const BYTES_PER_FIELD: usize = 4;
```
How can I resolve this? If you propose a fix, please make it concise.| where | ||
| Self: Sized, | ||
| { | ||
| const BYTES_PER_FIELD: usize = 32; |
There was a problem hiding this comment.
BYTES_PER_FIELD should be 4, not 32. KoalaBear is a u32 field, so each element is 4 bytes.
| const BYTES_PER_FIELD: usize = 32; | |
| const BYTES_PER_FIELD: usize = 4; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/math/src/field/fields/fft_friendly/quartic_koalabear.rs
Line: 287:287
Comment:
`BYTES_PER_FIELD` should be 4, not 32. KoalaBear is a u32 field, so each element is 4 bytes.
```suggestion
const BYTES_PER_FIELD: usize = 4;
```
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1136 +/- ##
==========================================
+ Coverage 72.35% 72.45% +0.10%
==========================================
Files 167 169 +2
Lines 37498 38052 +554
==========================================
+ Hits 27130 27569 +439
- Misses 10368 10483 +115 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…rsion KoalaBear uses u32 (4 bytes), not 32 bytes per field element. Fixed the ByteConversion impl for [FieldElement<Koalabear31PrimeField>; 4].
| /// BETA = 3 is the non-residue (both quadratic and quartic non-residue in KoalaBear). | ||
| /// Since `const_from_raw()` doesn't make the montgomery conversion, we calculated it. | ||
| /// The montgomery form of a number "a" is a * R mod p. | ||
| /// In KoalaBear field, R = 2^32 and p = 2130706433. |
There was a problem hiding this comment.
Correctness
- Error in ByteConversion: The size change in
BYTES_PER_FIELDfrom 32 to 4 bytes suggests that each field element inKoalabear31PrimeFieldis being represented as a 32-bit value rather than a 256-bit value. This discrepancy affects the deserialization of field elements, potentially leading to incorrect mathematical results throughout operations dealing with these elements.
Security
- No immediate issues found given the context of the changes. Ensure any arithmetic or comparison involving sensitive data remains constant-time, although this snippet doesn't directly touch on such concerns.
Performance
- Potential Performance Impact: The assumption that fields are 32-bits rather than 256 might indicate optimization issues elsewhere if 256-bit arithmetic is expected within the library.
Bugs & Errors
- Potential Deserialization Bugs: If the assumption that each element is 32 bits is incorrect, slicing the bytes in 4-byte increments will lead to incorrect initialization of
FieldElementvalues, leading to faulty operations or panics if bounds checks on the 'from_bytes' function are not comprehensive.
Code Simplicity
- Ambiguity in Field Size Constants: Ensure that the code clearly communicates why a 4-byte representation is appropriate and whether this aligns with expectations of other code portions relying on Koalabear31PrimeField.
Summary
- The change in
BYTES_PER_FIELDrequires a thorough review of how field elements are expected to be represented throughout the library. This affects deserialization and potentially invalidates the correctness of the operations. It's crucial to validate whether Koalabear31PrimeField genuinely uses u32 representation and if not, revert or adjust operations accordingly. Further inspection on potential impacts on cryptographic operations and alignment with the library's overall representation logic is needed before merging.
Kimi AI ReviewThe PR introduces new implementations for the KoalaBear prime field and its degree 4 extension. Here are my comments and concerns:
Overall, the PR makes significant changes to the field implementations. While the optimizations and custom operations seem to improve performance, there are concerns about the correctness of the two-adic primitive root of unity and the security implications of the new inversion algorithm. Additionally, the lack of FFT support and the need for better documentation and error handling should be addressed. |
Kimi AI ReviewReview FeedbackFile:
|
| } | ||
|
|
||
| #[test] | ||
| fn mul_2_3_is_6() { |
There was a problem hiding this comment.
Here are the issues identified in the code changes:
Correctness:
-
The functions in test cases, such as
to_bytes_from_bytes_be_is_the_identity, rely on hexadecimal and byte conversions but do not cover edge cases such as zero or the maximum field element value. -
Check if
Polynomial::evaluate_fftandPolynomial::interpolate_ffthandle edge cases like polynomials of zero length or constant polynomials correctly.
Security:
-
Ensure that all operations, especially those involving the
FE::from_bytes_beandFE::from_bytes_le, are implemented in constant time to avoid timing side channels. -
There is no indication of proper zeroization of sensitive data, such as intermediate calculations or results, which might be necessary depending on their usage.
-
Verify that cryptographically secure randomness (e.g., for nonce generation) is used where needed, such as in generating primitive roots or offsets.
Performance:
-
Consider adding tests for performance-critical paths, especially FFT-related computations, to ensure efficient execution.
-
Ensure no redundant memory allocations occur in loop constructs, particularly inside FFT and polynomial evaluations. Allocation, especially in a loop, can degrade performance.
Bugs & Errors:
-
Functions use
.expect()calls which may panic. Consider handling these situations gracefully or ensuring they will not fail in production. -
Since the fft and interpolation methods often involve indices and length calculations, make sure these calculations do not lead to off-by-one errors or overflows. Consider wrapping with a checked arithmetic if necessary.
Code Simplicity:
- Verifying duplicate logic in the test cases might help to simplify them. For example, similar logic is used in conversions (
to_bytes_leandto_bytes_be). Abstracting this functionality could reduce duplication.
Please address these issues to ensure that the code is correct and secure before merging.
| fn sub(a: &Self::BaseType, b: &Self::BaseType) -> Self::BaseType { | ||
| [a[0] - b[0], a[1] - b[1], a[2] - b[2], a[3] - b[3]] | ||
| } | ||
|
|
There was a problem hiding this comment.
Correctness
- The function
mul_by_betauses a simple and correct optimization for multiplying by the constant BETA, which is 3 in this context. It should be verified that the.double()method is correctly implemented for arithmetic within theKoalabear31PrimeField. - No code provided that involves handling of edge cases such as zero or infinity points for elliptic curves. Ensure that these are correctly handled elsewhere in the library.
Security
- There are no observed changes addressing timing side-channels or constant-time operations. Ensure that all sensitive field operations (such as multiplications or inversions involving secret data) are performed in constant time.
- No evidence of zeroization for sensitive data, for example in temporary variables used in field operations, exposing risk if these variables leak from memory in other parts of the code.
- Verify cryptographically secure randomness if used elsewhere; not enough context provided here.
- Ensure hash function usages are correctly domain-separated if hashes are used in proofs or random oracles.
Performance
- The current change focuses mainly on format adjustments and test cases.
- Test cases are invoking
.expect(), which can panic, consider handling errors gracefully in non-test code paths.
Bugs & Errors
- The use of
.expect()in test cases should be safe given the provided context; however, ensure that production code handles potential errors gracefully without panic.
Code Simplicity
- No complex logic changes in the provided diff.
- Avoid unnecessary line breaks in expressions for readability in functions and structs, unless naturally complex.
General Comments
The provided code is related to tests and minor format changes. Assuming the core implementation is elsewhere, verify that any underlying field and arithmetic operations are correctly implemented and constant-time as there are no direct algorithmic changes in the provided diff. Ensure that sensitive operations are secure and efficiencies like FFT and MSMs are maintained in other parts of the code.
…ar tests Tests using from_hex, to_bytes_*, ByteConversion, and CreationError require alloc feature. Added conditional compilation attributes to: - Import statements (CreationError, ByteConversion) - Test functions (byte_serialization_and_deserialization_works_*)
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Correctness:
- Ensure that all mathematical operations involving
FE(FieldElements) are consistent with the field arithmetic specifications, especially handling edge cases like zero and one. In this diff, there are no seen issues with the operations themselves, but it is crucial the library indeed implements such checks.
Security:
-
Constant-Time Operations: Ensure back-end implementations for arithmetic within
FieldElement, such asto_bytes_leandto_bytes_be, are constant-time because they manage potentially sensitive cryptographic elements. -
Zeroization: After operations, or when memory deallocation happens, confirm that any sensitive data (like field elements) is properly zeroed from memory.
-
Feature Guards: The use of
#[cfg(feature = "alloc")]suggests conditional compilation. Ensure that whenallocisn't enabled, the absence of these features doesn’t introduce vulnerabilities elsewhere in the compiled code.
Performance:
- No performance issues visible in the provided code chunk, but inside field operations, excessive use of allocations could be a concern when the
allocfeature is enabled.
Bugs & Errors:
- Panics/Unwraps: Using
expect("valid hex")can cause a panic if the assumption isn’t guaranteed. It’s better to handle this with error management or ensure through other checks that input can never be invalid.
Code Simplicity:
- Consider whether the conditional feature checks in imports (
#[cfg(feature = "alloc")]) may lead to duplicated logic, especially if different parts of the module have similar dependencies with differing feature requirements.
Overall:
- Address the panic/unwrap risk by implementing a more robust error-handling approach for hexadecimal parsing (e.g., propagating errors). Ensure field operations and conditional feature allocations do not open timing or zeroing vulnerabilities in cryptographic contexts. The absence of complete logic for operations makes a thorough assessment difficult, recommending further review focused on details of those methods/traits used extensively in the diff.
Kimi AI ReviewReview Feedback
|
Kimi AI ReviewHere are my specific comments on the provided PR diff:
Overall, the changes look good, and I don't see any immediate issues. The code is well-structured, follows the existing patterns, and includes comprehensive test cases. The update to the One minor suggestion: consider adding a comment explaining the change in the Let me know if you have any specific questions or need further clarification. I'm happy to help! |
3 participants
Add KoalaBear field (p = 2^31 - 2^24 + 1 = 2130706433) following Plonky3's implementation. Includes:
BETA = 3 is both a quadratic and quartic non-residue, making it the correct choice for the extension field (matching Plonky3's W=3).
The extension field supports basic arithmetic (add, mul, inv) but not FFT operations, as computing the primitive 2^26-th root of unity in F_{p^4} requires finding a multiplicative generator.
Acknowledgments: Plonky3 (Polygon) for field definition, RISC Zero for degree 4 inversion algorithm, lambdaworks BabyBear for implementation patterns.