[Build] Adding --enable-mainnet configuration option for running mainnet

jeffrade commented at 1:50 am on January 30, 2018: contributor

Implementation based on comments/requirements in issue #11901 quoted below:

add a configuration option ‘–enable-mainnet’ which defaults to false
When true, behavior is unaffected (is set to true by the gitian builds).
When false (the default for someone cloning the repo, building with no configuration options), the resulting binary will require new ’enablemainnet’ option to launch the daemon or Qt GUI with main net.
This prevents developers or downstream system integrators from accidentally corrupting main net
If the configure option is used, mainnet should be the default.
Only if configured with –disable-mainnet (default) should a command-line option be required.

Fixes #11901

fanquake added the label Build system on Jan 30, 2018

in src/chainparamsbase.cpp:108 in 4a236e8b8d outdated

104     if (fTestNet)
105         return CBaseChainParams::TESTNET;
106-    return CBaseChainParams::MAIN;
107+    if (fMainNet)
108+        return CBaseChainParams::MAIN;
109+    return CBaseChainParams::TESTNET;

instagibbs commented at 2:33 am on January 30, 2018:

Is silently running testnet really the best solution? This is going to confuse the heck out of me :)

jeffrade commented at 2:49 am on January 30, 2018:

I agree, will be an adjustment for anyone accustomed to running on mainnet when building from source.

sipa commented at 2:50 am on January 30, 2018: member

If we’re doing this, I would prefer it if by default the client would fail to start up (with a message “mainnet disabled” or so), unless you explicitly tell it to use testnet or regtest.

jeffrade commented at 3:05 am on January 30, 2018: contributor

@sipa Instead of the final return statement in ChainNameFromCommandLine(), something like this below:

0throw std::runtime_error("You must specify a chain. -regtest, -testnet, or -mainnet.");

sipa commented at 3:14 am on January 30, 2018: member

@jeffrade Alternatively, you leave in the mainnet definition and logic, but in init.cpp check whether the chosen chain doesn’t happen to be mainnet when it’s not supposed to, and exit with a clean error. Exiting by throwing an exception is really only for exceptional situations.

laanwj commented at 8:54 am on January 30, 2018: member

I’d also prefer an explicit error (and exit) when running mainnet without --enable-mainnet - silently running on testnet is going to confuse the hell out of people and probably waste a lot of time.

in src/chainparamsbase.cpp:95 in 4a236e8b8d outdated

