diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index a72d5d807ea4..de7c15496000 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -403,10 +403,8 @@ struct SelectionResult * When there is a discrepancy, most of the time the final change would be slightly bigger than estimated. * * Following are the possible factors of discrepancy: - * + non-input fees always include segwit flags - * + input fee estimation always include segwit stack size * + input fees are rounded individually and not collectively, which leads to small rounding errors - * - input counter size is always assumed to be 1vbyte + * + input counter is sized for all coins available for selection, not only the selected ones * * @param[in] min_viable_change Minimum amount for change output, if change would be less then we forgo change * @param[in] change_fee Fees to include change output in the tx diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index da03e0c62245..85a88e2a6a61 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -907,7 +907,9 @@ static util::Result CreateTransactionInternal( CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty()); } CTxOut change_prototype_txout(0, scriptChange); - coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); + // Adding the change output may also grow the output count CompactSize + coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout) + + GetSizeOfCompactSize(vecSend.size() + 1) - GetSizeOfCompactSize(vecSend.size()); // Get size of spending the change output int change_spend_size = CalculateMaximumSignedInputSize(change_prototype_txout, &wallet, /*coin_control=*/nullptr); @@ -952,8 +954,8 @@ static util::Result CreateTransactionInternal( const auto change_spend_fee = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size); coin_selection_params.min_viable_change = std::max(change_spend_fee + 1, dust); - // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count - coin_selection_params.tx_noinputs_size = 9 + GetSizeOfCompactSize(vecSend.size()) + extra_payload_bytes; // bytes for output count + // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime (input count is added once the coins are known) + coin_selection_params.tx_noinputs_size = 8 + GetSizeOfCompactSize(vecSend.size()) + extra_payload_bytes; // bytes for output count // vouts to the payees for (const auto& recipient : vecSend) @@ -969,16 +971,6 @@ static util::Result CreateTransactionInternal( txNew.vout.push_back(txout); } - // 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.m_subtract_fee_outputs ? 0 : coin_selection_params.tx_noinputs_size); - CAmount selection_target = recipients_sum + not_input_fees; - - // This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN) - // and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways - if (selection_target == 0 && !coin_control.HasSelected()) { - return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")}; - } - // Fetch manually selected coins PreSelectedInputs preset_inputs; if (coin_control.HasSelected()) { @@ -994,6 +986,20 @@ static util::Result CreateTransactionInternal( available_coins = AvailableCoins(wallet, &coin_control, coin_selection_params.m_effective_feerate); } + // Coin selection can't know in advance how many inputs it will pick, so size the input count for every coin + // it could pick. Otherwise transactions with 253+ inputs underpay the fee for the 3 byte CompactSize. + coin_selection_params.tx_noinputs_size += GetSizeOfCompactSize(preset_inputs.coins.size() + available_coins.Size()); + + // 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.m_subtract_fee_outputs ? 0 : coin_selection_params.tx_noinputs_size); + CAmount selection_target = recipients_sum + not_input_fees; + + // This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN) + // and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways + if (selection_target == 0 && !coin_control.HasSelected()) { + return util::Error{_("Transaction requires one destination of non-0 value, a non-0 feerate, or a pre-selected input")}; + } + // Choose coins to use auto select_coins_res = SelectCoins(wallet, available_coins, preset_inputs, /*nTargetValue=*/selection_target, coin_control, coin_selection_params); if (!select_coins_res) { diff --git a/test/functional/wallet_create_tx.py b/test/functional/wallet_create_tx.py index c7c33b93d178..994ed3d011bf 100755 --- a/test/functional/wallet_create_tx.py +++ b/test/functional/wallet_create_tx.py @@ -3,9 +3,11 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. +from test_framework.messages import COIN from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, + assert_greater_than_or_equal, assert_raises_rpc_error, ) @@ -23,6 +25,7 @@ def skip_test_if_missing_module(self): def run_test(self): self.test_anti_fee_sniping() self.test_tx_size_too_large() + self.test_compact_size_counts() def test_anti_fee_sniping(self): self.log.info('Check that we have some (old) blocks and that anti-fee-sniping is disabled') @@ -72,6 +75,40 @@ def test_tx_size_too_large(self): ) self.nodes[0].settxfee(0) + def test_compact_size_counts(self): + self.log.info('Check that the input and output counts are fully paid for once they need a 3 byte CompactSize') + node = self.nodes[0] + node.createwallet(wallet_name='compact_size', descriptors=self.options.descriptors) + wallet = node.get_wallet_rpc('compact_size') + default_wallet = node.get_wallet_rpc(self.default_wallet_name) + + def check_fee(txid, num_inputs, num_outputs): + tx = wallet.gettransaction(txid=txid, verbose=True) + vin, size = tx['decoded']['vin'], tx['decoded']['size'] + assert_greater_than_or_equal(len(vin), num_inputs) + assert_equal(len(tx['decoded']['vout']), num_outputs) + # The default feerate is 1 duff/byte. The fee is estimated with maximum size signatures, so it + # may exceed the size by a couple of bytes for each signature that came out shorter. + fee = -tx['fee'] * COIN + assert_greater_than_or_equal(fee, size) + assert_greater_than_or_equal(size + 2 * len(vin), fee) + + # Spend at least 253 inputs and create change + num_utxos = 260 + default_wallet.sendmany(dummy="", amounts={wallet.getnewaddress(): 0.01 for _ in range(num_utxos)}) + self.generate(node, 1) + txid = wallet.sendtoaddress(address=default_wallet.getnewaddress(), amount=2.55) + check_fee(txid, num_inputs=253, num_outputs=2) + self.generate(node, 1) + + # Pay 252 recipients, so that adding change makes it 253 outputs + default_wallet.sendtoaddress(address=wallet.getnewaddress(), amount=1) + self.generate(node, 1) + txid = wallet.sendmany(dummy="", amounts={default_wallet.getnewaddress(): 0.001 for _ in range(252)}) + check_fee(txid, num_inputs=1, num_outputs=253) + self.generate(node, 1) + wallet.unloadwallet() + if __name__ == '__main__': CreateTxWalletTest().main()