From 10d5def0692cc7929bc71c70d62885ab59ef55f5 Mon Sep 17 00:00:00 2001
From: Ben Edgington <ben@benjaminion.xyz>
Date: Wed, 3 Feb 2021 21:00:14 +0000
Subject: [PATCH] Refactor in preparation for next steps

---
 src/Makefile                      | 10 ++--
 src/blst_util.c                   | 47 +++++++++++++++++
 src/blst_util.h                   | 27 ++++++++++
 src/blst_util_test.c              | 83 +++++++++++++++++++++++++++++++
 src/{c-kzg.h => c_kzg.h}          |  0
 src/{test_util.c => debug_util.c} |  9 +---
 src/{test_util.h => debug_util.h} |  3 +-
 src/fft_fr.h                      |  2 +-
 src/fft_fr_test.c                 |  2 +-
 src/fft_g1.c                      | 12 -----
 src/fft_g1.h                      |  4 +-
 src/fft_g1_test.c                 | 33 +-----------
 src/fft_util.c                    | 15 +-----
 src/fft_util.h                    |  9 +---
 src/fft_util_test.c               | 37 +++-----------
 15 files changed, 180 insertions(+), 113 deletions(-)
 create mode 100644 src/blst_util.c
 create mode 100644 src/blst_util.h
 create mode 100644 src/blst_util_test.c
 rename src/{c-kzg.h => c_kzg.h} (100%)
 rename src/{test_util.c => debug_util.c} (90%)
 rename src/{test_util.h => debug_util.h} (93%)

diff --git a/src/Makefile b/src/Makefile
index 05ee27d..695b0bc 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -1,19 +1,19 @@
-TESTS = fft_util_test fft_fr_test fft_g1_test
-LIB_SRC = fft_util.c fft_fr.c fft_g1.c
+TESTS = blst_util_test fft_util_test fft_fr_test fft_g1_test
+LIB_SRC = blst_util.c fft_util.c fft_fr.c fft_g1.c
 LIB_OBJ = $(LIB_SRC:.c=.o)
 
 CFLAGS =
 
 .PRECIOUS: %.o
 
-%.o: %.c %.h c-kzg.h Makefile
+%.o: %.c %.h c_kzg.h Makefile
 	clang -Wall $(CFLAGS) -c $*.c
 
 libckzg.a: $(LIB_OBJ) Makefile
 	ar rc libckzg.a $(LIB_OBJ)
 
-%_test: %_test.c test_util.o libckzg.a Makefile
-	clang -Wall $(CFLAGS) -o $@ $@.c test_util.o libckzg.a -L../lib -lblst
+%_test: %_test.c debug_util.o libckzg.a Makefile
+	clang -Wall $(CFLAGS) -o $@ $@.c debug_util.o libckzg.a -L../lib -lblst
 	./$@
 
 lib: clean libckzg.a
