diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e0912d4acf..3bd0ff9fc8 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 @@ -1032,9 +1036,13 @@ bool CWallet::CreateTransactionInternal( if (!coin_selection_params.m_subtract_fee_outputs) { coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size) if (g_con_elementsmode) { - coin_selection_params.tx_noinputs_size += 44; // change output: 9 bytes value, 1 byte scriptPubKey, 33 bytes asset, 1 byte nonce + coin_selection_params.tx_noinputs_size += 46; // fee output: 9 bytes value, 1 byte scriptPubKey, 33 bytes asset, 1 byte nonce, 1 byte each for null rangeproof/surjectionproof } } + // ELEMENTS: If we have blinded inputs but no blinded outputs (which, since the wallet + // makes an effort to not produce change, is a common case) then we need to add a + // dummy output. + bool may_need_blinded_dummy = !!blind_details; for (const auto& recipient : vecSend) { CTxOut txout(recipient.asset, recipient.nAmount, recipient.scriptPubKey); @@ -1056,6 +1064,7 @@ bool CWallet::CreateTransactionInternal( if (blind_details) { blind_details->o_pubkeys.push_back(recipient.confidentiality_key); if (blind_details->o_pubkeys.back().IsFullyValid()) { + may_need_blinded_dummy = false; blind_details->num_to_blind++; blind_details->only_recipient_blind_index = txNew.vout.size()-1; if (!coin_selection_params.m_subtract_fee_outputs) { @@ -1064,6 +1073,35 @@ bool CWallet::CreateTransactionInternal( } } } + if (may_need_blinded_dummy && !coin_selection_params.m_subtract_fee_outputs) { + // dummy output: 33 bytes value, 2 byte scriptPubKey, 33 bytes asset, 1 byte nonce, 66 bytes dummy rangeproof, 1 byte null surjectionproof + // FIXME actually, we currently just hand off to BlindTransaction which will put + // a full rangeproof and surjectionproof. We should fix this when we overhaul + // the blinding logic. + coin_selection_params.tx_noinputs_size += 70 + 66 +(MAX_RANGEPROOF_SIZE + DEFAULT_SURJECTIONPROOF_SIZE + WITNESS_SCALE_FACTOR - 1)/WITNESS_SCALE_FACTOR; + } + // If we are going to issue an asset, add the issuance data to the noinputs_size so that + // we allocate enough coins for them. + if (issuance_details) { + size_t issue_count = 0; + for (unsigned int i = 0; i < txNew.vout.size(); i++) { + if (txNew.vout[i].nAsset.IsExplicit() && txNew.vout[i].nAsset.GetAsset() == CAsset(uint256S("1"))) { + issue_count++; + } else if (txNew.vout[i].nAsset.IsExplicit() && txNew.vout[i].nAsset.GetAsset() == CAsset(uint256S("2"))) { + issue_count++; + } + } + if (issue_count > 0) { + // Allocate space for blinding nonce, entropy, and whichever of nAmount/nInflationKeys is null + coin_selection_params.tx_noinputs_size += 2 * 32 + 2 * (2 - issue_count); + } + // Allocate non-null nAmount/nInflationKeys and rangeproofs + if (issuance_details->blind_issuance) { + coin_selection_params.tx_noinputs_size += issue_count * (33 * WITNESS_SCALE_FACTOR + MAX_RANGEPROOF_SIZE + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR; + } else { + coin_selection_params.tx_noinputs_size += issue_count * 9; + } + } // Include the fees for things that aren't inputs, excluding the change output const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); @@ -1087,6 +1125,17 @@ bool CWallet::CreateTransactionInternal( return false; } + // If all of our inputs are explicit, we don't need a blinded dummy + if (may_need_blinded_dummy) { + may_need_blinded_dummy = false; + for (const auto& coin : setCoins) { + if (!coin.txout.nValue.IsExplicit()) { + may_need_blinded_dummy = true; + break; + } + } + } + // Always make a change output // We will reduce the fee from this change output later, and remove the output if it is too small. // ELEMENTS: wrap this all in a loop, set nChangePosInOut specifically for policy asset @@ -1342,36 +1391,46 @@ bool CWallet::CreateTransactionInternal( CAmount change_amount = change_position->nValue.GetAmount(); if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change) { - txNew.vout.erase(change_position); + bool was_blinded = blind_details && blind_details->o_pubkeys[nChangePosInOut].IsValid(); - change_pos[nChangePosInOut] = std::nullopt; - tx_blinded.vout.erase(tx_blinded.vout.begin() + nChangePosInOut); - if (tx_blinded.witness.vtxoutwit.size() > (unsigned) nChangePosInOut) { - tx_blinded.witness.vtxoutwit.erase(tx_blinded.witness.vtxoutwit.begin() + nChangePosInOut); - } - if (blind_details) { - bool was_blinded = blind_details->o_pubkeys[nChangePosInOut].IsValid(); + // If the change was blinded, and was the only blinded output, we cannot drop it + // without causing the transaction to fail to balance. So keep it, and merely + // zero it out. + if (was_blinded && blind_details->num_to_blind == 1) { + assert (may_need_blinded_dummy); + change_position->scriptPubKey = CScript() << OP_RETURN; + change_position->nValue = 0; + } else { + txNew.vout.erase(change_position); - blind_details->o_amounts.erase(blind_details->o_amounts.begin() + nChangePosInOut); - blind_details->o_assets.erase(blind_details->o_assets.begin() + nChangePosInOut); - blind_details->o_pubkeys.erase(blind_details->o_pubkeys.begin() + nChangePosInOut); - // If change_amount == 0, we did not increment num_to_blind initially - // and therefore do not need to decrement it here. - if (was_blinded) { - blind_details->num_to_blind--; - blind_details->change_to_blind--; + change_pos[nChangePosInOut] = std::nullopt; + tx_blinded.vout.erase(tx_blinded.vout.begin() + nChangePosInOut); + if (tx_blinded.witness.vtxoutwit.size() > (unsigned) nChangePosInOut) { + tx_blinded.witness.vtxoutwit.erase(tx_blinded.witness.vtxoutwit.begin() + nChangePosInOut); + } + if (blind_details) { - // 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. - if (blind_details->num_to_blind == 1) { - resetBlindDetails(blind_details); - if (!fillBlindDetails(blind_details, this, txNew, selected_coins, error)) { - return false; + blind_details->o_amounts.erase(blind_details->o_amounts.begin() + nChangePosInOut); + blind_details->o_assets.erase(blind_details->o_assets.begin() + nChangePosInOut); + blind_details->o_pubkeys.erase(blind_details->o_pubkeys.begin() + nChangePosInOut); + // If change_amount == 0, we did not increment num_to_blind initially + // and therefore do not need to decrement it here. + if (was_blinded) { + blind_details->num_to_blind--; + blind_details->change_to_blind--; + + // 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 < 2) { + resetBlindDetails(blind_details, true /* don't wipe output data */); + if (!fillBlindDetails(blind_details, this, txNew, selected_coins, error)) { + return false; + } } } } @@ -1385,6 +1444,10 @@ bool CWallet::CreateTransactionInternal( fee_needed = coin_selection_params.m_effective_feerate.GetFee(nBytes); } + // The only time that fee_needed should be less than the amount available for fees (in change_and_fee - change_amount) is when + // we are subtracting the fee from the outputs. If this occurs at any other time, it is a bug. + assert(coin_selection_params.m_subtract_fee_outputs || fee_needed <= map_change_and_fee.at(policyAsset) - change_amount); + // Update nFeeRet in case fee_needed changed due to dropping the change output if (fee_needed <= map_change_and_fee.at(policyAsset) - change_amount) { nFeeRet = map_change_and_fee.at(policyAsset) - change_amount; @@ -1479,9 +1542,9 @@ bool CWallet::CreateTransactionInternal( summary += strprintf("#%d: %s%s [%s] (%s [%s])\n", i, txNew.vout[i].IsFee() ? "[fee] " : "", unblinded.nValue.GetAmount(), - txNew.vout[i].nValue.IsExplicit() ? "explicit" : "blinded", + blind_details->o_pubkeys[i].IsValid() ? "blinded" : "explicit", unblinded.nAsset.GetAsset().GetHex(), - txNew.vout[i].nAsset.IsExplicit() ? "explicit" : "blinded" + blind_details->o_pubkeys[i].IsValid() ? "blinded" : "explicit" ); } WalletLogPrintf(summary+"\n"); @@ -1498,6 +1561,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/test_runner.py b/test/functional/test_runner.py index ab0268f922..c8f2703bdd 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', + 'wallet_elements_regression_1172.py', # Longest test should go first, to favor running tests in parallel 'wallet_hd.py --legacy-wallet', 'wallet_hd.py --descriptors', diff --git a/test/functional/wallet_elements_regression_1172.py b/test/functional/wallet_elements_regression_1172.py new file mode 100755 index 0000000000..0c9afa6db9 --- /dev/null +++ b/test/functional/wallet_elements_regression_1172.py @@ -0,0 +1,126 @@ +#!/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 test_send(self, amt, from_idx, to_idx, confidential): + # Try to send those coins to yet another wallet, sending a large enough amount + # that the change output is dropped. + address = self.nodes[to_idx].getnewaddress() + if not confidential: + address = self.nodes[to_idx].getaddressinfo(address)['unconfidential'] + txid = self.nodes[from_idx].sendtoaddress(address, amt) + self.log.info(f"Sent {amt} LBTC to node {to_idx} in {txid}") + self.nodes[from_idx].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[from_idx].getbalance(), { "bitcoin": Decimal(0) }) + assert_equal(self.nodes[to_idx].getbalance(), { "bitcoin": amt }) + + 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)) + self.test_send(amt, 1, 2, True) + + # Repeat, sending to a non-confidential output + amt = satoshi_round(Decimal(amt - Decimal(0.00035))) + self.test_send(amt, 2, 1, False) + + # Again, sending from non-confidential to non-confidential + amt = satoshi_round(Decimal(amt - Decimal(0.00033))) + self.test_send(amt, 1, 2, False) + + # Finally sending from non-confidential to confidential + amt = satoshi_round(Decimal(amt - Decimal(0.0005))) + self.test_send(amt, 2, 1, True) + + # Then send the coins again to make sure they're spendable + amt = satoshi_round(Decimal(amt - Decimal(0.0005))) + self.test_send(amt, 1, 2, True) + + addresses = [ self.nodes[1].getnewaddress() for i in range(15) ] \ + + [ self.nodes[2].getnewaddress() for i in range(15) ] + txid = self.nodes[2].sendmany(amounts={address: satoshi_round(Decimal(0.00025)) for address in addresses}) + self.log.info(f"Sent many small UTXOs to nodes 1 and 2 in {txid}") + self.nodes[2].generate(2) + self.sync_all() + + self.log.info(f"Issuing some assets from node 1") + # Try issuing assets + amt = satoshi_round(Decimal(1)) + res1 = self.nodes[1].issueasset(amt, amt, True); + res2 = self.nodes[1].issueasset(amt, amt, False); + + assets = [ res1["asset"], res1["token"], res2["asset"], res2["token"] ] + addresses = [ self.nodes[2].getnewaddress() for i in range(len(assets)) ] + txid = self.nodes[1].sendmany( + amounts={address: amt for address in addresses}, + output_assets={addresses[i]: assets[i] for i in range(len(assets))}, + ) + self.log.info(f"Sent them to node 2 in {txid}") + self.nodes[1].generate(2) + self.sync_all() + # Send them back + addresses = [ self.nodes[1].getnewaddress() for i in range(len(assets)) ] + txid = self.nodes[2].sendmany( + amounts={address: amt for address in addresses}, + output_assets={addresses[i]: assets[i] for i in range(len(assets))}, + ) + self.log.info(f"Sent them back to node 1 in {txid}") + +if __name__ == '__main__': + WalletCtTest().main() + diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index e2ad295b9b..1c38dd2a58 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -404,10 +404,10 @@ class ImportDescriptorsTest(BitcoinTestFramework): address, solvable=True, ismine=True) - txid = w0.sendtoaddress(address, 49.99993240) + txid = w0.sendtoaddress(address, 49.99965520) w0.generatetoaddress(6, w0.getnewaddress()) self.sync_blocks() - tx = wpriv.createrawtransaction([{"txid": txid, "vout": 0}], [{w0.getnewaddress(): 49.999}, {"fee": 0.0009324}]) + tx = wpriv.createrawtransaction([{"txid": txid, "vout": 0}], [{w0.getnewaddress(): 49.999}, {"fee": 0.00065520}]) signed_tx = wpriv.signrawtransactionwithwallet(tx) w1.sendrawtransaction(signed_tx['hex']) diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index 0aee82c51d..97ea2e80c6 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -174,11 +174,11 @@ class KeyPoolTest(BitcoinTestFramework): res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00025000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00010}) assert_equal("psbt" in res, True) # should work without subtractFeeFromOutputs if the exact fee is subtracted from the amount - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00021650}], options={"feeRate": 0.00010}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008570}], options={"feeRate": 0.00010}) assert_equal("psbt" in res, True) # dust change should be removed - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00021000}], options={"feeRate": 0.00010}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008200}], options={"feeRate": 0.00010}) assert_equal("psbt" in res, True) # create a transaction without change at the maximum fee rate, such that the output is still spendable: