Bulletproof+ by sarang, tied to consensus #7170

moneromooo-monero · 2020-12-18T17:12:01Z

For reference, the audit:
https://suyash67.github.io/homepage/assets/pdfs/bulletproofs_plus_audit_report_v1.1.pdf

vtnerd

I did a partial review a while ago, but never got around to completing it. I will just post what I have so far. Its basically all C++ optimization stuff. IMO the biggie is the copy+pasted code that could be made available via header to prevent duplicate ASM copies.

vtnerd · 2020-12-19T02:28:11Z

src/ringct/bulletproofs_plus.cc

+    static std::shared_ptr<pippenger_cached_data> pippenger_HiGi_cache;
+
+    // Useful scalar constants
+    static const rct::key ZERO = { {0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00  } }; // 0


Mark these as constexpr.

vtnerd · 2020-12-19T02:28:25Z

src/ringct/bulletproofs_plus.cc

+    // Initial transcript hash
+    static rct::key initial_transcript;
+
+    static boost::mutex init_mutex;


Drop this, not needed in C++11+ after other proposed changes.

Not sure what the 'other proposed changes are', but init_exponents() should use std::call_once().

vtnerd · 2020-12-19T02:32:41Z

src/ringct/bulletproofs_plus.cc

+    static constexpr size_t maxM = BULLETPROOF_PLUS_MAX_OUTPUTS; // maximum number of outputs to aggregate into a single proof
+
+    // Cached public generators
+    static rct::key Hi[maxN*maxM], Gi[maxN*maxM];


This is something I just noticed about the original Bulletproof code too. The static init code is slightly sub-optimal for multi-threads.

Put all of these runtime computed statics in a struct as members. Then use a function static, which is required to be thread-safe construction:

