Auto merge of #1561 - str4d:778-randomise-note-order, r=daira

Randomize JoinSplit input and output orders

Closes #778.

src/Makefile.gtest.include

@@ -4,6 +4,7 @@ bin_PROGRAMS += zcash-gtest
 # tool for generating our public parameters
 zcash_gtest_SOURCES = \
 	gtest/main.cpp \
+	gtest/utils.cpp \
 	gtest/test_checktransaction.cpp \
 	gtest/json_test_vectors.cpp \
         gtest/json_test_vectors.h \
@@ -17,7 +18,9 @@ zcash_gtest_SOURCES = \
 	gtest/test_noteencryption.cpp \
 	gtest/test_merkletree.cpp \
 	gtest/test_pow.cpp \
+	gtest/test_random.cpp \
 	gtest/test_rpc.cpp \
+	gtest/test_transaction.cpp \
 	gtest/test_circuit.cpp \
 	gtest/test_txid.cpp \
 	gtest/test_libzcash_utils.cpp \

src/gtest/test_random.cpp

@@ -0,0 +1,27 @@
+#include <gtest/gtest.h>
+
+#include "random.h"
+
+extern int GenZero(int n);
+extern int GenMax(int n);
+
+TEST(Random, MappedShuffle) {
+    std::vector<int> a {8, 4, 6, 3, 5};
+    std::vector<int> m {0, 1, 2, 3, 4};
+
+    auto a1 = a;
+    auto m1 = m;
+    MappedShuffle(a1.begin(), m1.begin(), a1.size(), GenZero);
+    std::vector<int> ea1 {4, 6, 3, 5, 8};
+    std::vector<int> em1 {1, 2, 3, 4, 0};
+    EXPECT_EQ(ea1, a1);
+    EXPECT_EQ(em1, m1);
+
+    auto a2 = a;
+    auto m2 = m;
+    MappedShuffle(a2.begin(), m2.begin(), a2.size(), GenMax);
+    std::vector<int> ea2 {8, 4, 6, 3, 5};
+    std::vector<int> em2 {0, 1, 2, 3, 4};
+    EXPECT_EQ(ea2, a2);
+    EXPECT_EQ(em2, m2);
+}

src/gtest/test_transaction.cpp

