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

Revert default block priority size to 50k #7151

Closed
wants to merge 1 commit into from

Conversation

@luke-jr
Copy link
Member

luke-jr commented Dec 1, 2015

The priority area is often our only defense against spam, so disabling it should not be encouraged.
Additionally, the only benefit for disabling it is performance, which is no longer an issue with #6898.

While there may be arguably better policies for handling priority, these are not yet supported (nor even evaluated theoretically or tested in practice).
Until those are implemented and tested, we shouldn't regress default policy.

The priority area is often our only defense against spam, so disabling it should not be encouraged.
Additionally, the only benefit for disabling it is performance, which is no longer an issue with #6898.

While there may be arguably better policies for handling priority, these are not yet supported (nor even evaluated theoretically or tested in practice).
Until those are implemented and tested, we shouldn't regress default policy.
@MarcoFalke

This comment has been minimized.

Copy link

MarcoFalke commented on e4d7271 Dec 1, 2015

Don't forget to adjust util.py

This comment has been minimized.

Copy link
Owner Author

luke-jr replied Dec 1, 2015

I intentionally did not adjust util.py, since the change there makes sense regardless of the default we use here.

This comment has been minimized.

Copy link

MarcoFalke replied Dec 6, 2015

It does not make sense. There is currently no rpc test to verify priority behavior because it is broken. Setting a hardcoded value won't help.

This comment has been minimized.

Copy link

MarcoFalke replied Dec 6, 2015

I'd say a new rpc test exercising the priority code is needed within this PR

This comment has been minimized.

Copy link
Owner Author

luke-jr replied Dec 7, 2015

Priority has worked fine (not broken) up through 0.11. There is at least one RPC test that exercises it (hence the util.py change!).

This comment has been minimized.

Copy link

MarcoFalke replied Dec 7, 2015

We are talking about master not 0.11.

one RPC test that exercises it

With "exercises"you mean: Create one free transaction. The comment says a child of the free transaction should pay a fee but in fact this is nowhere asserted. (https://github.com/bitcoin/bitcoin/blob/075faaebf2e534265ff8aca015eaf03a8a156f32/qa/rpc-tests/wallet.py#L68)

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Dec 1, 2015

The point of setting it to 0 is to mark it deprecated, not run faster or encourage policy decisions. Feel free to bring this back up at an IRC meeting but there was strong consensus to push it to 0.

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 1, 2015

It's not deprecated, and should not be until there is a reasonable alternative.

Also, IRC meetings are explicitly not for decision-making. And there's no consensus while someone (eg, me) maintains a reasonable disagreement.

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Dec 2, 2015

ACK

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Dec 2, 2015

As discussed on IRC, its probably worth checking the following before merging this:

how many tx per day get confirmed solely due to priority - measure field use
(credit: @jgarzik)

and

how many transactions in blocks now ... would have been priority eligible at all; or otherwise become eligible within 24-36 hours.
(credit: @gmaxwell)

Could probably answer both questions with the same data.

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Dec 2, 2015

@dcousens Yes, this should have been done before merging the previous PR. Do we have any such data available?

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Dec 2, 2015

On the other hand: can we have such data at all?

If the first transaction in the block after the coinbase is a zero fee "high-priority" transaction, we simply can't take this as a "high-priority" transaction, because the miner could have mined this particular block because of his will and not because of the high-prio code in Bitcoin Core.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 2, 2015

There was common agreement to set it to 0 in #7022 - apart from you. My impression was that changing the default has been discussed in many places including the IRC meetings and mailing lists and the outcome was the same.

In general: Can we cut out this nonsense about default values? It's the same crap every time. I'm never going to merge mining/mempool related default change again. If people want a different value they'll just have to set it.

@petertodd

This comment has been minimized.

Copy link
Contributor

petertodd commented Dec 2, 2015

Can we cut out this nonsense about default values?

+1

@morcos

This comment has been minimized.

Copy link
Member

morcos commented Dec 2, 2015

@gmaxwell re: transactions that would have been priority eligible in blocks now: I think it's important to attempt to distinguish transactions that were sent with the intention to be confirmed by priority. Of course I think we will find those, I send those myself, but that doesn't mean those users won't adapt.

Just to summarize some of the reasons to deprecate priority:

  • code complexity: I think the computational complexity can be kept within reason at least for the existing functionality, but the code touches a lot of different places and it makes it more difficult to do perhaps more important work in those areas.
  • long term viability: If it's not incentive compatible, how long should we go about trying to put effort into supporting it.
  • it's currently broken: With mempool limiting not respecting priority its almost a useless concept anyway.
  • we're not foreclosing on the possibility of some future notion of coin age: Perhaps we can regain most of the lost benefits of the existing priority metric with something like sipa's proposed coin age based fee delta. Or even just a cleaner and more general way to implement a different metric that doesn't require so much core maintenance.
@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Dec 5, 2015

OK, as only me and @luke-jr ACKed, there is a clear dev-consensus that priority is to be removed from Bitcoin Core. I accept it myself. Do not agree with it, but accept it ;-) This will make the code lot simpler but we are loosing interesting aspect/motivation for some people. In fact this decision has economic effect (small though) on some bitcoin users.

