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
  1. luke-jr commented at 7:34 PM on January 14, 2016: member

    On top of #7336

  2. 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.


    luke-jr commented at 11:28 PM on January 14, 2016:

    This is part of #7336

  3. 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.

  4. 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.

  5. luke-jr force-pushed on Jan 14, 2016
  6. in 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).


    laanwj commented at 9:42 AM on January 18, 2016:

    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.

  7. jonasschnelli added the label Docs and Output on Jan 15, 2016
  8. release-notes: Cover priority changes correctly, removing mentions of possible futures 4b8d2bc625
  9. luke-jr force-pushed on Jan 18, 2016
  10. luke-jr commented at 6:13 PM on January 18, 2016: member

    Addressed nits.

  11. 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.

  12. luke-jr commented at 5:33 AM on January 28, 2016: member

    Poke @laanwj

  13. laanwj commented at 10:54 AM on January 28, 2016: member

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

  14. morcos commented at 12:33 PM on January 28, 2016: member

    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.

  15. laanwj commented at 12:36 PM on January 28, 2016: member

    @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.

  16. 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.
  17. 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.

  18. p3yot3 commented at 4:30 PM on January 28, 2016: none

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

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

  19. 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.

  20. 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.

  21. release-notes: Mention possibility of priority removal in 0.13, uncertainty of priority calculation being changed back, and request community input 73a0375ebe
  22. luke-jr commented at 8:19 AM on February 9, 2016: member

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

  23. petertodd commented at 12:31 PM on February 9, 2016: contributor
  24. MarcoFalke commented at 12:35 PM on February 9, 2016: member

    ACK

  25. sipa commented at 12:47 PM on February 9, 2016: member

    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.

  26. MarcoFalke commented at 1:46 PM on February 9, 2016: member

    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.

  27. sipa commented at 1:52 PM on February 9, 2016: member

    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.

  28. morcos commented at 3:01 PM on February 9, 2016: member

    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)

  29. luke-jr commented at 8:02 PM on February 9, 2016: member

    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.

  30. 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.

  31. gmaxwell commented at 8:14 PM on February 9, 2016: contributor

    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?

  32. sipa commented at 8:18 PM on February 9, 2016: member

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

  33. 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.

  34. 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.

  35. release-notes: Remove suggestion to use 0.11 d0dbb6daee
  36. luke-jr commented at 9:29 PM on February 9, 2016: member

    Removed 0.11 suggestion.

  37. wangchun commented at 10:02 PM on February 9, 2016: none

    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?

  38. luke-jr commented at 10:04 PM on February 9, 2016: member

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

  39. 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?

  40. luke-jr commented at 10:43 PM on February 9, 2016: member

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

  41. 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=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 <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.

  42. release-notes: Significantly rewrite priority transactions section 3450f4cc95
  43. release-notes: Minor corrections and clarifications re Priority b46000415c
  44. luke-jr commented at 11:32 PM on February 9, 2016: member

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

  45. rebroad commented at 2:46 AM on February 11, 2016: contributor

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

  46. luke-jr commented at 2:51 AM on February 11, 2016: member

    @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.

  47. da2ce7 commented at 1:38 PM on February 11, 2016: none

    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.

  48. sipa commented at 1:56 PM on February 11, 2016: member

    ACK

  49. laanwj merged this on Feb 11, 2016
  50. laanwj closed this on Feb 11, 2016

  51. laanwj referenced this in commit 04503f78c7 on Feb 11, 2016
  52. MarcoFalke 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