Merge bitcoin/bitcoin#25507: wallet: don't add change fee to target if subtracting fees from output

140d942634f9f1bba191aafa948df57812c0f3fe wallet: don't add change fee to target if subtracting fees from output (S3RK)

Pull request description:

  Change fee is payed by the recipient, so we don't need to add it to our target for coin selection.

ACKs for top commit:
  achow101:
    ACK 140d942634f9f1bba191aafa948df57812c0f3fe
  ishaanam:
    ACK 140d942634f9f1bba191aafa948df57812c0f3fe
  furszy:
    Code review ACK 140d9426

Tree-SHA512: b5efd0264c47ecee9204a3fd039bad24c69f9e614c6e1d9bb240ee5be6356b175aa074f3be123e6cfb8becd4d7bd1028eebe18801662cc69d19413d8d5a9dd5c
This commit is contained in:
Andrew Chow 2022-07-06 10:48:21 -04:00
commit aeab1b42e6
No known key found for this signature in database
GPG Key ID: 17565732E08E5E41

View File

@ -397,10 +397,13 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
// The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here.
std::vector<OutputGroup> all_groups = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, false /* positive_only */); std::vector<OutputGroup> all_groups = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, false /* positive_only */);
CAmount target_with_change = nTargetValue;
// While nTargetValue includes the transaction fees for non-input things, it does not include the fee for creating a change output. // While nTargetValue includes the transaction fees for non-input things, it does not include the fee for creating a change output.
// So we need to include that for KnapsackSolver as well, as we are expecting to create a change output. // So we need to include that for KnapsackSolver and SRD as well, as we are expecting to create a change output.
if (auto knapsack_result{KnapsackSolver(all_groups, nTargetValue + coin_selection_params.m_change_fee, if (!coin_selection_params.m_subtract_fee_outputs) {
coin_selection_params.m_min_change_target, coin_selection_params.rng_fast)}) { target_with_change += coin_selection_params.m_change_fee;
}
if (auto knapsack_result{KnapsackSolver(all_groups, target_with_change, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast)}) {
knapsack_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); knapsack_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
results.push_back(*knapsack_result); results.push_back(*knapsack_result);
} }
@ -409,7 +412,7 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
// barely meets the target. Just use the lower bound change target instead of the randomly // barely meets the target. Just use the lower bound change target instead of the randomly
// generated one, since SRD will result in a random change amount anyway; avoid making the // generated one, since SRD will result in a random change amount anyway; avoid making the
// target needlessly large. // target needlessly large.
const CAmount srd_target = nTargetValue + coin_selection_params.m_change_fee + CHANGE_LOWER; const CAmount srd_target = target_with_change + CHANGE_LOWER;
if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target, coin_selection_params.rng_fast)}) { if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target, coin_selection_params.rng_fast)}) {
srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
results.push_back(*srd_result); results.push_back(*srd_result);