Merge bitcoin#17568: wallet: fix when sufficient preset inputs and subtractFeeFromOutputs

eadd130 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow)
ff330ba Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow)

Pull request description:

  bitcoin#17290 introduced a bug where, when we had preset inputs that covered the amount being sent and subtractFeeFrromOutputs was being used, transaction funding would result in a `Fee exceeds maximum configured by -maxtxfee` error. This was happening because we weren't setting `bnb_used = false` when the preset inputs were used as it should have been. This resulted in a too high fee because the change would go to fees accidentally.

  Apparently this particular case doesn't have a test, so I've added one as well.

ACKs for top commit:
  Sjors:
    ACK eadd130. I can't get this new test to fail on macOS (without this PR). It passes whether or not I compile with `--enable-debug`. It does fail on Ubuntu. Yay undefined behavior... Anyway, it's a useful test.
  fanquake:
    ACK eadd130
  instagibbs:
    utACK bitcoin@eadd130

Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e

Author: fanquake

src/wallet/wallet.cpp

@@ -2270,12 +2270,12 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
     std::vector<COutput> vCoins(vAvailableCoins);
     CAmount value_to_select = nTargetValue;
 
+    // Default to bnb was not used. If we use it, we set it later
+    bnb_used = false;
+
     // coin control -> return all selected outputs (we want all selected to go into the transaction for sure)
     if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs)
     {
-        // We didn't use BnB here, so set it to false.
-        bnb_used = false;
-
         for (const COutput& out : vCoins)
         {
             if (!out.fSpendable)
@@ -2300,14 +2300,12 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
             const CWalletTx& wtx = it->second;
             // Clearly invalid input, fail
             if (wtx.tx->vout.size() <= outpoint.n) {
-                bnb_used = false;
                 return false;
             }
             // Just to calculate the marginal byte size
             CInputCoin coin(wtx.tx, outpoint.n, wtx.GetSpendSize(outpoint.n, false));
             nValueFromPresetInputs += coin.txout.nValue;
             if (coin.m_input_bytes <= 0) {
-                bnb_used = false;
                 return false; // Not solvable, can't estimate size for fee
             }
             coin.effective_value = coin.txout.nValue - coin_selection_params.effective_fee.GetFee(coin.m_input_bytes);
@@ -2318,7 +2316,6 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
             }
             setPresetCoins.insert(coin);
         } else {
-            bnb_used = false;
             return false; // TODO: Allow non-wallet inputs
         }
     }
@@ -2678,7 +2675,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
                 }
 
                 // Choose coins to use
-                bool bnb_used;
+                bool bnb_used = false;
                 if (pick_new_inputs) {
                     nValueIn = 0;
                     setCoins.clear();

test/functional/rpc_fundrawtransaction.py

@@ -92,6 +92,7 @@ def run_test(self):
         self.test_option_feerate()
         self.test_address_reuse()
         self.test_option_subtract_fee_from_outputs()
+        self.test_subtract_fee_with_presets()
 
     def test_change_position(self):
         """Ensure setting changePosition in fundraw with an exact match is handled properly."""
@@ -741,5 +742,17 @@ def test_option_subtract_fee_from_outputs(self):
         # The total subtracted from the outputs is equal to the fee.
         assert_equal(share[0] + share[2] + share[3], result[0]['fee'])
 
+    def test_subtract_fee_with_presets(self):
+        self.log.info("Test fundrawtxn subtract fee from outputs with preset inputs that are sufficient")
+
+        addr = self.nodes[0].getnewaddress()
+        txid = self.nodes[0].sendtoaddress(addr, 10)
+        vout = find_vout_for_address(self.nodes[0], txid, addr)
+
+        rawtx = self.nodes[0].createrawtransaction([{'txid': txid, 'vout': vout}], [{self.nodes[0].getnewaddress(): 5}])
+        fundedtx = self.nodes[0].fundrawtransaction(rawtx, {'subtractFeeFromOutputs': [0]})
+        signedtx = self.nodes[0].signrawtransactionwithwallet(fundedtx['hex'])
+        self.nodes[0].sendrawtransaction(signedtx['hex'])
+
 if __name__ == '__main__':
     RawTransactionsTest().main()