Chainparams: Separate globals and depedendent functions #6359

1. 
   jtimon commented at 0:14 am on July 1, 2015: contributor

   This shares the first commit with #6230. This is an alternative to https://github.com/theuni/bitcoin/commit/123f3b0dcef29355060290d56138ff99be212007 which @theuni wasn’t even proposing yet. But I would like your feedback on the factory and the container (which are shared with #6068 ). This is blocking #6229 and #5229. EDIT: The fixup commit cleaning up the includes may make more sense after more of #5970 (for example, #6163, we’re not that far away from making miner.cpp independent from the chainparams globals) is left for later.
2. Chainparams: Replace CBaseChainParams::Network enum with string constants (suggested by Wladimir) 246b4966c1
3. Introduce Container template a74eb425d2
4. Chainparams: Use a regular factory for creating chainparams d1b02c52e2
5. jtimon force-pushed on Jul 1, 2015
6. jtimon force-pushed on Jul 1, 2015
7. jtimon force-pushed on Jul 1, 2015
8. Chainparams: Translations: DRY: options and error strings

   Also remove SelectBaseParamsFromCommandLine and SelectParamsFromCommandLine

   19c3645778
9. remove SelectBaseParams(), BaseParams() and AreBaseParamsConfigured() 7fb9f6ad7b
10. MOVEONLY: Chainparams: Separate globals and functions depending on them db85b3d747
11. jtimon force-pushed on Jul 2, 2015
12. 
    jtimon commented at 2:36 pm on July 2, 2015: contributor

    Since I’m still working on this…closing for now. I mainly wanted to ping @theuni and what is here works and is enough to start receiving feedback.
13. jtimon closed this on Jul 2, 2015

    ---

14. 
    theuni commented at 4:43 pm on July 2, 2015: member

    @jtimon I’m not sure it’s worth the complication of a factory here. Given that all chainparams and consensusparams are immutable, why not just pass around new instances when necessary? That should still be cheap, and much simpler? Especially given the trend to pass the params around as necessary rather than call Params() directly.

    Something like:

     0static CBaseChainParams::Network nCurrentParams = CBaseChainParams::MAX_NETWORK_TYPES;
     1
     2const CChainParams Params() {
     3    switch (nCurrentParams) {
     4        case CBaseChainParams::MAIN:
     5            return CChainParams(CMainParams());
     6        case CBaseChainParams::TESTNET:
     7            return CChainParams(CTestNetParams());
     8        case CBaseChainParams::REGTEST:
     9            return CChainParams(CRegTestParams());
    10        default:
    11            assert(false && "Unimplemented network");
    12            return CChainParams(CMainParams());
    13    }
    14}
    15
    16void SelectParams(CBaseChainParams::Network network) {
    17    SelectBaseParams(network);
    18    nCurrentParams = network;
    19}
    

    Hell, since there are no virtual functions left (nor should there be), we could even do away with the inheritance and just make CBaseChainParams::Network a ctor arg for CChainParams.

15. 
    jtimon commented at 7:22 pm on July 2, 2015: contributor

    The main point of the factory is that it is not on globals/ because once we’re finished “passing chainparams around” there should be no need for any global at all: You either take it as parameter or use the factory (this includes removing the SelectParams from BasicTestingSetup::BasicTestingSetup(), since nobody depends on the global and tests can use the factory directly).

    But if copying the object in the factory (like you do with the Params() of your example), then the factory doesn’t need to return a pointer (it could just be the current parametrized Params() ), and the Container would also be unnecessary (and the same goes for CPolicy).

    The enum doesn’t scale very well once you go beyond -testnet and -regtest (see #5229 in fact, I’m preparing a PR to add infinite new testing altchains). The same happens with the single constructor parameter. But you are right, the inheritance is overkill here. With a fully parametric constructor the factory can just always call that constructor with different values. ie:

    0CChainParams* Params(const std::string& chain)
    1{
    2   if (chain == CBaseChainParams::MAIN)
    3        return new CChainParams("main", 210000, 750, 950, 1000...);
    4    else if (chain == CBaseChainParams::TESTNET)
    5 ...
    

16. 
    jtimon commented at 2:20 pm on July 6, 2015: contributor

    @theuni I’ve been exploring your suggestion and it’s not so straight-forward as you may think. If Params(string) returns a new object instead of a reference some places will crash, like this one: https://github.com/bitcoin/bitcoin/blob/master/src/test/alert_tests.cpp#L125 It also means that Params() and BaseParams() have to return a new object as well instead of a const reference. It works fine for ParamsBase, which doesn’t have a similar &ParamsBase(enum/string) psedo-factory. But I really prefer to just remove the interface functions in this case (where it’s easy, see https://github.com/jtimon/bitcoin/commit/7fb9f6ad7bad4b64df0fdb36e5230becd2290864 ). Can we first agree that it will be better to remove SelectParamsFromCommandLine and SelectBaseParamsFromCommandLine before separating the globals? I have reopened #6235 with that. @rustyrussell you may be interested in this discussion, or reviewing #6235.
17. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon theuni