90@@ -91,12 +91,19 @@ std::string ChainNameFromCommandLine()
91 {
92     bool fRegTest = gArgs.GetBoolArg("-regtest", false);
93     bool fTestNet = gArgs.GetBoolArg("-testnet", false);
94+    bool fMainNet = gArgs.GetBoolArg("-mainnet", false);

laanwj commented at 9:01 am on January 30, 2018:

I don’t think you need to change chainparamsbase for this.

I’d say a simpler solution would be: add logic to one of the Init() functions to detect running of mainnet when not #ifdef ENABLE_MAINNET and then InitError() out.

jeffrade commented at 2:26 pm on January 30, 2018:

Makes sense. Will revert src/chainparamsbase.cpp and make requested changes in src/init.cpp today or tomorrow. Thanks!

MarcoFalke commented at 12:59 pm on January 30, 2018: member

Agree that InitError makes the most sense instead of silently assuming a chain/network.

jeffrade force-pushed on Jan 31, 2018

jeffrade renamed this:
~~[Tests] Adding --enable-mainnet configuration option for ChainParams~~
[Build] Adding --enable-mainnet configuration option for running mainnet
on Jan 31, 2018

jeffrade commented at 2:45 am on January 31, 2018: contributor

Pushed changes based on reviews from @sipa and @laanwj

jonasschnelli commented at 3:32 am on January 31, 2018: contributor

This prevents developers or downstream system integrators from accidentally corrupting main net

Is this true? Do we have evidence that downstream integrators do corrupt main net rather then giving some useful test cases how to make the p2p layer more robust?

Forgetting a configuration option (in this case –enable-mainnet) would lead to a not-in-production useful software. This is uncommon behaviour.

Codewise it looks good to me.

fanquake commented at 3:38 am on January 31, 2018: member

cc @maaku

laanwj commented at 10:06 am on January 31, 2018: member

Is this true? Do we have evidence that downstream integrators do corrupt main net rather then giving some useful test cases how to make the p2p layer more robust?

From what I understand from discussions in this direction is that it’s not about corrupting mainnet. It’s not the network at risk but the risk of running unreleased software in production. E.g. people (building from source) accidentally run master instead of a release branch, taking more risks with their coins than they intend to.

I’m neutral on this - we already have the _CLIENT_VERSION_IS_RELEASE and associated warning.

jeffrade commented at 8:50 pm on January 31, 2018: contributor

Forgetting a configuration option (in this case –enable-mainnet) would lead to a not-in-production useful software. This is uncommon behaviour.

Just a thought, but one solution is we force users to explicitly choose -mainnet when starting bitcoind if they did not use the --enable_mainnet config. We can add -mainnet as an option here and then also check if not present in init.cpp:

0  if (chain == CBaseChainParams::MAIN && !gArgs.GetBoolArg("-mainnet", false))
1    return InitError("You did not configure with --enable-mainnet. You must set -testnet, -regtest or -mainnet option.");

sipa commented at 9:20 pm on January 31, 2018: member

@jeffrade I like that idea. When compiling with –enable-mainnet, the default chain is mainnet. Otherwise, there is no default and you must choose one.

jtimon commented at 9:27 pm on January 31, 2018: contributor

We can add -mainnet as an option here

I’ve done something similar in https://github.com/bitcoin/bitcoin/pull/8994/commits/46391b935fd68d4800170f11bd6a3426bffc96ed feel free to cherry pick or copy that commit if more people like the idea.

Whether with -mainnet or with -chain=main, I agree it should be possible to run mainnet even when the –enable_mainnet config is not used.

Perhaps a simple way to implement this is by returning ChainNameFromCommandLine CBaseChainParams::MAIN as default only when –enable_mainnet, otherwise it throws a std::runtime_error asking the user to specify the chain explicitly.

jeffrade commented at 9:43 pm on January 31, 2018: contributor

@jtimon Thanks, will look through your commit. Was going to simply add the new fMainNet boolean and the corresponding if block to check (see snippet below). If none are set, will fall through and init.cpp will pick it up and throw the InitError().

 0std::string ChainNameFromCommandLine()
 1{
 2    bool fRegTest = gArgs.GetBoolArg("-regtest", false);
 3    bool fTestNet = gArgs.GetBoolArg("-testnet", false);
 4    bool fMainNet = gArgs.GetBoolArg("-mainnet", false);
 5
 6    if (fTestNet && fRegTest)
 7        throw std::runtime_error("Invalid combination of -regtest and -testnet.");
 8    if (fRegTest)
 9        return CBaseChainParams::REGTEST;
10    if (fTestNet)
11        return CBaseChainParams::TESTNET;
12    if (fMainNet)
13        return CBaseChainParams::MAINNET;
14    return CBaseChainParams::MAIN;
15}

jtimon commented at 0:30 am on February 1, 2018: contributor

I was thinking more on something like this:

 0std::string ChainNameFromCommandLine()
 1{
 2    bool fRegTest = gArgs.GetBoolArg("-regtest", false);
 3    bool fTestNet = gArgs.GetBoolArg("-testnet", false);
 4    bool fMainNet = gArgs.GetBoolArg("-mainnet", false);
 5
 6    if (fTestNet + fRegTest + fMainNet > 1)
 7        throw std::runtime_error("Invalid combination of -mainnet, -regtest and -testnet.");
 8    if (fRegTest)
 9        return CBaseChainParams::REGTEST;
10    if (fTestNet)
11        return CBaseChainParams::TESTNET;
12#ifndef ENABLE_MAINNET
13    if (!fMainNet)
14        throw std::runtime_error("You did not configure with --enable-mainnet. You must set -mainnet, -testnet or -regtest option.");
15#endif
16    return CBaseChainParams::MAIN;
17}

This way you don’t need to touch init at all, the init for this is already handled in src/qt/bitcoin.cpp, src/bitcoind.cpp, src/bitcoin-tx.cpp and src/bitcoin-cli.cpp, so no need to touch src/init.cpp if you do it here in ChainNameFromCommandLine(). Besides I think this way it stays better encapsulated.

Or alternatively (my preference), something like:

 0std::string ChainNameFromCommandLine()
 1{
 2    bool fRegTest = gArgs.GetBoolArg("-regtest", false);
 3    bool fTestNet = gArgs.GetBoolArg("-testnet", false);
 4    bool is_chain_arg_set = gArgs.IsArgSet("-chain");
 5
 6    if (is_chain_arg_set + fRegTest + fTestNet > 1) {
 7        throw std::runtime_error("Invalid combination of -regtest, -testnet and -chain. Can use at most one.");
 8    }
 9    if (fRegTest)
10        return CBaseChainParams::REGTEST;
11    if (fTestNet)
12        return CBaseChainParams::TESTNET;
13
14#ifndef ENABLE_MAINNET
15    if (!is_chain_arg_set) {
16        throw std::runtime_error("You did not configure with --enable-mainnet. You must set -chain, -testnet or -regtest option.");
17    }
18#endif
19    return gArgs.GetArg("-chain", CBaseChainParams::MAIN);
20}

jeffrade commented at 0:36 am on February 1, 2018: contributor

@jtimon throwing the std::runtime_error and checking for ENABLE_MAINNET in ChainNameFromCommandLine() is what I originally had implemented. But received feedback and there was consensus to move logic into init.cpp. See comments above from everyone else above.

jtimon commented at 0:49 am on February 1, 2018: contributor

Yes, but unless I’m missing something everybody else is wrong and doing it there won’t result in a silent exit but on the error being printed and then existing due to the try {} catch structures surrounding ChainNameFromCommandLine() calls in src/qt/bitcoin.cpp, src/bitcoind.cpp, src/bitcoin-tx.cpp and src/bitcoin-cli.cpp. In the case of running the qt GUI it is my believe that the error will even be shown in a popup window.

Actually checking that I am right is quite simple: just try both -testnet -regtest with either bitcoind, bitcoin-cli, bitcoin-tx or bitcoin-qt and you will see that throwing a std::runtime_error from this function doesn’t produce a silent error but the expected behaviour.

jeffrade commented at 0:55 am on February 1, 2018: contributor

@jtimon I have been testing with bitcoind and getting expected results, but need to test more. I will push a commit based on your feedback/branch and from others shortly. Thanks for the help!

jeffrade commented at 2:07 am on February 1, 2018: contributor

I like that idea. When compiling with –enable-mainnet, the default chain is mainnet. Otherwise, there is no default and you must choose one. @sipa I pushed a second commit (will squash if requested) to show new usage when --enable-mainnet is not used. @jtimon Below is my local testing with bitcoind showing the InitError and process exiting correctly:

0$ ./confgiure
1<output ommitted>
2$ make
3<output ommitted>
4$ ./src/bitcoind 
5Error: You did not configure with --enable-mainnet, therefore must set -regtest, -testnet or -mainnet option.
6$

I haven’t looked at the client gui, so I will work on that next.

jeffrade force-pushed on Feb 6, 2018

jeffrade commented at 2:03 am on February 6, 2018: contributor

@jtimon I successfully tested bitcoin-qt and receive the same error as when trying to run bitcoind without the --enable-mainnet flag (see screenshot).

jeffrade force-pushed on Feb 6, 2018

in doc/man/bitcoind.1:432 in 53011b61ab outdated

428@@ -429,6 +429,10 @@ Shrink debug.log file on client startup (default: 1 when no \fB\-debug\fR)
429 .PP
430 Chain selection options:
431 .HP
432+\fB\-mainnet\fR

MarcoFalke commented at 11:36 pm on February 6, 2018:

No need to touch this file. Will be updated before release.

jeffrade commented at 2:19 am on February 7, 2018:

Fixed

in src/chainparamsbase.cpp:97 in 53011b61ab outdated

91@@ -91,9 +92,10 @@ std::string ChainNameFromCommandLine()
92 {
93     bool fRegTest = gArgs.GetBoolArg("-regtest", false);
94     bool fTestNet = gArgs.GetBoolArg("-testnet", false);
95+    bool fMainNet = gArgs.GetBoolArg("-mainnet", false);
96 
97-    if (fTestNet && fRegTest)
98-        throw std::runtime_error("Invalid combination of -regtest and -testnet.");
99+    if (fTestNet + fRegTest + fMainNet >= 2)

MarcoFalke commented at 11:36 pm on February 6, 2018:

Nit missing braces {} according to developer notes

jeffrade commented at 2:18 am on February 7, 2018:

Fixed for all if blocks in this method.

MarcoFalke commented at 11:38 pm on February 6, 2018: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Also, try to pick a commit title that fits in 70-80 chars. Put the rest in the commit body.

Adding --enable-mainnet configuration option.

By default, is disbled and must set -mainnet option on startup. This is a
safeguard against dev/local builds from running on mainnet chain by default.

(cherry picked from commit 3e86a97c4d41b30babb25f0c0ca48f14ae095e34)

3822afeecd

jeffrade force-pushed on Feb 7, 2018

jeffrade commented at 2:22 am on February 7, 2018: contributor

@MarcoFalke Fixed all nits and squashed commit (as well as fix up commit message). Thanks for feedback!

MarcoFalke commented at 3:27 am on February 7, 2018: member

utACK 3822afeecd62c5f28047140629f357db49ed6829

jnewbery commented at 2:24 pm on March 22, 2018: member

Sorry to concept NACK so late in the PR, but I really don’t like the extra config complexity that this adds. The original design was also NACKed by a few contributors in the IRC channel in January:

013:17 < gmaxwell> ugh. I really dislike 12300.
113:18 < gmaxwell> If we want to reduce screw up risk we should make the github default branch a release branch; not boobytrap the prerelease software.
213:21 < gmaxwell> for the original ask, a developer that doesn't want to mess up their own wallets, could be just as well addressed with a --disable-mainnet configure option that they could setup to normally use.
3...
413:44 < jonasschnelli> gmaxwell: agree
5...
613:52 < cfields> gmaxwell / jonasschnelli: I agree as well. If anything, I think that's kinda dangerous as downstreams can just quietly remove the ifdef, and users think they're shielded from mainnet
713:54 < cfields> i can see the need for some kind of switch like that, but I'd rather do it in the appropriate place. ie If exposing the wallet to mainnet is scary, then add something there.

(although I don’t know how @gmaxwell , @jonasschnelli and @theuni feel about the latest design).

What exactly are we protecting against here? Developers running unreleased code and corrupting mainnet datadirs/wallets? Really, developers shouldn’t be running untested code anywhere near their mainnet nodes/wallets, and no amount of config complexity can prevent people from making bad decisions like that.

MarcoFalke commented at 2:34 pm on March 22, 2018: member

@jnewbery I think this would protect users from doing git clone && make, which defaults to the master branch.

Changing the default branch is not possible, since pull requests are opened against the default branch by default.

laanwj commented at 12:38 pm on April 26, 2018: member

Closing, as there is no agreement to do this.

laanwj closed this on Apr 26, 2018

MarcoFalke locked this on Sep 8, 2021

[Build] Adding –enable-mainnet configuration option for running mainnet #12300