Remove coin age priority and free transactions

morcos commented at 5:30 pm on January 20, 2017: member

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 added the label Wallet on Jan 20, 2017

MarcoFalke added the label Mining on Jan 20, 2017

MarcoFalke added the label TX fees and policy on Jan 20, 2017

MarcoFalke commented at 6:14 pm on January 20, 2017: member

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

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

MarcoFalke commented at 12:40 pm on January 23, 2017: member

Needs rebase.

dcousens approved

dcousens commented at 1:56 am on January 24, 2017: contributor

concept ACK, light utACK

btcdrak commented at 7:23 am on January 27, 2017: contributor

concept ACK.

jtimon commented at 2:12 am on February 1, 2017: contributor

Concept ACK. Needs rebase.

morcos force-pushed on Feb 14, 2017

morcos force-pushed on Feb 22, 2017

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.

ddf58c7573

[rpc] Remove estimatepriority and estimatesmartpriority.

The RPC calls were already deprecated.

12839cdd56

[mining] Remove -blockprioritysize.

Remove ability of mining code to fill part of a block with transactions sorted by coin age.

272b25a6a9

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

400b15147c

[cleanup] Remove estimatePriority and estimateSmartPriority

Unused everywhere now except one test.

fe282acd76

[rpc] sendrawtransaction no longer bypasses minRelayTxFee

The prioritisetransaction API can always be used if a transaction needs to be submitted that bypasses minRelayTxFee.

ad727f4eaf

morcos force-pushed on Feb 27, 2017

in src/wallet/wallet.cpp: in 773f798d3e outdated

3841@@ -3842,9 +3842,6 @@ bool CWallet::ParameterInteraction()
3842     bSpendZeroConfChange = GetBoolArg("-spendzeroconfchange", DEFAULT_SPEND_ZEROCONF_CHANGE);
3843     fWalletRbf = GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF);
3844 
3845-    if (GetBoolArg("-sendfreetransactions", false))
3846-        InitWarning("The argument -sendfreetransactions is no longer supported.");

ryanofsky commented at 3:24 pm on February 28, 2017:

Instead first adding this check in ddf58c75739efc7509d529002975ea447a269b3a “wallet: Remove sendfree”, and then removing it here in 773f798d3e4cc032fce28138aede75ab6e2d6751 “No longer allow “free” transactions”, better to not add it in the first place.

in src/init.cpp: in 400b15147c outdated

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

sdaftuar commented at 3:58 pm on February 28, 2017:

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?

jnewbery commented at 2:22 pm on March 3, 2017:

Agree with @sdaftuar . Commit at https://github.com/jnewbery/bitcoin/commit/b83daf977f38d98a3b3aa9ed5857bfd2f19b2a09 adds -printfee option. Feel free to pull into this PR if you want it, or I’ll open another PR after this gets merged.

morcos commented at 3:14 pm on March 3, 2017:

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.

in src/miner.cpp: in 400b15147c outdated

