wallet: make OutputGroup "positive_only" filter explicit

And not hide it inside the `OutputGroup::Insert` method.
This method does not return anything if insertion fails.

We can know before calling `Insert` whether the coin
will be accepted or not.

Author: furszy

src/bench/coin_selection.cpp

@@ -91,7 +91,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup>
     tx.vout[nInput].nValue = nValue;
     COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 0, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ true, /*fees=*/ 0);
     set.emplace_back();
-    set.back().Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false);
+    set.back().Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0);
 }
 // Copied from src/wallet/test/coinselector_tests.cpp
 static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool)

src/wallet/coinselection.cpp

@@ -333,10 +333,7 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
 
  ******************************************************************************/
 
-void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only) {
-    // Filter for positive only here before adding the coin
-    if (positive_only && output.GetEffectiveValue() <= 0) return;
-
+void OutputGroup::Insert(const COutput& output, size_t ancestors, size_t descendants) {
     m_outputs.push_back(output);
     COutput& coin = m_outputs.back();
 

src/wallet/coinselection.h

@@ -237,7 +237,7 @@ struct OutputGroup
         m_subtract_fee_outputs(params.m_subtract_fee_outputs)
     {}
 
-    void Insert(const COutput& output, size_t ancestors, size_t descendants, bool positive_only);
+    void Insert(const COutput& output, size_t ancestors, size_t descendants);
     bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
     CAmount GetSelectionAmount() const;
 };

src/wallet/spend.cpp

@@ -417,13 +417,14 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C
             size_t ancestors, descendants;
             wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants);
 
-            // Make an OutputGroup containing just this output
-            OutputGroup group{coin_sel_params};
-            group.Insert(output, ancestors, descendants, positive_only);
+            // If 'positive_only' is set, filter for positive only before adding the coin
+            if (!positive_only || output.GetEffectiveValue() > 0) {
+                // Make an OutputGroup containing just this output
+                OutputGroup group{coin_sel_params};
+                group.Insert(output, ancestors, descendants);
 
-            // Check the OutputGroup's eligibility. Only add the eligible ones.
-            if (positive_only && group.GetSelectionAmount() <= 0) continue;
-            if (group.m_outputs.size() > 0 && group.EligibleForSpending(filter)) groups_out.push_back(group);
+                if (group.EligibleForSpending(filter)) groups_out.push_back(group);
+            }
         }
         return groups_out;
     }
@@ -462,8 +463,10 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C
             group = &groups.back();
         }
 
-        // Add the output to group
-        group->Insert(output, ancestors, descendants, positive_only);
+        // Filter for positive only before adding the output to group
+        if (!positive_only || output.GetEffectiveValue() > 0) {
+            group->Insert(output, ancestors, descendants);
+        }
     }
 
     // Now we go through the entire map and pull out the OutputGroups

src/wallet/test/coinselector_tests.cpp

@@ -53,7 +53,7 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result)
     tx.nLockTime = nextLockTime++;        // so all transactions get different hashes
     COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, /*fees=*/ 0);
     OutputGroup group;
-    group.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ true);
+    group.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0);
     result.AddInput(group);
 }
 
@@ -134,7 +134,7 @@ inline std::vector<OutputGroup>& GroupCoins(const std::vector<COutput>& availabl
     static_groups.clear();
     for (auto& coin : available_coins) {
         static_groups.emplace_back();
-        static_groups.back().Insert(coin, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false);
+        static_groups.back().Insert(coin, /*ancestors=*/ 0, /*descendants=*/ 0);
     }
     return static_groups;
 }

src/wallet/test/fuzz/coinselection.cpp

@@ -29,8 +29,10 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect
     auto output_group = OutputGroup(coin_params);
     bool valid_outputgroup{false};
     for (auto& coin : coins) {
-        output_group.Insert(coin, /*ancestors=*/0, /*descendants=*/0, positive_only);
-        // If positive_only was specified, nothing may have been inserted, leading to an empty output group
+        if (!positive_only || (positive_only && coin.GetEffectiveValue() > 0)) {
+            output_group.Insert(coin, /*ancestors=*/0, /*descendants=*/0);
+        }
+        // If positive_only was specified, nothing was inserted, leading to an empty output group
         // that would be invalid for the BnB algorithm
         valid_outputgroup = !positive_only || output_group.GetSelectionAmount() > 0;
         if (valid_outputgroup && fuzzed_data_provider.ConsumeBool()) {