Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default block priority size to 0 #7022

Merged
merged 1 commit into from Dec 1, 2015

Conversation

morcos
Copy link
Member

@morcos morcos commented Nov 15, 2015

As discussed on the mailing list, the current notion of priority is no longer completely supported with the 0.12 release. (In particular limited mempools will not allow free transactions when they have recently hit their limit). It is likely that further support will be removed in future release and the current concept is now deprecated. As such it makes sense to make the mining default not include priority space. It is still supported to create a priority space if the command line argument is changed. (To what degree depends on other open PR's)

@luke-jr
Copy link
Member

luke-jr commented Nov 15, 2015

NACK changing policy defaults as an attempt to influence miners. Also this change would likely be detrimental to Bitcoin in practice, since the priority space is mostly immune to spam attacks.

@TheBlueMatt
Copy link
Contributor

Concept ACK

On November 15, 2015 1:10:23 PM PST, Alex Morcos notifications@github.com wrote:

As discussed on the mailing
list
,
the current notion of priority is no longer completely supported with
the 0.12 release. (In particular limited mempools will not allow free
transactions when they have recently hit their limit). It is likely
that further support will be removed in future release and the current
concept is now deprecated. As such it makes sense to make the mining
default not include priority space. It is still supported to create a
priority space if the command line argument is changed. (To what
degree depends on other open PR's)
You can view, comment on, or merge this pull request online at:

#7022

-- Commit Summary --

  • Change default block priority size to 0

-- File Changes --

M src/policy/policy.h (2)

-- Patch Links --

https://github.com/bitcoin/bitcoin/pull/7022.patch
https://github.com/bitcoin/bitcoin/pull/7022.diff


Reply to this email directly or view it on GitHub:
#7022

@morcos
Copy link
Member Author

morcos commented Nov 15, 2015

I suppose I need to go fix up tests now.... I'll wait to see whether we want to merge this.

@paveljanik
Copy link
Contributor

NACK

Do we mix priority and low fee transactions here? Anyone seen sustaining attack of low fee high priority transactions?

@dcousens
Copy link
Contributor

Concept ACK/utACK, this matches the development agenda of removing priority in general.

@laanwj laanwj added the Mining label Nov 16, 2015
@sdaftuar
Copy link
Member

Concept ACK

@TheBlueMatt
Copy link
Contributor

Can we also mark the option as "deprecated". Though we may re-add a variation on it in the future, its current version will likely no longer be supported.

@petertodd
Copy link
Contributor

utACK

Also agree w/ the idea of marking it "deprecated"

Make RPC tests have a default block priority size of 50000 (the old default) so we can still use free transactions in RPC tests.  When priority is eliminated, we will have to make a different change if we want to continue allowing free txs.
@morcos
Copy link
Member Author

morcos commented Nov 30, 2015

Updated to make the RPC tests still work by passing in the old -blockprioritysize

To be clear this just changes the default for mining code so that we are properly signaling the intention to remove the current concept of priority in future releases.

@luke-jr
Copy link
Member

luke-jr commented Nov 30, 2015

We do not intend to remove priority in future releases (for mining).

@TheBlueMatt
Copy link
Contributor

@luke-jr yes. I think everyone but you would very much to prefer to
remove priority as it is used today. There is a bunch of discussion
about adding a similar feature that, instead, effects transactions by
modifying their "effective feerate" in its place, but block priority
space will not be in future releases.

On 11/30/15 22:48, Luke-Jr wrote:

We do not intend to remove priority in future releases (for mining).


Reply to this email directly or view it on GitHub
#7022 (comment).

@luke-jr
Copy link
Member

luke-jr commented Dec 1, 2015

@TheBlueMatt Then miners would be seriously harming Bitcoin to use future releases. NACK.

@TheBlueMatt
Copy link
Contributor

utACK

@laanwj laanwj merged commit 50947ef into bitcoin:master Dec 1, 2015
laanwj added a commit that referenced this pull request Dec 1, 2015
50947ef Change default block priority size to 0 (Alex Morcos)
@dcousens
Copy link
Contributor

dcousens commented Dec 2, 2015

@luke-jr from what I can read from the last few IRC meetings, there was a some agreement [yourself excluded] that this would go ahead for 0.12.
Obviously evident from the merge, that seems to be the consensus reached on this?

IRC meeting summaries:
https://forum.bitcoin.com/bitcoin-discussion/bitcoin-dev-irc-meeting-in-layman-s-terms-2015-11-19-t2943.html
https://forum.bitcoin.com/bitcoin-discussion/bitcoin-dev-irc-meeting-in-layman-s-terms-2015-11-12-t2682.html
https://forum.bitcoin.com/bitcoin-discussion/bitcoin-dev-irc-meeting-in-layman-s-terms-2015-11-05-t2285.html

@@ -217,7 +217,8 @@ def start_node(i, dirname, extra_args=None, rpchost=None, timewait=None, binary=
datadir = os.path.join(dirname, "node"+str(i))
if binary is None:
binary = os.getenv("BITCOIND", "bitcoind")
args = [ binary, "-datadir="+datadir, "-keypool=1", "-discover=0", "-rest" ]
# RPC tests still depend on free transactions
args = [ binary, "-datadir="+datadir, "-keypool=1", "-discover=0", "-rest", "-blockprioritysize=50000" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how this changes the default for master but the unit tests run with different settings by default. See #7177

@FinalHash
Copy link

As one of the worlds biggest miners I, and i think i speak for all my colleagues when i say, WE DO NOT SUPPORT THIS CHANGE. yes we really dont use it much but when tx's get stuck we can make it work. This is horseshit. Some dude just trying to get a commit based on a conversation. Revert it

@dcousens
Copy link
Contributor

@FinalHash then don't run it on your node?

@sipa
Copy link
Member

sipa commented Dec 14, 2015

Marshall, you may be missing some context here.

This is a change that has been discussed many times among Core's
developers, and many (but not all) have wanted to make for a long time.

I'm a bit confused about your statement that "when tx's get stuck we can
make it work". Are you sure you're not talking about the
prioritizetransaction RPC? That keeps working and will continue to do so,
and allows manually bumping a transaction ahead of the queue for mining
purposes.

All this change does is reduce the default size of the amount of block
space dedicated to transactions that get included based on
bitcoindays-destroyed-per-byte (instead of fee per byte) to zero. If there
are problems that can be solved by having such a space, you can always turn
it back on by setting a non-zero size manually.

Several people (myself included) would like the concept of sorting by
bitcoindays-destroyed-per-byte go away entirely over time, however, and
this is a step in that direction.

  • Maintaining this "priority area" complicates the mining-, mempool-,
    wallet-, and fee-estimation code significantly.
  • It works in a very non-optimal way. With a priority space, you may well
    kick out high fee transactions in favor of 0-fee transactions that just
    spend slightly older outputs. If bitcoin-days-destroyed-per-byte is
    something we value, I think we should account for it by having a single
    sort order that combines both, rather than having a dedicated space inside
    blocks.
  • At 50 kB it is hardly a useful measure against spam attacks. If it is, it
    would mean that 950 kB of data is hardly usable due to being filled with
    high-fee spam, and we get a 50 kB "usable" space in every block in which
    actual transactions can occur. The priority space can be set to be bigger
    (and still can be after this PR!), but at that point risks reducing fee
    income significantly. That may not be a worry now at the current fee
    levels, but it's not a solution we should encourage long term, as it is
    incompatible with Bitcoin's long term security (with fees paying for
    security).
  • Bitcoindays-destroyed-per-byte sorting unnecessarily benefits people who
    hold large amounts of old BTC.

@FinalHash
Copy link

This is just a warning shot for when you guys make changes without
discussing it with any miners.

Additionally, I think its a waste of time to do this. All miners have
effectively unanimously agreed two days ago to go with seg wit. I¹ve only
had a small amount of kick back from one dev in particular and the extent of
the warning was, ³Some security issues.² Can you email me and lets get seg
wit going.

From: Pieter Wuille notifications@github.com
Reply-To: bitcoin/bitcoin
<reply+00699eb269f54a2d00441fe9882e973444a98fb312160a3c92cf000000011286680d9
2a169ce06f98dc2@reply.github.com>
Date: Monday, December 14, 2015 at 6:20 AM
To: bitcoin/bitcoin bitcoin@noreply.github.com
Cc: Marshall Long marshall.long@me.com
Subject: Re: [bitcoin] Change default block priority size to 0 (#7022)

Marshall, you may be missing some context here.

This is a change that has been discussed many times among Core's
developers, and many (but not all) have wanted to make for a long time.

I'm a bit confused about your statement that "when tx's get stuck we can
make it work". Are you sure you're not talking about the
prioritizetransaction RPC? That keeps working and will continue to do so,
and allows manually bumping a transaction ahead of the queue for mining
purposes.

All this change does is reduce the default size of the amount of block
space dedicated to transactions that get included based on
bitcoindays-destroyed-per-byte (instead of fee per byte) to zero. If there
are problems that can be solved by having such a space, you can always turn
it back on by setting a non-zero size manually.

Several people (myself included) would like the concept of sorting by
bitcoindays-destroyed-per-byte go away entirely over time, however, and
this is a step in that direction.

  • Maintaining this "priority area" complicates the mining-, mempool-,
    wallet-, and fee-estimation code significantly.
  • It works in a very non-optimal way. With a priority space, you may well
    kick out high fee transactions in favor of 0-fee transactions that just
    spend slightly older outputs. If bitcoin-days-destroyed-per-byte is
    something we value, I think we should account for it by having a single
    sort order that combines both, rather than having a dedicated space inside
    blocks.
  • At 50 kB it is hardly a useful measure against spam attacks. If it is, it
    would mean that 950 kB of data is hardly usable due to being filled with
    high-fee spam, and we get a 50 kB "usable" space in every block in which
    actual transactions can occur. The priority space can be set to be bigger
    (and still can be after this PR!), but at that point risks reducing fee
    income significantly. That may not be a worry now at the current fee
    levels, but it's not a solution we should encourage long term, as it is
    incompatible with Bitcoin's long term security (with fees paying for
    security).
  • Bitcoindays-destroyed-per-byte sorting unnecessarily benefits people who
    hold large amounts of old BTC.


Reply to this email directly or view it on GitHub
#7022 (comment) .

@sipa
Copy link
Member

sipa commented Dec 14, 2015 via email

@luke-jr
Copy link
Member

luke-jr commented Dec 14, 2015

In particular, this change is needed to get the much-faster CreateNewBlock RPC that 0.12 is introducing.

This is not true...

@jyap808
Copy link
Contributor

jyap808 commented Dec 14, 2015

NACK. I have experienced to 2 very normal transactions take a very long time to confirm in the past few days. One took over 13 hours, another took over 24 hours.

@dcousens
Copy link
Contributor

@jyap808 and that is related how?

@jyap808
Copy link
Contributor

jyap808 commented Dec 15, 2015

@dcousens isn't it related to this change being committed?

@gmaxwell
Copy link
Contributor

no, jyap808, the things here are in software that won't be released for months. And, I expect, will go along with other changes that haven't been written yet.

@sipa
Copy link
Member

sipa commented Dec 15, 2015

@jyap808 This is about a change that is merged, but is not in any release, nor anywhere deployed in production yet. If your transaction in the current environment take a long time to confirm, it's certainly not due to this change, and they're likely both too low in fee and too low in priority.

@jyap808
Copy link
Contributor

jyap808 commented Dec 15, 2015

Thanks, I'll stop reading Reddit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet