Use constants and minor fixes by luke-jr #6905

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:lukejr-constants-no-mergeConf changing 7 files +27 −16

MarcoFalke commented at 6:28 PM on October 29, 2015: member

Not sure if #6349 finds its way out of rebase hell anytime soon. Those three commits can be merged without rebase (I preserved the commit hashes)

They already got some sort of code review as part of #6349 so it's better to merge them without rebase, imo.
Bugfix: RPC: blockchain: Display correct defaults in help for verifychain method caa3d42f06
Bugfix: Describe dblogsize option correctly (it refers to the wallet database, not memory pool) 420a82f1ae
Bugfix: If genproclimit is omitted to RPC setgenerate, don't change it; also show correct default in getmininginfo 5f9260f458
MarcoFalke closed this on Oct 29, 2015
MarcoFalke deleted the branch on Oct 29, 2015
in src/rpcmining.cpp:None in 5f9260f458 outdated
```
 210 | @@ -211,7 +211,7 @@ UniValue setgenerate(const UniValue& params, bool fHelp)
 211 |      if (params.size() > 0)
 212 |          fGenerate = params[0].get_bool();
 213 |  
 214 | -    int nGenProcLimit = -1;
 215 | +    int nGenProcLimit = GetArg("-genproclimit", DEFAULT_GENERATE_THREADS);
```
jtimon commented at 6:54 PM on October 29, 2015:

Why this changes from -1 to GetArg("-genproclimit", DEFAULT_GENERATE_THREADS) but in getmininginfo() the change is from -1 to DEFAULT_GENERATE_THREADS?

MarcoFalke commented at 7:08 PM on October 29, 2015:

You still want to obey the command line arg if nothing is set via rpc.

jtimon commented at 10:30 AM on October 30, 2015:

Is it a bugfix or enhanced functionality? Again, why one uses GetArg() but the other doesn't?

MarcoFalke commented at 10:39 AM on October 30, 2015:

Yes, it fixes one bug: If genproclimit is omitted to RPC setgenerate, don't change it; (copied from commit msg)

MarcoFalke commented at 10:41 AM on October 30, 2015:

Making getmininginfo push back the "correct" value nGenProcLimit may involve some more work. But at least this PR did not introduce the issue. The PR is not perfect, but improves things considerably.

jtimon commented at 10:45 AM on October 30, 2015:

Thank you. Everything else was completely trivial to review, but I thought this deserved a more detailed explanation.

in src/rpcblockchain.cpp:None in 5f9260f458 outdated

 546 |              "verifychain ( checklevel numblocks )\n"
 547 |              "\nVerifies blockchain database.\n"
 548 |              "\nArguments:\n"
 549 | -            "1. checklevel   (numeric, optional, 0-4, default=3) How thorough the block verification is.\n"
 550 | -            "2. numblocks    (numeric, optional, default=288, 0=all) The number of blocks to check.\n"
 551 | +            "1. checklevel   (numeric, optional, 0-4, default=" + strprintf("%d", nCheckLevel) + ") How thorough the block verification is.\n"

jonasschnelli commented at 7:17 PM on October 29, 2015:

hmm... is "default" the correct context here? Because it's no longer the default value if you have provided a different value over the command line argument. Maybe current default value?

jtimon commented at 10:31 AM on October 30, 2015:

The default value means "the default value unless you set it via command line argument".

luke-jr commented at 2:23 PM on October 30, 2015:

"default" means whatever you get if you don't provide it.

jonasschnelli commented at 7:22 PM on October 29, 2015: contributor

I think this is a good idea. utACK.

Miner and Wallet init help strings could also slowly migrate into wallet/ miner/ classes. I once did this for the wallet, but it requires rebase and maybe more small, reviewable PRs: #5990
MarcoFalke reopened this on Oct 29, 2015
Bugfix: Omit wallet-related options from -help when wallet is disabled a6efc01908
MarcoFalke force-pushed on Oct 29, 2015
dcousens commented at 5:55 AM on October 30, 2015: contributor

utACK
jtimon commented at 10:32 AM on October 30, 2015: contributor

utACK besides my nit/question.
laanwj added the label Refactoring on Oct 30, 2015

in src/init.cpp:None in a6efc01908

 403 | @@ -404,12 +404,16 @@ std::string HelpMessage(HelpMessageMode mode)
 404 |      if (showDebug)
 405 |      {
 406 |          strUsage += HelpMessageOpt("-checkpoints", strprintf("Disable expensive verification for known chain history (default: %u)", 1));
 407 | -        strUsage += HelpMessageOpt("-dblogsize=<n>", strprintf("Flush database activity from memory pool to disk log every <n> megabytes (default: %u)", 100));
 408 | +#ifdef ENABLE_WALLET
 409 | +        strUsage += HelpMessageOpt("-dblogsize=<n>", strprintf("Flush wallet database activity from memory to disk log every <n> megabytes (default: %u)", DEFAULT_WALLET_DBLOGSIZE));

laanwj commented at 10:39 AM on November 4, 2015:

Good find on missing ENABLE_WALLET here

MarcoFalke commented at 10:48 AM on November 4, 2015:

All credit goes to @luke-jr ;)

laanwj commented at 10:40 AM on November 4, 2015: member

utACK
laanwj merged this on Nov 4, 2015
laanwj closed this on Nov 4, 2015
laanwj referenced this in commit aca0c00ae1 on Nov 4, 2015
luke-jr referenced this in commit f7b8ee551c on Nov 18, 2015
luke-jr referenced this in commit d67c116ab1 on Nov 18, 2015
luke-jr referenced this in commit 965269dbf7 on Nov 18, 2015
luke-jr referenced this in commit 6f6b1d484c on Nov 18, 2015
luke-jr referenced this in commit 45f2e9d3d1 on Dec 8, 2015
luke-jr referenced this in commit 20b079b4b2 on Dec 8, 2015
luke-jr referenced this in commit b049e29c34 on Dec 8, 2015
luke-jr referenced this in commit ab99a5bb72 on Dec 8, 2015
zkbot referenced this in commit f1aeaec471 on Mar 21, 2018
zkbot referenced this in commit 4fc490c430 on Dec 4, 2019
zkbot referenced this in commit 868c63f92d on Dec 4, 2019
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6349 Constrain constant values to a single location in code