scratch space: use single allocation

[apoelstra]

No description provided.

[real-or-random]

Wouldn't all of that be easier if we just checked the canary only at top-level, i.e., in exposed API functions (like we use ARG_CHECK only in exposed functions?)

[apoelstra]

Addressed jonas' nits.

[apoelstra]

Addressed Tim's nits

[apoelstra]

Tim, regarding doing these checks only at the endpoints, the problem is that the user-level "endpoints" are everything that might use a scratch space, including everything that might use ecmult_multi. So this set is unbounded and it's something we'd have to maintain going forward.

This isn't a big deal; we already have this situation for e.g. context objects where every new function needs to ARG_CHECK that the appropriate precomp tables are initialized. But it's annoying and I think this situation is simpler/more maintainable.

[real-or-random]

Uh, I haven't looked at it from that point of view. Agreed, checking it deep in the call tree needs less duplicated code. It's is less consistent with what we do now for ARG_CHECKs though. But as you say, that's maybe because it was not the best design choice.

[apoelstra]

My view is that for the context stuff it's necessary to have so much duplication because different functions have different precomp requirements. In this case this is sanity checking that really shouldn't concern any code outside of the scratch space module.

[real-or-random]

It should be noted that this PR fixes the following bug (for which I had the description text in some open editor windows already but Andrew fixed it faster than I managed to report it. 👍 )

1. Create context ctx with default_error_callback
2. Create scratch space s with ctx.
3. Change error_callback of ctx to cb
4. Trigger a call to secp256k1_ecmult_multi_var(ctx,..., which will call scratch_space_allocate_frame. Assume checked_malloc fails.

Expected:
cb is called as error callback

Actual:
default_error_callback is called as error callback

[real-or-random]

I discussed this with @jonasnick: We're not sure what the point of the frames still is when there is only a single malloc now. Maybe there is one but it seems that everything that can be done with frames can be done with similar programming ergonomics without frames.

If we keep frames, then I think the internal API should be documented a little more because it's not immediately clear how to use it. In particular the wording is somewhat confusing: we "allocate" frames and "alloc(cate)" objects but then e.g., what does the "allocation" in "max_allocation" then give you? All of this is well-defined in the end but one needs to read the code to figure it out.

[jonasnick]

Needs rebase

[apoelstra]

Rebased; squashed fixups

[apoelstra]

Will revisit to remove the idea of "frames" entirely.

[jonasnick]

it seems that everything that can be done with frames can be done with similar programming ergonomics without frames.

It has the small advantage that you can deallocate the whole frame and not have to deallocate all objects one by one. However, that could be done without the api requiring to give the frame size in advance when allocating it. This seems to be unnecessary right now. But I don't recall the reason for "frames" either.

[apoelstra]

Rebased and replaced frames with "checkpoints" which don't require pre-registering an allocation size and don't have a limit to how many are usable.

[real-or-random]

LGTM

[apoelstra]

Rebased, fixed date/data typo, used ROUND_TO_ALIGN in place of manual alignment calculations.

[gmaxwell]

ACK