rangeproof: clean up legacy rangeproof code #160

apoelstra · 2022-01-04T22:13:38Z

This is the first round of cleanups with the goal of reducing the stack space usage of the rangeproof proving and verification code. This is needed for many embedded applications, and even on hosted machines it's a bit obscene that we're throwing tens of kilobytes of stuff onto the stack unnecessarily.

There is some stack reduction in this PR itself (dropping the 4k prep array from rangeproof_genrand_sign and the 1k k array from rangeproof_sign_impl but that isn't the main goal of this PR. The goal is to get the code into a state where it's easier to read and reason about.

This PR does not include the addition of closures or other complex APIs that might be controversial. It is a collection of cherry-picks from my work in November on stack space reduction, where I excluded everything I thought was "experimental" or unlikely to work out.

apoelstra · 2022-01-05T18:46:28Z

src/modules/rangeproof/rangeproof_impl.h

+                        tmp[b] ^= message[(i * 4 + j) * 32 + b];
+                    }
+                }
+            } else if (i == header->n_rings - 1) {


There is a change in signing behavior here, which is that we no longer encode data in the (potentially two) s-values in the final ring.

The tests don't catch this, and I hate that this behavior exists, but I think I should restore it so that there is no potential change in behavior. I will come up with a fixed test vector for this.

Actually, we are okay. In e7a8a5f (July 2016) I made it illegal (fails rangeproof_sign) to produce signatures that used these last two s values for sidechannel purposes, even though it looks like gmax did actually implement the logic for it in 2015.

So it has never been possible, using the libsecp-zkp API, to produce a rangeproof that embedded data in this space, so I'm comfortable removing the ability to as part of these cleanups.

Made this explicit in the new commit 0db19e5

real-or-random · 2022-01-06T11:55:48Z

Is this ready for review or are you working on further changes?

apoelstra · 2022-01-06T15:31:57Z

@real-or-random ready for review. I am building stuff on top of this, which seem to be working now (I eliminated all the massive arrays in rangeproof_rewind_inner :)) so I think this is the right foundation.

apoelstra · 2022-01-06T15:34:45Z

My current tentative roadmap:

This "cleanup"/testing PR
A followup which moves some rewinding logic into borromean_verify, decreases the speed of rewinding a fair bit (but speeds up failed rewinding by letting us bail out before doing any EC ops), but eliminates all the huge stack objects for rewinding
A followup which does the same for verification (all that is left is the pubs array)
A followup(s) which makes similar changes for rangeproof signing.
(Maybe) an overall followup which tries to do microoptimizations and other stuff, once everything else has settled out.

jonasnick

First (shallow) review pass. Nice to see all those cleanups!

src/modules/rangeproof/main_impl.h

src/modules/rangeproof/rangeproof_impl.h

src/modules/rangeproof/tests_impl.h

src/modules/rangeproof/rangeproof_impl.h

src/modules/rangeproof/rangeproof.h

src/modules/rangeproof/tests_impl.h

jonasnick

I haven't fully investigated this but

./tests 64 106275f9d56e923d4698c32cae3b1634

fails with

src/modules/rangeproof/tests_impl.h:517: test condition failed: secp256k1_rangeproof_sign(ctx, proof, &len, vmin, &commit, blind, commit.data, exp, min_bits, v, NULL, 0, NULL, 0, secp256k1_generator_h)

It seems like secp256k1_rangeproof_header_set_for_value fails because secp256k1_rangeproof_header_expand fails because header->max_value > UINT64_MAX - header->min_value.

jonasnick