diff --git a/src/blst_util.c b/src/blst_util.c
new file mode 100644
index 0000000..480531a
--- /dev/null
+++ b/src/blst_util.c
@@ -0,0 +1,47 @@
+/*
+ * Copyright 2021 Benjamin Edgington
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "blst_util.h"
+
+bool fr_is_one(const blst_fr *fr_p) {
+    uint64_t a[4];
+    blst_uint64_from_fr(a, fr_p);
+    return a[0] == 1 && a[1] == 0 && a[2] == 0 && a[3] == 0;
+}
+
+void fr_from_uint64(blst_fr *a, uint64_t n) {
+    uint64_t vals[] = {n, 0, 0, 0};
+    blst_fr_from_uint64(a, vals);
+}
+
+bool fr_equal(blst_fr *aa, blst_fr *bb) {
+    uint64_t a[4], b[4];
+    blst_uint64_from_fr(a, aa);
+    blst_uint64_from_fr(b, bb);
+    return a[0] == b[0] && a[1] == b[1] && a[2] == b[2] && a[3] == b[3];
+}
+
+void p1_mul(blst_p1 *out, const blst_p1 *a, const blst_fr *b) {
+    blst_scalar s;
+    blst_scalar_from_fr(&s, b);
+    blst_p1_mult(out, a, s.b, 8 * sizeof(blst_scalar));
+}
+
+void p1_sub(blst_p1 *out, const blst_p1 *a, const blst_p1 *b) {
+    blst_p1 bneg = *b;
+    blst_p1_cneg(&bneg, true);
+    blst_p1_add_or_double(out, a, &bneg);
+}
diff --git a/src/blst_util.h b/src/blst_util.h
new file mode 100644
index 0000000..be8c9cf
--- /dev/null
+++ b/src/blst_util.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright 2021 Benjamin Edgington
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "c_kzg.h"
+
+// This is 1 in Blst's `blst_fr` limb representation. Crazy but true.
+static const blst_fr one =
+    {0x00000001fffffffeL, 0x5884b7fa00034802L, 0x998c4fefecbc4ff5L, 0x1824b159acc5056fL};
+
+bool fr_is_one(const blst_fr *fr_p);
+void fr_from_uint64(blst_fr *a, uint64_t n);
+bool fr_equal(blst_fr *aa, blst_fr *bb);
+void p1_mul(blst_p1 *out, const blst_p1 *a, const blst_fr *b);
+void p1_sub(blst_p1 *out, const blst_p1 *a, const blst_p1 *b);
diff --git a/src/blst_util_test.c b/src/blst_util_test.c
new file mode 100644
index 0000000..4d41d1e
--- /dev/null
+++ b/src/blst_util_test.c
@@ -0,0 +1,83 @@
+/*
+ * Copyright 2021 Benjamin Edgington
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "../inc/acutest.h"
+#include "debug_util.h"
+#include "blst_util.h"
+
+void fr_is_one_works(void) {
+    TEST_CHECK(true == fr_is_one(&one));
+}
+
+void fr_from_uint64_works(void) {
+    blst_fr a;
+    fr_from_uint64(&a, 1);
+    TEST_CHECK(true == fr_is_one(&a));
+}
+
+void fr_equal_works(void) {
+    // A couple of arbitrary roots of unity
+    uint64_t aa[] = {0x0001000000000000L, 0xec03000276030000L, 0x8d51ccce760304d0L, 0x0000000000000000L};
+    uint64_t bb[] = {0x8dd702cb688bc087L, 0xa032824078eaa4feL, 0xa733b23a98ca5b22L, 0x3f96405d25a31660L};
+    blst_fr a, b;
+    blst_fr_from_uint64(&a, aa);
+    blst_fr_from_uint64(&b, bb);
+    TEST_CHECK(true == fr_equal(&a, &a));
+    TEST_CHECK(false == fr_equal(&a, &b));
+}
+
+void p1_mul_works(void) {
+    // This is -1 (the second root of unity)
+    uint64_t m1[] = {0xffffffff00000000L, 0x53bda402fffe5bfeL, 0x3339d80809a1d805L, 0x73eda753299d7d48L};
+    blst_fr minus1;
+    blst_p1 g1_gen, g1_gen_neg, res;
+
+    // Multiply the generator by minus one (the second root of unity)
+    blst_p1_from_affine(&g1_gen, &BLS12_381_G1);
+    blst_fr_from_uint64(&minus1, m1);
+    p1_mul(&res, &g1_gen, &minus1);
+
+    // We should end up with negative the generator
+    blst_p1_from_affine(&g1_gen_neg, &BLS12_381_NEG_G1);
+
+    TEST_CHECK(blst_p1_is_equal(&res, &g1_gen_neg));
+}
+
+void p1_sub_works(void) {
+    blst_p1 g1_gen, g1_gen_neg;
+    blst_p1 tmp, res;
+
+    blst_p1_from_affine(&g1_gen, &BLS12_381_G1);
+    blst_p1_from_affine(&g1_gen_neg, &BLS12_381_NEG_G1);
+
+    // 2 * g1_gen = g1_gen - g1_gen_neg
+    blst_p1_double(&tmp, &g1_gen);
+    p1_sub(&res, &g1_gen, &g1_gen_neg);
+
+    TEST_CHECK(blst_p1_is_equal(&tmp, &res));
+}
+
+
+
+TEST_LIST =
+    {
+     {"fr_is_one_works", fr_is_one_works },
+     {"fr_from_uint64_works", fr_from_uint64_works},
+     {"fr_equal_works", fr_equal_works},
+     {"p1_mul_works", p1_mul_works},
+     {"p1_sub_works", p1_sub_works},
+     { NULL, NULL }     /* zero record marks the end of the list */
+    };
diff --git a/src/c-kzg.h b/src/c_kzg.h
similarity index 100%
rename from src/c-kzg.h
rename to src/c_kzg.h
diff --git a/src/test_util.c b/src/debug_util.c
similarity index 90%
rename from src/test_util.c
rename to src/debug_util.c
index a8e1c48..4e4c1b4 100644
--- a/src/test_util.c
+++ b/src/debug_util.c
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "test_util.h"
+#include "debug_util.h"
 
 //
 // General Utilities
@@ -45,13 +45,6 @@ void print_fr(const blst_fr *a) {
     print_bytes_as_hex_le(b.b, 0, 32);
 }
 
