Make REGTEST quack like a duck #3812

1. 
   jtimon commented at 6:53 am on March 7, 2014: contributor

   Less calls to functions that could be methods of CChainParams.
2. 
   laanwj commented at 7:06 am on March 7, 2014: member

   I usually recommend against core changes just to make it easier for the GUI. In this case, the logic “is a test net?” is only used in the outer function to determine whether to tint the client green or not. NACK on moving it to ChainParams.
3. 
   jtimon commented at 7:31 am on March 7, 2014: contributor

   Thanks for the feedback. Thinking more about it, doesn’t really make much sense. Another related idea would be to move TestNet() and RegTest() to be methods of CChainParams.

   TestNet() may seem more convenient than Params().testNet() or Params().isTestNet() but makes the code less readable. Actually my preference would be params.isTestNet(), using a simple instance instead of a const global-ish function, but it’s probably better to refactor a little bit at a time. Could I make that change?

4. 
   laanwj commented at 7:44 am on March 7, 2014: member

   The intention was to get rid of TestNet/RegTest eventually.

   The idea behind CChainParams is to encompass the properties “quacks like a duck” of the chain.

   TestNet() / RegTest() are “is this specific chain the …” identify functions. Moving them to the object is a bit strange in my eyes as by definition there is only one TestNet and one RegTest.

   The idea would be to split the TestNet() / RegTest() up into specific properties of the appropriate networks. However the few calls to TestNet/RegTest left are very specialized properties like “switch difficulty to min after nTargetSpacing*2 time between blocks” for which it’s likely not worth making individual queries (come up with a name for that!).

5. 
   jtimon commented at 8:14 am on March 7, 2014: contributor

   Ok, that makes sense. So, yes, the proposed change is not a big move in that direction. I’m also noticing that there’s no consisntency in the use of “Params().NetworkID() == CChainParams::TESTNET” vs “TestNet()”. I now have a clearer idea of the changes I can do to like this more. Hopefully you will like my next push more. I’ll reuse this pull request and change the tittle.
6. Consistency in the use of Params().NetworkID() (use it less) b571b9da97
7. Get rid of Params().NetworkID() ccb6e3eab9
8. 
   jtimon commented at 7:40 pm on March 7, 2014: contributor

   Any idea why this could have failed?

   I’m going back and forth on whether “Params().NetworkID() ==” or TestNet() is better but we should certainly only have one or the other and be consistent. My initial thought that was “Params().NetworkID() ==” was different from “Params().NetworkID() !=” and harder to grep, but that could be easily solved with Params().isNetwork(network) or something like that.

   Anyway, now it’s easier for me to get rid of Params().isRegtest() which is really my goal here (moving towards the “quack like a duck”).

   So this is definitely not prepared for merging yet, but it’s good to have some feedback in the meantime.

9. MainNet is the special case for the GUI 7fe2c85e71
10. CheckMemPool() and MiningRequiresPeers() isntead of isRegTest 941d25e696
11. Move DefaultMinerThreads to chain params 337164b797
12. MineBlocksOnDemand() instead of isRegTest() 9cf034e46f
13. Restore NetworkID(), get rid of isRegTest() 992f5be1e1
14. 
    jtimon commented at 9:28 pm on March 7, 2014: contributor

    Now REGTEST “quacks like a duck”. I’m thinking that the test errors may be related to the windows or mac build. Hopefully restoring NetworkID will make it disappear?

    It’s useful for me to destroy and restore things when refactoring, but maybe is not so good for merging? Feedback on that welcomed as well

15. 
    BitcoinPullTester commented at 10:09 pm on March 7, 2014: none

    Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/992f5be1e106affc55109749ac7940cd11b5cef7 for binaries and test log.

    This could happen for one of several reasons:

    1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
    2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
    3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
    4. The test suite fails on either Linux i386 or Win32
    5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

    If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

    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.

16. 
    jtimon commented at 1:39 am on March 8, 2014: contributor

    Ok, it seems I’m not building on top of the latest changes, I’ll start again. Closing.
17. jtimon closed this on Mar 8, 2014

    ---

18. jtimon deleted the branch on Mar 10, 2014
19. DrahtBot locked this on Sep 8, 2021

Contributors
jtimon laanwj BitcoinPullTester