Remove CChainParams::NetworkID()

jtimon commented at 9:06 pm on August 31, 2014: contributor

Continues #4801 (and #3823 #3824 #4333).

in src/chainparams.cpp: in 37eb685462 outdated

116@@ -116,6 +117,13 @@ class CMainParams : public CChainParams {
117         fAllowMinDifficultyBlocks = false;
118         fRequireStandard = true;
119         fMineBlocksOnDemand = false;
120+        fGuiTestColor = false;

laanwj commented at 7:49 am on September 1, 2014:

Please don’t do this. Keep GUI-specific decisions in the GUI. If you really insist on getting rid of the enumeration, you could match on NetworkIDString() instead to determine the network-specific theming.

jtimon force-pushed on Sep 1, 2014

jtimon commented at 9:33 am on September 1, 2014: contributor

Removed the gui specific flag in favor of using the bip70 string id. If there’s nothing controversial with the checkpoints in #4801 I can just close it as it is included here (also close this one since it is included in #4804 ). I probably shouldn’t have open so many of them in the first place…

TheBlueMatt commented at 5:50 am on September 3, 2014: member

I’m not sure this code is really more readable. Comparing to TESTNET to set a testnet flag is very readable (also, why are you saying testnet is deprecated?). Comparing to a string always seems worse than comparing to a constant to me, and I’m not convinced moving the checkpoint data to a separate file just to #include it in one cpp file is a good idea.

laanwj commented at 6:18 am on September 3, 2014: member

Testnet is not going to be deprecated, but the getinfo call is (and the testnet field on it). Ever since we support three chains, having a boolean is not enough anymore. getblockchaininfo returns much more information about the current chain.

And well, I can understand that the idea behind this is to have all the chain-specific data in a parameter data structure without ‘identity’. But the GUI and the deprecated flag needs to know the network identity so there’s an inherent conflict there.

jtimon commented at 8:59 am on September 3, 2014: contributor

Yeah, comparing to the testnet id was very readable so it’s the new flag. The difference is that when people see “NetworkID() == something” they think they can use that too, while FlagWithLongNameIncludingTheWordDeprecated() is not as likely to be used or that’s my hope. @laanwj explained the deprecation thing, but basically, RPCs can just use bip70’s networkidstring() For the string, I was going to create another flag but @laanwj wants to keep chainparams clean of GUI-related things. Of course comparing to an enumerator is better than comparing to a string, again, the goal is that people don’t see the comparation to the enumeration and think they can call CChainParams::NetworkID(), hopefully the comparison with a string will make people use that approach less, but we need the string for BIP70 anyway.

With respect to the checkpoints,t his way we avoid the calls to Params().NetworkID() in checkpoints.cpp. If at some point we’re going to read the chainparams from data files, we probably want to put the checkpoint data there as well. At that point writing a new mode should be as simple as writing a json or xml file.

So in summary, the goal here is not readability, is to maintain readability by killing NetworkID(), which shouldn’t be used.

laanwj commented at 9:13 am on September 3, 2014: member

With regard to the GUI comparing directly against the string, that doesn’t have to stay that way in the future. Best would be to have a style sheet class name based on the network name or something like that. Anyhow, it’s exceptional usage and only affects visuals.

laanwj added the label Improvement on Sep 3, 2014

in src/checkpoints.cpp: in 13e029df67 outdated

3@@ -4,18 +4,17 @@
4 
5 #include "checkpoints.h"
6 
7+#include "chainparams.h"
8+#include "checkpoint_data.h"

laanwj commented at 10:31 am on September 22, 2014:

checkpoint_data shouldn’t be included here (as that duplicates the data)

in src/checkpoint_data.h: in 13e029df67 outdated

18+    int64_t nTimeLastCheckpoint;
19+    int64_t nTransactionsLastCheckpoint;
20+    double fTransactionsPerDay;
21+};
22+
23+// What makes a good checkpoint block?

laanwj commented at 10:33 am on September 22, 2014:

I’d prefer putting this data in chainparams.cpp directly. If, at some point in the future, we want to auto-generate this code we’d want to use some other division between structure definitions/data definitions anyway.

jtimon force-pushed on Sep 22, 2014

jtimon commented at 6:44 pm on September 22, 2014: contributor

Since I had to squash the fix addressing @laanwj suggestions and the first two commits, I just rebased on top of the current master.

BitcoinPullTester commented at 6:56 pm on September 22, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4802_9d9770dbf0612dba4073437e0a69259f299908f7/ for binaries and test log. 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.

in src/rpcmining.cpp: in 9d9770dbf0 outdated

238@@ -239,7 +239,7 @@ Value getmininginfo(const Array& params, bool fHelp)
239     obj.push_back(Pair("genproclimit",     (int)GetArg("-genproclimit", -1)));
240     obj.push_back(Pair("networkhashps",    getnetworkhashps(params, false)));
241     obj.push_back(Pair("pooledtx",         (uint64_t)mempool.size()));
242-    obj.push_back(Pair("testnet",          Params().NetworkID() == CBaseChainParams::TESTNET));
243+    obj.push_back(Pair("testnet",          Params().TestnetToBeDeprecatedFieldRPC()));

Diapolo commented at 8:03 am on September 23, 2014:

I still don’t understand why that weird function name is used?

laanwj commented at 8:06 am on September 23, 2014:

Because this flag (and the entire getinfo() ) is going to be deprecated. A boolean is no longer enough to define the chain that is used. For example it is defined as false for regtest, which is confusing to people.

Diapolo commented at 8:11 am on September 23, 2014:

Your answer is true for getinfo as we intend to remove or deprecate that, but in getmininginfo we should perhaps just update to what is now needed to define the chain?

laanwj commented at 8:18 am on September 23, 2014:

Agreed. Eventually we want to just have the chain identifier there (like in getblockchaininfo). That’s unrelated to this pull though.

jtimon force-pushed on Oct 6, 2014

jtimon commented at 4:02 pm on October 6, 2014: contributor

rebased

in src/qt/bitcoin.cpp: in c1c7440c55 outdated

571@@ -572,7 +572,7 @@ int main(int argc, char *argv[])
572     if (!PaymentServer::ipcParseCommandLine(argc, argv))
573         exit(0);
574 #endif
575-    bool isaTestNet = Params().NetworkID() != CBaseChainParams::MAIN;
576+    bool isaTestNet = Params().NetworkIDString() != "main";

sipa commented at 2:01 am on October 9, 2014:

How is this an improvement?

TheBlueMatt commented at 2:11 am on October 9, 2014:

I find the new option much less “neat”. Though a general Params().GUITestNetFlag() might make sense here (and then use that flag with a // Deprecated comment in getinfo too - changes the meaning of “testnet” under regtest, but oh well).

jtimon commented at 8:03 am on October 9, 2014:

my initial version of the rpc flag changed the behaviour of regtest as suggested here, but it wasn’t coupled with the gui flag.

sipa commented at 2:03 am on October 9, 2014: member

I’m going to defer to @laanwj’s opinion on the concept here; I personally don’t consider string matching better than identifier matching…

I do like the movement of the checkpoint data to chainparams, though.

gmaxwell commented at 2:04 am on October 9, 2014: contributor

Code is fine, movement of checkpoint is good. The comparison with the string seemed unusual for our codebase, but perhaps it makes sense in the context of the GUI. Whatever wumpus wants. Conditional-ACK-on-Wumpus

TheBlueMatt commented at 2:13 am on October 9, 2014: member

If you provide a GUITestNetFlag and use that in getinfo too, I’ll be happy to ACK here.

laanwj commented at 7:55 am on October 9, 2014: member

@sipa @gmaxwell The string matching will at most be temporary - as I said, it makes much more sense to have a stylesheet (or image directory name) with the name of the network (and that will be necessary to have different theming for regtest/testnet/mainnet anyway). That hasn’t been implemented yet. It is a presentation-only attribute so I’m not concerned with having a string match here.

Please do not add a a GUITestNetFlag to chain parameters, I’ve tried to get rid of GUI-specific flags in the core all the time. You cannot use the same flag for getinfo because the getinfo testnet has a different interpretation: regtest is not testnet according to getinfo, where it is for the GUI (right now!).

jtimon commented at 8:00 am on October 9, 2014: contributor

“If you provide a GUITestNetFlag” I did that previously but @laanwj doesn’t want anything gui-specific in chainparams, which makes a lot of sense. Making an ismainet flag defeats the purpose of removing networkID(). Thus we used the bip70 string. The performance penalty is meaningless, specially in the context of a GUI.

“and use that in getinfo too”. No, the testnet flag is a deprecated flag that has to be removed when it is possible, thus it shouldn’t be reused for anything else. Future RPC/rest interface should use the string instead.

laanwj commented at 8:01 am on October 9, 2014: member

If the string match in the GUI is such a big problem, I’ll go refactor that right now… Edit: uh… didn’t mean to close this, sorry.

laanwj closed this on Oct 9, 2014

laanwj reopened this on Oct 9, 2014

laanwj referenced this in commit 34a7365bbe on Oct 9, 2014

laanwj commented at 9:16 am on October 9, 2014: member

See #5066.

laanwj referenced this in commit b42db95fe6 on Oct 9, 2014

laanwj referenced this in commit 99a814342b on Oct 9, 2014

jtimon force-pushed on Oct 9, 2014

jtimon commented at 5:11 pm on October 9, 2014: contributor

Rebased on top of #5066, no more string comparison.

laanwj commented at 6:44 am on October 10, 2014: member

@jtimon can you please cherry-pick laanwj@afd2a6b into my commit here? (small fix after nit by @theuni) Tested ACK apart from that.

qt: add network-specific style object

Mainly cleanups: Gets rid of isTestNet everywhere, by keeping track
of network-specific theming in a central place.

Also makes GUI no longer dependent on the network ID enumeration, which
alleviates concerns about #4802.

6de50c3c9a

Move checkpoint data selection to chainparams e11712df7e

Add fTestnetToBeDeprecatedFieldRPC to CChainParams cc97210799

Remove CChainParams::NetworkID() 6fd546dd96

jtimon force-pushed on Oct 10, 2014

jtimon commented at 9:02 am on October 10, 2014: contributor

@laanwj cherry picked and squashed

laanwj merged this on Oct 10, 2014

laanwj closed this on Oct 10, 2014

laanwj referenced this in commit 023690c0f2 on Oct 10, 2014

reddink referenced this in commit beb69a1dec on May 27, 2020

MarcoFalke locked this on Sep 8, 2021

Remove CChainParams::NetworkID() #4802