Fix confusing blockmax{size,weight} options, dont default to throwing away money #11100

TheBlueMatt · 2017-08-20T21:54:34Z

No sensible user will ever keep the default settings here, so not
having sensible defaults only serves to screw users who are
paying less attention, which makes for terrible defaults.

Additionally, support for block-size-limiting directly has been removed:

This removes block-size-limiting code in favor of GBT clients
doing the limiting themselves (if at all).
-blockmaxsize is deprecated and only used to calculate an implied
blockmaxweight, addressing confusion from multiple users.
getmininginfo's currentblocksize return value was returning
garbage values, and has been removed, also removing a
GetSerializeSize call in some block generation inner loops and
potentially addressing some performance edge cases.

luke-jr · 2017-08-20T21:59:37Z

NACK, these are not sensible. If anything, they should be reduced.

laanwj · 2017-08-21T07:59:10Z

Please, not again, no more defaults fighting...

gmaxwell · 2017-08-23T00:17:00Z

ACK. The current settings are reprehensible: They result in guaranteed income reduction for anyone who follows them for almost no measurable benefit-- because as we see almost everyone maxes them out regardless. The losses are also not trivial, but are often about 5% of mining income-- which wasn't the case when they were last discussed but is now.

Luke's argument rests on an unlikely and now conclusively disproven theory that miners will individually act against their own short term economic best interests to preserve the usability of the network. This is demonstratively false and even if it were true, setting an economically rational default wouldn't change it. (these hypothetical altruistic miners would simply lower their defaults, if they existed)

To the limited extent that anyone does mine with these defaults (not much) they also make fee estimates less reliable by adding additional variance to the capacity of the network. A few blocks showing up underfilled is doing no one any favors-- it doesn't meaningfully reduce the initial synchronization time. All it does is lower the revenue of those who do it. To the extent that any of that is driven by a genuine desire to aid the network (rather than just misconfiguration)-- do we really want people who want to help to be less successful (and thus less able to add hashpower and increase their influence)

Setting the defaults in a way which virtually no one will use also has the problem of causing people to change the default and then when the way the options works needs to change their outdated settings can cause unexpected behavior. For example, there are many miners who have set the max size to a million bytes to override these broken defaults. And then with segwit they'll be producing undersized blocks unintentionally. They may even be paying enough attention to realize they need to change them, only to set them to 2 million bytes and still produce undefilled blocks.

Setting a reduced default against the traffic in the network today violates the principle the software should act in the interest of the people using it and in doing so shows disrespect where none is intended, it violates the principle that absent significant harm to it users it should act in the public interest (by making fee estimation less reliable).

IMO if it isn't reduced it should be removed completely. At least there is a strong argument that miners producing blocks which fail to confirm all transactions is harmful to the public interest, and improvements in block propagation have largely eliminated size based considerations as legitimate reasons to choose to produce a smaller block. (Moreover if one was for some reason still concerned about size impacts on propagation the received time sensitive framework from Suhas would be the right way to go about it, not simply reducing the maximum size.)

laanwj · 2017-08-23T10:31:06Z

Previous discussion was here (possibly more, but these I could find in a quick search):

Increase DEFAULT_BLOCK_MAX_SIZE to 1MB Increase DEFAULT_BLOCK_MAX_SIZE to 1MB #6231
Increase DEFAULT_BLOCK_MAX_SIZE to 1MB Increase DEFAULT_BLOCK_MAX_SIZE to 1MB #7622
Updating default max block size Updating default max block size #8268

As we know all miners will override the defaults, and for other users the value doesn't matter, I think this is a pointless waste of energy, creating angry discussions unnecessarily.

I'm unsubscribing from this thread.

janbraiins · 2017-08-24T06:10:30Z

Hi,
this make sense to me. From pool operator's prospective, we set this to max for the reasons stated by Greg. What is confusing is that without researching the code, it is not quite clear how each of the limits influence each other. A sane default for us was just to set blockmaxweight, having the core derive the blockmaxsize.
Jan

gmaxwell · 2017-08-24T08:53:19Z

@honzik666 Right, the correct settings are to remove any mention of blockmaxsize from your configuration and set only blockmaxweight=4000000.

Blockmaxsize is a legacy option that wasn't even existent at first in the segwit settings. It doesn't make sense to use it, setting it (at least to anything other than the maximum of 4 million) will just lower your income.

TheBlueMatt · 2017-08-24T15:42:17Z

Pushed a new commit to hopefully help address the confusion that a few people have had on these options, which also fixes a bug or two in the process.

sipa · 2017-08-24T16:16:24Z

src/policy/policy.h

@@ -17,9 +17,9 @@ class CCoinsViewCache;
 class CTxOut;

 /** Default for -blockmaxsize, which controls the maximum size of block the mining code will create **/
-static const unsigned int DEFAULT_BLOCK_MAX_SIZE = 750000;
+static const unsigned int DEFAULT_BLOCK_MAX_SIZE = MAX_BLOCK_SERIALIZED_SIZE - 1000;


delete instead?

meshcollider · 2017-08-24T19:25:08Z

Concept ACK as discussed in meeting

sipa · 2017-08-24T19:27:21Z

src/miner.cpp

        if (!fWeightSet) {
-            options.nBlockMaxWeight = options.nBlockMaxSize * WITNESS_SCALE_FACTOR;
+            options.nBlockMaxWeight = gArgs.GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_WEIGHT / WITNESS_SCALE_FACTOR) * WITNESS_SCALE_FACTOR;
        }


Add an else branch here that gives an error "-blockmaxsize and -blockmaxweight cannot be set simultaneously".

morcos · 2017-08-24T19:33:42Z

Concept ACK

luke-jr · 2017-08-24T19:37:37Z

Miners have in recent years maxed out 1 MB. That is completely different from 4 MB, and does not prove they will increase to 4 MB immediately in any sense.

Luke's argument rests on an unlikely and now conclusively disproven theory that miners will individually act against their own short term economic best interests to preserve the usability of the network.

It has not been disproven. Miners have been given bad advice.

setting an economically rational default wouldn't change it.

Only in the case that the default doesn't matter. In which case, the argument to change the default falls apart completely.

For example, there are many miners who have set the max size to a million bytes to override these broken defaults. And then with segwit they'll be producing undersized blocks unintentionally.

If they want 1 MB, then their configuration of 1 MB should get them 1 MB. There is no such thing as undersized blocks.

Setting a reduced default against the traffic in the network today violates the principle the software should act in the interest of the people using it and in doing so shows disrespect where none is intended,

No such principle has been established. Defaults should within reason be what is best for Bitcoin as a whole.

it violates the principle that absent significant harm to it users it should act in the public interest (by making fee estimation less reliable).

Fee estimation is an unrelated matter. If it can't handle a variety of block sizes, that's a bug in the estimation logic.

improvements in block propagation have largely eliminated size based considerations as legitimate reasons to choose to produce a smaller block.

No, they haven't. Block propagation has been "solved" by centralised relay networks (FIBRE) and SPV mining, both of which are harmful to the network and not real solutions. Additionally, the most bandwidth-contrained users run in blocksonly mode, which doesn't benefit from compact blocks or other such improvements. Furthermore, none of this addresses the issue of blockchain size and IBD, at all.

Again, NACK.

