clearmatics · AntoineRondelet · May 28, 2020 · May 18, 2020 · May 7, 2020 · May 7, 2020
diff --git a/.clang-tidy b/.clang-tidy
@@ -1,5 +1,3 @@
----
-
 #
 # Clang-tidy configuration for Zeth
 # This file follows the YAML syntax, see: https://yaml.org/spec/
@@ -27,7 +25,43 @@
 #   flexible. We consider this negligible compared to the benefits of
 #   manipulating variables that are explicitly typed.
 #
-Checks: >
+# -modernize-use-trailing-return-type,
+# -modernize-return-braced-init-list,
+#   These are removed since they conflict with the current code style.
+#
+# -modernize-pass-by-value
+# -modernize-deprecated-headers
+# -modernize-use-default-member-init
+# -readability-redundant-member-init
+# -bugprone-exception-escape
+#   These conflict with the current code style, or would require larger
+#   changes. May wish to review in the future.
+#
+# -readability-magic-numbers
+#   Disabled for now as it picks up some constants which don't necessarily need
+#   their own variables (for some trivial multiplication and masking) and some
+#   test data. May wish to review in the future.
+#
+# -readability-implicit-bool-conversion
+#   We use implicit bool conversion for specifying many constants in terms of
+#   `0` and `1`. (TODO: it may be a good idea to enable this to fix implicit
+#   conversion in conditions).
+#
+# -readability-named-parameter,
+#   We occasionally omit the name of a parameter in a function definition if the
+#   parameter is unused, and avoid a compiler warning. This check complains
+#   in these cases.
+#
+# -readability-identifier-naming
+#   TODO: Retry this (attempts to customize this to our standards failed under
+#   clang-tidy-10 - see CheckOptions below).
+#
+# -misc-non-private-member-variables-in-classes,
+#   TODO: Public member variables are often used in our code, and in
+#   dependencies. There is a good argument for not exposing these. Decide on a
+#   policy and update this comment.
+
+Checks:
     -*,
     bugprone-*,
     llvm-*,
@@ -38,30 +72,45 @@ Checks: >
     readability-*,
     -modernize-avoid-c-arrays,
     -modernize-use-bool-literals,
-    -modernize-use-auto
+    -modernize-use-auto,
+    -modernize-use-trailing-return-type,
+    -readability-magic-numbers,
+    -readability-implicit-bool-conversion,
+    -modernize-return-braced-init-list,
+    -readability-named-parameter,
+    -modernize-pass-by-value,
+    -modernize-use-default-member-init,
+    -bugprone-exception-escape,
+    -misc-non-private-member-variables-in-classes,
+    -readability-redundant-member-init,
+    -modernize-deprecated-headers,
+    -readability-identifier-naming,
+
+WarningsAsErrors:
+    '*'
 
 HeaderFilterRegex: 'libzeth/*.(h|hpp|cpp)'
 
-CheckOptions:
-    - key: readability-identifier-naming.MacroDefinitionCase
-      value: UPPER_CASE
-    - key: readability-identifier-naming.StaticConstantCase
-      value: UPPER_CASE
-    - key: readability-identifier-naming.ConstantParameterCase
-      value: lower_case
-    - key: readability-identifier-naming.FunctionCase
-      value: lower_case
-    - key: readability-identifier-naming.VariableCase
-      value: lower_case
-    - key: readability-identifier-naming.ClassCase
-      value: lower_case
-    - key: readability-identifier-naming.ClassMemberCase
-      value: lower_case
-    - key: readability-identifier-naming.NamespaceCase
-      value: lower_case
-    - key: readability-identifier-naming.TemplateParameterCase
-      value: CamelCase
-    - key: readability-identifier-naming.TemplateParameterSuffix
-      value: T
-    - key: readability-implicit-bool-conversion.AllowIntegerConditions
-      value: '1'
+# CheckOptions:
+#     - key: readability-identifier-naming.MacroDefinitionCase
+#       value: UPPER_CASE
+#     - key: readability-identifier-naming.StaticConstantCase
+#       value: UPPER_CASE
+#     - key: readability-identifier-naming.ConstantParameterCase
+#       value: lower_case
+#     - key: readability-identifier-naming.FunctionCase
+#       value: lower_case
+#     - key: readability-identifier-naming.VariableCase
+#       value: lower_case
+#     - key: readability-identifier-naming.ClassCase
+#       value: lower_case
+#     - key: readability-identifier-naming.ClassMemberCase
+#       value: lower_case
+#     - key: readability-identifier-naming.NamespaceCase
+#       value: lower_case
+#     - key: readability-identifier-naming.TemplateParameterCase
+#       value: CamelCase
+#     - key: readability-identifier-naming.TemplateParameterSuffix
+#       value: T
+#     - key: readability-implicit-bool-conversion.AllowIntegerConditions
+#       value: '1'
diff --git a/.cppcheck.suppressions b/.cppcheck.suppressions
@@ -0,0 +1,5 @@
+// Errors and Warnings to Suppress
+useStlAlgorithm:*
+
+// This raises some false positives, and inline suppressions seem not to work
+unusedFunction:*
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
@@ -60,3 +60,12 @@ jobs:
         submodules: recursive
     - name: Execute
       run: CI_USE_DOCKER=1 CI_CONFIG=Release CI_ZKSNARK=PGHR13 scripts/ci build
