add NetworkIDString() to chainparams #4326

1. 
   Diapolo commented at 10:34 am on June 11, 2014: none

   • returns the BIP70 network string
   • use that new function in the core and GUI code and remove unused code and functions
2. in src/qt/clientmodel.cpp: in 5a7cc2bef5 outdated

   153+    if (networkId == CChainParams::MAIN)
   154+        return "main";
   155+    else if (networkId == CChainParams::TESTNET)
   156+        return "test";
   157+    else if (networkId == CChainParams::REGTEST)
   158+        return "test";
   

   ---

   ---

   

   laanwj commented at 12:07 pm on June 11, 2014:

   Shouldn’t this be “regtest”?

   ---

   

   Diapolo commented at 12:38 pm on June 11, 2014:

   Indeed…
3. in src/qt/clientmodel.cpp: in 5a7cc2bef5 outdated

   143@@ -144,10 +144,16 @@ void ClientModel::updateAlert(const QString &hash, int status)
   144 
   145 QString ClientModel::getNetworkName() const
   

   ---

   ---

   

   laanwj commented at 12:10 pm on June 11, 2014:

   Please add a comment in the header that the value returned here is the BIP70 network string

   ---

   

   Diapolo commented at 12:40 pm on June 11, 2014:

   See new comment in the header below.
4. 
   laanwj commented at 3:55 pm on June 11, 2014: member

   Hmm I just realize that we have the same information in getblockchaininfo, there it also uses the datadir name to identify the network. Not sure whether it’s nice to change the debug window now (or we should use the BIP70 naming in getblockchaininfo as well, which may make sense… ).
5. 
   Diapolo commented at 6:22 am on June 12, 2014: none

   @laanwj IMHO it should be consistent all over the place and if we want to compare the strings it would be best to use ones that can be compared, so I vote for the BIP70 naming.
6. 
   Diapolo commented at 6:25 am on June 12, 2014: none

   I could add a NetworkIDToString() function in chainparams, which will be the ClientModel::getNetworkName() code. I then also can remove the ClientModel from PaymentServer.

   Edit: See 2nd commit, that could be used instead, what do you think?

7. 
   laanwj commented at 6:45 am on June 12, 2014: member

   Yes - I was initially against putting the BIP70 names on ChainParams as it was seemingly a UI-only thing @jtimon but with the field in getblockchaininfo it makes sense to move it to the core anyway.
8. 
   jtimon commented at 11:12 am on June 12, 2014: contributor

   Yes, I would have used a string network id chainparam like it’s done in https://github.com/Diapolo/bitcoin/commit/dcca44115dfcafb38cc435679b40fde3c8179d19 I would mention the BIP 70 there in chainparams directly. But once that is done I don’t see the need to keep the ClientModel::getNetworkName() method with the ugly elif list.

   You can just replace if (details.network() != clientModel->getNetworkName().toStdString()) with if (details.network() != Params().NetworkIDString())

   and completely forget about my PaymentServer::mapNetworkIdToName and your ClientModel::getNetworkName. That’s what I would have done from the beginning if I wasn’t adviced against it. I would also replace getinfo’s (rpcmisc.cpp:76) and getmininginfo’s (rpcmining.cpp:271) “testnet” in favor of “network”, “mode” or something like that which would also call Params().NetworkIDString(), allowing us to also get rid of CChainParams::RPCisTestNet()

9. 
   Diapolo commented at 11:14 am on June 12, 2014: none

   I’m currently reworking this…
10. add NetworkIDString() to chainparams

    - returns the BIP70 network string
    - use that new function in the core and GUI code and remove unused code
      and functions

    f5ae6c9826
11. Diapolo renamed this:
    [Qt] rework ClientModel::getNetworkName()
    add NetworkIDString() to chainparams
    on Jun 12, 2014
12. 
    Diapolo commented at 11:22 am on June 12, 2014: none

    Done…
13. 
    laanwj commented at 11:24 am on June 12, 2014: member

    Seems we have two equivalent pulls now…
14. 
    Diapolo commented at 11:25 am on June 12, 2014: none

    @laanwj Not really, I took care of clientmodel and rpcconsole also ;).
15. 
    laanwj commented at 11:26 am on June 12, 2014: member

    Right :) Code changes look OK to me, haven’t tested yet.
16. 
    jtimon commented at 11:37 am on June 12, 2014: contributor

    I created another PR (#4333) using https://github.com/jtimon/bitcoin/commit/f5ae6c98260fdd3c0ca203ce67c6467d9cdaea95 and making my other proposed change (removing chainparams::RPCisTestnet) on top of it.
17. 
    BitcoinPullTester commented at 11:41 am on June 12, 2014: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f5ae6c98260fdd3c0ca203ce67c6467d9cdaea95 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.
18. laanwj merged this on Jun 16, 2014
19. laanwj closed this on Jun 16, 2014

    ---

20. laanwj referenced this in commit 807691ca96 on Jun 16, 2014
21. Diapolo deleted the branch on Jun 17, 2014
22. MarcoFalke locked this on Sep 8, 2021

Contributors
Diapolo laanwj jtimon BitcoinPullTester