@@ -0,0 +1,85 @@
+#include <gtest/gtest.h>
+
+#include "primitives/transaction.h"
+#include "zcash/Note.hpp"
+#include "zcash/Address.hpp"
+
+extern ZCJoinSplit* params;
+extern int GenZero(int n);
+extern int GenMax(int n);
+
+TEST(Transaction, JSDescriptionRandomized) {
+    // construct a merkle tree
+    ZCIncrementalMerkleTree merkleTree;
+
+    libzcash::SpendingKey k = libzcash::SpendingKey::random();
+    libzcash::PaymentAddress addr = k.address();
+
+    libzcash::Note note(addr.a_pk, 100, uint256(), uint256());
+
+    // commitment from coin
+    uint256 commitment = note.cm();
+
+    // insert commitment into the merkle tree
+    merkleTree.append(commitment);
+
+    // compute the merkle root we will be working with
+    uint256 rt = merkleTree.root();
+
+    auto witness = merkleTree.witness();
+
+    // create JSDescription
+    uint256 pubKeyHash;
+    boost::array<libzcash::JSInput, ZC_NUM_JS_INPUTS> inputs = {
+        libzcash::JSInput(witness, note, k),
+        libzcash::JSInput() // dummy input of zero value
+    };
+    boost::array<libzcash::JSOutput, ZC_NUM_JS_OUTPUTS> outputs = {
+        libzcash::JSOutput(addr, 50),
+        libzcash::JSOutput(addr, 50)
+    };
+    boost::array<size_t, ZC_NUM_JS_INPUTS> inputMap;
+    boost::array<size_t, ZC_NUM_JS_OUTPUTS> outputMap;
+
+    {
+        auto jsdesc = JSDescription::Randomized(
+            *params, pubKeyHash, rt,
+            inputs, outputs,
+            inputMap, outputMap,
+            0, 0, false);
+
+        std::set<size_t> inputSet(inputMap.begin(), inputMap.end());
+        std::set<size_t> expectedInputSet {0, 1};
+        EXPECT_EQ(expectedInputSet, inputSet);
+
+        std::set<size_t> outputSet(outputMap.begin(), outputMap.end());
+        std::set<size_t> expectedOutputSet {0, 1};
+        EXPECT_EQ(expectedOutputSet, outputSet);
+    }
+
+    {
+        auto jsdesc = JSDescription::Randomized(
+            *params, pubKeyHash, rt,
+            inputs, outputs,
+            inputMap, outputMap,
+            0, 0, false, GenZero);
+
+        boost::array<size_t, ZC_NUM_JS_INPUTS> expectedInputMap {1, 0};
+        boost::array<size_t, ZC_NUM_JS_OUTPUTS> expectedOutputMap {1, 0};
+        EXPECT_EQ(expectedInputMap, inputMap);
+        EXPECT_EQ(expectedOutputMap, outputMap);
+    }
+
+    {
+        auto jsdesc = JSDescription::Randomized(
+            *params, pubKeyHash, rt,
+            inputs, outputs,
+            inputMap, outputMap,
+            0, 0, false, GenMax);
+
+        boost::array<size_t, ZC_NUM_JS_INPUTS> expectedInputMap {0, 1};
+        boost::array<size_t, ZC_NUM_JS_OUTPUTS> expectedOutputMap {0, 1};
+        EXPECT_EQ(expectedInputMap, inputMap);
+        EXPECT_EQ(expectedOutputMap, outputMap);
+    }
+}

src/gtest/utils.cpp

@@ -0,0 +1,13 @@
+#include "zcash/JoinSplit.hpp"
+
+ZCJoinSplit* params = ZCJoinSplit::Unopened();
+
+int GenZero(int n)
+{
+    return 0;
+}
+
+int GenMax(int n)
+{
+    return n-1;
+}

src/primitives/transaction.cpp

@@ -41,6 +41,30 @@ JSDescription::JSDescription(ZCJoinSplit& params,
     );
 }
 
+JSDescription JSDescription::Randomized(
+            ZCJoinSplit& params,
+            const uint256& pubKeyHash,
+            const uint256& anchor,
+            boost::array<libzcash::JSInput, ZC_NUM_JS_INPUTS>& inputs,
+            boost::array<libzcash::JSOutput, ZC_NUM_JS_OUTPUTS>& outputs,
+            boost::array<size_t, ZC_NUM_JS_INPUTS>& inputMap,
+            boost::array<size_t, ZC_NUM_JS_OUTPUTS>& outputMap,
+            CAmount vpub_old,
+            CAmount vpub_new,
+            bool computeProof,
+            std::function<int(int)> gen)
+{
+    // Randomize the order of the inputs and outputs
+    inputMap = {0, 1};
+    outputMap = {0, 1};
+    MappedShuffle(inputs.begin(), inputMap.begin(), ZC_NUM_JS_INPUTS, gen);
+    MappedShuffle(outputs.begin(), outputMap.begin(), ZC_NUM_JS_OUTPUTS, gen);
+
+    return JSDescription(
+        params, pubKeyHash, anchor, inputs, outputs,
+        vpub_old, vpub_new, computeProof);
+}
+
 bool JSDescription::Verify(
     ZCJoinSplit& params,
     const uint256& pubKeyHash

src/primitives/transaction.h

@@ -7,6 +7,7 @@
 #define BITCOIN_PRIMITIVES_TRANSACTION_H
 
 #include "amount.h"
+#include "random.h"
 #include "script/script.h"
 #include "serialize.h"
 #include "uint256.h"
@@ -78,6 +79,20 @@ public:
             bool computeProof = true // Set to false in some tests
     );
 
