Require configuration of mining before creating blocks

luke-jr commented at 11:37 pm on November 10, 2013: member

As discussed on IRC, this removes the “defaults” miners often use as an excuse to not make mining decisions. getblocktemplate is disabled unless bitcoind is explicitly configured with mining settings.

in src/rpcmining.cpp: in b0c2b169a2 outdated

39+{
40+    return
41+        mapArgs.count("-blockminsize") &&
42+        mapArgs.count("-blockmaxsize") &&
43+        mapArgs.count("-blockprioritysize") &&
44+    true;

PRabahy commented at 2:24 am on November 11, 2013:

I’m not a C++ programmer, I don’t see why this last “&& true” exists.

laanwj commented at 6:58 am on November 11, 2013:

I am a C++ programmer but… huh :pig: :koala: :cow: Thinking about it it may be a way to temporary switch the result for testing, like #if 0. In any case, it shouldn’t be there in the final code.

luke-jr commented at 7:23 pm on November 11, 2013:

It makes the list of arguments consistent per-line.

Diapolo commented at 8:07 pm on November 11, 2013:

My vote, don’t do this…

in src/miner.cpp: in b0c2b169a2 outdated

163@@ -164,13 +164,13 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
164     pblocktemplate->vTxSigOps.push_back(-1); // updated at end
165 
166     // Largest block you're willing to create:
167-    unsigned int nBlockMaxSize = GetArg("-blockmaxsize", MAX_BLOCK_SIZE_GEN/2);
168+    unsigned int nBlockMaxSize = GetArg("-blockmaxsize", MAX_BLOCK_SIZE);

laanwj commented at 10:27 am on November 11, 2013:

Why are you changing these defaults, if you don’t want to allow using defaults? (The new value is MAX_BLOCKSIZE (1000000) whereas the old value was MAX_BLOCKSIZE/4 (250000))

luke-jr commented at 7:22 pm on November 11, 2013:

GetArg doesn’t allow omitting defaults, and leaving them in main.h is unclear.

jtimon commented at 0:24 am on July 23, 2015:

Maybe we should create GetMandatoryArg versions that throw exceptions or assert-fail when required values are not provided ? If you do this, please use the global argMap explictly as a const ref argument in the new versions, like in #6423

laanwj commented at 11:08 am on November 11, 2013: member

I’d propose to implement this slightly differently, more straightforward: set the default to invalid values (such as -1 or MAXINT or 0, whatever is most useless) then check for valid values in IsMiningSetup() instead of probing the arguments.

luke-jr commented at 7:23 pm on November 11, 2013: member

That’d probably make it clearer too, okay…

luke-jr commented at 8:02 pm on November 11, 2013: member

@laanwj That’d need making the variables globals again.. so maybe this is better after all?

laanwj commented at 8:33 pm on November 11, 2013: member

Ugh, yes I now see that CreateNewBlock also queries and parse the arguments every time. That’s the same as using globals, just in a hidden way. At least I now understand your reasoning for doing it that way in IsMiningSetup.

mikehearn commented at 5:34 pm on November 12, 2013: contributor

The obvious downside of this approach being ….. once a miner has set them, we lose the ability to select smarter defaults, because whatever they pick could end up being used indefinitely.

luke-jr commented at 6:40 pm on November 12, 2013: member

“We” shouldn’t be setting defaults anyway. That’s the point.

gmaxwell commented at 6:54 pm on November 12, 2013: contributor

If we think that the common settings become wrong for an option there is always the possibility of just renaming the option to force resetting it.

luke-jr commented at 2:09 pm on December 3, 2013: member

Rebased without “&& true”

BitcoinPullTester commented at 2:55 pm on December 3, 2013: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/03528791ea2b870595723642dbac74bfa4f0923e for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 11:43 am on December 8, 2013: member

I think this needs a documentation update too. Miners upgrading to 0.9.0 will have no idea what to set the defaults to, so the documentation could suggest some sane default values (I get the irony here) and to sources of information with regard to setting the values. We wouldn’t want them to just put some nonsense values.

jgarzik commented at 0:17 am on December 13, 2013: contributor

NAK

This is fundamentally “force X to think” which is unfriendly. We want miners to be thinking about this, agreed; But I see nothing wrong and much good by continuing to provide a sane default mining policy. Let’s continue to play nice with our miner friends.

laanwj commented at 11:51 am on January 10, 2014: member

Seems consensus is against this (at least in the current state), so closing for now.

laanwj closed this on Jan 10, 2014

luke-jr commented at 7:02 am on February 5, 2014: member

Huh? Most people seemed to support this from the comments… Just realised this was closed without merging :(

laanwj reopened this on Jul 23, 2015

luke-jr force-pushed on Jul 23, 2015

luke-jr commented at 3:48 pm on July 23, 2015: member

Rebased

gavinandresen commented at 4:01 pm on July 23, 2015: contributor

This probably breaks a bunch of qa/rpc-tests (which run -regtest and which rely on defaults).

jgarzik commented at 4:16 pm on July 23, 2015: contributor

The same objections remain…

luke-jr commented at 4:20 pm on July 23, 2015: member

@jgarzik This does give a randomised selection of not-unreasonable settings.

jgarzik commented at 4:33 pm on July 23, 2015: contributor

@luke-jr AFAICS this change requires miners to have something set in their configs, which they may not have set before, by checking mapArgs.count()

Thus it breaks e.g. every miner, including p2pool miners, which do not have those settings in their config file.

Am I missing something?

Require configuration of mining before creating blocks 96bf230940

luke-jr force-pushed on Jul 23, 2015

luke-jr commented at 6:13 pm on July 23, 2015: member

@jgarzik No, that sounds about right. It breaks miners who have thus far been too negligent to configure their node, until they do so. @gavinandresen Fixed QA tests by skipping config check on regtest/testnet.

jgarzik commented at 6:17 pm on July 23, 2015: contributor

NAK maintained.

This would seem to increase the barrier to entry for p2pool etc. miners and knowingly break currently-working configurations.

in src/rpcmining.cpp: in 96bf230940

57+    unsigned long maxsz = std::max((unsigned long)250000, minsz);
58+    maxsz = maxsz + (insecure_rand() % (MAX_BLOCK_SIZE - maxsz));
59+    unsigned long prisz = insecure_rand() % MAX_BLOCK_SIZE;
60+    unsigned long datacarrier = insecure_rand() % 2;
61+    unsigned long datacarriersize = insecure_rand() % 0x100;
62+    throw JSONRPCError(RPC_NOT_CONFIGURED, strprintf(

jtimon commented at 2:11 am on July 24, 2015:

Can’t you just use the defaults here and log a warning when the miner hasn’t configured certain options instead of throwing a JSONRPCError for now? @jgarzik would that be acceptable?

jgarzik commented at 5:48 am on July 24, 2015: contributor

Just seems like way too many problems or oddities with this patch:

Raises the bar for getting p2pool working: Breaks current documentation + requires users to perform lots of research vs. simply taking a useful default.
Breaks existing, working configs.
Picking random values for key settings is just odd, and runs counter to normal engineering practice. Less deterministic, makes testing more difficult, and more.

More generally, “pushy” changes that halt software operation until the user educates themselves to a higher degree are distasteful. Bullying the user into learning the intricacies of mining policy is not the way to go.

It is a lot of headache just to avoid providing a sane default.

luke-jr commented at 5:36 am on January 28, 2016: member

Closing due to inactivity.

luke-jr closed this on Jan 28, 2016

laanwj commented at 9:58 am on January 28, 2016: member

Yes, while I philosophically agree with this pull, and I’d rather get rid of any unnecessary default values any day, it’s so inconvenient to miners that we can’t really merge it.

Bushstar referenced this in commit 2ae1ce4800 on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021

Require configuration of mining before creating blocks #3229