Merge #6517: backport: merge bitcoin#25497, bitcoin#17331, bitcoin#22155

 (wallet backports: part 1)

153bdc2 merge bitcoin#22155: Add test for subtract fee from recipient behavior (Kittywhiskers Van Gogh)
0185847 fix: correct fee calculations in `CreateTransactionInternal` (Kittywhiskers Van Gogh)
445f489 merge bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh)
7e54bd9 wallet: copy and sort `vecSend` if BIP69 sorting enabled, enable sorting (Kittywhiskers Van Gogh)
9e9e66f partial bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh)
66fe2d4 merge bitcoin#25497: more accurate target for large transactions (Kittywhiskers Van Gogh)
6e4d789 wallet: add back missing `CoinSelectionParams` assignments (Kittywhiskers Van Gogh)
bd35042 wallet: move `CoinSelectionParams` to positions expected upstream (Kittywhiskers Van Gogh)
0711e67 wallet: shuffle transaction inputs if we aren't using BIP69 (Kittywhiskers Van Gogh)
1cf1c58 refactor: move selected coin and txin sorting to the end of the scope (Kittywhiskers Van Gogh)
ab756ba wallet: Fail if maximum weight is too large (Kittywhiskers Van Gogh)
05c319e refactor: move oversized transaction check to tail end of scope (Kittywhiskers Van Gogh)
6ca51df wallet: Use existing feerate instead of getting a new one (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #6543

  * Dependency for #6529

  * [bitcoin#17331](bitcoin#17331) logically partially reverts [dash#3368](#3668) as Dash Core implemented a calculate-before approach (compared to _then_ Bitcoin Core's calculate-and-adjust approach) and it is being replaced  with the current upstream calculate-after approach done in a single-pass instead of iteratively (like the former two).
    * As the changes are non-trivial, they have been split into a "partial" and a "merge" commit, the first half dedicated just to the logical partial revert and the latter half dedicated to using effective values in coin selection.
      * BIP69 sorting is disabled in the former half to allow the fix to be in a separate commit while allowing validation of the remaining set of changes. The fix re-enables BIP69 sorting.
    * Due to the changes introduced in [dash#3368](#3668), a lot of then-redundant code was removed and changes to it upstream were not mirrored in Dash Core. To allow [bitcoin#17331](bitcoin#17331) to work properly, a lot of that unmirrored code was reintroduced and existing code readjusted to match upstream.

  * `coin_selection_params.tx_noinputs_size` is said to have a size (sans output count) of `9` instead of `10` as we don't have a SegWit field (referred to as `1 witness overhead (dummy, flag, stack size)` in a code comment) on account of not having SegWit.

  * To allow for backporting [bitcoin#17331](bitcoin#17331), portions of [bitcoin#21083](bitcoin#21083) (1a6a0b0) and [bitcoin#20536](bitcoin@51e2cd3) (3e69939) were backported.

  * [bitcoin#17331](bitcoin#17331) seems to have silently broken `CreateTransactionInternal` as functional tests fail (see below) despite the backport not intending to change behavior. This was caught due to unit tests introduced in [dash#3667](#3667).

    The aberration seems be remedied by portions of [bitcoin#25647](bitcoin#25647) and [bitcoin#26643](bitcoin#26643) and they have been incorporated into this pull request in a separate commit.

    **Special thanks to UdjinM6 for figuring this out!** 🎉

    <details>

    <summary>Error log:</summary>

    ```
     dash@479e0aa4ebbf:/src/dash$ ./src/test/test_dash -t coinselector_tests,wallet_tests
     Running 21 test cases...
     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 2 - 5

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 4 - 4

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 4 - 5

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 6 - 0

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 6 - 2

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 6 - 4

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 6 - 5

     *** 7 failures are detected in the test module "Dash Core Test Suite"
    ```

    </details>

  ## How Has This Been Tested?

  153bdc2 was tested on Debian 12 (`bookworm`) mixing ~2 tDASH on default settings.

  ![CoinJoin run](https://github.com/user-attachments/assets/da1f13e7-dd83-4211-8d42-0cd4c770bbf1)

  ## Breaking Changes

  * If a transaction isn't shuffled using BIP69 (i.e. if an explicit position for the change txout is specified), it will be randomly shuffled (mirroring upstream behavior, [source](https://github.com/bitcoin/bitcoin/blob/51a3ac242c92e69b59df26f8f9e287b31e5c3b0f/src/wallet/wallet.cpp#L3048)). This deviates from earlier behavior where no shuffling would be done at all if BIP69 isn't applied.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 153bdc2
  PastaPastaPasta:
    utACK 153bdc2

Tree-SHA512: 709b77dce3cea2bbf09eab42c7e70171c3bc6527ff4c387a4db75994da5c0d59025b641d90f734b87a62cdfa8e422d09513697a6e875635de2765a1c9141233e

src/Makefile.test.include

@@ -194,6 +194,7 @@ BITCOIN_TESTS += \
   wallet/test/bip39_tests.cpp \
   wallet/test/coinjoin_tests.cpp \
   wallet/test/psbt_wallet_tests.cpp \
+  wallet/test/spend_tests.cpp \
   wallet/test/wallet_tests.cpp \
   wallet/test/walletdb_tests.cpp \
   wallet/test/wallet_crypto_tests.cpp \

src/bench/coin_selection.cpp

@@ -48,15 +48,14 @@ static void CoinSelection(benchmark::Bench& bench)
         coins.emplace_back(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */);
     }
     const CoinEligibilityFilter filter_standard(1, 6, 0);
-    const CoinSelectionParams coin_selection_params(/* use_bnb= */ true, /* change_output_size= */ 34,
+    const CoinSelectionParams coin_selection_params(/* change_output_size= */ 34,
                                                     /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0),
                                                     /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
                                                     /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false);
     bench.run([&] {
         std::set<CInputCoin> setCoinsRet;
         CAmount nValueRet;
-        bool bnb_used;
-        bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params, bnb_used);
+        bool success = wallet.SelectCoinsMinConf(1003 * COIN, filter_standard, coins, setCoinsRet, nValueRet, coin_selection_params);
         assert(success);
         assert(nValueRet == 1003 * COIN);
         assert(setCoinsRet.size() == 2);
@@ -94,12 +93,11 @@ static void BnBExhaustion(benchmark::Bench& bench)
     std::vector<OutputGroup> utxo_pool;
     CoinSet selection;
     CAmount value_ret = 0;
-    CAmount not_input_fees = 0;
 
     bench.run([&] {
         // Benchmark
         CAmount target = make_hard_case(17, utxo_pool);
-        SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret, not_input_fees); // Should exhaust
+        SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret); // Should exhaust
 
         // Cleanup
         utxo_pool.clear();

src/wallet/coinselection.cpp

@@ -16,7 +16,7 @@
 struct {
     bool operator()(const OutputGroup& a, const OutputGroup& b) const
     {
-        return a.effective_value > b.effective_value;
+        return a.GetSelectionAmount() > b.GetSelectionAmount();
     }
 } descending;
 
@@ -51,37 +51,34 @@ struct {
  * @param const std::vector<CInputCoin>& utxo_pool The set of UTXOs that we are choosing from.
  *        These UTXOs will be sorted in descending order by effective value and the CInputCoins'
  *        values are their effective values.
- * @param const CAmount& target_value This is the value that we want to select. It is the lower
+ * @param const CAmount& selection_target This is the value that we want to select. It is the lower
  *        bound of the range.
  * @param const CAmount& cost_of_change This is the cost of creating and spending a change output.
- *        This plus target_value is the upper bound of the range.
+ *        This plus selection_target is the upper bound of the range.
  * @param std::set<CInputCoin>& out_set -> This is an output parameter for the set of CInputCoins
  *        that have been selected.
  * @param CAmount& value_ret -> This is an output parameter for the total value of the CInputCoins
  *        that were selected.
- * @param CAmount not_input_fees -> The fees that need to be paid for the outputs and fixed size
- *        overhead (version, locktime, marker and flag)
  */
 
 static const size_t TOTAL_TRIES = 100000;
 
-bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret, CAmount not_input_fees)
+bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret)
 {
     out_set.clear();
     CAmount curr_value = 0;
 
     std::vector<bool> curr_selection; // select the utxo at this index
     curr_selection.reserve(utxo_pool.size());
-    CAmount actual_target = not_input_fees + target_value;
 
     // Calculate curr_available_value
     CAmount curr_available_value = 0;
     for (const OutputGroup& utxo : utxo_pool) {
         // Assert that this utxo is not negative. It should never be negative, effective value calculation should have removed it
-        assert(utxo.effective_value > 0);
-        curr_available_value += utxo.effective_value;
+        assert(utxo.GetSelectionAmount() > 0);
+        curr_available_value += utxo.GetSelectionAmount();
     }
-    if (curr_available_value < actual_target) {
+    if (curr_available_value < selection_target) {
         return false;
     }
 
@@ -96,12 +93,12 @@ bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_v
     for (size_t i = 0; i < TOTAL_TRIES; ++i) {
         // Conditions for starting a backtrack
         bool backtrack = false;
-        if (curr_value + curr_available_value < actual_target ||                // Cannot possibly reach target with the amount remaining in the curr_available_value.
-            curr_value > actual_target + cost_of_change ||    // Selected value is out of range, go back and try other branch
+        if (curr_value + curr_available_value < selection_target ||                // Cannot possibly reach target with the amount remaining in the curr_available_value.
+            curr_value > selection_target + cost_of_change ||    // Selected value is out of range, go back and try other branch
             (curr_waste > best_waste && (utxo_pool.at(0).fee - utxo_pool.at(0).long_term_fee) > 0)) { // Don't select things which we know will be more wasteful if the waste is increasing
             backtrack = true;
-        } else if (curr_value >= actual_target) {       // Selected value is within range
-            curr_waste += (curr_value - actual_target); // This is the excess value which is added to the waste for the below comparison
+        } else if (curr_value >= selection_target) {       // Selected value is within range
+            curr_waste += (curr_value - selection_target); // This is the excess value which is added to the waste for the below comparison
             // Adding another UTXO after this check could bring the waste down if the long term fee is higher than the current fee.
             // However we are not going to explore that because this optimization for the waste is only done when we have hit our target
             // value. Adding any more UTXOs will be just burning the UTXO; it will go entirely to fees. Thus we aren't going to
@@ -114,7 +111,7 @@ bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_v
                     break;
                 }
             }
-            curr_waste -= (curr_value - actual_target); // Remove the excess value as we will be selecting different coins now
+            curr_waste -= (curr_value - selection_target); // Remove the excess value as we will be selecting different coins now
             backtrack = true;
         }
 
@@ -123,7 +120,7 @@ bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_v
             // Walk backwards to find the last included UTXO that still needs to have its omission branch traversed.
             while (!curr_selection.empty() && !curr_selection.back()) {
                 curr_selection.pop_back();
-                curr_available_value += utxo_pool.at(curr_selection.size()).effective_value;
+                curr_available_value += utxo_pool.at(curr_selection.size()).GetSelectionAmount();
             }
 
             if (curr_selection.empty()) { // We have walked back to the first utxo and no branch is untraversed. All solutions searched
@@ -133,24 +130,24 @@ bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_v
             // Output was included on previous iterations, try excluding now.
             curr_selection.back() = false;
             OutputGroup& utxo = utxo_pool.at(curr_selection.size() - 1);
-            curr_value -= utxo.effective_value;
+            curr_value -= utxo.GetSelectionAmount();
             curr_waste -= utxo.fee - utxo.long_term_fee;
         } else { // Moving forwards, continuing down this branch
             OutputGroup& utxo = utxo_pool.at(curr_selection.size());
 
             // Remove this utxo from the curr_available_value utxo amount
-            curr_available_value -= utxo.effective_value;
+            curr_available_value -= utxo.GetSelectionAmount();
 
             // Avoid searching a branch if the previous UTXO has the same value and same waste and was excluded. Since the ratio of fee to
             // long term fee is the same, we only need to check if one of those values match in order to know that the waste is the same.
             if (!curr_selection.empty() && !curr_selection.back() &&
-                utxo.effective_value == utxo_pool.at(curr_selection.size() - 1).effective_value &&
+                utxo.GetSelectionAmount() == utxo_pool.at(curr_selection.size() - 1).GetSelectionAmount() &&
                 utxo.fee == utxo_pool.at(curr_selection.size() - 1).fee) {
                 curr_selection.push_back(false);
             } else {
                 // Inclusion branch first (Largest First Exploration)
                 curr_selection.push_back(true);
-                curr_value += utxo.effective_value;
+                curr_value += utxo.GetSelectionAmount();
                 curr_waste += utxo.fee - utxo.long_term_fee;
             }
         }
@@ -283,14 +280,14 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
             if (tryDenom == 0 && CoinJoin::IsDenominatedAmount(group.m_value)) {
                 continue; // we don't want denom values on first run
             }
-            if (group.m_value == nTargetValue) {
+            if (group.GetSelectionAmount() == nTargetValue) {
                 util::insert(setCoinsRet, group.m_outputs);
                 nValueRet += group.m_value;
                 return true;
-            } else if (group.m_value < nTargetValue + nMinChange) {
+            } else if (group.GetSelectionAmount() < nTargetValue + nMinChange) {
                 applicable_groups.push_back(group);
-                nTotalLower += group.m_value;
-            } else if (!lowest_larger || group.m_value < lowest_larger->m_value) {
+                nTotalLower += group.GetSelectionAmount();
+            } else if (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount()) {
                 lowest_larger = group;
             }
         }
@@ -336,7 +333,7 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
     // If we have a bigger coin and (either the stochastic approximation didn't find a good solution,
     //                                   or the next bigger coin is closer), return the bigger coin
     if (lowest_larger &&
-        ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || lowest_larger->m_value <= nBest)) {
+        ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || lowest_larger->GetSelectionAmount() <= nBest)) {
         util::insert(setCoinsRet, lowest_larger->m_outputs);
         nValueRet += lowest_larger->m_value;
     } else {
@@ -400,3 +397,8 @@ bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_f
         && m_ancestors <= eligibility_filter.max_ancestors
         && m_descendants <= eligibility_filter.max_descendants;
 }
+
+CAmount OutputGroup::GetSelectionAmount() const
+{
+    return m_subtract_fee_outputs ? m_value : effective_value;
+}

src/wallet/coinselection.h

@@ -57,6 +57,45 @@ class CInputCoin {
     }
 };
 
+/** Parameters for one iteration of Coin Selection. */
+struct CoinSelectionParams
+{
+    /** Size of a change output in bytes, determined by the output type. */
+    size_t change_output_size = 0;
+    /** Size of the input to spend a change output in virtual bytes. */
+    size_t change_spend_size = 0;
+    /** Cost of creating the change output. */
+    CAmount m_change_fee{0};
+    /** Cost of creating the change output + cost of spending the change output in the future. */
+    CAmount m_cost_of_change{0};
+    /** The fee to spend these UTXOs at the long term feerate. */
+    CFeeRate m_effective_feerate;
+    /** The feerate estimate used to estimate an upper bound on what should be sufficient to spend
+     * the change output sometime in the future. */
+    CFeeRate m_long_term_feerate;
+    /** If the cost to spend a change output at the discard feerate exceeds its value, drop it to fees. */
+    CFeeRate m_discard_feerate;
+    size_t tx_noinputs_size = 0;
+    /** Indicate that we are subtracting the fee from outputs */
+    bool m_subtract_fee_outputs = false;
+    /** When true, always spend all (up to OUTPUT_GROUP_MAX_ENTRIES) or none of the outputs
+     * 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;
+
+    CoinSelectionParams(size_t change_output_size, size_t change_spend_size, CFeeRate effective_feerate,
+                        CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) :
+        change_output_size(change_output_size),
+        change_spend_size(change_spend_size),
+        m_effective_feerate(effective_feerate),
+        m_long_term_feerate(long_term_feerate),
+        m_discard_feerate(discard_feerate),
+        tx_noinputs_size(tx_noinputs_size),
+        m_avoid_partial_spends(avoid_partial)
+    {}
+    CoinSelectionParams() {}
+};
+
 /** Parameters for filtering which OutputGroups we may use in coin selection.
  * We start by being very selective and requiring multiple confirmations and
  * then get more permissive if we cannot fund the transaction. */
@@ -109,18 +148,23 @@ struct OutputGroup
      * a lower feerate). Calculated using long term fee estimate. This is used to decide whether
      * it could be economical to create a change output. */
     CFeeRate m_long_term_feerate{0};
+    /** Indicate that we are subtracting the fee from outputs.
+     * When true, the value that is used for coin selection is the UTXO's real value rather than effective value */
+    bool m_subtract_fee_outputs{false};
 
     OutputGroup() {}
-    OutputGroup(const CFeeRate& effective_feerate, const CFeeRate& long_term_feerate) :
-        m_effective_feerate(effective_feerate),
-        m_long_term_feerate(long_term_feerate)
+    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)
     {}
 
     void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only);
     bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter, bool isISLocked) const;
+    CAmount GetSelectionAmount() const;
 };
 
-bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret, CAmount not_input_fees);
+bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret);
 
 // Original coin selection algorithm as a fallback
 bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& groups, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet, bool fFulyMixedOnly, CAmount maxTxFee);