Merge bitcoin/bitcoin#27227: wallet: 25806 follow-up

475c20a wallet: remove coin control arg from AutomaticCoinSelection (furszy)
8a55831 wallet: remove unused methods (furszy)
8471967 wallet: GroupOutput, remove unneeded "spendable" check (furszy)
a9aa041 wallet: OutputGroup, remove unused effective_feerate member (furszy)
99034b2 wallet: APS, don't create empty groups (furszy)
805f399 wallet: do not make two COutputs, use shared_ptr (furszy)

Pull request description:

  Few small findings post-#25806 and extra cleanups, nothing biggie.

ACKs for top commit:
  S3RK:
    Code review ACK 475c20a
  Xekyo:
    utACK 475c20a
  achow101:
    ACK 475c20a

Tree-SHA512: df45efebd6e2e4ecac619d6ecef794979c328a2d6ef532e25124d0dc1c72b55ccf13498f61fb65958b907bfba6a72ed569bf34eb5fbe35419632fe0406e78798

Author: achow101

src/wallet/coinselection.cpp

@@ -387,13 +387,6 @@ void OutputGroupTypeMap::Push(const OutputGroup& group, OutputType type, bool in
     }
 }
 
-std::optional<Groups> OutputGroupTypeMap::Find(OutputType type)
-{
-    auto it_by_type = groups_by_type.find(type);
-    if (it_by_type == groups_by_type.end()) return std::nullopt;
-    return it_by_type->second;
-}
-
 CAmount GetSelectionWaste(const std::set<std::shared_ptr<COutput>>& inputs, CAmount change_cost, CAmount target, bool use_effective_value)
 {
     // This function should not be called with empty inputs as that would mean the selection failed

src/wallet/coinselection.h

@@ -153,6 +153,11 @@ struct CoinSelectionParams {
      * associated with the same address. This helps reduce privacy leaks resulting from address
      * reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */
     bool m_avoid_partial_spends = false;
+    /**
+     * When true, allow unsafe coins to be selected during Coin Selection. This may spend unconfirmed outputs:
+     * 1) Received from other wallets, 2) replacing other txs, 3) that have been replaced.
+     */
+    bool m_include_unsafe_inputs = false;
 
     CoinSelectionParams(FastRandomContext& rng_fast, size_t change_output_size, size_t change_spend_size,
                         CAmount min_change_target, CFeeRate effective_feerate,
@@ -222,8 +227,6 @@ struct OutputGroup
     CAmount effective_value{0};
     /** The fee to spend these UTXOs at the effective feerate. */
     CAmount fee{0};
-    /** The target feerate of the transaction we're trying to build. */
-    CFeeRate m_effective_feerate{0};
     /** The fee to spend these UTXOs at the long term feerate. */
     CAmount long_term_fee{0};
     /** The feerate for spending a created change output eventually (i.e. not urgently, and thus at
@@ -238,7 +241,6 @@ struct OutputGroup
 
     OutputGroup() {}
     OutputGroup(const CoinSelectionParams& params) :
-        m_effective_feerate(params.m_effective_feerate),
         m_long_term_feerate(params.m_long_term_feerate),
         m_subtract_fee_outputs(params.m_subtract_fee_outputs)
     {}
@@ -266,8 +268,6 @@ struct OutputGroupTypeMap
     // Based on the insert flag; appends group to the 'mixed_group' and, if value > 0, to the 'positive_group'.
     // This affects both; the groups filtered by type and the overall groups container.
     void Push(const OutputGroup& group, OutputType type, bool insert_positive, bool insert_mixed);
-    // Retrieves 'Groups' filtered by type
-    std::optional<Groups> Find(OutputType type);
     // Different output types count
     size_t TypesCount() { return groups_by_type.size(); }
 };

src/wallet/spend.cpp

@@ -415,9 +415,6 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
         // Allowing partial spends means no grouping. Each COutput gets its own OutputGroup
         for (const auto& [type, outputs] : coins.coins) {
             for (const COutput& output : outputs) {
-                // Skip outputs we cannot spend
-                if (!output.spendable) continue;
-
                 // Get mempool info
                 size_t ancestors, descendants;
                 wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants);
@@ -444,11 +441,10 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
     // to the last OutputGroup in the vector for the scriptPubKey. When the last OutputGroup has
     // OUTPUT_GROUP_MAX_ENTRIES COutputs, a new OutputGroup is added to the end of the vector.
     typedef std::map<std::pair<CScript, OutputType>, std::vector<OutputGroup>> ScriptPubKeyToOutgroup;
-    const auto& group_outputs = [](
-            const COutput& output, OutputType type, size_t ancestors, size_t descendants,
-            ScriptPubKeyToOutgroup& groups_map, const CoinSelectionParams& coin_sel_params,
-            bool positive_only) {
-        std::vector<OutputGroup>& groups = groups_map[std::make_pair(output.txout.scriptPubKey,type)];
+    const auto& insert_output = [&](
+            const std::shared_ptr<COutput>& output, OutputType type, size_t ancestors, size_t descendants,
+            ScriptPubKeyToOutgroup& groups_map) {
+        std::vector<OutputGroup>& groups = groups_map[std::make_pair(output->txout.scriptPubKey,type)];
 
         if (groups.size() == 0) {
             // No OutputGroups for this scriptPubKey yet, add one
@@ -467,25 +463,24 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
             group = &groups.back();
         }
 
-        // Filter for positive only before adding the output to group
-        if (!positive_only || output.GetEffectiveValue() > 0) {
-            group->Insert(std::make_shared<COutput>(output), ancestors, descendants);
-        }
+        group->Insert(output, ancestors, descendants);
     };
 
     ScriptPubKeyToOutgroup spk_to_groups_map;
     ScriptPubKeyToOutgroup spk_to_positive_groups_map;
     for (const auto& [type, outs] : coins.coins) {
         for (const COutput& output : outs) {
-            // Skip outputs we cannot spend
-            if (!output.spendable) continue;
-
             size_t ancestors, descendants;
             wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants);
 
-            group_outputs(output, type, ancestors, descendants, spk_to_groups_map, coin_sel_params, /*positive_only=*/ false);
-            group_outputs(output, type, ancestors, descendants, spk_to_positive_groups_map,
-                          coin_sel_params, /*positive_only=*/ true);
+            const auto& shared_output = std::make_shared<COutput>(output);
+            // Filter for positive only before adding the output
+            if (output.GetEffectiveValue() > 0) {
+                insert_output(shared_output, type, ancestors, descendants, spk_to_positive_groups_map);
+            }
+
+            // 'All' groups
+            insert_output(shared_output, type, ancestors, descendants, spk_to_groups_map);
         }
     }
 