The next time we plan to do something similar, we should announce the plan to do so first (no code changes) and then do it in some of the next releases. This was done correctly (IMO) in case of "accounts". We see a lot of DEPRECATED in the helptexts etc. Lets learn from it.

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 10, 2015

Initial analysis shows approximately 6% of transactions during 2014 confirmed by priority.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 10, 2015

approximately 6% of transactions during 2014

Would be awesome to see a historic chart which includes

  • all transaction
  • transaction eligible for priority
  • transaction which used priority
@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 10, 2015

AFAIK nobody has kept such records, and they can't be deduced from the blockchain.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 11, 2015

You can still estimate the bounds assuming default values?

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 11, 2015

"transaction eligible for priority" requires knowing unconfirmed transactions at all points of the history.

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Dec 11, 2015

@MarcoFalke this should have been done prior to setting the prio to 0, not now...

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 13, 2015

A specific analysis of https://np.reddit.com/r/Bitcoin/comments/3wln8w/0_confirmations_after_12_hours/ wherein a user sent a transaction without a [significant] fee, and the recipient was concerned that 12 hours had passed without it being mined. This transaction would have become eligible for relaying and mining under the default priority policy in 0.11, after 21 additional hours (total 33 hours). Without priority, it would never be mined, leaving no recourse for the participants until the sender upgraded to a RBF-capable wallet (which currently do not exist) and decided to make use of that feature.

@morcos

This comment has been minimized.

Copy link
Member

morcos commented Dec 13, 2015

When you say eligible for priority. You don't mean the AllowFree threshold do you? You mean actually enough to get into a block? Those are several orders of magnitude different.

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 13, 2015

@morcos The AllowFree threshold is the only minimum... Once it's in the memory pool, the first 50k (or whatever the priority size is configured as) of top-priority transactions get mined. So the only way that example wouldn't have been priority-mined (after the total 33 hours), is if there were higher priority transactions waiting (in which case any of them would work just as well for an example).

@morcos

This comment has been minimized.

Copy link
Member

morcos commented Dec 13, 2015

@luke-jr you know the first 50k of top-priority transactions includes fee paying txs right? So very few if any of them would have served as a good example b/c they would be mined because of fee anyway. A tx will never be mined by priority until its priority is at least 1000x higher than AllowFree.

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 13, 2015

@morcos That does not follow, especially under the conditions of spam attacks. Where do you get your 1000x from? I don't see that in the code...

@morcos

This comment has been minimized.

Copy link
Member

morcos commented Dec 13, 2015

@luke-jr It's not in the code. I'm just saying that if you look at the last tx in the 50k priority block (so the lowest priority tx that made it into the block by priority) you will discover its priority is almost always above 1e10, maybe closer to 1e12 typically. What this means is that a tx that pays too low a fee and is hoping to get into a block based on priority, it'll need to wait until its priority is that high to have a chance.

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 13, 2015

That just means the priority space is being used up. If anything it would suggest an increase in the size, not removal of it...

@morcos

This comment has been minimized.

Copy link
Member

morcos commented Dec 14, 2015

Less than 1% of txs benefit from priority.

I ran a brief test to see how many txs, which were likely included in a block as a result of their priority, would not have been included in a block based on their fee in the near future. I looked at 6 weeks of data starting Oct 1st and only at blocks which had at least 100 txs. I then assumed that the last 10 txs in each of those blocks were fee based txs. And the lowest fee of those 10 txs was the necessary fee to be included in the block. So I looked for each block at how many txs were in it that had a fee less than what would have been necessary to be included in at least 2 out of the next 10 blocks, and I assumed those were the txs that benefited by priority. This amounted to 56K out of 6.7M txs or 0.83%.

@luke-jr

This comment has been minimized.

Copy link
Member Author

luke-jr commented Dec 14, 2015

@morcos You appear to be assuming ideal or current conditions. Spam attacks can suddenly increase the minimum feerate in a feerate-only policy for a prolonged period. Furthermore, any analysis of recent blocks is likely to be tainted by miners who have disabled the priority area due to perceptions of performance problems in 0.11 (that have since been improved).

@jtimon

This comment has been minimized.

Copy link
Member

jtimon commented Jan 3, 2016

It's not deprecated, and should not be until there is a reasonable alternative.

We want to deprecate it and that's what the 0 default signals. We can discuss the "alternative" without changing its default again.
NACK

@laanwj laanwj closed this Jan 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.