0.12 release notes: Mining Policy Changes

[luke-jr]

On top of #7336

[paveljanik]

The text in () is part of the sentence, so please move the dot.

[luke-jr]

This is part of #7336

[paveljanik]

Well said.

[jonasschnelli]

Don't get me wrong,.. I like the "Bitcoin LJR" code fork. But IMO we should not mention code forks in our release notes. "[...] consider merging [...]" should be do the thing.

[sdaftuar]

I agree with @jonasschnelli that we should not suggest code forks in our release notes.

I also don't think we can suggest that people merge #7149 themselves. Even if your explanation were true (that it's due to lack of review -- I disagree with that characterization as other contributors to core have explicitly NACKed priority-related pulls recently, but I think it's beside the point), it's not reasonable for our release notes to suggest people run code that hasn't gone through Bitcoin Core's peer review process.

Finally I have a small quibble with the phrasing, that "calculation of the priority for transactions is no longer updated correctly". I think of it as an intentional change Bitcoin Core made to the definition of priority, rather than a "bug" introduced.

[sdaftuar]

(My earlier comment seems to have been zapped by github when the commit changed; hopefully this one will stick.)

I think it's inappropriate for the release notes to point people at PR's that haven't been reviewed and merged -- ascertaining the correctness of the code, and evaluating the various tradeoffs that could come up (including performance impact) is the whole point of Bitcoin Core's code review process. It doesn't make sense that we would recommend that users should consider short-circuiting that.

Also, I think the characterization of the change to priority is incorrect (that is "no longer updated correctly"); a more correct statement would be that we've changed the definition of priority so that we only age inputs that were confirmed at the time the transaction was received. The description here suggests that a bug was introduced, but we discussed this very issue when the changes were made, and it was clearly an intentional change -- even if not everyone agrees with it.

For discussion of the intentional change to the priority calculation, please see here: #7008

[luke-jr]

Users should be doing their own policy patching anyway. Core is a reference codebase, it isn't meant to be just taken and used as-is, especially for miners.

Justifying a clear bug by "intentional change" is just silly, especially when the new behaviour is not even deterministic. Note that the comments in #7008 do not in fact discuss this matter... and much of the support for merging the broken behaviour without the fix came from a misunderstanding that it was expensive to fix (which it isn't).

[laanwj]

I agre with @sdaftuar - let's not refer to code under development. Also this is too detailed for the release notes. The release notes are not meant as a documentation compendium, but as a quick overview of changes between releases.

[luke-jr]

Addressed nits.

[MarcoFalke]

This sounds odd if you don't mention the reason: Priority code is scheduled for removal in Bitcoin Core 0.13

[luke-jr]

It is not.

[paveljanik]

I like @MarcoFalke addition. It is good to inform our users about the plan.

[luke-jr]

There is no such plan.

[luke-jr]

Poke @laanwj

[laanwj]

utACK, but would like at least one ACK from someone that worked on this code to verify this that this is correct.

[morcos]

I prefer the current wording to these changes.

Also I believe with the mempool limiting changes we have badly broken the functioning of priority transactions, and it does developers and users a disservice to keep it limping along. It would be better to inform them of its impending dismissal.

@sdaftuar and I spent considerable time yesterday discussing badly needed improvements to fee estimation before deciding they probably weren't worth the hassle as long as priority continued to be brokenly supported. This is a common refrain. I thought we had made a clear decision as a project to drop it. This limbo state in between doesn't serve anyone.

[laanwj]

@morcos Right. But I think it doesn't hurt to mention how to still achieve some compatibility with the old code.
I agree adding the underlying reason and eventual plan makes sense, too.

[morcos]

@laanwj Sure I don't mind telling them how to set the old default, although I imagine they could figure that out by themselves.
The two more significant changes I object to which are:

• removing the notice that priority is scheduled for removal
• telling miners to use 0.11 if they care about accurate priority. Even for miners that care strongly about continuing to support what priority txs remain, I would highly recommend that they use 0.12 and its modified notion of priority. The differences in tx selection are minor.

[luke-jr]

@morcos Priority has absolutely nothing to do with fee estimation, and blaming it is just a lame excuse. Ignore priority entirely in the wallet if you want.

Priority continues to be a key part of the best policy for mining, and breaking it is a serious regression for which miners should remain at 0.11 until it is fixed. If it is removed entirely, I may very well just cease contributing to Core.

[p3yot3]

luke-jr says - "I may very well just cease contributing to Core."

I may very well start using Core again then......

[morcos]

@luke-jr It is actually quite intertwined with fee estimation. But it's also a problem with many areas of the code.

Its certainly not up to me whether priority is removed or not. In fact @sdaftuar and I have been the two people doing the most work to keep priority functioning correctly, and eventually decided it was too big an uphill battle and I believe its overall a detriment to keep priority at this point. But this is a decision for the whole project to take (if it hasn't already).

Obviously I don't want you to stop contributing to Core. I think that would be a huge loss for all of us. But I don't think thats a good way for us to go about deciding whether we are keeping priority or not. In any case, certainly not an appropriate discussion for a GitHub PR.

[luke-jr]

@morcos Please explain why you cannot ignore priority entirely in fee estimation. (Note that miners will continue to use priority-supporting policies even if Core removes it, so "we can pretend it doesn't exist at all" is not an answer.)

Obviously I don't want you to stop contributing to Core. I think that would be a huge loss for all of us. But I don't think thats a good way for us to go about deciding whether we are keeping priority or not. In any case, certainly not an appropriate discussion for a GitHub PR.

It's a cost. I will be keeping priority, period, whether it be in Core or only LJR. The cost of maintaining both priority-less code and the priority-supporting version is much more than just maintaining one or the other. So if Core refuses to support sane mining policies that are necessary for the health of the network, the least-expensive avenue is to maintain only LJR.

[luke-jr]

@morcos Added a final paragraph covering your nits. Look okay now?

[petertodd]

utACK luke-jr@73a0375

[MarcoFalke]

ACK

[sipa]

I disagree with recommending to use 0.11. If we believe that, we shouldn't release 0.12. However, It's a tradeoff and one I believe to be the right one. It simplifies code and improves performance, and is unlikely to matter in practice. Especially now old transactions expire and get reevaluated when received again. Can we just list the change and its motivation?

Something like:

Priority calculations for transactions are now only approximate, as maintaining the exact old semantics is complicated with the new getblocktemplate implementation relying on precalculated indexes. This is unlikely to affect mined blocks significantly.

[MarcoFalke]

and is unlikely to matter in practice

All versions of bitcoin core still support priority in the wallet via -sendfreetransactions. #7022 did not address this issue. Instead of #7022, the wallet code should be changed first to no longer support priority and then have the miners deprecate priority in a later release (if desired).

If all miners switch to 0.12 or master in the coming weeks without changing the defaults, priority transactions will get into the mempool of nodes but never actually get mined. It seems pulls were merged in the wrong order.

[sipa]

You're talking about the change of default to 0 bytes priority area. Bitcoin Core does apply estimation to this, so it should detect the case of miners adopting that (or not). I was talking about the non-updating of priorities in the mempool. The PR suggests sticking to 0.11 to retain accurate priority calculations. I believe that's not useful advice, and I don't expect it to matter. Your comment could be translated as a suggestion to set a non-zero priority area; it's not need to suggest not updating.

[morcos]

Yes I'd prefer something like what @sipa suggests.

Regarding @MarcoFalke's comments, I think we often confuse talking about priority transactions and free transactions. It appears to me that free transactions are a thing of the past (I have mempools well over 1GB of usage of fee paying txs and growing). Given the way mempool limiting code works, it could lead to a confusing user experience where you think a free transaction is possible, but in reality it has a very difficult time propagating.

Would we all be in agreement to at least take the intermediate step of removing support for free transactions in master? That is in order to get into a mempool and be relayed only your fee is considered but at least for now we'd leave the ability to select a mining policy which fills part of the block ordered by the new modified calculation of priority.

I think it would benefit us all to agree on a short/medium term direction so we don't spend so much time going in circles on the same issue. (Sorry for highjacking this PR with this segue)

[luke-jr]

I disagree with recommending to use 0.11. If we believe that, we shouldn't release 0.12.

@sipa I personally will be not recommending miners upgrade to [Core] 0.12. Miners should be given /useful/ information; either that's 0.11 or Knots 0.12.

However, It's a tradeoff and one I believe to be the right one. It simplifies code and improves performance, and is unlikely to matter in practice.

It's premature optimisation (GBT's speed shouldn't matter in practice). But perhaps more relevant: non-deterministic priorities makes proper unit tests (eg, as in #7149) infeasible.

Priority calculations for transactions are now only approximate, as maintaining the exact old semantics is complicated with the new getblocktemplate implementation relying on precalculated indexes. This is unlikely to affect mined blocks significantly.

It's not really complicated. The most "complicated" part is the usage of boost::multi_index::modify semantics rather than BOOST_FOREACH; if that is decidedly too complicated, I am happy to rewrite it using the latter.

Would we all be in agreement to at least take the intermediate step of removing support for free transactions in master? That is in order to get into a mempool and be relayed only your fee is considered but at least for now we'd leave the ability to select a mining policy which fills part of the block ordered by the new modified calculation of priority.

@morcos I was under the impression this was already the case for the 0.12 wallet, but recently noticed it is still an option. It's probably too late to change this for 0.12, and it really should be removed from the wallet before being removed elsewhere. Of course, if in practice it becomes impossible to relay gratis transactions before 0.13, it might make sense to skip the tiered removal.

[sipa]

@sipa I personally will be not recommending miners upgrade to [Core] 0.12. Miners should be given /useful/ information; either that's 0.11 or Knots 0.12.

And it's your full right to inform people either way of your opinion. But I disagree that your opinion should be what's written in the release notes.

Priority calculations for transactions are now only approximate, as maintaining the exact old semantics is complicated with the new getblocktemplate implementation relying on precalculated indexes. This is unlikely to affect mined blocks significantly.

It's not really complicated. The most "complicated" part is the usage of boost::multi_index::modify semantics rather than BOOST_FOREACH; if that is decidedly too complicated, I am happy to rewrite it using the latter.

It's more code to maintain that IMHO does not provide a benefit. Again, you're free to disagree.

[gmaxwell]

Priority calculations for transactions are now only approximate, as maintaining the exact old semantics is complicated with the new getblocktemplate implementation relying on precalculated indexes. This is unlikely to affect mined blocks significantly.

If you say it's unlikely to have a significant impact (I agree)-- than whats with the recommendation to use 0.11?

[sipa]

@gmaxwell That was my suggested language to use instead of the 0.11 recommendation.

[luke-jr]

@gmaxwell Oops, sorry for the confusion there, I guess I pasted it extra and without the quote marker.

And it's your full right to inform people either way of your opinion. But I disagree that your opinion should be what's written in the release notes.

@sipa So what should it suggest? "miners who consider accurate priority accounting important can go screw themselves"? I'm at a loss for how to resolve this.

[sipa]

@luke-jr In my opinion there is no reason why retaining the exact semantics that priority currently has is important. The definition of priority and its surrounding policy is arbitrary (rate limited, using fee as a limit, sorted after dividing by an arbitrary modified size), and it has changed several times. Why does it matter to maintain its exact same semantics? It at worst affects a tiny minority of transactions, and is always fixed by resubmitting. It won't affect things in practice.

Miners are free to run whatever policy code they prefer, so we should be honest about the changes we're making in the software we're producing. But what you're suggesting is essentially asking that 0.12's own release notes say "Don't run this". I think that's terrible advice for various reasons.

[luke-jr]

Removed 0.11 suggestion.

[wangchun]

Developers should provide a "framework" instead of making the decision, so that miners could easily write their own transaction selection policy. If some miners want the old-style priority, it should be able to calculate the value with a handy call within CreateNewBlock(). It also helps altcoin development, as they do not have to maintain unnecessary changesets. We cannot enforce all altcoins to drop the priority mining, can we?

[luke-jr]

@wangchun Agree with the first part. But altcoins are not our concern/responsibility at all.

[gmaxwell]

@wangchun What makes you think anyone is doing otherwise?

A simple "your costing function here" is already provided via RPC via prioritisetransaction, which eliminates the need to make difficult to maintain and potentially risky internal changes. Because the rpc is out of the critical path its acceptable for its cost function to be slow.

The internal design is driven by efficiency and must be. Structuring it as a fully general GetCost would result in txin lookups for each transaction and re-sorting the whole mempool every time createnewblock is called. This is the difference between milliseconds of computation and seconds. Some miners that violate the protocol by extending a blockchain without verifying it, or skipping transaction processing may not care about CreateNewBlock speed... but others do greatly.

The combination if a lightning fast internal decision coupled with RPC policy lets miners safely twiddle their policy without reliability risks and without delays that result in zero-tx blocks or skipping validation. Hopefully it is the best of all worlds.

What do you need that isn't yet being provided?

[luke-jr]

@gmaxwell prioritisetransaction is not a useful replacement for algorithmic policy decisions.

[gmaxwell]

@luke-jr text is still confusing. It sounds like priority transactions' won't be mined even if they meet the fee or that the rpc isn't functional.

Let me try:

Relay and Mining: Priority transactions

---

Bitcoin Core has a heuristic 'priority' based on coin value and age for transactions which do not meet pay
the minimum relay fee. Bitcoin Core relays and mines these transactions depending on the setting of -limitfreerelay=<r> (default: r=15 kB per minute) and -blockprioritysize=<s>.

In Bitcoin Core 0.12 when mempool limit has been reached a higher minimum relay fee takes effect to limit memory usage. Transactions which do not meet this higher effective minimum relay fee will not be relayed or mined even if they would rank highly according to the priority heuristic if they were accepted.

In Bitcoin Core 0.12 the reserved space for priority heuristic selected transactions is also set to zero.
To re-enable it, simply set -blockprioritysize=<n> where is the size in bytes of your
blocks to reserve for these transactions. The old default was 50k, so to retain the same policy, you would set -blockprioritysize=50000.

Additionally, as a result of computational simplifications, the priority value used for transactions received with unconfirmed inputs is lower than in prior versions to due avoiding recomputing the amounts as transactions confirm.

External miner policy set via the prioritisetransaction RPC to rank transactions already in the mempool continues to work as it has previously.

This internal automatic prioritization handling is being considered for removal entirely in Bitcoin Core 0.13. Community direction on this topic is particularly requested to help set project priorities.

[luke-jr]

Adopted @gmaxwell 's modifications and made some further clarifications. How's this looking now?

[rebroad]

So old and small transactions will no longer have a chance of being transmitted economically if these proposed changes go through?

[luke-jr]

@rebroad That's a related but different subject. In any case, this is just for discussion of the 0.12 release notes, not the actual topic itself.

[da2ce7]

I think that this would be nice to include in the 0.12 release. Personally I've taken advantage of the High-Priority transaction policies when making some personal transactions.

This is an important change of default behaviour that will affects both users and miners.

I believe the wording post @gmaxwell changes is clear and reasonable.

[sipa]

ACK