jeffro comments

Author: j-berman

src/blockchain_db/blockchain_db.cpp

@@ -236,8 +236,7 @@ void BlockchainDB::add_transaction(const crypto::hash& blk_hash, const transacti
     if (miner_tx && tx.version == 2)
     {
       cryptonote::tx_out vout = tx.vout[i];
-      // TODO: avoid multiple expensive zeroCommitVartime call here + get_outs_by_last_locked_block + ver_non_input_consensus
-      rct::key commitment = rct::getCommitment(tx, i);
+      rct::key commitment = rct::getCtKey(tx, i).mask;
       vout.amount = 0;
       amount_output_indices[i] = add_output(tx_hash, vout, i, tx.unlock_time,
         &commitment);

src/blockchain_db/blockchain_db_utils.cpp

@@ -66,23 +66,19 @@ static uint64_t set_tx_outs_by_last_locked_block(const cryptonote::transaction &
     {
         const auto &out = tx.vout[i];
 
-        crypto::public_key output_public_key;
-        if (!cryptonote::get_output_public_key(out, output_public_key))
-            throw std::runtime_error("Could not get an output public key from a tx output.");
-
         static_assert(CURRENT_TRANSACTION_VERSION == 2, "This section of code was written with 2 tx versions in mind. "
             "Revisit this section and update for the new tx version.");
         CHECK_AND_ASSERT_THROW_MES(tx.version == 1 || tx.version == 2, "encountered unexpected tx version");
 
         TIME_MEASURE_NS_START(getting_commitment);
 
-        rct::key commitment = rct::getCommitment(tx, i);
+        rct::ctkey ctKey = rct::getCtKey(tx, i);
 
         TIME_MEASURE_NS_FINISH(getting_commitment);
 
         auto output_pair = fcmp_pp::curve_trees::OutputPair{
-                .output_pubkey = std::move(output_public_key),
-                .commitment    = std::move(commitment)
+                .output_pubkey = std::move(rct::rct2pk(ctKey.dest)),
+                .commitment    = std::move(ctKey.mask)
             };
 
         const uint64_t output_id = first_output_id + i;

src/cryptonote_core/tx_verification_utils.cpp

@@ -445,13 +445,11 @@ bool collect_pubkeys_and_commitments(const transaction& tx, std::vector<rct::key
 {
     for (std::size_t i = 0; i < tx.vout.size(); ++i)
     {
-        crypto::public_key output_pubkey;
-        if (!cryptonote::get_output_public_key(tx.vout[i], output_pubkey))
-            return false;
-
-        rct::key pubkey = rct::pk2rct(output_pubkey);
-        pubkeys_and_commitments_inout.emplace_back(std::move(pubkey));
-        pubkeys_and_commitments_inout.emplace_back(rct::getCommitment(tx, i));
+        rct::ctkey ctKey;
+        try { ctKey = rct::getCtKey(tx, i); }
+        catch (...) { return false; }
+        pubkeys_and_commitments_inout.emplace_back(std::move(ctKey.dest));
+        pubkeys_and_commitments_inout.emplace_back(std::move(ctKey.mask));
     }
 
     return true;

src/fcmp_pp/curve_trees.h

@@ -123,9 +123,14 @@ struct OutputContext final
     uint64_t output_id{0};
     // TODO: consider using a variant instead
     // True if the output pair elems are guaranteed to not have torsion and are not equal to identity
-    bool torsion_checked{false};
+    uint8_t torsion_checked{0};
     OutputPair output_pair;
 
+    // Using uint8_t for torsion_checked because sizeof(bool) is platform-dependent: https://github.com/seraphis-migration/monero/pull/20#discussion_r2027787352
+    // These static asserts ensure booleans cast to the expected uint8_t value and vice versa
+    static_assert((uint8_t) 0 == (uint8_t) false && (uint8_t) 1 == (uint8_t) true, "Unexpected bool <> uint8_t casting");
+    static_assert((bool) ((uint8_t) 0) == false && (bool) ((uint8_t) 1) == true, "Unexpected uint8_t <> bool casting");
+
     bool operator==(const OutputContext &other) const { return output_id == other.output_id && output_pair == other.output_pair; }
 
     template <class Archive>

src/ringct/rctOps.h

@@ -45,7 +45,6 @@ extern "C" {
 #include "rctCryptoOps.h"
 }
 #include "crypto/crypto.h"
-#include "cryptonote_basic/cryptonote_basic.h"
 
 #include "rctTypes.h"
 

src/ringct/rctSigs.cpp

@@ -1751,15 +1751,29 @@ namespace rct {
       return decodeRctSimple(rv, sk, i, mask, hwdev);
     }
 
-    key getCommitment(const cryptonote::transaction &tx, const std::size_t output_idx) {
-      const bool miner_tx = cryptonote::is_coinbase(tx);
-      if (miner_tx || tx.version < 2)
+    ctkey getCtKey(const cryptonote::transaction &tx, const std::size_t output_idx) {
+      if (tx.rct_signatures.outPk.size() > output_idx)
       {
-        CHECK_AND_ASSERT_THROW_MES(tx.vout.size() > output_idx, "unexpected size of vout");
-        return zeroCommitVartime(tx.vout[output_idx].amount);
+        return tx.rct_signatures.outPk[output_idx];
       }
-      CHECK_AND_ASSERT_THROW_MES(tx.rct_signatures.outPk.size() > output_idx, "unexpected size of outPk");
-      return tx.rct_signatures.outPk[output_idx].mask;
+
+      // Expand tx.rct_signatures.outPk to include the output pubkey and commitment, so that we don't have to call
+      // the expensive zeroCommitVartime multiple times unnecessarily.
+      // If we don't expand outPk in order of its outputs, then the if statement above could return the incorrect mask.
+      CHECK_AND_ASSERT_THROW_MES(tx.rct_signatures.outPk.size() == output_idx, "must expand outPk in order");
+
+      // We expect all txs to have outPk expanded already, except for coinbase and v1 txs
+      const bool miner_tx = cryptonote::is_coinbase(tx);
+      CHECK_AND_ASSERT_THROW_MES(miner_tx || tx.version < 2, "unexpected tx missing commitment");
+
+      crypto::public_key output_public_key;
+      CHECK_AND_ASSERT_THROW_MES(tx.vout.size() > output_idx, "tx.vout too small");
+      CHECK_AND_ASSERT_THROW_MES(cryptonote::get_output_public_key(tx.vout[output_idx], output_public_key),
+        "failed to get output public key");
+
+      auto outPk = ctkey{.dest = rct::pk2rct(output_public_key), .mask = zeroCommitVartime(tx.vout[output_idx].amount)};
+      tx.rct_signatures.outPk.emplace_back(std::move(outPk));
+      return tx.rct_signatures.outPk.back();
     }
 
     bool verPointsForTorsion(const std::vector<key> & pts) {

src/ringct/rctSigs.h

@@ -142,7 +142,7 @@ namespace rct {
     xmr_amount decodeRctSimple(const rctSig & rv, const key & sk, unsigned int i, key & mask, hw::device &hwdev);
     xmr_amount decodeRctSimple(const rctSig & rv, const key & sk, unsigned int i, hw::device &hwdev);
     key get_pre_mlsag_hash(const rctSig &rv, hw::device &hwdev);
-    key getCommitment(const cryptonote::transaction &tx, const std::size_t output_idx);
+    ctkey getCtKey(const cryptonote::transaction &tx, const std::size_t output_idx);
 
     // Make sure points are valid points, don't have torsion, and are not equal to identity
     bool verPointsForTorsion(const std::vector<key> & pts);

src/ringct/rctTypes.h

@@ -325,7 +325,7 @@ namespace rct {
         //pairs that you mix with
         keyV pseudoOuts; //C - for simple rct
         std::vector<ecdhTuple> ecdhInfo;
-        ctkeyV outPk;
+        mutable ctkeyV outPk; // mutable so that getCtKey can expand it
         xmr_amount txnFee; // contains b
 
         rctSigBase() :