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

fanquake · jasonbcox · commit 78322dfdfa3c · 2020-09-30T15:35:28.000-07:00
Summary: eadd1304c81e0b89178e4cc7630bd31650850c85 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow) ff330badd45067cb520b1cfa1844f60a4c9f2031 Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow) Pull request description: #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. Backport of Core [[bitcoin/bitcoin#17568 | PR17568]] Depends on D7688 Test Plan: ``` ninja check test_runner.py rpc_fundrawtransaction ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D7691
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -2535,12 +2535,12 @@ bool CWallet::SelectCoins(const std::vector<COutput> &vAvailableCoins,
     std::vector<COutput> vCoins(vAvailableCoins);
     Amount 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) {
                 continue;
@@ -2567,15 +2567,13 @@ bool CWallet::SelectCoins(const std::vector<COutput> &vAvailableCoins,
             const CWalletTx &wtx = it->second;
             // Clearly invalid input, fail
             if (wtx.tx->vout.size() <= outpoint.GetN()) {
-                bnb_used = false;
                 return false;
             }
             // Just to calculate the marginal byte size
             CInputCoin coin(wtx.tx, outpoint.GetN(),
                             wtx.GetSpendSize(outpoint.GetN(), false));
             nValueFromPresetInputs += coin.txout.nValue;
             if (coin.m_input_bytes <= 0) {
-                bnb_used = false;
                 // Not solvable, can't estimate size for fee
                 return false;
             }
@@ -2589,7 +2587,6 @@ bool CWallet::SelectCoins(const std::vector<COutput> &vAvailableCoins,
             }
             setPresetCoins.insert(coin);
         } else {
-            bnb_used = false;
             return false; // TODO: Allow non-wallet inputs
         }
     }
@@ -3018,7 +3015,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock &locked_chainIn,
             }
 
             // Choose coins to use
-            bool bnb_used;
+            bool bnb_used = false;
             if (pick_new_inputs) {
                 nValueIn = Amount::zero();
                 setCoins.clear();
diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
@@ -90,6 +90,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 that setting changePosition in fundraw with an exact match is
@@ -848,6 +849,21 @@ 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()
