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

pull jaeheonshim wants to merge 2 commits into bitcoin:master from jaeheonshim:2412-refactor-fee-rate-rounding changing 1 files +1 −4

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:
```
if (nFee == 0 && nSize != 0) {
    if (nSatoshisPerK > 0) nFee = CAmount(1);
    if (nSatoshisPerK < 0) nFee = CAmount(-1);
}
```
Addresses #31558. This fix improves code readability.

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

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.
DrahtBot added the label Refactoring on Dec 27, 2024

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.

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.
Remove unnecessary if statement 90e989e47c
jaeheonshim commented at 1:23 PM on December 31, 2024: none

What @maflcko said makes sense. I'll do some more investigating.
jaeheonshim closed this on Dec 31, 2024
bitcoin locked this on Dec 31, 2025

Contributors

Labels

Linked (view graph)

#31558 Stale code (that has no effect)

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

Code Coverage & Benchmarks

Reviews