Merge pull request #1067

70b78bb wallet: fix misdetection of duplicates (moneromooo-monero)
7d413f6 rct: rework serialization to avoid storing vector sizes (moneromooo-monero)
0ce79ef core: cleanup some typecasting (moneromooo-monero)

src/cryptonote_core/cryptonote_basic.h

@@ -230,24 +230,22 @@ namespace cryptonote
       }
       else
       {
-        FIELD(rct_signatures)
+        ar.tag("rct_signatures");
-        switch (rct_signatures.type)
+        if (!vin.empty())
         {
-        case rct::RCTTypeNull:
+          ar.begin_object();
-          break;
+          bool r = rct_signatures.serialize_rctsig_base(ar, vin.size(), vout.size());
-        case rct::RCTTypeSimple:
+          if (!r || !ar.stream().good()) return false;
-          if (rct_signatures.mixRing.size() && rct_signatures.mixRing.size() != vin.size())
+          ar.end_object();
-            return false;
+          if (rct_signatures.type != rct::RCTTypeNull)
-          break;
-        case rct::RCTTypeFull:
-          for (size_t i = 0; i < rct_signatures.mixRing.size(); ++i)
           {
-            if (rct_signatures.mixRing[i].size() != vin.size())
+            ar.tag("rctsig_prunable");
-              return false;
+            ar.begin_object();
+            r = rct_signatures.p.serialize_rctsig_prunable(ar, rct_signatures.type, vin.size(), vout.size(),
+                vin[0].type() == typeid(txin_to_key) ? boost::get<txin_to_key>(vin[0]).key_offsets.size() - 1 : 0);
+            if (!r || !ar.stream().good()) return false;
+            ar.end_object();
           }
-          break;
-        default:
-          return false;
         }
       }
     END_SERIALIZE()

src/cryptonote_core/cryptonote_boost_serialization.h

@@ -162,11 +162,16 @@ namespace boost
     a & x.vout;
     a & x.extra;
     if (x.version == 1)
+    {
       a & x.signatures;
-    else
-      a & x.rct_signatures;
     }
-
+    else
+    {
+      a & (rct::rctSigBase&)x.rct_signatures;
+      if (x.rct_signatures.type != rct::RCTTypeNull)
+        a & x.rct_signatures.p;
+    }
+  }
 
   template <class Archive>
   inline void serialize(Archive &a, cryptonote::block &b, const boost::serialization::version_type ver)
@@ -262,6 +267,13 @@ namespace boost
     a & x.txnFee;
   }
 