262@@ -263,11 +263,7 @@ void BlockAssembler::AddToBlock(CTxMemPool::txiter iter)
263 
264     bool fPrintPriority = GetBoolArg("-printpriority", DEFAULT_PRINTPRIORITY);
265     if (fPrintPriority) {
266-        double dPriority = iter->GetPriority(nHeight);
267-        CAmount dummy;
268-        mempool.ApplyDeltas(iter->GetTx().GetHash(), dPriority, dummy);
269-        LogPrintf("priority %.1f fee %s txid %s\n",
270-                  dPriority,
271+        LogPrintf("fee %s txid %s\n",

sdaftuar commented at 3:58 pm on February 28, 2017:

nit: if we’re changing this anyway how about we switch “fee” -> “feerate”?

jnewbery commented at 3:18 pm on March 3, 2017:

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

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

morcos commented at 3:33 pm on March 3, 2017:

changed anyway

in src/init.cpp: in a1ac40b499 outdated

982     // cost to you of processing a transaction.
983     if (IsArgSet("-minrelaytxfee"))
984     {
985         CAmount n = 0;
986-        if (!ParseMoney(GetArg("-minrelaytxfee", ""), n) || 0 == n)
987+        if (!ParseMoney(GetArg("-minrelaytxfee", ""), n))

sdaftuar commented at 4:05 pm on February 28, 2017:

nit: add the curly braces for this if while you’re at it?

morcos commented at 3:33 pm on March 3, 2017:

ok

sdaftuar commented at 4:28 pm on February 28, 2017: member

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.

ryanofsky commented at 4:42 pm on February 28, 2017: member

utACK a1ac40b49917638ebfa9d65a773a739519ea03da

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.

in src/wallet/wallet.cpp: in ddf58c7573 outdated

3844     fWalletRbf = GetBoolArg("-walletrbf", DEFAULT_WALLET_RBF);
3845 
3846-    if (fSendFreeTransactions && GetArg("-limitfreerelay", DEFAULT_LIMITFREERELAY) <= 0)
3847-        return InitError("Creation of free transactions with their relay disabled is not supported.");
3848+    if (GetBoolArg("-sendfreetransactions", false))
3849+        InitWarning("The argument -sendfreetransactions is no longer supported.");

TheBlueMatt commented at 4:29 pm on March 2, 2017:

I believe this should be translated.

morcos commented at 3:17 pm on March 3, 2017:

This was added in #9391 whose commit was included here, but then removed by the end of this PR. Would have been cleaner to take it out of the first commit, but I’m going to aim for less history churn here, it doesn’t hurt anything…

in src/net_processing.cpp: in 773f798d3e outdated

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

TheBlueMatt commented at 8:20 pm on March 2, 2017:

Commit message here is wrong…

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

morcos commented at 3:32 pm on March 3, 2017:

will fix

in src/txmempool.cpp: in 6c7a7693dc outdated

928@@ -930,18 +929,17 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, double dPriorityDelt
929             }
930         }
931     }
932-    LogPrintf("PrioritiseTransaction: %s priority += %f, fee += %d\n", hash.ToString(), dPriorityDelta, FormatMoney(nFeeDelta));
933+    LogPrintf("PrioritiseTransaction: %s fee += %d\n", hash.ToString(), FormatMoney(nFeeDelta));

TheBlueMatt commented at 8:35 pm on March 2, 2017:

FormatMoney returns std::string, this should be %s %s.

morcos commented at 3:32 pm on March 3, 2017:

ok

sdaftuar commented at 9:52 pm on March 2, 2017: member

ACK a1ac40b49917638ebfa9d65a773a739519ea03da

TheBlueMatt commented at 9:55 pm on March 2, 2017: member

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

in contrib/debian/examples/bitcoin.conf: in ddf58c7573 outdated

117@@ -118,9 +118,6 @@
118 
119 # Transaction Fee Changes in 0.10.0
120

jnewbery commented at 10:19 pm on March 2, 2017:

I’d also lose the ‘Transaction Fee Changes in 0.10.0’ in the line above

morcos commented at 3:49 pm on March 3, 2017:

ok

in src/init.cpp: in a1ac40b499 outdated

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

jnewbery commented at 3:37 pm on March 3, 2017:

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.

morcos commented at 3:49 pm on March 3, 2017:

ok

jnewbery commented at 4:55 pm on March 3, 2017:

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?

in src/rpc/rawtransaction.cpp: in ad727f4eaf outdated

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

jnewbery commented at 4:43 pm on March 3, 2017:

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.

in src/rpc/mining.cpp: in 6c7a7693dc outdated

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

jnewbery commented at 5:38 pm on March 3, 2017:

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.

dcousens commented at 1:24 am on March 7, 2017:

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?

in qa/rpc-tests/smartfees.py: in 89b2681dc6 outdated

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

jnewbery commented at 5:59 pm on March 3, 2017:

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

in src/txmempool.cpp: in be0adbb50e outdated

913@@ -914,7 +914,7 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD
914             }
915         }
916     }
917-    LogPrintf("PrioritiseTransaction: %s fee += %d\n", hash.ToString(), FormatMoney(nFeeDelta));
918+    LogPrintf("PrioritiseTransaction: %s feerate += %s\n", hash.ToString(), FormatMoney(nFeeDelta));

sdaftuar commented at 7:15 pm on March 3, 2017:

Good catch!

sdaftuar commented at 7:18 pm on March 3, 2017: member

re-ACK 7445fe9

jnewbery commented at 7:42 pm on March 3, 2017: member

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

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.

f838005444

[test] Remove priority from tests

Remove all coin age priority functionality from unit tests and RPC tests.

0315888d0d

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

49be7e1bef

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

f9b9371c60

[cleanup] Remove coin age priority completely.

Remove GetPriority and ComputePriority. Remove internal machinery for tracking priority in CTxMemPoolEntry.

359e8a03d1

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.

7d4e9509ad

Update example bitcoin.conf b421e6ddcf

morcos force-pushed on Mar 3, 2017

morcos commented at 10:00 pm on March 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.

jnewbery commented at 11:00 pm on March 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.

sdaftuar commented at 3:14 pm on March 6, 2017: member

re-ACK b421e6ddcf00f220732f43742393452bb8bf4cdd

TheBlueMatt commented at 4:16 pm on March 6, 2017: member

utACK b421e6ddcf00f220732f43742393452bb8bf4cdd

MarcoFalke commented at 4:52 pm on March 6, 2017: member

utACK b421e6d

(only reviewed tests and wallet, did not look closely at miner and mempool changes.)

Some notes about doc fixes for later commits:

