fuzz: coinselection: cover SetBumpFeeDiscount #31806

1. 
   brunoerg commented at 5:46 pm on February 5, 2025: contributor

   SetBumpFeeDiscount sets the bump fee discount which is used to calculate the waste. We currently have no fuzz coverage for this function, so this PR adds it by calling SetBumpFeeDiscount before RecalculateWaste.
2. fuzz: coinselection: cover `SetBumpFeeDiscount` 0289f03790
3. 
   DrahtBot commented at 5:46 pm on February 5, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31806.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	Prabhat1308, arejula27

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31870 (fuzz: split coinselection harness by brunoerg)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. DrahtBot added the label Tests on Feb 5, 2025
5. 
   Prabhat1308 commented at 10:30 am on February 11, 2025: none

   ACK 0289f03

   result_{selection-algo-type}->SetBumpFeeDiscount() sets discount amount before Recalculating the Waste . This will catch any underflow scenarios if occuring while subtracting the discount from waste. Also adding it increased fuzz coverage for earlier untouched part.

   Also shouldn’t it also be added in FUZZ_TARGET(coin_grinder_is_optimal) here ?

    0if (best_weight < std::numeric_limits<int>::max()) {
    1        // Sufficient funds and acceptable weight: CoinGrinder should find at least one solution
    2        int high_max_selection_weight = fuzzed_data_provider.ConsumeIntegralInRange<int>(best_weight, std::numeric_limits<int>::max());
    3
    4        auto result_cg = CoinGrinder(group_pos, target, coin_params.m_min_change_target, high_max_selection_weight);
    5        assert(result_cg);
    6        assert(result_cg->GetWeight() <= high_max_selection_weight);
    7        assert(result_cg->GetSelectedEffectiveValue() >= target + coin_params.m_min_change_target);
    8        assert(best_weight < result_cg->GetWeight() || (best_weight == result_cg->GetWeight() && best_amount <= result_cg->GetSelectedEffectiveValue()));
    9        if (result_cg->GetAlgoCompleted()) {
   10            // If CoinGrinder exhausted the search space, it must return the optimal solution
   11            assert(best_weight == result_cg->GetWeight());
   12            assert(best_amount == result_cg->GetSelectedEffectiveValue());
   13        }
   14    }
   

   just before first assert statement withGetSelectedEffectiveValue() since it also uses bump_fee_group_discount

6. 
   arejula27 commented at 5:38 pm on February 11, 2025: none

   ACK 0289f03

   This PR aims to improve code coverage of the fuzzing test by ensuring SetBumpFeeDiscount is called before the fee discount is used in RecalculateWaste. To achieve this, SetBumpFeeDiscount is added before RecalculateWaste is invoked. As @Prabhat1308 pointed out, there is another instance where this function could be added (line 290).

   The value set by SetBumpFeeDiscount is also used in other functions, such as GetSelectedEffectiveValue and GetTotalBumpFees, which are referenced, for example, in line 203. @brunoerg Is there a specific reason why you applied SetBumpFeeDiscount only to RecalculateWaste and not to these other functions? If not, it might be worth considering adding it to them as well.

   I ran the fuzz test for 1 minute and It was ok

Contributors
brunoerg DrahtBot Prabhat1308 arejula27

Labels
Tests