class public_generators { public_generators() { // paste code from `init_generators` } public: // members public_generators(const& public_generator&) = delete; public_generators& operator=(const& public_generator&) = delete; static const public_generators& instance() { static const public_generators singleton; return singleton; } };

The result will be a more efficient double-check locking scheme generated by the compiler. This verification code will never be stalled with multi-threads/cores writing/acquiring the static mutex.

Can you expand on why this is faster, since the compiler would have to lock too ?

I re-checked the concurrency for Bulletproof checks. None! The code is designed for aggregate proof checks (unlike Borromean). So the change is unimportant, although it can't hurt to prevent minor CPU stalling.

Acquiring a lock requires exclusive cacheline access for the write. Each core has to wait "in-a-line" for the cacheline, with communication stalls for each acquire. The C++11 function-static does a double-check lock, which only requires exclusive cacheline when an atomic init flag is false (before+during initialization).

Slightly longer -
I thought this was spreading the checks across cores like Borromean. The lock acquire creates a single-file "gate" that every core must pass-through. The wait time foreach CPU-core depends on the cacheline state of the CPU, and whether other CPUs are involved. Ryzen 3+ chips with 9+ cores are effectively 2-CPU systems crammed onto one die. So concurrency is secretly getting more difficult. And identifying drops in throughput are maddening because its transient (as-in, I've seen 50% drops in throughput or 0% depending on thread scheduling).

The comments about removing allocations are related - each allocation is potentially a global sync event. Allocators have moved away from this in some small allocation requests.

vtnerd · 2020-12-19T02:39:03Z

src/ringct/bulletproofs_plus.cc

+        return BulletproofPlus(std::move(V), A, A1, B, r1, s1, d1, std::move(L), std::move(R));
+    }
+
+    BulletproofPlus bulletproof_plus_PROVE(const std::vector<uint64_t> &v, const rct::keyV &gamma)


This argument should be a span.

Maybe, but fuck this waste of time trying to get the compiler to like it. I give up, it's just so litle compared to the time this function call will take in the first place, not worth my time.

Its seriously not that difficult. But ok, the other bulletproof code can be updated too.

Then you get to do it. I spend close to an hour fighting the compiler for something that's just not worth an hour of work.

vtnerd · 2020-12-19T02:40:20Z

src/ringct/bulletproofs_plus.cc

+    // Given a value v [0..2**N) and a mask gamma, construct a range proof
+    BulletproofPlus bulletproof_plus_PROVE(const rct::key &sv, const rct::key &gamma)
+    {
+        return bulletproof_plus_PROVE(rct::keyV(1, sv), rct::keyV(1, gamma));


Take the address and pass a span of 1.

vtnerd · 2020-12-19T03:15:43Z

src/ringct/bulletproofs_plus.cc

+        data.reserve(maxN*maxM*2);
+        for (size_t i = 0; i < maxN*maxM; ++i)
+        {
+            Hi[i] = get_exponent(rct::H, i * 2);


So this only happens during init but bothers me for some reason (it can be shortened). Unless the goal was verification ? @SarangNoether

Hi and Gi appear to be unnecessary static variables that are never used again. Drop them regardless.

get_exponent can return a ge_p3, its going to use RVO anyway. Unless the goal is an extra verification step (ge_frombytes_vartime), which might make some sense. Otherwise Gi_p3[i] = get_exponent(rct::H, i * 2);, etc.

vtnerd · 2020-12-19T03:22:26Z

src/ringct/bulletproofs_plus.cc

+        sc_sub(TWO_SIXTY_FOUR_MINUS_ONE.bytes, TWO_SIXTY_FOUR_MINUS_ONE.bytes, ONE.bytes);
+
+        // Generate the initial Fiat-Shamir transcript hash, which is constant across all proofs
+        static const std::string domain_separator(config::HASH_KEY_BULLETPROOF_PLUS_TRANSCRIPT);


Temporary/copy not needed here. Definitely drop the static.

It'd need to be replaced by strlen AFAICT, and ir's only at init time, who cares anyway.

Yes, its at init time only, thus remove the static keyword. Its a potential waste of heap memory for little benefit (save a call to free until end of program?). The memory is trivial, but its also just erasing the keyword.

Replacement would be const boost::string_ref domain_separator(...) which behaves identically without the alloc/free behavior.

Oh right, I thought you meant dropping the static variable itself (and use config::HASH_KEY_BULLETPROOF_PLUS_TRANSCRIPT directly). I'll drop the keyword.

vtnerd · 2020-12-19T04:46:01Z

src/ringct/bulletproofs_plus.cc

+        multiexp_data.reserve(nV + (2 * (max_logM + logN) + 3) * proofs.size() + 2 * maxMN);
+        multiexp_data.resize(2 * maxMN);
+
+        const std::vector<rct::key> inverses = invert(to_invert);


Move, then clear (just to be safe).

const std::vector<rct::key> inverses = invert(std::move(to_invert)); to_invert.clear();

vtnerd · 2020-12-20T03:19:17Z

src/ringct/bulletproofs_plus.cc

+    }
+
+    // Given a set of values v [0..2**N) and masks gamma, construct a range proof
+    BulletproofPlus bulletproof_plus_PROVE(const rct::keyV &sv, const rct::keyV &gamma)


The first argument should be a span (see other comment) -> ...(epee::span<const rct::key> sv, ...)

vtnerd · 2020-12-20T04:26:25Z

src/ringct/bulletproofs_plus.cc

+    static boost::mutex init_mutex;
+
+    // Use the generator caches to compute a multiscalar multiplication
+    static inline rct::key multiexp(const std::vector<MultiexpData> &data, size_t HiGi_size)


There's a bunch of functions that appear to be directly copy+pasted from bulletproofs.cc. This needs to be avoided, otherwise there are two copies of the ASM will be in the binary. The functions that I've noticed:

multiexp

is_reduced

vector_exponent

vector_add

vector_subtract

vector_scalar

sm

invert

vtnerd · 2021-09-08T03:54:55Z

Looks like there is some conflicting files on this PR ...

selsta · 2021-10-11T17:42:28Z

2021-10-04T12:43:03.7854657Z /home/runner/work/monero/monero/src/cryptonote_basic/cryptonote_boost_serialization.h:340:15: error: use of overloaded operator '>=' is ambiguous (with operand types 'const boost::serialization::version_type' and 'int')
2021-10-04T12:43:03.7856066Z       if (ver >= 2)
2021-10-04T12:43:03.7856677Z           ~~~ ^  ~
2021-10-04T12:43:03.7858153Z /home/runner/work/monero/monero/contrib/depends/x86_64-unknown-freebsd/include/boost/operators.hpp:132:18: note: candidate function
2021-10-04T12:43:03.7859205Z      friend bool operator>=(const T& x, const U& y) { return !static_cast<bool>(x < y); }
2021-10-04T12:43:03.7859787Z                  ^
2021-10-04T12:43:03.7860941Z /home/runner/work/monero/monero/src/cryptonote_basic/cryptonote_boost_serialization.h:340:15: note: built-in candidate operator>=(unsigned int, int)
2021-10-04T12:43:03.7861842Z       if (ver >= 2)
2021-10-04T12:43:03.7862267Z               ^
2021-10-04T12:43:03.8324301Z /home/runner/work/monero/monero/src/cryptonote_basic/cryptonote_boost_serialization.h:370:15: error: use of overloaded operator '>=' is ambiguous (with operand types 'const boost::serialization::version_type' and 'int')
2021-10-04T12:43:03.8325420Z       if (ver >= 2)
2021-10-04T12:43:03.8325771Z           ~~~ ^  ~
2021-10-04T12:43:03.8326878Z /home/runner/work/monero/monero/contrib/depends/x86_64-unknown-freebsd/include/boost/operators.hpp:132:18: note: candidate function
2021-10-04T12:43:03.8327932Z      friend bool operator>=(const T& x, const U& y) { return !static_cast<bool>(x < y); }
2021-10-04T12:43:03.8328484Z                  ^
2021-10-04T12:43:03.8329653Z /home/runner/work/monero/monero/src/cryptonote_basic/cryptonote_boost_serialization.h:370:15: note: built-in candidate operator>=(unsigned int, int)
2021-10-04T12:43:03.8330530Z       if (ver >= 2)
2021-10-04T12:43:03.8330875Z               ^

selsta · 2021-10-11T17:43:18Z

/home/runner/work/monero/monero/src/wallet/api/wallet.cpp: In member function ‘virtual uint64_t Monero::WalletImpl::estimateTransactionFee(const std::vector<std::pair<std::__cxx11::basic_string<char>, long unsigned int> >&, Monero::PendingTransaction::Priority) const’:
/home/runner/work/monero/monero/src/wallet/api/wallet.cpp:1765:46: error: no matching function for call to ‘tools::wallet2::estimate_fee(bool, bool, int, uint64_t, std::vector<std::pair<std::__cxx11::basic_string<char>, long unsigned int> >::size_type, const size_t&, bool, bool, uint64_t, uint64_t, uint64_t)’
 1765 |         m_wallet->get_fee_quantization_mask());
      |                                              ^
In file included from /home/runner/work/monero/monero/src/wallet/api/wallet.h:35,
                 from /home/runner/work/monero/monero/src/wallet/api/wallet.cpp:32:
/home/runner/work/monero/monero/src/wallet/wallet2.h:1414:14: note: candidate: ‘uint64_t tools::wallet2::estimate_fee(bool, bool, int, int, int, size_t, bool, bool, bool, uint64_t, uint64_t, uint64_t) const’
 1414 |     uint64_t estimate_fee(bool use_per_byte_fee, bool use_rct, int n_inputs, int mixin, int n_outputs, size_t extra_size, bool bulletproof, bool clsag, bool bulletproof_plus, uint64_t base_fee, uint64_t fee_multiplier, uint64_t fee_quantization_mask) const;
      |              ^~~~~~~~~~~~
/home/runner/work/monero/monero/src/wallet/wallet2.h:1414:14: note:   candidate expects 12 arguments, 11 provided
make[3]: *** [src/wallet/api/CMakeFiles/obj_wallet_api.dir/build.make:76: src/wallet/api/CMakeFiles/obj_wallet_api.dir/wallet.cpp.o] Error 1
make[3]: *** Waiting for unfinished jobs....

libwallet also needs a small update

vtnerd

A few more comments

vtnerd · 2021-10-22T20:40:08Z

src/ringct/rctTypes.cpp

+        for (const BulletproofPlus &proof: proofs)
+        {
+            size_t n2 = n_bulletproof_plus_amounts(proof);
+            CHECK_AND_ASSERT_MES(n2 < std::numeric_limits<uint32_t>::max() - n, 0, "Invalid number of bulletproofs");


Minor (because this should never be an issue on any arch), but uint32_t and size_t are different types (size_t theoretical could be smaller than uint32_t) ?

I use uin32_t because this is consensus, so the check has to be the same on all archs. I did not think size_t could be smaller than uin32_t though, that'd on ... MSDOS ? Other bits of Monero would break first anyway.

vtnerd · 2021-10-22T20:42:22Z

src/ringct/rctTypes.cpp

+        for (const BulletproofPlus &proof: proofs)
+        {
+            size_t n2 = n_bulletproof_plus_max_amounts(proof);
+            CHECK_AND_ASSERT_MES(n2 < std::numeric_limits<uint32_t>::max() - n, 0, "Invalid number of bulletproofs");


Same thing here with uint32_t.

vtnerd · 2021-11-03T02:49:33Z

src/ringct/bulletproofs_plus.cc

+
+        rct::key res = ONE;
+        if (n == 1)
+            return res;


I think this should be return x ? I don't think this case can ever trigger though.

Yes, I think you are correct, fixed.

vtnerd · 2021-11-03T03:15:29Z

src/ringct/bulletproofs_plus.cc

+
+    static rct::key weighted_inner_product(const rct::keyV &a, const epee::span<const rct::key> &b, const rct::key &y)
+    {
+        CHECK_AND_ASSERT_THROW_MES(a.size() == b.size(), "Incompatible sizes of a and b");


This could be return weighted_inner_product(epee:to_span(a), b, y); to reduce the copy-paste in here.

vtnerd · 2021-11-03T05:09:58Z

src/ringct/rctTypes.h

+          {
+            uint32_t nbp = bulletproofs_plus.size();
+            VARINT_FIELD(nbp)
+            ar.tag(type >= RCTTypeBulletproofPlus ? "bpp" : "bp");


This should always be bpp.

moneromooo-monero · 2022-01-29T18:23:43Z

Rebased to latest master.

UkoeHB

LGTM, just some minor comments.

UkoeHB · 2022-04-03T19:10:11Z

src/cryptonote_basic/cryptonote_format_utils.cpp

+            LOG_PRINT_L1("Failed to parse transaction from blob, bad bulletproofs_plus L size in tx " << get_transaction_hash(tx));
+            return false;
+          }
+          const size_t max_outputs = 1 << (rv.p.bulletproofs_plus[0].L.size() - 6);


Suggested change

const size_t max_outputs = 1 << (rv.p.bulletproofs_plus[0].L.size() - 6);

const size_t max_outputs = rct::n_bulletproof_plus_max_amounts(rv.p.bulletproofs_plus);

Would need to rearrange this to the end of the conditional (after .V is set).

What does" Would need to rearrange this to the end of the conditional (after .V is set)." mean ?

n_bulletproof_plus_max_amounts() requires a BP+ to be fully defined. At this point in the code, rv.p.bulletproofs_plus[0] is only partially-defined.

AFAICT it just needs L and R. Or do you mean we should not rely on this ?

Oh whoops, I got mixed up going back and forth between n_bulletproof_plus_max_amounts() and n_bulletproof_plus_amounts() (which needs V). The code here is valid.

UkoeHB · 2022-04-03T20:24:16Z

src/cryptonote_core/blockchain.cpp

+  // from v16, forbid bulletproofs
+  if (hf_version > HF_VERSION_BULLETPROOF_PLUS) {


Is this for a 2-step hardfork? v15 adds BP+, then v16 disables BP?

This^ is how I read it. v15 should allow both BP+ and BP as written

UkoeHB · 2022-04-03T20:47:24Z

src/ringct/bulletproofs.cc

+static const constexpr rct::key TWO = { {0x02, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00  } };
+static const constexpr rct::key MINUS_ONE = { { 0xec, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58, 0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10 } };
+static const constexpr rct::key MINUS_INV_EIGHT = { { 0x74, 0xa4, 0x19, 0x7a, 0xf0, 0x7d, 0x0b, 0xf7, 0x05, 0xc2, 0xda, 0x25, 0x2b, 0x5c, 0x0b, 0x0d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a } };


Suggested change

static const constexpr rct::key TWO = { {0x02, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 } };

static const constexpr rct::key MINUS_ONE = { { 0xec, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58, 0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10 } };

static const constexpr rct::key MINUS_INV_EIGHT = { { 0x74, 0xa4, 0x19, 0x7a, 0xf0, 0x7d, 0x0b, 0xf7, 0x05, 0xc2, 0xda, 0x25, 0x2b, 0x5c, 0x0b, 0x0d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a } };

static constexpr rct::key TWO = { {0x02, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 } };

static constexpr rct::key MINUS_ONE = { { 0xec, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58, 0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10 } };

static constexpr rct::key MINUS_INV_EIGHT = { { 0x74, 0xa4, 0x19, 0x7a, 0xf0, 0x7d, 0x0b, 0xf7, 0x05, 0xc2, 0xda, 0x25, 0x2b, 0x5c, 0x0b, 0x0d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a } };

const constexpr is redundant here afaict.

UkoeHB · 2022-04-03T21:13:03Z

src/ringct/bulletproofs_plus.cc

+    // Initial transcript hash
+    static rct::key initial_transcript;
+
+    static boost::mutex init_mutex;


Not sure what the 'other proposed changes are', but init_exponents() should use std::call_once().

UkoeHB · 2022-04-03T21:16:37Z

src/ringct/bulletproofs_plus.cc

+    static const constexpr rct::key ZERO = { {0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00  } }; // 0
+    static const constexpr rct::key ONE = { {0x01, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00  } }; // 1
+    static const constexpr rct::key TWO = { {0x02, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00  } }; // 2
+    static const constexpr rct::key MINUS_ONE = { { 0xec, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58, 0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10 } }; // -1
+    static const constexpr rct::key MINUS_INV_EIGHT = { { 0x74, 0xa4, 0x19, 0x7a, 0xf0, 0x7d, 0x0b, 0xf7, 0x05, 0xc2, 0xda, 0x25, 0x2b, 0x5c, 0x0b, 0x0d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a } }; // -(8**(-1))


Suggested change

static const constexpr rct::key ZERO = { {0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 } }; // 0

static const constexpr rct::key ONE = { {0x01, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 } }; // 1

static const constexpr rct::key TWO = { {0x02, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 } }; // 2

static const constexpr rct::key MINUS_ONE = { { 0xec, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58, 0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10 } }; // -1

static const constexpr rct::key MINUS_INV_EIGHT = { { 0x74, 0xa4, 0x19, 0x7a, 0xf0, 0x7d, 0x0b, 0xf7, 0x05, 0xc2, 0xda, 0x25, 0x2b, 0x5c, 0x0b, 0x0d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a } }; // -(8**(-1))

static constexpr rct::key ZERO = { {0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 } }; // 0

static constexpr rct::key ONE = { {0x01, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 } }; // 1

static constexpr rct::key TWO = { {0x02, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 , 0x00, 0x00, 0x00,0x00 } }; // 2

static constexpr rct::key MINUS_ONE = { { 0xec, 0xd3, 0xf5, 0x5c, 0x1a, 0x63, 0x12, 0x58, 0xd6, 0x9c, 0xf7, 0xa2, 0xde, 0xf9, 0xde, 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10 } }; // -1

static constexpr rct::key MINUS_INV_EIGHT = { { 0x74, 0xa4, 0x19, 0x7a, 0xf0, 0x7d, 0x0b, 0xf7, 0x05, 0xc2, 0xda, 0x25, 0x2b, 0x5c, 0x0b, 0x0d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0a } }; // -(8**(-1))

Again, maybe I am missing something about const constexpr here.

I see plenty of const constexpr in the source, whch hints it's not the same and this is not buggy, I'll keep it that way. I don't know the fine details though.

UkoeHB · 2022-04-03T22:44:45Z

src/ringct/rctSigs.cpp

+            rct::key sv8, sv;
+            sv = rct::zero();
+            sv.bytes[0] = outamounts[i] & 255;
+            sv.bytes[1] = (outamounts[i] >> 8) & 255;
+            sv.bytes[2] = (outamounts[i] >> 16) & 255;
+            sv.bytes[3] = (outamounts[i] >> 24) & 255;
+            sv.bytes[4] = (outamounts[i] >> 32) & 255;
+            sv.bytes[5] = (outamounts[i] >> 40) & 255;
+            sv.bytes[6] = (outamounts[i] >> 48) & 255;
+            sv.bytes[7] = (outamounts[i] >> 56) & 255;
+            sc_mul(sv8.bytes, sv.bytes, rct::INV_EIGHT.bytes);
+            rct::addKeys2(C[i], rct::INV_EIGHT, sv8, rct::H);


Suggested change

rct::key sv8, sv;

sv = rct::zero();

sv.bytes[0] = outamounts[i] & 255;

sv.bytes[1] = (outamounts[i] >> 8) & 255;

sv.bytes[2] = (outamounts[i] >> 16) & 255;

sv.bytes[3] = (outamounts[i] >> 24) & 255;

sv.bytes[4] = (outamounts[i] >> 32) & 255;

sv.bytes[5] = (outamounts[i] >> 40) & 255;

sv.bytes[6] = (outamounts[i] >> 48) & 255;

sv.bytes[7] = (outamounts[i] >> 56) & 255;

sc_mul(sv8.bytes, sv.bytes, rct::INV_EIGHT.bytes);

rct::addKeys2(C[i], rct::INV_EIGHT, sv8, rct::H);

// C[i] = (1/8)*(1 G + a[i] H)

rct::scalarmultKey(C[i], rct::commit(outamounts[i], I), rct::INV_EIGHT);

Is it buggy ?

No, it's needlessly verbose.

UkoeHB · 2022-04-03T23:43:29Z

src/wallet/wallet2.cpp

-    tx.rct_signatures.p.bulletproofs.empty() ? rct::RangeProofBorromean : rct::RangeProofPaddedBulletproof,
-    use_fork_rules(HF_VERSION_CLSAG, -10) ? 3 : use_fork_rules(HF_VERSION_SMALLER_BP, -10) ? 2 : 1


Is it ok to get rid of these old config values?

@paulshapiro raised a good point to me one time that making a change like this would make it harder to make older tx types for historical research purposes. Aside from that, seems this change would have no impact on normal users

If you want to make old tx types, you just have to checkout the right version from git. Much easier than patching current source, which you'd have to do anyway.

UkoeHB · 2022-04-03T23:48:44Z

tests/core_tests/bulletproofs.cpp

@@ -193,6 +193,7 @@ bool gen_bp_tx_validation_base::check_bp(const cryptonote::transaction &tx, size
 {
  DEFINE_TESTS_ERROR_CONTEXT(context);
  CHECK_TEST_CONDITION(tx.version >= 2);
+MGINFO("type: " << (unsigned)tx.rct_signatures.type);


UkoeHB · 2022-04-03T23:57:01Z

tests/performance_tests/bulletproof_plus.h

+// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+// 
+// Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers


Suggested change

// Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers

To make sure, did you actually verify that ? I tend to err on hte side of safety and I don't really fancy diving through git history to make sure.

These are bp+ files, which were written from scratch by you and sarang.

Copied from an existing file to start with. I suppose any leftover code, if any, is likely minimal but at least the class structure with the a_ parameters is based off original CN code.

UkoeHB · 2022-04-04T00:27:35Z

tests/core_tests/bulletproof_plus.cpp

+// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+// 
+// Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers


Suggested change

// Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers

j-berman

The higher level logic looks good to me (I didn't review the crypto). My comments are minor nits, not necessary changes for merge

j-berman · 2022-04-04T01:44:27Z

src/cryptonote_basic/cryptonote_format_utils.cpp

@@ -412,7 +441,7 @@ namespace cryptonote
  {
    CHECK_AND_ASSERT_MES(tx.pruned, std::numeric_limits<uint64_t>::max(), "get_pruned_transaction_weight does not support non pruned txes");
    CHECK_AND_ASSERT_MES(tx.version >= 2, std::numeric_limits<uint64_t>::max(), "get_pruned_transaction_weight does not support v1 txes");
-    CHECK_AND_ASSERT_MES(tx.rct_signatures.type >= rct::RCTTypeBulletproof2 || tx.rct_signatures.type == rct::RCTTypeCLSAG,
+    CHECK_AND_ASSERT_MES(tx.rct_signatures.type == rct::RCTTypeBulletproof2 || tx.rct_signatures.type == rct::RCTTypeCLSAG || tx.rct_signatures.type == rct::RCTTypeBulletproofPlus,
        std::numeric_limits<uint64_t>::max(), "get_pruned_transaction_weight does not support older range proof types");


The logic of this check is changing slightly s.t. the message could read: "unsupported rct_signature type in get_pruned_transaction_weight"

(it won't support newer types either now, not just older types)

j-berman · 2022-04-04T03:53:22Z

src/ringct/rctTypes.h

@@ -416,7 +475,7 @@ namespace rct {
            ar.end_array();
          }

-          if (type == RCTTypeCLSAG)
+          if (type == RCTTypeCLSAG || type == RCTTypeBulletproofPlus)


is_rct_clsag(type) here and in other places would be nice for readability + to reduce the chance of messing up this logic in a future change. Unfortunately in this spot in particular, would need to mess with code structure a bit to get it to compile. Not a big deal, pretty minor nit

Having them that way reduces the chances of messing up. You can move stuff around whlie still keeping CLSAGs, and is_clsag would still say true. Though you could also make up a case the other way.

j-berman · 2022-04-04T03:54:01Z

src/cryptonote_basic/cryptonote_format_utils.cpp

    weight += extra;

    // calculate deterministic CLSAG/MLSAG data size
    const size_t ring_size = boost::get<cryptonote::txin_to_key>(tx.vin[0]).key_offsets.size();
-    if (tx.rct_signatures.type == rct::RCTTypeCLSAG)
+    if (tx.rct_signatures.type == rct::RCTTypeCLSAG || tx.rct_signatures.type == rct::RCTTypeBulletproofPlus)


rct::is_rct_clsag(tx.rct_signatures.type)

j-berman · 2022-04-04T03:55:09Z

src/cryptonote_core/blockchain.cpp

@@ -3224,7 +3250,7 @@ bool Blockchain::expand_transaction_2(transaction &tx, const crypto::hash &tx_pr
      }
    }
  }
-  else if (rv.type == rct::RCTTypeCLSAG)
+  else if (rv.type == rct::RCTTypeCLSAG || rv.type == rct::RCTTypeBulletproofPlus)


rct::is_rct_clsag(rv.type)

j-berman · 2022-04-04T03:55:34Z

src/cryptonote_core/blockchain.cpp

@@ -3551,7 +3578,7 @@ bool Blockchain::check_tx_inputs(transaction& tx, tx_verification_context &tvc,
        }
      }

-      const size_t n_sigs = rv.type == rct::RCTTypeCLSAG ? rv.p.CLSAGs.size() : rv.p.MGs.size();
+      const size_t n_sigs = rv.type == rct::RCTTypeCLSAG || rv.type == rct::RCTTypeBulletproofPlus ? rv.p.CLSAGs.size() : rv.p.MGs.size();


rct::is_rct_clsag(rv.type)

j-berman · 2022-04-04T03:55:46Z

src/cryptonote_core/blockchain.cpp

@@ -3560,7 +3587,7 @@ bool Blockchain::check_tx_inputs(transaction& tx, tx_verification_context &tvc,
      for (size_t n = 0; n < tx.vin.size(); ++n)
      {
        bool error;
-        if (rv.type == rct::RCTTypeCLSAG)
+        if (rv.type == rct::RCTTypeCLSAG || rv.type == rct::RCTTypeBulletproofPlus)


rct::is_rct_clsag(rv.type)

j-berman · 2022-04-04T04:12:09Z

src/wallet/wallet2.cpp

+      ++log_padded_outputs;
+    size += (2 * (6 + log_padded_outputs) + 6) * 32 + 3;
+  }
+  else if (bulletproof)


Can combine the if (bulletproof_plus) and the else if (bulletproof) into one :

if (bulletproof_plus || bulletproof) { size_t log_padded_outputs = 0; while ((1<<log_padded_outputs) < n_outputs) ++log_padded_outputs; size += (2 * (6 + log_padded_outputs) + (bulletproof_plus ? 6 : 9)) * 32 + 3; }

j-berman · 2022-04-04T04:33:41Z

src/ringct/rctSigs.cpp

-        if (!proofs.empty() && !verBulletproof(proofs))
+        if (!bpp_proofs.empty() && !verBulletproofPlus(bpp_proofs))
+        {
+          if (!waiter.wait())


Confused why this is here:

if (!waiter.wait()) return false;

Probably because I was printing stuff from the results when debugging. Removed.

Actually that's to avoid going past the scope while results (and possibly others) are still used by the threads. The thread dtor will wait, but it's a failsafe, you're supposed to wait first. I'll redo and add it to the other error path.

j-berman · 2022-04-04T06:59:58Z

src/wallet/wallet2.cpp

-    tx.rct_signatures.p.bulletproofs.empty() ? rct::RangeProofBorromean : rct::RangeProofPaddedBulletproof,
-    use_fork_rules(HF_VERSION_CLSAG, -10) ? 3 : use_fork_rules(HF_VERSION_SMALLER_BP, -10) ? 2 : 1


@paulshapiro raised a good point to me one time that making a change like this would make it harder to make older tx types for historical research purposes. Aside from that, seems this change would have no impact on normal users

j-berman · 2022-04-04T07:25:49Z

src/cryptonote_core/blockchain.cpp

+  // from v16, forbid bulletproofs
+  if (hf_version > HF_VERSION_BULLETPROOF_PLUS) {


This^ is how I read it. v15 should allow both BP+ and BP as written

https://suyash67.github.io/homepage/assets/pdfs/bulletproofs_plus_audit_report_v1.1.pdf

moneromooo-monero · 2022-04-04T09:09:31Z

There are two comments that I don't get a "reply" widget for:

Not sure what the 'other proposed changes are', but init_exponents() should use std::call_once().

I don't understand the first part about proposed changes. There is no such string in the quoted patch.
About std::call_once, why ? If it's not buggy, I'll keep it that way unless there's a good reason to change.

Agreed

There's just that word. Missing comment/question there ? It's on a line that just has a prototype.

moneromooo-monero · 2022-04-04T12:09:26Z

Pushed with a rebase to current master, the changes from these two recent reviews are in the last commit, which will get squashed with the relevant others once reviewed

UkoeHB · 2022-04-04T12:44:24Z

There are two comments that I don't get a "reply" widget for:

These are replies to @vtnerd's review comments. Github shows 'prototypes' for replies that are added in later reviews.

UkoeHB · 2022-04-04T12:46:30Z

src/cryptonote_basic/cryptonote_format_utils.cpp

@@ -179,7 +179,7 @@ namespace cryptonote
            LOG_PRINT_L1("Failed to parse transaction from blob, bad bulletproofs_plus L size in tx " << get_transaction_hash(tx));
            return false;
          }
-          const size_t max_outputs = 1 << (rv.p.bulletproofs_plus[0].L.size() - 6);
+          const size_t max_outputs = rct::n_bulletproof_plus_max_amounts(rv.p.bulletproofs_plus[0]);


This must go below the line rv.p.bulletproofs_plus[0].V[i] = rv.outPk[i].mask;. The precondition of n_bulletproof_plus_max_amounts() is that the bp+ passed in is fully defined.

EDIT: nvm, this function only needs L and R.

Maybe I'm missing your point, but if you do that, you'll get the max_output check after the resize, which means you can make it allocate a large amount of memory. And AFAICT n_bulletproof_plus_max_amounts does not need V.

UkoeHB · 2022-04-04T12:49:39Z

src/ringct/rctTypes.cpp

@@ -251,7 +251,8 @@ namespace rct {
        }
    }

-    size_t n_bulletproof_amounts(const Bulletproof &proof)
+    template<typename proof_t>
+    static size_t n_bulletproof_bulletproof_plus_amounts(const proof_t &proof)


If you are going to do this, then you need to assert that BULLETPROOF_MAX_OUTPUTS == BULLETPROOF_PLUS_MAX_OUTPUTS, and maybe static assert that proof_t is BulletProof or BulletProofPlus. My recommended function signature was meant to decouple the function from the implementation details of the BP proofs.

Good point on the first one.

UkoeHB

I still think init_exponents() in ringct/bulletproofs_plus.cc should use std::call_once() (which matches the semantics of that function), and make_dummy_bulletproof_plus() in ringct/rctSigs.cpp can be cleaned up. However, the PR appears correct and merge-able.

It avoids dividing by 8 when deserializing a tx, which is a slow operation, and multiplies by 8 when verifying and extracing the amount, which is much faster as well as less frequent

UkoeHB

LGTM, only diff with my local copy is adding waiters to verRctSemanticsSimple().

vtnerd reviewed Feb 10, 2021

View reviewed changes

moneromooo-monero force-pushed the bp+c branch 3 times, most recently from f460f21 to 68032bf Compare February 20, 2021 11:37

moneromooo-monero force-pushed the bp+c branch from 68032bf to 6d446f5 Compare October 4, 2021 12:20

moneromooo-monero force-pushed the bp+c branch 2 times, most recently from 0dc7182 to 1d43476 Compare October 11, 2021 18:41

vtnerd reviewed Nov 3, 2021

View reviewed changes

moneromooo-monero force-pushed the bp+c branch from 1d43476 to 61f892d Compare November 5, 2021 10:24

j-berman mentioned this pull request Nov 15, 2021

Add view tags to outputs to reduce wallet scanning time #8061

Merged

sethforprivacy mentioned this pull request Nov 19, 2021

Monero v15 hard-fork planning monero-project/meta#630

Closed

erciccione mentioned this pull request Jan 27, 2022

Monero Dev Meeting: v15 network upgrade - Sat 29 January 2022 @ 17:00 UTC monero-project/meta#655

Closed

moneromooo-monero force-pushed the bp+c branch 2 times, most recently from 3c5dfa1 to 37e8cfe Compare January 29, 2022 18:23

rbrunner7 mentioned this pull request Mar 27, 2022

Monero Dev Meeting: v15 network upgrade - Sat 2 April 2022 @ 17:00 UTC monero-project/meta#680

Closed

UkoeHB suggested changes Apr 4, 2022

View reviewed changes

UkoeHB mentioned this pull request Apr 4, 2022

Monero Multisig for HF #8237

Closed

j-berman approved these changes Apr 4, 2022

View reviewed changes

SarangNoether added 3 commits April 4, 2022 08:23

Bulletproofs+

bd27deb

Precompute initial transcript hash

b7713cc

Updates from security audit

b535d66

https://suyash67.github.io/homepage/assets/pdfs/bulletproofs_plus_audit_report_v1.1.pdf

moneromooo-monero force-pushed the bp+c branch from 37e8cfe to 0293fc8 Compare April 4, 2022 12:08

UkoeHB suggested changes Apr 4, 2022

View reviewed changes

moneromooo-monero force-pushed the bp+c branch from 0293fc8 to bfc05cd Compare April 4, 2022 13:39

UkoeHB approved these changes Apr 4, 2022

View reviewed changes

j-berman approved these changes Apr 4, 2022

View reviewed changes

moneromooo-monero added 5 commits April 5, 2022 18:50

plug bulletproofs plus into consensus

a0d80b1

ringct: a few minor optimizations from review

75bd004

ringct: port some of vtnerd's review changes from BP+ to BP

a345060

store outPk/8 in the tx for speed

4c94cfe

It avoids dividing by 8 when deserializing a tx, which is a slow operation, and multiplies by 8 when verifying and extracing the amount, which is much faster as well as less frequent

bulletproofs+: some minor cleanup from vtnerd's review

5acdd0e

moneromooo-monero force-pushed the bp+c branch from bfc05cd to 5acdd0e Compare April 5, 2022 18:52

UkoeHB approved these changes Apr 5, 2022

View reviewed changes

j-berman approved these changes Apr 5, 2022

View reviewed changes

luigi1111 merged commit d054def into monero-project:master Apr 6, 2022

	const size_t max_outputs = 1 << (rv.p.bulletproofs_plus[0].L.size() - 6);
	const size_t max_outputs = rct::n_bulletproof_plus_max_amounts(rv.p.bulletproofs_plus);

		// from v16, forbid bulletproofs
		if (hf_version > HF_VERSION_BULLETPROOF_PLUS) {

		tx.rct_signatures.p.bulletproofs.empty() ? rct::RangeProofBorromean : rct::RangeProofPaddedBulletproof,
		use_fork_rules(HF_VERSION_CLSAG, -10) ? 3 : use_fork_rules(HF_VERSION_SMALLER_BP, -10) ? 2 : 1

Bulletproof+ by sarang, tied to consensus #7170

Bulletproof+ by sarang, tied to consensus #7170

Conversation

moneromooo-monero commented Dec 18, 2020 • edited Loading

vtnerd 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

vtnerd Feb 12, 2021 • 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

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

vtnerd commented Sep 8, 2021

selsta commented Oct 11, 2021

selsta commented Oct 11, 2021

vtnerd 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

moneromooo-monero commented Jan 29, 2022

UkoeHB 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

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

j-berman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-berman Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

moneromooo-monero Apr 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

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

moneromooo-monero commented Dec 18, 2020 •

edited

Loading

vtnerd Feb 12, 2021 •

edited

Loading

j-berman Apr 4, 2022 •

edited

Loading

moneromooo-monero Apr 4, 2022 •

edited

Loading