-bool fr_equal(blst_fr *aa, blst_fr *bb) {
-    uint64_t a[4], b[4];
-    blst_uint64_from_fr(a, aa);
-    blst_uint64_from_fr(b, bb);
-    return a[0] == b[0] && a[1] == b[1] && a[2] == b[2] && a[3] == b[3];
-}
-
 //
 // G1 and G2 utilities
 //
diff --git a/src/test_util.h b/src/debug_util.h
similarity index 93%
rename from src/test_util.h
rename to src/debug_util.h
index 44036c3..4d7fbc0 100644
--- a/src/test_util.h
+++ b/src/debug_util.h
@@ -16,7 +16,7 @@
 
 #include <stdlib.h>
 #include <stdio.h>
-#include "../inc/blst.h"
+#include "c_kzg.h"
 
 // General Utilities
 void print_bytes_as_hex(byte *bytes, int start, int len);
@@ -24,7 +24,6 @@ void print_bytes_as_hex_le(byte *bytes, int start, int len);
 
 // Fr utilities
 void print_fr(const blst_fr *a);
-bool fr_equal(blst_fr *aa, blst_fr *bb);
 
 // G1 and G2 utilities
 void print_p1_bytes(byte p1[96]);
diff --git a/src/fft_fr.h b/src/fft_fr.h
index 03cb147..292729f 100644
--- a/src/fft_fr.h
+++ b/src/fft_fr.h
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include "c-kzg.h"
+#include "c_kzg.h"
 #include "fft_util.h"
 
 void fft_fr_slow(blst_fr *out, blst_fr *in, uint64_t stride, blst_fr *roots, uint64_t roots_stride, uint64_t l);
diff --git a/src/fft_fr_test.c b/src/fft_fr_test.c
index d186493..236ecd8 100644
--- a/src/fft_fr_test.c
+++ b/src/fft_fr_test.c
@@ -15,7 +15,7 @@
  */
 
 #include "../inc/acutest.h"
-#include "test_util.h"
+#include "debug_util.h"
 #include "fft_fr.h"
 
 const uint64_t inv_fft_expected[][4] =
diff --git a/src/fft_g1.c b/src/fft_g1.c
index afed84f..2228626 100644
--- a/src/fft_g1.c
+++ b/src/fft_g1.c
@@ -16,18 +16,6 @@
 
 #include "fft_g1.h"
 
