rangeproof: add secp256k1_rangeproof_max_size function to estimate rangeproof size

[apoelstra]

Provides a method that will give an upper bound on the size of a rangeproof,
given an upper bound on the value to be passed in and an upper bound on the
min_bits parameter.

There is a lot of design freedom here since the actual size of the rangeproof
depends on every parameter passed to rangeproof_sign, including the value to
be proven, often in quite intricate ways. For the sake of simplicity we assume
a nonzero min_value and that exp will be 0 (the default, and size-maximizing,
choice), and provide an exact value for a proof of the given value and min_bits.

[jonasnick]

Looks good overall. I created a branch on top of the additional tests of #160 and added more rangeproof_max_size tests. Some of those tests fail: it seems like max_size returns the wrong result when min_bits is 0. The branch is here (also includes a small fixup).

[real-or-random]

I think this would make the text clearer:

Suggested change

	* To obtain the size of largest possible proof, set `min_value` to 1 and
	* `max_value` to UINT64_MAX.
	* To obtain an upper bound that is valid for all values of `min_value`
	* and `max_value`, set `min_value` to 1 and `max_value` to UINT64_MAX.

[real-or-random]

ah no, this function doesn't even have a min_value arg :P

[apoelstra]

Oops, it originally did, which would save a few bytes when min_value was 0 ... but then I realized that sometimes rangeproof_sign would override the user's choice of 0 if exp was also set, so I gave up on it.

BTW @jonasnick I wonder if this addresses your "sometimes wrong values when min_value is 0" observation?

[apoelstra]

@real-or-random I will remove the reference to min_value but I think the existing text "size of the largest possible proof" is correct and what we want to say.

An "upper bound" is too weak, and it seems weird to mention min_value and max_value without mentioning ct_bits or exp or any of the other parameters to rangeproof_sign which could affect the size.

[jonasnick]

@apoelstra I wrote max_value but I meant min_bits (it's right in the branch)

[real-or-random]

Ok, how it is implied? I don't see it. Shouldn't the upper limit of the range be independent of value? I mean that's the entire point of a rangeproof?

Sorry, I must be missing something, I guess I'm having a mental block here.... ^^

[apoelstra]

It's not independent because gmax's code will increase the range if the value would otherwise be outside of it.

[real-or-random]

Oh I see. That was totally unclear to me from the docs. I think we should clarify this in the docs of rangeproof_sign, ideally by describing max_value as mathematical function of value and min_bits...

Also this is unclear to the reader in the docs.
min_bits: Number of bits of the value to keep private. (0 = auto/minimal, - 64).

• which bits? most significant or least significant?
• what's "auto/minimal" supposed to do?

But this can be done in another PR.

[apoelstra]

That was totally unclear to me from the docs.

Lol. We could add something to rangeproof_sign to the effect of "other than value and blind basically every value may be overridden by the signing logic, in ad-hoc ways, if the proof otherwise would be impossible to construct.

ideally by describing max_value as mathematical function of value and min_bits

I mean, this function is a somewhat arbitrary choice of function here. It won't match the behavior of rangeproof_sign because rangeproof_sign also takes its other arguments into account.

which bits? most significant or least significant?

All of them. If value is too large the code just increases min_bits, unless exp is -1 (IIRC) in which case it will create a single-value proof.

In general I think it's a fool's errand to try to document the existing rangeproof signer logic (it may be feasible to just rip it out, respect the user's params, and refuse to sign if they're incompatible, but it's quite hard to communicate what needs to be adjusted, which I think is why gmax made the choices he did). What I need right now is a way for Elements to swag the size of a rangeproof, for fee estimation, given that it knows the user's choice of min_bits and that any output value will be <= MAX_MONEY.

[real-or-random]

ideally by describing max_value as mathematical function of value and min_bits

I mean, this function is a somewhat arbitrary choice of function here. It won't match the behavior of rangeproof_sign because rangeproof_sign also takes its other arguments into account.

Ah I see. Sorry, I misunderstood. I assumed the max value in rangeproof_sign only depends on these two arguments because you said "it's implied by the union of value and min_bits".

Anyway, this PR here is fine. I think the docs of rangeproof_sign can be improved but if anything, that's a separate issue.

[apoelstra]

Pushed to address @real-or-random's comments.

Ideally we could merge #160 so that I could just cherry-pick @jonasnick's commits... as is I'm not sure where the bug is and it's a bit of a PITA for me to find it.

Edit ok, I'm just working directly on his branch while I investigate. This is astonishing -- my function returns 106 for an upper bound while the actual returned plen is 5000.

[apoelstra]

@jonasnick actually I think your tests are wrong.

The first CHECK that you've commented out fails because you're doing a check after rangeproof_sign failed (and when it fails it doesn't set plen at all). The second CHECK that you've commented out actually passes for me when I uncomment it.

[jonasnick]

facepalm sorry! I fixed my commit in the branch (feel free to cherry-pick) https://github.com/jonasnick/secp256k1-zkp/tree/2022-01--rangeproof-cleanups-jn

[apoelstra]

I would love to cherry-pick but the commits don't apply cleanly without also bringing #160 in

[jonasnick]

Only the first two commits of #160 which only add tests and seem uncontroversial. But I'm fine with this PR either way.

[apoelstra]

Ok, I took your three commits (one of which is a cherry-pick from 160). Will push them when my local tests finish, probably 10-15 minutes.

[apoelstra]

Pushed. You can verify with git range-diff that they match your branch (or better, that they match #160). Note that the range-diff will show a bunch of removed memcmp->secp256k1_memcmp_var changes; this is because #160 is out of date. These changes were included in #198.

[apoelstra]

Updated only to change the docs as @real-or-random suggested.

[real-or-random]

tACK 6b6ced9

[apoelstra]

@jonasnick ok to merge?

[jonasnick]

ACK 6b6ced9