From 25749f1df77c78eb146978ca0485cf9f91b5ce49 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 11 May 2022 11:39:54 -0300 Subject: [PATCH 1/4] =?UTF-8?q?wallet:=20unify=20=E2=80=9Callow/block=20ot?= =?UTF-8?q?her=20inputs=E2=80=9C=20concept?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seeking to make the `CoinControl` option less confusing/redundant. In #16377 the `CoinControl` flag ‘m_add_inputs’ was added to tell the coin filtering and selection process two things: - Coin Filtering: Only use the provided inputs. Skip the Rest. - Coin Selection: Search the wtxs-outputs and append all the `CoinControl` selected outpoints to the selection result (skipping all the available output checks). Nothing else. Meanwhile, in `CoinControl` we already have a flag ‘fAllowOtherInputs’ which is already saying: - Coin Filtering: Only use the provided inputs. Skip the Rest. - Coin Selection: If false, no selection process -> append all the `CoinControl` selected outpoints to the selection result (while they passed all the `AvailableCoins` checks and are available in the 'vCoins' vector). As can notice, the first point in the coin filtering process is duplicated in the two option flags. And the second one, is slightly different merely because it takes into account whether the coin is on the `AvailableCoins` vector or not. So it makes sense to merge ‘m_add_inputs’ and ‘fAllowOtherInputs’ into a single field for the coin filtering process while introduce other changes to add the missing/skipped coins into 'vCoins' vector if they were manually selected by the user (follow-up commits). --- src/wallet/coincontrol.h | 5 ++--- src/wallet/rpc/spend.cpp | 10 +++++----- src/wallet/spend.cpp | 7 ------- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index 65a5c83366f..e9687de14de 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -33,11 +33,10 @@ public: CTxDestination destChange = CNoDestination(); //! Override the default change type if set, ignored if destChange is set std::optional m_change_type; - //! If false, only selected inputs are used - bool m_add_inputs = true; //! If false, only safe inputs will be used bool m_include_unsafe_inputs = false; - //! If false, allows unselected inputs, but requires all selected inputs be used + //! If true, the selection process can add extra unselected inputs from the wallet + //! while requires all selected inputs be used bool fAllowOtherInputs = false; //! Includes watch only addresses which are solvable bool fAllowWatchOnly = false; diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index e2ea2b691db..0aaa3c2ac37 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -535,8 +535,8 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, }, true, true); - if (options.exists("add_inputs") ) { - coinControl.m_add_inputs = options["add_inputs"].get_bool(); + if (options.exists("add_inputs")) { + coinControl.fAllowOtherInputs = options["add_inputs"].get_bool(); } if (options.exists("changeAddress") || options.exists("change_address")) { @@ -823,7 +823,7 @@ RPCHelpMan fundrawtransaction() int change_position; CCoinControl coin_control; // Automatically select (additional) coins. Can be overridden by options.add_inputs. - coin_control.m_add_inputs = true; + coin_control.fAllowOtherInputs = true; FundTransaction(*pwallet, tx, fee, change_position, request.params[1], coin_control, /*override_min_fee=*/true); UniValue result(UniValue::VOBJ); @@ -1225,7 +1225,7 @@ RPCHelpMan send() CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. - coin_control.m_add_inputs = rawTx.vin.size() == 0; + coin_control.fAllowOtherInputs = rawTx.vin.size() == 0; SetOptionsInputWeights(options["inputs"], options); FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/false); @@ -1649,7 +1649,7 @@ RPCHelpMan walletcreatefundedpsbt() CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. - coin_control.m_add_inputs = rawTx.vin.size() == 0; + coin_control.fAllowOtherInputs = rawTx.vin.size() == 0; SetOptionsInputWeights(request.params[0], options); FundTransaction(wallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/true); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 4547dc41334..693158fafbd 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -168,11 +168,6 @@ CoinsResult AvailableCoins(const CWallet& wallet, const CTxOut& output = wtx.tx->vout[i]; const COutPoint outpoint(wtxid, i); - // Only consider selected coins if add_inputs is false - if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(outpoint)) { - continue; - } - if (output.nValue < nMinimumAmount || output.nValue > nMaximumAmount) continue; @@ -1033,8 +1028,6 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet, vecSend.push_back(recipient); } - coinControl.fAllowOtherInputs = true; - // Acquire the locks to prevent races to the new locked unspents between the // CreateTransaction call and LockCoin calls (when lockUnspents is true). LOCK(wallet.cs_wallet); From b4e2d4d4eea5f53aeb3124f966157222ea91106b Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 19 May 2022 09:48:49 -0300 Subject: [PATCH 2/4] wallet: move "use-only coinControl inputs" below the selected inputs lookup Otherwise, RPC commands such as `walletcreatefundedpsbt` will not support the manual selection of locked, spent and externally added coins. Full explanation is inside #25118 comments but brief summary is: `vCoins` at `SelectCoins` time could not be containing the manually selected input because, even when they were selected by the user, the current `AvailableCoins` flow skips locked and spent coins. Extra note: this is an intermediate step to unify the `fAllowOtherInputs`/`m_add_inputs` concepts. It will not be a problem anymore in the future when we finally decouple the wtx-outputs lookup process from `SelectCoins` and don't skip the user's manually selected coins in `AvailableCoins`. --- src/wallet/spend.cpp | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 693158fafbd..0d3107e0639 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -433,23 +433,6 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec OutputGroup preset_inputs(coin_selection_params); - // coin control -> return all selected outputs (we want all selected to go into the transaction for sure) - if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs) - { - for (const COutput& out : vCoins) { - if (!out.spendable) continue; - /* Set ancestors and descendants to 0 as these don't matter for preset inputs as no actual selection is being done. - * positive_only is set to false because we want to include all preset inputs, even if they are dust. - */ - preset_inputs.Insert(out, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); - } - SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); - result.AddInput(preset_inputs); - if (result.GetSelectedValue() < nTargetValue) return std::nullopt; - result.ComputeAndSetWaste(coin_selection_params.m_cost_of_change); - return result; - } - // calculate value from preset inputs and store them std::set preset_coins; @@ -497,6 +480,15 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec preset_inputs.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ 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) { + SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); + result.AddInput(preset_inputs); + if (result.GetSelectedValue() < nTargetValue) return std::nullopt; + result.ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + return result; + } + // remove preset inputs from vCoins so that Coin Selection doesn't pick them. for (std::vector::iterator it = vCoins.begin(); it != vCoins.end() && coin_control.HasSelected();) { From 8dea74a8ff8f5d3fdf6602c0eaa0fcc5877fc569 Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 19 May 2022 09:59:28 -0300 Subject: [PATCH 3/4] refactor: use GetWalletTx in SelectCoins instead of access mapWallet --- src/wallet/spend.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 0d3107e0639..6119a047d62 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -441,15 +441,14 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec for (const COutPoint& outpoint : vPresetInputs) { int input_bytes = -1; CTxOut txout; - std::map::const_iterator it = wallet.mapWallet.find(outpoint.hash); - if (it != wallet.mapWallet.end()) { - const CWalletTx& wtx = it->second; + auto ptr_wtx = wallet.GetWalletTx(outpoint.hash); + if (ptr_wtx) { // Clearly invalid input, fail - if (wtx.tx->vout.size() <= outpoint.n) { + if (ptr_wtx->tx->vout.size() <= outpoint.n) { return std::nullopt; } - input_bytes = GetTxSpendSize(wallet, wtx, outpoint.n, false); - txout = wtx.tx->vout.at(outpoint.n); + input_bytes = GetTxSpendSize(wallet, *ptr_wtx, outpoint.n, false); + txout = ptr_wtx->tx->vout.at(outpoint.n); } else { // The input is external. We did not find the tx in mapWallet. if (!coin_control.GetExternalOutput(outpoint, txout)) { From d33871288636d9e4cdde2e1430de1f4223e2dd3f Mon Sep 17 00:00:00 2001 From: furszy Date: Thu, 19 May 2022 14:37:19 -0300 Subject: [PATCH 4/4] scripted-diff: rename fAllowOtherInputs -> m_allow_other_inputs -BEGIN VERIFY SCRIPT- sed -i 's/fAllowOtherInputs/m_allow_other_inputs/g' -- $(git grep --files-with-matches 'fAllowOtherInputs') -END VERIFY SCRIPT- --- src/wallet/coincontrol.h | 2 +- src/wallet/feebumper.cpp | 2 +- src/wallet/rpc/spend.cpp | 8 ++++---- src/wallet/spend.cpp | 4 ++-- src/wallet/test/coinselector_tests.cpp | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index e9687de14de..d08d3664c43 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -37,7 +37,7 @@ public: bool m_include_unsafe_inputs = false; //! If true, the selection process can add extra unselected inputs from the wallet //! while requires all selected inputs be used - bool fAllowOtherInputs = false; + bool m_allow_other_inputs = false; //! Includes watch only addresses which are solvable bool fAllowWatchOnly = false; //! Override automatic min/max checks on fee, m_feerate must be set if true diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index afd2b839712..5e70ed4a308 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -213,7 +213,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo for (const auto& inputs : wtx.tx->vin) { new_coin_control.Select(COutPoint(inputs.prevout)); } - new_coin_control.fAllowOtherInputs = true; + new_coin_control.m_allow_other_inputs = true; // We cannot source new unconfirmed inputs(bip125 rule 2) new_coin_control.m_min_depth = 1; diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 0aaa3c2ac37..0e8cee5db88 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -536,7 +536,7 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out, true, true); if (options.exists("add_inputs")) { - coinControl.fAllowOtherInputs = options["add_inputs"].get_bool(); + coinControl.m_allow_other_inputs = options["add_inputs"].get_bool(); } if (options.exists("changeAddress") || options.exists("change_address")) { @@ -823,7 +823,7 @@ RPCHelpMan fundrawtransaction() int change_position; CCoinControl coin_control; // Automatically select (additional) coins. Can be overridden by options.add_inputs. - coin_control.fAllowOtherInputs = true; + coin_control.m_allow_other_inputs = true; FundTransaction(*pwallet, tx, fee, change_position, request.params[1], coin_control, /*override_min_fee=*/true); UniValue result(UniValue::VOBJ); @@ -1225,7 +1225,7 @@ RPCHelpMan send() CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. - coin_control.fAllowOtherInputs = rawTx.vin.size() == 0; + coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(options["inputs"], options); FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/false); @@ -1649,7 +1649,7 @@ RPCHelpMan walletcreatefundedpsbt() CCoinControl coin_control; // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. - coin_control.fAllowOtherInputs = rawTx.vin.size() == 0; + coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; SetOptionsInputWeights(request.params[0], options); FundTransaction(wallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/true); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 6119a047d62..5799a9ff2ab 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -171,7 +171,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, if (output.nValue < nMinimumAmount || output.nValue > nMaximumAmount) continue; - if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(outpoint)) + if (coinControl && coinControl->HasSelected() && !coinControl->m_allow_other_inputs && !coinControl->IsSelected(outpoint)) continue; if (wallet.IsLockedCoin(outpoint)) @@ -480,7 +480,7 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec } // coin control -> return all selected outputs (we want all selected to go into the transaction for sure) - if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs) { + if (coin_control.HasSelected() && !coin_control.m_allow_other_inputs) { SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); result.AddInput(preset_inputs); if (result.GetSelectedValue() < nTargetValue) return std::nullopt; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 76f28917a47..d6f47e99548 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -336,7 +336,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) add_coin(coins, *wallet, 3 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); add_coin(coins, *wallet, 2 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true); CCoinControl coin_control; - coin_control.fAllowOtherInputs = true; + coin_control.m_allow_other_inputs = true; coin_control.Select(coins.at(0).outpoint); coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); const auto result10 = SelectCoins(*wallet, coins, 10 * CENT, coin_control, coin_selection_params_bnb); @@ -392,7 +392,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) expected_result.Clear(); add_coin(9 * CENT, 2, expected_result); add_coin(1 * CENT, 2, expected_result); - coin_control.fAllowOtherInputs = true; + coin_control.m_allow_other_inputs = true; coin_control.Select(coins.at(1).outpoint); // pre select 9 coin const auto result13 = SelectCoins(*wallet, coins, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result13));