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.

5 files changed, 19 insertions, 17 deletions

diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
index e6ba89627c..cebe2cd7ed 100644
--- a/src/wallet/coinselection.cpp
+++ b/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();
 
diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h
index 9ff2011ce3..0e5e86f652 100644
--- a/src/wallet/coinselection.h
+++ b/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;
 };
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 865f6e41a6..25af6f49bd 100644
--- a/src/wallet/spend.cpp
+++ b/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
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index 1c731b95e5..1c2a561bef 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/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;
 }
diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp
index 1a0708c43a..95060c748f 100644
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/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()) {