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

TheBlueMatt commented at 9:54 pm on August 20, 2017: member

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 commented at 9:59 pm on August 20, 2017: member

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

laanwj commented at 7:59 am on August 21, 2017: member

Please, not again, no more defaults fighting…

gmaxwell commented at 0:17 am on August 23, 2017: contributor

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 commented at 10:31 am on August 23, 2017: member

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

Increase DEFAULT_BLOCK_MAX_SIZE to 1MB #6231
Increase DEFAULT_BLOCK_MAX_SIZE to 1MB #7622
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.

TheBlueMatt renamed this:
~~Use a sensible default for blockmax{size,weight}~~
Dont default to throwing away money for blockmax{size,weight}
on Aug 23, 2017

janbraiins commented at 6:10 am on August 24, 2017: contributor

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 commented at 8:53 am on August 24, 2017: contributor

@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 renamed this:
~~Dont default to throwing away money for blockmax{size,weight}~~
Fix confusing blockmax{size,weight} options, dont default to throwing away money
on Aug 24, 2017

TheBlueMatt commented at 3:42 pm on August 24, 2017: member

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.

in src/policy/policy.h:20 in 8893ed7786 outdated

16@@ -17,9 +17,9 @@ class CCoinsViewCache;
17 class CTxOut;
18 
19 /** Default for -blockmaxsize, which controls the maximum size of block the mining code will create **/
20-static const unsigned int DEFAULT_BLOCK_MAX_SIZE = 750000;
21+static const unsigned int DEFAULT_BLOCK_MAX_SIZE = MAX_BLOCK_SERIALIZED_SIZE - 1000;

sipa commented at 4:16 pm on August 24, 2017:

delete instead?

TheBlueMatt commented at 6:43 pm on August 24, 2017:

Done.

TheBlueMatt force-pushed on Aug 24, 2017

meshcollider commented at 7:25 pm on August 24, 2017: contributor

Concept ACK as discussed in meeting

in src/miner.cpp:93 in 10d21dce79 outdated

90     if (gArgs.IsArgSet("-blockmaxsize")) {
91-        options.nBlockMaxSize = gArgs.GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_SIZE);
92         if (!fWeightSet) {
93-            options.nBlockMaxWeight = options.nBlockMaxSize * WITNESS_SCALE_FACTOR;
94+            options.nBlockMaxWeight = gArgs.GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_WEIGHT / WITNESS_SCALE_FACTOR) * WITNESS_SCALE_FACTOR;
95         }

sipa commented at 7:27 pm on August 24, 2017:

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

TheBlueMatt commented at 9:35 pm on August 24, 2017:

Done.

morcos commented at 7:33 pm on August 24, 2017: member

Concept ACK

luke-jr commented at 7:37 pm on August 24, 2017: member

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 commented at 8:22 pm on August 24, 2017: contributor

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.

in src/miner.cpp:245 in 44dfcd23ac outdated

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

sdaftuar commented at 8:32 pm on August 24, 2017:

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

TheBlueMatt commented at 9:35 pm on August 24, 2017:

Fixed.

in src/policy/policy.h:20 in 10d21dce79 outdated

15@@ -16,10 +16,8 @@
16 class CCoinsViewCache;
17 class CTxOut;
18 
19-/** Default for -blockmaxsize, which controls the maximum size of block the mining code will create **/
20-static const unsigned int DEFAULT_BLOCK_MAX_SIZE = 750000;
21 /** Default for -blockmaxweight, which controls the range of block weights the mining code will create **/
22-static const unsigned int DEFAULT_BLOCK_MAX_WEIGHT = 3000000;
23+static const unsigned int DEFAULT_BLOCK_MAX_WEIGHT = MAX_BLOCK_WEIGHT - 4000;

sdaftuar commented at 8:37 pm on August 24, 2017:

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

TheBlueMatt commented at 9:30 pm on August 24, 2017:

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…….)

ajtowns commented at 3:48 pm on August 25, 2017:

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…

TheBlueMatt commented at 4:07 pm on August 25, 2017:

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.