+    static JSDescription Randomized(
+            ZCJoinSplit& params,
+            const uint256& pubKeyHash,
+            const uint256& rt,
+            boost::array<libzcash::JSInput, ZC_NUM_JS_INPUTS>& inputs,
+            boost::array<libzcash::JSOutput, ZC_NUM_JS_OUTPUTS>& outputs,
+            boost::array<size_t, ZC_NUM_JS_INPUTS>& inputMap,
+            boost::array<size_t, ZC_NUM_JS_OUTPUTS>& outputMap,
+            CAmount vpub_old,
+            CAmount vpub_new,
+            bool computeProof = true, // Set to false in some tests
+            std::function<int(int)> gen = GetRandInt
+    );
+
     // Verifies that the JoinSplit proof is correct.
     bool Verify(ZCJoinSplit& params, const uint256& pubKeyHash) const;
 

src/random.h

@@ -8,6 +8,7 @@
 
 #include "uint256.h"
 
+#include <functional>
 #include <stdint.h>
 
 /**
@@ -24,6 +25,31 @@ uint64_t GetRand(uint64_t nMax);
 int GetRandInt(int nMax);
 uint256 GetRandHash();
 
+/**
+ * Rearranges the elements in the range [first,first+len) randomly, assuming
+ * that gen is a uniform random number generator. Follows the same algorithm as
+ * std::shuffle in C++11 (a Durstenfeld shuffle).
+ *
+ * The elements in the range [mapFirst,mapFirst+len) are rearranged according to
+ * the same permutation, enabling the permutation to be tracked by the caller.
+ *
+ * gen takes an integer n and produces a uniform random output in [0,n).
+ */
+template <typename RandomAccessIterator, typename MapRandomAccessIterator>
+void MappedShuffle(RandomAccessIterator first,
+                   MapRandomAccessIterator mapFirst,
+                   size_t len,
+                   std::function<int(int)> gen)
+{
+    for (size_t i = len-1; i > 0; --i) {
+        auto r = gen(i+1);
+        assert(r >= 0);
+        assert(r <= i);
+        std::swap(first[i], first[r]);
+        std::swap(mapFirst[i], mapFirst[r]);
+    }
+}
+
 /**
  * Seed insecure_rand using the random pool.
  * @param Deterministic Use a deterministic seed

src/test/transaction_tests.cpp

@@ -367,8 +367,6 @@ BOOST_AUTO_TEST_CASE(test_basic_joinsplit_verification)
         test.anchor = GetRandHash();
         BOOST_CHECK(!test.Verify(*p, pubKeyHash));
     }
-
-    delete p;
 }
 
 BOOST_AUTO_TEST_CASE(test_simple_joinsplit_invalidity)

src/wallet/asyncrpcoperation_sendmany.cpp

@@ -29,6 +29,23 @@
 
 using namespace libzcash;
 
+int find_output(Object obj, int n) {
+    Value outputMapValue = find_value(obj, "outputmap");
+    if (outputMapValue.type() != array_type) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Missing outputmap for JoinSplit operation");
+    }
+
+    Array outputMap = outputMapValue.get_array();
+    assert(outputMap.size() == ZC_NUM_JS_OUTPUTS);
+    for (size_t i = 0; i < outputMap.size(); i++) {
+        if (outputMap[i] == n) {
+            return i;
+        }
+    }
+
+    throw std::logic_error("n is not present in outputmap");
+}
+
 AsyncRPCOperation_sendmany::AsyncRPCOperation_sendmany(
         std::string fromAddress,
         std::vector<SendManyRecipient> tOutputs,
@@ -372,6 +389,7 @@ bool AsyncRPCOperation_sendmany::main_impl() {
      */
     Object obj;
     CAmount jsChange = 0;   // this is updated after each joinsplit
