Testchains: Introduce custom chain whose constructor... #8994

...reads from runtime params and simplify the creation of partitioned chains by simply generating different gensis block hashes from a given custom name.

It also allows to customize any chain param in these custom chains (but not the other chains).

Dependencies:

• Testschains: Many regtests with different genesis and default datadir #17037
• Tests: Chainparams: Make regtest almost fully customizable #17032
• Tests: Use self.chain instead of 'regtest' in all current tests #16681
• Preparations for more testchains #16680
• Use a proper factory for creating chainparams #8855 - [ ] Really don't validate genesis block #9102
• Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs #9494
• QA: segwit.py: s/find_unspent/find_spendable_utxo/ #11869
• Refactor: One CBaseChainParams should be enough #12128

Other features:

• Uses a custom chain for all python tests.
• Create new testchains with different genesis hashes at will.
• Load chainparams from separated file or command line. (file left for later, see https://github.com/jtimon/bitcoin/tree/b16-new-testnet-file )
• New chains are neither orange, blue nor green: they're purple and have your custom chain petname shown in the GUI.
• Extra context: some people asked for signed blocks but that's way more disruptive and this is already review-thirsty (see #9177 ).

Concept ACK

Some ideas from IRC today: Allow users to specify a .json file with all the parameters, and include an option for federated signing with list of signing keys.

Needed rebase. Also, new configurable parameter was introduced in #9053

Rebased on top of #9102

Since #8855 needed rebase, this one too. Also adapted some more rpc tests to use the custom chain instead of regtest (only 4 missing it seems, but travis should pass).

Needed rebase.

Update:

• Now all rpc tests pass for self.chain="regtest"

"Magic" fixes:

• pruning.py and mempool_packages.py now pass with both regtest and custom (they were passing travis before because they are in the extended set of tests).

"Magic" fails:

• Now p2p-segwit.py only passes with regtest but not with custom like before

This is related to #9102 (see #9102 (comment) ping @laanwj ) in the sense that we cannot test that the genesis block is not validated unless we can run the system for a chain whose genesis block doesn't comply with the rules. For example, the custom chain (unlike regtest) doesn't comply with pow (although some values for -chainpetname should make it). @gmaxwell, you suggested that something like #9177 would need a lot of coverage testing and review, and I completely agree. Any suggestion to advance in that front in this PR instead of #9177 (which currently doesn't work and has one unittest that is not independent) would be welcomed.

You have recently made improvements in the rpc test framework, @MarcoFalke , and are familiar with it. If you get bored and can help fix one of the 3 tests that aren't passing for custom or just comment about the python changes in general, that would be great.

REM:

• The last 2 commits are not necessary, but anyone feel free to comment on them.
• Except for p2p-compactblocks.py, segwit.py (and now p2p-segwit.py too) all rpc tests pass with "custom" too

@TheBlueMatt said in #9243 :

I think for things which are their own classes, they should either have the arguments they care about passed into the constructor, or passed in when they need them (eg the way mempool limiting is done now)

The longer version of CreateChainParams could indeed take all fields in CChainParams and Consensus::Params as arguments, but that would break the encapsulation. Every time a field is added or removed all calls would need to be adapted. Note that ideally, in the future, all tests would rely on CreateChainParams but none of them on the chainparams globals (accessed to via SelectParams and Params() ).

Another simpler option would be to simply let chainparams.cpp depend on globals mapArgs and mapMultiArgs (it depends on util.o anyway).

Yet another option is to undo some of that changes in #9243 if that gets merged first, which would be my personal preference.

If anyone has more options, I'm all ears.

Rebased

Rebased

As a reminder, segwit.py and p2p-compactblocks.py still need to run with self.chain = "regtest" instead of self.chain = "custom". Perhaps that's ok?

Needs rebase after #9243

Rebased, now including #9494 as a dependency. At the same time, some simplifications have been done in the last rebase like less disruption to avoid usage of globals in SelectParams. Also not do anything with bip9params like before (it is allowed for both retest and custom, unless you set fmineblocksondemand=0 in your custom chainparams.conf). Trying to move the bip9params args from regular args to the custom chainparams config file what was what causing errors unless self.chain = "regtest" before in p2p-segwit.py and p2p-compactblocks.py.

segwit.py is still failing with self.chain = "custom". Unless I'm missing something else, it all points to hardcoded hashes depending on the genesis block's, which should be the only different thing from regtest to custom without touching the custom chain defaults.

Sorry for the back and forth, but we only need #9494 as a dependency if we want the args to be loaded from a different .conf file. If loading from the same conf file (or regular command line args) is good enough as a first step there's no need to interfere with #9494 or temporarily creating a more generic version of ReadConfigFile that doesn't take the lock. In case that's a strong requirement, I left a version that includes #9494 and loads from a separate file at https://github.com/jtimon/bitcoin/tree/0.13-new-testchain-separated-file

Concept Ack; I really like this, especially as lightning's spec has resorted to specifying a full blockchain for the test vectors (only way to get specific txids, see https://github.com/lightningnetwork/lightning-rfc/blob/master/03-transactions.md#appendix-c-funding-transaction-test-vectors )

Rebased, but the python tests were broken again.

Sorry, false alarm, it was just pruning which seems to be dependent on the regtest genesis block too. In the process I noticed that I was missing option con_defaultassumevalid.

Needed rebase like #8855 and in some python tests. Also fixed pruning.py so that it can run with self.chain="custom" again.

Needed rebase for python tests. Also, after #8855 has been merged, it goes from 9 commits and +248-137 to 6 commits and +161-55. Some squashing could be done too.

concept ACK

what's the presumed use-case for the petname? A way to introduce some randomness into the genesis block? Also long-term what is the role of regtest in the functional test framework in your view since you're replacing it in most tests?

Fixed some nits

what's the presumed use-case for the petname? A way to introduce some randomness into the genesis block?

It allows you to create another chain with a different genesis block but with the exact same parameters on the rest. For example, let's say people create "lightningtestnet", then gets attacked and people just need to change the petname "lightningtestnet2" to coordinate another chain. It can also be helpful for reading conf files if you maintain several, ie:

lightningtest.conf

chain=custom
chainpetname=lightningtest

otherconfig.conf

chain=custom
chainpetname=lightningtest2

But this is less valuable since you can simple use the file name for that.

Also long-term what is the role of regtest in the functional test framework in your view since you're replacing it in most tests?

I think we should consider removing it in the future (we would need to declare it deprecated first, or maybe we can rename custom to regtest and explain the genesis block for regtest changed), but note that after this segwit.py still uses regtest (presumably for its genesis block).

What's the reason for the dependency on #9102? Also, you could remove #8855 from the list of dependencies in the PR description since it's now merged.

@ryanofsky not sure if it occurs here, but if a custom chain has a PoW that fails the minimum PoW check, it will fail to validate.

Updated the list. Yes, that's the reason for #9102, so that any arbitrary genesis block doesn't fail on pow checks (whether the chain is regtest-like or not). At the same time, I think this PR is the best way to test this, so perhaps #9102 should be closed as independent?

@jtimon Agreed, I think it fails to be mergable if not packaged with tests. Closing mine(for now?).

Although it didn't strictly needed, rebased after #9494 I can show more easily my preferred option of only allowing to load the custom chainparam arguments from file (not command line options), and only from a separated file from the one with the rest of the config. So I added a commit to do that as this PR did before (which can be squashed if it is liked or removed if it is not).

Needed rebase

I don't understand the use of -chainconf argument. It looks like the actual functionality is just an additional config file that gets loaded after the main bitcoin.conf file. I originally thought that it'd be doing something like #9374, and allowing the user to specify a config file that would only get used if a specific chain was chosen, which would be very useful.

In any case, I think the loading of the additional config file can be done in the new ReadConfigFiles() function in #10267. There's nothing that requires it to be in chainparams.cpp.

I have to admit I don't really understand the purpose of it either. With #10267 you could also just do -datadir and/or -conf from command line and the chain specific config would includeconf the main config file.

I don't think you are understanding what the last commit does.

Example after last commit: bitcoind -chain=custom -chainconf=mytestnet.conf mytestnet.conf:

chainpetname=mytestnet1
con_fpowallowmindifficultyblocks=0
con_npowtargetspacing=700
...

Example before the last commit (edited, it had a mistake):

bitcoind -chain=custom -con_fpowallowmindifficultyblocks=0 -chainpetanme=mytestnet1 -conf=mytestnet2.conf

In mytestnet2.conf:

dbcache=6000
con_fpowallowmindifficultyblocks=1
chainpetanme=mytestnet2
includeconf=mytestnet3.conf

In mytestnet3.conf:

dbcache=8000
con_fpowallowmindifficultyblocks=0
chainpetanme=mytestnet2

What is the resulting chainpetanme in the second example? This is not a small detail since the hash of the genesis block depends on it (it could depend on all consensus parameters as well). Is con_fpowallowmindifficultyblocks 1 or 0? You better know because this is consensus critical.

I think only allowing the custom chainparams to be loaded from one place and decoupled from the rest of the configuration options (much more versatile) simplifies things and will less likely lead to unexpected behavior. But if people prefer the flexibility that the second example gives users to shot themselves in the foot much more easily, I can drop the last commit.

@jtimon -includeconf is not allowed on command line, only the config file. So in your case (I believe you meant includeconf=mytestnet3.conf there btw, but you wrote mytestnet2.conf in both), mytestnet2.conf would includeconf mytestnet3.conf, which actually isn't allowed and probably makes no sense.

I'm still not really sure why you can't put

includeconf=globalstuff.conf

in the chain conf file and just do -conf= that file.

Sorry, if I wasn't clear enough and that the example was wrong about -includeconf (corrected). To be clear, without the last commit (and with you pr) you can do what you describe, with the last commit, you cannot, or put chainparams in the regular config file nor on the command line. Disallowing those options in favor of only allowing completely separated file for custom chainparams is the whole point of the last commit.

Since it conflicted with it, preemptively rebase on top of #10339

Needed rebase, also removed the commit from #10339 .

Needed trivial rebase after #9176

Needed rebase.

Concept ACK. I like this idea. A few high-level thoughts:

• it'd be nice if we could just move the functional tests across to exclusively use the custom chain. Then you wouldn't need to make the changes in test_framework to store and pass around the chain name. What are the problems with the segwit.py test that prevent you from doing that?
• What do you think about simplifying the config model:
  • change -chain= to take any string, where main, test and regtest are reserved and have special meaning. Any other value becomes the name of the custom chain (eg custom or mychain or supereasydifficulty or whatever).
  • remove chainconf. The consensus config should always appear in <chainname>_chain.conf, so if I want to use a custom chain called "lowdifficulty", I set -chain=lowdifficulty and add a lowdifficulty_chain.conf
  • drop the chainpetname parameter and add a -consensus_genesiscoinbasetext to the consensus parameters
• this model could be extended to allow a main_chain.conf, which would allow only consensus_assumevalid to be updated. That would remove the global hashAssumeValid and put it back in CChainParams (can be done in a follow-up PR)

Can you clean up the original PR notes so they describe exactly what's in this PR (they seem to have drifted a bit since you opened this)?

A few small review comments inline

Rebased. Hopefully fixed some nits and suggestions by @jnewbery . Also removed a warning in the qt change. Updated OP.

Regarding allowing -chainconf or main_chain.conf for main, testnet3 or regtest, yeah, I think that would belong in another PR, but interested on hearing more thoughts on that.

Details of the segwit failure (I'll leave it failing on travis):

segwit.py failed, Duration: 16 s

stdout:
2017-08-14 23:19:17.140000 TestFramework (INFO): Initializing test directory /tmp/bitcoin_test_runner_20170815_011849/segwit_82
2017-08-14 23:19:19.529000 TestFramework (INFO): Verify sigops are counted in GBT with pre-BIP141 rules before the fork
2017-08-14 23:19:21.425000 TestFramework (INFO): Verify default node can't accept any witness format txs before fork
2017-08-14 23:19:21.452000 TestFramework (INFO): Verify witness txs are skipped for mining before the fork
2017-08-14 23:19:21.711000 TestFramework (INFO): Verify unsigned bare witness txs in versionbits-setting blocks are valid before the fork
2017-08-14 23:19:21.808000 TestFramework (INFO): Verify unsigned p2sh witness txs without a redeem script are invalid
2017-08-14 23:19:21.812000 TestFramework (INFO): Verify unsigned p2sh witness txs with a redeem script in versionbits-settings blocks are valid before the fork
2017-08-14 23:19:21.897000 TestFramework (INFO): Verify previous witness txs skipped for mining can now be mined
2017-08-14 23:19:21.936000 TestFramework (INFO): Verify block and transaction serialization rpcs return differing serializations depending on rpc serialization flag
2017-08-14 23:19:22.029000 TestFramework (INFO): Verify witness txs without witness data are invalid after the fork
2017-08-14 23:19:22.119000 TestFramework (INFO): Verify default node can now use witness txs
2017-08-14 23:19:22.353000 TestFramework (INFO): Verify sigops are counted in GBT with BIP141 rules after the fork
2017-08-14 23:19:22.477000 TestFramework (INFO): Non-segwit miners are able to use GBT response after activation.
2017-08-14 23:19:22.593000 TestFramework (INFO): Verify behaviour of importaddress, addwitnessaddress and listunspent
2017-08-14 23:19:26.018000 TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/home/jt/datafast/code/bitcoin/test/functional/test_framework/test_framework.py", line 152, in main
    self.run_test()
  File "/home/jt/code/bitcoin/test/functional/segwit.py", line 458, in run_test
    spendable_txid.append(self.mine_and_test_listunspent(spendable_anytime + spendable_after_importaddress, 2))
  File "/home/jt/code/bitcoin/test/functional/segwit.py", line 582, in mine_and_test_listunspent
    txid = self.nodes[0].sendrawtransaction(signresults, True)
  File "/home/jt/datafast/code/bitcoin/test/functional/test_framework/coverage.py", line 46, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/jt/datafast/code/bitcoin/test/functional/test_framework/authproxy.py", line 154, in __call__
    raise JSONRPCException(response['error'])
test_framework.authproxy.JSONRPCException: Missing inputs (-25)
2017-08-14 23:19:26.020000 TestFramework (INFO): Stopping nodes
2017-08-14 23:19:32.786000 TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_test_runner_20170815_011849/segwit_82
2017-08-14 23:19:32.786000 TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_test_runner_20170815_011849/segwit_82/test_framework.log

Some squashing is due once there's more feedback on the latest suggestions.

Rebased. Squashed except for the 3 file-related commits at the end, which could be one. Perhaps we can leave loading from a different file for later? EDIT: Squashed. Since there's some discussion related to the config files and potential conflict with other PRs, I left that part for later in https://github.com/jtimon/bitcoin/compare/0.13-new-testchain...jtimon:b16-new-testnet-file

The tests seem to be broken in rebase one more time. @jnewbery I would really prefer to leave segwit.py for later unless somebody finds the fix. Rebasing this PR is costly, mostly due to the tests. Any further extension or improvement will be much easier to rebase and review afterwards.

Fixed the tests, except for the segwit one which still needs to remain in regtest or have some tests commented (not an option, but see last commit, in case anybody has an idea why may be failing).

Alright, it seems I solved segwit.py too, it seems I needed https://github.com/bitcoin/bitcoin/pull/8994/commits/9b4feab8d48972b5f3241d1926095da3401f536b

Needed rebase

Needed rebase.

Needed rebase, also some new adaptations in the python tests and taking care of bech32_hrp.

I'd say I'm -0 on this by the scale at #11426 (comment) - I'm fine with this if others want to review and maintain this, but unless we need it for testing, I dont think its worth the effort at all. Currently all our tests that need to change chainparams flags are pretty contained to only change one chainparams value (and we only ever care about one or three of them anyway), so I really dont think its worth the effort to do so, and its certainly not worth the effort to make random other altcoins/sidechains/whatever simpler.

The code looks well written and I really like that experiment. Though, I fail to see how this helps with testing Bitcoin Core. We already have three networks that should cover all our testing needs. I am -0 on this change, given that it is a nice generalizing refactor, but might not be worth the effort of review and maintaining.

Several new testchains were created when testing segwit. Lightning developers also wanted their own testnet, preferably with signed blocks (related to that, I would like to continue my work on #9177 if this gets merged). Perhaps similar testing needs/wants will arise again in the future. Regarding testing needs, I would say they're always unbounded.

This is also a way to test #9102 which has never been merged due to lack of tests. Another way to test it could be to change the genesis block of regtest without changing anything else. In that case, the "custom chain" could simply be regtest itself.

This is also a way to test #9102 which has never been merged due to lack of tests.

Well. That reasoning is circular. #9102 was never merged because it is only required for this pull.

Though, easier creation of future testchains (similar to segnet) is a valid rationale for this pull request. I change my preference to +0.

I actually begin to like this pull. Going to review...

utACK b43a3cd851f5652ee9ed6f101d8e3c1a6dc54a78. Haven't looked at the test code yet.

Came accross this PR today after waiting forever for my SegWit transactions to confirm on testnet, which makes #11403 a pain to test. Concept ACK. I'll try after the next rebase.

Rebased. Hopefully solved or answered many nits by @MarcoFalke but not all. Some nits remain as debt for now but new nits are welcomed.

The RPC port is 18553, right? When I run cgminer I get: No Stratum, GBT or Solo support in pool 0. It works for me on testnet (although its difficulty is out of reach for me).

Maybe it's because initialblockdownload: true? How would I go about bootstrapping my custom chain and convincing my node that it is indeed alone in the blockchain universe?

The RPC port is 18553, right? When I run cgminer I get: No Stratum, GBT or Solo support in pool 0. It works for me on testnet (although its difficulty is out of reach for me).

Yeah, the default port, you can set it to something else. I don't know much about cgminer but this shouldn't affect it. Does it work for you with -regtest (or -chain=regtest)?

Maybe it's because initialblockdownload: true?

I don't see the relation, sorry.

How would I go about bootstrapping my custom chain and convincing my node that it is indeed alone in the blockchain universe?

It should work just by choosing a chain name that nobody does, for example: -chain=aloneintheworld. Since the name (except for main, test and regtest) is used to build the genesis block, the chain will be invalid for everyone else unless they chose the same name.

@jtimon I haven't tried mining on regtest, only on testnet. I'm not familiar with how the difference between regtest and testnet would impact the ability to use mining software like cgminer.

Regardless of mining, shouldn't initialblockdownload in getblockchaininfo be false? It remained true when I used -chain=sjors.

Well, this changes nothing about testnet or getblockchaininfo, just allows you to use -chain=test instead of -testnet additionally. I really don't know what you're trying to do with the mining software, but if it's currently not working for -regtest it can't work with this patch and -chain=sjors, becasue -chain=sjors is basically just regtest with a different genesis block. There's not much to test regarding testnet and this PR.

Rebased, mainly to make sure new tests were failing. Updated the OP accordingly. Ok, also updated because one more commit was separated into its own PR (ie #12128 ). Currently 2 tests are failing documented in the TODO section of the OP.

EDIT: Also add one last commit for CCustomParams to extend from CRegTestParams as suggested by @MarcoFalke (although it is incpmplete for fields that default as hash for regtest but string for custom chains).

Squashed the commit in which CCustomParams extends CRegTestParams and rebased.

Added commit named "regtest and custom could be the same", with it, commits "QA: Adapt BitcoinTestFramework for chains other than "regtest"" and "QA: Use custom chain instead of regtest for rpc tests" aren't really necesary to test CCustomParams, since with it only CCustomParams be used, even for -regtest, never CRegTestParams on its own. IMO it still wouldn't hurt to have those 2 last commits changing the tests. Thoughts?

Needed rebase

To clarify my earlier question, if we unify regtest CRegTestParams and CCustomParams (ie we use the equivalent of -chain=regtest for -regtest) there would be no need to touch the python tests to test the CCustomParams class, since it would already be used for all python tests (with the custom name "regtest"). But this is a HF to regtest because it would change its genesis block, and people would need to remove their ./bitcoin/regtest directory. Would that be acceptable? Which of the two options is preferred, hardfork for segwit or changing the python tests? The former is both less code changes and less total code at the end.

EDIT: another possibility is to do both and rename the new regtest to regtest2

Assuming regtest is really only used by developers, it's not too much to ask them to delete it once, if it results in cleaner code. Using a new directory could lead to confusion "where did my regtest history go?"

This needs a release note.

Concept re-ACK. This might also make it easier to test interaction with consensus incompatible peers.

Do we really need the con_ prefix for these new arguments? It makes them hard to read.

I'm concerned what happens if any of these parameters are changed between launches, or you just forget to set them during the next launch. If parameters can't be changed between launches, shouldn't bitcoind throw an error upon launch? Or is that too restrictive? For example, if I change -bech32_hrp between launches, it will consider its own previously generated addresses invalid.

I like the -chainconf approach better (instead of command line args), where that file contains the chain name and other settings. That should make mistakes less likely. To make future changes easier, the file should contain regtest=1. @jnewbery's suggestion works for me too:

remove chainconf. The consensus config should always appear in <chainname>_chain.conf

Would it make sense to leave out params like defaultAssumeValid that are 0x00 in regtest? If not, is this behavior sufficiently tested? Fewer con_ parameters would also make this PR easier to test.

Do we really need the con_ prefix for these new arguments? It makes them hard to read.

Well, it's to distinguish them from the non-consensus chain params.

If parameters can't be changed between launches, shouldn't bitcoind throw an error upon launch?

I don't think so, no. Unless the change itself produces an error.

For example, if I change -bech32_hrp between launches, it will consider its own previously generated addresses invalid.

And won't that throw an error like you wanted? Another possibility is to make the genesis block dependent on some or all fields like we do with strNetworkID so that changing them simply results on a different genesis block. This is what we're doing in elements for a few fields, see https://github.com/ElementsProject/elements/blob/elements-0.14.1/src/chainparams.cpp#L20

I like the -chainconf approach better (instead of command line args), where that file contains the chain name and other settings.

I also prefer it. I can recover stuff from https://github.com/jtimon/bitcoin/tree/b16-new-testnet-file but I left that out since there was too much discussion about how it fits with #10267 . Or that can be done after merging this.

To make future changes easier, the file should contain regtest=1

Assuming we unify CRegTestParams and CCustomParams, regtest=1 is just equivalent to chain=regtest (chain=regtest2 or anything will work too, but with a different genesis block). If we separate the chain params config to a different conf file, chain/regtest/testnet options are still set up on the regular conf file (or command line).

Would it make sense to leave out params like defaultAssumeValid that are 0x00 in regtest?

Why? It's almost no extra effort to do it with all fields and some users may use them.

If not, is this behavior sufficiently tested?

Well, we change consensus.nMinimumChainWork and consensus.defaultAssumeValid for mainnet and testnet with every release, so, yes, I think it is sufficiently tested.

Fewer con_ parameters would also make this PR easier to test.

None of them are actually being tested automatically, but the point is to allow people to change them for their automatic tests! I don't see any value in excluding any field that is easy to implement. Of course if we remove fields from CChainParams later, great to remove them from here too!

discussion about how it fits with #10267 . Or that can be done after merging this.

If consensus paramaters are only stored in a .conf and can't be changed through parameters, this shouldn't get in the way of #10267.

For example, if I change -bech32_hrp between launches, it will consider its own previously generated addresses invalid.

And won't that throw an error like you wanted?

The UI throws an error if you try to enter such an address, but previously received transactions don't become invalid. It's hard to think through every parameter and what things to test when changing it. However, if the purpose is only to facilitate writing tests, this may not be an issue.

Another possibility is to make the genesis block dependent on some or all fields like we do with strNetworkID so that changing them simply results on a different genesis block.

It may actually be useful in tests to be able to change consensus rules on the fly without a new genesis block. Using a configuration file rather than parameters should be sufficient protection against accidental changes.

If consensus paramaters are only stored in a .conf and can't be changed through parameters, this shouldn't get in the way of #10267.

Well, in previous discussion it seemed that using a separate file was getting the PR stuck instead of merged, so I decided to leave that for a following PR, but I have the code written, it should just be one rebase if we prefer that (but I'm really not sure that's the case, although thanks for voicing your opinion, that is helpful).

The UI throws an error if you try to enter such an address, but previously received transactions don't become invalid.

why should previously received txs become invalid? I don't even understand why someone would want to use 2 different bech32_hrp on the same chain at different times...

It may actually be useful in tests to be able to change consensus rules on the fly without a new genesis block.

I assume by "on the fly", you mean stopping the node and restarting it with different rules but the same genesis block and datadir. Yeah, I guess that could perhaps be useful and it's not compatible with committing those consensus chain params to the genesis block. Perhaps it makes sense to commit certain chainparams but not others.

I don't even understand why someone would want to use 2 different bech32_hrp on the same chain at different times

Me neither. I assume this would only happen by accident and confuse a developer until they realize what's going on.

by "on the fly", you mean stopping the node and restarting it with different rules but the same genesis block and datadir

Correct. Perhaps in order to simulate two nodes in a (accidental) hard fork scenario.

@MarcoFalke I honestly don't know what's the status of this at this point. I feel I asked too many question and I got too many different answers.

There's a few components here:

1. regtest could make all its attributes configurable within its constructor. we already do this outside the constructor for bip9 and plan to do the same for segwitheight (there's older examples for trying to edit "Params()", but we want Params() to be immutable after init, at least for production. You can have your cake and it eat to as long as you make it in the constructor)
2. chainparams.cpp doesn't need any more classes even if we decide to add 10 new testnets, and -testnet, -regtest doesn't scale, there's should be just one -chain=<chainame> option and unless chainame is testnet3 or main, it should be just a configurable regtest which inclides the selected name in the hash of the genesis block (so if dumb miners chose to attack a testnet chain, let's say, lightningtestnet2, you just tell your friend testing on the same chain to change to -chain=lightningtestnet3 and replay all relevant txs).
3. The python framework heavily assumes Params().strNetworkID == "regtest", that shouldn't be the case if chain=regtest is actually chain=<anything_except_main_and_testet3> . Combined with step 2, this could also serve to write tests to make sure that peers with incompatible consensus rules fork from each other as expected.

I have no concerns in rebasing this forever (or until it's merged, obviously), but there's no harm in proposing and testing things one by one separatedly.

Although I previously wanted to leave that for later, I suggest bip9 as an excuse to use gArgs/g_args from regtest's constructor (that's going to net remove a bunch of lines from init.cpp, so people may like it at a glance). Once people can do anything with regtest chainparams they may stop proposing to edit them on runtime, something I've been opposed to for years, but always comes back (see segwitheight).

But I think this PR in particular should be closed because maintaining the part about the simplest way of changing the genesis block into something that depends on <petname> and at the same time doesn't produce enough pow it's too expensive, at least expensive to maintain until there's even a more generic -chain=<petname> option other than -regtest.

Finally, the qt part is just about having a different logo color for any other name (and put the name of the random chain in the title of the qt gui). I do care about he part of the custom petname being printed in the gui, I do not care about adding a new color, I'm fine reusing regtest color as long it's not bitcoin's or testnet3's. NACKs to specific commits or changes very welcomed too, or other logo default colors.

tl;dr

I mainly use this PR to monitor changes in chainparams or related or tests that may break changing the genesis block, petname (assuming there's a -chain=<petname> option) I will likely create a similar PR in the future, but the interesting part is, I think, feedback on the small parts that could be merged independently of my eternal "altchainer vision".

Which parts should be opened independently after closing this PR?

@jtimon Sorry that I haven't looked at this after the rebase. I just wanted to get in #12300 first, which touches the same files.

@MarcoFalke It seems there's some controversy around #12300 though. Rebasing either one on top of the other should be relatively easy I think.

@jtimon Sounds fine. (Needs rebase)

Have spoken to @jtimon. He said he's planning on rebasing this within ~1 week from today, so this can remain open until at least then.

Actually, it's more likely to be ~2 weeks, but yeah, I intend to rebase this.

Rebased earlier than anticipated, but...

Even without running with:

python3 ./test/functional/test_runner.py -j4 --extended

... the following test fail:

feature_block.py                      | ✖ Failed  | 1 s
feature_cltv.py                       | ✖ Failed  | 11 s
feature_csv_activation.py             | ✖ Failed  | 1 s
feature_dersig.py                     | ✖ Failed  | 9 s
interface_bitcoin_cli.py              | ✖ Failed  | 1 s
mining_basic.py                       | ✖ Failed  | 3 s
p2p_invalid_block.py                  | ✖ Failed  | 2 s
p2p_invalid_tx.py                     | ✖ Failed  | 1 s
p2p_segwit.py                         | ✖ Failed  | 3 s
rpc_getblockstats.py                  | ✖ Failed  | 1 s
rpc_scantxoutset.py                   | ✖ Failed  | 3 s
wallet_disableprivatekeys.py --usecli | ✖ Failed  | 1 s
wallet_multiwallet.py --usecli        | ✖ Failed  | 1 s

Any help welcomed

feature_block.py fails for me because it's looking for a log file in regtest instead of regtest2: <img width="1055" alt="schermafbeelding 2018-10-13 om 16 33 54" src="https://user-images.githubusercontent.com/10217/46902769-f4952000-cf05-11e8-9464-e3130dd1960a.png"> <img width="1056" alt="schermafbeelding 2018-10-13 om 16 35 35" src="https://user-images.githubusercontent.com/10217/46902772-0bd40d80-cf06-11e8-9b38-39b0a1544126.png">

Probably test_node.py needs a tweak: <img width="680" alt="schermafbeelding 2018-10-13 om 16 38 02" src="https://user-images.githubusercontent.com/10217/46902790-64a3a600-cf06-11e8-9ae1-9b1623193bd9.png">

I'm able to use chain= in bitcoin.conf, but launching with either -chain= or --chain= results in Error parsing command line arguments: Invalid parameter -chain. Did some light testing, e.g. I was able to generate sjimmie1qkte... :-)

Fixed nits, thanks! And made a few more other fixes. With that and running with the -extended tests, they pass.

-chain now works, but other consensus parameters still throw an Invalid parameter error, unless I put them in the config file.

@Sjors It seems I had to add them to hidden_args in init.cpp. Hopefully fixed.

Added: -seednode, '-fallback_fee_enabled', '-pubkeyprefix', '-scriptprefix', '-secretprefix', '-extpubkeyprefix', '-extprvkeyprefix', '-pchmessagestart'

Removed: -frequirestandard (since it is redundant with acceptnonstdtxn).

Fixed @practicalswift 's nits.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #17037 (Testschains: Many regtests with different genesis and default datadir by jtimon)
• #17032 (Tests: Chainparams: Make regtest almost fully customizable by jtimon)
• #16770 (Chainparams: Rename IsTestChain() to AllowAcceptNonstd() by jtimon)
• #16527 (B: Get rid of Params().RequireStandard() by jtimon)
• #16526 (A: Chainparams: Rename RequireStandard() to DefaultAcceptNonstd() by jtimon)
• #16411 (Signet support by kallewoof)
• #16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)
• #14866 (Improve property evaluation way in bitcoin.conf by AkioNak)
• #13972 (Remove 16 bits from versionbits signalling system (BIP320) by btcdrak)
• #13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by l2a5b1)

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.

Some changes are required to make it easier to write tests that use multiple different chains.

Concept ACK on making it easier to test the interaction between multiple chains. Optionally combined with #12134 it offers a lot flexibility when testing future soft-forks. The change to the test framework looks simple enough, but could use @jnewbery's eyes.

Rebased. A new commit undoing recent changes was also needed. There are still some pending nits/discussions.

Rebased again

Added customization for -assumed_blockchain_size and -assumed_chain_state_size introduced in https://github.com/bitcoin/bitcoin/pull/13216/ . I forgot that on rebase.

Also, added basic documentation for all fields as discussed before. Although nits for the documention are of course welcomed, we could perhaps fine tune that and, more importantly, which fields don't even deserve to be exposed, on a different PR, not to add further noise.

In that spirit, I brought back -ndefaultport and -frequirestandard, even though I personally don't think they deserve to be exposed. More generally, I would suggest adding to that list any field containing the word "consider" for its documentation in my initial proposed documentation, sorry for the various redundancies.

They can be discussed individually in different PRs is my main point in bringing them back, I guess. Perhaps we can keep them all as a way to document the existing alternatives people could be using and to remind us that perhaps not all of them even belong in chainparams at all.

Honestly, to me this is mostly about being able to produce many regtest networks with the same binary that are incompatible between them because they don't share the same genesis block hash, which happens to be a consensus rule intrinsically by definition (for any block with height 1 needs to be a child of the genesis block). Perhaps I should add a simple test about that.

To reiterate other motivations, of course it's also for never adding more parameters for each new type of testing chain we want. That scales linearly by the number of chains. @kallewoof is trying to add some -signet option to make it 3, perhaps 4 in the future, who knows. I say 3 options it's fine, -chain=signet or even -chain=kalle if he wants should be enough. If people ever go crazy again and start trading testnet3 for real money, testnet4 could be simply a config file, or even a section starting with:

[testnet4]

Additionally, we will never need a set method for chainparams ever again (I've seen those come and try to come, and go, and I fought for them to go for a while), so our chainparams singleton could be const forever after initialization, just as everybody reasons about it, and as it should IMO.

concept ACK

It makes testing easier, enough said.

Concept ACK and light code re-utACK.

I am not a big fan of adding a ton of new -options but I don't see a way around it without some custom grokkery. The # of lines added (+351-267=+84) seems warranted for the functionality this provides, and I think this would go hand in hand with signet.

Well, we can leave some of the options undocumented and we can also not expose some of the chain params at all since there are alternative ways to test that functionality (like the port number or allownonstd). Not sure if that's what you mean by "some custom grokkery though".

I meant a new file format for custom chains that is separate from the main argument stuff. People would either provide a JSON object describing all the options or a file that had the options in it.

That said though, isn't it already possible to do e.g. -chain="{\"name\":\"foochain\", \"optionxyz\": 36, ...}? That would put your command additions at one, but the downside is that error checking would be less obvious (e.g. you would probably have to manually check that no unknown keys are in there).

Concept ACK. There's two things I'm "Approach NACK" on -- all the extra options, and switching to regtest2.

I think the options can be broken out into:

• chain params: halving interval, (initial reward?), pow (limit, interval, spacing, retargeting, min chain work)
• soft-fork params: vbparams, bip16exception, etc
• p2p params: default port, seednode, pchmessagestart
• UI elements: bech32 hrp, pubkey,script,secret,extpub,extprv prefixes, assumed chain sizes

Would it make sense to have that stuff be encoded as a hex blob -- that way it can be cut and paste as a unit, and maybe a checksum can be added? You could make that work by running a script to generate it (along with a genesis block, with sufficient proof of work to not need to add exceptions to the tests), and having the script spit out a hex blob. Then you just paste the hex blob into your bitcoind.conf and share elsewhere? So you end up with a few chain specs in your config:

customchain=regtest42:041a0532342fea1...
customchain=kallesig:a41a42340591eaf42...

and can then choose one by saying --chain=kallesig.

I think setting things up that way would make it pretty easy to allow you to have your own newspaper quote, which should in turn make it easy to have regtest use this code path without needing a hardfork.

Not sure about the hex part. That makes it hard to change a param. What about requiring that the specs are provided in a json file (-customchainfile=foo.json) and can not be passed in on the command line?

What is the advantage of the blob? How is copying and pasting from conf files harder than running a script to then copy and paste just the same? If you require proof of work, then the same config for a chain won't necessarily result in the same genesis hash. You can already "chose your newspaper quote", whatever you put in -chain=hashthis is going to be hashed in the genesis block instead of satoshi's quote.

Regarding moving the tests from regtest to regtest2 (or custom, any name except main, test and regtest would do) is the way to test this. I don't see the disadvantage on moving away from regtest there, they're practically the same and some tests could make use of the custom chainparams. People can still use the old regtest if they want.

What about requiring that the specs are provided in a json file (-customchainfile=foo.json) and can not be passed in on the command line?

I proposed this in the past, I think I had it implemented at some point. Right, see #8994 (comment)

Now that there are sections for the config file, I don't care so much about it, you can do things like: https://github.com/jtimon/multi-ln-demo/blob/master/conf/bob_bitcoind.conf so I don't see what the advantage of using a sepaarated file would be anymore. But if people prefer that, I can implement that again, or perhaps in a different PR. Let's see what other people think, I don't even remember the previous discussions about it very clearly myself.

Can we make the consensus arguments available exclusively via the config file? In that case you wouldn't need the -con_ prefix either. And it will make it easier in the future to change how we configure these new chains, e.g. via the hex string @ajtowns suggested above; that would then be a simple matter of the regular parser ignoring sections other than [main], [testnet] and [regtest].

The con_ prefix is just meant to be informative, I don't see what would be the point in trating the consensus args differently from the others. As said I don't see how @ajtowns's sugfestion simplifies anything, I think it actually makes things more complicated.

The parser already separates the sections by chain, now you would just put ut under [mychain].

Regarding allowing the args to be set only in the config file. I'm fine doing that, even in a separated file. But since it's more work and not everyone seems convinced, perhaps it would be better to leave it for a follow up pr?

To ubderstand better, what's the concern in allowing the args to be set in both the config file or command line like any other parameter? Remember these parameters can only be set for custom chains which are supposed to be like regtest, testsnets or similar, you cannot use them for beither main, testnet3 nor regular regtest.

On Fri, Jul 5, 2019, 16:53 Sjors Provoost notifications@github.com wrote:

Can we make the consensus arguments available exclusively via the config file? In that case you wouldn't need the -con_ prefix either. And it will make it easier in the future to change how we configure these new chains, e.g. via the hex string @ajtowns https://github.com/ajtowns suggested above; that would then be a simple matter of the regular parser ignoring sections other than [main], [testnet] and [regtest].

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/8994?email_source=notifications&email_token=AAHWGSSYQFUL7CSLF7TNKUDP55N6NA5CNFSM4CTW5XKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZJWUCQ#issuecomment-508783114, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHWGSXWYHM6YPXM7XTW5J3P55N6NANCNFSM4CTW5XKA .

Rebased and then squashed the documentation commit, since nobody seemed to dislike it. Also removed the arguments from the hidden_args variable in init.cpp.

Rebased and adapted the tests a little bit more

Fixed @MarcoFalke 's nit. Also fixed new test (kind of secretly introduced by in the merge commit of #16366 ) that this PR was breaking.

Also, I don't think this needs concept ACKs. I mean, I guess nacks are still "welcomed", even though potential nackers have had plenty of time. I think it just needs ACKs, and I'm afraid the needs concept acks label may be dissuading people from reviewing it.

I can see this being very useful in combination with Signet (#16411). For example someone might use the default signet as a custom signet for projects they're working on. Right now that requires separate data dirs. @kallewoof any thoughts on sequencing?

The signet PR also takes the approach of passing consensus params via the config file and bitcoind, so let's just stick to that. The extra documentation for bitcoind only shows up with --help-debug so that's no a big deal.

Signet switched to grinding proof of work, so I think you can drop commit 9102: Really don't validate genesis block, just to avoid having to debate it.

I still prefer "hardforking" regtest over adding this new "regtest2".

The only commits that are useful for signet are 66e8da33b45a7cf5affc2afaff1c7ed594c1d755 and the gui change, so maybe create a separate pull request with those?

@MarcoFalke custom consensus parameters are also useful, but I agree it could be a good idea to split those commits into their own PR.

I'm happy to separate commits, but I disagree those are rhe only changes useful for signet. Signet could be implemented just by adding an optional parameter to the custom chain chainparams.

On Mon, Aug 19, 2019, 16:28 Sjors Provoost notifications@github.com wrote:

@MarcoFalke https://github.com/MarcoFalke custom consensus parameters are also useful, but I agree it could be a good idea to split those commits into their own PR.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/8994?email_source=notifications&email_token=AAHWGSUX5XKAYXDJL2IHYE3QFKUXNA5CNFSM4CTW5XKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TDQ2Y#issuecomment-522598507, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHWGSQVVSEIPQ77JGJ7JZLQFKUXNANCNFSM4CTW5XKA .

Rebased, also separated #16680 as suggested.

Also separated #16681

Rebased on top of the latest:

• Tests: Use self.chain instead of 'regtest' in all current tests #16681
• Preparations for more testchains #16680

...and added them to the dependencies in the OP.

Rebased after #16680 was merged

Small change: instead of simply replacing the pszTimestamp string in the genesis block with the chain name, hash it like in signet #16411

Needed rebase after #16551 was merged.

Rebased. Also I left rpc_getblockstats in "regtest" instead of moving it to "regtest2" like all other tests since that's trivial to do and review later and adapting it now made the total diff much more scary (+312-230 adapting it vs +206-123 now).

Rebased and changed the documentation string for -is_test_chain after #16524 was merged.

Rebased on top of #17032 , again trying to separate things.

Rebased on top of separated https://github.com/bitcoin/bitcoin/pull/17037

Rebased on top of rebased and modified #16681 , one less commit in total, but the total diff it's the same.

Since #16681 was closed, closing this too. There's also more discussions there with people saying they don't like the idea of moving the test framework default chain from "regtest" to "regtest2".