Cleanup and fix modular integer constructors

NilFoundation · Nov 13, 2024 · 95daa6b · 95daa6b
1 parent 34de455
commit 95daa6b
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 52 deletions.
diff --git a/...3/libs/multiprecision/include/nil/crypto3/multiprecision/big_integer/big_integer_impl.hpp b/...3/libs/multiprecision/include/nil/crypto3/multiprecision/big_integer/big_integer_impl.hpp
@@ -27,9 +27,6 @@
 #include "nil/crypto3/multiprecision/big_integer/detail/intel_intrinsics.hpp"
 #include "nil/crypto3/multiprecision/big_integer/storage.hpp"
 
-// TODO(ioxid): boost is used for
-// boost::hash_combine
-
 namespace nil::crypto3::multiprecision {
     template<std::size_t Bits>
     class big_integer;
@@ -159,7 +156,6 @@ namespace nil::crypto3::multiprecision {
 
         inline constexpr big_integer(const char* str) { *this = str; }
 
-        // TODO(ioxid): forbid signed, implement comparison with signed instead
         template<class T,
                  std::enable_if_t<std::is_integral_v<T> /*&& std::is_unsigned_v<T>*/, int> = 0>
         inline constexpr big_integer(T val) noexcept {
@@ -195,7 +191,6 @@ namespace nil::crypto3::multiprecision {
             return *this;
         }
 
-        // TODO(ioxid): forbid signed, implement comparison with signed instead
         template<typename T,
                  std::enable_if_t<std::is_integral_v<T> /*&& std::is_unsigned_v<T>*/, int> = 0>
         inline constexpr big_integer& operator=(T val) noexcept {
@@ -278,12 +273,11 @@ namespace nil::crypto3::multiprecision {
             return 0;
         }
 
-        // TODO(ioxid): make them private?
-
         // Arithmetic operations
 
         // Addition/subtraction
 
+      private:
         static inline constexpr void add_constexpr(big_integer& result, const big_integer& a,
                                                    const big_integer& b) noexcept {
             //
@@ -317,7 +311,7 @@ namespace nil::crypto3::multiprecision {
                 carry >>= big_integer::limb_bits;
                 ++pr, ++pa, ++pb;
             }
-            if (Bits % big_integer::limb_bits == 0) {
+            if constexpr (Bits % big_integer::limb_bits == 0) {
                 result.set_carry(carry);
             } else {
                 limb_type mask = big_integer::upper_limb_mask;
@@ -363,6 +357,7 @@ namespace nil::crypto3::multiprecision {
             NIL_CO3_MP_ASSERT(0 == borrow);
         }
 
+      public:
 #ifdef NIL_CO3_MP_HAS_IMMINTRIN_H
         //
         // This is the key addition routine:
@@ -436,7 +431,7 @@ namespace nil::crypto3::multiprecision {
                     std::copy(pa + i, pa + x, pr + i);
                 }
 
-                if (Bits % big_integer::limb_bits == 0) {
+                if constexpr (Bits % big_integer::limb_bits == 0) {
                     result.set_carry(carry);
                 } else {
                     limb_type mask = big_integer::upper_limb_mask;
@@ -530,7 +525,7 @@ namespace nil::crypto3::multiprecision {
             if (&a != &result) {
                 std::copy(pa + i, pa + a.size(), pr + i);
             }
-            if (Bits % big_integer::limb_bits == 0) {
+            if constexpr (Bits % big_integer::limb_bits == 0) {
                 result.set_carry(carry);
             } else {
                 limb_type mask = big_integer::upper_limb_mask;
@@ -1253,7 +1248,6 @@ namespace nil::crypto3::multiprecision {
              typename largest_t =                                                                 \
                  big_integer<std::max(detail::get_bits<T1>(), detail::get_bits<T2>())>>
 
-    // TODO(ioxid): somehow error on overflow
 #define NIL_CO3_MP_BIG_INTEGER_INTEGRAL_ASSIGNMENT_TEMPLATE                                        \
     template<typename big_integer_t, typename T,                                                   \
              std::enable_if_t<detail::is_big_integer_v<big_integer_t> && detail::is_integral_v<T>, \
@@ -1280,7 +1274,6 @@ namespace nil::crypto3::multiprecision {
     NIL_CO3_MP_BIG_INTEGER_IMPL_OPERATOR(==)
     NIL_CO3_MP_BIG_INTEGER_IMPL_OPERATOR(!=)
 
-    // TODO(ioxid): implement comparison with signed types, needed for boost::random
 #undef NIL_CO3_MP_BIG_INTEGER_IMPL_OPERATOR
 
     // Arithmetic operations

diff --git a/...ision/include/nil/crypto3/multiprecision/big_integer/modular/modular_big_integer_impl.hpp b/...ision/include/nil/crypto3/multiprecision/big_integer/modular/modular_big_integer_impl.hpp
@@ -23,7 +23,6 @@
 #include "nil/crypto3/multiprecision/big_integer/detail/assert.hpp"
 #include "nil/crypto3/multiprecision/big_integer/modular/modular_ops.hpp"
 #include "nil/crypto3/multiprecision/big_integer/modular/modular_ops_storage.hpp"
-#include "nil/crypto3/multiprecision/big_integer/storage.hpp"
 
 namespace nil::crypto3::multiprecision {
     namespace detail {
@@ -83,7 +82,6 @@ namespace nil::crypto3::multiprecision {
           protected:
             modular_ops_storage_t m_modular_ops_storage;
 
-            // TODO(ioxid): separate type for montgomery form to make the form explicit
             big_integer_t m_raw_base;
         };
     }  // namespace detail
@@ -106,23 +104,17 @@ namespace nil::crypto3::multiprecision {
             this->ops().adjust_modular(this->m_raw_base, b);
         }
 
-        // TODO(ioxid): this is buggy, need to remove it asap
-        // A method for converting a signed integer to a modular adaptor. We are not supposed to
+        // A method for converting a signed integer to a modular adaptor.
+        // TODO: We are not supposed to
         // have this, but in the code we already have conversion for an 'int' into modular type.
         // In the future we must remove.
         template<typename SI,
                  typename std::enable_if_t<std::is_integral_v<SI> && std::is_signed_v<SI>, int> = 0>
-        constexpr modular_big_integer_ct_impl(SI b) : base_type(big_integer_t(0u), {}) {
-            if (b >= 0) {
-                this->m_raw_base = static_cast<std::make_unsigned_t<SI>>(b);
-            } else {
-                this->m_raw_base = this->mod();
-                // TODO(ioxid): should work not just with limb_type, and this does not really
-                // work (m_base may underflow)
-                this->m_raw_base -= static_cast<detail::limb_type>(-b);
+        constexpr modular_big_integer_ct_impl(SI b)
+            : base_type(big_integer<sizeof(SI) * CHAR_BIT>(b), {}) {
+            if (b < 0) {
+                this->negate();
             }
-
-            this->ops().adjust_modular(this->m_raw_base);
         }
 
         template<typename UI, typename std::enable_if_t<
@@ -149,17 +141,10 @@ namespace nil::crypto3::multiprecision {
         template<typename SI,
                  std::enable_if_t<std::is_integral_v<SI> && std::is_signed_v<SI>, int> = 0>
         constexpr modular_big_integer_rt_impl(SI b, const big_integer_t& m)
-            : base_type(big_integer_t(0u), m) {
-            if (b >= 0) {
-                this->m_raw_base = b;
-            } else {
-                this->m_raw_base = this->mod();
-                // TODO(ioxid): should work not just with limb_type, and this does not really
-                // work (m_base may underflow)
-                this->m_raw_base -= static_cast<detail::limb_type>(-b);
+            : base_type(big_integer<sizeof(SI) * CHAR_BIT>(std::abs(b)), m) {
+            if (b < 0) {
+                this->negate();
             }
-
-            this->ops().adjust_modular(this->m_raw_base);
         }
 
         template<std::size_t Bits2>
@@ -207,7 +192,6 @@ namespace nil::crypto3::multiprecision {
 
     // Comparison
 
-// TODO(ioxid): comparison with big_integer and basic types (including signed)
 #define NIL_CO3_MP_MODULAR_BIG_INTEGER_COMPARISON_IMPL(op)                 \
     template<typename T1, typename T2,                                     \
              std::enable_if_t<detail::is_modular_big_integer_v<T1> &&      \

diff --git a/...ibs/multiprecision/include/nil/crypto3/multiprecision/big_integer/modular/modular_ops.hpp b/...ibs/multiprecision/include/nil/crypto3/multiprecision/big_integer/modular/modular_ops.hpp
@@ -299,13 +299,15 @@ namespace nil::crypto3::multiprecision::detail {
             m_no_carry_montgomery_mul_allowed = is_applicable_for_no_carry_montgomery_mul();
         }
 
-        // TODO(ioxid): no exception actually
         /*
          * Compute -input^-1 mod 2^limb_bits. Throws an exception if input
          * is even. If input is odd, then input and 2^n are relatively prime
          * and an inverse exists.
          */
         constexpr limb_type monty_inverse(const limb_type &a) {
+            if (a % 2 == 0) {
+                throw std::invalid_argument("inverse does not exist");
+            }
             limb_type b = 1;
             limb_type r = 0;
 
@@ -350,15 +352,11 @@ namespace nil::crypto3::multiprecision::detail {
                 accum += prod;
             }
             accum >>= this->m_mod.size() * limb_bits;
-            // TODO(ioxid): true?
-            // We cannot use -= for numbers of difference sizes, so resizing
-            // m_mod.
-            big_integer_doubled_padded_limbs large_mod = this->m_mod;
-            if (accum >= large_mod) {
-                accum -= large_mod;
+
+            if (accum >= this->m_mod) {
+                accum -= this->m_mod;
             }
-            // Here only the bytes that fit in sizeof result will be copied, and that's
-            // intentional.
+
             result = accum;
         }
 

diff --git a/crypto3/libs/multiprecision/include/nil/crypto3/multiprecision/big_integer/ops/ressol.hpp b/crypto3/libs/multiprecision/include/nil/crypto3/multiprecision/big_integer/ops/ressol.hpp
@@ -111,7 +111,7 @@ namespace nil::crypto3::multiprecision {
                 ++i;
 
                 if (i >= s) {
-                    // TODO(ioxid): when can this happen?
+                    // TODO(ioxid): when can this happen? (jacobi said that this should not happen)
                     throw std::invalid_argument("Not a quadratic residue");
                 }
             }

diff --git a/...libs/multiprecision/include/nil/crypto3/multiprecision/big_integer/signed_big_integer.hpp b/...libs/multiprecision/include/nil/crypto3/multiprecision/big_integer/signed_big_integer.hpp
@@ -12,9 +12,6 @@
 #include "nil/crypto3/multiprecision/big_integer/detail/assert.hpp"
 #include "nil/crypto3/multiprecision/big_integer/detail/config.hpp"
 
-// TODO(ioxid): boost is used for
-// boost::hash_combine
-
 namespace nil::crypto3::multiprecision {
     template<std::size_t Bits_>
     class signed_big_integer {
@@ -267,7 +264,6 @@ namespace nil::crypto3::multiprecision {
              typename largest_t =                                                                  \
                  signed_big_integer<std::max(detail::get_bits<T1>(), detail::get_bits<T2>())>>
 
-    // TODO(ioxid): somehow error on overflow
 #define NIL_CO3_MP_SIGNED_BIG_INTEGER_INTEGRAL_ASSIGNMENT_TEMPLATE                     \
     template<typename signed_big_integer_t, typename T,                                \
              std::enable_if_t<detail::is_signed_big_integer_v<signed_big_integer_t> && \
@@ -295,7 +291,6 @@ namespace nil::crypto3::multiprecision {
     NIL_CO3_MP_SIGNED_BIG_INTEGER_IMPL_OPERATOR(==)
     NIL_CO3_MP_SIGNED_BIG_INTEGER_IMPL_OPERATOR(!=)
 
-    // TODO(ioxid): implement comparison with signed types, needed for boost::random
 #undef NIL_CO3_MP_SIGNED_BIG_INTEGER_IMPL_OPERATOR
 
     // Arithmetic operations