Auto merge of #2440 - bitcartel:1360_reduce_info_leakage_vpub_new, r=str4d

Fix intermediate vpub_new leakage in multi joinsplit tx

Resolves information leakage discussed here https://github.com/zcash/zcash/issues/1360#issuecomment-284561331

Example output in debug.log showing how intermediate vpub_new is now 0
https://gist.github.com/bitcartel/86ca316b74f0b1ec6d8de3ac132006da

When reviewing, note that the indentation of a large chunk of code (scenario 3 part 2) changed, so use `git diff -w` (or similar) or append `?w=1` to the Github URL e.g. https://github.com/zcash/zcash/pull/2440/files?w=1

src/wallet/asyncrpcoperation_sendmany.cpp

@@ -342,9 +342,14 @@ bool AsyncRPCOperation_sendmany::main_impl() {
     tx_ = CTransaction(mtx);
 
     // Copy zinputs and zoutputs to more flexible containers
-    std::deque<SendManyInputJSOP> zInputsDeque;
+    std::deque<SendManyInputJSOP> zInputsDeque; // zInputsDeque stores minimum numbers of notes for target amount
+    CAmount tmp = 0;
     for (auto o : z_inputs_) {
         zInputsDeque.push_back(o);
+        tmp += std::get<2>(o);
+        if (tmp >= targetAmount) {
+            break;
+        }
     }
     std::deque<SendManyRecipient> zOutputsDeque;
     for (auto o : z_outputs_) {
@@ -442,103 +447,25 @@ bool AsyncRPCOperation_sendmany::main_impl() {
      * zaddr -> taddrs
      *       -> zaddrs
      * 
-     * Processing order:
+     * Send to zaddrs by chaining JoinSplits together and immediately consuming any change
-     * Part 1: taddrs and miners fee
+     * Send to taddrs by creating dummy z outputs and accumulating value in a change note
-     * Part 2: zaddrs 
+     * which is used to set vpub_new in the last chained joinsplit.
-     */
-    
-    /**
-     * SCENARIO #3
-     * Part 1: Add to the transparent value pool.
      */
     UniValue obj(UniValue::VOBJ);
     CAmount jsChange = 0;   // this is updated after each joinsplit
     int changeOutputIndex = -1; // this is updated after each joinsplit if jsChange > 0
-    bool minersFeeProcessed = false;
+    bool vpubNewProcessed = false;  // updated when vpub_new for miner fee and taddr outputs is set in last joinsplit
-
+    CAmount vpubNewTarget = minersFee;
     if (t_outputs_total > 0) {
         add_taddr_outputs_to_tx();
-        CAmount taddrTargetAmount = t_outputs_total + minersFee;
+        vpubNewTarget += t_outputs_total;
-        minersFeeProcessed = true;
-        while (zInputsDeque.size() > 0 && taddrTargetAmount > 0) {
-            AsyncJoinSplitInfo info;
-            info.vpub_old = 0;
-            info.vpub_new = 0;
-            std::vector<JSOutPoint> outPoints;
-            int n = 0;
-            while (n++ < ZC_NUM_JS_INPUTS && taddrTargetAmount > 0) {
-                SendManyInputJSOP o = zInputsDeque.front();
-                JSOutPoint outPoint = std::get<0>(o);
-                Note note = std::get<1>(o);
-                CAmount noteFunds = std::get<2>(o);
-                zInputsDeque.pop_front();
-
-                info.notes.push_back(note);
-                outPoints.push_back(outPoint);
-
-                int wtxHeight = -1;
-                int wtxDepth = -1;
-                {
-                    LOCK2(cs_main, pwalletMain->cs_wallet);
-                    const CWalletTx& wtx = pwalletMain->mapWallet[outPoint.hash];
-                    wtxHeight = mapBlockIndex[wtx.hashBlock]->nHeight;
-                    wtxDepth = wtx.GetDepthInMainChain();
     }
-                LogPrint("zrpcunsafe", "%s: spending note (txid=%s, vjoinsplit=%d, ciphertext=%d, amount=%s, height=%d, confirmations=%d)\n",
-                        getId(),
-                        outPoint.hash.ToString().substr(0, 10),
-                        outPoint.js,
-                        int(outPoint.n), // uint8_t
-                        FormatMoney(noteFunds),
-                        wtxHeight,
-                        wtxDepth
-                        );
-
-                
-                // Put value back into the value pool
-                if (noteFunds >= taddrTargetAmount) {
-                    jsChange = noteFunds - taddrTargetAmount;
-                    info.vpub_new += taddrTargetAmount;
-                } else {
-                    info.vpub_new += noteFunds;
-                }
-
-                taddrTargetAmount -= noteFunds;
-                if (taddrTargetAmount <= 0) {
-                    break;
-                }
-            }
-
-            if (jsChange > 0) {
-                info.vjsout.push_back(JSOutput());
-                info.vjsout.push_back(JSOutput(frompaymentaddress_, jsChange));
-                
-                LogPrint("zrpcunsafe", "%s: generating note for change (amount=%s)\n",
-                        getId(),
-                        FormatMoney(jsChange)
-                        );
-            }
-
-            obj = perform_joinsplit(info, outPoints);
-
-            if (jsChange > 0) {
-                changeOutputIndex = find_output(obj, 1);
-            }
-        }
-    }
-
-
-    /**
-     * SCENARIO #3
-     * Part 2: Send to zaddrs by chaining JoinSplits together and immediately consuming any change
-     */            
-    if (z_outputs_total>0) {
 
     // Keep track of treestate within this transaction
     boost::unordered_map<uint256, ZCIncrementalMerkleTree, CCoinsKeyHasher> intermediates;
     std::vector<uint256> previousCommitments;
 
-        while (zOutputsDeque.size() > 0) {
+    while (!vpubNewProcessed) {
         AsyncJoinSplitInfo info;
         info.vpub_old = 0;
         info.vpub_new = 0;
@@ -699,26 +626,37 @@ bool AsyncRPCOperation_sendmany::main_impl() {
             std::copy(vInputNotes.begin(), vInputNotes.end(), std::back_inserter(info.notes));
         }
 
-            
-            //
         // Find recipient to transfer funds to
-            //            
+        std::string address, hexMemo;
+        CAmount value = 0;
+        if (zOutputsDeque.size() > 0) {
             SendManyRecipient smr = zOutputsDeque.front();
-            std::string address = std::get<0>(smr);
+            address = std::get<0>(smr);
-            CAmount value = std::get<1>(smr);
+            value = std::get<1>(smr);
-            std::string hexMemo = std::get<2>(smr);
+            hexMemo = std::get<2>(smr);
             zOutputsDeque.pop_front();
+        }
 
-            // Will we have any change?  Has the miners fee been processed yet?
+        // Reset change
         jsChange = 0;
         CAmount outAmount = value;
-            if (!minersFeeProcessed) {
-                if (jsInputValue < minersFee) {
-                    throw JSONRPCError(RPC_WALLET_ERROR, "Not enough funds to pay miners fee");
-                }
-                outAmount += minersFee;
-            }
 
+        // Set vpub_new in the last joinsplit (when there are no more notes to spend or zaddr outputs to satisfy)
+        if (zOutputsDeque.size() == 0 && zInputsDeque.size() == 0) {
+            assert(!vpubNewProcessed);
+            if (jsInputValue < vpubNewTarget) {
+                throw JSONRPCError(RPC_WALLET_ERROR,
+                    strprintf("Insufficient funds for vpub_new %s (miners fee %s, taddr outputs %s)",
+                    FormatMoney(vpubNewTarget), FormatMoney(minersFee), FormatMoney(t_outputs_total)));
+            }
+            outAmount += vpubNewTarget;
+            info.vpub_new += vpubNewTarget; // funds flowing back to public pool
+            vpubNewProcessed = true;
+            jsChange = jsInputValue - outAmount;
+            assert(jsChange >= 0);
+        }
+        else {
+            // This is not the last joinsplit, so compute change and any amount still due to the recipient
             if (jsInputValue > outAmount) {
                 jsChange = jsInputValue - outAmount;
             } else if (outAmount > jsInputValue) {
@@ -729,23 +667,21 @@ bool AsyncRPCOperation_sendmany::main_impl() {
 
                 // reduce the amount being sent right now to the value of all inputs
                 value = jsInputValue;
-                if (!minersFeeProcessed) {
-                    value -= minersFee;
             }
         }
 
-            if (!minersFeeProcessed) {
-                minersFeeProcessed = true;
-                info.vpub_new += minersFee; // funds flowing back to public pool
-            }
-            
         // create output for recipient
+        if (address.empty()) {
+            assert(value==0);
+            info.vjsout.push_back(JSOutput());  // dummy output while we accumulate funds into a change note for vpub_new
+        } else {
             PaymentAddress pa = CZCPaymentAddress(address).Get();
             JSOutput jso = JSOutput(pa, value);
             if (hexMemo.size() > 0) {
                 jso.memo = get_memo_from_hex_string(hexMemo);
             }
             info.vjsout.push_back(jso);
+        }
 
         // create output for any change
         if (jsChange>0) {
@@ -763,7 +699,11 @@ bool AsyncRPCOperation_sendmany::main_impl() {
             changeOutputIndex = find_output(obj, 1);
         }
     }
-    }
+
+    // Sanity check in case changes to code block above exits loop by invoking 'break'
+    assert(zInputsDeque.size() == 0);
+    assert(zOutputsDeque.size() == 0);
+    assert(vpubNewProcessed);
 
     sign_send_raw_transaction(obj);
     return true;