+  template <class Archive>
+  inline void serialize(Archive &a, rct::rctSigPrunable &x, const boost::serialization::version_type ver)
+  {
+    a & x.rangeSigs;
+    a & x.MGs;
+  }
+
   template <class Archive>
   inline void serialize(Archive &a, rct::rctSig &x, const boost::serialization::version_type ver)
   {

src/cryptonote_core/cryptonote_format_utils.cpp

@@ -471,11 +471,11 @@ namespace cryptonote
   //---------------------------------------------------------------
   bool construct_tx_and_get_tx_key(const account_keys& sender_account_keys, const std::vector<tx_source_entry>& sources, const std::vector<tx_destination_entry>& destinations, std::vector<uint8_t> extra, transaction& tx, uint64_t unlock_time, crypto::secret_key &tx_key, bool rct)
   {
-    std::vector<crypto::secret_key> amount_keys;
+    std::vector<rct::key> amount_keys;
     tx.vin.clear();
     tx.vout.clear();
     tx.signatures.clear();
-    tx.rct_signatures = rct::rctSig();
+    tx.rct_signatures.type = rct::RCTTypeNull;
     amount_keys.clear();
 
     tx.version = rct ? 2 : 1;
@@ -593,7 +593,7 @@ namespace cryptonote
       {
         crypto::secret_key scalar1;
         crypto::derivation_to_scalar(derivation, output_index, scalar1);
-        amount_keys.push_back(scalar1);
+        amount_keys.push_back(rct::sk2rct(scalar1));
       }
       r = crypto::derive_public_key(derivation, output_index, dst_entr.addr.m_spend_public_key, out_eph_public_key);
       CHECK_AND_ASSERT_MES(r, false, "at creation outs: failed to derive_public_key(" << derivation << ", " << output_index << ", "<< dst_entr.addr.m_spend_public_key << ")");
@@ -750,9 +750,9 @@ namespace cryptonote
       get_transaction_prefix_hash(tx, tx_prefix_hash);
       rct::ctkeyV outSk;
       if (use_simple_rct)
-        tx.rct_signatures = rct::genRctSimple(rct::hash2rct(tx_prefix_hash), inSk, destinations, inamounts, outamounts, amount_in - amount_out, mixRing, (const rct::keyV&)amount_keys, index, outSk);
+        tx.rct_signatures = rct::genRctSimple(rct::hash2rct(tx_prefix_hash), inSk, destinations, inamounts, outamounts, amount_in - amount_out, mixRing, amount_keys, index, outSk);
       else
-        tx.rct_signatures = rct::genRct(rct::hash2rct(tx_prefix_hash), inSk, destinations, outamounts, mixRing, (const rct::keyV&)amount_keys, sources[0].real_output, outSk); // same index assumption
+        tx.rct_signatures = rct::genRct(rct::hash2rct(tx_prefix_hash), inSk, destinations, outamounts, mixRing, amount_keys, sources[0].real_output, outSk); // same index assumption
 
       CHECK_AND_ASSERT_MES(tx.vout.size() == outSk.size(), false, "outSk size does not match vout");
 
@@ -948,11 +948,35 @@ namespace cryptonote
     // prefix
     get_transaction_prefix_hash(t, hashes[0]);
 
-    // base rct data
+    transaction &tt = const_cast<transaction&>(t);
-    get_blob_hash(t_serializable_object_to_blob((const rct::rctSigBase&)t.rct_signatures), hashes[1]);
 
-    // prunable rct data
+    // base rct
-    get_blob_hash(t_serializable_object_to_blob(t.rct_signatures.p), hashes[2]);
+    {
+      std::stringstream ss;
+      binary_archive<true> ba(ss);
+      const size_t inputs = t.vin.size();
+      const size_t outputs = t.vout.size();
+      bool r = tt.rct_signatures.serialize_rctsig_base(ba, inputs, outputs);
+      CHECK_AND_ASSERT_MES(r, false, "Failed to serialize rct signatures base");
+      cryptonote::get_blob_hash(ss.str(), hashes[1]);
+    }
+
+    // prunable rct
+    if (t.rct_signatures.type == rct::RCTTypeNull)
+    {
+      hashes[2] = cryptonote::null_hash;
+    }
+    else
+    {
+      std::stringstream ss;
+      binary_archive<true> ba(ss);
+      const size_t inputs = t.vin.size();
+      const size_t outputs = t.vout.size();
+      const size_t mixin = t.vin.empty() ? 0 : t.vin[0].type() == typeid(txin_to_key) ? boost::get<txin_to_key>(t.vin[0]).key_offsets.size() - 1 : 0;
+      bool r = tt.rct_signatures.p.serialize_rctsig_prunable(ba, t.rct_signatures.type, inputs, outputs, mixin);
+      CHECK_AND_ASSERT_MES(r, false, "Failed to serialize rct signatures prunable");
+      cryptonote::get_blob_hash(ss.str(), hashes[2]);
+    }
 
     // the tx hash is the hash of the 3 hashes
     res = cn_fast_hash(hashes, sizeof(hashes));

src/ringct/rctSigs.cpp

@@ -350,8 +350,16 @@ namespace rct {
       keyV hashes;
       hashes.push_back(rv.message);
       crypto::hash h;
-      cryptonote::get_blob_hash(cryptonote::t_serializable_object_to_blob((const rctSigBase&)rv), h);
+
+      std::stringstream ss;
+      binary_archive<true> ba(ss);
+      const size_t inputs = rv.pseudoOuts.size();
+      const size_t outputs = rv.ecdhInfo.size();
+      CHECK_AND_ASSERT_THROW_MES(const_cast<rctSig&>(rv).serialize_rctsig_base(ba, inputs, outputs),
+          "Failed to serialize rctSigBase");
+      cryptonote::get_blob_hash(ss.str(), h);
       hashes.push_back(hash2rct(h));
+
       keyV kv;
       for (auto r: rv.p.rangeSigs)
       {
@@ -364,7 +372,6 @@ namespace rct {
           kv.push_back(r.Ci[n]);
       }
       hashes.push_back(cn_fast_hash(kv));
-
       return cn_fast_hash(hashes);
     }
 

src/ringct/rctTypes.h

@@ -188,49 +188,136 @@ namespace rct {
         ctkeyV outPk;
         xmr_amount txnFee; // contains b
 
-        BEGIN_SERIALIZE()
+        template<bool W, template <bool> class Archive>
+        bool serialize_rctsig_base(Archive<W> &ar, size_t inputs, size_t outputs)
+        {
           FIELD(type)
           if (type == RCTTypeNull)
             return true;
+          if (type != RCTTypeFull && type != RCTTypeSimple)
+            return false;
+          VARINT_FIELD(txnFee)
+          // inputs/outputs not saved, only here for serialization help
           // FIELD(message) - not serialized, it can be reconstructed
           // FIELD(mixRing) - not serialized, it can be reconstructed
           if (type == RCTTypeSimple)
-              FIELD(pseudoOuts)
+          {
-            FIELD(ecdhInfo)
+            ar.tag("pseudoOuts");
-            if (typename Archive<W>::is_saving()) {
+            ar.begin_array();
-              keyV outPk(this->outPk.size());
+            PREPARE_CUSTOM_VECTOR_SERIALIZATION(inputs, pseudoOuts);
-              for (size_t n = 0; n < outPk.size(); ++n)
+            if (pseudoOuts.size() != inputs)
-                outPk[n] = this->outPk[n].mask;
+              return false;
-              FIELD(outPk)
+            for (size_t i = 0; i < inputs; ++i)
+            {
+              FIELDS(pseudoOuts[i])
+              if (inputs - i > 1)
+                ar.delimit_array();
             }
-            else {
+            ar.end_array();
-              keyV outPk;
+          }
-              FIELD(outPk)
+
-              this->outPk.resize(outPk.size());
+          ar.tag("ecdhInfo");
-              for (size_t n = 0; n < outPk.size(); ++n)
+          ar.begin_array();
-                this->outPk[n].mask = outPk[n];
+          PREPARE_CUSTOM_VECTOR_SERIALIZATION(outputs, ecdhInfo);
+          if (ecdhInfo.size() != outputs)
+            return false;
+          for (size_t i = 0; i < outputs; ++i)
+          {
+            FIELDS(ecdhInfo[i])
+            if (outputs - i > 1)
+              ar.delimit_array();
+          }
+          ar.end_array();
+
+          ar.tag("outPk");
+          ar.begin_array();
+          PREPARE_CUSTOM_VECTOR_SERIALIZATION(outputs, outPk);
+          if (outPk.size() != outputs)
+            return false;
+          for (size_t i = 0; i < outputs; ++i)
+          {
+            FIELDS(outPk[i].mask)
+            if (outputs - i > 1)
+              ar.delimit_array();
+          }
+          ar.end_array();
+          return true;
         }
-            VARINT_FIELD(txnFee)
-        END_SERIALIZE()
     };
     struct rctSigPrunable {
         vector<rangeSig> rangeSigs;
         vector<mgSig> MGs; // simple rct has N, full has 1
 
-        BEGIN_SERIALIZE()
+        template<bool W, template <bool> class Archive>
-            FIELD(rangeSigs)
+        bool serialize_rctsig_prunable(Archive<W> &ar, uint8_t type, size_t inputs, size_t outputs, size_t mixin)
-            FIELD(MGs)
+        {
-        END_SERIALIZE()
+          if (type == RCTTypeNull)
+            return true;
+          if (type != RCTTypeFull && type != RCTTypeSimple)
+            return false;
+          ar.tag("rangeSigs");
+          ar.begin_array();
+          PREPARE_CUSTOM_VECTOR_SERIALIZATION(outputs, rangeSigs);
+          if (rangeSigs.size() != outputs)
+            return false;
+          for (size_t i = 0; i < outputs; ++i)
+          {
+            FIELDS(rangeSigs[i])
+            if (outputs - i > 1)
+              ar.delimit_array();
+          }
+          ar.end_array();
+
+          ar.tag("MGs");
+          ar.begin_array();
+          // we keep a byte for size of MGs, because we don't know whether this is
+          // a simple or full rct signature, and it's starting to annoy the hell out of me
+          size_t mg_elements = type == RCTTypeSimple ? inputs : 1;
+          PREPARE_CUSTOM_VECTOR_SERIALIZATION(mg_elements, MGs);
+          if (MGs.size() != mg_elements)
+            return false;
+          for (size_t i = 0; i < mg_elements; ++i)
+          {
+            // we save the MGs contents directly, because we want it to save its
+            // arrays and matrices without the size prefixes, and the load can't
+            // know what size to expect if it's not in the data
+            ar.tag("ss");
+            ar.begin_array();
+            PREPARE_CUSTOM_VECTOR_SERIALIZATION(mixin + 1, MGs[i].ss);
+            if (MGs[i].ss.size() != mixin + 1)
+              return false;
+            for (size_t j = 0; j < mixin + 1; ++j)
+            {
+              ar.begin_array();
+              size_t mg_ss2_elements = (type == RCTTypeSimple ? 1 : inputs) + 1;
+              PREPARE_CUSTOM_VECTOR_SERIALIZATION(mg_ss2_elements, MGs[i].ss[j]);
+              if (MGs[i].ss[j].size() != mg_ss2_elements)
+                return false;
+              for (size_t k = 0; k < mg_ss2_elements; ++k)
+              {
+                FIELDS(MGs[i].ss[j][k])
+                if (mg_ss2_elements - j > 1)
+                  ar.delimit_array();
+              }
+              ar.end_array();
+
+              if (mixin + 1 - j > 1)
+                ar.delimit_array();
+            }
+            ar.end_array();
+
+            FIELDS(MGs[i].cc)
+            // MGs[i].II not saved, it can be reconstructed
+            if (mg_elements - i > 1)
+              ar.delimit_array();
+          }
+          ar.end_array();
+          return true;
+        }
+
     };
     struct rctSig: public rctSigBase {
         rctSigPrunable p;
-
-        BEGIN_SERIALIZE_OBJECT()
-            FIELDS(*static_cast<rctSigBase *>(this))
-            if (type == RCTTypeNull)
-              return true;
-            FIELDS(p);
-        END_SERIALIZE()
     };
 
     //other basepoint H = toPoint(cn_fast_hash(G)), G the basepoint

src/wallet/wallet2.cpp

@@ -2806,9 +2806,10 @@ void wallet2::get_outs(std::vector<std::vector<entry>> &outs, const std::list<tr
           continue;
         if (!daemon_resp.outs[i].unlocked) // don't add locked outs
           continue;
-        if (o > 0 && daemon_resp.outs[i].key == daemon_resp.outs[i - 1].key) // don't add duplicates
+        auto item = std::make_tuple(req.outputs[i].index, daemon_resp.outs[i].key, daemon_resp.outs[i].mask);
+        if (std::find(outs.back().begin(), outs.back().end(), item) != outs.back().end()) // don't add duplicates
           continue;
-        outs.back().push_back(std::make_tuple(req.outputs[i].index, daemon_resp.outs[i].key, daemon_resp.outs[i].mask));
+        outs.back().push_back(item);
       }
       if (outs.back().size() < fake_outputs_count + 1)
       {
@@ -2818,15 +2819,6 @@ void wallet2::get_outs(std::vector<std::vector<entry>> &outs, const std::list<tr
       {
         // sort the subsection, so any spares are reset in order
         std::sort(outs.back().begin(), outs.back().end(), [](const entry &a, const entry &b) { return std::get<0>(a) < std::get<0>(b); });
-
-        // sanity check
-        for (size_t n = 1; n < outs.back().size(); ++n)
-        {
-          THROW_WALLET_EXCEPTION_IF(std::get<0>(outs.back()[n]) == std::get<0>(outs.back()[n-1]), error::wallet_internal_error,
-              "Duplicate indices though we did not ask for any");
-          THROW_WALLET_EXCEPTION_IF(std::get<1>(outs.back()[n]) == std::get<1>(outs.back()[n-1]), error::wallet_internal_error,
-              "Duplicate keys after we have weeded them out");
-        }
       }
       base += requested_outputs_count;
     }

tests/unit_tests/serialization.cpp

@@ -589,6 +589,7 @@ TEST(Serialization, serializes_ringct_types)
   ASSERT_TRUE(serialization::parse_binary(blob, rg1));
   ASSERT_TRUE(!memcmp(&rg0, &rg1, sizeof(rg0)));
 
+#if 0
   ASSERT_TRUE(serialization::dump_binary(s0, blob));
   ASSERT_TRUE(serialization::parse_binary(blob, s1));
   ASSERT_TRUE(s0.type == s1.type);
@@ -621,16 +622,18 @@ TEST(Serialization, serializes_ringct_types)
     // serialization only does the mask
     ASSERT_TRUE(!memcmp(&s0.outPk[n].mask, &s1.outPk[n].mask, sizeof(s0.outPk[n].mask)));
   }
+#endif
 
   tx0.set_null();
   tx0.version = 2;
   cryptonote::txin_to_key txin_to_key1;
-  txin_to_key1.key_offsets.resize(2);
+  txin_to_key1.key_offsets.resize(4);
   cryptonote::txin_to_key txin_to_key2;
-  txin_to_key2.key_offsets.resize(2);
+  txin_to_key2.key_offsets.resize(4);
   tx0.vin.push_back(txin_to_key1);
   tx0.vin.push_back(txin_to_key2);
   tx0.vout.push_back(cryptonote::tx_out());
+  tx0.vout.push_back(cryptonote::tx_out());
   tx0.rct_signatures = s0;
   ASSERT_EQ(tx0.rct_signatures.p.rangeSigs.size(), 2);
   ASSERT_TRUE(serialization::dump_binary(tx0, blob));