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

Remove coin age priority and free transactions - implementation #9602

Merged
merged 13 commits into from Mar 7, 2017

Conversation

@morcos
Member

morcos commented Jan 20, 2017

Please see #9601 for all non-technical discussion.
Includes #9391 as the first commit
Please tag 0.15

This PR removes all coin age priority functionality from the codebase.

Some remaining open questions:

  • The prioritisetransaction RPC API is broken. Should we instead try to implement backwards compatible support, or is it fair to break it? Previously it required exactly 3 arguments, now it requires exactly 2, so it will not result in unintended consequences.
  • The 6th commit changes sendrawtransaction to no longer bypass minRelayTxFee by setting fLimitFree to false. prioritisetransaction can always be used to achieve the same effect. This seems an improvement in behavior, but isn't required (although the prioritise_transaction.py RPC test would need to be fixed if we don't include this).
  • fLimitFree is now only set false for transactions we are trying to reaccept to the mempool from a disconnected block. Should it also be used to bypass the mempool min fee in the event of mempool limiting. I think yes, but can be left for a separate PR. (EDIT for clarification: currently if mempool limiting is active, even if fLimitFree is set false, transactions which don't pass the mempool min fee set by the last evicted transaction will still not be accepted to the mempool. It is probably preferable that they are accepted and then the mempool will be limited again post acceptance and either there will have been room for them, they will be evicted, or they will be part of a more expensive package and something else will be evicted.)
  • The last commit is optional and introduces ability to set -minrelaytxfee to 0. This isn't necessary, but seemed better to me especially now other ways to accept transactions without fee have been removed.

Fixes #9601
Fixes #6675
Fixes #2673 (thanks @MarcoFalke)

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 20, 2017

Member

Concept ACK for removing it from the wallet. This will cause less headache when reviewing/reworking coin selection.

Member

MarcoFalke commented Jan 20, 2017

Concept ACK for removing it from the wallet. This will cause less headache when reviewing/reworking coin selection.

@MarcoFalke MarcoFalke added this to the 0.15.0 milestone Jan 20, 2017

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 23, 2017

Member

Needs rebase.

Member

MarcoFalke commented Jan 23, 2017

Needs rebase.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 24, 2017

Contributor

concept ACK, light utACK

Contributor

dcousens commented Jan 24, 2017

concept ACK, light utACK

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jan 27, 2017

Member

concept ACK.

Member

btcdrak commented Jan 27, 2017

concept ACK.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Feb 1, 2017

Member

Concept ACK. Needs rebase.

Member

jtimon commented Feb 1, 2017

Concept ACK. Needs rebase.

MarcoFalke and others added some commits Dec 20, 2016

wallet: Remove sendfree
This removes the option from the wallet to not pay a fee on "small"
transactions which spend "old" inputs.

This code is no longer worth keeping around, as almost all miners
prefer not to include transactions which pay no fee at all.
[rpc] Remove estimatepriority and estimatesmartpriority.
The RPC calls were already deprecated.
[mining] Remove -blockprioritysize.
Remove ability of mining code to fill part of a block with transactions sorted by coin age.
[debug] Change -printpriority option
-printpriority output is now changed to only show the fee rate and hash of transactions included in a block by the mining code.
[cleanup] Remove estimatePriority and estimateSmartPriority
Unused everywhere now except one test.
[rpc] sendrawtransaction no longer bypasses minRelayTxFee
The prioritisetransaction API can always be used if a transaction needs to be submitted that bypasses minRelayTxFee.
@sdaftuar

Nice! Code review ACK, will test.

Left a few non-blocking nits for you to consider; I think they would be fine to address/discuss in future PRs if you preferred.

@@ -456,7 +456,7 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-printtoconsole", _("Send trace/debug info to console instead of debug.log file"));
if (showDebug)
{
strUsage += HelpMessageOpt("-printpriority", strprintf("Log transaction priority and fee per kB when mining blocks (default: %u)", DEFAULT_PRINTPRIORITY));
strUsage += HelpMessageOpt("-printpriority", strprintf("Log transaction fee per kB when mining blocks (default: %u)", DEFAULT_PRINTPRIORITY));

This comment has been minimized.

@sdaftuar

sdaftuar Feb 28, 2017

Member

I'm fine with backwards compatibility for a while but can we start to move to a better option name for this feature, and log that the old name is deprecated?

@sdaftuar

sdaftuar Feb 28, 2017

Member

I'm fine with backwards compatibility for a while but can we start to move to a better option name for this feature, and log that the old name is deprecated?

This comment has been minimized.

@jnewbery

jnewbery Mar 3, 2017

Member

Agree with @sdaftuar . Commit at jnewbery@b83daf9 adds -printfee option. Feel free to pull into this PR if you want it, or I'll open another PR after this gets merged.

@jnewbery

jnewbery Mar 3, 2017

Member

Agree with @sdaftuar . Commit at jnewbery@b83daf9 adds -printfee option. Feel free to pull into this PR if you want it, or I'll open another PR after this gets merged.

This comment has been minimized.

@morcos

morcos Mar 3, 2017

Member

I always interpreted this as print the priority as in the sort order for mining. I have no problem changing it, but I'll leave that for another PR if people want it.

@morcos

morcos Mar 3, 2017

Member

I always interpreted this as print the priority as in the sort order for mining. I have no problem changing it, but I'll leave that for another PR if people want it.

mempool.ApplyDeltas(iter->GetTx().GetHash(), dPriority, dummy);
LogPrintf("priority %.1f fee %s txid %s\n",
dPriority,
LogPrintf("fee %s txid %s\n",

This comment has been minimized.

@sdaftuar

sdaftuar Feb 28, 2017

Member

nit: if we're changing this anyway how about we switch "fee" -> "feerate"?

@sdaftuar

sdaftuar Feb 28, 2017

Member

nit: if we're changing this anyway how about we switch "fee" -> "feerate"?

This comment has been minimized.

@jnewbery

jnewbery Mar 3, 2017

Member

meh. It's pretty obvious that this is feerate from context:

2017-03-03 14:19:33 fee 0.00005913 BTC/kB txid 3bbae9e9e8299f6335b5e99912cb6717df794212477a92fb425c2b5993d3bdc7
2017-03-03 14:19:33 fee 0.00004437 BTC/kB txid a3c976f58b52b8ab29eda3c434cb21d1d3ba1d30136b329a51ba4a3f493e6adf
@jnewbery

jnewbery Mar 3, 2017

Member

meh. It's pretty obvious that this is feerate from context:

2017-03-03 14:19:33 fee 0.00005913 BTC/kB txid 3bbae9e9e8299f6335b5e99912cb6717df794212477a92fb425c2b5993d3bdc7
2017-03-03 14:19:33 fee 0.00004437 BTC/kB txid a3c976f58b52b8ab29eda3c434cb21d1d3ba1d30136b329a51ba4a3f493e6adf

This comment has been minimized.

@morcos

morcos Mar 3, 2017

Member

changed anyway

@morcos

morcos Mar 3, 2017

Member

changed anyway

Show outdated Hide outdated src/init.cpp Outdated
@ryanofsky

utACK a1ac40b

  • Changes make sense, though I think this might been have been easier to review if it consisted of a just few minimal commits that update the behavior, followed by a single larger commit that removes dead code without changing behavior.
  • I didn't understand what you mean about potentially bypassing the mempool min fee in the event of mempool limiting. Could you say more about how this would work, what would be the advantages / disadvantages? Also, in the comment that "fLimitFree is now only set for transactions we are trying to reaccept to the mempool," assuming you meant "set false" not "set".
  • Re: prioritisetransaction, one risk of reducing the number of arguments is that at some point in the future, somebody adds a new argument, and then old code calling the RPC with priority may appear to work but silently be broken. You could keep the priority_delta arg and require it to be null or 0 to prevent this. But I think getting rid of the argument is nice if you don't think it's likely someone will add a new prioritisetransaction argument in the future.
Show outdated Hide outdated src/wallet/wallet.cpp Outdated
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar
Member

sdaftuar commented Mar 2, 2017

ACK a1ac40b

@TheBlueMatt

You may also wish to update the comment above paytxfee in contrib/debian/examples/bitcoin.conf

Show outdated Hide outdated src/wallet/wallet.cpp Outdated
@@ -1853,7 +1853,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
LogPrint("mempool", " invalid orphan tx %s\n", orphanHash.ToString());
}
// Has inputs but not accepted to mempool
// Probably non-standard or insufficient fee/priority
// Probably non-standard or insufficient fee

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 2, 2017

Contributor

Commit message here is wrong...

it mentions that you dont enforce minRelayTxFee on sendrawtransaciton but the previous commit removed that.

@TheBlueMatt

TheBlueMatt Mar 2, 2017

Contributor

Commit message here is wrong...

it mentions that you dont enforce minRelayTxFee on sendrawtransaciton but the previous commit removed that.

This comment has been minimized.

@morcos

morcos Mar 3, 2017

Member

will fix

@morcos

morcos Mar 3, 2017

Member

will fix

Show outdated Hide outdated src/txmempool.cpp Outdated
@@ -977,12 +977,12 @@ bool AppInitParameterInteraction()
// If you are mining, be careful setting this:
// if you set it to zero then
// a transaction spammer can cheaply fill blocks using
// 1-satoshi-fee transactions. It should be set above the real
// 0-fee transactions. It should be set above the real

This comment has been minimized.

@jnewbery

jnewbery Mar 3, 2017

Member

Consider also changing the first sentence in this comment: Fee-per-kilobyte amount considered the same as "free" doesn't make much sense now that we don't relay free transactions by default.

@jnewbery

jnewbery Mar 3, 2017

Member

Consider also changing the first sentence in this comment: Fee-per-kilobyte amount considered the same as "free" doesn't make much sense now that we don't relay free transactions by default.

This comment has been minimized.

@morcos

morcos Mar 3, 2017

Member

ok

@morcos

morcos Mar 3, 2017

Member

ok

This comment has been minimized.

@jnewbery

jnewbery Mar 3, 2017

Member

Sorry, on second thoughts, perhaps change the comment entirely. If you are mining, be careful setting this: if you set it to zero then a transaction spammer can cheaply fill blocks using 0-fee transactions. is not really true, since CNB will fill blocks based on feerate. I think the risk we're trying to mitigate with -minrelaytxfee is spammers filling nodes' mempools with cheap transactions. Is that right? If so, can we update the comment?

@jnewbery

jnewbery Mar 3, 2017

Member

Sorry, on second thoughts, perhaps change the comment entirely. If you are mining, be careful setting this: if you set it to zero then a transaction spammer can cheaply fill blocks using 0-fee transactions. is not really true, since CNB will fill blocks based on feerate. I think the risk we're trying to mitigate with -minrelaytxfee is spammers filling nodes' mempools with cheap transactions. Is that right? If so, can we update the comment?

Show outdated Hide outdated src/txmempool.cpp Outdated
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 3, 2017

Member

re-ACK 7445fe9

Member

sdaftuar commented Mar 3, 2017

re-ACK 7445fe9

@@ -891,7 +891,7 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
const uint256& hashTx = tx->GetHash();
bool fLimitFree = false;
bool fLimitFree = true;

This comment has been minimized.

@jnewbery

jnewbery Mar 3, 2017

Member

Consider removing this local variable and just calling AcceptToMemoryPool() with true (or at least moving this variable declaration down to the only place that it's used).

Also consider changing the local variable name nLimitFree in AcceptToMemoryPool(), AcceptToMemoryWithTime() and AcceptToMemoryPoolWorker() to fCheckFee or similar.

@jnewbery

jnewbery Mar 3, 2017

Member

Consider removing this local variable and just calling AcceptToMemoryPool() with true (or at least moving this variable declaration down to the only place that it's used).

Also consider changing the local variable name nLimitFree in AcceptToMemoryPool(), AcceptToMemoryWithTime() and AcceptToMemoryPoolWorker() to fCheckFee or similar.

@@ -977,12 +977,12 @@ bool AppInitParameterInteraction()
// If you are mining, be careful setting this:
// if you set it to zero then
// a transaction spammer can cheaply fill blocks using
// 1-satoshi-fee transactions. It should be set above the real
// 0-fee transactions. It should be set above the real

This comment has been minimized.

@jnewbery

jnewbery Mar 3, 2017

Member

Sorry, on second thoughts, perhaps change the comment entirely. If you are mining, be careful setting this: if you set it to zero then a transaction spammer can cheaply fill blocks using 0-fee transactions. is not really true, since CNB will fill blocks based on feerate. I think the risk we're trying to mitigate with -minrelaytxfee is spammers filling nodes' mempools with cheap transactions. Is that right? If so, can we update the comment?

@jnewbery

jnewbery Mar 3, 2017

Member

Sorry, on second thoughts, perhaps change the comment entirely. If you are mining, be careful setting this: if you set it to zero then a transaction spammer can cheaply fill blocks using 0-fee transactions. is not really true, since CNB will fill blocks based on feerate. I think the risk we're trying to mitigate with -minrelaytxfee is spammers filling nodes' mempools with cheap transactions. Is that right? If so, can we update the comment?

@@ -258,31 +258,28 @@ UniValue getmininginfo(const JSONRPCRequest& request)
// NOTE: Unlike wallet RPC (which use BTC values), mining RPCs follow GBT (BIP 22) in using satoshi amounts
UniValue prioritisetransaction(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() != 3)

This comment has been minimized.

@jnewbery

jnewbery Mar 3, 2017

Member

I really don't like backwards-incompatible changes to RPCs like this. It breaks prioritisetransaction for all clients.

An alternative to this is to change argument 2 to be a dummy argument that has to be set to '0.0'. That means that miners who were using prioritisetransaction to bump the apparent fee on transactions can continue to do so without having to make changes to their client software.

@jnewbery

jnewbery Mar 3, 2017

Member

I really don't like backwards-incompatible changes to RPCs like this. It breaks prioritisetransaction for all clients.

An alternative to this is to change argument 2 to be a dummy argument that has to be set to '0.0'. That means that miners who were using prioritisetransaction to bump the apparent fee on transactions can continue to do so without having to make changes to their client software.

This comment has been minimized.

@dcousens

dcousens Mar 7, 2017

Contributor

Agreed, my only concern is this breaking change to the RPC API. It will hurt someone.
Can we deprecate it and introduce an alternative method?

@dcousens

dcousens Mar 7, 2017

Contributor

Agreed, my only concern is this breaking change to the RPC API. It will hurt someone.
Can we deprecate it and introduce an alternative method?

@@ -150,7 +151,7 @@ def __init__(self):
def setup_network(self):

This comment has been minimized.

@jnewbery

jnewbery Mar 3, 2017

Member

Please update the comment further down 'Node1 mines small blocks but that are bigger than the expected transaction rate and allows free transactions.`

@jnewbery

jnewbery Mar 3, 2017

Member

Please update the comment further down 'Node1 mines small blocks but that are bigger than the expected transaction rate and allows free transactions.`

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Mar 3, 2017

Member

I don't like the fact that prioritisetransaction is not back-compatible. Other than that, looks good. A few nits inline.

Member

jnewbery commented Mar 3, 2017

I don't like the fact that prioritisetransaction is not back-compatible. Other than that, looks good. A few nits inline.

morcos added some commits Jan 19, 2017

[rpc] Remove priority information from mempool RPC calls
"startingpriority" and "currentpriority" are no longer returned in the JSON information about a mempool entry.  This affects getmempoolancestors, getmempooldescendants, getmempooolentry, and getrawmempool.
No longer allow "free" transactions
Remove -limitfreerelay and always enforce minRelayTxFee in the mempool (except from disconnected blocks)

Remove -relaypriority, the option was only used for the ability to allow free transactions to be relayed regardless of their priority.  Both notions no longer apply.
[test] Remove priority from tests
Remove all coin age priority functionality from unit tests and RPC tests.
[rpc] Remove priorityDelta from prioritisetransaction
This a breaking API change to the prioritisetransaction RPC call which previously required exactly three arguments and now requires exactly two (hash and feeDelta).  The function prioritiseTransaction is also updated.
Allow setting minrelaytxfee to 0
Setting minrelaytxfee to 0 will allow all transactions regardless of fee to enter your mempool until it reaches its size limit.  However now that mempool limiting is governed by a separate incrementalrelay fee, it is an unnecessary restriction to prevent a minrelaytxfee of 0.
[cleanup] Remove coin age priority completely.
Remove GetPriority and ComputePriority.  Remove internal machinery for tracking priority in CTxMemPoolEntry.
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Mar 3, 2017

Member

Nits addressed

Squashed 3b2b549 (nopriority-tag2) -> b421e6d

@jnewbery I didn't want to get into a whole big deal about what the current purpose of minrelaytxfee is so I left the comment minimally changed for now.

As for the backwards compatibility of prioritisetransaction, that was one of my open questions from the top of the PR. I'll defer to group consensus, but my viewpoint is if we never change it then we're stuck with a stupid dummy field for all eternity, and if we are ever going to change it, now is the time to do so. I think its a pretty safe change as it'll either work the old way or the new but they can't be mixed up.

Member

morcos commented Mar 3, 2017

Nits addressed

Squashed 3b2b549 (nopriority-tag2) -> b421e6d

@jnewbery I didn't want to get into a whole big deal about what the current purpose of minrelaytxfee is so I left the comment minimally changed for now.

As for the backwards compatibility of prioritisetransaction, that was one of my open questions from the top of the PR. I'll defer to group consensus, but my viewpoint is if we never change it then we're stuck with a stupid dummy field for all eternity, and if we are ever going to change it, now is the time to do so. I think its a pretty safe change as it'll either work the old way or the new but they can't be mixed up.

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Mar 3, 2017

Member

As for the backwards compatibility of prioritisetransaction...

My preference would be to maintain backwards compatibility, but I'll go along with group consensus and don't think this should delay merging this PR.

utACK.

Member

jnewbery commented Mar 3, 2017

As for the backwards compatibility of prioritisetransaction...

My preference would be to maintain backwards compatibility, but I'll go along with group consensus and don't think this should delay merging this PR.

utACK.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 6, 2017

Member

re-ACK b421e6d

Member

sdaftuar commented Mar 6, 2017

re-ACK b421e6d

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Mar 6, 2017

Contributor

utACK b421e6d

Contributor

TheBlueMatt commented Mar 6, 2017

utACK b421e6d

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 6, 2017

Member
Member

MarcoFalke commented Mar 6, 2017

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Mar 6, 2017

Member

Note: this post will be updated as I progress in my review.
fast review ACK: ddf58c7
utACK: 12839cd fe282ac

Member

jtimon commented Mar 6, 2017

Note: this post will be updated as I progress in my review.
fast review ACK: ddf58c7
utACK: 12839cd fe282ac

@dcousens

utACK b421e6d

@laanwj laanwj merged commit b421e6d into bitcoin:master Mar 7, 2017

1 check passed

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

laanwj added a commit that referenced this pull request Mar 7, 2017

Merge #9602: Remove coin age priority and free transactions - impleme…
…ntation

b421e6d Update example bitcoin.conf (Alex Morcos)
7d4e950 Allow setting minrelaytxfee to 0 (Alex Morcos)
359e8a0 [cleanup] Remove coin age priority completely. (Alex Morcos)
f9b9371 [rpc] Remove priorityDelta from prioritisetransaction (Alex Morcos)
49be7e1 [rpc] Remove priority information from mempool RPC calls (Alex Morcos)
0315888 [test] Remove priority from tests (Alex Morcos)
f838005 No longer allow "free" transactions (Alex Morcos)
ad727f4 [rpc] sendrawtransaction no longer bypasses minRelayTxFee (Alex Morcos)
fe282ac [cleanup] Remove estimatePriority and estimateSmartPriority (Alex Morcos)
400b151 [debug] Change -printpriority option (Alex Morcos)
272b25a [mining] Remove -blockprioritysize. (Alex Morcos)
12839cd [rpc] Remove estimatepriority and estimatesmartpriority. (Alex Morcos)
ddf58c7 wallet: Remove sendfree (MarcoFalke)

Tree-SHA512: a9a4499405923ce794ef18f9e334dbbd59dfc73a3dc2df6f85cc9c62af6f353ec2eed9c2d5e58e904f918d0d7ab738f403dd4939d9bc2276136864fe63710782

@MarcoFalke MarcoFalke referenced this pull request Mar 19, 2017

Closed

TODO for release notes 0.15.0 #9889

12 of 12 tasks complete

MarcoFalke added a commit that referenced this pull request Sep 29, 2017

Merge #11309: Minor cleanups for AcceptToMemoryPool
bf64c3c Ignore transactions added to mempool during a reorg for fee estimation purposes. (Alex Morcos)
04f78ab Do not reject based on mempool min fee when bypass_limits is set. (Alex Morcos)
fd849e1 Change AcceptToMemoryPool function signature (Alex Morcos)

Pull request description:

  First commit just removes default arguments from `AcceptToMemoryPool` and consolidates two arguments, it does not change behavior.

  Second commit finally fixes the fact that we're not meant to reject based on mempool min fee when adding a transaction from a disconnected block during a reorg as mentioned [here](#9602 (comment))

  Third commit makes fee estimation ignore transactions added from a disconnected block during a reorg. I think this was another source of fee estimates returning estimates below 1000 sat/kB as in #11303.

Tree-SHA512: 30925ca8b341915bb214f1d2590b36b7931f2e125b7660150e38ae70338f00db5aa7f1608546dddb181446924177eb7cf62ea8bd2583068acc074d6c3f86bc0c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment