Policy: Remove free transactions special case code

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

This is slightly modified version of #6405 that is rebased on top of #6445 (well, a rebased version of it). The small modification (in the last commit) solves my nit there: #6405 (review)

Optimizations: Consensus: In main::AcceptToMemoryPool, main::ConnectBlock, and miner::CreateNewBlock and

In all of them, reject transactions creating new money earlier.
Consensus::CheckTxInputs gets nTxFee as output parameter and is separated from main::CheckInputs [renamed CheckInputsScripts]

- Consensus::CheckTxInputs (called by the rest):

Don't calculate nValueOut twice
Don't check nFees < 0 twice

- main::AcceptToMemoryPool:

Don't call CCoinsViewCache::HaveInputs twice
Don't calculate nValueIn 3 times
Don't calculate nValueOut 5 times

- miner::CreateNewBlock:

Don't call CCoinsViewCache::HaveInputs twice
Don't calculate nValueIn twice
Don't calculate nValueOut 3 times

- main::ConnectBlock:

Still call CCoinsViewCache::HaveInputs twice
Don't calculate nValueIn twice
Don't calculate nValueOut 3 times

52279bc40d

miner: remove prio calculations and dPriority from TxPriority tuple a7db703647

mempool: remove priority from memory pool per-entry container 19391192fc

mempool, miner, qt, main: remove uses of priority, free transactions 48abd4a916

laanwj added the label TX fees and policy on Aug 24, 2015

Policy: Remove free transactions from python tests 442808e65f

jtimon commented at 10:13 pm on August 25, 2015: contributor

@laanwj shouldn’t this be labeled with “mempool” too like #6405 ? @jgarzik you didn’t adapted the python tests, feel free to cherry pick it for #6405

By the way, I’m aware that we talked about merging #6470 first. But if it’s takes much longer to merge it for whatever reason, I think we should be blocking other things like this or a dynamic min relay fee (which I would really want to do after #6068 ).

luke-jr commented at 0:17 am on August 26, 2015: member

Oversimplifying default policy also makes policy customisation more difficult. So IMO concept NACK unless there’s a good reason for this (missing from description).

sipa commented at 0:29 am on August 26, 2015: member

In my opinion, policy abstraction (ability for customization) is something to plug in after the codebase in which it’s plugged into is stable. That is currently not the case, the mempool will very likely significantly change for memory limiting, floating relay fee, indexes for block creation, … much of that is much harder when trying to maintain a (IMO legacy and mostly broken) hardcoded policy.

So, what we should do, IMO, is get rid of the free relay policy and restrict to pure feerate, improve the mempool code to work based on that, and then plug in a sane policy configuration that can tweak what “cost” and “fee” we associate with transactions.

dcousens commented at 1:18 am on August 26, 2015: contributor

Concept ACK

Oversimplifying default policy also makes policy customisation more difficult. So IMO concept NACK unless there’s a good reason for this (missing from description).

IMHO, it does the opposite, it makes it easier to customize. However, my experience here is probably not as extensive as yours, as it is based purely on editing the code as it stands not implementing policies specifically.

sipa commented at 7:34 pm on August 26, 2015: member

I agree with @dcousens. The default policy that is implemented now is very difficult to customize (it relies on 2 different ordering criteria, with associated specific code in block construction, and specific code in mempool acceptance to work). A policy that just introduces a “generalized cost” and “generalized fee” for transactions would be much more flexible and easier to work with.

jtimon commented at 8:57 pm on August 26, 2015: contributor

@luke-jr the goal is simplifying this special-cased code. Once we have a CPolicy interface and a -policy parameter to select alternative policies we will make it easier for miners to implement their own policies. But changing the default policy is in my opinion mostly neutral to policy modularization. If you could point to the parts of the removed code are you worried about concretely, that would be helpful. It is also worth noting that it is noncompetitive/irrational for miners to include free transactions in their blocks (there’s a block relay costs to them but no gain in exchange).

paveljanik commented at 5:57 am on August 27, 2015: contributor

NACK Free transactions are social aspect of the bitcoin (even 0.00001 is a lot of money somewhere). In the future, they should be made optional. Miner fee can be paid offline, so you can’t say it is irrational for them to include such transaction. Maybe it is a miner’s own transaction…

dcousens commented at 7:48 am on August 27, 2015: contributor

@paveljanik this doesn’t stop miners from accepting ~~free~~ 0 fee transactions. This just makes the behaviour rational and based on the incentive, rather than something that is difficult to reason about in terms of miner priority.

chris-belcher commented at 2:50 pm on August 30, 2015: contributor

It may be rational for miners to accept free txes if it decreases the UTXO set size.