+
+  check-cpp-linux:
+    runs-on: ubuntu-18.04
+    steps:
+    - uses: actions/checkout@v1
+      with:
+        submodules: recursive
+    - name: Execute
+      run: CI_CHECK_FORMAT=1 CI_USE_DOCKER=1 scripts/ci check_cpp
diff --git a/cmake/clang_dev_tools.cmake b/cmake/clang_dev_tools.cmake
@@ -26,7 +26,6 @@ option(
 if(USE_CLANG_FORMAT)
   find_program(CLANG_FORMAT "clang-format")
   if(USE_CLANG_FORMAT)
-    message("clang-format found!")
     message("Using clang-format. Creating target... To run, use: make clang-format")
     add_custom_target(
       clang-format
@@ -42,15 +41,26 @@ endif()
 # The configuration below requires cmake 3.6.3+
 # (https://cmake.org/cmake/help/v3.6/variable/CMAKE_LANG_CLANG_TIDY.html)
 if(USE_CLANG_TIDY)
-  find_program(CLANG_TIDY clang-tidy DOC "Path to the clang-tidy tool")
+  find_program(CLANG_TIDY clang-tidy DOC "Path to clang-tidy tool")
   if(CLANG_TIDY)
-    message("clang-tidy found!")
-    message("Using clang-tidy. Creating target... To run, use: make clang-tidy (assumes run-clang-tidy.py has been fetched)")
-    add_custom_target(
-      clang-tidy
-      COMMAND python run-clang-tidy.py
-      WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
-    )
+    find_program(RUN_CLANG_TIDY run-clang-tidy.py DOC "Path to run-clang-tidy")
+    if(RUN_CLANG_TIDY)
+      message("Using clang-tidy. Creating target... To run, use: make clang-tidy")
+      add_custom_target(
+        clang-tidy
+        COMMAND run-clang-tidy.py
+        WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
+        )
+      file(MAKE_DIRECTORY ${PROJECT_BINARY_DIR}/api)
+      file(WRITE ${PROJECT_BINARY_DIR}/api/.clang-tidy
+        "Checks: '-*,misc-definitions-in-headers'
+CheckOptions:
+  - { key: HeaderFileExtensions,          value: \"x\" }")
+    else()
+      message(
+        FATAL_ERROR
+        "run-clang-tidy.py not found. (Download and place in PATH). Aborting...")
+    endif()
   else()
     message(FATAL_ERROR "clang-tidy not found. Aborting...")
   endif()
@@ -62,7 +72,6 @@ endif()
 if(USE_IWYU)
   find_program(IWYU include-what-you-use DOC "Path to the include-what-you-use tool")
   if(USE_IWYU)
-    message("IWYU found!")
     message("Using include-what-you-use...")
     set(CMAKE_CXX_INCLUDE_WHAT_YOU_USE "${IWYU};-Xiwyu;")
   else()

diff --git a/cmake/gcc_dev_tools.cmake b/cmake/gcc_dev_tools.cmake
@@ -13,16 +13,28 @@ option(
 # Adding clang-format target if executable is found
 if(USE_CPP_CHECK)
   find_program(CPP_CHECK "cppcheck")
-  if(USE_CPP_CHECK)
+  if(CPP_CHECK)
     message("cppcheck found!")
     message("Using cppcheck. Creating target... To run, use: make cppcheck")
+    # TODO: gtest includes are not found by cppcheck, because they are marked
+    # as "SYSTEM" (to prevent compile warnings). We exclude all tests for now,
+    # to avoid many "syntaxError" reports, but the analysis should be run on
+    # tests.
     add_custom_target(
       cppcheck
       COMMAND cppcheck
-      --project=compile_commands.json
+      --project=${CMAKE_CURRENT_BINARY_DIR}/compile_commands.json
       --enable=all
+      --suppressions-list=${PROJECT_SOURCE_DIR}/.cppcheck.suppressions
+      --inline-suppr
       --quiet
       -i${DEPENDS_DIR}
+      --suppress='*:${DEPENDS_DIR}/*'
+      -i${PROJECT_BINARY_DIR}/api
+      --suppress='*:${PROJECT_BINARY_DIR}/api/*'
+      --suppress='unmatchedSuppression:*'
+      --error-exitcode=1
+      --suppress='*:${PROJECT_SOURCE_DIR}/libzeth/tests/*'
     )
   else()
       message(FATAL_ERROR "cppcheck not found. Aborting...")

diff --git a/depends/.clang-tidy b/depends/.clang-tidy
@@ -0,0 +1,5 @@
+# Disable all checks.
+# (Dummy check here prevents error from clang-tidy)
+Checks: '-*,misc-definitions-in-headers'
+CheckOptions:
+  - { key: HeaderFileExtensions,          value: "x" }
diff --git a/libzeth/circuits/binary_operation.tcc b/libzeth/circuits/binary_operation.tcc
@@ -251,15 +251,10 @@ void double_bit32_sum_eq_gadget<FieldT>::generate_r1cs_constraints(
 template<typename FieldT>
 void double_bit32_sum_eq_gadget<FieldT>::generate_r1cs_witness()
 {
-    bits32 a_bits32;
-    bits32 b_bits32;
-    for (size_t i = 0; i < 32; i++) {
-        a_bits32[i] = a.get_bits(this->pb)[i];
-        b_bits32[i] = b.get_bits(this->pb)[i];
-    }
-
+    bits32 a_bits32 = bits32::from_vector(a.get_bits(this->pb));
+    bits32 b_bits32 = bits32::from_vector(b.get_bits(this->pb));
     bits32 left_side_acc = bits_add<32>(a_bits32, b_bits32, false);
-    res.fill_with_bits(this->pb, bits32_to_vector(left_side_acc));
+    left_side_acc.fill_variable_array(this->pb, res);
 }
 
 } // namespace libzeth

diff --git a/libzeth/circuits/blake2s/blake2s.hpp b/libzeth/circuits/blake2s/blake2s.hpp
@@ -25,66 +25,67 @@ template<typename FieldT> class BLAKE2s_256 : public libsnark::gadget<FieldT>
 {
 private:
     // Parameter block, size set to 32 bytes, fanout and depth set to serial
-    // mode. See: Section 2.8 https://blake2.net/blake2.pdf Table 2
-    std::array<std::array<bool, BLAKE2s_word_size>, 8> parameter_block = {
+    // mode. See: Section 2.8 https://blake2.net/blake2.pdf Table 2. Note, byte
+    // endianness has been swapped to avoid doing this repeatedly at runtime.
+    const std::array<const bits<BLAKE2s_word_size>, 8> parameter_block = {
         {{
              // Digest byte length, Key byte length, Fanout, Depth
-             0, 0, 1, 0, 0, 0, 0, 0, // 0x20 (32 bytes)
-             0, 0, 0, 0, 0, 0, 0, 0, // 0x00 (key length)
+             0, 0, 0, 0, 0, 0, 0, 1, // 0x01 (depth 1)
              0, 0, 0, 0, 0, 0, 0, 1, // 0x01 (fanout 1)
-             0, 0, 0, 0, 0, 0, 0, 1  // 0x01 (depth 1)
+             0, 0, 0, 0, 0, 0, 0, 0, // 0x00 (key length)
+             0, 0, 1, 0, 0, 0, 0, 0, // 0x20 (32 bytes)
          },
          {
              // Leaf length
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
-             0, 0, 0, 0, 0, 0, 0, 0  // 00
+             0, 0, 0, 0, 0, 0, 0, 0, // 00
          },
          {
              // Node offset
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
-             0, 0, 0, 0, 0, 0, 0, 0  // 00
+             0, 0, 0, 0, 0, 0, 0, 0, // 00
          },
          {
              // Node offset (cont.), Node depth, Inner length
+             0, 0, 0, 0, 0, 0, 0, 0, // 00 (inner length)
+             0, 0, 0, 0, 0, 0, 0, 0, // 00 (node depth)
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
-             0, 0, 0, 0, 0, 0, 0, 0, // 00 (node depth)
-             0, 0, 0, 0, 0, 0, 0, 0  // 00 (inner length)
          },
          {
              // Salt
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
-             0, 0, 0, 0, 0, 0, 0, 0  // 00
+             0, 0, 0, 0, 0, 0, 0, 0, // 00
          },
          {
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
-             0, 0, 0, 0, 0, 0, 0, 0  // 00
+             0, 0, 0, 0, 0, 0, 0, 0, // 00
          },
          {
              // Personalization
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
-             0, 0, 0, 0, 0, 0, 0, 0  // 00
+             0, 0, 0, 0, 0, 0, 0, 0, // 00
          },
          {
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
              0, 0, 0, 0, 0, 0, 0, 0, // 00
-             0, 0, 0, 0, 0, 0, 0, 0  // 00
+             0, 0, 0, 0, 0, 0, 0, 0, // 00
          }}};
 
     // See: Appendix A.2 of https://blake2.net/blake2.pdf for the specification
     // of the IV used in BLAKE2s
-    std::array<std::array<bool, BLAKE2s_word_size>, 8> BLAKE2s_IV = {
+    std::array<bits<BLAKE2s_word_size>, 8> BLAKE2s_IV = {
         {{
              0, 1, 1, 0, 1, 0, 1, 0, // 6A
              0, 0, 0, 0, 1, 0, 0, 1, // 09

diff --git a/libzeth/circuits/blake2s/blake2s.tcc b/libzeth/circuits/blake2s/blake2s.tcc
@@ -99,10 +99,9 @@ template<typename FieldT> void BLAKE2s_256<FieldT>::generate_r1cs_witness()
     // See: Appendix A.1 of https://blake2.net/blake2.pdf
     std::vector<bool> h_bits;
     for (size_t i = 0; i < 8; i++) {
-        std::array<bool, BLAKE2s_word_size> pb_swapped =
-            swap_byte_endianness(parameter_block[i]);
+        bits<BLAKE2s_word_size> pb_swapped = parameter_block[i];
         std::vector<bool> h_part =
-            bits32_to_vector(bits_xor(pb_swapped, BLAKE2s_IV[i]));
+            bits_xor(pb_swapped, BLAKE2s_IV[i]).to_vector();
         h_bits.insert(h_bits.end(), h_part.begin(), h_part.end());
     }
     h[0].bits.fill_with_bits(this->pb, h_bits);

diff --git a/libzeth/circuits/blake2s/blake2s_comp.hpp b/libzeth/circuits/blake2s/blake2s_comp.hpp
@@ -35,14 +35,14 @@ class BLAKE2s_256_comp : public libsnark::gadget<FieldT>
 private:
     // See: Appendix A.2 of https://blake2.net/blake2.pdf for the specification
     // of the IV used in BLAKE2s
-    static const std::array<std::array<bool, BLAKE2s_word_size>, 8> BLAKE2s_IV;
+    static const std::array<bits<BLAKE2s_word_size>, 8> BLAKE2s_IV;
 
     // Section 2.1 of https://blake2.net/blake2.pdf specifies that BLAKE2s has
     // 10 rounds
     static const int rounds = 10;
 
     // Low and High words of the offset
-    std::array<std::array<bool, BLAKE2s_word_size>, 2> t;
+    std::array<bits<BLAKE2s_word_size>, 2> t;
 
     // Chaining values
     libsnark::digest_variable<FieldT> h;