wallet: don't clear out all the blinding data when dropping change

The Elements 22 blinding logic has an edge case where when we drop change,
leaving only a single blinded output, we recompute a bunch of blinding
data to handle the potential for us to have 0 inputs and 1 output to blind.
(BlindTransaction will fail in this case because it cannot make the
transaction balance with only one output to mess with.)

In this recomputation, we dropped more data than we meant to, causing us
to incorrectly blind an output.

src/wallet/spend.cpp

@@ -718,25 +718,29 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, const uin
 }
 
 // Reset all non-global blinding details.
-void resetBlindDetails(BlindDetails* det) {
+static void resetBlindDetails(BlindDetails* det, bool preserve_output_data = false) {
     det->i_amount_blinds.clear();
     det->i_asset_blinds.clear();
     det->i_assets.clear();
     det->i_amounts.clear();
 
     det->o_amounts.clear();
-    det->o_pubkeys.clear();
+    if (!preserve_output_data) {
+        det->o_pubkeys.clear();
+    }
     det->o_amount_blinds.clear();
     det->o_assets.clear();
     det->o_asset_blinds.clear();
 
-    det->num_to_blind = 0;
-    det->change_to_blind = 0;
-    det->only_recipient_blind_index = -1;
-    det->only_change_pos = -1;
+    if (!preserve_output_data) {
+        det->num_to_blind = 0;
+        det->change_to_blind = 0;
+        det->only_recipient_blind_index = -1;
+        det->only_change_pos = -1;
+    }
 }
 
-bool fillBlindDetails(BlindDetails* det, CWallet* wallet, CMutableTransaction& txNew, std::vector<CInputCoin>& selected_coins, bilingual_str& error) {
+static bool fillBlindDetails(BlindDetails* det, CWallet* wallet, CMutableTransaction& txNew, std::vector<CInputCoin>& selected_coins, bilingual_str& error) {
     int num_inputs_blinded = 0;
 
     // Fill in input blinding details
@@ -1393,15 +1397,15 @@ bool CWallet::CreateTransactionInternal(
                     blind_details->num_to_blind--;
                     blind_details->change_to_blind--;
 
-                    // FIXME: I promise this makes sense and fixes an actual problem
-                    // with the wallet that users could encounter. But no human could
-                    // follow the logic as to what this does or why it is safe. After
-                    // the 22.0 rebase we need to double-back and replace the blinding
-                    // logic to eliminate a bunch of edge cases and make this logic
-                    // incomprehensible. But in the interest of minimizing diff during
-                    // the rebase I am going to do this for now.
+                    // FIXME: If we drop the change *and* this means we have only one
+                    //  blinded output *and* we have no blinded inputs, then this puts
+                    //  us in a situation where BlindTransaction will fail. This is
+                    //  prevented in fillBlindDetails, which adds an OP_RETURN output
+                    //  to handle this case. So do this ludicrous hack to accomplish
+                    //  this. This whole lump of un-followable-logic needs to be replaced
+                    //  by a complete rewriting of the wallet blinding logic.
                     if (blind_details->num_to_blind == 1) {
-                        resetBlindDetails(blind_details);
+                        resetBlindDetails(blind_details, true /* don't wipe output data */);
                         if (!fillBlindDetails(blind_details, this, txNew, selected_coins, error)) {
                             return false;
                         }
@@ -1535,6 +1539,7 @@ bool CWallet::CreateTransactionInternal(
             int ret = BlindTransaction(blind_details->i_amount_blinds, blind_details->i_asset_blinds, blind_details->i_assets, blind_details->i_amounts, blind_details->o_amount_blinds, blind_details->o_asset_blinds,  blind_details->o_pubkeys, issuance_asset_keys, issuance_token_keys, txNew);
             assert(ret != -1);
             if (ret != blind_details->num_to_blind) {
+                WalletLogPrintf("ERROR: tried to blind %d outputs but only blinded %d\n", (int) blind_details->num_to_blind, (int) ret);
                 error = _("Unable to blind the transaction properly. This should not happen.");
                 return false;
             }

test/functional/elements_regression_1172.py

@@ -0,0 +1,73 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017-2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test blinding logic when change is dropped and we have only one other blinded input
+
+Constructs a transaction with a sufficiently small change output that it
+gets dropped, in which there is only one other blinded input. In the case
+that we have no blinded inputs, we would need to add an OP_RETURN output
+to the transaction, neccessitating special logic.
+
+Check that this special logic still results in a correct transaction that
+sends the money to the desired recipient (and that the recipient is able
+to receive/spend the money).
+"""
+
+from decimal import Decimal
+
+from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+    satoshi_round,
+)
+
+class WalletCtTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 3
+        self.extra_args = [[
+            "-blindedaddresses=1",
+            "-initialfreecoins=2100000000000000",
+            "-con_blocksubsidy=0",
+            "-con_connect_genesis_outputs=1",
+            "-txindex=1",
+        ]] * self.num_nodes
+        self.extra_args[0].append("-anyonecanspendaremine=1") # first node gets the coins
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def run_test(self):
+        # Mine 101 blocks to get the initial coins out of IBD
+        self.nodes[0].generate(COINBASE_MATURITY + 1)
+        self.nodes[0].syncwithvalidationinterfacequeue()
+        self.sync_all()
+
+        for i in range(self.num_nodes):
+            self.log.info(f"Starting with node {i} balance: {self.nodes[i].getbalance()}")
+
+        # Send 1 coin to a new wallet
+        txid = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 1)
+        self.log.info(f"Sent one coin to node 1 in {txid}")
+        self.nodes[0].generate(2)
+        self.sync_all()
+
+        # Try to send those coins to yet another wallet, sending a large enough amount
+        # that the change output is dropped.
+        amt = satoshi_round(Decimal(0.9995))
+        txid = self.nodes[1].sendtoaddress(self.nodes[2].getnewaddress(), amt)
+        self.log.info(f"Sent {amt} LBTC to node 2 in {txid}")
+        self.nodes[1].generate(2)
+        self.sync_all()
+
+        for i in range(self.num_nodes):
+            self.log.info(f"Finished with node {i} balance: {self.nodes[i].getbalance()}")
+
+        assert_equal(self.nodes[1].getbalance(), { "bitcoin": Decimal(0) })
+        assert_equal(self.nodes[2].getbalance(), { "bitcoin": amt })
+
+if __name__ == '__main__':
+    WalletCtTest().main()
+

test/functional/test_runner.py

@@ -109,6 +109,7 @@ BASE_SCRIPTS = [
     'feature_initial_reissuance_token.py',
     'feature_progress.py',
     'rpc_getnewblockhex.py',
+    'elements_regression_1172.py',
     # Longest test should go first, to favor running tests in parallel
     'wallet_hd.py --legacy-wallet',
     'wallet_hd.py --descriptors',