Remove coin age priority and free transactions

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

I’ve opened #9602 to remove all coin age priority functionality from the codebase.

I think this has been discussed many times and hopefully it will not be the subject of another lengthy discussion, but just in case, I’ve opened this issue so any discussion not related to the technical merits of the implementation can happen here and the PR itself can remain free for discussion of the actual code.

This is targeted for 0.15, and shouldn’t be merged until we ship 0.14, but it came up again recently and so I figured I might as well post the PR. Also in the event that we feel anything needs to be pre-announced with the 0.14 release this will give us a chance to think about that, although I don’t think there is anything pressing.

MarcoFalke added the label Brainstorming on Jan 20, 2017

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

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

Looks like this is a duplicate of #6675

MarcoFalke added the label Wallet on Jan 20, 2017

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

MarcoFalke added the label Mining on Jan 20, 2017

MarcoFalke added the label Mempool on Jan 20, 2017

ghost commented at 3:43 pm on January 25, 2017: none

I am against It works as an anti-spam addresses

luke-jr commented at 3:53 am on February 14, 2017: member

Concept NACK to removal from mining:

Miner(s) still use priority. Whether or not you agree with this, policy is not up to developers to decide and force on miners.
Coin age is a useful algorithm for altruistic miners who wish to keep the network functioning smoothly even during flood attacks.
Coin age ensures that so long as a non-spam transaction pays a reasonable fee, it will eventually get mined. (RBF may eventually make this assurance unnecessary, but we’re not there yet.)
Using a single algorithm for block creation makes it significantly easier for floods to game the algorithm to DoS the entire network. Merely the existence of multiple different sorting algorithms for blocks helps avoid the situation where blocks are pure flood and legitimate transactions can’t get through.
Regardless of past complications, the current code is cleanly isolated from the rest of the code, and a mere 100 lines. If it creates a burden for future modifications, removing priority can be put back on the table and weighed against the value of those modifications (or ported by someone interested in keeping it; eg, me).
Multiple algorithms for mining (regardless of what those algorithms may be) also avoids the situation where policy logic gets too deeply embedded into non-policy code, creating huge risks and burdens for anyone who wishes to customise it without modifying any non-policy code.

I can alternatively maintain coin-age priority in Knots only, but I do not think Core should head in this direction of pushing a single specific political agenda at the exclusion of all others. If it does go that route, it should be made clear that Core is no longer a reference implementation, and its political agenda documented.

Concept ACK to everything else (eg, removal of support for zero-fee transactions, and priority estimation)

jnewbery commented at 8:02 pm on February 17, 2017: member

@luke-jr you have your opinions on priority, which I completely respect. However, this:

Regardless of past complications, the current code is cleanly isolated from the rest of the code, and a mere 100 lines.

is simply not true. #9602 is the WIP PR for removing the priority code, and it already has >100 insertions and >800 deletions:

 0→ git diff --stat master..HEAD
 1 contrib/debian/examples/bitcoin.conf   |   3 ---
 2 contrib/devtools/check-doc.py          |   2 +-
 3 qa/rpc-tests/abandonconflict.py        |   1 -
 4 qa/rpc-tests/bip68-sequence.py         |   8 ++++----
 5 qa/rpc-tests/importmulti.py            |  33 +--------------------------------
 6 qa/rpc-tests/mempool_packages.py       |   4 ++--
 7 qa/rpc-tests/nulldummy.py              |   4 ++--
 8 qa/rpc-tests/prioritise_transaction.py |  34 +++++++++++-----------------------
 9 qa/rpc-tests/replace-by-fee.py         |   4 ++--
10 qa/rpc-tests/smartfees.py              |  13 +++++++------
11 qa/rpc-tests/test_framework/util.py    |  41 -----------------------------------------
12 src/addrman.cpp                        |   6 +++++-
13 src/bench/mempool_eviction.cpp         |   5 ++---
14 src/coins.cpp                          |  19 -------------------
15 src/coins.h                            |   7 -------
16 src/init.cpp                           |   9 +++------
17 src/miner.cpp                          | 159 +++------------------------------------------------------------------------------------------------------------------------------------------------------------
18 src/miner.h                            |  12 ------------
19 src/net_processing.cpp                 |   7 +++----
20 src/policy/fees.cpp                    |  18 ------------------
21 src/policy/fees.h                      |  15 ---------------
22 src/policy/policy.h                    |   2 --
23 src/primitives/transaction.cpp         |  26 --------------------------
24 src/primitives/transaction.h           |   6 ------
25 src/qt/coincontroldialog.cpp           |  24 +++---------------------
26 src/qt/guiutil.cpp                     |   3 ---
27 src/qt/paymentrequestplus.h            |   3 ---
28 src/rpc/blockchain.cpp                 |   4 ----
29 src/rpc/client.cpp                     |   5 +----
30 src/rpc/mining.cpp                     |  84 ++++++++----------------------------------------------------------------------------
31 src/rpc/misc.cpp                       |   5 +----
32 src/rpc/net.cpp                        |   2 +-
33 src/rpc/rawtransaction.cpp             |   2 +-
34 src/test/mempool_tests.cpp             |  43 ++++++++++++++++++++-----------------------
35 src/test/miner_tests.cpp               |   4 +---
36 src/test/policyestimator_tests.cpp     |  10 ++++------
37 src/test/test_bitcoin.cpp              |  13 +++++--------
38 src/test/test_bitcoin.h                |   9 +++------
39 src/txmempool.cpp                      |  57 +++++++++++++++------------------------------------------
40 src/txmempool.h                        |  52 +++++-----------------------------------------------
41 src/validation.cpp                     |  44 +++++++++-----------------------------------
42 src/validation.h                       |   2 --
43 src/wallet/rpcdump.cpp                 |  33 ++++++++++++++++++---------------
44 src/wallet/wallet.cpp                  |  80 +++++++++++++++-----------------------------------------------------------------
45 src/wallet/wallet.h                    |  32 ++++++--------------------------
46 src/wallet/walletdb.cpp                |  43 +++++++++++++++++--------------------------
47 src/wallet/walletdb.h                  |   2 +-
48 47 files changed, 180 insertions(+), 814 deletions(-)