The change in check-doc.py seems no longer relevant
The rpc breakage should be mentioned in the release notes
The misleading comment in init.cpp about minrelaytxfee can be removed alltogether. Better no doc than wrong doc and inline comments are the wrong place for user documentation anyway.

On Mon, Mar 6, 2017 at 5:17 PM, Matt Corallo notifications@github.com wrote:

utACK b421e6d

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jtimon commented at 10:48 pm on March 6, 2017: contributor

Note: this post will be updated as I progress in my review. fast review ACK: https://github.com/bitcoin/bitcoin/pull/9602/commits/ddf58c75739efc7509d529002975ea447a269b3a utACK: https://github.com/bitcoin/bitcoin/pull/9602/commits/12839cdd56ab5ebb06c59bbf9a064f4bd23b7398 https://github.com/bitcoin/bitcoin/pull/9602/commits/fe282acd7604b5265762b24e531bdf1ebb1f009b

dcousens approved

dcousens commented at 1:25 am on March 7, 2017: contributor

utACK b421e6d

laanwj merged this on Mar 7, 2017

laanwj closed this on Mar 7, 2017

laanwj referenced this in commit 30ff3a2fc9 on Mar 7, 2017

MarcoFalke referenced this in commit 93d20a734d on Sep 29, 2017

PastaPastaPasta referenced this in commit 8ba4b5c903 on Dec 30, 2018

PastaPastaPasta referenced this in commit e9d4f73154 on Dec 30, 2018

PastaPastaPasta referenced this in commit 223f4a9199 on Dec 30, 2018

PastaPastaPasta referenced this in commit a6013c08c8 on Dec 30, 2018

PastaPastaPasta referenced this in commit 522db613b6 on Dec 30, 2018

PastaPastaPasta referenced this in commit 8ffed6a59b on Dec 30, 2018

PastaPastaPasta referenced this in commit c818bf4ed6 on Dec 30, 2018

PastaPastaPasta referenced this in commit 2a621d0af1 on Dec 31, 2018

PastaPastaPasta referenced this in commit 07b11015d5 on Dec 31, 2018

PastaPastaPasta referenced this in commit 957b744a54 on Dec 31, 2018

PastaPastaPasta referenced this in commit f2582e4692 on Dec 31, 2018

PastaPastaPasta referenced this in commit 9b99599991 on Dec 31, 2018

PastaPastaPasta referenced this in commit 7c226c9148 on Dec 31, 2018

PastaPastaPasta referenced this in commit 63427686b6 on Dec 31, 2018

PastaPastaPasta referenced this in commit c4ca82e80a on Dec 31, 2018

PastaPastaPasta referenced this in commit 363506f27c on Jan 2, 2019

PastaPastaPasta referenced this in commit 1b3a50bc3d on Jan 2, 2019

PastaPastaPasta referenced this in commit 0e9b1429fe on Jan 2, 2019

PastaPastaPasta referenced this in commit c697dfad60 on Jan 2, 2019

PastaPastaPasta referenced this in commit 30901f8a61 on Jan 3, 2019

PastaPastaPasta referenced this in commit e86f222955 on Jan 3, 2019

PastaPastaPasta referenced this in commit 3b94dd3792 on Jan 21, 2019

PastaPastaPasta referenced this in commit c0e2c0b2d3 on Jan 21, 2019

PastaPastaPasta referenced this in commit 4564885f32 on Jan 27, 2019

PastaPastaPasta referenced this in commit 08ba055a5d on Jan 27, 2019

PastaPastaPasta referenced this in commit 830d6538cb on Jan 29, 2019

PastaPastaPasta referenced this in commit 8180f2f3b4 on Jan 29, 2019

PastaPastaPasta referenced this in commit 85e5d994cd on Feb 5, 2019

PastaPastaPasta referenced this in commit 9461663cf9 on Mar 13, 2019

PastaPastaPasta referenced this in commit 2da06e256c on Mar 13, 2019

PastaPastaPasta referenced this in commit e83bad1bbe on Mar 13, 2019

UdjinM6 referenced this in commit 6f90cf7a17 on Mar 14, 2019

PastaPastaPasta referenced this in commit 2208b6320b on Mar 14, 2020

PastaPastaPasta referenced this in commit df3e0e6ebc on Mar 15, 2020

PastaPastaPasta referenced this in commit 0aa6508e31 on Mar 16, 2020

PastaPastaPasta referenced this in commit 75f32fb50f on Mar 16, 2020

ckti referenced this in commit 9bb144fee3 on Mar 28, 2021

gades referenced this in commit 1733872eda on Jun 30, 2021

furszy referenced this in commit b554c29785 on Jul 14, 2021

MarcoFalke locked this on Sep 8, 2021

Remove coin age priority and free transactions - implementation #9602