Stale code (that has no effect)

tcharding commented at 10:59 pm on December 23, 2024: contributor

Is there an existing issue for this?

I have searched the existing issues

Current behaviour

I am not a C++ programmer. This code looks like it has 4 lines that do nothing.

In commit: 0fbaef9676a a call to std::ceil was added which makes the following if statement do nothing. There is no harm done but it makes the code confusing for non-c++ devs to read.

 0CAmount CFeeRate::GetFee(uint32_t num_bytes) const
 1{
 2    const int64_t nSize{num_bytes};
 3
 4    // Be explicit that we're converting from a double to int64_t (CAmount) here.
 5    // We've previously had issues with the silent double->int64_t conversion.
 6    CAmount nFee{static_cast<CAmount>(std::ceil(nSatoshisPerK * nSize / 1000.0))};
 7
 8    if (nFee == 0 && nSize != 0) {
 9        if (nSatoshisPerK > 0) nFee = CAmount(1);
10        if (nSatoshisPerK < 0) nFee = CAmount(-1);
11    }
12
13    return nFee;
14}

Expected behaviour

There is no behaviour issue with the current code.

tcharding commented at 11:00 pm on December 23, 2024: contributor

cc @achow101

maflcko commented at 10:53 am on December 24, 2024: member

Removing the code yields a test failure for negative feerates for me:

0test/amount_tests.cpp(64): error: in "amount_tests/GetFeeTest": check feeRate.GetFee(8) == CAmount(-1) has failed [0 != -1]
1
2*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

I’ve documented that in fa2da2cb607ba359231fccc9635abe7c8616de56 .

Though, I find it a bit odd to treat negative and positives feerates differently. So the code should probably still be removed and the ceil should be fixed to always ceil away from zero.

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.

maflcko added the label TX fees and policy on Dec 24, 2024

maflcko added the label RPC/REST/ZMQ on Dec 24, 2024

jaeheonshim referenced this in commit d7a341c06b on Dec 27, 2024

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

With the current unit tests, I don’t think ceil away from zero will pass unit tests. The way the logic is currently implemented, negative values are rounded up towards zero unless they are greater than -1, in which case they are instead rounded down towards -1. I’m not sure if this is the intended behavior, but if a fix like this were to be implemented:

0double value = nSatoshisPerK * nSize / 1000.0;
1nFee = static_cast<CAmount>(std::copysign(std::ceil(std::abs(value)), value));

This test case will fail:

0feeRate = CFeeRate(-123);
1BOOST_CHECK_EQUAL(feeRate.GetFee(9), CAmount(-1));

As -123 * 9 / 1000.0 = -1.107, the old code would round towards 0 to -1, whereas the new code would round away from zero to -2.

Just thought it was worth pointing that out.

yancyribbens commented at 10:53 pm on December 31, 2024: contributor

Maybe this issue should be closed and a new one started. The original issue states the code section isn’t used, but it pretty clearly is, especially since there’s even a test that shows it is. It is a bit of a mystery what the authors intent was and a code comment might have been nice to explain why if the ceil function resulted in zero and the size is not zero, then return -1 if fee_rate is negative or 1 if fee_rate is positive.

yancyribbens commented at 1:33 am on January 1, 2025: contributor

Actually, I think the “stale code” section is to prevent a fee of zero, and if the fee would be zero, then make it either 1 sat or -1 sat depending of if the fee_rate is positive or negative.

jaeheonshim commented at 1:40 am on January 1, 2025: none

Right, but I think the question is whether negative fee rates should be rounded away from or towards zero. Also, I’m not sure what kind of scenario a negative fee rate would be used.

tcharding commented at 2:47 am on January 1, 2025: contributor

In commit: https://github.com/bitcoin/bitcoin/commit/0fbaef9676a1dcb84bcf95afd8d994831ab327b6 a call to std::ceil was added which makes the following if statement do nothing.

Oh it seems I can’t do integer division. nFee will be zero any time 0 < (nSatoshisPerK * nSize) < 1000

EDIT: I was confused in the line above, the code in question uses / 1000.0 not integer division. Hence I was not being totally retarded when I opened this issue. I was just missing the fact that nSatoshisPerK could be negative.

I have no opinion on the rounding issue and will leave that to others to raise an issue if needed. Excuse the noise.

tcharding closed this on Jan 1, 2025

yancyribbens commented at 6:42 pm on January 1, 2025: contributor

Right, but I think the question is whether negative fee rates should be rounded away from or towards zero

ceil rounds towards zero: ceil(-2.4) = -2.000000. Rounding toward zero when negative is correct imo. Rounding up is done to make sure the fee is adequate instead of falling short.

Also, I’m not sure what kind of scenario a negative fee rate would be used.

Good question. frameworks like rust-bitcoin use an unsigned int for FeeRate so it can’t be negative.

tcharding commented at 8:13 pm on January 1, 2025: contributor

If I’m not mistaken (again) this line of code can never be hit.

if (nSatoshisPerK > 0) nFee = CAmount(1);

sipa commented at 8:43 pm on January 1, 2025: member

I believe the only way to hit negative feerates is when using the prioritizetransaction RPC with a (highly) negative fee_delta argument. I don’t know why in this context rounding away from zero (as CFeeRate does) would matter. I believe the correct rounding mode depends on the context:

In fee estimation context, one always wants to round up (if you need 351.3 sats to hit a desired feerate, you need to pay 352).
In mining/mempool context, one wants to round down probably for sorting purposes (or use exact fraction arithmetic).

Longer term, I think CFeeRate should be replaced with, or implemented using, FeeFrac which uses exact integer arithmetic, and avoids divisions where possible. #30535 adds the ability to evaluate a feerate for a given size, while rounding up or rounding down, giving it feature parity with CFeeRate.

yancyribbens commented at 11:01 pm on January 1, 2025: contributor

If I’m not mistaken (again) this line of code can never be hit.

If the goal is to prevent a fee of zero, it probably should be: if (nSatoshisPerK == 0) nFee = CAmount(1);

sipa commented at 11:24 pm on January 1, 2025: member

For context, support for negative feerates in CFeeRate was added in #7796.

Stale code (that has no effect) #31558

Is there an existing issue for this?

Current behaviour

Expected behaviour