+    int changeOutputIndex = -1; // this is updated after each joinsplit if jsChange > 0
     bool minersFeeProcessed = false;
 
     if (t_outputs_total > 0) {
@@ -429,6 +447,10 @@ bool AsyncRPCOperation_sendmany::main_impl() {
             }
 
             obj = perform_joinsplit(info, outPoints);
+
+            if (jsChange > 0) {
+                changeOutputIndex = find_output(obj, 1);
+            }
         }
     }
 
@@ -442,9 +464,6 @@ bool AsyncRPCOperation_sendmany::main_impl() {
         // Keep track of treestate within this transaction 
         boost::unordered_map<uint256, ZCIncrementalMerkleTree, CCoinsKeyHasher> intermediates;
         std::vector<uint256> previousCommitments;
-
-        // NOTE: Randomization of input and output order could break this in future
-        const int changeOutputIndex = 1;
         
         while (zOutputsDeque.size() > 0) {
             AsyncJoinSplitInfo info;
@@ -483,7 +502,6 @@ bool AsyncRPCOperation_sendmany::main_impl() {
                     throw JSONRPCError(RPC_WALLET_ERROR, "Could not find previous JoinSplit anchor");
                 }
                 
-                // NOTE: We assume the last commitment, output 1, is the change we want
                 for (const uint256& commitment : prevJoinSplit.commitments) {
                     tree.append(commitment);
                     previousCommitments.push_back(commitment);                   
@@ -645,6 +663,10 @@ bool AsyncRPCOperation_sendmany::main_impl() {
             }
 
             obj = perform_joinsplit(info, witnesses, jsAnchor);
+
+            if (jsChange > 0) {
+                changeOutputIndex = find_output(obj, 1);
+            }
         }
     }
 
@@ -845,11 +867,20 @@ Object AsyncRPCOperation_sendmany::perform_joinsplit(
             );
 
     // Generate the proof, this can take over a minute.
-    JSDescription jsdesc(*pzcashParams,
+    boost::array<libzcash::JSInput, ZC_NUM_JS_INPUTS> inputs
+            {info.vjsin[0], info.vjsin[1]};
+    boost::array<libzcash::JSOutput, ZC_NUM_JS_OUTPUTS> outputs
+            {info.vjsout[0], info.vjsout[1]};
+    boost::array<size_t, ZC_NUM_JS_INPUTS> inputMap;
+    boost::array<size_t, ZC_NUM_JS_OUTPUTS> outputMap;
+    JSDescription jsdesc = JSDescription::Randomized(
+            *pzcashParams,
             joinSplitPubKey_,
             anchor,
-            {info.vjsin[0], info.vjsin[1]},
+            inputs,
-            {info.vjsout[0], info.vjsout[1]},
+            outputs,
+            inputMap,
+            outputMap,
             info.vpub_old,
             info.vpub_new,
             !this->testmode);
@@ -910,10 +941,21 @@ Object AsyncRPCOperation_sendmany::perform_joinsplit(
         encryptedNote2 = HexStr(ss2.begin(), ss2.end());
     }
 
+    Array arrInputMap;
+    Array arrOutputMap;
+    for (size_t i = 0; i < ZC_NUM_JS_INPUTS; i++) {
+        arrInputMap.push_back(inputMap[i]);
+    }
+    for (size_t i = 0; i < ZC_NUM_JS_OUTPUTS; i++) {
+        arrOutputMap.push_back(outputMap[i]);
+    }
+
     Object obj;
     obj.push_back(Pair("encryptednote1", encryptedNote1));
     obj.push_back(Pair("encryptednote2", encryptedNote2));
     obj.push_back(Pair("rawtxn", HexStr(ss.begin(), ss.end())));
+    obj.push_back(Pair("inputmap", arrInputMap));
+    obj.push_back(Pair("outputmap", arrOutputMap));
     return obj;
 }
 

src/wallet/gtest/test_wallet.cpp

@@ -13,7 +13,7 @@
 
 using ::testing::Return;
 
-ZCJoinSplit* params = ZCJoinSplit::Unopened();
+extern ZCJoinSplit* params;
 
 ACTION(ThrowLogicError) {
     throw std::logic_error("Boom");