sdaftuar commented at 6:47 pm on August 30, 2017:

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.

sdaftuar commented at 8:41 pm on August 24, 2017: member

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

luke-jr commented at 8:49 pm on August 24, 2017: member

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.

TheBlueMatt force-pushed on Aug 24, 2017

jtimon commented at 11:30 pm on August 24, 2017: contributor

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 commented at 2:18 am on August 25, 2017: member

Concept ACK. Will review soon.

in src/miner.cpp:90 in 21e1f070ee outdated

89     }
90     if (gArgs.IsArgSet("-blockmaxsize")) {
91-        options.nBlockMaxSize = gArgs.GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_SIZE);
92         if (!fWeightSet) {
93-            options.nBlockMaxWeight = options.nBlockMaxSize * WITNESS_SCALE_FACTOR;
94+            options.nBlockMaxWeight = gArgs.GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_WEIGHT / WITNESS_SCALE_FACTOR) * WITNESS_SCALE_FACTOR;

promag commented at 3:21 am on August 25, 2017:

Add warn about deprecation?

jnewbery commented at 3:00 pm on August 25, 2017:

Agree. A deprecation warning here would be helpful.

TheBlueMatt commented at 3:01 pm on August 25, 2017:

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.

jnewbery commented at 3:45 pm on August 25, 2017:

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

TheBlueMatt commented at 4:07 pm on August 25, 2017:

Ugh, fine.

in src/rpc/mining.cpp:218 in 21e1f070ee outdated

213@@ -214,7 +214,6 @@ UniValue getmininginfo(const JSONRPCRequest& request)
214 
215     UniValue obj(UniValue::VOBJ);
216     obj.push_back(Pair("blocks",           (int)chainActive.Height()));
217-    obj.push_back(Pair("currentblocksize", (uint64_t)nLastBlockSize));

promag commented at 3:25 am on August 25, 2017:

Breaking change, needs release note.

jnewbery commented at 2:37 pm on August 25, 2017:

yes, this should be included in release notes.

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

TheBlueMatt commented at 3:31 pm on August 25, 2017:

Done and done.

in src/miner.cpp:179 in 21e1f070ee outdated

166@@ -175,7 +167,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
167     int64_t nTime1 = GetTimeMicros();
168 
169     nLastBlockTx = nBlockTx;
170-    nLastBlockSize = nBlockSize;

jnewbery commented at 2:56 pm on August 25, 2017:

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.

TheBlueMatt commented at 3:31 pm on August 25, 2017:

Removed.

jnewbery commented at 3:07 pm on August 25, 2017: member

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?

TheBlueMatt force-pushed on Aug 25, 2017

in doc/release-notes.md:70 in 2fada8c712 outdated

65+well as non-optimal fee selection and ever-so-slightly worse performance, and has thus
66+now been deprecated. Further, the blockmaxsize option is now used only to calculate an
67+implied blockmaxweight, instead of limiting block size directly. Any miners who wish
68+to limit their blocks by size, instead of by weight, will have to do so manually by
69+removing transactions from their block template directly. The "currentblocksize" value
70+in getmininginfo has been removed.

jnewbery commented at 3:48 pm on August 25, 2017:

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

TheBlueMatt commented at 4:07 pm on August 25, 2017:

Done.

TheBlueMatt force-pushed on Aug 25, 2017

jnewbery commented at 4:35 pm on August 25, 2017: member

utACK faeafe2f20fa046f8a974940b42968dff05cd89c. Looks great.

sipa commented at 4:39 pm on August 25, 2017: member

utACK faeafe2f20fa046f8a974940b42968dff05cd89c

luke-jr commented at 5:52 pm on August 25, 2017: member

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

sdaftuar commented at 6:46 pm on August 30, 2017: member

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. @luke-jr I am doubtful that what you describe is a “real” use case, but even if we accept that assumption – you haven’t responded to @TheBlueMatt’s observation that this can be done by gbt callers simply by truncating the transaction list to the desired serialized size.

