diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -60,7 +60,8 @@ } static void add_coin(CWallet &wallet, const Amount nValue, int nAge = 6 * 24, - bool fIsFromMe = false, int nInput = 0) { + bool fIsFromMe = false, int nInput = 0, + bool spendable = false) { balance += nValue; static int nextLockTime = 0; CMutableTransaction tx; @@ -68,6 +69,12 @@ tx.nLockTime = nextLockTime++; tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; + if (spendable) { + CTxDestination dest; + std::string error; + assert(wallet.GetNewDestination(OutputType::LEGACY, "", dest, error)); + tx.vout[nInput].scriptPubKey = GetScriptForDestination(dest); + } if (fIsFromMe) { // IsFromMe() returns (GetDebit() > 0), and GetDebit() is 0 if // vin.empty(), so stop vin being empty, and cache a non-zero Debit to @@ -289,19 +296,43 @@ 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); - // Make sure that we aren't using BnB when there are preset inputs + // Test fees subtracted from output: + empty_wallet(); + add_coin(m_wallet, 1 * CENT); + vCoins.at(0).nInputBytes = 40; + BOOST_CHECK(!m_wallet.SelectCoinsMinConf( + 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, + coin_selection_params_bnb, bnb_used)); + coin_selection_params_bnb.m_subtract_fee_outputs = true; + BOOST_CHECK(m_wallet.SelectCoinsMinConf( + 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, + coin_selection_params_bnb, bnb_used)); + BOOST_CHECK_EQUAL(nValueRet, 1 * CENT); + + // Make sure that can use BnB when there are preset inputs empty_wallet(); - add_coin(m_wallet, 5 * CENT); - add_coin(m_wallet, 3 * CENT); - add_coin(m_wallet, 2 * CENT); - CCoinControl coin_control; - coin_control.fAllowOtherInputs = true; - coin_control.Select(COutPoint(vCoins.at(0).tx->GetId(), vCoins.at(0).i)); - BOOST_CHECK(m_wallet.SelectCoins(vCoins, 10 * CENT, setCoinsRet, nValueRet, - coin_control, coin_selection_params_bnb, - bnb_used)); - BOOST_CHECK(!bnb_used); - BOOST_CHECK(!coin_selection_params_bnb.use_bnb); + { + auto wallet = + std::make_unique(Params(), m_chain.get(), WalletLocation(), + WalletDatabase::CreateMock()); + bool firstRun; + wallet->LoadWallet(firstRun); + LOCK(wallet->cs_wallet); + wallet->SetupLegacyScriptPubKeyMan(); + add_coin(*wallet, 5 * CENT, 6 * 24, false, 0, true); + add_coin(*wallet, 3 * CENT, 6 * 24, false, 0, true); + add_coin(*wallet, 2 * CENT, 6 * 24, false, 0, true); + CCoinControl coin_control; + coin_control.fAllowOtherInputs = true; + coin_control.Select( + COutPoint(vCoins.at(0).tx->GetId(), vCoins.at(0).i)); + coin_selection_params_bnb.effective_fee = CFeeRate(Amount::zero()); + BOOST_CHECK(wallet->SelectCoins(vCoins, 10 * CENT, setCoinsRet, + nValueRet, coin_control, + coin_selection_params_bnb, bnb_used)); + BOOST_CHECK(bnb_used); + BOOST_CHECK(coin_selection_params_bnb.use_bnb); + } } BOOST_AUTO_TEST_CASE(knapsack_solver_test) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -650,6 +650,8 @@ size_t change_spend_size = 0; CFeeRate effective_fee = CFeeRate(Amount::zero()); size_t tx_noinputs_size = 0; + //! Indicate that we are subtracting the fee from outputs + bool m_subtract_fee_outputs = false; CoinSelectionParams(bool use_bnb_, size_t change_output_size_, size_t change_spend_size_, CFeeRate effective_fee_, diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2532,7 +2532,11 @@ coin.m_input_bytes < 0 ? Amount::zero() : long_term_feerate.GetFee(coin.m_input_bytes); - group.effective_value += effective_value; + if (coin_selection_params.m_subtract_fee_outputs) { + group.effective_value += coin.txout.nValue; + } else { + group.effective_value += effective_value; + } ++it; } else { it = group.Discard(coin); @@ -2568,13 +2572,14 @@ CoinSelectionParams &coin_selection_params, bool &bnb_used) const { std::vector 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; @@ -2595,27 +2600,34 @@ coin_control.ListSelected(vPresetInputs); for (const COutPoint &outpoint : vPresetInputs) { - // For now, don't use BnB if preset inputs are selected. TODO: Enable - // this later - bnb_used = false; - coin_selection_params.use_bnb = false; - std::map::const_iterator it = mapWallet.find(outpoint.GetTxId()); - if (it == mapWallet.end()) { - // TODO: Allow non-wallet inputs - return false; - } - - const CWalletTx &wtx = it->second; - // Clearly invalid input, fail. - if (wtx.tx->vout.size() <= outpoint.GetN()) { - return false; + if (it != mapWallet.end()) { + const CWalletTx &wtx = it->second; + // Clearly invalid input, fail + if (wtx.tx->vout.size() <= outpoint.GetN()) { + 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) { + // Not solvable, can't estimate size for fee + return false; + } + coin.effective_value = + coin.txout.nValue - + coin_selection_params.effective_fee.GetFee(coin.m_input_bytes); + if (coin_selection_params.use_bnb) { + value_to_select -= coin.effective_value; + } else { + value_to_select -= coin.txout.nValue; + } + setPresetCoins.insert(coin); + } else { + return false; // TODO: Allow non-wallet inputs } - - // Just to calculate the marginal byte size - nValueFromPresetInputs += wtx.tx->vout[outpoint.GetN()].nValue; - setPresetCoins.insert(CInputCoin(wtx.tx, outpoint.GetN())); } // Remove preset inputs from vCoins @@ -2649,39 +2661,39 @@ vCoins, !coin_control.m_avoid_partial_spends, max_ancestors); bool res = - nTargetValue <= nValueFromPresetInputs || - SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, - CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, - nValueRet, coin_selection_params, bnb_used) || - SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, - CoinEligibilityFilter(1, 1, 0), groups, setCoinsRet, - nValueRet, coin_selection_params, bnb_used) || + value_to_select <= Amount::zero() || + SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), + groups, setCoinsRet, nValueRet, + coin_selection_params, bnb_used) || + SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), + groups, setCoinsRet, nValueRet, + coin_selection_params, bnb_used) || (m_spend_zero_conf_change && - SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, - CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, - nValueRet, coin_selection_params, bnb_used)) || + SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), + groups, setCoinsRet, nValueRet, + coin_selection_params, bnb_used)) || (m_spend_zero_conf_change && SelectCoinsMinConf( - nTargetValue - nValueFromPresetInputs, - CoinEligibilityFilter(0, 1, std::min(4, max_ancestors / 3), - std::min(4, max_descendants / 3)), + value_to_select, + CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors / 3), + std::min((size_t)4, max_descendants / 3)), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || (m_spend_zero_conf_change && - SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, + SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors / 2, max_descendants / 2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || (m_spend_zero_conf_change && - SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, + SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors - 1, max_descendants - 1), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf( - nTargetValue - nValueFromPresetInputs, + value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits::max()), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); @@ -2960,8 +2972,11 @@ // BnB selector is the only selector used when this is true. // That should only happen on the first pass through the loop. - // If we are doing subtract fee from recipient, then don't use BnB - coin_selection_params.use_bnb = nSubtractFeeFromAmount == 0; + coin_selection_params.use_bnb = true; + // If we are doing subtract fee from recipient, don't use effective + // values + coin_selection_params.m_subtract_fee_outputs = + nSubtractFeeFromAmount != 0; // Start with no fee and loop until there is enough fee while (true) { nChangePosInOut = nChangePosRequest; @@ -2974,9 +2989,12 @@ nValueToSelect += nFeeRet; } - // Static size overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 - // input count, 1 output count - coin_selection_params.tx_noinputs_size = 10; + // vouts to the payees + if (!coin_selection_params.m_subtract_fee_outputs) { + // Static size overhead + outputs vsize. 4 nVersion, 4 + // nLocktime, 1 input count, 1 output count + coin_selection_params.tx_noinputs_size = 10; + } // vouts to the payees for (const auto &recipient : vecSend) { CTxOut txout(recipient.nAmount, recipient.scriptPubKey); @@ -2996,8 +3014,10 @@ // Include the fee cost for outputs. Note this is only used for // BnB right now - coin_selection_params.tx_noinputs_size += - ::GetSerializeSize(txout, PROTOCOL_VERSION); + if (!coin_selection_params.m_subtract_fee_outputs) { + coin_selection_params.tx_noinputs_size += + ::GetSerializeSize(txout, PROTOCOL_VERSION); + } if (IsDust(txout, chain().relayDustFee())) { if (recipient.fSubtractFeeFromAmount && @@ -3020,7 +3040,7 @@ } // 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 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -90,6 +90,7 @@ 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 @@ # 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()