[amount] Add tests and make GetFee() monotonic

MarcoFalke commented at 4:07 pm on March 17, 2016: member

[qa] Add amount tests fab6880494

[amount] Make GetFee() monotonic

This reverts the hard-to-read and buggy code introduced in
d88af560111863c3e9c1ae855dcc287f04dffb02 and adds documentation

faf756ae4e

MarcoFalke force-pushed on Mar 17, 2016

MarcoFalke commented at 4:35 pm on March 17, 2016: member

@laanwj raised the concern that it should be mentioned in the release notes, but I would be really surprised if anyone was relying on this behavior.

Current master fails the unit test (see travis):

0test/amount_tests.cpp(23): error: check feeRate.GetFee(0) == 0 has failed [1000 != 0]
1test/amount_tests.cpp(32): error: check feeRate.GetFee(0) == 0 has failed [123 != 0]
2test/amount_tests.cpp(33): error: check feeRate.GetFee(8) == 1 has failed [123 != 1]

laanwj commented at 4:35 pm on March 17, 2016: member

I’d like some more details here, you left the opening post empty!

What do you mean with “make GetFee() monotonic”, and what purpose does this have (from a user perspective)?

MarcoFalke commented at 5:02 pm on March 17, 2016: member

What do you mean with “make GetFee() monotonic”,

The current behavior of the code is CFeeRate(1000).GetFee(0) == 1000 and CFeeRate(123).GetFee(8) == 123, which is obviously wrong. This was introduced in d88af56 (#4465).

monotonic

Please have a look at the following sketches:

master: screenshot from 2016-03-17 17-53-24

patch: screenshot from 2016-03-17 17-53-45

what purpose does this have

The purpose of this pull is to get rid of unexpected behavior.

in src/amount.cpp: in faf756ae4e outdated

23+    CAmount nFee = nSatoshisPerK * nSize / 1000;
24 
25-    if (nFee == 0 && nSatoshisPerK > 0)
26-        nFee = nSatoshisPerK;
27+    if (nFee == 0 && nSize != 0 && nSatoshisPerK != 0)
28+        nFee = CAmount(1);

MarcoFalke commented at 5:06 pm on March 17, 2016:

Ideally this code should be removed entirely, but I don’t see that happening soon taking into account the limited effect and the substantial amount of refactoring in wallet code.

laanwj commented at 5:15 pm on March 17, 2016: member

Thanks for the plots, that makes things a lot more clear. That outlier around 0 clearly looks wrong. Yes, this change makes sense.

utACK

jonasschnelli commented at 7:12 am on March 18, 2016: contributor

utACK

jonasschnelli added the label Refactoring on Mar 18, 2016

in src/amount.cpp: in faf756ae4e outdated

18@@ -19,10 +19,10 @@ CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nSize)
19 
20 CAmount CFeeRate::GetFee(size_t nSize) const
21 {
22-    CAmount nFee = nSatoshisPerK*nSize / 1000;
23+    CAmount nFee = nSatoshisPerK * nSize / 1000;
24 
25-    if (nFee == 0 && nSatoshisPerK > 0)
26-        nFee = nSatoshisPerK;
27+    if (nFee == 0 && nSize != 0 && nSatoshisPerK != 0)

morcos commented at 8:11 pm on March 18, 2016:

I don’t understand the != 0 here. Why would you turn a small negative fee rate into a small positive one? @sdaftuar can comment on the vagaries of negative CFeeRates, they are already fraught with peril, but they can exist at least via prioritisetransaction. I think it would be much safer to keep this as > 0.

MarcoFalke commented at 11:02 am on March 19, 2016:

Thanks, I didn’t know negative rates exist.

~~Added tests for negative rates in another commit.~~

MarcoFalke commented at 11:41 am on March 19, 2016:

@morcos Actually, I don’t think we support negative amounts for nSatoshisPerK right now. 64-bit platforms will convert nSatoshisPerK to unsiged (size_t). 32-bit platforms will convert nSize to signed CAmount.

morcos commented at 1:12 pm on March 19, 2016:

TBH, I’m not sure exactly what happens. Although I thought nSatoshiPerK was a CAmount which is an int64_t. I’m not sure it’s meant to support negative rates, but at some point we need to either make sure they are supported or prevent against them. All I’m suggesting is that right now we don’t change any edge case behavior around negative amounts, b/c its already fragile.

MarcoFalke commented at 2:43 pm on March 19, 2016:

at some point we need to either make sure they are supported or prevent against them

I think we agree on this but I’d rather not do it as part of this pull.

MarcoFalke force-pushed on Mar 19, 2016

[amount] Preempt issues with negative fee rates fad13b1612

morcos commented at 8:14 pm on March 19, 2016: member

@MarcoFalke thanks.

utACK fad13b1

laanwj merged this on Mar 21, 2016

laanwj closed this on Mar 21, 2016

laanwj referenced this in commit ddfd79659e on Mar 21, 2016

MarcoFalke deleted the branch on Mar 21, 2016

jtimon commented at 4:17 pm on March 21, 2016: contributor

Somehow I missed this. Even if it’s kind of meaningless now, “posthumous utACK”.

I’ll test this now by rebasing https://github.com/bitcoin/bitcoin/compare/master...jtimon:0.12.99-feerate and repeating the rpc tests locally. I still don’t know them all that well, does it make sense for me to run the all the rpc tests with the -extended label for anything that could be affected by changes in CFeeRate ?

in src/test/amount_tests.cpp: in fad13b1612

28+    BOOST_CHECK_EQUAL(feeRate.GetFee(9e3), 9e3);
29+
30+    feeRate = CFeeRate(123);
31+    // Truncates the result, if not integer
32+    BOOST_CHECK_EQUAL(feeRate.GetFee(0), 0);
33+    BOOST_CHECK_EQUAL(feeRate.GetFee(8), 1); // Special case: returns 1 instead of 0

jtimon commented at 4:25 pm on March 21, 2016:

Now the special case is less special, but, yes, is still a special case, so thank you for documenting it.

laanwj commented at 4:44 pm on March 21, 2016: member

Somehow I missed this. Even if it’s kind of meaningless now, “posthumous utACK”.

No, it’s not meaningless. Reviewing commits that are merged is also essential.

MarcoFalke commented at 4:46 pm on March 21, 2016: member

repeating the rpc tests locally

I can’t think of any way your results should be different by rebasing onto this pull. This only has an effect if the size and/or fee rate is really small.

jtimon commented at 8:01 pm on March 21, 2016: contributor

I can’t think of …

Well, I can think of them. That doesn’t mean they make sense, see #7731

codablock referenced this in commit 25215cf7b5 on Sep 16, 2017

codablock referenced this in commit 909751dbbd on Sep 19, 2017

codablock referenced this in commit 0862da875b on Dec 9, 2017

codablock referenced this in commit 8fde2fd5fa on Dec 19, 2017

DrahtBot locked this on Sep 8, 2021

[amount] Add tests and make GetFee() monotonic #7705