Add dedicated methods for verifying asset/value proofs #195

apoelstra · 2022-07-22T17:55:08Z

implement value proof verification
implement asset proof verification
add more tests

jgriffiths · 2022-07-24T23:13:49Z

include/secp256k1_rangeproof.h

+/** Verify a rangeproof with a single-value range. Useful as a "proof of value"
+ *  of a Pedersen commitment. Such proofs can be created with `secp256k1_rangeproof_sign`
+ *  by passing an `exp` parameter of -1 and the target value as both `value` and `min_value`.
+ *  Returns 1: Proof was valid and proved the given value


Is it also the case that min_bits must be 0 for the rangeproof, (as elements generates it) - should this be documented here?

I just checked, and interestingly no -- you can set min_bits to anything and it'll just be ignored.

Added a comment to this effect.

nit: Maybe that part of the comment should be moved to rangeproof_sign?

You gave a ":+1:" but I think this hasn't been fixed.

include/secp256k1_surjectionproof.h

src/modules/rangeproof/main_impl.h

jgriffiths · 2022-07-26T23:41:25Z

utack 9eea081

I don't have time at this stage to test this; wally uses the full proof validation code for blind asset/value proofs since we use that code already for blinding. Once PSBTv2 is in I should be able to switch over and make sure our tests still pass, may be a while though.

From an API perspective, it may be nice to have create_value variants for creating single value proofs that take the reduced set of parameters and call the full functions with the repeated/defaulted values (even if its not worth a custom implementation of the actual proof creation).

apoelstra · 2022-07-27T12:34:15Z

Agreed, create_ variants would be nice. I'll add those to this PR.

apoelstra · 2022-08-03T20:37:52Z

I have added a rangeproof_create_value function to create a single-value rangeproof. I don't think there's much value in doing the same for surjection proofs -- to create a single-value surjection proof you just use the normal surjectionproof in the "obvious" way by providing a single input and setting n_ephemeral_input_tags to 1. No magic or unused parameters.

The memory usage of surjectionproof_generate is pretty awful but not nearly as bad as rangeproof_sign -- if this is a problem then I can create a dedicated function. You can also compile with --enable-reduced-surjection-proof-size which will reduce the memory usage, at the expense of no longer being able to validate proofs for txes with more than 16 inputs.

jgriffiths · 2022-08-04T02:23:33Z

@apoelstra rangeproof was the painful case for me, so thanks for that, LGTM.

wally (following the Elements impl) only creates surjection proofs with a maximum of 3 used inputs, so for generation even the reduced mode has 12 more unused stack elements than we need. However, we don't enable reduced mode so that we can verify proofs created by others since wally is available for use as a general purpose lib. In an ideal world we'd be able to specify the number we use instead of having a fixed limit, and even better be able to generate with one limit and verify with another. I can see Jade/bitbox using 3/16 for create/verify for example assuming they can both verify on device.

jgriffiths · 2022-08-04T02:46:39Z

but not nearly as bad as rangeproof_sign

Note for secp256k1_rangeproof_sign, the final memset(prep, 0, 4096); is redundant, having been zeroed above. If the compiler doesn't eliminate that as a dead store that would keep prep alive for the whole function which would prevent some other arrays like pubs from reusing its space.

You could use a narrower unsigned type for rsizes and secidx to trivially shave some stack space in the general case, otherwise I suspect some judicious use of restrict would be needed to allow the compiler to optimally overlay the used stack items.

apoelstra · 2022-08-04T19:32:45Z

@jgriffiths i actually have a massive branch (which will need to be rebased now since i reused bits and pieces for this) whose goal is to dramatically reduce the stack space of the rangeproof code. Unfortunately it required a near rewrite of all the logic, pulling the borromean sig stuff out of its own functions and inlining it in the rangeproof/surjectionproof code.

Essentially the way it works is by directly using the proof buffer to store data so that we don't need all these arrays of intermediate values.

I'm quite proud of it but I don't think we have the review bandwidth to do anything with it :/ and I'm not sure that I finished it.

apoelstra · 2022-08-04T19:35:11Z

#160 is the start of it

jgriffiths · 2022-08-04T21:36:02Z

I don't think we have the review bandwidth to do anything with it

The eternal problem lol.

At this stage things are fine for us; its only if the hardware wallets have issues that we may need to look at lowering resource usage. That time is creeping forward for Jade at least, since the qrcode handling has been improved to the point where we should be able to take psbts from the camera to sign. But until we are ready to resource that we can't commit anyone to review or test either.

In the meantime though I'd remove the redundant memset just because its, well, redundant :)

ChristopherA · 2022-08-04T21:58:39Z

since the qrcode handling has been improved to the point where we should be able to take psbts from the camera to sign.

Have you looked at how we do animated QR PSBTs? A number of both hardware & software bitcoin wallets and services now support it. It is quite mature, has been ported to multiple languages, and has other advantages.

https://github.com/BlockchainCommons/Research/blob/master/papers/bcr-2020-006-urtypes.md#partially-signed-bitcoin-transaction-psbt-crypto-psbt

jgriffiths · 2022-08-04T22:27:35Z

Hi @ChristopherA - yes, I believe we are targeting that format, however this PR and repo aren't the place to discuss it. Please follow https://github.com/Blockstream/Jade where the PR adding support will land after internal review, or raise an issue there if you have questions/comments, thanks.

real-or-random · 2022-08-05T08:04:00Z

In the meantime though I'd remove the redundant memset just because its, well, redundant :)

edit: Oh, sorry, I see you're saying it's really redundant because there's another memset that zeroes the array already... Yes, sure, let's remove it...

old:
Please leave the memset in, it's there for a reason. It's to remove secrets from the stack. Now of course everyone knows that this won't work because the compiler will remove dead stores. But for some reasons we have the plain memsets in the code because this is how the code evolved. There's an old PR upstream that replaces all the memsets by proper methods to overwrite secret data. (Yeah I should rebase this...) Once this lands in upstream, we should do the same changes here.

So in sense, the dead memsets in the code base currently act as a marker for where we should do proper overwriting in the future, and so we should keep them.

real-or-random · 2022-08-05T08:10:06Z

I don't know how other reviewers feel but I think I'd need some refresher on how the surjection proofs work before I'm a position to review this. Or maybe @apoelstra can walk one of us through the changes.

apoelstra · 2022-08-05T16:21:51Z

@real-or-random the surjection proofs work by taking a single output commitment, a variable number of input commitments, and then doing a ring signature over (output - input_i) for all i. The premise is that the user will only be able to construct such a ring signature if the output is merely a reblinding of one of the inputs.

Everything else in this code is just mechanical stuff to make the commitment structure and proof serialization match the deployed surjection proofs in Elements.

apoelstra · 2022-08-05T16:22:16Z

Oh -- and to add, a "ring signature" is just a Schnorr signature in both cases, since we're proving a single value so there is only one key to check.

apoelstra · 2022-08-11T13:35:08Z

@real-or-random I'm going to make another PR on top of this which pulls the asset ID/entropy logic from Elements Core into secp-zkp, so the sooner we can merge this the simpler my life will be.

apoelstra · 2022-08-11T14:34:44Z

Actually, scrap that. On further inspection I think the API separation is already fine.

real-or-random · 2022-08-11T15:58:29Z

Ok I'll try to have a look tomorrow. :)

real-or-random

Just for my the understanding: The benefit here is simply efficiency, right?

In general, I think we could add some more tests that test consistency with the ordinary functions, maybe also with bitflips (because we have many and "bitmaps"/"flags").

src/modules/rangeproof/tests_impl.h

real-or-random · 2022-08-12T11:31:04Z

src/modules/rangeproof/tests_impl.h

+    test_single_value_proof(0);
+    test_single_value_proof(12345678);
+    test_single_value_proof(UINT64_MAX);


nit: won't hurt to add some randomized cases, too

maybe add a small loop?

real-or-random · 2022-08-12T11:34:28Z

include/secp256k1_rangeproof.h

+/** Verify a rangeproof with a single-value range. Useful as a "proof of value"
+ *  of a Pedersen commitment. Such proofs can be created with `secp256k1_rangeproof_sign`
+ *  by passing an `exp` parameter of -1 and the target value as both `value` and `min_value`.
+ *  Returns 1: Proof was valid and proved the given value


nit: Maybe that part of the comment should be moved to rangeproof_sign?

include/secp256k1_rangeproof.h

real-or-random · 2022-08-12T13:37:39Z

src/modules/surjection/main_impl.h

+
+    /* Now we just have a Schnorr signature in (e, s) form. The verification
+     * equation is e == H(sG - eX || proof params), where X is the difference
+     * between the output and input. */


