Chainparams: Generic chainparam selection with -chain=<strNetworkID>

jtimon commented at 5:55 pm on November 6, 2014: contributor

Continues #4804. Deprecate -testnet and -regtest in favor of -network=testnet and -network=regtest. When the old args are used a warning will be logged. The repetition of the invalid combination error string is also avoided. Additionally, the chainparam networkID is removed since it’s not used anymore (as it shouldn’t).

jtimon renamed this:
~~Generic chainparam selection with -network=<network>~~
Generic chainparam selection with -network=<strNetworkID>
on Nov 6, 2014

in src/chainparams.h: in 1df7eb85b9 outdated

144@@ -146,9 +145,11 @@ CModifiableParams *ModifiableParams();
145 void SelectParams(CBaseChainParams::Network network);
146 
147 /**
148- * Looks for -regtest or -testnet and then calls SelectParams as appropriate.
149- * Returns false if an invalid combination is given.
150+ * Returns the appropriate Network ID from the string provided in -network.

sipa commented at 5:59 pm on November 6, 2014:

It doesn’t actually return anything.

jtimon commented at 10:52 pm on November 6, 2014:

I will correct it.

jgarzik commented at 6:48 pm on November 6, 2014: contributor

The value of removing -testnet and -regtest is unclear.

Also, please test and make sure bitcoin-cli and bitcoin-tx utilities remain consistent with bitcoind.

in src/chainparamsbase.cpp: in 1df7eb85b9 outdated

101+    std::string network = GetArg("-network", "");
102+    if (network == "main")
103+        return CBaseChainParams::MAIN;
104+    if (network == "testnet")
105+        return CBaseChainParams::TESTNET;
106+    if (network == "testnet")

Diapolo commented at 6:50 pm on November 6, 2014:

2 times “testnet”? Guess this should read “regtest”.

jtimon commented at 10:56 pm on November 6, 2014:

Ops, I will correct it.

in src/chainparamsbase.cpp: in 1df7eb85b9 outdated

 96     }
 97 }
 98 
 99 CBaseChainParams::Network NetworkIdFromCommandLine()
100 {
101+    std::string network = GetArg("-network", "");

Diapolo commented at 6:51 pm on November 6, 2014:

An empty network should also be mainnet, or?

jtimon commented at 10:56 pm on November 6, 2014:

That’s what it does, look at the last return. I use "" as the default for the later (network != “”) check to throw an error.

Diapolo commented at 6:19 am on November 7, 2014:

I’d use if (network.empty() || network == "main") that way the code is much easier to read IMHO. Or even better, make the default in GetArg “main”?

jtimon commented at 9:24 am on November 7, 2014:

When the user doesn’t provide a -network you don’t want to default to main yet, you want to check the -testnet and -regtest flags first. When we stop supporting -testnet and -regtest, the function can be as simple as this:

 0CBaseChainParams::Network NetworkIdFromCommandLine()
 1{
 2    std::string network = GetArg("-network", "main");
 3    if (network == "main")
 4        return CBaseChainParams::MAIN;
 5    if (network == "test")
 6        return CBaseChainParams::TESTNET;
 7    if (network == "regtest")
 8        return CBaseChainParams::REGTEST;
 9    if (network == "unittest")
10        return CBaseChainParams::UNITTEST;
11    throw std::runtime_error("Unknown network " + network + "\n");
12}

jtimon commented at 9:33 am on November 7, 2014:

Actually I wanted to check network before checking testnet/regtest so that if you provide both network=“regtest” and -testnet, the network arg has preference and the -testnet arg is ignored. But doing it in the other order allows this simplification now and produces less changes when those flags are disabled so I’ll change it.

in src/chainparams.cpp: in 1df7eb85b9 outdated

348@@ -353,8 +349,7 @@ CChainParams &Params(CBaseChainParams::Network network) {
349         case CBaseChainParams::UNITTEST:
350             return unitTestParams;
351         default:
352-            assert(false && "Unimplemented network");
353-            return mainParams;
354+            throw std::runtime_error("Unimplemented network\n");

Diapolo commented at 6:52 pm on November 6, 2014:

I think unknown or invalid sounds more common. Same for the usages below.

jtimon commented at 10:51 pm on November 6, 2014:

I just copied the message that we had, but I actually prefer unknown or not supported. I will change it to unknown.

in src/chainparamsbase.cpp: in 1df7eb85b9 outdated

116     if (fTestNet && fRegTest)
117-        return CBaseChainParams::MAX_NETWORK_TYPES;
118-    if (fRegTest)
119+        throw std::runtime_error("Invalid combination of -regtest and -testnet. Additionally -testnet and -regtest are deprecated, use -network=testnet instead.\n");
120+    if (fRegTest) {
121+        LogPrintStr("WARNING: -regtest is deprecated, use -network=regtest instead.");

Diapolo commented at 6:54 pm on November 6, 2014:

Mind using the same phrase or form as we already have in init.cpp, e.g. Warning: Unsupported argument -debugnet ignored, use -debug=net..

jtimon commented at 10:57 pm on November 6, 2014:

But -testnet and -regtest are still supported, I’m just warning that their use is not recommended since they may dissappear in the future.

in src/chainparamsbase.cpp: in 1df7eb85b9 outdated

121+        LogPrintStr("WARNING: -regtest is deprecated, use -network=regtest instead.");
122         return CBaseChainParams::REGTEST;
123-    if (fTestNet)
124+    }
125+    if (fTestNet) {
126+        LogPrintStr("WARNING: -testnet is deprecated, use -network=testnet instead.");

Diapolo commented at 6:54 pm on November 6, 2014:

Same here…

jtimon commented at 11:02 pm on November 6, 2014: contributor

@jgarzik -testnet and -regtest still work, I’m just logging a warning to discourage its use. I thought that we can remove them later. About the errors, I will catch them (like in the qt case) and return false for consistency.

jtimon force-pushed on Nov 6, 2014

laanwj commented at 9:44 am on November 7, 2014: member

NACK on removing/deprecating -testnet and -regtest. Selecting the network is a primary function of the client, so it can be a top-level option.

jtimon commented at 9:54 am on November 7, 2014: contributor

I think it has been a misunderstanding. This PR doesn’t disable -testnet and -regtest, just allows using -network instead. #5238 does disable them. Maybe I should replace “is deprecated” with “will be deprecated” (maybe that only makes sense after we have at least one more network [since unitest is unlikely to be selected on startup]). As said in #5238, we cannot disable these flags without first providing the alternative and warning about the old flags for a while (maybe one release? next major release? 0.12? 1.0? 5 years from now? I don’t know). This is just a first step which also makes it easier to add new networks. I’ve always used “deprecated” as “still enabled but with a recommended alternative, will eventually disappear”, like the java anotation @deprecated. It seems everyone else means “disabled, it used to be enabled in the past”.

laanwj commented at 10:04 am on November 7, 2014: member

I just don’t want them to be deprecated. People rely on them, and they work fine. Forcing people to use another option seems like a boondoggle.

I’m not against also adding -network= - but do we really plan on adding more networks in the future? What is the use-case here for Bitcoin?

jtimon commented at 10:10 am on November 7, 2014: contributor

So are you against the warning messages? For bitcoin, right now it only adds the ability to select “unittest” which I admit may not be very useful. I don’t know if we will add another mode anytime soon, but it makes it easier to do so in the future. It also simplifies things for people maintaining their own custom modes locally. Finally, If the final goal is to load chainparams from files I think this is a step in the right direction.

laanwj commented at 10:22 am on November 7, 2014: member

Indeed, remove the warning messages. It makes sense to keep those quick shortcut option to select regtest and testnet (but not for more obscure networks like unittest).

jtimon commented at 2:07 pm on November 7, 2014: contributor

Warnings removed. I’m fine just not adding new network shortcuts from now on.

jtimon commented at 11:08 am on November 8, 2014: contributor

Should I squash already?

petertodd commented at 3:46 pm on November 10, 2014: contributor

NACK on removing/deprecating -testnet and -regtest.

laanwj added the label Improvement on Nov 10, 2014

jtimon commented at 7:17 pm on November 10, 2014: contributor

@petertodd I told @jgarzik and then @Diapolo that this does not disable -testnet or -regtest. Then @laanwj complained about the warnings saying they are deprecated and I removed the warnings too. Please, read the code, or at least the thread. Maybe it is less clear due to all the “squashme” commits, I’ll squash now…

jtimon force-pushed on Nov 10, 2014

jtimon force-pushed on Nov 13, 2014

jtimon force-pushed on Dec 11, 2014

jtimon commented at 7:20 pm on January 4, 2015: contributor

Closing until #5598 gets merged.

jtimon closed this on Jan 4, 2015

jtimon reopened this on Jan 7, 2015

jtimon force-pushed on Jan 7, 2015

jtimon commented at 11:47 pm on January 7, 2015: contributor

Reopened and rebased after #5598 has been merged. Now it’s easier to review.

petertodd commented at 0:00 am on January 8, 2015: contributor

Rather that having the argument be “-network” why don’t we rename it to “-chain”?

Network is a term that’s already overloaded in multiple contexts. After all, we called it chain params rather than network params.

On 7 January 2015 18:48:08 GMT-05:00, “Jorge Timón” notifications@github.com wrote:

Reopened and rebased after #5598 has been merged. Now it’s easier to review.

Reply to this email directly or view it on GitHub: #5229 (comment)

jtimon commented at 5:34 pm on January 10, 2015: contributor

Yes, I also thought about “mode” and “chain”. Technically main and unittest are the same chain, since they share the same genesis block and all, so I decided for “network”. But that’s the only exception so probably “chain” still makes sense. Let’s wait for other people to decide the color of the bike shed and I’ll make the change to whatever is more popular.

jtimon force-pushed on Jan 10, 2015

jtimon renamed this:
~~Generic chainparam selection with -network=<strNetworkID>~~
Chainparams: Generic chainparam selection with -network=<strNetworkID>
on Jan 21, 2015

jtimon force-pushed on Mar 26, 2015

jtimon commented at 4:07 pm on March 26, 2015: contributor

Needed rebase so I squashed the nits.

laanwj commented at 2:34 pm on March 30, 2015: member

I’m still not convinced that this is necessary. The reason for this change seems very academic; -testnet -regtest versus -network=testnet -network=regtest, meh.

petertodd commented at 3:56 am on April 1, 2015: contributor

@laanwj FWIW I always use –testnet and –regtest in my command-line tools.

jtimon renamed this:
~~Chainparams: Generic chainparam selection with -network=<strNetworkID>~~
Chainparams: Generic chainparam selection with -chain=<strNetworkID>
on Apr 1, 2015

in src/chainparamsbase.cpp: in 1dba120678 outdated

101         return CBaseChainParams::TESTNET;
102-    return CBaseChainParams::MAIN;
103+
104+    std::string chain = GetArg("-chain", "main");
105+    if (chain == "main")
106+        return CBaseChainParams::MAIN;

laanwj commented at 9:07 am on April 2, 2015:

What if these would be defined as string constants, e.g.:

0    const std::string CBaseChainParams::MAIN = "main";
1    const std::string CBaseChainParams::TESTNET = "testnet";
2    const std::string CBaseChainParams::REGTEST = "regtest";

… I guess we could get rid of the CBaseChainParams::Network enumeration type completely, making SelectParams take a string. This kind of switching code would be no longer necessary apart from the data structure selection in SelectParams itself.

jtimon commented at 11:14 pm on April 8, 2015:

Well, we can get rid of the enumeration even if the strings aren’t declared as constants. I won’t opposed to it but working at big java projects were those type of constants are abundant I have always wondered if the gain in readability that CBaseChainParams::TESTNET provides compared to “testnet”. Yes, yes, developers could type “testtnet” and have some fun getting an “unkown chain ’testtnet’” exception…

Personally I like the Params(type) or Params(std::string) factory more than Params() in combination with SelectParams(). I mean, this is closer to what I would do: https://github.com/jtimon/bitcoin/commit/d135001a0a8ae9536553d49b1ac8a3c2e0643e41#diff-d22bc3e058f8982972e2eb381a1df668R34 In fact my preference when it comes to creating polymorphic objects would be for the factory function to return a pointer to a newly created object that the caller has to delete, a good-old factory without singleton [like Params()] or per-type singleton [like Params(std::string)], but that’s more than we need. I was scared that moving from enum to string was going to be something people wouldn’t like, but if that’s not the case I can definitely improve this. The remaining question is, do we really need the string constants when you will get an exception right in the face if you don’t use the correct string? In my opinion, “the same string you use in command line and it’s documented” would be ideal.

laanwj commented at 9:10 am on April 2, 2015: member

This also needs changes to the qt autostart functionality, #5793, which relies on passing through -regtest/-testnet at the moment. But at least that is a simplification, you could just pass -chain=%s with %s the current chain name.

jtimon commented at 11:17 pm on April 8, 2015: contributor

@laanwj could you give a line number link on where is that passed?

jtimon commented at 11:52 pm on April 8, 2015: contributor

@petertodd as discussed those options will remain usable and not deprecated. Given that unittestnet has disappeared, it’s really mainly an extensibility and altchain friendly patch so I understand if it remains in limbo until it is more directly useful for bitcoin. It won’t be hard to maintain unmerged anyway. Whenever somebody thinks about adding a third –harforktestnet or whatever this PR will be here to remind us that N is better than 2 + 1 (+ the implicit default “main” or “bitcoin”). We can label it low priority or I can put an uppercased word at the beginning of the tittle to clarify that people shouldn’t get very distracted by this unless there’s nothing else to merge or the somebody reanimates it [by proposing an additional chainparam option in the command line]. Paint it black and let it be, or something. I can add a compilation error for a red cross to be shown. But keeping it open I remember to rebase it and “police” this part of the code in some sense. I can also just set an alarm to rebase it with the PR closed. Not a strong opinion besides “if a chainparam option has to be added, it must be generic”. Sorry for the wall of text.

jtimon force-pushed on Apr 14, 2015

jtimon commented at 6:44 pm on April 14, 2015: contributor

Updated with @laanwj ’s suggestion for replacing the enum with strings, and added a new GetParamsHelpMessages() function. Note that it could be used in src/bitcoin-cli.cpp too but I wasn’t sure if we wanted that depending on ui_interface.

jtimon force-pushed on Apr 15, 2015

jtimon commented at 11:50 am on April 15, 2015: contributor

Sorry, I forgot to deal with the qt autostart functionality as pointed out by@laanwj. It is done now.

I also stopped using the bip70 nomenclature for the string constants since that changes “testnet” for just “test”, which wouldn’t have worked very well with the qt part. Is there any possibility to correct bip70 in that sense?

in src/qt/guiutil.cpp: in 587e225f86 outdated

716+        if (chain == "main")
717             optionFile << "Name=Bitcoin\n";
718-        optionFile << "Exec=" << pszExePath << strprintf(" -min -testnet=%d -regtest=%d\n", GetBoolArg("-testnet", false), GetBoolArg("-regtest", false));
719+        else
720+            optionFile << strprintf("Name=Bitcoin (%s)\n", chain);
721+        optionFile << "Exec=" << pszExePath << strprintf(" -min -chain=%s -regtest=%d\n", chain);

laanwj commented at 12:04 pm on April 15, 2015:

Spurious -regtest=%d

jtimon force-pushed on Apr 16, 2015

jtimon commented at 1:15 pm on April 16, 2015: contributor

Oopss, thanks @laanwj , fixed. Also adding a commit in which src/bitcoin-cli.cpp also uses GetParamsHelpMessages() [and also ui_interface]. Thoughts on that commit? @theuni is there an specific reason for bitcoin-cli not to use ui_interface that I’m missing?

jtimon commented at 1:22 pm on April 16, 2015: contributor

I mean, the last commit would be kind of undoing https://github.com/bitcoin/bitcoin/commit/73ac7abd08a70adf22e24d0f5f7631e7f8b7c5bb

That’s why I’m assuming people may not lie the idea.

jtimon force-pushed on Apr 16, 2015

jtimon commented at 3:53 pm on April 16, 2015: contributor

Never mind about the SQUASHME commits, I rebased on top of #6022, which is what I think makes most sense.

jtimon force-pushed on Apr 16, 2015

jtimon commented at 10:00 am on April 24, 2015: contributor

Closing until #6022 is merged.

jtimon closed this on Apr 24, 2015

jtimon reopened this on May 1, 2015

jtimon force-pushed on May 1, 2015

jtimon commented at 8:40 am on May 1, 2015: contributor

Rebased after https://github.com/jtimon/bitcoin/commit/b74dcb3b4aa29958d22135f2d6c015578069e83c ( #6022 )

jtimon force-pushed on May 6, 2015

jtimon commented at 11:14 am on May 6, 2015: contributor

Needed rebase

jtimon force-pushed on May 6, 2015

jtimon force-pushed on May 22, 2015

jtimon commented at 4:33 am on May 22, 2015: contributor

Added 3 commits to hopefully make it more interesting. The added tests are kind of redundant, but won’t hurt. I’m specially interested on @theuni ’s opinion on (~~https://github.com/jtimon/bitcoin/commit/65e2a722d64454378bb481afab154e4c9e1178d9~~) commit named “Chainparams: Use a regular factory for creating chainparams”] (since I know he wants to eventually separate the state-related functions out, but ParamsFactory() doesn’t rely on any state).

jtimon force-pushed on May 22, 2015

jtimon commented at 9:38 am on May 22, 2015: contributor

Removed https://github.com/jtimon/bitcoin/commit/ac1c555f275109582974ab0245a0d81854cf1ddd since is now a proposal for the trivial branch (https://github.com/theuni/bitcoin/pull/27).

To better read commit “Chainparams: The hash of the genesis block it’s the genesis checkpoint and chain id”, please rgrep hashGenesisBlock

All mentions of it are whether to:

Its own asignments, of course.
Make sure the genesis block is in a block index we’re maintaining, like in ./init.cpp:1112, of course.
Asserts in chainparams.cpp (no need to aditionally check genesis.hashMerkleRoot), let’s keep this, why not?
Avoid unnecessary checkpoints for the genesis block, in case we want to apply what’s inside it somewhere, for example: ./main.cpp:1708, ./main.cpp:2641, ./main.cpp:2732, ./main.cpp:3385, ./main.cpp:3400, ./main.cpp:3495

Why the heck are we trying to check the genesis block in the first place? The genesis block is not something to test, it is actually the first rule of the chain, it has to be right or the chain rules are broken (softforks and hardforks will come at a later nHeight).

But what about checkpoints?

The logic of checkpoints can always safely rely on the first checkpoint being true. The last point may not be true if three’s a reorg behind it, otherwise you’re ready to optimize. We could introduce a configurable third layer of “this is my new genesis no matter what”, which would be good for pruning and skipping all validations on blocks that are too old to believe they can be reorg-ed, but this would only really make sense with some form of utxo commitment (did I say that append-only-txo + append-only-confirmed is enough for this?).

Therefore, we should not:

Disperse hardcoded values.
Double-check hashes of hashes
Leave checkpoints without the only true first safe checkpoint: the genesis block

jtimon force-pushed on May 22, 2015

jtimon force-pushed on May 23, 2015

jtimon commented at 9:48 am on May 23, 2015: contributor

Sorry, pushed too fast. Now I use containers instead of those ugly ClearSelectedParams functions, and the first commit actually compiles (now all of them do).

jtimon force-pushed on May 23, 2015

jtimon commented at 12:22 pm on May 27, 2015: contributor

Closing for now, I will open a version with some of the changes but without adding the new option.

jtimon closed this on May 27, 2015

Chainparams: The hash of the genesis block it's the genesis checkpoint and chain id 15cabcdb12

Chainparams: Replace CBaseChainParams::Network enum with string constants (suggested by Wladimir) 5cd9cd822c

fixup! RENAME: strNetworkID = CBaseChainParams::REGTEST, strDataDir = CBaseChainParams::REGTEST

s/"regtest"/CBaseChainParams::REGTEST
s/"main"/CBaseChainParams::MAIN
s/"testnet"/CBaseChainParams::TESTNET

3e03bb7883

Chainparams: Translations: DRY: options strings e477f2d3b2

fixup! Chainparams: Translations: DRY: error strings 9cccb27820

Chainparams: Use a regular factory for creating chainparams 6fa0ef9b2c

DRY: TODO find out why this excludes regtest bab13a9142

Change?: Chainparams: DRY: Make qt/guiutil.cpp fit BIP70 chain name strings 5a9dd942e4

Change?: bip70 CBaseChainParams::TESTNET = from "test" to "testnet3" (and user-facing testnet to "testnet3") f61c8f4c43

Chainparams: Generic selection with -chain=<chainString> in addition of -testnet and -regtest 0c70392c0d

jtimon reopened this on Jun 4, 2015

jtimon force-pushed on Jun 4, 2015

jtimon commented at 10:11 am on June 4, 2015: contributor

This is the promised version with some of the improvements but without adding the new option: #6229 Opened for discussion.

jtimon closed this on Jun 4, 2015

jtimon referenced this in commit 1cdb9c8b5f on Apr 6, 2016

MarcoFalke locked this on Sep 8, 2021

Chainparams: Generic chainparam selection with -chain= #5229