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. 
   DrahtBot commented at 5:46 PM on February 5, 2025: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	marcofleon
   Stale 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.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

3. DrahtBot added the label Tests on Feb 5, 2025
4. 
   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 ?

   if (best_weight < std::numeric_limits<int>::max()) {
           // Sufficient funds and acceptable weight: CoinGrinder should find at least one solution
           int high_max_selection_weight = fuzzed_data_provider.ConsumeIntegralInRange<int>(best_weight, std::numeric_limits<int>::max());
   
           auto result_cg = CoinGrinder(group_pos, target, coin_params.m_min_change_target, high_max_selection_weight);
           assert(result_cg);
           assert(result_cg->GetWeight() <= high_max_selection_weight);
           assert(result_cg->GetSelectedEffectiveValue() >= target + coin_params.m_min_change_target);
           assert(best_weight < result_cg->GetWeight() || (best_weight == result_cg->GetWeight() && best_amount <= result_cg->GetSelectedEffectiveValue()));
           if (result_cg->GetAlgoCompleted()) {
               // If CoinGrinder exhausted the search space, it must return the optimal solution
               assert(best_weight == result_cg->GetWeight());
               assert(best_amount == result_cg->GetSelectedEffectiveValue());
           }
       }
   

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

5. 
   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

6. 
   fanquake commented at 8:55 AM on March 21, 2025: member

   cc @marcofleon

7. DrahtBot added the label Needs rebase on Mar 21, 2025
8. fuzz: coinselection: cover `SetBumpFeeDiscount` 0ff66b1c4a
9. brunoerg force-pushed on Mar 21, 2025
10. 
    brunoerg commented at 1:17 PM on March 21, 2025: contributor

    Rebased

11. DrahtBot removed the label Needs rebase on Mar 21, 2025
12. 
    marcofleon commented at 8:35 PM on March 24, 2025: contributor

    ACK 0ff66b1c4ab0e5cba00a178b24f2c5de75df360f

    Checked for the additional coverage to be sure, lgtm

13. DrahtBot requested review from Prabhat1308 on Mar 24, 2025
14. ryanofsky assigned ryanofsky on Apr 1, 2025
15. ryanofsky merged this on Apr 1, 2025
16. ryanofsky closed this on Apr 1, 2025

    ---

Review Requested
Prabhat1308