E.G. https://www.blocktrail.com/BTC/tx/9b933f6f811dcc920d35bc48876181da4b29282ad452bdeb7563cff7c6c09bf9

ABISprotocol commented at 6:05 am on September 3, 2015: none

NACK. NACK. I agree with @paveljanik et. al.

jtimon commented at 6:56 pm on September 3, 2015: contributor

@paveljanik

Free transactions are social aspect of the bitcoin (even 0.00001 is a lot of money somewhere).

Free transactions create the illusion that the system (currently heavily subsidized by seigniorage) is cheap when it is actually very costly. They will eventually disappear when miners start using competitive policies and in the meantime are creating false expectations and confusing users and entrepreneurs. One satoshi is currently 0.000002 usd. I believe that if minimum mined fees rise from 0 to 1 satoshi we’re still fine when it comes to attracting adoption (but it would still be a huge step forwards in miners implementing competitive policies).

In the future, they should be made optional

Fees are already optional. The policy minimum relay fee is also configurable at run time: you can set it to zero if you want to.

Miner fee can be paid offline, so you can’t say it is irrational for them to include such transaction.

Miner’s can still implement their own policy, but offline fees are useless when it comes to DoS network protections (which is what the minimum relay fee is about). They could even receive a fee in-chain and give it back offline (on whatever payment method they’re using to settle with their customers).

Maybe it is a miner’s own transaction…

They can pay fees to themselves (which is better for their privacy) or, again, implement their own policy. The current code is not addressing any of those cases either. @chris-belcher That’s a benefit for all miners, not just yourself (who are paying the cost). There should be an incentive to reduce/not-increase the utxo size built in the consensus rules (for example consensus_size(tx) = real_size(tx) + delta_to_the_utxo_size(tx)).

Anyway, since this seems to be controversial, I’m closing it. @jgarzik feel free to cherry pick from here if you still want to keep #6405 open (this one is rebased, passes travis tests and removes a little bit more code). @sipa If I create a PR that instead of removing the code at least encapsulates it in a function (hopefully a method assuming #6068 gets merged this year) in the policy folder, will it have to wait for #6470 (or its current replacement) too? What if I remove the minRelayTxFee global (make it an attribute in CStandardPolicy) and implement a dynamic minRelayTxFee that uses the fee estimator at the same time?

I can have the code ready relatively fast if there’s interest in reviewing it and it’s not going to be blocked by #6470.

jtimon closed this on Sep 3, 2015

morcos commented at 2:19 am on September 15, 2015: member

@jtimon Heh, I know we were just together, but I’ve been totally behind on PR’s for the last couple weeks so didn’t see this. I just wanted to give my two cents that I’m now happy to rebase any of my existing mempool limiting PR’s on top of policy changes. During the rapid iteration to find a solution I felt like that was going to be a hindrance, but I think the work is far enough along now that it shouldn’t hold up anything else.

ABISprotocol commented at 9:17 am on September 15, 2015: none

I agree with @luke-jr @sipa and @paveljanik ~ I realize this is closed out, but in light of new comments, I’m just emphasizing my prior NACK. See in particular @sipa’s comments. cc @morcos

dcousens commented at 6:47 am on September 16, 2015: contributor

@ABISprotocol: @sipa’s comments were in favour of this change? Could you link directly to what you mean?

sipa commented at 11:57 am on September 16, 2015: member

I’m in favor, but let’s get the mempool refactorsnin first. This should be a simple change to redo (correct me if I’m wrong @jgarzik).

jgarzik commented at 12:52 pm on September 16, 2015: contributor

Correct, it is trivial to regenerate at any time.

ABISprotocol commented at 11:28 pm on September 16, 2015: none

@dcousens Please see here and also @sipa’s more recent comment above (Sept. 16) here. My comments above may have seemed conflicting, to simplify, I think this puts the cart before the horse. Finish what was intended here and in #6557 then move on to other matters that are being suggested here in this issue and in 6405.

jtimon commented at 1:55 pm on September 17, 2015: contributor

Yes, this is trivial to rebase. Also, I’m going to try another approach where the functionality is unaffected but encapsulated in policy and at the same time minTxRealyFee stops being a global and it’s dynamic (using the fee estimator) instead. But I won’t open a PR with that branch at least until #6068 is merged, but people can take a look at https://github.com/bitcoin/bitcoin/compare/master...jtimon:policy-tip-0.12.99

jtimon commented at 1:57 pm on September 17, 2015: contributor

Sorry, I meant this branch: https://github.com/bitcoin/bitcoin/compare/master...jtimon:policy-minrelayfee-0.12.99

DrahtBot locked this on Sep 8, 2021

Policy: Remove free transactions special case code #6584