Added three more fixed test vectors to my branch (https://github.com/jonasnick/secp256k1-zkp/commits/rangeproof-cleanups-jn). Feel free to cherry-pick (and modify).

apoelstra · 2022-01-12T18:44:41Z

Thanks! Pulled it into my branch.

jonasnick

I wonder about the implications of removing the 64-byte extra space for Elements.

Elements has this reject condition msg_size != SIDECHANNEL_MSG_SIZE where msg_size is the m_len argument of rewind, msg_size = 64 and SIDECHANNEL_MSG_SIZE = 64. If one creates a rangeproof with a single ring of size 4, non-updated nodes will get msg_size = 64 and won't reject the transaction. Updated nodes however will have msg_size = 0 and therefore reject the tx.

src/modules/rangeproof/rangeproof_impl.h

apoelstra · 2022-01-13T14:39:44Z

@jonasnick regarding Elements, that check is actually wrong right now because the returned msg_size from rangeproof_rewind is 64 bytes larger than the actual capacity available from rangeproof_sign.

It is true that currently wallets will accept 2-bit rangeproofs that have embedded messages, and this change will cause those to be rejected, but producing such rangeproofs would require a custom rangeproof signing implementation. I'm not aware of any such implementations, nor am I aware of 2-bit rangeproofs being used in practice.

There is a sorta-attack here where somebody could make such a rangeproof, have it be accepted by an Elements 22 wallet, then the user updates to Elements 23 and loses her backup and needs to recover the data from the rangeproof again, and now can't. But this is a really long chain of implausible scenarios, which results in a situation which is recoverable (with some extra crypto work from us) if anybody actually encounters it.

jonasnick

Needs rebase but LGTM. header_set_for_value makes sense to me but I can't fully verify that it's exactly equivalent to the proveparams function it replaces. Luckily the tests are quite comprehensive.

I've added another test to my branch to exercise the following condition in proveparams which wasn't hit by the existing tests:

        if ((*min_value && value > INT64_MAX) || (value && *min_value >= INT64_MAX))
            return 0;

See commit rangeproof: add test for large value and minimum value and fixup! rangeproof: use rangeproof_header structure for proving here.

apoelstra · 2022-01-17T15:26:37Z

Rebased and pulled both your commits into rangeproof: use rangeproof_header structure for proving

jonasnick

ACK d1a3479

real-or-random

only reviewed 3/10 commits so far

real-or-random · 2022-01-28T15:23:02Z

src/modules/rangeproof/tests_impl.h

+    CHECK(min_val_out == val);
+    CHECK(max_val_out == val);
+    CHECK(m_len_out == 0);
+    CHECK(memcmp(blind, blind_out, 32) == 0);


Suggested change

CHECK(memcmp(blind, blind_out, 32) == 0);

CHECK(secp256k1_memcmp_var(blind, blind_out, 32) == 0);

real-or-random · 2022-01-28T15:25:53Z

src/modules/rangeproof/tests_impl.h

+    unsigned char blind_out[32];
+    unsigned char nonce[32];
+    const unsigned char message[1] = " "; /* no message will fit into a single-value proof */
+    unsigned char message_out[sizeof(proof)] = { 0 };


nit: Maybe this could use a more arbitrary default value than 0. (It doesn't really matter for this test, but rather in the tests below)

By the way, nice use of information-theory here.

I'm going to leave this be -- I did consider using a nonzero value, but decided against it, because

0 is very easy to notice in debug output (though I suppose so would any constant value)

the failure mode in message decoding will be to output random-looking data (since rewinding extracts from a RNG and then xors the output). So it's hard to see how rewinding could fail in a way that would be confused with constant 0s

real-or-random · 2022-01-28T15:29:27Z

src/modules/rangeproof/tests_impl.h

+        secp256k1_generator_h
+    ));
+
+    CHECK(memcmp(blind, blind_1, 32) == 0);


Suggested change

CHECK(memcmp(blind, blind_1, 32) == 0);

CHECK(memcmp(blind, blind_1, 32) == 0);

I feel bad for point out this on many PR. Please tell me if I should stop. The point is that there's a plan upstream to check that the code is free of direct calls to memcmp. (There are more calls below, may be a good idea to grep for it.)

edit: In fact, it seems that we never replaced the memcmp in our non-test code here (equivalent to bitcoin-core/secp256k1@6173839). We should just do this.

real-or-random · 2022-01-28T15:38:46Z

src/modules/rangeproof/tests_impl.h

+};
+
+/* Maximum length of a message that can be embedded into a rangeproof */
+#define MAX_MESSAGE_LEN 3968


Nice! Can you document this in the API docs? It will solve #42.

real-or-random · 2022-01-28T15:55:00Z

src/modules/rangeproof/rangeproof_impl.h

    /* Like in the rsize[] == 1 case, Having figured out which s is the one which was not forged, we can recover the blinding factor. */
-    secp256k1_rangeproof_recover_x(&stmp, &s_orig[skip2], &ev[skip2], &s[skip2]);
+    final_x_pos = 4 * (rings - 1) + ((*v >> (2 * (rings - 1))) & 3);


Here you use *v instead of value. I believe the entire function is somewhat inconsistent here (even before your PR): Some uses of *v are covered by if (v) but *v = UINT64_MAX; and yours here is not. This made me think that this can be called with NULL but it cannot.

real-or-random

5/10...

real-or-random · 2022-02-01T16:25:20Z

