Tests: Chainparams: Make regtest almost fully customizable #17032

pull jtimon wants to merge 2 commits into bitcoin:master from jtimon:b20-customizable-regtest changing 3 files +105 −49
  1. jtimon commented at 10:20 pm on October 2, 2019: contributor

    Almost all chain params fields are made configurable for regtest and documented in –help for debug. Unlike #8994 this doesn’t allow to create an arbitrary number of regtests with different genesis blocks, but it also doesn’t touch consensus code nor changes all the tests from “regtest” to “regtest2” to make sure the custom genesis blocks work, so it should be much easier to review in comparison.

    This is supposed to save people from having to create more set methods on CRegTestParams in the future. This is also supposed to force programmers (that’s us) to maintain a documentation readable with –help for any present or future chainparam and their modifications.

    As a simple example, it is tested that regtest can disallow setting -acceptnonstdtxn=1, which is something, for example, signets may want some times. If someone doesn’t like the example chainparam field I picked for the test for whatever reason, anybody’s welcomed to suggest some other field or set of fields to test in a new test.py file. I’m happy to code it myself or take someone else’s test as a replacement.

    A long time ago, we talked about loading the chainparams from a file. Well, combined with the existing section (by chain) feature for loading the config file, this provides that feature.

  2. DrahtBot added the label Tests on Oct 2, 2019
  3. DrahtBot added the label Validation on Oct 2, 2019
  4. DrahtBot commented at 8:30 pm on October 3, 2019: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18267 (BIP-325: Signet [consensus] by kallewoof)
    • #17556 (test: Change feature_config_args.py not to rely on strange regtest=0 behavior by ryanofsky)
    • #16770 (Chainparams: Rename IsTestChain() to AllowAcceptNonstd() by jtimon)
    • #16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. MarcoFalke commented at 2:03 pm on October 8, 2019: member
    Concept ACK. Didn’t look at the approach.
  6. in src/chainparams.cpp:294 in 6c54f7d447 outdated
    290@@ -334,10 +291,12 @@ class CRegTestParams : public CChainParams {
    291         consensus.vDeployments[d].nTimeout = nTimeout;
    292     }
    293     void UpdateActivationParametersFromArgs(const ArgsManager& args);
    


    fjahr commented at 5:45 pm on October 28, 2019:
    Since this is called inside of UpdateFromArgs() I think this line can be removed?

    jtimon commented at 7:10 pm on October 29, 2019:

    No, the method still needs to be declared.

     0chainparams.cpp:296:6: error: no declaration matches void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager&)
     1 void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
     2      ^~~~~~~~~~~~~~
     3chainparams.cpp:296:6: note: no functions named void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager&)
     4chainparams.cpp:254:7: note: class CRegTestParams defined here
     5 class CRegTestParams : public CChainParams {
     6       ^~~~~~~~~~~~~~
     7chainparams.cpp: In member function void CRegTestParams::UpdateFromArgs(const ArgsManager&):
     8chainparams.cpp:342:5: error: UpdateActivationParametersFromArgs was not declared in this scope
     9     UpdateActivationParametersFromArgs(args);
    10     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    11chainparams.cpp:342:5: note: suggested alternative: UpdateVersionBitsParameters
    12     UpdateActivationParametersFromArgs(args);
    13     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    14     UpdateVersionBitsParameters
    
  7. in src/chainparams.cpp:257 in 6c54f7d447 outdated
    254@@ -255,54 +255,18 @@ class CRegTestParams : public CChainParams {
    255 public:
    256     explicit CRegTestParams(const ArgsManager& args) {
    257         strNetworkID = "regtest";
    


    fjahr commented at 5:45 pm on October 28, 2019:
    nit: could set this string from CBaseChainParams::REGTEST.

    jtimon commented at 7:32 pm on October 29, 2019:
    mhmm. I remember this failing to compile or to run at some point, but it seems to work now. Perhaps it had to do with some older version of cpp or something. Or perhaps I misremember and it was only networksyle.cpp where I wasn’t able to to it. In any case, since this can be done for other chains other than regtest and in other places other than chainparams.cpp, I’m going to do it in another PR: https://github.com/bitcoin/bitcoin/pull/17306
  8. fjahr commented at 5:46 pm on October 28, 2019: member
    Concept ACK. Did a light code review.
  9. DrahtBot added the label Needs rebase on Oct 30, 2019
  10. jtimon force-pushed on Oct 30, 2019
  11. jtimon commented at 12:53 pm on October 30, 2019: contributor
    Rebased after #17306 was merged. Fixed nit in #17306 (review) here.
  12. DrahtBot removed the label Needs rebase on Oct 30, 2019
  13. DrahtBot added the label Needs rebase on Feb 18, 2020
  14. jtimon force-pushed on Feb 19, 2020
  15. jtimon commented at 8:41 pm on February 19, 2020: contributor
    Rebased
  16. Chainparams: Make regtest almost fully customizable 340d62d48d
  17. Tests: Make sure regtest is now customizable 2aaf473e57
  18. jtimon force-pushed on Feb 19, 2020
  19. DrahtBot removed the label Needs rebase on Feb 19, 2020
  20. Sjors referenced this in commit 1bf328127f on Mar 3, 2020
  21. in src/chainparamsbase.cpp:48 in 340d62d48d outdated
    43+    gArgs.AddArg("-ndefaultport", "The port to listen for connections on by default. Consider using -port instead of changing the default.  Default: 18444 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    44+    gArgs.AddArg("-npruneafterheight", "Only start prunning after this height. Default: 1000 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    45+    gArgs.AddArg("-assumed_blockchain_size", "Estimated current blockchain size (in GB) for UI purposes. Default 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    46+    gArgs.AddArg("-assumed_chain_state_size", "Estimated current chain state size (in GB) for UI purposes. Default 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    47+    gArgs.AddArg("-fdefaultconsistencychecks", "Whether -checkblockindex and -checkmempool are active by default or not. Consider using those options instead. Default: 1 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    48+    gArgs.AddArg("-frequirestandard", "Whether standard policy rules are applied in the local mempool by default. Consider using -acceptnonstdtxn=0 instead of changing the default. Default: 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    


    Sjors commented at 4:56 pm on March 3, 2020:
    Why even expose this option if -acceptnonstdtxn=0 does the trick?
  22. in src/chainparamsbase.cpp:49 in 340d62d48d outdated
    44+    gArgs.AddArg("-npruneafterheight", "Only start prunning after this height. Default: 1000 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    45+    gArgs.AddArg("-assumed_blockchain_size", "Estimated current blockchain size (in GB) for UI purposes. Default 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    46+    gArgs.AddArg("-assumed_chain_state_size", "Estimated current chain state size (in GB) for UI purposes. Default 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    47+    gArgs.AddArg("-fdefaultconsistencychecks", "Whether -checkblockindex and -checkmempool are active by default or not. Consider using those options instead. Default: 1 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    48+    gArgs.AddArg("-frequirestandard", "Whether standard policy rules are applied in the local mempool by default. Consider using -acceptnonstdtxn=0 instead of changing the default. Default: 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    49+    gArgs.AddArg("-is_test_chain", "Whether it's allowed to set -acceptnonstdtxn=0 for this chain or not. Default: 1 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    


    Sjors commented at 4:57 pm on March 3, 2020:
    This seems useless?
  23. in src/chainparamsbase.cpp:50 in 340d62d48d outdated
    45+    gArgs.AddArg("-assumed_blockchain_size", "Estimated current blockchain size (in GB) for UI purposes. Default 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    46+    gArgs.AddArg("-assumed_chain_state_size", "Estimated current chain state size (in GB) for UI purposes. Default 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    47+    gArgs.AddArg("-fdefaultconsistencychecks", "Whether -checkblockindex and -checkmempool are active by default or not. Consider using those options instead. Default: 1 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    48+    gArgs.AddArg("-frequirestandard", "Whether standard policy rules are applied in the local mempool by default. Consider using -acceptnonstdtxn=0 instead of changing the default. Default: 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    49+    gArgs.AddArg("-is_test_chain", "Whether it's allowed to set -acceptnonstdtxn=0 for this chain or not. Default: 1 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    50+    gArgs.AddArg("-is_mockable_chain", "Whether mocktime rpc calls are allowed or not. Default: 1 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    


    Sjors commented at 4:58 pm on March 3, 2020:
    Can you think of a use case where disabling mocktime on regtest makes sense?
  24. in src/chainparamsbase.cpp:52 in 340d62d48d outdated
    47+    gArgs.AddArg("-fdefaultconsistencychecks", "Whether -checkblockindex and -checkmempool are active by default or not. Consider using those options instead. Default: 1 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    48+    gArgs.AddArg("-frequirestandard", "Whether standard policy rules are applied in the local mempool by default. Consider using -acceptnonstdtxn=0 instead of changing the default. Default: 0 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    49+    gArgs.AddArg("-is_test_chain", "Whether it's allowed to set -acceptnonstdtxn=0 for this chain or not. Default: 1 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    50+    gArgs.AddArg("-is_mockable_chain", "Whether mocktime rpc calls are allowed or not. Default: 1 (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    51+    gArgs.AddArg("-bech32_hrp", "Human readable part for bech32 addresses. See BIP173 for more info. Default: bcrt (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    52+    gArgs.AddArg("-pubkeyprefix", "Magic for base58 pubkeys. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    


    Sjors commented at 5:00 pm on March 3, 2020:
    Light preference for making this hex, to be consistent with -extpubkeyprefix and friends.
  25. in test/functional/feature_config_args.py:45 in 2aaf473e57
    41@@ -42,9 +42,9 @@ def test_config_file_parser(self):
    42             self.nodes[0].assert_start_raises_init_error(expected_msg='Error: Config setting for -wallet only applied on %s network when in [%s] section.' % (self.chain, self.chain))
    43 
    44         with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
    45-            conf.write('regtest=0\n') # mainnet
    46+            conf.write('is_test_chain=0\n') # like mainnet
    


    Sjors commented at 5:06 pm on March 3, 2020:
    This seems unsafe. We really want to test with actual mainnet, see e.g. #17449.
  26. Sjors commented at 5:14 pm on March 3, 2020: member

    Concept ACK. Lightly tested 2aaf473e57de8e4a016372273a95dd89e68caf7f in a different project, by simulating a chain split caused by a different halving interval. It sets up two nodes, a regular one (A) and one (B) with -con_nsubsidyhalvinginterval=10. It generates a longer chain on A, which is rejected by B, and shows up as invalid in getchaintips. A test like that would be useful here.

    You may also want to add tests that at least touch the other params, and e.g. make sure the node doesn’t complain about unrecognized params.

  27. jtimon commented at 1:35 pm on May 6, 2020: contributor
    Thanks for all the reviews, really. But I’m no longer interested in this. Please, if anyone is interested, don’t hesitate to take the branch and improve it and try, rewrite it or whatever.
  28. jtimon closed this on May 6, 2020

  29. DrahtBot locked this on Feb 15, 2022

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-11-21 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me