Make pippenger_scratch_size and pippenger_max_points more precise by taking correct

alignment padding into account instead of assuming the maximum padding. This also allows
to precisely test pippenger_scratch_size against what is actually allocated.

Author: jonasnick

src/ecmult_impl.h

@@ -982,18 +982,51 @@ SECP256K1_INLINE static void secp256k1_ecmult_endo_split(secp256k1_scalar *s1, s
 }
 #endif
 
+
+static size_t secp256k1_pippenger_scratch_size_constant(int bucket_window) {
+    size_t size = 0;
+    size += ROUND_TO_ALIGN(sizeof(struct secp256k1_pippenger_state));
+    size += ROUND_TO_ALIGN(sizeof(secp256k1_gej) << bucket_window);
+    return size;
+}
+
+static SECP256K1_INLINE size_t secp256k1_pippenger_entries(size_t n_points) {
+#ifdef USE_ENDOMORPHISM
+    return 2*n_points + 2;
+#else
+    return n_points + 1;
+#endif
+}
+
+/**
+ * Returns the scratch size required for a given number of points excluding
+ * base point G and excluding the parts not dependent on the number of points.
+ * If called with 0 n_points it'll return the size required for the base point
+ * G. If the aligned argument is 0, this function will not take alignment into
+ * account. Otherwise it will align the individual parts in the same way as
+ * secp256k1_scratch_alloc.
+ */
+static size_t secp256k1_pippenger_scratch_size_points(size_t n_points, int bucket_window, int aligned) {
+    size_t entries = secp256k1_pippenger_entries(n_points);
+    size_t size = 0;
+    size += entries * sizeof(secp256k1_ge);
+    size = aligned ? ROUND_TO_ALIGN(size) : size;
+    size += entries * sizeof(secp256k1_scalar);
+    size = aligned ? ROUND_TO_ALIGN(size) : size;
+    size += entries * sizeof(struct secp256k1_pippenger_point_state);
+    size = aligned ? ROUND_TO_ALIGN(size) : size;
+    size += entries * WNAF_SIZE(bucket_window+1) * sizeof(int);
+    size = aligned ? ROUND_TO_ALIGN(size) : size;
+    return size;
+}
+
 /**
  * Returns the scratch size required for a given number of points (excluding
  * base point G) without considering alignment.
  */
 static size_t secp256k1_pippenger_scratch_size(size_t n_points, int bucket_window) {
-#ifdef USE_ENDOMORPHISM
-    size_t entries = 2*n_points + 2;
-#else
-    size_t entries = n_points + 1;
-#endif
-    size_t entry_size = sizeof(secp256k1_ge) + sizeof(secp256k1_scalar) + sizeof(struct secp256k1_pippenger_point_state) + (WNAF_SIZE(bucket_window+1)+1)*sizeof(int);
-    return (sizeof(secp256k1_gej) << bucket_window) + sizeof(struct secp256k1_pippenger_state) + entries * entry_size;
+    return secp256k1_pippenger_scratch_size_constant(bucket_window)
+            + secp256k1_pippenger_scratch_size_points(n_points, bucket_window, 1);
 }
 
 static int secp256k1_ecmult_pippenger_batch_allocate(const secp256k1_callback* error_callback, secp256k1_scratch *scratch, size_t scratch_checkpoint, size_t entries, int bucket_window, secp256k1_ge **points, secp256k1_scalar **scalars, secp256k1_gej **buckets, struct secp256k1_pippenger_state **state_space) {
@@ -1019,11 +1052,7 @@ static int secp256k1_ecmult_pippenger_batch(const secp256k1_callback* error_call
     /* Use 2(n+1) with the endomorphism, n+1 without, when calculating batch
      * sizes. The reason for +1 is that we add the G scalar to the list of
      * other scalars. */
-#ifdef USE_ENDOMORPHISM
-    size_t entries = 2*n_points + 2;
-#else
-    size_t entries = n_points + 1;
-#endif
+    size_t entries = secp256k1_pippenger_entries(n_points);
     secp256k1_ge *points;
     secp256k1_scalar *scalars;
     secp256k1_gej *buckets;
@@ -1095,27 +1124,39 @@ static int secp256k1_ecmult_pippenger_batch_single(const secp256k1_callback* err
  * used.
  */
 static size_t secp256k1_pippenger_max_points(const secp256k1_callback* error_callback, secp256k1_scratch *scratch) {
-    size_t max_alloc = secp256k1_scratch_max_allocation(error_callback, scratch, PIPPENGER_SCRATCH_OBJECTS);
+    /* Call max_allocation with 0 objects because it would assume worst case
+     * padding but in this function we want to get an exact number (and
+     * pippenger_scratch_size_points will already account for alignment). */
+    size_t max_alloc = secp256k1_scratch_max_allocation(error_callback, scratch, 0);
     int bucket_window;
     size_t res = 0;
 
     for (bucket_window = 1; bucket_window <= PIPPENGER_MAX_BUCKET_WINDOW; bucket_window++) {
         size_t n_points;
         size_t max_points = secp256k1_pippenger_bucket_window_inv(bucket_window);
         size_t space_for_points;
-        size_t space_overhead;
-        size_t entry_size = sizeof(secp256k1_ge) + sizeof(secp256k1_scalar) + sizeof(struct secp256k1_pippenger_point_state) + (WNAF_SIZE(bucket_window+1)+1)*sizeof(int);
+        size_t space_constant;
+        size_t entry_size = secp256k1_pippenger_scratch_size_points(0, bucket_window, 0);
 
-#ifdef USE_ENDOMORPHISM
-        entry_size = 2*entry_size;
-#endif
-        space_overhead = (sizeof(secp256k1_gej) << bucket_window) + entry_size + sizeof(struct secp256k1_pippenger_state);
-        if (space_overhead > max_alloc) {
+        space_constant = secp256k1_pippenger_scratch_size_constant(bucket_window);
+        if (space_constant + entry_size > max_alloc) {
             break;
         }
-        space_for_points = max_alloc - space_overhead;
+        space_for_points = max_alloc - space_constant;
+
+        /* Compute an upper bound for the number of points after subtracting
+         * space for the base point G. It's an upper bound because alignment is
+         * not taken into account. */
+        n_points = (space_for_points - entry_size)/entry_size;
+        if (n_points > 0
+              && space_for_points < secp256k1_pippenger_scratch_size_points(n_points, bucket_window, 1)) {
+            /* If there's not enough space after alignment is taken into
+             * account it suffices to decrease n_points by one. This is because
+             * the maximum padding required is less than an entry. */
+            n_points -= 1;
+            VERIFY_CHECK(space_for_points >= secp256k1_pippenger_scratch_size_points(n_points, bucket_window, 1));
+        }
 
-        n_points = space_for_points/entry_size;
         n_points = n_points > max_points ? max_points : n_points;
         if (n_points > res) {
             res = n_points;

src/tests.c

@@ -2961,18 +2961,20 @@ void test_secp256k1_pippenger_bucket_window_inv(void) {
 
 /**
  * Probabilistically test the function returning the maximum number of possible points
- * for a given scratch space.
+ * for a given scratch space. This works by trying various different scratch sizes and
+ * checking that the resulting max_points actually fit into the scratch space.
  */
 void test_ecmult_multi_pippenger_max_points(void) {
     size_t scratch_size = secp256k1_rand_int(256);
-    size_t max_size = secp256k1_pippenger_scratch_size(secp256k1_pippenger_bucket_window_inv(PIPPENGER_MAX_BUCKET_WINDOW-1)+512, 12);
+    /* Pick a max scratch size that permits using enough points to require the
+     * maximum bucket window */
+    size_t max_points = secp256k1_pippenger_bucket_window_inv(PIPPENGER_MAX_BUCKET_WINDOW-1)+512;
+    size_t max_size = secp256k1_pippenger_scratch_size(max_points, PIPPENGER_MAX_BUCKET_WINDOW);
     secp256k1_scratch *scratch;
     size_t n_points_supported;
     int bucket_window = 0;
 
-    for(; scratch_size < max_size; scratch_size+=256) {
-        size_t i;
-        size_t total_alloc;
+    for(; scratch_size < max_size; scratch_size += 32) {
         size_t checkpoint;
         scratch = secp256k1_scratch_create(&ctx->error_callback, scratch_size);
         CHECK(scratch != NULL);
@@ -2983,13 +2985,16 @@ void test_ecmult_multi_pippenger_max_points(void) {
             continue;
         }
         bucket_window = secp256k1_pippenger_bucket_window(n_points_supported);
-        /* allocate `total_alloc` bytes over `PIPPENGER_SCRATCH_OBJECTS` many allocations */
-        total_alloc = secp256k1_pippenger_scratch_size(n_points_supported, bucket_window);
-        for (i = 0; i < PIPPENGER_SCRATCH_OBJECTS - 1; i++) {
-            CHECK(secp256k1_scratch_alloc(&ctx->error_callback, scratch, 1));
-            total_alloc--;
+        {
+            /* Check that pippenger_scratch_size matches what's actually allocated */
+            secp256k1_ge *points;
+            secp256k1_scalar *scalars;
+            secp256k1_gej *buckets;
+            struct secp256k1_pippenger_state *state_space;
+            size_t entries = secp256k1_pippenger_entries(n_points_supported);
+            CHECK(secp256k1_ecmult_pippenger_batch_allocate(&ctx->error_callback, scratch, 0, entries, bucket_window, &points, &scalars, &buckets, &state_space));
+            CHECK(scratch->alloc_size == secp256k1_pippenger_scratch_size(n_points_supported, bucket_window));
         }
-        CHECK(secp256k1_scratch_alloc(&ctx->error_callback, scratch, total_alloc));
         secp256k1_scratch_apply_checkpoint(&ctx->error_callback, scratch, checkpoint);
         secp256k1_scratch_destroy(&ctx->error_callback, scratch);
     }
@@ -3087,7 +3092,7 @@ void test_ecmult_multi_batching(void) {
     /* Test with space for 1 point in pippenger. That's not enough because
      * ecmult_multi selects strauss which requires more memory. It should
      * therefore select the simple algorithm. */
-    scratch = secp256k1_scratch_create(&ctx->error_callback, secp256k1_pippenger_scratch_size(1, 1) + PIPPENGER_SCRATCH_OBJECTS*ALIGNMENT);
+    scratch = secp256k1_scratch_create(&ctx->error_callback, secp256k1_pippenger_scratch_size(1, 1));
     CHECK(secp256k1_ecmult_multi_var(&ctx->error_callback, &ctx->ecmult_ctx, scratch, &r, &scG, ecmult_multi_callback, &data, n_points));
     secp256k1_gej_add_var(&r, &r, &r2, NULL);
     CHECK(secp256k1_gej_is_infinity(&r));
@@ -3097,7 +3102,7 @@ void test_ecmult_multi_batching(void) {
         if (i >= ECMULT_PIPPENGER_THRESHOLD) {
             int bucket_window = secp256k1_pippenger_bucket_window(i);
             size_t scratch_size = secp256k1_pippenger_scratch_size(i, bucket_window);
-            scratch = secp256k1_scratch_create(&ctx->error_callback, scratch_size + PIPPENGER_SCRATCH_OBJECTS*ALIGNMENT);
+            scratch = secp256k1_scratch_create(&ctx->error_callback, scratch_size);
         } else {
             size_t scratch_size = secp256k1_strauss_scratch_size(i);
             scratch = secp256k1_scratch_create(&ctx->error_callback, scratch_size);