Chainparams: Use a regular factory for creating chainparams #6907

pull jtimon wants to merge 2 commits into bitcoin:master from jtimon:chainparams-factory-0.12.99 changing 11 files +64 −51
  1. jtimon commented at 9:11 pm on October 29, 2015: contributor

    This makes it simpler for people to create new testchains in their own branches by removing the need to create a new static global variable for the new chains.

    It also prepares chainparams and chainparamsbase to be cleaned out of globals by moving the globals and related functions (Params(), SelectParams(), etc) outside, maybe to globals/server.o and globals/common.o respectively.

    But that would need another PR after this, something @theuni has been wanting to do for some time. I would prefer to do this first to be able to leave the 2 factories (which don’t need to depend on any global) where they are. Part of #6382

  2. gmaxwell commented at 9:21 pm on October 29, 2015: contributor
    This adds news without frees. I really don’t think making this non-static is the right thing to do.
  3. jtimon commented at 10:32 pm on October 29, 2015: contributor
    Moved the containers from extern back to static.
  4. in src/test/alert_tests.cpp: in 5a3a718cf5 outdated 
    199@@ -199,8 +200,8 @@ BOOST_AUTO_TEST_CASE(PartitionAlert)
200     // Test PartitionCheck
201     CCriticalSection csDummy;
202     CBlockIndex indexDummy[100];
203-    CChainParams& params = Params(CBaseChainParams::MAIN);
204-    int64_t nPowTargetSpacing = params.GetConsensus().nPowTargetSpacing;
205+    const Container<CChainParams> cTestChainParams(CChainParams::Factory(CBaseChainParams::MAIN));
206+    int64_t nPowTargetSpacing = cTestChainParams.Get().GetConsensus().nPowTargetSpacing;
207
    jtimon commented at 10:45 pm on October 29, 2015:
    Probably there’s a better example of tests that don’t have to depend on the chainparam globals (in this case cGlobalSwitchingChainParams) anymore to make sure they test the same as when CBaseChainParams::MAIN is selected. Note how the const for the params’ reference wasn’t possible before.
  5. in src/chainparamsbase.cpp: in 5a3a718cf5 outdated 
    85+    return cGlobalChainBaseParams.Get();
86 }
87 
88-void SelectBaseParams(const std::string& chain)
89+CBaseChainParams* CBaseChainParams::Factory(const std::string& chain)
90 {
    dcousens commented at 5:49 am on October 30, 2015:
    Return a managed pointer instead?
    dcousens commented at 4:12 am on November 30, 2015:
    @jtimon ^
    jtimon commented at 8:29 am on November 30, 2015:
    Well, no, we’re using the scoped pointer, but outside. The factory is, I believe, more generic like this.
  6. in src/templates.hpp: in 5a3a718cf5 outdated 
     8+
 9+#include <assert.h>
10+#include <cstddef>
11+
12+template<typename T>
13+class Container
    dcousens commented at 5:50 am on October 30, 2015:
    What is the purpose of this Container? It seems to be managing the lifetime of a pointer, is a boost managed pointer not suitable?
    jonasschnelli commented at 8:11 am on October 30, 2015:

    Not sure, but we might prefer something like smart c++11 pointers (long term, once switched to c++11) over boost managed pointers (to – at least – not increase the boost library adhesion).

    If we stick with the Container class: Container as classname in the global space seems to be to general.

    dcousens commented at 8:50 am on October 30, 2015:
    @jonasschnelli of course, but the effort to swap between the two is trivial, so the point remains.
    jtimon commented at 10:04 am on October 30, 2015:
    I wanted to allow reviewers to see that we can maintain all the asserts exactly as they are right now, that the functionality with this container is almost identical to the functionality before it. But I was expecting people to propose other boost/c++11 equivalent classes. Links to the documentation of your preferred data structure are welcomed.
    jtimon commented at 1:19 pm on November 4, 2015:
    What about http://www.boost.org/doc/libs/1_59_0/libs/smart_ptr/scoped_ptr.htm ?
  7. in src/chainparamsbase.cpp: in 5a3a718cf5 outdated 
    84-    return *pCurrentBaseParams;
85+    return cGlobalChainBaseParams.Get();
86 }
87 
88-void SelectBaseParams(const std::string& chain)
89+CBaseChainParams* CBaseChainParams::Factory(const std::string& chain)
    jonasschnelli commented at 8:13 am on October 30, 2015:
    What is the reason for using std::string instead of a enum for chain identification? We still could have something like static std::string ChainParams::ToString(enum chain)
    jtimon commented at 10:12 am on October 30, 2015:
    Using strings instead of enums for chain identification selection (for identification we should use the genesis block) it’s not something that’s being changed here, it was changed in #6235. Apart from the simplifications in that PR (and the fact that this very PR would be more complicated without having done #6235 first), here’s a few other commits in #6382 that get simpler/more consistent by using strings instead of an enum: #6908 https://github.com/jtimon/bitcoin/commit/453597a3897a22ef45aa132ac285ccd71eba2397 https://github.com/jtimon/bitcoin/commit/ff440ce6f7e20a410d43cbf111d49e53ab497eb8 https://github.com/jtimon/bitcoin/commit/d644c2993454d29ee0d9c26bc9e107831e1e5b44 https://github.com/jtimon/bitcoin/commit/b7bce5c5571200361e3b74406d0f9bc986960491
  8. laanwj added the label Refactoring on Oct 30, 2015
  9. jtimon commented at 2:06 pm on November 4, 2015: contributor
    Added some commits to replace Container with boost::scoped_pointer. That also reduces the total diff (by not adding a new file), which is nice. I’m ready to squash if people are happy with boost::scoped_pointer
  10. dcousens commented at 0:47 am on November 5, 2015: contributor
    utACK
  11. dcousens commented at 0:48 am on November 5, 2015: contributor
    Nice work @jtimon
  12. jtimon force-pushed on Nov 10, 2015
  13. jtimon commented at 3:00 pm on November 10, 2015: contributor
    Squashed.
  14. jtimon referenced this in commit 10cb9b264b on Nov 25, 2015
  15. in src/chainparams.cpp: in f4d308a30e outdated 
    284+        return new CRegTestParams();
