New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[amount] Add support for negative fee rates #7796

Merged
merged 3 commits into from Apr 14, 2016

Conversation

Projects
None yet
6 participants
@MarcoFalke
Member

MarcoFalke commented Apr 3, 2016

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 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).

Member

laanwj commented Apr 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 3, 2016

Member
Member

sipa commented Apr 3, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 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.

Member

laanwj commented Apr 3, 2016

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 laanwj added the Tests label Apr 3, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 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.

Member

MarcoFalke commented Apr 3, 2016

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

@laanwj

View changes

Show outdated Hide outdated src/amount.cpp
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 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.

Member

laanwj commented Apr 3, 2016

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 3, 2016

Member

Ok, will add the doc later...

Member

MarcoFalke commented Apr 3, 2016

Ok, will add the doc later...

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos
Member

morcos commented Apr 3, 2016

ping @sdaftuar

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Apr 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.

Member

sdaftuar commented Apr 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 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.

Member

MarcoFalke commented Apr 8, 2016

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 added some commits Apr 3, 2016

[amount] Add support for negative fee rates
Currently negative fee rates are not supported on archs of 64-bit or
more
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Apr 9, 2016

Member

Before on 64-bit:

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

After:
tests pass

Member

MarcoFalke commented Apr 9, 2016

Before on 64-bit:

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

After:
tests pass

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 14, 2016

Member

utACK facf5a4

Member

laanwj commented Apr 14, 2016

utACK facf5a4

@laanwj laanwj merged commit facf5a4 into bitcoin:master Apr 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 14, 2016

Merge #7796: [amount] Add support for negative fee rates
facf5a4 [amount] tests: Fix off-by-one mistake (MarcoFalke)
fa2da2c [amount] Add support for negative fee rates (MarcoFalke)
11114a6 [amount] test negative fee rates and full constructor (MarcoFalke)
CAmount nFee = nSatoshisPerK * nSize / 1000;
if (nFee == 0 && nSize != 0 && nSatoshisPerK > 0)
nFee = CAmount(1);
if (nFee == 0 && nSize != 0) {

This comment has been minimized.

@jtimon

jtimon Apr 14, 2016

Member

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.

@jtimon

jtimon Apr 14, 2016

Member

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.

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 14, 2016

Member

Sure!

I am only aware of the wallet using it:

if (nFeeNeeded == 0) {

@MarcoFalke

MarcoFalke Apr 14, 2016

Member

Sure!

I am only aware of the wallet using it:

if (nFeeNeeded == 0) {

This comment has been minimized.

@jtimon

jtimon Apr 14, 2016

Member

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

jtimon Apr 14, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Apr 14, 2016

Member

After this, the following change will fail the new tests in amount_tests.cpp: 5ca2473

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?

Member

jtimon commented Apr 14, 2016

After this, the following change will fail the new tests in amount_tests.cpp: 5ca2473

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 MarcoFalke deleted the MarcoFalke:Mf1604-amountNeg64 branch Apr 14, 2016

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7796: [amount] Add support for negative fee rates
facf5a4 [amount] tests: Fix off-by-one mistake (MarcoFalke)
fa2da2c [amount] Add support for negative fee rates (MarcoFalke)
11114a6 [amount] test negative fee rates and full constructor (MarcoFalke)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7796: [amount] Add support for negative fee rates
facf5a4 [amount] tests: Fix off-by-one mistake (MarcoFalke)
fa2da2c [amount] Add support for negative fee rates (MarcoFalke)
11114a6 [amount] test negative fee rates and full constructor (MarcoFalke)

codablock added a commit to codablock/dash that referenced this pull request Dec 20, 2017

Merge #7796: [amount] Add support for negative fee rates
facf5a4 [amount] tests: Fix off-by-one mistake (MarcoFalke)
fa2da2c [amount] Add support for negative fee rates (MarcoFalke)
11114a6 [amount] test negative fee rates and full constructor (MarcoFalke)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment