Remove TX priority and free transaction area from mempool, block creator.

jgarzik commented at 6:01 am on July 9, 2015: contributor

It was noted on #bitcoin-dev that many simplifications arise from removing the complex, dual-policy aspects of priority-based low fee transactions on policy estimation, block template creation and mempool code. This change updates Bitcoin Core to relay purely based on fee/kb rate, removing the concepts of priority and free transactions from the codebase.

fSendFreeTransactions, fLimitFree and similar controls have been operating in the field defaulting to fee-based choices already.

This change should make other changes such as @pstratem ’s work to cap the mempool size dynamically much easier.

Notes:

Status: Watching travis. A large change, largely untested, so not ready for merging without lots of feedback and testing.
Thus, requesting feedback and testing…
Transaction fee deltas are preserved - thus TX “priority” remains to some extent
prioritisetransaction RPC call continues to work, by adjusting the fee.
needs focused review in the area of wallet, to ensure IsMine() transactions work fine through reorgs

miner: remove prio calculations and dPriority from TxPriority tuple a92df77011

mempool: remove priority from memory pool per-entry container 7ce77af67b

mempool, miner, qt, main: remove uses of priority, free transactions 711b2bfea5

luke-jr commented at 6:03 am on July 9, 2015: member

NACK. This makes a number of policies harder to implement for the sake of a “simpler” (and less competent) default policy. I’m working on a mempool size cap that doesn’t break policy freedom so completely.

jgarzik commented at 6:07 am on July 9, 2015: contributor

Can you be more specific about which policies become harder to implement?

luke-jr commented at 6:10 am on July 9, 2015: member

Priority-sorted area followed by fee/kB-sorted area (the previous default policy) would be the most obvious one. The policy before that (gradually higher min fee/kB as blocks are filled) as well.

jgarzik commented at 6:15 am on July 9, 2015: contributor

@luke-jr Read the code. You can implement such a policy with fee deltas.

petertodd commented at 6:58 am on July 9, 2015: contributor

Concept ACK, will review code and test later.

I’ve looked into policy stuff myself, and fee-delta methods make the most sense to me.

domob1812 commented at 6:19 pm on July 9, 2015: contributor

I actually like the concept of priority, which means that coinage can be used to “pay” for fees as a spam prevention. The “production rate” of coinage is bounded, which means that one can set a minimum priority for transactions and a certain reserved size in each block and be ensured that, on average, all free transactions fit in this reserved block space forever. I think that’s a nice feature of the current fee policy, because it allows free transactions (psychologically appealing to users) for everyone who does not overuse their coins. No spamming or DoS possible.

But of course, I agree that the change makes the code much simpler.

dgenr8 commented at 7:59 pm on July 9, 2015: contributor

Design ACK

rubensayshi commented at 11:31 pm on July 9, 2015: contributor

Concept ACK, I think the illusion that miners will mine TXs with 0 fee and high priority should be burned to the ground.

no matter what the blocksize debat does we’ll eventually end up with fees being a significant part of miner revenue and I don’t see miners maintaining the high priority policy at that point, might as well get rid of the code for it and since you’re keeping some of the priority stuff left intact a miner who truely believe they wants to keep doing that could still do so.

laanwj added the label Mempool on Jul 13, 2015

in src/main.cpp: in 711b2bfea5

851 
852         // Don't accept it if it can't get into a block
853-        CAmount txMinFee = GetMinRelayFee(tx, nSize, true);
854-        if (fLimitFree && nFees < txMinFee)
855+        CAmount txMinFee = GetMinRelayFee(tx, nSize);
856+        if (nFees < txMinFee)

jtimon commented at 0:31 am on August 24, 2015:

If GetMinRelayFee returns 0 this error only triggers when new coins are being created, which is checked later in Consensus::CheckTxInputs later (which should be checked before doing any policy checks about inputs, see #6445 ).

If it returns minRelayTxFee, you’re just repeating the check that is done a few lines later (if (nFees < ::minRelayTxFee.GetFee(nSize))).

Therefore this check and GetMinRelayFee() can be completely removed.

dcousens commented at 0:34 am on August 24, 2015: contributor

concept ACK, needs rebase

jtimon commented at 1:57 am on August 24, 2015: contributor

#6584 solves my nit here (and is rebased). It also includes #6445. That of course means I utACK this one, I just prefer my version (where I remove GetMinRelayFee()).

jgarzik commented at 1:07 am on September 15, 2015: contributor

Closing

jgarzik closed this on Sep 15, 2015

dcousens commented at 1:49 am on September 15, 2015: contributor

@jgarzik for the same reasons as #6584?

jgarzik commented at 1:57 am on September 15, 2015: contributor

The effort of frequent rebasing isn’t worth keeping this open.

dcousens commented at 2:36 am on September 15, 2015: contributor

@jgarzik is there any consensus on the change? It seems wrong to close this unless the concept has been rejected? @laanwj if I were to pick up this PR, would it be worth keeping open in light of #6584?

jgarzik commented at 3:06 am on September 15, 2015: contributor

As a general principle, we probably don’t want big pull requests to linger for a long time. This is straightforward to regenerate.

pstratem commented at 3:44 am on September 15, 2015: contributor

@jgarzik Is there an issue for this?

jgarzik commented at 5:08 am on September 15, 2015: contributor

@pstratem Good point - an issue would probably be appropriate, linking to this outdated branch as an example.

wallclockbuilder commented at 6:47 am on September 15, 2015: none

Concept ACK. Simplicity FTW!

DrahtBot locked this on Sep 8, 2021

Remove TX priority and free transaction area from mempool, block creator. #6405