diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 316ac45b35..00c9dfc90a 100644 --- a/src/wallet/spend.cpp +++ b/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& selected_coins, bilingual_str& error) { +static bool fillBlindDetails(BlindDetails* det, CWallet* wallet, CMutableTransaction& txNew, std::vector& 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; } diff --git a/test/functional/elements_regression_1172.py b/test/functional/elements_regression_1172.py new file mode 100755 index 0000000000..79d48dde86 --- /dev/null +++ b/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() + diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index ab0268f922..ef2c7865cf 100755 --- a/test/functional/test_runner.py +++ b/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',