Stale code (that has no effect) #31558

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.

CAmount CFeeRate::GetFee(uint32_t num_bytes) const
{
    const int64_t nSize{num_bytes};

    // Be explicit that we're converting from a double to int64_t (CAmount) here.
    // We've previously had issues with the silent double->int64_t conversion.
    CAmount nFee{static_cast<CAmount>(std::ceil(nSatoshisPerK * nSize / 1000.0))};

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

    return nFee;
}

Expected behaviour

There is no behaviour issue with the current code.

cc @achow101

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

test/amount_tests.cpp(64): error: in "amount_tests/GetFeeTest": check feeRate.GetFee(8) == CAmount(-1) has failed [0 != -1]

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

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:

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

This test case will fail:

feeRate = CFeeRate(-123);
BOOST_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.

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.

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.

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.

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.

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.

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

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

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.

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);

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