[amount] Add tests and make GetFee() monotonic #7705
Conversation
This reverts the hard-to-read and buggy code introduced in d88af56 and adds documentation
maflcko
force-pushed
the
Mf1603-amountFix
branch
from
faf756a to
fab6880
Compare
|
@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): test/amount_tests.cpp(23): error: check feeRate.GetFee(0) == 0 has failed [1000 != 0]
test/amount_tests.cpp(32): error: check feeRate.GetFee(0) == 0 has failed [123 != 0]
test/amount_tests.cpp(33): error: check feeRate.GetFee(8) == 1 has failed [123 != 1] |
|
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)? |
The current behavior of the code is
Please have a look at the following sketches:
The purpose of this pull is to get rid of unexpected behavior. |
| if (nFee == 0 && nSatoshisPerK > 0) | ||
| nFee = nSatoshisPerK; | ||
| if (nFee == 0 && nSize != 0 && nSatoshisPerK != 0) | ||
| nFee = CAmount(1); |
There was a problem hiding this comment.
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.
|
Thanks for the plots, that makes things a lot more clear. That outlier around 0 clearly looks wrong. utACK |
|
utACK |
src/amount.cpp
Outdated
|
|
||
| if (nFee == 0 && nSatoshisPerK > 0) | ||
| nFee = nSatoshisPerK; | ||
| if (nFee == 0 && nSize != 0 && nSatoshisPerK != 0) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks, I didn't know negative rates exist.
Added tests for negative rates in another commit.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
maflcko
force-pushed
the
Mf1603-amountFix
branch
from
fa8177f to
faf756a
Compare
|
@MarcoFalke thanks. utACK fad13b1 |
|
Somehow I missed this. Even if it's kind of meaningless now, "posthumous utACK". I'll test this now by rebasing 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 ? |
| feeRate = CFeeRate(123); | ||
| // Truncates the result, if not integer | ||
| BOOST_CHECK_EQUAL(feeRate.GetFee(0), 0); | ||
| BOOST_CHECK_EQUAL(feeRate.GetFee(8), 1); // Special case: returns 1 instead of 0 |
There was a problem hiding this comment.
Now the special case is less special, but, yes, is still a special case, so thank you for documenting it.
No, it's not meaningless. Reviewing commits that are merged is also essential. |
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. |
Well, I can think of them. That doesn't mean they make sense, see #7731 |
5 participants
No description provided.