src/modules/rangeproof/rangeproof.h

+    /** Number of bits used to represent the proven value
+     *
+     * Encoded in the header. */
+    int mantissa;


Say that this is 0 for exact value proofs?

real-or-random · 2022-02-01T16:54:18Z

src/modules/rangeproof/rangeproof_impl.h

-    VERIFY_CHECK(npub <= 128);
-    VERIFY_CHECK(npub >= 1);


I'm pretty sure this holds, but I can't be wrong to bring those back (to _expand then).

real-or-random · 2022-02-01T16:57:29Z

src/modules/rangeproof/rangeproof_impl.h

@@ -394,11 +482,10 @@ SECP256K1_INLINE static int secp256k1_rangeproof_rewind_inner(secp256k1_scalar *
        }
        return 1;
    }
-    npub = (rings - 1) << 2;


huh, re-setting npub was really bad if you want to read the code. good to remove this.

real-or-random · 2022-02-01T16:59:46Z

src/modules/rangeproof/rangeproof_impl.h

    for (j = 0; j < 2; j++) {
        size_t idx;
        /* Look for a value encoding in the last ring. */
-        idx = npub + rsizes[rings - 1] - 1 - j;
+        idx = 4 * (header->n_rings - 1) + header->rsizes[header->n_rings - 1] - 1 - j;


Suggested change

idx = 4 * (header->n_rings - 1) + header->rsizes[header->n_rings - 1] - 1 - j;

idx = header->n_pubs - 1 - j;

equivalent but cleaner

real-or-random

7/10

real-or-random · 2022-02-02T09:36:18Z

src/modules/rangeproof/rangeproof_impl.h

+     * the 2015-era API, and all of these issues will go away when we merge
+     * Bulletproofs. */


nit: I wouldn't make promises in comments. :)

I think this one is fine :) BPs simply have none of these insane options so there is no room for complex adjustment logic.

real-or-random · 2022-02-02T09:42:14Z

src/modules/rangeproof/rangeproof_impl.h