285     else
286         throw std::runtime_error(strprintf("%s: Unknown chain %s.", __func__, chain));
287 }
288 
289+const CChainParams& Params(const std::string& chain)
    sipa commented at 1:22 pm on November 28, 2015:
    This looks dangerous, as it destroys the chainparams object created by the previous caller. Can you either document this, or switch to a solution that doesn’t need that (for example, keeping a std::map<string, CChainParams)?
  16. jtimon commented at 1:26 pm on November 28, 2015: contributor
    @sipa good point. I would prefer to just remove all its uses (now the factory can be used directly instead) than to think much about solving that problem though (maybe there’s not so many calls at this point, I will check).
  17. jtimon force-pushed on Nov 28, 2015
  18. jtimon commented at 2:08 pm on November 28, 2015: contributor
    Added a commit to get rid of CChainParams& Params(std::string). It’s not very big (+18 -23) but it required me to rebase.
  19. sipa commented at 3:33 pm on November 28, 2015: member
    test/miner_tests.cpp:62 needs the old function still.
  20. jtimon force-pushed on Nov 28, 2015
  21. jtimon commented at 4:45 pm on November 28, 2015: contributor
    Oops, fixed.
  22. jtimon force-pushed on Nov 28, 2015
  23. jtimon force-pushed on Nov 29, 2015
  24. jtimon commented at 11:18 pm on November 29, 2015: contributor
    Rebased(1)
  25. dcousens commented at 4:15 am on November 30, 2015: contributor
    re-ACK
  26. jtimon force-pushed on Nov 30, 2015
  27. jtimon commented at 11:43 am on November 30, 2015: contributor
    @MarcoFalke Thanks for the heads up, build fixed.
  28. MarcoFalke commented at 2:04 pm on December 2, 2015: member
    Needs rebase due to #7128
  29. jtimon force-pushed on Dec 3, 2015
  30. jtimon commented at 11:50 am on December 3, 2015: contributor
    Rebased(2) [2 since I started counting for this particular PR. But although the commit says may 22, I’ve been trying to introduce a proper factory since much longer, at the very least since Nov 6, 2014 see #5229. I’ve been rebasing maintaining it in one way or another since then, but, still, I’ll pretend I understand why this is not ready for 0.12 and that #7128 has been merged first. I’ll keep rebasing this particular PR until it’s finally merged or nacked, but I’m afraid I won’t be able to make this kind of promise many more times].
  31. in src/bitcoin-cli.cpp: in d4bb7a8a7d outdated 
    34@@ -34,7 +35,7 @@ std::string HelpMessageCli()
35     strUsage += HelpMessageOpt("-datadir=<dir>", _("Specify data directory"));
36     AppendParamsHelpMessages(strUsage);
37     strUsage += HelpMessageOpt("-rpcconnect=<ip>", strprintf(_("Send commands to node running on <ip> (default: %s)"), DEFAULT_RPCCONNECT));
38-    strUsage += HelpMessageOpt("-rpcport=<port>", strprintf(_("Connect to JSON-RPC on <port> (default: %u or testnet: %u)"), BaseParams(CBaseChainParams::MAIN).RPCPort(), BaseParams(CBaseChainParams::TESTNET).RPCPort()));
39+    strUsage += HelpMessageOpt("-rpcport=<port>", strprintf(_("Connect to JSON-RPC on <port> (default: %u)"), defaultBaseParams->RPCPort()));
    laanwj commented at 11:56 am on December 3, 2015:
    I don’t like this change, it is useful to have the port numbers for both networks in the help output (and we have the same for -port)
    jtimon commented at 12:38 pm on December 3, 2015:
    And I don’t like special cases for testnet3 (or any other chain), which I usually find with rgrep “test” or rgrep CBaseChainParams::TESTNET. Maybe we can loop through all the default ports for each supported chain as in https://github.com/jtimon/bitcoin/commit/28d961ff939bfc0887c6c8b8b288733864559abd ? I mean, that would require something like https://github.com/jtimon/bitcoin/commit/042fdb0f58313fbade482cc53e39aed0421437d8 first.
    laanwj commented at 12:47 pm on December 3, 2015:

    As said, we’re doing exactly the same for -port. What makes -rpcport different? #7128 unified these.

    Looping through all networks would be overkill, there’s no good reason to list regtest here, which is hidden (unless –help-debug). I think the current solution is fine.

    jtimon commented at 1:10 pm on December 3, 2015:
    @laanwj what about https://github.com/jtimon/bitcoin/commit/dd3547118df166704061fdce3aabb198236c8fca ? This way you can bitcoind -testnet --help but bitcoind -regtest --help too.
    jtimon commented at 1:14 pm on December 3, 2015:
    re difference between -port and rpc-port, I’m removing the testnet3 special case from port too, see https://github.com/bitcoin/bitcoin/pull/6907/files#diff-c865a8939105e6350a50af02766291b7R374
    jtimon commented at 1:29 pm on December 3, 2015:
    rewarding unification, the change in port was also introduced only 5 days ago in #6961
    laanwj commented at 1:30 pm on December 3, 2015:

    This way you can bitcoind -testnet –help but bitcoind -regtest –help too.

    I don’t think that is better than just listing both in the standard help message. Parametrizing the help message on the network, I dunno, that’s not something I’d expect as user.

    jtimon commented at 5:31 pm on December 3, 2015:
    That can be documented, but I’ll just restore the old messages and make another local variable for testnet3.
    jtimon commented at 5:31 pm on December 3, 2015:
    well, 2 more.
    jtimon commented at 0:26 am on December 4, 2015:
    Regarding looping and not wanting to show regtest’s information, we could easily do that just by leaving regtest out of supportedChains (see https://github.com/jtimon/bitcoin/commit/160d874dfdb53c71a07b1bf7bdd0dab571dabefa#diff-64cbe1ad5465e13bc59ee8bb6f3de2e7R20 ). Although that would also leave it out from https://github.com/jtimon/bitcoin/commit/160d874dfdb53c71a07b1bf7bdd0dab571dabefa#diff-de33fb0129cd5121c7098035527b26fdR14 and https://github.com/jtimon/bitcoin/commit/28d961ff939bfc0887c6c8b8b288733864559abd , which is probably not a big deal. The special case for testnet3 can be solved by replacing a “main and testnet” enumaration whether we loop around 2 or 3 elements. Just a thought. Or we can just leave it as it is, it’s just I thought that I had when I was paranoid about https://github.com/jtimon/bitcoin/commit/28d961ff939bfc0887c6c8b8b288733864559abd maybe being controversial (as a “too altcoinish” commit) and I just remembered. Of course, that can be done later if it’s done.
  32. jtimon force-pushed on Dec 3, 2015
  33. jtimon commented at 7:58 pm on December 3, 2015: contributor
    Restored the testnet3 special case as requested by @laanwj
  34. in src/init.cpp: in d7938c81f9 outdated 
    476@@ -473,7 +477,7 @@ std::string HelpMessage(HelpMessageMode mode)
477 
478     strUsage += HelpMessageGroup(_("Node relay options:"));
479     if (showDebug)
480-        strUsage += HelpMessageOpt("-acceptnonstdtxn", strprintf("Relay and mine \"non-standard\" transactions (%sdefault: %u)", "testnet/regtest only; ", !Params(CBaseChainParams::TESTNET).RequireStandard()));
481+        strUsage += HelpMessageOpt("-acceptnonstdtxn", strprintf("Relay and mine \"non-standard\" transactions (%sdefault: %u)", "testnet/regtest only; ", defaultChainParams->RequireStandard()));
    MarcoFalke commented at 8:05 pm on December 3, 2015:
    Where is the !?
    jtimon commented at 8:30 pm on December 3, 2015:
    For some reason we we’re using !testnet, which is the same as main and !!main, but the opposite of !!testnet.
    jtimon commented at 8:36 pm on December 3, 2015:
    I wanted to have solved it ages ago in https://github.com/jtimon/bitcoin/commit/3d03f15126b79bd9f22ccd27f1d8ca43d9e16bf5 (#6423) but that commit depended on the cursed PR #6068 (just as cursed as its several predecessors).
    MarcoFalke commented at 8:44 pm on December 3, 2015:
    In theory it’s not defined for main, it’s defined for testnet/regtest only;
    jtimon commented at 8:50 pm on December 3, 2015:
    For main, it is defined in https://github.com/bitcoin/bitcoin/blob/master/src/chainparams.cpp#L120
    jtimon commented at 8:57 pm on December 3, 2015:
    What that text refers to, is that users aren’t currently allowed to -acceptnonstdtxn=true on mainnet, they can only set -acceptnonstdtxn=false in testnet/regtest, which makes the fact that the displayed default is false (when in testnet/regtest the default is actually true) even more stupid. I wanted to have solved this in https://github.com/jtimon/bitcoin/commit/a4b28c491202a1d6c6e77f9439b08b3b2613e818 but it depends on the previously mentioned commit.
    MarcoFalke commented at 1:30 pm on January 7, 2016:
    What about default: %u -> default: %u (testnet), default: %u (regtest)?
    jtimon commented at 6:44 pm on January 7, 2016:
    I really think we should solve this by allowing -acceptnonstdtxn=true on mainnet. See https://github.com/jtimon/bitcoin/commit/a4b28c491202a1d6c6e77f9439b08b3b2613e818
  35. in src/bitcoin-cli.cpp: in d7938c81f9 outdated 
    26@@ -27,6 +27,8 @@ static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
27 
28 std::string HelpMessageCli()
29 {
30+    const boost::scoped_ptr<CBaseChainParams> defaultBaseParams(CBaseChainParams::Factory(CBaseChainParams::MAIN));
31+    const boost::scoped_ptr<CBaseChainParams> testnetBaseParams(CBaseChainParams::Factory(CBaseChainParams::MAIN));
    dcousens commented at 8:09 pm on December 3, 2015:
    CBaseChainParams::TESTNET?
    jtimon commented at 8:24 pm on December 3, 2015:
    oops, just realized locally now…
    jtimon commented at 8:29 pm on December 3, 2015:
    Fixed
  36. jtimon force-pushed on Dec 3, 2015
  37. dcousens commented at 8:33 pm on December 3, 2015: contributor
    re-utACK
  38. jtimon commented at 12:38 pm on January 7, 2016: contributor
    ping
  39. jtimon force-pushed on Jan 13, 2016
  40. jtimon commented at 8:36 pm on January 13, 2016: contributor
    Rebased (3)
  41. dcousens commented at 10:02 pm on January 28, 2016: contributor
    re-ACK bd13782
  42. Chainparams: Use a regular factory for creating chainparams 22ce3d1eaf
  43. jtimon force-pushed on Feb 20, 2016
  44. jtimon commented at 4:14 am on February 20, 2016: contributor
    Rebase(4)
  45. Chainparams: Get rid of CChainParams& Params(std::string) 670a464d34
  46. jtimon force-pushed on Feb 22, 2016
  47. jtimon commented at 10:30 am on February 26, 2016: contributor
    This is way older than “Oct 29, 2015”, @laanwj can you please merge it or close it?
  48. jtimon closed this on Mar 3, 2016
  49. laanwj commented at 1:27 pm on March 3, 2016: member

    I’m not convinced of the need for this. At least to me it doesn’t really make the code easier to understand or follow.

    Do you need this for anything specific?

  50. jtimon commented at 1:40 pm on March 3, 2016: contributor

    I’ve provided many examples of how this could be useful in the past (including the sizetest testchain, sorry, I don’t have the pr number at hand). This also allows to remove effectively-global-variables from chainparams (without moving the factory out) as discussed very long ago with @theuni. Hopefully my refactors will make more sense as part of bitcoin jt once it starts implementing extra functionality in minimal ways (thanks to the previous refactors). And hopefully somebody will look at that branch and ask for things from it to be backported to bitcoin.

    Thank you for asking about what you don’t understand, even if it’s very late. A “no” is always better than continued silence.

  51. laanwj commented at 1:45 pm on March 3, 2016: member

    Thank you for asking about what you don’t understand, even if it’s very late. A “no” is always better than continued silence.

    Yes, sorry for that. I’m just very busy. I have trouble keeping up with all the pulls, and I need to prioritize things that fix bugs or address immediate issues to things that are more speculative like refactors. I wish it was different, and we had a paid full-time team on this and it didn’t rest for such a large part on my shoulders :(

  52. jtimon referenced this in commit 62f96fb5a7 on Mar 11, 2016
  53. jtimon referenced this in commit 7180042104 on Mar 11, 2016
  54. jtimon referenced this in commit 1cdb9c8b5f on Apr 6, 2016
  55. MarcoFalke locked this on Sep 8, 2021


jtimon gmaxwell dcousens jonasschnelli sipa MarcoFalke laanwj

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me