Bp7 #2883

moneromooo-monero · 2017-12-03T18:15:07Z

The fork numbers and checking will be changed in a few days (especially the testnet/mainnet fork height for consensus).

moneromooo-monero · 2017-12-05T15:17:07Z

New version of the last commit, which changes serialization to fill V from outPk, rather than the other way round, which will play better with pruning.

moneromooo-monero · 2017-12-07T15:20:17Z

A few fixes and tweaks as last 3 commits, I'm keeping them out for now for ease of review, but I'll squash them before they get merged (fluffypony, ping me for this).

danrmiller · 2017-12-07T17:54:44Z

Some minor build oddities I brought up on IRC, but I think it could be worth considering working around them so that people can build easily with default setups.

Ubuntu 16.04 gcc 5.4:
https://build.getmonero.org/builders/monero-static-ubuntu-amd64/builds/2972/steps/compile/logs/stdio

/home/vagrant/slave/monero-static-ubuntu-amd64/build/src/cryptonote_basic/cryptonote_boost_serialization.h:287:38: error: logical 'and' of mutually exclusive tests is always false [-Werror=logical-op]
     if (x.type == rct::RCTTypeSimple && x.type == rct::RCTTypeSimpleBulletproof)
                                      ^

For this one (Clang3.8), should I just set CFLAGS=-Wno-error when I build, or do you think we should handle it specifically?:

https://build.getmonero.org/builders/monero-static-debian-armv8/builds/2590/steps/compile/logs/stdio

/mnt/buildbot/buildbot/slave/monero-static-debian-armv8/build/src/ringct/bulletproofs.cc:250:2: error: TODO: find a better invert func ? Though it seems pretty fast anyway [-Werror,-W#warnings]
#warning TODO: find a better invert func ? Though it seems pretty fast anyway
 ^
1 error generated.

fluffypony

Reviewed; hard to thoroughly review the crypto for robustness, but that's what testnet deployment is for:)

Allows use in more complex expressions

moneromooo-monero · 2017-12-07T19:24:28Z

Odd about that #warning, it's not seen as an error for me... Maybe -Werror is set for CLANG only ?
Anyway, I'll remove it.

moneromooo-monero · 2017-12-07T19:48:23Z

Fixed, and rebased.

stoffu · 2017-12-07T23:26:07Z

src/cryptonote_core/cryptonote_tx_utils.cpp

@@ -461,7 +461,7 @@ namespace cryptonote

      // the non-simple version is slightly smaller, but assumes all real inputs
      // are on the same index, so can only be used if there just one ring.
-      bool use_simple_rct = sources.size() > 1;
+      bool use_simple_rct = sources.size() > 1 || bulletproof;


Why is the Simple RingCT always used when using BP, even when the number of input is just one?

Because it was only done for simple originally. I will remove this now.

stoffu · 2017-12-07T23:33:35Z

src/ringct/rctSigs.cpp

 #include "cryptonote_basic/cryptonote_format_utils.h"
+#include "rctSigs.h"


Did you bring this include to the last because the ordering matters? IMO, when possible, headers should be written such that the inclusion ordering doesn't matter.

EDIT: The ordering didn't have any effect with Clang.

It was just missing AFAICT.

I'm not sure what you mean by that and if I made my intent clear to you. My intent was: isn't it sufficient to just add include for bulletproofs.h, without moving the line of include for rctSigs.h (which adds an unnecessary change)?

I see now. I did not notice it had moved, I'll put it back where it was. I thought it was missing all along.

stoffu · 2017-12-07T23:40:45Z

src/ringct/rctSigs.cpp

@@ -335,16 +345,41 @@ namespace rct {
      hashes.push_back(hash2rct(h));

      keyV kv;
-      kv.reserve((64*3+1) * rv.p.rangeSigs.size());
-      for (auto r: rv.p.rangeSigs)
+      if (rv.type == RCTTypeSimpleBulletproof)


Shouldn't this be checking also || rv.type == RCTTypeFullBulletproof?

stoffu · 2017-12-07T23:45:35Z

src/ringct/rctSigs.cpp

+        kv.reserve((6*2+10) * rv.p.bulletproofs.size());
+        for (const auto &p: rv.p.bulletproofs)
+        {
+          CHECK_AND_ASSERT_THROW_MES(p.V.size() == 1, "V has not exactly one element");


Perhaps do CHECK_AND_ASSERT_THROW_MES also for L and R (not empty and the same size)?

Or maybe, since this validity checking is also done elsewhere, perhaps the class rct::Bulletproof could have a member function that checks this?

It doesn't really matter to this function. In fact, I probably should just push all Vs, and let the other code decide on the semantics, that'll be more future proof.

stoffu · 2017-12-08T00:37:27Z

src/ringct/rctSigs.cpp

@@ -575,7 +610,10 @@ namespace rct {
        rv.type = RCTTypeFull;


I believe this should be:

rv.type = bulletproof ? RCTTypeFullBulletproof : RCTTypeFull;

stoffu · 2017-12-08T00:54:36Z

src/ringct/rctSigs.cpp

@@ -717,7 +770,10 @@ namespace rct {
        CHECK_AND_ASSERT_MES(rv.type == RCTTypeFull, false, "verRct called on non-full rctSig");


I believe this should be:

CHECK_AND_ASSERT_MES(rv.type == RCTTypeFull || rv.type == RCTTypeFullBulletproof, false, "verRct called on non-full rctSig");

assuming that BPs are allowed for the Full RingCT.

stoffu · 2017-12-08T00:55:56Z

src/ringct/rctSigs.cpp

@@ -736,7 +792,10 @@ namespace rct {
            DP("range proofs verified?");
            for (size_t i = 0; i < rv.outPk.size(); i++) {
              tpool.submit(&waiter, [&, i] {
-                results[i] = verRange(rv.outPk[i].mask, rv.p.rangeSigs[i]);
+                if (rv.p.rangeSigs.empty())
+                  results[i] = bulletproof_VERIFY(rv.p.bulletproofs[i]); // TODO


Why TODO? Don't BPs work for the Full RingCT?

stoffu · 2017-12-08T01:00:41Z

src/ringct/rctSigs.cpp

@@ -717,7 +770,10 @@ namespace rct {
        CHECK_AND_ASSERT_MES(rv.type == RCTTypeFull, false, "verRct called on non-full rctSig");
        if (semantics)
        {
-          CHECK_AND_ASSERT_MES(rv.outPk.size() == rv.p.rangeSigs.size(), false, "Mismatched sizes of outPk and rv.p.rangeSigs");
+          if (rv.p.rangeSigs.empty())


I'm not a big fan of this way of determining the type of range proofs by checking the Borromean sigs being empty, because it doesn't check for other erroneous cases such as both Borromean and BP sigs being non-empty. Probably this is unimportant, and I also don't know what would be a better way.

stoffu · 2017-12-08T01:07:40Z

src/ringct/rctSigs.cpp

+        else
+        {
+          CHECK_AND_ASSERT_THROW_MES(rv.outPk.size() == rv.ecdhInfo.size(), "Mismatched sizes of rv.outPk and rv.ecdhInfo");
+        }


I don't see the equivalent changes made to decodeRct. Does this mean that the BPs are somehow disallowed for the Full RingCT? But that sounds strange, because the range proofs and the input ring signatures are completely independent. Also, the proving part genRct takes a switch bulletproof and does use BP.

stoffu · 2017-12-08T01:10:37Z

src/ringct/rctTypes.h

@@ -52,7 +52,6 @@ extern "C" {
 #include "serialization/binary_archive.h"
 #include "serialization/json_archive.h"

-


Is this intended?

stoffu · 2017-12-08T01:13:38Z

src/ringct/rctTypes.h

            return false;
-          for (size_t i = 0; i < outputs; ++i)
+          if (type == RCTTypeSimpleBulletproof)


Shouldn't this be also checking for type == RCTTypeFullBulletproof ?

stoffu · 2017-12-08T01:23:05Z

src/ringct/rctTypes.h

@@ -490,5 +545,6 @@ VARIANT_TAG(json_archive, rct::mgSig, "rct_mgSig");
 VARIANT_TAG(json_archive, rct::rangeSig, "rct_rangeSig");
 VARIANT_TAG(json_archive, rct::boroSig, "rct_boroSig");
 VARIANT_TAG(json_archive, rct::rctSig, "rct_rctSig");
+VARIANT_TAG(json_archive, rct::Bulletproof, "rct_bulletproof");


Perhaps src/serialization/json_object needs to be updated as well? Just a guess.

Hmm. I guess, I don't know about this one... I think it's a new 0mq one. I'll fix it up too.

stoffu · 2017-12-08T01:34:11Z

src/wallet/wallet2.cpp

+  if (bulletproof)
+    size += (704 + 3) * n_outputs;
+  else
+    size += (2*64*32+32+64*32) * n_outputs;


AIUI, for the Borromean case, there are 2*64 +1 scalars (s0, s1, ee in boroSig) and 64 points (Ci in rangeSig) that make up this number. What would be the breakdown for BPs?

The paper states at the end of Section 4.2 that

In total the prover sends 2*log_2(n)+4 group elements and 5 elements in Zp.

So these 4 group elements and 5 scalar elements must be A, S, T1, T2 and taux, mu, a, b, t in Bulletproof, respectively. And we have n=64 so 2*log_2(n) = 12. So my calculation would be 32*(12 + 9) = 672. Where am I wrong?

Maybe it's more helpful to just keep the breakdown as in the paper: (2*6 + 4 + 5)*32

I had counted V too, but I don't save it anymore, so this needs updating, yes.

stoffu · 2017-12-08T01:39:24Z

src/wallet/wallet2.cpp

@@ -493,22 +496,31 @@ size_t estimate_rct_tx_size(int n_inputs, int mixin, int n_outputs, size_t extra
  // ecdhInfo
  size += 2 * 32 * n_outputs;
  // outPk - only commitment is saved
-  size += 32 * n_outputs;
+  if (!bulletproof)
+    size += 32 * n_outputs;


Oh, does the above calculation for BP include the commitment itself, too? Then my calculation gets closer (704). Actually I don't know where that remaining 3 bytes come from.

In this case, isn't it simpler to keep this line unchanged and change the above calculation to:

size += (672 + 3) * n_outputs;

so that both Borromean & BP cases become consistent?

I was originally saving V, and omitting these. This was changed since (easier for pruning) so I changed this to what you said. The 3 bytes are vector sizes (varint).

stoffu · 2017-12-08T01:46:17Z

src/wallet/wallet2.cpp

  // txnFee
  size += 4;

-  LOG_PRINT_L2("estimated rct tx size for " << n_inputs << " with ring size " << (mixin+1) << " and " << n_outputs << ": " << size << " (" << ((32 * n_inputs/*+1*/) + 2 * 32 * (mixin+1) * n_inputs + 32 * n_outputs) << " saved)");
+  LOG_PRINT_L2("estimated rct tx size for " << n_inputs << " with ring size " << (mixin+1) << " and " << n_outputs << ": " << size << " (" << ((32 * n_inputs/*+1*/) + 2 * 32 * (mixin+1) * n_inputs + (bulletproof ? 0 : (32 * n_outputs))) << " saved)");


I'm not sure what this change means. Could you elaborate?

Moot now since I swapped V and outPk saving. I'll remove.

stoffu · 2017-12-08T01:52:35Z

tests/core_tests/rct.cpp

@@ -132,7 +132,7 @@ bool gen_rct_tx_validation_base::generate_with(std::vector<test_event_entry>& ev
      CHECK_AND_ASSERT_MES(r, false, "Failed to generate key derivation");
      crypto::secret_key amount_key;
      crypto::derivation_to_scalar(derivation, o, amount_key);
-      if (rct_txes[n].rct_signatures.type == rct::RCTTypeSimple)
+      if (rct_txes[n].rct_signatures.type == rct::RCTTypeSimple || rct_txes[n].rct_signatures.type == rct::RCTTypeSimpleBulletproof)


I think a similar change is needed for this line tests/performance_tests/check_tx_signature.h:83:

if (m_tx.rct_signatures.type == rct::RCTTypeFull)

stoffu · 2017-12-08T02:08:49Z

src/ringct/bulletproofs.h

+#ifndef BULLETPROOFS_H
+#define BULLETPROOFS_H
+
+#include "serialization/serialization.h"


This include doesn't seem necessary.

stoffu · 2017-12-08T02:18:53Z

src/ringct/bulletproofs.cc

+
+  CHECK_AND_ASSERT_MES(proof.L.size() == proof.R.size(), false, "Mismatched L and R sizes");
+  CHECK_AND_ASSERT_MES(proof.L.size() > 0, false, "Empty proof");
+  CHECK_AND_ASSERT_MES(proof.L.size() <= 6, false, "Proof is too large");


Can a valid proof have L & R whose size isn't 6? Shouldn't their size always be exactly 6?

It's exactly 6 if you prove 64 bits. If L is > 6, then this code will not work. I'm not sure whether it'll work for < 6.

And aren't we always proving for exactly 64 bits? I highly doubt that the code would work if L's size is < 6, but I'd like to hear Sarang's comment.

stoffu · 2017-12-08T06:26:40Z

src/ringct/bulletproofs.cc

+  rct::key ipt = inner_product(twoN, aL);
+  sc_muladd(test_t0.bytes, zsq.bytes, ipt.bytes, test_t0.bytes);
+  sc_add(test_t0.bytes, test_t0.bytes, k.bytes);
+  CHECK_AND_ASSERT_THROW_MES(t0 == test_t0, "t0 check failed");


If this debug code is to be activated under some circumstance, I think this shouldn't throw here, because otherwise the unit test doesn't pass for the intended failure cases.

I don't think this is an intended failure case, it checks the code above is correct. Which unit test is not passing when this is enabled ?

This is what I get when DEBUG_BP is activated:

[----------] 4 tests from bulletproofs [ RUN ] bulletproofs.valid_zero [ OK ] bulletproofs.valid_zero (211 ms) [ RUN ] bulletproofs.valid_max [ OK ] bulletproofs.valid_max (236 ms) [ RUN ] bulletproofs.invalid_8 2017-12-08 09:22:35.557 0x7fffae0623c0 ERROR bulletproofs src/ringct/bulletproofs.cc:395 t0 check failed unknown file: Failure C++ exception with description "t0 check failed" thrown in the test body. [ FAILED ] bulletproofs.invalid_8 (54 ms) [ RUN ] bulletproofs.invalid_31 2017-12-08 09:22:35.614 0x7fffae0623c0 ERROR bulletproofs src/ringct/bulletproofs.cc:395 t0 check failed unknown file: Failure C++ exception with description "t0 check failed" thrown in the test body. [ FAILED ] bulletproofs.invalid_31 (52 ms) [----------] 4 tests from bulletproofs (554 ms total)

I see, that's because I split the function in two so I could monkey with the keyed amount. Will fix.

Actually I'll leave this one as is. Just don't run the unit tests with the debug code in. We want to be able to check the verification fails on such a proof.

stoffu · 2017-12-08T06:43:00Z

src/ringct/bulletproofs.cc

+
+  // PAPER LINES 54-57
+  rct::keyV l = vector_add(vector_subtract(aL, vpIz), vector_scalar(sL, x));
+  rct::keyV r = vector_add(hadamard(yN, vector_add(aR, vector_add(vpIz, vector_scalar(sR, x)))), vector_scalar(twoN, zsq));


I can see some opportunities for reusing already computed data such as:

vector_subtract(aL, vpIz) vector_add(aR, vpIz) vector_scalar(twoN, zsq)

Ah, I did not make another pass after the log modifications :/ Will fix.

stoffu · 2017-12-08T06:44:13Z

src/ringct/bulletproofs.cc

+  rct::key test_t;
+  sc_muladd(test_t.bytes, t1.bytes, x.bytes, t0.bytes);
+  sc_muladd(test_t.bytes, t2.bytes, xsq.bytes, test_t.bytes);
+  CHECK_AND_ASSERT_THROW_MES(test_t == t, "test_t check failed");


Same comment about throwing (or not)

stoffu · 2017-12-08T13:35:39Z

src/ringct/rctSigs.cpp

        CHECK_AND_ASSERT_THROW_MES(i < rv.ecdhInfo.size(), "Bad index");
+        const bool bulletproof = rv.type == RCTTypeFullBulletproof || rv.type == RCTTypeSimpleBulletproof;


It seems weird to test against rv.type == RCTTypeSimpleBulletproof which is explicitly ruled out in the assertion above.

stoffu · 2017-12-08T13:36:02Z

src/ringct/rctSigs.cpp

        CHECK_AND_ASSERT_THROW_MES(i < rv.ecdhInfo.size(), "Bad index");
+        const bool bulletproof = rv.type == RCTTypeFullBulletproof || rv.type == RCTTypeSimpleBulletproof;


The same comment as above.

Based on Java code from Sarang Noether

Changes from sarang

moneromooo-monero · 2017-12-08T13:52:36Z

This now uses OpenSSL for inverse, rather than GMP.

fluffypony

Reviewed

c83d0b3 add bulletproofs from v7 on testnet (moneromooo-monero) 8620ef0 bulletproofs: switch H/G in Pedersen commitments to match rct (moneromooo-monero) d58835b integrate bulletproofs into monero (moneromooo-monero) 90b8d9f add bulletproofs to the build, with basic unit tests (moneromooo-monero) fe12026 perf_timer: add non scoped start/stop timer defines (moneromooo-monero) ada4291 add a version of ge_double_scalarmult_precomp_vartime with A precomp (moneromooo-monero) d43eef6 ringct: add a version of addKeys which returns the result (moneromooo-monero) 7ff0792 sc_mul and sc_muladd (luigi1111) 3d0b54b epee: add do while(0) around brace statement in a macro (moneromooo-monero)

moneromooo-monero force-pushed the bp7 branch 2 times, most recently from 839ff7e to 47646a7 Compare December 5, 2017 15:16

fluffypony approved these changes Dec 7, 2017

View reviewed changes

moneromooo-monero and others added 5 commits December 7, 2017 19:23

epee: add do while(0) around brace statement in a macro

3d0b54b

Allows use in more complex expressions

sc_mul and sc_muladd

7ff0792

ringct: add a version of addKeys which returns the result

d43eef6

add a version of ge_double_scalarmult_precomp_vartime with A precomp

ada4291

perf_timer: add non scoped start/stop timer defines

fe12026

moneromooo-monero force-pushed the bp7 branch from 5d00b4a to 2eae051 Compare December 7, 2017 19:47

stoffu reviewed Dec 7, 2017

View reviewed changes

stoffu reviewed Dec 8, 2017

View reviewed changes

moneromooo-monero force-pushed the bp7 branch 3 times, most recently from 28c735c to ed13b6c Compare December 8, 2017 12:46

stoffu reviewed Dec 8, 2017

View reviewed changes

add bulletproofs to the build, with basic unit tests

90b8d9f

Based on Java code from Sarang Noether

moneromooo-monero force-pushed the bp7 branch from ed13b6c to 16c84d2 Compare December 8, 2017 13:45

moneromooo-monero added 3 commits December 8, 2017 13:48

integrate bulletproofs into monero

d58835b

bulletproofs: switch H/G in Pedersen commitments to match rct

8620ef0

Changes from sarang

add bulletproofs from v7 on testnet

c83d0b3

moneromooo-monero force-pushed the bp7 branch from 16c84d2 to c83d0b3 Compare December 8, 2017 13:52

stoffu approved these changes Dec 8, 2017

View reviewed changes

fluffypony approved these changes Dec 8, 2017

View reviewed changes

fluffypony merged commit c83d0b3 into monero-project:master Dec 8, 2017

		#include "cryptonote_basic/cryptonote_format_utils.h"
		#include "rctSigs.h"

		@@ -717,7 +770,10 @@ namespace rct {
		CHECK_AND_ASSERT_MES(rv.type == RCTTypeFull, false, "verRct called on non-full rctSig");

		@@ -52,7 +52,6 @@ extern "C" {
		#include "serialization/binary_archive.h"
		#include "serialization/json_archive.h"

		CHECK_AND_ASSERT_THROW_MES(i < rv.ecdhInfo.size(), "Bad index");
		const bool bulletproof = rv.type == RCTTypeFullBulletproof \|\| rv.type == RCTTypeSimpleBulletproof;

Bp7 #2883

Bp7 #2883

Conversation

moneromooo-monero commented Dec 3, 2017

moneromooo-monero commented Dec 5, 2017 • edited Loading

moneromooo-monero commented Dec 7, 2017

danrmiller commented Dec 7, 2017

fluffypony left a comment

Choose a reason for hiding this comment

moneromooo-monero commented Dec 7, 2017

moneromooo-monero commented Dec 7, 2017

stoffu Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Dec 7, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

stoffu Dec 8, 2017 • 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

Choose a reason for hiding this comment

stoffu Dec 8, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

moneromooo-monero Dec 8, 2017 • 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

stoffu Dec 8, 2017 • 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

moneromooo-monero commented Dec 8, 2017

fluffypony left a comment

Choose a reason for hiding this comment

moneromooo-monero commented Dec 5, 2017 •

edited

Loading

stoffu Dec 7, 2017 •

edited

Loading

stoffu Dec 7, 2017 •

edited

Loading

stoffu Dec 8, 2017 •

edited

Loading

stoffu Dec 7, 2017 •

edited

Loading

stoffu Dec 8, 2017 •

edited

Loading

stoffu Dec 8, 2017 •

edited

Loading

stoffu Dec 8, 2017 •

edited

Loading

stoffu Dec 8, 2017 •

edited

Loading

stoffu Dec 8, 2017 •

edited

Loading

moneromooo-monero Dec 8, 2017 •

edited

Loading

stoffu Dec 8, 2017 •

edited

Loading