I do not think Core should head in this direction of pushing a single specific political agenda at the exclusion of all others. If it does go that route, it should be made clear that Core is no longer a reference implementation, and its political agenda documented.

No need for inflammatory language like this.

luke-jr commented at 8:07 pm on February 17, 2017: member

@jnewbery #9602 removes much more than mining-only priority sorting, which is what I was referring to there.

I do not think Core should head in this direction of pushing a single specific political agenda at the exclusion of all others. If it does go that route, it should be made clear that Core is no longer a reference implementation, and its political agenda documented.

No need for inflammatory language like this.

The change is what is inflammatory…

sipa commented at 0:25 am on February 18, 2017: member

Miner(s) still use priority. Whether or not you agree with this, policy is not up to developers to decide and force on miners.

I don’t believe that this happens in any significant measure.

Coin age is a useful algorithm for altruistic miners who wish to keep the network functioning smoothly even during flood attacks.

It’s a potentially useful metric when it is used by wallets. No wallet ever had a decent implementation of priority-maximizing behaviour. Because of this, miners who increase priority based area will just increase unpredictable behavior for wallets.

Coin age ensures that so long as a non-spam transaction pays a reasonable fee, it will eventually get mined. (RBF may eventually make this assurance unnecessary, but we’re not there yet.)

Only if the priority-based capacity exceeds the demand, and without lower bound on cost, I believe that demand can grow arbitrarily large.

Using a single algorithm for block creation makes it significantly easier for floods to game the algorithm to DoS the entire network. Merely the existence of multiple different sorting algorithms for blocks helps avoid the situation where blocks are pure flood and legitimate transactions can’t get through.

Perhaps, but multiple algorithms can easily be built on top (see below).

Regardless of past complications, the current code is cleanly isolated from the rest of the code, and a mere 100 lines. If it creates a burden for future modifications, removing priority can be put back on the table and weighed against the value of those modifications (or ported by someone interested in keeping it; eg, me).

See @jnewbery’s comment above.

Multiple algorithms for mining (regardless of what those algorithms may be) also avoids the situation where policy logic gets too deeply embedded into non-policy code, creating huge risks and burdens for anyone who wishes to customise it without modifying any non-policy code.

That “hardcoded” logic you’re referring to sorting based on a single ‘cost’ (called size) vs ‘benefit’ (called fee) ratio. Changing what those cost and benefit are is easy, and can be done without performance impact. Anything else can be done on top through prioritizetransaction.

I can alternatively maintain coin-age priority in Knots only, but I do not think Core should head in this direction of pushing a single specific political agenda at the exclusion of all others. If it does go that route, it should be made clear that Core is no longer a reference implementation, and its political agenda documented.

I don’t believe that the removal of priority is pushing anything. It’s simply recognizing the fact that coin age based mining is a historical artifact that - to the contributors of this project - seems unlikely to become relevant again.

luke-jr commented at 0:49 am on February 18, 2017: member

I don’t believe that this happens in any significant measure.

I don’t believe there is evidence to support this disbelief.

It’s a potentially useful metric when it is used by wallets. No wallet ever had a decent implementation of priority-maximizing behaviour.

Priority doesn’t require maximization. Even entirely random coin selection is likely to trigger a higher priority for legitimate usage than spam.

Only if the priority-based capacity exceeds the demand, and without lower bound on cost, I believe that demand can grow arbitrarily large.

In the scope of infinite time, capacity is also infinite so long as the per-block priority size is non-zero. Also, there is a lower bound on cost: the miner’s mempool size limit.

See @jnewbery’s comment above.

I already pointed out why that is incorrect.

Perhaps, but multiple algorithms can easily be built on top (see below). … Anything else can be done on top through prioritizetransaction.

That has never been demonstrated. I don’t believe it is true. It may be possible, but certainly not easy - it creates a lot of unnecessary overhead and complexity compared to a native implementation.

I don’t believe that the removal of priority is pushing anything.

How can it not be? There are no other grounds to object to leaving it in as an option so long as someone else (ie, myself) does the work to maintain it. Can you show me where any alternate policy option is being allowed into Core 0.15? (I can point to #9749 as a counter-example…)

It’s simply recognizing the fact that coin age based mining is a historical artifact that - to the contributors of this project - seems unlikely to become relevant again.

In other words, you don’t consider me a contributor of this project?

sipa commented at 0:59 am on February 18, 2017: member

How can it not be? There are no other grounds to object to leaving it in as an option so long as someone else (ie, myself) does the work to maintain it.

That assumes that it only requires one person to maintain, ignoring all coordination and review.

It’s simply recognizing the fact that coin age based mining is a historical artifact that - to the contributors of this project - seems unlikely to become relevant again. In other words, you don’t consider me a contributor of this project?

I didn’t say that every single individual contributor agrees. But if we want to get out of this, you’ll need to compromise and accept that your own opinion alone can’t dictate the project’s choices as a whole.

laanwj closed this on Mar 7, 2017

unknown referenced this in commit a9f2e085ee on Mar 10, 2017

lateminer referenced this in commit b93df64bb3 on May 24, 2017

pyritepirate referenced this in commit ce2cf38172 on Nov 21, 2018

MarcoFalke locked this on Sep 8, 2021

Remove coin age priority and free transactions - discussion #9601