(Maybe #3229 can be reopened?)

jameshilliard · 2017-08-24T20:22:09Z

ACK

Block propagation has been "solved" by centralised relay networks (FIBRE) and SPV mining, both of which are harmful to the network and not real solutions.

Even without FIBRE or SPV mining it makes individual economic sense for miners to mine max weight blocks due to relatively high transaction fees and compact blocks having a significant improvement on propagation.

sdaftuar

Concept ACK, left a couple comments, plus I agree with @sipa's comment about preventing use of both -blockmaxweight and -blockmaxsize.

sdaftuar · 2017-08-24T20:32:04Z

src/miner.cpp

@@ -241,19 +230,11 @@ bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost
 // - serialized size (in case -blockmaxsize is in use)
 bool BlockAssembler::TestPackageTransactions(const CTxMemPool::setEntries& package)
 {
-    uint64_t nPotentialBlockSize = nBlockSize; // only used with fNeedSizeAccounting


The comment above needs to be updated to remove reference to serialized size.

sdaftuar · 2017-08-24T20:37:48Z

src/policy/policy.h

 /** Default for -blockmaxweight, which controls the range of block weights the mining code will create **/
-static const unsigned int DEFAULT_BLOCK_MAX_WEIGHT = 3000000;
+static const unsigned int DEFAULT_BLOCK_MAX_WEIGHT = MAX_BLOCK_WEIGHT - 4000;


Isn't the - 4000 unnecessary here? We already separately reserve space for the coinbase in CreateNewBlock.

We've always had it and I'm skeptical of just removing it...I could absolutely see people accidentally building invalid blocks if we just remove it and they dont notice (which, given how much people appear to read release notes.......)

DEFAULT_BLOCK_MAX_WEIGHT is used to initialise the nBlockMaxWeight field in BlockAssembler::Options, but that's then used as BlockAssembler::nBlockMaxWeight = max(4000, min(MAX_BLOCK_WEIGHT-4000, options.nBlockMaxWeight)) in the BlockAssembler constructor, so it will already be trimmed.

So I think the "- 4000" here is a no op since min(MBW-4000,MBW) = min(MBW-4000,MBW-4000) = MBW-4000...

Ah, I see your point. Still, it feels misleading to print in the help text that the default value is 4000000 if thats outside the acceptable range.

Yeah actually what I meant was that we also start off our block counter at a weight of 4000, so limiting to MAX_BLOCK_WEIGHT-4000 (both here and in BlockAssembler::BlockAssembler()) causes us to limit block sizes more than we probably intend. But not necessary to change here.

luke-jr · 2017-08-24T20:49:16Z

There are real use-cases to set both blockmaxsize and blockmaxweight: one might want to mine a block that has at most a weight of 3M, but never more than 1 MB for example.

jtimon · 2017-08-24T23:30:02Z

As commented on IRC blockmaxsize doesn't make sense after segwit activation. I wouldn't even bother with the deprecation step, I would go straight to removal and just document it in the release notes once.
But if we're going to maintain both simultaneously for any longer I agree the 2 constants for the defaults and forbidding using both at the same time (ie like you can't use -testnet and -regtest at once) is the less insane option.
Do we have more examples of defaults depending on selections of other options?
If so, I think we should get rid of all of them independently of this PR.

Miners have in recent years maxed out 1 MB. That is completely different from 4 MB, and does not prove they will increase to 4 MB immediately in any sense.

I will treat any arguments regarding defaults as if they were arguments for a lower value for DEFAULT_BLOCK_MAX_WEIGHT, not as arguments for the removal/deprecation of blockmaxsize in itself.
Regarding all those arguments, here's all I have to say: I trust miners to end up doing what's economically better for them, but I'm not opposed to a very low default for DEFAULT_BLOCK_MAX_WEIGHT. I think that will only prove that the default won't matter at all unless it's economically optimal or close: otherwise it'll be a few accidental bad blocks, perhaps the lower the default the faster they will read about the option.
I'm also not opposed to saving the unnecessary disruption, since as others have argued that would only change their default to something unexpected.

Concept ACK

jnewbery · 2017-08-25T02:18:36Z

Concept ACK. Will review soon.

promag · 2017-08-25T03:21:58Z

src/miner.cpp

        if (!fWeightSet) {
-            options.nBlockMaxWeight = options.nBlockMaxSize * WITNESS_SCALE_FACTOR;
+            options.nBlockMaxWeight = gArgs.GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_WEIGHT / WITNESS_SCALE_FACTOR) * WITNESS_SCALE_FACTOR;


Add warn about deprecation?

Agree. A deprecation warning here would be helpful.

I'd prefer to not add more garbage to init, if possible (and printing every time a BlockAssembler is created also sucks, though it will do that now if you set both :( ). I think its fine, and since we're changing the defaults to something reasonable, removing the option a release later likely won't break anything.

Then add the LogPrintf()s to InitParameterInteraction() in init.cpp instead.

promag · 2017-08-25T03:25:06Z

src/rpc/mining.cpp

@@ -214,7 +214,6 @@ UniValue getmininginfo(const JSONRPCRequest& request)

    UniValue obj(UniValue::VOBJ);
    obj.push_back(Pair("blocks",           (int)chainActive.Height()));
-    obj.push_back(Pair("currentblocksize", (uint64_t)nLastBlockSize));


Breaking change, needs release note.

yes, this should be included in release notes.

Also, please remove currentblocksize from help text (currently L198)

Done and done.

jnewbery

utACK. A couple of nits inline.

I think the second commit is good, but needs to be very explicitly communicated to users. Defaults changing unexpectedly under users' feet is a bad experience, even if the new default is better.

Deprecation of blockmaxsize should be swiftly followed by removal in the next release.

Side note: how does this not break any tests?! Do we not have any testing for mining size/weight accounting?

jnewbery · 2017-08-25T14:37:00Z

src/rpc/mining.cpp

@@ -214,7 +214,6 @@ UniValue getmininginfo(const JSONRPCRequest& request)

    UniValue obj(UniValue::VOBJ);
    obj.push_back(Pair("blocks",           (int)chainActive.Height()));
-    obj.push_back(Pair("currentblocksize", (uint64_t)nLastBlockSize));


yes, this should be included in release notes.

Also, please remove currentblocksize from help text (currently L198)

jnewbery · 2017-08-25T14:56:42Z

src/miner.cpp

@@ -175,7 +167,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
    int64_t nTime1 = GetTimeMicros();

    nLastBlockTx = nBlockTx;
-    nLastBlockSize = nBlockSize;


There's a call below to GetSerializeSize() for logging the block size. We should remove that potentially expensive call and update the log to not print size as part of this PR.

jnewbery · 2017-08-25T15:00:22Z

src/miner.cpp

        if (!fWeightSet) {
-            options.nBlockMaxWeight = options.nBlockMaxSize * WITNESS_SCALE_FACTOR;
+            options.nBlockMaxWeight = gArgs.GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_WEIGHT / WITNESS_SCALE_FACTOR) * WITNESS_SCALE_FACTOR;


Agree. A deprecation warning here would be helpful.

jnewbery · 2017-08-25T15:48:04Z

doc/release-notes.md

+implied blockmaxweight, instead of limiting block size directly. Any miners who wish
+to limit their blocks by size, instead of by weight, will have to do so manually by
+removing transactions from their block template directly. The "currentblocksize" value
+in getmininginfo has been removed.


This is good, but I think the removal of currentblocksize in getmininginfo should [also?] go in a Low-level RPC changes section.

jnewbery · 2017-08-25T16:35:16Z

utACK faeafe2. Looks great.

sipa · 2017-08-25T16:39:40Z

utACK faeafe2

luke-jr · 2017-08-25T17:52:29Z

Again, hard NACK for all the unrefuted reasons already given.

sipa · 2017-09-11T18:11:46Z

@TheBlueMatt mining.py fails (conflict introduced by #11150).

* This removes block-size-limiting code in favor of GBT clients doing the limiting themselves (if at all). * -blockmaxsize is deprecated and only used to calculate an implied blockmaxweight, addressing confusion from multiple users. * getmininginfo's currentblocksize return value was returning garbage values, and has been removed, also removing a GetSerializeSize call in some block generation inner loops and potentially addressing some performance edge cases.

No sensible user will ever keep the default settings here, so not having sensible defaults only serves to screw users who are paying less attention, which makes for terrible defaults.

TheBlueMatt · 2017-09-11T19:52:06Z

Rebased and fixed conflict.

…lt to throwing away money 6f703e9 Add release notes describing blockmaxweight deprecation (Matt Corallo) 3dc263c Use a sensible default for blockmaxweight (Matt Corallo) ba206d2 Deprecate confusing blockmaxsize, fix getmininginfo output (Matt Corallo) Pull request description: No sensible user will ever keep the default settings here, so not having sensible defaults only serves to screw users who are paying less attention, which makes for terrible defaults. Additionally, support for block-size-limiting directly has been removed: * This removes block-size-limiting code in favor of GBT clients doing the limiting themselves (if at all). * -blockmaxsize is deprecated and only used to calculate an implied blockmaxweight, addressing confusion from multiple users. * getmininginfo's currentblocksize return value was returning garbage values, and has been removed, also removing a GetSerializeSize call in some block generation inner loops and potentially addressing some performance edge cases. Tree-SHA512: 33010540faf5d6225ad575488b804e180a8d53a41be484ca2932a0485595e28da62f0ade4b279a6bf1c947c7ce389f51fde8651b2ba25deb25e766e0813b993c

luke-jr · 2017-09-19T03:13:03Z

I do think it should be project policy to focus on features that matter. And an option that everyone overrides serves nobody.

That assumes everyone sets it to the absolute no-op maximum. There is no evidence of this.

This PR is contentious, should not have been merged, and should be reverted.

sdaftuar · 2017-10-26T15:15:47Z

I think this should be tagged for backport.

gmaxwell · 2017-11-02T06:48:50Z

ACK for backport.

maflcko · 2017-11-02T17:00:31Z

Moved to 0.15.0.2 milestone for the convenience of miners.

* This removes block-size-limiting code in favor of GBT clients doing the limiting themselves (if at all). * -blockmaxsize is deprecated and only used to calculate an implied blockmaxweight, addressing confusion from multiple users. * getmininginfo's currentblocksize return value was returning garbage values, and has been removed, also removing a GetSerializeSize call in some block generation inner loops and potentially addressing some performance edge cases. Github-Pull: bitcoin#11100 Rebased-From: ba206d2

No sensible user will ever keep the default settings here, so not having sensible defaults only serves to screw users who are paying less attention, which makes for terrible defaults. Github-Pull: bitcoin#11100 Rebased-From: 3dc263c

Github-Pull: bitcoin#11100 Rebased-From: 6f703e9

TheBlueMatt changed the title ~~Use a sensible default for blockmax{size,weight}~~ Dont default to throwing away money for blockmax{size,weight} Aug 23, 2017

TheBlueMatt changed the title ~~Dont default to throwing away money for blockmax{size,weight}~~ Fix confusing blockmax{size,weight} options, dont default to throwing away money Aug 24, 2017

sipa reviewed Aug 24, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-08-sane-default-limits branch from 8893ed7 to 10d21dc Compare August 24, 2017 18:43

sipa reviewed Aug 24, 2017

View reviewed changes

sdaftuar reviewed Aug 24, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-08-sane-default-limits branch from 10d21dc to 21e1f07 Compare August 24, 2017 21:34

promag reviewed Aug 25, 2017

View reviewed changes

jnewbery reviewed Aug 25, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-08-sane-default-limits branch 2 times, most recently from 9bf4197 to 2fada8c Compare August 25, 2017 15:31

jnewbery reviewed Aug 25, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-08-sane-default-limits branch from 2fada8c to faeafe2 Compare August 25, 2017 16:06

TheBlueMatt added 3 commits September 11, 2017 15:51

Use a sensible default for blockmaxweight

3dc263c

No sensible user will ever keep the default settings here, so not having sensible defaults only serves to screw users who are paying less attention, which makes for terrible defaults.

Add release notes describing blockmaxweight deprecation

6f703e9

TheBlueMatt force-pushed the 2017-08-sane-default-limits branch from faeafe2 to 6f703e9 Compare September 11, 2017 19:51

sipa merged commit 6f703e9 into bitcoin:master Sep 11, 2017

maflcko added the Needs backport label Oct 28, 2017

maflcko added this to the 0.15.1 milestone Oct 28, 2017

maflcko modified the milestones: 0.15.1, 0.15.0.2 Nov 2, 2017

morcos mentioned this pull request Nov 2, 2017

0.15: Backports #11592

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 2, 2017

Add release notes describing blockmaxweight deprecation

bb83fe1

Github-Pull: bitcoin#11100 Rebased-From: 6f703e9

maflcko removed the Needs backport label Nov 2, 2017

ajtowns mentioned this pull request Mar 7, 2018

blockmaxsize parameter has no effect #12640

Closed

This was referenced Mar 23, 2018

[config] Remove blockmaxsize option #12756

Merged

[tests] Fix block size config in feature_fee_estimation.py, feature_maxuploadtarget.py and feature_pruning.py #12768

Closed

ariard mentioned this pull request May 14, 2021

Duplicate coinbase transaction space reservation in CreateNewBlock #21950

Closed

Edgaralcaraz1 mentioned this pull request May 18, 2021

. #21986

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Fix confusing blockmax{size,weight} options, dont default to throwing away money #11100

Fix confusing blockmax{size,weight} options, dont default to throwing away money #11100

Uh oh!

Conversation

TheBlueMatt commented Aug 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Aug 20, 2017

Uh oh!

laanwj commented Aug 21, 2017

Uh oh!

gmaxwell commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janbraiins commented Aug 24, 2017

Uh oh!

gmaxwell commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Aug 24, 2017

Uh oh!

luke-jr commented Aug 24, 2017

Uh oh!

jameshilliard commented Aug 24, 2017

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Aug 24, 2017

Uh oh!

jtimon commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Aug 20, 2017 •

edited

Loading

gmaxwell commented Aug 23, 2017 •

edited

Loading

laanwj commented Aug 23, 2017 •

edited

Loading

gmaxwell commented Aug 24, 2017 •

edited

Loading

jtimon commented Aug 24, 2017 •

edited

Loading

sipa commented Sep 11, 2017 •

edited

Loading