More broadly, the -blockmaxsize option is deceptive and confusing to users, since the code doesn’t try to optimize for fee-per-serialized-byte. If someone really wants to mine based on number of serialized bytes (which I doubt), they should use different software; this software is optimized for fee-per-weight.

Anyone is free to work on software that optimizes for other properties, but not in this project please; we’ve gone to considerable effort to optimize for miner income given the consensus rules. This helps level the playing field amongst miners, so that one needn’t be a software engineer in order to be competitive. I think this is obviously good for the network.

ACK faeafe2.

nit: Perhaps edit the commit message on that last commit to say Add release notes describing blockmaxsize deprecation rather than ...blockmaxweight deprecation....

luke-jr commented at 6:55 am on September 2, 2017: member

Outsourcing this to callers is obviously unreasonable.

More broadly, the -blockmaxsize option is deceptive and confusing to users, since the code doesn’t try to optimize for fee-per-serialized-byte. If someone really wants to mine based on number of serialized bytes (which I doubt), they should use different software; this software is optimized for fee-per-weight.

It’s not deceptive at all. It limits the max size of blocks produced. It makes no statement on what policy is used to select transactions, at all.

Anyone is free to work on software that optimizes for other properties, but not in this project please; we’ve gone to considerable effort to optimize for miner income given the consensus rules.

There is no project policy to only optimise for miner income, nor should there be. The incentives on Bitcoin mining are clearly broken, and we should encourage behaviour beneficial to the network at least until such a time as those incentives are no longer broken. To do otherwise is suicide.

sipa commented at 6:00 pm on September 11, 2017: member

It’s not deceptive at all. It limits the max size of blocks produced.

At the very least, ‘size’ is ambiguous. At least some people believed it refers to stripped size, and feared setting it higher than 1M.

And there is no need for that extra complication. We don’t have options to limit the number of sigops, the maximum number of inputs, or the maximum change in UTXO set size. I believe all of these are more relevant to network health than serialized size of blocks. But I don’t see anyone arguing that functionality should be built in to let GBT restrict by those. Why? Because nobody will use an option that makes them actively lose money, and we shouldn’t expect them to.

The same is true for size now. With SegWit, the constrained resource is weight.

There is no project policy to only optimise for miner income, nor should there be.

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

sipa commented at 6:11 pm on September 11, 2017: member

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

Deprecate confusing blockmaxsize, fix getmininginfo output

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

ba206d2c63

Use a sensible default for blockmaxweight

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.

3dc263c9b9

Add release notes describing blockmaxweight deprecation 6f703e9bf1

TheBlueMatt force-pushed on Sep 11, 2017

TheBlueMatt commented at 7:52 pm on September 11, 2017: member

Rebased and fixed conflict.

sipa merged this on Sep 11, 2017

sipa closed this on Sep 11, 2017

sipa referenced this in commit 1afc22a766 on Sep 11, 2017

luke-jr commented at 3:13 am on September 19, 2017: member

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 commented at 3:15 pm on October 26, 2017: member

I think this should be tagged for backport.

MarcoFalke added the label Needs backport on Oct 28, 2017

MarcoFalke added this to the milestone 0.15.1 on Oct 28, 2017

gmaxwell commented at 6:48 am on November 2, 2017: contributor

ACK for backport.

MarcoFalke removed this from the milestone 0.15.1 on Nov 2, 2017

MarcoFalke added this to the milestone 0.15.0.2 on Nov 2, 2017

MarcoFalke commented at 5:00 pm on November 2, 2017: member

Moved to 0.15.0.2 milestone for the convenience of miners.

MarcoFalke referenced this in commit 8baec2f266 on Nov 2, 2017

MarcoFalke referenced this in commit b84429b9db on Nov 2, 2017

MarcoFalke referenced this in commit 308eb3d08f on Nov 2, 2017

MarcoFalke referenced this in commit 7871a7d3be on Nov 2, 2017

MarcoFalke referenced this in commit 4c82cea99b on Nov 2, 2017

MarcoFalke referenced this in commit bb83fe1902 on Nov 2, 2017

MarcoFalke removed the label Needs backport on Nov 2, 2017

DrahtBot locked this on Sep 8, 2021

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