Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function to compute optimal ecmult_multi scratch size for a number of points #638

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jonasnick
Copy link
Contributor

@jonasnick jonasnick commented Jun 12, 2019

@DavidBurkett requested to allow computing the optimal scratch size for Schnorr batch verification (BlockstreamResearch/secp256k1-zkp#69). This PR is a prerequisite but also contains a bunch of other fixups.

Other than adding the new function this PR refactors scratch space handling in ecmult_impl to improve code quality, tests and documentation.

The biggest part of this PR is to make computing the scratch size and its inverse more precise by not assuming maximum padding when aligning, but rather using the actual padding. This is not strictly necessary but removes a leaky abstraction and makes testing easier.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(My review is best viewed commit by commit.)

src/tests.c Outdated
@@ -3094,7 +3094,7 @@ void test_ecmult_multi_batching(void) {
secp256k1_scratch_destroy(&ctx->error_callback, scratch);

for(i = 1; i <= n_points; i++) {
if (i > ECMULT_PIPPENGER_THRESHOLD) {
if (i >= ECMULT_PIPPENGER_THRESHOLD) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

state_space->wnaf_na = (int *) secp256k1_scratch_alloc(error_callback, scratch, entries*(WNAF_SIZE(bucket_window+1)) * sizeof(int));
buckets = (secp256k1_gej *) secp256k1_scratch_alloc(error_callback, scratch, (1<<bucket_window) * sizeof(*buckets));
state_space->wnaf_na = (int *) secp256k1_scratch_alloc(error_callback, scratch, entries * WNAF_SIZE(bucket_window+1) * sizeof(int));
buckets = (secp256k1_gej *) secp256k1_scratch_alloc(error_callback, scratch, sizeof(*buckets) << bucket_window);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

src/util.h Outdated
@@ -93,7 +93,7 @@ static SECP256K1_INLINE void *checked_realloc(const secp256k1_callback* cb, void
#define ALIGNMENT 16
#endif

#define ROUND_TO_ALIGN(size) (((size + ALIGNMENT - 1) / ALIGNMENT) * ALIGNMENT)
#define ROUND_TO_ALIGN(size) ((((size) + ALIGNMENT - 1) / ALIGNMENT) * ALIGNMENT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

return secp256k1_pippenger_scratch_size(n_points, bucket_window);
} else {
return secp256k1_strauss_scratch_size(n_points);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's an approach ACK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I guess it's a concept ack. I was confused by other meanings of approach :D

Copy link
Contributor

@real-or-random real-or-random Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actively testing bitcoin/bitcoin#16149 here

edit: except that I just write "ACK". All my "ACK"s in this review mean "ACK thorough code inspection"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No action needed) Responding to this 2 year old comment 😅

In Core, it looks like "Approach ACK" means "Concept ACK, and I agree with the approach of this change (but I haven't reviewed the code in detail)":

https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#conceptual-review

(*state_space)->wnaf_na = (int *) secp256k1_scratch_alloc(error_callback, scratch, entries * WNAF_SIZE(bucket_window+1) * sizeof(int));
*buckets = (secp256k1_gej *) secp256k1_scratch_alloc(error_callback, scratch, sizeof(secp256k1_gej) << bucket_window);
if ((*state_space)->ps == NULL || (*state_space)->wnaf_na == NULL || *buckets == NULL) {
secp256k1_scratch_apply_checkpoint(error_callback, scratch, scratch_checkpoint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK
I think it's cleaner to apply the checkpoint outside this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

#endif
return n_points*point_size;
size += ROUND_TO_ALIGN(n_points * sizeof(struct secp256k1_strauss_point_state));
return size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm somewhat unsure about this. It seems like a layer violation to care about the alignment here.

Copy link
Contributor Author

@jonasnick jonasnick Jun 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to have a function that returns the required scratch space given a number of points (and we should) then we need to add the padding somewhere. While we can assume the worst case padding somewhere else I would prefer to have the *_scratch_space function return exact results. This makes it much easier to think about and also helps testing because now we can just check that what is allocated actually matches what we computed with *_scratch_space (see 24553bf#diff-4655d106bf03045a3a50beefc800db21R2996). Or do you have an alternative in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added function alloc_size to scratch space

return secp256k1_scratch_max_allocation(error_callback, scratch, STRAUSS_SCRATCH_OBJECTS) / secp256k1_strauss_scratch_size(1);
/* Call max_allocation with 0 objects because we've already accounted for
* alignment in strauss_scratch_size. */
return secp256k1_scratch_max_allocation(error_callback, scratch, 0) / secp256k1_strauss_scratch_size(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and wasn't the previous version (without changes in strauss_scratch_size) more precise?
We need to round up to the alignment only once per array (e.g., once for the scalars array).

In the proposed revision, I think we overestimate the required padding a lot because we still call strauss_scratch_size(1) here but this has padding now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct - sorry I overlooked this. Will add fix and test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* 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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion with @jonasnick, one of the things I'm not sure about is the added complexity in this function.
On the one hand, this function is accurate now and users of the function can rely on that.
On the other hand, if we just call secp256k1_scratch_max_allocation with PIPPENGER_SCRATCH_OBJECTS instead of 0, we may return a value that is one too small. That's not terrible for performance but it potentially makes the function a little bit harder to use and test because you may need to remember that it is not accurate.

Copy link
Contributor Author

@jonasnick jonasnick Jun 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like both hands are arguments in favor of the change (i.e. calling secp256k1_scratch_max_allocation with 0).

We need to do that for strauss anyway because otherwise

n_points == strauss_max_points(..., scratch_create(strauss_scratch_size(n_points)))

wouldn't hold.

@jonasnick jonasnick force-pushed the ecmult-scratch branch 2 times, most recently from 872a17b to cbe3cc7 Compare June 16, 2019 19:45
@jonasnick
Copy link
Contributor Author

I made a couple of changes and in order to avoid adding code that is deleted in later commits I force pushed, sorry. Summary of the changes:

  • added function alloc_size to scratch space to compute actual size allocated for a given number of objects
  • fixed bug in strauss_max_points (thanks @real-or-random) that vastly underestimated the number of points actually fitting into the scratch space. Also added test which would have caught this issue.
  • added a verify check to ensure that the space required for a single point/entry is smaller than the worst case padding


n_points = space_for_points/entry_size;
n_points = (space_for_points - entry_size)/entry_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? It's equivalent to space_for_points / entry_size - 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is right. Simplified the line according to your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assignment to n_points (along with the comment) redundant with above?

@sipa
Copy link
Contributor

sipa commented Jul 23, 2019

Concept ACK, I still need to go over the logic changes.

@real-or-random
Copy link
Contributor

needs rebase

@jonasnick
Copy link
Contributor Author

Rebased and polished quite a bit. Also added fix for bug in master that we noticed before iirc. So to make sure it gets in I opened #1004.

Still, I didn't fully try to understand how this PR works. Also, it seems like ecmult_multi_scratch_size doesn't give the exact optimal result. That's because a scratch space of size pippenger_scratch_size(n_points, bucket_window) it may happen that strauss_max_points(error_callback, scratch), n) (the actual batch size) is smaller than n_points.

Copy link
Contributor

@robot-dreams robot-dreams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

size_t sum = 0;

for (i = 0; i < n_sizes; i++) {
sum += ROUND_TO_ALIGN(sizes[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The existing secp256k1_scratch_max_allocation seems very careful about checking for overflow. For consistency, is it necessary to do the same here? For example:

// Check for overflow
if (sum + ROUND_TO_ALIGN(sizes[i]) < sum) {
    return 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I fixed this. Also added test.

src/ecmult_impl.h Outdated Show resolved Hide resolved

n_points = space_for_points/entry_size;
n_points = (space_for_points - entry_size)/entry_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assignment to n_points (along with the comment) redundant with above?

* Returns the maximum number of points in addition to G that can be used with
* a given scratch space. The function ensures that fewer points may also be
* used.
/* Returns the (near) maximum number of points in addition to G that can be
Copy link
Contributor

@robot-dreams robot-dreams Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you already know how it might fail to be a maximum? (No worries if not, I still want to revisit these details carefully.)

Edit: Could this fail to be a maximum because the constant space used by the buckets decreases when you jump to the next bucket window size?

Copy link
Contributor

@robot-dreams robot-dreams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

It's unfortunate that:

  • Padding / alignment adds so much complexity to these calculations
  • Pippenger sizes have this weird non-monotonic behavior

But I still think your change makes sense.

My only general feedback is that updating the scratch space usage would involve keeping a lot of different things in sync, similar to what @real-or-random mentioned at #1004 (comment). Is there a way to refactor or add comments to make the task easier in the future (e.g. by sharing code between scratch_size_raw and batch_allocate)?

}

static int secp256k1_ecmult_pippenger_batch(const secp256k1_callback* error_callback, secp256k1_scratch *scratch, secp256k1_gej *r, const secp256k1_scalar *inp_g_sc, secp256k1_ecmult_multi_callback cb, void *cbdata, size_t n_points, size_t cb_offset) {
const size_t scratch_checkpoint = secp256k1_scratch_checkpoint(error_callback, scratch);
/* Use 2(n+1) with the endomorphism, when calculating batch
* sizes. The reason for +1 is that we add the G scalar to the list of
* other scalars. */
size_t entries = 2*n_points + 2;
size_t entries = secp256k1_pippenger_entries(n_points);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should the comment go above the definition of the function secp256k1_pippenger_entries instead?

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit (feel free to ignore): Was this check previously here for avoiding underflow (rather than short-circuiting)? If so would it make sense to keep the check as space_constant > max_alloc to make the intent clear?

void test_ecmult_multi_strauss_max_points(void) {
size_t scratch_size = secp256k1_strauss_scratch_size_raw(1, 0);
size_t max_scratch_size = secp256k1_strauss_scratch_size_raw(1, 1) + 1;
for (; scratch_size < max_scratch_size; scratch_size++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to check bigger scratch_size (so that n_points is bigger too), but increase the amount scratch_size is incremented on each iteration?

secp256k1_scratch *scratch = secp256k1_scratch_create(&ctx->error_callback, scratch_size);
size_t n_points = secp256k1_strauss_max_points(&ctx->error_callback, scratch);
CHECK(secp256k1_scratch_max_allocation(&ctx->error_callback, scratch, 0) == scratch_size);
CHECK(scratch_size >= secp256k1_strauss_scratch_size(n_points));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to also check that the result is exact, e.g. by adding a check like this:

CHECK(scratch_size < secp256k1_strauss_scratch_size(n_points + 1));

return secp256k1_pippenger_scratch_size(n_points, bucket_window);
} else {
return secp256k1_strauss_scratch_size(n_points);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No action needed) Responding to this 2 year old comment 😅

In Core, it looks like "Approach ACK" means "Concept ACK, and I agree with the approach of this change (but I haven't reviewed the code in detail)":

https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#conceptual-review

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;
/* Compute entry size without taking alignment into account */
size_t entry_size = secp256k1_pippenger_scratch_size_points(0, bucket_window, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit (feel free to ignore): Should this be called point_size instead to avoid confusion, since in other places you get 2(n+1) entries from the endomorphism?

Take actual alignment into account instead of assuming worst case.
This improves test because it can be checked that *_scratch_size matches
what is actually allocated.
Take actual alignment into account instead of assuming the worst case. This
allows more precise tests for strauss, because if a scratch space has exactly
strauss_scratch_size(n_points) left, then secp256k1_strauss_max_points(cb,
scratch) = n_points.
@jonasnick
Copy link
Contributor Author

I rebased this to see how master affects this PR. Will still need to address review comments and add better explanations to the commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants