On top of #7336
0.12 release notes: Mining Policy Changes #7346
pull luke-jr wants to merge 5 commits into bitcoin:0.12 from luke-jr:0.12-release-notes-mining changing 1 files +35 −17-
luke-jr commented at 7:34 PM on January 14, 2016: member
-
in doc/release-notes.md:None in 1c3a5325be outdated
149 | -limit on OP_RETURN output size is now applied to the entire serialized 150 | -scriptPubKey, 83 bytes by default. (the previous 80 byte default plus three 151 | -bytes overhead) 152 | +combination of data pushes and numeric constant opcodes (OP_1 to OP_16) after 153 | +the OP_RETURN. The limit on OP_RETURN output size is now applied to the entire 154 | +serialized scriptPubKey, 83 bytes by default. (the previous 80 byte default plus
paveljanik commented at 8:27 PM on January 14, 2016:The text in () is part of the sentence, so please move the dot.
in doc/release-notes.md:None in 1c3a5325be outdated
180 | +simply set `-blockprioritysize=<n>` where <n> is the size in bytes of your 181 | +blocks to reserve for priority transactions. The old default was 50k, so to 182 | +retain the same policy, you must set `-blockprioritysize=50000`; however, 183 | +miners are encouraged to consider the trade-off between network health and 184 | +fees, and make their own decision for how much of the block to commit for this 185 | +purpose.
paveljanik commented at 8:29 PM on January 14, 2016:Well said.
in doc/release-notes.md:None in 1c3a5325be outdated
186 | + 187 | +Additionally, calculation of the priority for transactions is no longer updated 188 | +correctly when they are received with unconfirmed inputs. There is a proposed 189 | +fix for this (#7149), but due to lack of review, it is not ready in time for 190 | +0.12.0. Miners concerned with this regression should consider merging the fix 191 | +themselves, or using Bitcoin LJR which will include it.
jonasschnelli commented at 8:31 PM on January 14, 2016: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 commented at 9:11 PM on January 14, 2016: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.
luke-jr force-pushed on Jan 14, 2016in doc/release-notes.md:None in acae4ffd60 outdated
186 | + 187 | +Additionally, calculation of the priority for transactions is no longer updated 188 | +correctly when they are received with unconfirmed inputs. There is a proposed 189 | +fix for this (#7149), but due to lack of review, it is not ready in time for 190 | +0.12.0. Miners concerned with this regression should consider merging the fix 191 | +themselves.
sdaftuar commented at 1:08 AM on January 15, 2016:(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 commented at 1:16 AM on January 15, 2016: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).
jonasschnelli added the label Docs and Output on Jan 15, 2016release-notes: Cover priority changes correctly, removing mentions of possible futures 4b8d2bc625luke-jr force-pushed on Jan 18, 2016luke-jr commented at 6:13 PM on January 18, 2016: memberAddressed nits.
in doc/release-notes.md:None in 4b8d2bc625 outdated
187 | +(default: `r=15` kB per minute) and `-blockprioritysize=<s>`. 188 | + 189 | +In Bitcoin Core 0.12, priority transactions are not accepted to the mempool nor 190 | +relayed if mempool limiting has triggered a higher effective minimum relay fee. 191 | + 192 | +Mining of priority transactions is also now disabled by default. To re-enable
MarcoFalke commented at 6:26 PM on January 18, 2016:This sounds odd if you don't mention the reason:
Priority code is scheduled for removal in Bitcoin Core 0.13
luke-jr commented at 7:37 PM on January 18, 2016:It is not.
paveljanik commented at 1:31 PM on January 23, 2016:I like @MarcoFalke addition. It is good to inform our users about the plan.
luke-jr commented at 1:48 PM on January 23, 2016:There is no such plan.
laanwj commented at 10:54 AM on January 28, 2016: memberutACK, but would like at least one ACK from someone that worked on this code to verify this that this is correct.
morcos commented at 12:33 PM on January 28, 2016: memberI 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.
morcos commented at 12:42 PM on January 28, 2016: member@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 commented at 4:21 PM on January 28, 2016: member@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 commented at 4:30 PM on January 28, 2016: noneluke-jr says - "I may very well just cease contributing to Core."
I may very well start using Core again then......
morcos commented at 5:07 PM on January 28, 2016: member@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 commented at 11:38 PM on January 28, 2016: member@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.
release-notes: Mention possibility of priority removal in 0.13, uncertainty of priority calculation being changed back, and request community input 73a0375ebepetertodd commented at 12:31 PM on February 9, 2016: contributorMarcoFalke commented at 12:35 PM on February 9, 2016: memberACK
sipa commented at 12:47 PM on February 9, 2016: memberI 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 commented at 1:46 PM on February 9, 2016: memberand 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.12ormasterin 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 commented at 1:52 PM on February 9, 2016: memberYou'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 commented at 3:01 PM on February 9, 2016: memberYes 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 commented at 8:02 PM on February 9, 2016: memberI 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 commented at 8:07 PM on February 9, 2016: member@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 commented at 8:14 PM on February 9, 2016: contributorPriority 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?
luke-jr commented at 8:44 PM on February 9, 2016: member@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 commented at 9:23 PM on February 9, 2016: member@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.
release-notes: Remove suggestion to use 0.11 d0dbb6daeeluke-jr commented at 9:29 PM on February 9, 2016: memberRemoved 0.11 suggestion.
wangchun commented at 10:02 PM on February 9, 2016: noneDevelopers 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?
gmaxwell commented at 10:31 PM on February 9, 2016: contributor@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?
gmaxwell commented at 10:46 PM on February 9, 2016: contributor@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=15kB 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 <n> 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.
release-notes: Significantly rewrite priority transactions section 3450f4cc95release-notes: Minor corrections and clarifications re Priority b46000415crebroad commented at 2:46 AM on February 11, 2016: contributorSo old and small transactions will no longer have a chance of being transmitted economically if these proposed changes go through?
da2ce7 commented at 1:38 PM on February 11, 2016: noneI 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 commented at 1:56 PM on February 11, 2016: memberACK
laanwj merged this on Feb 11, 2016laanwj closed this on Feb 11, 2016laanwj referenced this in commit 04503f78c7 on Feb 11, 2016MarcoFalke locked this on Sep 8, 2021
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me