@@ -622,7 +617,7 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av
     }
 
     // Start wallet Coin Selection procedure
-    auto op_selection_result = AutomaticCoinSelection(wallet, available_coins, selection_target, coin_control, coin_selection_params);
+    auto op_selection_result = AutomaticCoinSelection(wallet, available_coins, selection_target, coin_selection_params);
     if (!op_selection_result) return op_selection_result;
 
     // If needed, add preset inputs to the automatic coin selection result
@@ -637,7 +632,7 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av
     return op_selection_result;
 }
 
-util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params)
+util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CoinSelectionParams& coin_selection_params)
 {
     unsigned int limit_ancestor_count = 0;
     unsigned int limit_descendant_count = 0;
@@ -646,12 +641,10 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
     const size_t max_descendants = (size_t)std::max<int64_t>(1, limit_descendant_count);
     const bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS);
 
-    // form groups from remaining coins; note that preset coins will not
-    // automatically have their associated (same address) coins included
-    if (coin_control.m_avoid_partial_spends && available_coins.Size() > OUTPUT_GROUP_MAX_ENTRIES) {
-        // Cases where we have 101+ outputs all pointing to the same destination may result in
-        // privacy leaks as they will potentially be deterministically sorted. We solve that by
-        // explicitly shuffling the outputs before processing
+    // Cases where we have 101+ outputs all pointing to the same destination may result in
+    // privacy leaks as they will potentially be deterministically sorted. We solve that by
+    // explicitly shuffling the outputs before processing
+    if (coin_selection_params.m_avoid_partial_spends && available_coins.Size() > OUTPUT_GROUP_MAX_ENTRIES) {
         available_coins.Shuffle(coin_selection_params.rng_fast);
     }
 
@@ -678,7 +671,7 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
             ordered_filters.push_back({CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, /*include_partial=*/true)});
             // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs
             // received from other wallets.
-            if (coin_control.m_include_unsafe_inputs) {
+            if (coin_selection_params.m_include_unsafe_inputs) {
                 ordered_filters.push_back({CoinEligibilityFilter(/*conf_mine=*/0, /*conf_theirs*/0, max_ancestors-1, max_descendants-1, /*include_partial=*/true)});
             }
             // Try with unlimited ancestors/descendants. The transaction will still need to meet
@@ -809,6 +802,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
 
     CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy
     coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
+    coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs;
 
     // Set the long term feerate estimate to the wallet's consolidate feerate
     coin_selection_params.m_long_term_feerate = wallet.m_consolidate_feerate;

src/wallet/spend.h

@@ -191,7 +191,7 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
  *                                                or (2) an specific error message if there was something particularly wrong (e.g. a selection
  *                                                result that surpassed the tx max weight size).
  */
-util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control,
+util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue,
                  const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 
 /**

src/wallet/wallet.h

@@ -592,12 +592,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, bool relay) const
         EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
-    bool DummySignTx(CMutableTransaction &txNew, const std::set<CTxOut> &txouts, const CCoinControl* coin_control = nullptr) const
-    {
-        std::vector<CTxOut> v_txouts(txouts.size());
-        std::copy(txouts.begin(), txouts.end(), v_txouts.begin());
-        return DummySignTx(txNew, v_txouts, coin_control);
-    }
     bool DummySignTx(CMutableTransaction &txNew, const std::vector<CTxOut> &txouts, const CCoinControl* coin_control = nullptr) const;
 
     bool ImportScripts(const std::set<CScript> scripts, int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);