Would it make sense to extract this code into a single function used by rangeproofs and surjectionproofs?

It differs in just enough places that I don't think it's practical.

Oh, I see, between rangeproofs and surjection proofs ... yes, that might make sense. I thought you meant between proving and verifying.

Yeah, I think it would make sense (e.g., it would have prevented the issue with the duplicate comment, and it will make you change memcmp only once. ;)) But your goal of making them self-consistent is also good. Whatever you prefer.

src/modules/surjection/main_impl.h

src/modules/rangeproof/tests_impl.h

real-or-random · 2022-08-12T13:47:42Z

src/modules/rangeproof/main_impl.h

+    if (*plen < 73 || (value == 0 && *plen < 65)) {
+        return 0;


if (*plen < 65 || (value != 0 && *plen < 73)) {

or more readable (I think):

if (*plen < (value == 0) ? 65 : 73) {

(This implies that we should have also a test for value == 0.)

BTW there is an explicit test for the size being correct when value == 0. You can search for the comment DIfferent proof sizes are unfortunate.

Ah what I meant here is: If we had a (unit) test that calls this function with value == 0 and 65 <= size < 73, it would have caught this bug.

Lol, there's an even worse bug here -- we never actually set plen. It looks like I did basically no tests on create_value.

apoelstra · 2022-08-12T23:32:22Z

@real-or-random I have pushed fixup commits which address all your nits except the code-sharing ones. If you'd like I can refactor the code, but I don't think the savings would be super big and it'd undermine the "make these functions as self-contained as possible" goal of writing this.

real-or-random · 2022-08-13T08:56:54Z

Just for my the understanding: The benefit here is simply efficiency, right?

In general, I think we could add some more tests that test consistency with the ordinary functions, maybe also with bitflips (because we have many and "bitmaps"/"flags").

You didn't answer these. I guess then an another goal is to have a simple readable "reference-like" implementation?

And for the tests: I still think that consistency tests could be useful. (Not sure about the bitflips anymore, I think you've convinced me that we know what we're doing with the header bits and "used input" bitmap.)

real-or-random · 2022-08-13T08:43:48Z

src/modules/rangeproof/main_impl.h

+    secp256k1_sha256_finalize(&sha2, tmpch);
+
+    /* 3. Check computed e against original e */
+    return !memcmp(tmpch, &proof[offset], 32);


There are more memcmps, and those could actually matter because they're not in the real code, not in the tests.

real-or-random · 2022-08-13T08:44:30Z

src/modules/rangeproof/main_impl.h

+    secp256k1_sha256_write(&sha2, tmpch, 33);
+    secp256k1_rangeproof_serialize_point(tmpch, &genp);
+    secp256k1_sha256_write(&sha2, tmpch, 33);
+    secp256k1_sha256_write(&sha2, proof, offset); /* lol we commit to one extra byte here */


Here's a duplicate of the removed comment.

src/modules/surjection/main_impl.h

real-or-random · 2022-08-13T08:46:19Z

src/modules/surjection/main_impl.h

+
+    /* Now we just have a Schnorr signature in (e, s) form. The verification
+     * equation is e == H(sG - eX || proof params), where X is the difference
+     * between the output and input. */


Yeah, I think it would make sense (e.g., it would have prevented the issue with the duplicate comment, and it will make you change memcmp only once. ;)) But your goal of making them self-consistent is also good. Whatever you prefer.

real-or-random · 2022-08-13T08:47:23Z

src/modules/rangeproof/tests_impl.h

+    test_single_value_proof(0);
+    test_single_value_proof(12345678);
+    test_single_value_proof(UINT64_MAX);


maybe add a small loop?

real-or-random · 2022-08-13T08:49:24Z

include/secp256k1_rangeproof.h

+/** Verify a rangeproof with a single-value range. Useful as a "proof of value"
+ *  of a Pedersen commitment. Such proofs can be created with `secp256k1_rangeproof_sign`
+ *  by passing an `exp` parameter of -1 and the target value as both `value` and `min_value`.
+ *  Returns 1: Proof was valid and proved the given value


You gave a ":+1:" but I think this hasn't been fixed.

real-or-random · 2022-08-13T08:52:14Z

src/modules/rangeproof/main_impl.h

+    if (*plen < 73 || (value == 0 && *plen < 65)) {
+        return 0;


Ah what I meant here is: If we had a (unit) test that calls this function with value == 0 and 65 <= size < 73, it would have caught this bug.

apoelstra · 2022-08-13T14:05:58Z

Regarding the purpose of these functions, there are two goals: to be a reference implementation for somebody who is working on implementing them on a Ledger with bare crypto primitives; and efficiency.
Regarding the comment, revisiting this I think the "how to create these" comment does make sense on the verify function and less so on the create function.

I'll add some more consistency tests, and a test for the size thing.

apoelstra · 2022-08-13T14:21:57Z

One last thing -- @real-or-random should I rename these functions from create_value and verify_value to create_single and verify_single?

real-or-random · 2022-08-13T14:27:25Z

One last thing -- @real-or-random should I rename these functions from create_value and verify_value to create_single and verify_single?

Ah yeah, I haven't really thought about the names. Now that you mention it, I think create_single and verify_single are slightly better. I'd suggest create_exact and verify_exact. This is even more precise if you ask me.

edit: Oh and I wonder why the tests now fail.

apoelstra · 2022-08-13T14:30:48Z

@real-or-random I think one of the API tests tripped the "providing plen between 65 and 73 doesn't work" bug, and I was asserting that it failed (copy/paste error because almost all the API tests end in == 0..). So when I fixed it it broke the API test.

I'll start running git rebase -x 'make && ./tests 1' before pushing from now on.

apoelstra · 2022-08-13T14:46:21Z

...ok, I might not be able to fix this today, but there's another bug in create_value where I use &proof[offset] before this value is filled in ... the fact that the tests were passing before seems to be a very surprising coincidence related to my calling create_value twice in a row with the same parameters before calling verify_value.

The fix is simple enough -- re-order the operations in create_value. But it may involve rewriting much of the function because I reused so many variables to save stack space.i

edit flight delayed, got it. I did indeed have to rewrite create_value, sorry. running through tests again and then will push

apoelstra · 2022-08-13T15:12:18Z

Ok @real-or-random one more question ... should you be able to "rewind" single-value proofs? I'm going to vote no, it will make my life much harder (I will need to recreate the insane hmac-sha2-based RNG and run a ton of crap through it) and I don't see any real value to it.

apoelstra · 2022-08-13T15:26:25Z

Ok, this one should work 😅

real-or-random · 2022-08-25T15:18:33Z

Fixups look good except for one point below. Can you squash?

I liked the previous method of computing k more. If you derive k from pp_comm, it's much easier to see that reuse of k is excluded. And indeed, at the moment, it's somewhat strange: A prover who knows the dlog between the two generators (it's not absurd that this form of "trapdoor commitment" is useful in some protocol), would be able to generate inputs that lead to reuse of k.

real-or-random · 2022-08-25T15:18:58Z

Ok @real-or-random one more question ... should you be able to "rewind" single-value proofs? I'm going to vote no, it will make my life much harder (I will need to recreate the insane hmac-sha2-based RNG and run a ton of crap through it) and I don't see any real value to it.

Yep, if you don't think we need them. (I don't know, I don't use these methods :D).

apoelstra · 2022-08-25T19:53:31Z

Rebase/squash only. Will add one more fixup which changes how k is computed.

…verify_exact

apoelstra · 2022-08-25T20:52:51Z

Ok, rebased and added one more commit with @real-or-random's suggestion to compute k differently. I can squash that commit if you'd like though I don't really think it's necessary.

real-or-random

ACK a20c1e9

apoelstra · 2022-09-11T13:32:33Z

Ok to merge?

apoelstra force-pushed the 2022-07--simple-proof-verification branch from 3cd525b to 2f4b275 Compare July 22, 2022 22:44

jgriffiths reviewed Jul 24, 2022

View reviewed changes

apoelstra force-pushed the 2022-07--simple-proof-verification branch from 2f4b275 to 593686c Compare July 26, 2022 17:30

apoelstra force-pushed the 2022-07--simple-proof-verification branch from 08dfa68 to 9d28292 Compare August 3, 2022 20:39

real-or-random reviewed Aug 12, 2022

View reviewed changes

apoelstra force-pushed the 2022-07--simple-proof-verification branch from 9d28292 to 533f423 Compare August 12, 2022 23:31

apoelstra mentioned this pull request Aug 12, 2022

rangeproof: add a bunch more testing #149

Closed

real-or-random reviewed Aug 13, 2022

View reviewed changes

apoelstra force-pushed the 2022-07--simple-proof-verification branch from 533f423 to 3617dfe Compare August 13, 2022 15:25

apoelstra force-pushed the 2022-07--simple-proof-verification branch from 3617dfe to 94e7f05 Compare August 25, 2022 19:53

apoelstra added 6 commits August 25, 2022 20:30

rangeproof: add method to verify single-value proofs

8281968

surjectionproof: add method to verify single-input proofs

0b7cf31

rangeproof: add verify_value function to create single-value proofs

b6b33c9

rangeproof: add more tests for exact-value proofs

b8fc967

rangeproof: rename create_value and verify_value to create_exact and …

7cf34e2

…verify_exact

rangeproof_sign_exact: compute k in a more obviously non-reusing way

a20c1e9

apoelstra force-pushed the 2022-07--simple-proof-verification branch from 94e7f05 to a20c1e9 Compare August 25, 2022 20:52

real-or-random approved these changes Aug 25, 2022

View reviewed changes

Add dedicated methods for verifying asset/value proofs #195

Are you sure you want to change the base?

Add dedicated methods for verifying asset/value proofs #195

Conversation

apoelstra commented Jul 22, 2022 • edited Loading

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

jgriffiths commented Jul 26, 2022

apoelstra commented Jul 27, 2022

apoelstra commented Aug 3, 2022 • edited Loading

jgriffiths commented Aug 4, 2022 • edited Loading

jgriffiths commented Aug 4, 2022 • edited Loading

apoelstra commented Aug 4, 2022

apoelstra commented Aug 4, 2022

jgriffiths commented Aug 4, 2022

ChristopherA commented Aug 4, 2022

jgriffiths commented Aug 4, 2022

real-or-random commented Aug 5, 2022 • edited Loading

real-or-random commented Aug 5, 2022

apoelstra commented Aug 5, 2022

apoelstra commented Aug 5, 2022

apoelstra commented Aug 11, 2022

apoelstra commented Aug 11, 2022

real-or-random commented Aug 11, 2022

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

apoelstra commented Aug 12, 2022

real-or-random commented Aug 13, 2022

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

apoelstra commented Aug 13, 2022

apoelstra commented Aug 13, 2022

real-or-random commented Aug 13, 2022 • edited Loading

apoelstra commented Aug 13, 2022

apoelstra commented Aug 13, 2022 • edited Loading

apoelstra commented Aug 13, 2022

apoelstra commented Aug 13, 2022

real-or-random commented Aug 25, 2022

real-or-random commented Aug 25, 2022 • edited Loading

apoelstra commented Aug 25, 2022

apoelstra commented Aug 25, 2022

real-or-random left a comment

Choose a reason for hiding this comment

apoelstra commented Sep 11, 2022

apoelstra commented Jul 22, 2022 •

edited

Loading

apoelstra commented Aug 3, 2022 •

edited

Loading

jgriffiths commented Aug 4, 2022 •

edited

Loading

jgriffiths commented Aug 4, 2022 •

edited

Loading

real-or-random commented Aug 5, 2022 •

edited

Loading

real-or-random commented Aug 13, 2022 •

edited

Loading

apoelstra commented Aug 13, 2022 •

edited

Loading

real-or-random commented Aug 25, 2022 •

edited

Loading