fix(wallet): pay for 3 byte input and output counts so 253+ input sends work

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

src/wallet/spend.cpp

@@ -907,7 +907,9 @@ static util::Result<CreatedTransactionResult> 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<CreatedTransactionResult> 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<CreatedTransactionResult> 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<CreatedTransactionResult> 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) {

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()