-void p1_mul(blst_p1 *out, const blst_p1 *a, const blst_fr *b) {
-    blst_scalar s;
-    blst_scalar_from_fr(&s, b);
-    blst_p1_mult(out, a, s.b, 8 * sizeof(blst_scalar));
-}
-
-void p1_sub(blst_p1 *out, const blst_p1 *a, const blst_p1 *b) {
-    blst_p1 bneg = *b;
-    blst_p1_cneg(&bneg, true);
-    blst_p1_add_or_double(out, a, &bneg);
-}
-
 // Slow Fourier Transform (simple, good for small sizes)
 void fft_g1_slow(blst_p1 *out, blst_p1 *in, uint64_t stride, blst_fr *roots, uint64_t roots_stride, uint64_t l) {
     blst_p1 v, last, jv;
diff --git a/src/fft_g1.h b/src/fft_g1.h
index dde2bda..5419305 100644
--- a/src/fft_g1.h
+++ b/src/fft_g1.h
@@ -14,11 +14,9 @@
  * limitations under the License.
  */
 
-#include "c-kzg.h"
+#include "c_kzg.h"
 #include "fft_util.h"
 
-void p1_mul(blst_p1 *out, const blst_p1 *a, const blst_fr *b);
-void p1_sub(blst_p1 *out, const blst_p1 *a, const blst_p1 *b);
 void fft_g1_slow(blst_p1 *out, blst_p1 *in, uint64_t stride, blst_fr *roots, uint64_t roots_stride, uint64_t l);
 void fft_g1_fast(blst_p1 *out, blst_p1 *in, uint64_t stride, blst_fr *roots, uint64_t roots_stride, uint64_t l);
 C_KZG_RET fft_g1 (blst_p1 *out, blst_p1 *in, FFTSettings *fs, bool inv, uint64_t n);
diff --git a/src/fft_g1_test.c b/src/fft_g1_test.c
index 1dadb40..52e7432 100644
--- a/src/fft_g1_test.c
+++ b/src/fft_g1_test.c
@@ -15,7 +15,7 @@
  */
 
 #include "../inc/acutest.h"
-#include "test_util.h"
+#include "debug_util.h"
 #include "fft_g1.h"
 
 void make_data(blst_p1 *out, uint64_t n) {
@@ -27,35 +27,6 @@ void make_data(blst_p1 *out, uint64_t n) {
     }
 }
 
-void p1_mul_works(void) {
-    blst_fr minus1;
-    blst_p1 g1_gen, g1_gen_neg, res;
-
-    // Multiply the generator by minus one (the second root of unity)
-    blst_p1_from_affine(&g1_gen, &BLS12_381_G1);
-    blst_fr_from_uint64(&minus1, scale2_root_of_unity[1]);
-    p1_mul(&res, &g1_gen, &minus1);
-
-    // We should end up with negative the generator
-    blst_p1_from_affine(&g1_gen_neg, &BLS12_381_NEG_G1);
-
-    TEST_CHECK(blst_p1_is_equal(&res, &g1_gen_neg));
-}
-
-void p1_sub_works(void) {
-    blst_p1 g1_gen, g1_gen_neg;
-    blst_p1 tmp, res;
-
-    blst_p1_from_affine(&g1_gen, &BLS12_381_G1);
-    blst_p1_from_affine(&g1_gen_neg, &BLS12_381_NEG_G1);
-
-    // 2 * g1_gen = g1_gen - g1_gen_neg
-    blst_p1_double(&tmp, &g1_gen);
-    p1_sub(&res, &g1_gen, &g1_gen_neg);
-
-    TEST_CHECK(blst_p1_is_equal(&tmp, &res));
-}
-
 void compare_sft_fft(void) {
     // Initialise: arbitrary size
     unsigned int size = 6;
@@ -99,8 +70,6 @@ void roundtrip_fft(void) {
 
 TEST_LIST =
     {
-     {"p1_mul_works", p1_mul_works},
-     {"p1_sub_works", p1_sub_works},
      {"compare_sft_fft", compare_sft_fft},
      {"roundtrip_fft", roundtrip_fft},
      { NULL, NULL }     /* zero record marks the end of the list */
diff --git a/src/fft_util.c b/src/fft_util.c
index 717fe46..c3961e1 100644
--- a/src/fft_util.c
+++ b/src/fft_util.c
@@ -16,32 +16,21 @@
 
 #include "fft_util.h"
 
-bool is_one(const blst_fr *fr_p) {
-    uint64_t a[4];
-    blst_uint64_from_fr(a, fr_p);
-    return a[0] == 1 && a[1] == 0 && a[2] == 0 && a[3] == 0;
-}
-
 bool is_power_of_two(uint64_t n) {
     return (n & (n - 1)) == 0;
 }
 
-void fr_from_uint64(blst_fr *a, uint64_t n) {
-    uint64_t vals[] = {n, 0, 0, 0};
-    blst_fr_from_uint64(a, vals);
-}
-
 // Create an array of powers of the root of unity
 // The `out` array must be of size `width + 1`
 C_KZG_RET expand_root_of_unity(blst_fr *roots, blst_fr *root_of_unity, uint64_t width) {
     roots[0] = one;
     roots[1] = *root_of_unity;
 
-    for (int i = 2; !is_one(&roots[i - 1]); i++) {
+    for (int i = 2; !fr_is_one(&roots[i - 1]); i++) {
         ASSERT(i <= width, C_KZG_ERROR);
         blst_fr_mul(&roots[i], &roots[i - 1], root_of_unity);
     }
-    ASSERT(is_one(&roots[width]), C_KZG_ERROR);
+    ASSERT(fr_is_one(&roots[width]), C_KZG_ERROR);
 
     return C_KZG_SUCCESS;
 }
diff --git a/src/fft_util.h b/src/fft_util.h
index ccec456..59723ef 100644
--- a/src/fft_util.h
+++ b/src/fft_util.h
@@ -15,11 +15,8 @@
  */
 
 #include <stdlib.h>
-#include "c-kzg.h"
-
-// This is 1 in Blst's `blst_fr` limb representation. Crazy but true.
-static const blst_fr one =
-    {0x00000001fffffffeL, 0x5884b7fa00034802L, 0x998c4fefecbc4ff5L, 0x1824b159acc5056fL};
+#include "c_kzg.h"
+#include "blst_util.h"
 
 // MODULUS = 52435875175126190479447740508185965837690552500527637822603658699938581184513
 // PRIMITIVE_ROOT = 5
@@ -69,9 +66,7 @@ typedef struct {
     blst_fr *reverse_roots_of_unity;
 } FFTSettings;
 
-bool is_one(const blst_fr *fr_p);
 bool is_power_of_two(uint64_t n);
-void fr_from_uint64(blst_fr *a, uint64_t n);
 C_KZG_RET expand_root_of_unity(blst_fr * roots, blst_fr *root_of_unity, uint64_t width);
 C_KZG_RET reverse(blst_fr *out, blst_fr *roots, uint64_t width);
 C_KZG_RET new_fft_settings(FFTSettings *s, unsigned int max_scale);
diff --git a/src/fft_util_test.c b/src/fft_util_test.c
index 7541e55..ea7776a 100644
--- a/src/fft_util_test.c
+++ b/src/fft_util_test.c
@@ -15,15 +15,11 @@
  */
 
 #include "../inc/acutest.h"
-#include "test_util.h"
+#include "debug_util.h"
 #include "fft_util.h"
 
 #define NUM_ROOTS 32
 
-void is_one_works(void) {
-    TEST_CHECK(true == is_one(&one));
-}
-
 void roots_of_unity_is_the_expected_size(void) {
     TEST_CHECK(NUM_ROOTS ==
                sizeof(scale2_root_of_unity) / sizeof(scale2_root_of_unity[0]));
@@ -36,7 +32,7 @@ void roots_of_unity_are_plausible(void) {
         for (unsigned int j = 0; j < i; j++) {
             blst_fr_sqr(&r, &r);
         }
-        TEST_CHECK(true == is_one(&r));
+        TEST_CHECK(true == fr_is_one(&r));
         TEST_MSG("Root %d failed", i);
     }
 }
@@ -58,9 +54,9 @@ void reverse_works(void) {
     // Verify - decreasing values
     for (int i = 0; i < n; i++) {
         blst_fr_sub(&diff, rev + i, rev + i + 1);
-        TEST_CHECK(true == is_one(&diff));
+        TEST_CHECK(true == fr_is_one(&diff));
     }
-    TEST_CHECK(true == is_one(rev + n));
+    TEST_CHECK(true == fr_is_one(rev + n));
 }
 
 void expand_roots_is_plausible(void) {
@@ -74,11 +70,11 @@ void expand_roots_is_plausible(void) {
     TEST_CHECK(expand_root_of_unity(expanded, &root, width) == C_KZG_SUCCESS);
 
     // Verify - each pair should multiply to one
-    TEST_CHECK(true == is_one(expanded + 0));
-    TEST_CHECK(true == is_one(expanded + width));
+    TEST_CHECK(true == fr_is_one(expanded + 0));
+    TEST_CHECK(true == fr_is_one(expanded + width));
     for (unsigned int i = 1; i <= width / 2; i++) {
         blst_fr_mul(&prod, expanded + i, expanded + width - i);
-        TEST_CHECK(true == is_one(&prod));
+        TEST_CHECK(true == fr_is_one(&prod));
     }
 }
 
@@ -94,7 +90,7 @@ void new_fft_settings_is_plausible(void) {
     // Verify - each pair should multiply to one
     for (unsigned int i = 1; i <= width; i++) {
         blst_fr_mul(&prod, s.expanded_roots_of_unity + i, s.reverse_roots_of_unity + i);
-        TEST_CHECK(true == is_one(&prod));
+        TEST_CHECK(true == fr_is_one(&prod));
     }
 
     free_fft_settings(&s);
@@ -115,30 +111,13 @@ void is_power_of_two_works(void) {
     TEST_CHECK(false == is_power_of_two(1234567));
 }
 
-void fr_from_uint64_works(void) {
-    blst_fr a;
-    fr_from_uint64(&a, 1);
-    TEST_CHECK(true == is_one(&a));
-}
-
-void fr_equal_works(void) {
-    blst_fr a, b;
-    blst_fr_from_uint64(&a, scale2_root_of_unity[15]);
-    blst_fr_from_uint64(&b, scale2_root_of_unity[16]);
-    TEST_CHECK(true == fr_equal(&a, &a));
-    TEST_CHECK(false == fr_equal(&a, &b));
-}
-
 TEST_LIST =
     {
-     {"is_one_works", is_one_works },
      {"roots_of_unity_is_the_expected_size", roots_of_unity_is_the_expected_size},
      {"roots_of_unity_are_plausible", roots_of_unity_are_plausible},
      {"reverse_works", reverse_works},
      {"expand_roots_is_plausible", expand_roots_is_plausible},
      {"new_fft_settings_is_plausible", new_fft_settings_is_plausible},
      {"is_power_of_two_works", is_power_of_two_works},
-     {"fr_from_uint64_works", fr_from_uint64_works},
-     {"fr_equal_works", fr_equal_works},
      { NULL, NULL }     /* zero record marks the end of the list */
     };