diff --git a/doc/release-notes.md b/doc/release-notes.md --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -5,24 +5,3 @@ This release includes the following features and fixes: - -Command-line options --------------------- - -- The `-debug=db` logging category has been renamed to `-debug=walletdb`, to distinguish it from `coindb`. - `-debug=db` has been deprecated and will be removed in a next release. - -Low-level RPC Changes ---------------------- - -- The RPC gettransaction, listtransactions and listsinceblock responses now also -includes the height of the block that contains the wallet transaction, if any. - -Deprecated or removed RPCs --------------------------- - -- The `getaddressinfo` RPC `labels` field now returns an array of label name - strings. Previously, it returned an array of JSON objects containing `name` and - `purpose` key/value pairs, which is now deprecated and will be removed in a future - release. To re-enable the previous behavior, launch bitcoind with - `-deprecatedrpc=labelspurpose`. diff --git a/doc/release-notes/release-notes-.md b/doc/release-notes/release-notes-.md --- a/doc/release-notes/release-notes-.md +++ b/doc/release-notes/release-notes-.md @@ -6,7 +6,23 @@ This release includes the following features and fixes: -Wallet ------- +Command-line options +-------------------- -- The way that output trust was computed has been fixed, which impacts confirmed/unconfirmed balance status and coin selection. +- The `-debug=db` logging category has been renamed to `-debug=walletdb`, to distinguish it from `coindb`. + `-debug=db` has been deprecated and will be removed in a next release. + +Low-level RPC Changes +--------------------- + +- The RPC gettransaction, listtransactions and listsinceblock responses now also +includes the height of the block that contains the wallet transaction, if any. + +Deprecated or removed RPCs +-------------------------- + +- The `getaddressinfo` RPC `labels` field now returns an array of label name + strings. Previously, it returned an array of JSON objects containing `name` and + `purpose` key/value pairs, which is now deprecated and will be removed in a future + release. To re-enable the previous behavior, launch bitcoind with + `-deprecatedrpc=labelspurpose`. 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,8 +60,7 @@ } static void add_coin(CWallet &wallet, const Amount nValue, int nAge = 6 * 24, - bool fIsFromMe = false, int nInput = 0, - bool spendable = false) { + bool fIsFromMe = false, int nInput = 0) { balance += nValue; static int nextLockTime = 0; CMutableTransaction tx; @@ -69,12 +68,6 @@ 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 @@ -294,42 +287,19 @@ 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used)); - // 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 + // Make sure that we aren't using BnB when there are preset inputs empty_wallet(); - { - auto wallet = - std::make_unique(Params(), m_chain.get(), WalletLocation(), - WalletDatabase::CreateMock()); - bool firstRun; - wallet->LoadWallet(firstRun); - LOCK(wallet->cs_wallet); - 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); - } + 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); } 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 @@ -619,8 +619,6 @@ 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 @@ -2534,11 +2534,7 @@ coin.m_input_bytes < 0 ? Amount::zero() : long_term_feerate.GetFee(coin.m_input_bytes); - if (coin_selection_params.m_subtract_fee_outputs) { - group.effective_value += coin.txout.nValue; - } else { - group.effective_value += effective_value; - } + group.effective_value += effective_value; ++it; } else { it = group.Discard(coin); @@ -2574,14 +2570,13 @@ 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; @@ -2602,34 +2597,27 @@ 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()) { - 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 + 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; } + + // 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 @@ -2662,39 +2650,39 @@ "-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS); bool res = - 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) || + 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) || (m_spend_zero_conf_change && - SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), - groups, setCoinsRet, nValueRet, - coin_selection_params, bnb_used)) || + SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, + CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, + nValueRet, coin_selection_params, bnb_used)) || (m_spend_zero_conf_change && SelectCoinsMinConf( - value_to_select, - CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors / 3), - std::min((size_t)4, max_descendants / 3)), + nTargetValue - nValueFromPresetInputs, + CoinEligibilityFilter(0, 1, std::min(4, max_ancestors / 3), + std::min(4, max_descendants / 3)), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || (m_spend_zero_conf_change && - SelectCoinsMinConf(value_to_select, + SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors / 2, max_descendants / 2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || (m_spend_zero_conf_change && - SelectCoinsMinConf(value_to_select, + SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, max_ancestors - 1, max_descendants - 1), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) || (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf( - value_to_select, + nTargetValue - nValueFromPresetInputs, CoinEligibilityFilter(0, 1, std::numeric_limits::max()), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)); @@ -2975,11 +2963,8 @@ // BnB selector is the only selector used when this is true. // That should only happen on the first pass through the loop. - 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; + // If we are doing subtract fee from recipient, then don't use BnB + coin_selection_params.use_bnb = nSubtractFeeFromAmount == 0; // Start with no fee and loop until there is enough fee while (true) { nChangePosInOut = nChangePosRequest; @@ -2992,12 +2977,9 @@ nValueToSelect += nFeeRet; } - // 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; - } + // 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); @@ -3017,10 +2999,8 @@ // Include the fee cost for outputs. Note this is only used for // BnB right now - if (!coin_selection_params.m_subtract_fee_outputs) { - coin_selection_params.tx_noinputs_size += - ::GetSerializeSize(txout, PROTOCOL_VERSION); - } + coin_selection_params.tx_noinputs_size += + ::GetSerializeSize(txout, PROTOCOL_VERSION); if (IsDust(txout, chain().relayDustFee())) { if (recipient.fSubtractFeeFromAmount && @@ -3043,7 +3023,7 @@ } // Choose coins to use - bool bnb_used = false; + bool bnb_used; 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,7 +90,6 @@ 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 @@ -849,21 +848,6 @@ # 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()