Remove CChainParams::NetworkID() #4802

jtimon · 2014-08-31T21:06:58Z

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

laanwj · 2014-09-01T07:49:47Z

src/chainparams.cpp

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 · 2014-09-01T09:33:52Z

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 · 2014-09-03T05:50:27Z

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 · 2014-09-03T06:18:22Z

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 · 2014-09-03T08:59:10Z

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 · 2014-09-03T09:13:59Z

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 · 2014-09-22T10:31:22Z

src/checkpoints.cpp

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

jtimon · 2014-09-22T18:44:03Z

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 · 2014-09-22T18:56:41Z

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.

Diapolo · 2014-09-23T08:03:20Z

src/rpcmining.cpp

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

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.

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?

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

jtimon · 2014-10-06T16:02:45Z

rebased

sipa · 2014-10-09T02:01:58Z

src/qt/bitcoin.cpp

How is this an improvement?

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).

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 · 2014-10-09T02:03:14Z

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 · 2014-10-09T02:04:22Z

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 · 2014-10-09T02:13:30Z

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

laanwj · 2014-10-09T07:55:30Z

@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 · 2014-10-09T08:00:54Z

"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 · 2014-10-09T08:01:42Z

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 · 2014-10-09T09:16:01Z

See #5066.

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 bitcoin#4802.

jtimon · 2014-10-09T17:11:11Z

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

laanwj · 2014-10-10T06:44:18Z

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

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 bitcoin#4802.

jtimon · 2014-10-10T09:02:15Z

@laanwj cherry picked and squashed

6fd546d Remove CChainParams::NetworkID() (jtimon) cc97210 Add fTestnetToBeDeprecatedFieldRPC to CChainParams (jtimon) e11712d Move checkpoint data selection to chainparams (jtimon) 6de50c3 qt: add network-specific style object (Wladimir J. van der Laan)

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 bitcoin#4802. (cherry picked from commit 6de50c3) # Conflicts: # src/qt/bitcoingui.cpp # src/qt/splashscreen.cpp

This was referenced Aug 31, 2014

Switch of the test framework from MAIN to REGTEST #4732

Closed

Remove CBaseChainParams::NetworkID() #4804

Merged

laanwj reviewed Sep 1, 2014
View reviewed changes

jtimon force-pushed the chainparams2 branch from 37eb685 to 13e029d Compare September 1, 2014 09:30

jtimon mentioned this pull request Sep 2, 2014

Move checkpoint data selection to chainparams #4801

Closed

laanwj added the Improvement label Sep 3, 2014

laanwj reviewed Sep 22, 2014
View reviewed changes

src/checkpoints.cpp Outdated

Copy link

Member

laanwj Sep 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jtimon force-pushed the chainparams2 branch from 13e029d to 9d9770d Compare September 22, 2014 18:42

Diapolo reviewed Sep 23, 2014
View reviewed changes

jtimon force-pushed the chainparams2 branch 2 times, most recently from f595af7 to c1c7440 Compare October 6, 2014 07:55

sipa reviewed Oct 9, 2014
View reviewed changes

laanwj closed this Oct 9, 2014

laanwj reopened this Oct 9, 2014

laanwj mentioned this pull request Oct 9, 2014

qt: add network-specific style object #5066

Closed

jtimon force-pushed the chainparams2 branch from c1c7440 to 0aadb3c Compare October 9, 2014 17:10

laanwj and others added 4 commits October 10, 2014 11:00

qt: add network-specific style object

6de50c3

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 bitcoin#4802.

Move checkpoint data selection to chainparams

e11712d

Add fTestnetToBeDeprecatedFieldRPC to CChainParams

cc97210

Remove CChainParams::NetworkID()

6fd546d

jtimon force-pushed the chainparams2 branch from 0aadb3c to 6fd546d Compare October 10, 2014 09:01

laanwj merged commit 6fd546d into bitcoin:master Oct 10, 2014

jtimon mentioned this pull request Oct 14, 2016

RPC: Chainparams: Remove Chainparams::fTestnetToBeDeprecatedFieldRPC #8921

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Remove CChainParams::NetworkID() #4802

Remove CChainParams::NetworkID() #4802

Uh oh!

Conversation

jtimon commented Aug 31, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Sep 1, 2014

Uh oh!

TheBlueMatt commented Sep 3, 2014

Uh oh!

laanwj commented Sep 3, 2014

Uh oh!

jtimon commented Sep 3, 2014

Uh oh!

laanwj commented Sep 3, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Sep 22, 2014

Uh oh!

BitcoinPullTester commented Sep 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Oct 6, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Oct 9, 2014

Uh oh!

gmaxwell commented Oct 9, 2014

Uh oh!

TheBlueMatt commented Oct 9, 2014

Uh oh!

laanwj commented Oct 9, 2014

Uh oh!

jtimon commented Oct 9, 2014

Uh oh!

laanwj commented Oct 9, 2014

Uh oh!

laanwj commented Oct 9, 2014

Uh oh!

jtimon commented Oct 9, 2014

Uh oh!

laanwj commented Oct 10, 2014

Uh oh!

jtimon commented Oct 10, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants