refactor: Remove redundant edge case in fee rate rounding logic #31572

1. 
   jaeheonshim commented at 1:14 pm on December 27, 2024: none

   Positive fractional fee rates are already rounded away from zero, so the first half of this if-statement is unnecessary:

   0if (nFee == 0 && nSize != 0) {
   1    if (nSatoshisPerK > 0) nFee = CAmount(1);
   2    if (nSatoshisPerK < 0) nFee = CAmount(-1);
   3}
   

   Addresses #31558. This fix improves code readability.

2. Refactor redundant code in CFeeRate::GetFee fee rate calculation logic (fixes #31558)

   An edge case in fee rate rounding only occurs when the fee rate is negative but greater than -1, in which case we wish to round down to -1. In all other cases, negative fee rates should be rounded towards zero. The previous code included an edge case for fractional positive fee rates, but this is not necessary as the case is handled by ceil.

   d7a341c06b
3. 
   DrahtBot commented at 1:14 pm on December 27, 2024: 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/31572.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

4. DrahtBot added the label Refactoring on Dec 27, 2024
5. in src/policy/feerate.cpp:31 in d7a341c06b outdated

   27@@ -28,10 +28,7 @@ CAmount CFeeRate::GetFee(uint32_t num_bytes) const
   28     // We've previously had issues with the silent double->int64_t conversion.
   29     CAmount nFee{static_cast<CAmount>(std::ceil(nSatoshisPerK * nSize / 1000.0))};
   30 
   31-    if (nFee == 0 && nSize != 0) {
   32-        if (nSatoshisPerK > 0) nFee = CAmount(1);
   33-        if (nSatoshisPerK < 0) nFee = CAmount(-1);
   34-    }
   35+    if (nFee == 0 && nSize != 0 && nSatoshisPerK < 0) if (nSatoshisPerK < 0) nFee = CAmount(-1);
   

   ---

   ---

   

   davidgumberg commented at 6:56 am on December 31, 2024:

   Why the second if statement?

   ---

   

   jaeheonshim commented at 1:22 pm on December 31, 2024:

   Sorry, that if statement is unnecessary. I must have missed it.
6. 
   maflcko commented at 10:11 am on December 31, 2024: member

   Addresses #31558. This fix improves code readability.

   I don’t think it does. Someone will need to investigate why the behavior is not symmetric for negative feerates and whether making it symmetric is a bugfix. See #31558 (comment):

   I haven’t checked if such a change is a bugfix or behavior change for prioritisetransaction, so it would be good to check that as well.

7. Remove unnecessary if statement 90e989e47c
8. 
   jaeheonshim commented at 1:23 pm on December 31, 2024: none

   What @maflcko said makes sense. I’ll do some more investigating.
9. jaeheonshim closed this on Dec 31, 2024

   ---

Contributors
jaeheonshim DrahtBot davidgumberg maflcko

Labels
Refactoring