[amount] Add support for negative fee rates

MarcoFalke commented at 11:22 am on April 3, 2016: member

Currently negative fee rates are not supported on archs of 64-bit or more

laanwj commented at 11:28 am on April 3, 2016: member

Negative fee rates imply that you pay a smaller, more negative fee the larger the transaction? What do negative fees even mean, steal from the miner? Or is this so that anti-transactions of negative size pay a positive fee?

I’d say this is an edge case better to get rid of (e.g. assert or throw an error).

sipa commented at 11:30 am on April 3, 2016: member

Uninformed guess: maybe they are relevant for priotizetransaction?

laanwj commented at 11:31 am on April 3, 2016: member

According to @MarcoFalke they already don’t work on 64-bit architectures, which is probably 90%+ of all nodes. Better to remove it for 32 bit as well.

laanwj added the label Tests on Apr 3, 2016

MarcoFalke commented at 11:35 am on April 3, 2016: member

@sipa is correct @laanwj I have pushed the fix as a second commit, so the travis result can be compared. The fix is probably a lot smaller than the fix which removes support.

in src/amount.cpp: in fa064427ab outdated

13+CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nSize_)
14 {
15+    int64_t nSize = nSize_;
16     if (nSize > 0)
17-        nSatoshisPerK = nFeePaid*1000/nSize;
18+        nSatoshisPerK = nFeePaid * 1000 / nSize;

laanwj commented at 11:46 am on April 3, 2016:

Instead of the temporary variable, why not just:

0nSatoshisPerK = nFeePaid * 1000 / static_cast<int64_t>(nSize);

Also at least theoretically you should handle the case where nSize is outside the range of int64_t, though I doubt someone will ever care about transactions that large.

MarcoFalke commented at 12:00 pm on April 3, 2016:

If it is used more than once, you’d rather want to do the cast only once. I guess an alternative would be to change it in the constructor arguments, but that is less verbose, imo.

laanwj commented at 12:12 pm on April 3, 2016:

I guess an alternative would be to change it in the constructor arguments

I thought of that, but didn’t mention it at second thought. Before you know it people will want full support for negative sizes as well. The appropriate type is size_t.

MarcoFalke commented at 5:01 pm on April 8, 2016:

This would require a cast in 4 places instead of two (see diff below) but I am happy to apply the diff an squash. Please let me know how to proceed so we can finalize this.

 0diff --git a/src/amount.cpp b/src/amount.cpp
 1index 7b8618d..3966c5f 100644
 2--- a/src/amount.cpp
 3+++ b/src/amount.cpp
 4@@ -13,8 +13,7 @@ CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nBytes_)
 5 {
 6     assert(nBytes_ <= uint64_t(std::numeric_limits<int64_t>::max()));
 7-    int64_t nSize = int64_t(nBytes_);
 8
 9-    if (nSize > 0)
10-        nSatoshisPerK = nFeePaid * 1000 / nSize;
11+    if (int64_t(nBytes_) > 0)
12+        nSatoshisPerK = nFeePaid * 1000 / int64_t(nBytes_);
13     else
14         nSatoshisPerK = 0;
15@@ -24,9 +23,8 @@ CAmount CFeeRate::GetFee(size_t nBytes_) const
16 {
17     assert(nBytes_ <= uint64_t(std::numeric_limits<int64_t>::max()));
18-    int64_t nSize = int64_t(nBytes_);
19
20-    CAmount nFee = nSatoshisPerK * nSize / 1000;
21+    CAmount nFee = nSatoshisPerK * int64_t(nBytes_) / 1000;
22
23-    if (nFee == 0 && nSize != 0) {
24+    if (nFee == 0 && int64_t(nBytes_) != 0) {
25         if (nSatoshisPerK > 0)
26             nFee = CAmount(1);

laanwj commented at 11:48 am on April 3, 2016: member

Ok, at least document this then, for example in the doc comment of the constructor. It’s extremely unintuitive to me and probably to others reading this code as well.

MarcoFalke commented at 12:00 pm on April 3, 2016: member

Ok, will add the doc later…

morcos commented at 1:54 pm on April 3, 2016: member

ping @sdaftuar

sdaftuar commented at 2:08 pm on April 5, 2016: member

I think it’s probably better to fix support for negative fee rates on 64-bit platforms, because (a) fixing the prioritisetransaction RPC to disallow negative feerates would be difficult/annoying because fee deltas can be stored prior to transaction acceptance, (b) it would be counterintuitive if you called prioritisetransaction with -X and then later with X but didn’t get back to where you started, and (c) I think there might be legitimate reasons to want to keep an ordering of transactions you don’t want to mine right now (which you’d lose if you tried to floor everything at zero, which is the easiest reasonable alternative I can come up with).

But to be fair, I’m surprised that no one has complained about this before, because as far as I can tell, up until 0.12, if you used prioritisetransaction to apply a negative feerate, then the mining code would immediately select it as a super-high-fee transaction. (I stumbled upon this behavior while working on #7063.)

So, concept ACK. I agree with @laanwj’s comment about the cast. Also, I think it probably makes most sense to enforce the property that CFeeRate(X).GetFee(size) == -1 * CFeeRate(-X).GetFee(size); if everyone agrees, then we should make the special case rounding recently introduced (where we return CFeeRate(1)) also apply in the negative fee rate case.

Also, we should add a unit test that exercises the constructor CFeeRate(const CAmount& nFeePaid, size_t nSize_) that is changed in this PR.

MarcoFalke force-pushed on Apr 8, 2016

MarcoFalke commented at 5:01 pm on April 8, 2016: member

I think it probably makes most sense to enforce the property that CFeeRate(X).GetFee(size) == -1 * CFeeRate(-X).GetFee(size);

Makes sense, done.

add a unit test that exercises the constructor CFeeRate(const CAmount& nFeePaid, size_t nSize_)

Done.

MarcoFalke force-pushed on Apr 8, 2016

[amount] test negative fee rates and full constructor 11114a69c8

[amount] Add support for negative fee rates

Currently negative fee rates are not supported on archs of 64-bit or
more

fa2da2cb60

MarcoFalke force-pushed on Apr 8, 2016

MarcoFalke force-pushed on Apr 9, 2016

MarcoFalke commented at 11:54 am on April 9, 2016: member

Before on 64-bit:

0test/amount_tests.cpp(33): error: feeRate.GetFee(1) == -1 has failed [18446744073709550 != -1]
1test/amount_tests.cpp(34): error: feeRate.GetFee(121) == -121 has failed [18446744073709430 != -121]
2test/amount_tests.cpp(35): error: feeRate.GetFee(999) == -999 has failed [18446744073708552 != -999]
3test/amount_tests.cpp(36): error: feeRate.GetFee(1e3) == -1e3 has failed [18446744073708551 != -1000]
4test/amount_tests.cpp(37): error: feeRate.GetFee(9e3) == -9e3 has failed [18446744073700551 != -9000]
5test/amount_tests.cpp(53): error: feeRate.GetFee(8) == -1 has failed [18446744073709550 != -1]
6test/amount_tests.cpp(54): error: feeRate.GetFee(9) == -1 has failed [18446744073709550 != -1]
7test/amount_tests.cpp(58): error: CFeeRate(CAmount(-1), 1000) == CFeeRate(-1) has failed

After: tests pass

MarcoFalke force-pushed on Apr 9, 2016

[amount] tests: Fix off-by-one mistake facf5a4947

laanwj commented at 9:54 am on April 14, 2016: member

utACK facf5a4

laanwj merged this on Apr 14, 2016

laanwj closed this on Apr 14, 2016

laanwj referenced this in commit 536b75e946 on Apr 14, 2016

in src/amount.cpp: in facf5a4947

30+
31     CAmount nFee = nSatoshisPerK * nSize / 1000;
32 
33-    if (nFee == 0 && nSize != 0 && nSatoshisPerK > 0)
34-        nFee = CAmount(1);
35+    if (nFee == 0 && nSize != 0) {

jtimon commented at 1:58 pm on April 14, 2016:

Couldn’t have we just removed this special case from here (ie move this check to the callers that need it)? I know that would be more disruptive, but it will also make increasing the internal precision easier later.

MarcoFalke commented at 2:55 pm on April 14, 2016:

Sure!

I am only aware of the wallet using it: https://github.com/bitcoin/bitcoin/blob/430fffefaab6832b8f6605f14a992e7e55b9547e/src/wallet/wallet.cpp#L2335

jtimon commented at 5:03 pm on April 14, 2016:

Oh, that’s great. Moving from nSatoshisPerK to 1 was already and step forward. But I’ve strong reason to believe that this special case was the most important impediment for not being able to “easily” do more than *2 on internal precision in #7731(in fact, I don’t think it would pass all the tests if I rebased this now due to this change).

If you create a PR to move the special case to the wallet only, please ping me for review, I am very interested in seeing that happening. Looking at the code you link to, it looks like that part can actually be simplified as a result, instead of complicating it. But tests will break…

jtimon commented at 2:42 pm on April 14, 2016: contributor

After this, the following change will fail the new tests in amount_tests.cpp: https://github.com/bitcoin/bitcoin/pull/7728/commits/5ca24733d08ac57e01c521ded864fd516448f351

This is a mystery to me, can anybody help me understand? I get the same errors that MarcoFalke gets “Before on 64-bit:”, do I have to do something locally on 64 bits linux?

MarcoFalke deleted the branch on Apr 14, 2016

codablock referenced this in commit b3077e092f on Sep 16, 2017

codablock referenced this in commit 3f17151c16 on Sep 19, 2017

codablock referenced this in commit 20cb9aa21d on Dec 20, 2017

MarcoFalke locked this on Sep 8, 2021

[amount] Add support for negative fee rates #7796