+ *
+ * Returns 1 on success, 0 if the `max_value` field would exceed UINT64_MAX */
+static int secp256k1_rangeproof_header_expand(secp256k1_rangeproof_header* header) {
+    if (header->mantissa == 0) {


nit: May be slightly cleaner check for header->exp == -1 instead (making this the "canonical" check: a proof is single-value iff header->exp == -1 and all others values are derived.)

real-or-random · 2022-02-02T09:51:58Z

src/modules/rangeproof/rangeproof_impl.h

-        if ((*min_value && value > INT64_MAX) || (value && *min_value >= INT64_MAX)) {
-            /* If either value or min_value is >= 2^63-1 then the other must by zero to avoid overflowing the proven range. */
-            return 0;


I can't find the equivalent check in the new code. (May be intentional but then please explain in the commit message or in a comment.)

I think the old check was just wrong, in multiple ways.

First, if min_value is greater than INT64_MAX or indeed greater than 0, then value must also be greater than 0, since it must be >= min_value. In the old code this was checked before even calling proveparams.

If this instead intended to check that min_value == value in the case that either one was > INT64_MAX, this still makes no sense. There is no reason I shouldn't be able to have min_value be INT64_MAX + 1 and value be INT64_MAX + 2, say.

real-or-random · 2022-02-02T10:01:35Z

src/modules/rangeproof/rangeproof_impl.h

+    if (!secp256k1_rangeproof_header_expand(header)) {
+        return 0;
+    }
+    *proven_value = (value - header->min_value) / header->scale;


I don't see how this is equivalent to the old code. I think the equivalent to the old code would be to divide by max and not by header->scale (see the different loop conditions in the old code that compute *v2 (= max) and *scale.

I think conceptually we could also first reexpand, which will compute header->max_value (=max, I think) then compute `proven_value and reexpand again? (Not a real suggestion, just to help me and you understand.)

Dividing by max is definitely wrong, and I don't think the old code does anything like that.

real-or-random · 2022-02-02T10:15:48Z

src/modules/rangeproof/rangeproof_impl.h

+    /* Increasing the mantissa will have increased the number of rings etc
+     * so re-expand the header to recompute the other derived values. */
+    return secp256k1_rangeproof_header_expand(header);


Also mention that we may have changed header->exp and header->min_value.

real-or-random · 2022-02-02T10:35:20Z

src/modules/rangeproof/rangeproof_impl.h

-        VERIFY_CHECK(*mantissa>0);
-        VERIFY_CHECK((*v & ~(UINT64_MAX>>(64-*mantissa))) == 0); /* Did this get all the bits? */


It may be worth to recycle these VERIFY_CHECKs

real-or-random · 2022-02-02T10:35:59Z

src/modules/rangeproof/rangeproof_impl.h

+    VERIFY_CHECK(header.n_rings > 0);
+    VERIFY_CHECK(header.n_rings <= 32);
+    VERIFY_CHECK(header.n_pubs <= 128);


Shouldn't these three be in the header_expand function? I think they holds for any valid proof (i.e., they won't fail in the verification code)

Sure. In fact, it looks like they were already there, and these VERIFY_CHECKs are redundant.

real-or-random · 2022-02-02T10:56:26Z

src/modules/rangeproof/rangeproof_impl.h

+        const unsigned int max_bits = min_value ? secp256k1_clz64_var(min_value) : 64;
+        if (min_bits > max_bits) {
+            header->mantissa = max_bits;
+            min_bits = max_bits;


Dead store.

You can leave min_bits const because you use header->mantissa below. Also update the comment above this scope to mention mantissa instead of min_bits.

Made this change. Hopefully the resulting range-diff isn't too messy (this was a pretty short patch so changing only a few lines pretty-much rewrote it).

real-or-random · 2022-02-04T15:50:47Z

src/modules/whitelist/main_impl.h

-            unsigned char nonce32[32];
-            int done;
-            ret = secp256k1_nonce_function_default(nonce32, msg32, seckey32, NULL, NULL, count);
-            if (!ret) {
-                break;
-            }
-            secp256k1_scalar_set_b32(&non, nonce32, &overflow);
-            memset(nonce32, 0, 32);
-            if (overflow || secp256k1_scalar_is_zero(&non)) {
-                count++;
-                continue;
-            }
-            done = 1;


nit: I believe this change is fine. But in general, I'm not sure it's worth making similar changes if the only short term goal is to reduce stack usage and and if we're going to replace this anyway.

This removed logic is incompatible with moving the nonce-computation logic into borromean_sign, which is (a) necessary to do any useful amount of stack space reduction; (b) reduces the reviewing burden by no longer requiring that array indices of secret data in far-away files be compatible.

I'm also unsure what you mean by "we're going to replace this anyway"

real-or-random · 2022-02-04T15:51:31Z

10/10

Add two new fixed rangeproof vectors; check that various extracted values are correct; add a test for creating and verifying single-value proofs.

apoelstra · 2022-03-06T17:38:39Z

(Pushed rebase onto current master, resulting in f25f555. Will followup with fixes for Tim's comments.)

Fixes BlockstreamResearch#42

…f sidechannel space This space has never been usable, in the sense that rangeproof_sign would refuse to use it, and it complicates the rewinding logic a fair bit to retain the ability of the rewinder to access it. This does result in a minor API change, which is that the returned `m_len` variable, which indicates the total size of the rangeproof sidechannel, is reduced by 64 bytes for any proof that covers a multiple-of-4 number of bit. This change is reflected as a change in the unit tests.

…fication/rewind

Copied some more logic from the 2015-era code.

This is purely to reduce the number of arguments being passed into one function at once. Also improves const-correctness.

Also eliminate `prep` array from genrand_sign

Reduces stack usage of rangeproof_sign by 1056 bytes, is a bit safer as it doesn't require the caller of borromean_sign to know which indices are going to be overwritten, is a net-negative code diff, and reduces the amount of shared data between the borrom ean logic and its callers.

apoelstra · 2022-03-06T18:27:36Z

Addressed all comments in 5e36b32 (which I just pushed). I left a 👍 emoji on the ones that I accepted, and replied to all the ones that I didn't.

jonasnick

Changes look good to me.

apoelstra · 2022-08-04T19:36:06Z

Is this good to merge actually? (Other than needing a rebase?)

apoelstra requested review from jonasnick and real-or-random January 4, 2022 22:13

apoelstra force-pushed the 2022-01--rangeproof-cleanups branch from 67b8b74 to 7ecde97 Compare January 4, 2022 23:03

apoelstra commented Jan 5, 2022

View reviewed changes

apoelstra force-pushed the 2022-01--rangeproof-cleanups branch from 7ecde97 to 75a5781 Compare January 5, 2022 20:56

jonasnick reviewed Jan 6, 2022

View reviewed changes

jonasnick reviewed Jan 9, 2022

View reviewed changes

src/modules/rangeproof/tests_impl.h Show resolved Hide resolved

jonasnick reviewed Jan 11, 2022

View reviewed changes

apoelstra force-pushed the 2022-01--rangeproof-cleanups branch from 2515fa2 to 83d4b72 Compare January 11, 2022 19:31

jonasnick reviewed Jan 12, 2022

View reviewed changes

apoelstra force-pushed the 2022-01--rangeproof-cleanups branch from 83d4b72 to 48b2068 Compare January 12, 2022 18:44

jonasnick reviewed Jan 13, 2022

View reviewed changes

src/modules/rangeproof/rangeproof_impl.h Show resolved Hide resolved

jonasnick reviewed Jan 16, 2022

View reviewed changes

apoelstra force-pushed the 2022-01--rangeproof-cleanups branch from 48b2068 to d1a3479 Compare January 17, 2022 15:26

jonasnick reviewed Jan 17, 2022

View reviewed changes

real-or-random reviewed Jan 28, 2022

View reviewed changes

real-or-random reviewed Feb 1, 2022

View reviewed changes

real-or-random reviewed Feb 2, 2022

View reviewed changes

real-or-random reviewed Feb 4, 2022

View reviewed changes

rangeproof: add a bunch more testing

d2d0068

Add two new fixed rangeproof vectors; check that various extracted values are correct; add a test for creating and verifying single-value proofs.

apoelstra force-pushed the 2022-01--rangeproof-cleanups branch from d1a3479 to f25f555 Compare March 6, 2022 17:38

rangeproof: add more static test vectors

cb1a4bb

Fixes BlockstreamResearch#42

apoelstra added 8 commits March 6, 2022 17:46

add bench_whitelist to .gitignore

9ddcf41

rangeproof: introduce rangeproof_header structure and use it for veri…

17037ff

…fication/rewind

rangeproof: use rangeproof_header structure for proving

ecdf4c5

rangeproof: fix Jonas' bug

e36d073

Copied some more logic from the 2015-era code.

rangeproof: refactor rangeproof_genrand into two functions

1e5c300

This is purely to reduce the number of arguments being passed into one function at once. Also improves const-correctness.

rangeproof: split genrand into genrand_sign and genrang_rewind

05a618b

Also eliminate `prep` array from genrand_sign

apoelstra force-pushed the 2022-01--rangeproof-cleanups branch from f25f555 to 5e36b32 Compare March 6, 2022 18:26

jonasnick reviewed Mar 10, 2022

View reviewed changes

apoelstra mentioned this pull request Aug 4, 2022

Add dedicated methods for verifying asset/value proofs #195

Open

3 tasks

apoelstra mentioned this pull request Aug 22, 2022

rangeproof: add secp256k1_rangeproof_max_size function to estimate rangeproof size #201

Merged

	CHECK(memcmp(blind, blind_out, 32) == 0);
	CHECK(secp256k1_memcmp_var(blind, blind_out, 32) == 0);

	CHECK(memcmp(blind, blind_1, 32) == 0);
	CHECK(memcmp(blind, blind_1, 32) == 0);

	idx = 4 * (header->n_rings - 1) + header->rsizes[header->n_rings - 1] - 1 - j;
	idx = header->n_pubs - 1 - j;

		* the 2015-era API, and all of these issues will go away when we merge
		* Bulletproofs. */

		VERIFY_CHECK(*mantissa>0);
		VERIFY_CHECK((v & ~(UINT64_MAX>>(64-mantissa))) == 0); /* Did this get all the bits? */

rangeproof: clean up legacy rangeproof code #160

Are you sure you want to change the base?

rangeproof: clean up legacy rangeproof code #160

Conversation

apoelstra commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random commented Jan 6, 2022

apoelstra commented Jan 6, 2022

apoelstra commented Jan 6, 2022

jonasnick left a comment

Choose a reason for hiding this comment

jonasnick left a comment

Choose a reason for hiding this comment

jonasnick left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 12, 2022

jonasnick left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 13, 2022

jonasnick left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 17, 2022

jonasnick left a comment • edited Loading

Choose a reason for hiding this comment

real-or-random left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random commented Feb 4, 2022

apoelstra commented Mar 6, 2022

apoelstra commented Mar 6, 2022

jonasnick left a comment

Choose a reason for hiding this comment

apoelstra commented Aug 4, 2022

jonasnick left a comment •

edited

Loading

real-or-random Jan 28, 2022 •

edited

Loading

real-or-random Feb 4, 2022 •

edited

Loading