Testchains: Introduce custom chain whose constructor...

jtimon commented at 2:25 am on October 22, 2016: contributor

…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 ).

btcdrak commented at 7:08 am on October 22, 2016: contributor

Concept ACK

btcdrak commented at 7:27 am on October 22, 2016: contributor

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.

MarcoFalke added the label Tests on Oct 23, 2016

MarcoFalke added the label Consensus on Oct 23, 2016

jtimon force-pushed on Oct 25, 2016

jtimon force-pushed on Oct 29, 2016

jtimon force-pushed on Nov 3, 2016

jtimon commented at 10:39 pm on November 3, 2016: contributor

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

jtimon force-pushed on Nov 3, 2016

jtimon force-pushed on Nov 8, 2016

jtimon commented at 3:50 am on November 8, 2016: contributor

Rebased on top of #9102

jtimon force-pushed on Nov 10, 2016

jtimon force-pushed on Nov 14, 2016

jtimon commented at 8:02 pm on November 14, 2016: contributor

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).

jtimon force-pushed on Nov 15, 2016

jtimon force-pushed on Nov 18, 2016

jtimon force-pushed on Dec 2, 2016

jtimon commented at 9:02 am on December 2, 2016: contributor

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

jtimon commented at 10:24 pm on December 2, 2016: contributor

@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.

jtimon force-pushed on Dec 3, 2016

jtimon commented at 1:13 pm on December 3, 2016: contributor

Rebased

jtimon force-pushed on Dec 20, 2016

jtimon commented at 7:08 am on December 20, 2016: contributor

Rebased

jtimon commented at 10:24 am on December 20, 2016: contributor

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?

jtimon commented at 4:21 pm on January 3, 2017: contributor

Needs rebase after #9243

jtimon force-pushed on Jan 9, 2017

jtimon commented at 10:36 pm on January 9, 2017: contributor

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.

jtimon force-pushed on Jan 11, 2017

jtimon commented at 3:56 pm on January 11, 2017: contributor

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

jtimon force-pushed on Jan 24, 2017

rustyrussell commented at 11:25 pm on February 21, 2017: contributor

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 )

jtimon force-pushed on Mar 24, 2017

jtimon commented at 4:45 am on March 24, 2017: contributor

Rebased, but the python tests were broken again.

jtimon force-pushed on Mar 25, 2017

jtimon commented at 9:04 pm on March 25, 2017: contributor

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.

jtimon force-pushed on May 3, 2017

jtimon commented at 6:56 pm on May 3, 2017: contributor

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

jtimon force-pushed on May 4, 2017

jtimon force-pushed on May 9, 2017

jtimon commented at 12:47 pm on May 9, 2017: contributor

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.

in src/chainparamsbase.cpp:118 in e7aad1a77b outdated

 98@@ -98,5 +99,5 @@ std::string ChainNameFromCommandLine()
 99         return CBaseChainParams::REGTEST;
100     if (fTestNet)
101         return CBaseChainParams::TESTNET;
102-    return CBaseChainParams::MAIN;
103+    return GetArg("-chain", CBaseChainParams::MAIN);

instagibbs commented at 2:34 pm on May 9, 2017:

do we want to disallow both chain and testnet/regtest being given?

in contrib/devtools/check-doc.py:25 in 0510e271c0 outdated

20@@ -21,7 +21,8 @@
21 REGEX_ARG = re.compile(r'(?:map(?:Multi)?Args(?:\.count\(|\[)|Get(?:Bool)?Arg\()\"(\-[^\"]+?)\"')
22 REGEX_DOC = re.compile(r'HelpMessageOpt\(\"(\-[^\"=]+?)(?:=|\")')
23 # list unsupported, deprecated and duplicate args as they need no documentation
24-SET_DOC_OPTIONAL = set(['-rpcssl', '-benchmark', '-h', '-help', '-socks', '-tor', '-debugnet', '-whitelistalwaysrelay', '-prematurewitness', '-walletprematurewitness', '-promiscuousmempoolflags', '-blockminsize', '-sendfreetransactions'])
25+SET_DOC_OPTIONAL = set(['-rpcssl', '-benchmark', '-h', '-help', '-socks', '-tor', '-debugnet', '-whitelistalwaysrelay', '-prematurewitness', '-walletprematurewitness', '-promiscuousmempoolflags', '-blockminsize', '-sendfreetransactions',
26+                        '-con_fpowallowmindifficultyblocks', '-con_fpownoretargeting', '-con_nsubsidyhalvinginterval', '-con_bip34height', '-con_bip65height', '-con_bip66height', '-con_npowtargettimespan', '-con_npowtargetspacing', '-con_nrulechangeactivationthreshold', '-con_nminerconfirmationwindow', '-con_powlimit', '-con_bip34hash', '-con_nminimumchainwork', '-con_defaultassumevalid', '-ndefaultport', '-npruneafterheight', '-fdefaultconsistencychecks', '-frequirestandard', '-fmineblocksondemand'])

instagibbs commented at 2:39 pm on May 9, 2017:

nit:extra space

jtimon commented at 5:37 pm on May 9, 2017:

Isn’t this the correct indentation?

in src/chainparams.cpp:384 in 0510e271c0 outdated

379+
380+        pchMessageStart[0] = 0xfa;
381+        pchMessageStart[1] = 0xbf;
382+        pchMessageStart[2] = 0xb5;
383+        pchMessageStart[3] = 0xda;
384+        vFixedSeeds.clear(); //!< Regtest mode doesn't have any fixed seeds.

instagibbs commented at 4:46 pm on May 9, 2017:

s/Regtest/custom

in src/chainparams.cpp:385 in 0510e271c0 outdated

380+        pchMessageStart[0] = 0xfa;
381+        pchMessageStart[1] = 0xbf;
382+        pchMessageStart[2] = 0xb5;
383+        pchMessageStart[3] = 0xda;
384+        vFixedSeeds.clear(); //!< Regtest mode doesn't have any fixed seeds.
385+        vSeeds.clear();      //!< Regtest mode doesn't have any DNS seeds.

instagibbs commented at 4:46 pm on May 9, 2017:

s/Regtest/custom

in src/chainparamsbase.h:12 in 0510e271c0 outdated

 8@@ -9,6 +9,8 @@
 9 #include <string>
10 #include <vector>
11 
12+#define CHAINPARAMS_CONF_FILENAME "chainparams.conf"

instagibbs commented at 4:51 pm on May 9, 2017:

this is unused.

jtimon commented at 5:36 pm on May 9, 2017:

Oops, this are rests from when the PR only allowed to set the custom chainparams parameter from its own specific conf file, but not from the command line options nor the regular config file (to separate concerns). If that’s preferred, it shouldn’t be hard to do again on top of #9494

instagibbs commented at 5:01 pm on May 9, 2017: member

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?

jtimon force-pushed on May 9, 2017

jtimon commented at 7:01 pm on May 9, 2017: contributor

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

0chain=custom
1chainpetname=lightningtest

otherconfig.conf

0chain=custom
1chainpetname=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).

ryanofsky commented at 3:41 pm on May 10, 2017: member

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.

instagibbs commented at 8:47 pm on May 10, 2017: member

@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.

jtimon commented at 6:43 pm on May 18, 2017: contributor

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?

instagibbs commented at 6:51 pm on May 18, 2017: member

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

jtimon force-pushed on May 30, 2017

jtimon commented at 7:49 pm on May 30, 2017: contributor

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).

jtimon force-pushed on Jun 2, 2017

jtimon force-pushed on Jun 5, 2017

jtimon commented at 6:45 pm on June 5, 2017: contributor

Needed rebase

jnewbery commented at 8:48 pm on June 5, 2017: member

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.

kallewoof commented at 0:34 am on June 7, 2017: member

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.

jtimon commented at 12:31 pm on June 7, 2017: contributor

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

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

0chainpetname=mytestnet1
1con_fpowallowmindifficultyblocks=0
2con_npowtargetspacing=700
3...

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

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

In mytestnet2.conf:

0dbcache=6000
1con_fpowallowmindifficultyblocks=1
2chainpetanme=mytestnet2
3includeconf=mytestnet3.conf

In mytestnet3.conf:

0dbcache=8000
1con_fpowallowmindifficultyblocks=0
2chainpetanme=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.

kallewoof commented at 12:55 pm on June 7, 2017: member

@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

0includeconf=globalstuff.conf

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

jtimon commented at 1:19 pm on June 7, 2017: contributor

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.

jtimon force-pushed on Jun 8, 2017

jtimon commented at 0:31 am on June 8, 2017: contributor

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

jtimon force-pushed on Jun 25, 2017

jtimon commented at 1:07 am on June 25, 2017: contributor

Needed rebase, also removed the commit from #10339 .

jtimon force-pushed on Jun 25, 2017

jtimon force-pushed on Jun 26, 2017

jtimon commented at 0:58 am on June 26, 2017: contributor

Needed trivial rebase after #9176

jtimon force-pushed on Jun 26, 2017

jtimon force-pushed on Jul 4, 2017

jtimon commented at 3:24 pm on July 4, 2017: contributor

Needed rebase.

jnewbery commented at 7:20 pm on August 9, 2017: member

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)?

in contrib/devtools/check-doc.py:25 in 5faee932e1 outdated

20@@ -21,7 +21,9 @@
21 REGEX_ARG = re.compile(r'(?:map(?:Multi)?Args(?:\.count\(|\[)|Get(?:Bool)?Arg\()\"(\-[^\"]+?)\"')
22 REGEX_DOC = re.compile(r'HelpMessageOpt\(\"(\-[^\"=]+?)(?:=|\")')
23 # list unsupported, deprecated and duplicate args as they need no documentation
24-SET_DOC_OPTIONAL = set(['-rpcssl', '-benchmark', '-h', '-help', '-socks', '-tor', '-debugnet', '-whitelistalwaysrelay', '-prematurewitness', '-walletprematurewitness', '-promiscuousmempoolflags', '-blockminsize', '-dbcrashratio'])
25+SET_DOC_OPTIONAL = set(['-rpcssl', '-benchmark', '-h', '-help', '-socks', '-tor', '-debugnet', '-whitelistalwaysrelay', '-prematurewitness', '-walletprematurewitness', '-promiscuousmempoolflags', '-blockminsize', '-dbcrashratio',
26+                        '-con_fpowallowmindifficultyblocks', '-con_fpownoretargeting', '-con_nsubsidyhalvinginterval', '-con_bip34height', '-con_bip65height', '-con_bip66height', '-con_npowtargettimespan', '-con_npowtargetspacing', '-con_nrulechangeactivationthreshold', '-con_nminerconfirmationwindow', '-con_powlimit', '-con_bip34hash', '-con_nminimumchainwork', '-con_defaultassumevalid', '-ndefaultport', '-npruneafterheight', '-fdefaultconsistencychecks', '-frequirestandard', '-fmineblocksondemand',

jnewbery commented at 7:21 pm on August 9, 2017:

recommend you keep the consensus parameters separated in their own set and then add them with the existing SET_DOC_OPTIONAL

in src/chainparams.h:107 in 5faee932e1 outdated

103@@ -102,6 +104,8 @@ class CChainParams
104  * @throws a std::runtime_error if the chain is not supported.
105  */
106 std::unique_ptr<CChainParams> CreateChainParams(const std::string& chain);
107+/** Extended version for unittests with custom chainparams */

jnewbery commented at 7:23 pm on August 9, 2017:

Comment seems wrong. This isn’t just for unit tests.

in src/chainparamsbase.cpp:28 in 5faee932e1 outdated

23     if (debugHelp) {
24         strUsage += HelpMessageOpt("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. "
25                                    "This is intended for regression testing tools and app development.");
26+        strUsage += HelpMessageGroup(_("Custom chain selection options (only for -chain=custom):"));
27+        strUsage += HelpMessageOpt("-chainconf=<file>", strprintf(_("Specify configuration file for chain parameters (default: %s). All custom chain arguments except this one must be configured using this file."), CHAINPARAMS_DEFAULT_CONF_FILE));
28+        strUsage += HelpMessageOpt("-chainpetname=<name>", _("Alternative name for custom chain (default: custom). This changes the genesis block."));

jnewbery commented at 7:23 pm on August 9, 2017:

Should be part of consensus params, not general params.

jtimon commented at 11:12 pm on August 14, 2017:

I like more your suggested idea of -chainpetaname and taking it directly from -chain if other than main, test and regtest which are reserved.

jnewbery commented at 7:24 pm on August 9, 2017: member

A few small review comments inline

jtimon force-pushed on Aug 14, 2017

jtimon commented at 1:15 am on August 15, 2017: contributor

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):

 0segwit.py failed, Duration: 16 s
 1
 2stdout:
 32017-08-14 23:19:17.140000 TestFramework (INFO): Initializing test directory /tmp/bitcoin_test_runner_20170815_011849/segwit_82
 42017-08-14 23:19:19.529000 TestFramework (INFO): Verify sigops are counted in GBT with pre-BIP141 rules before the fork
 52017-08-14 23:19:21.425000 TestFramework (INFO): Verify default node can't accept any witness format txs before fork
 62017-08-14 23:19:21.452000 TestFramework (INFO): Verify witness txs are skipped for mining before the fork
 72017-08-14 23:19:21.711000 TestFramework (INFO): Verify unsigned bare witness txs in versionbits-setting blocks are valid before the fork
 82017-08-14 23:19:21.808000 TestFramework (INFO): Verify unsigned p2sh witness txs without a redeem script are invalid
 92017-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
102017-08-14 23:19:21.897000 TestFramework (INFO): Verify previous witness txs skipped for mining can now be mined
112017-08-14 23:19:21.936000 TestFramework (INFO): Verify block and transaction serialization rpcs return differing serializations depending on rpc serialization flag
122017-08-14 23:19:22.029000 TestFramework (INFO): Verify witness txs without witness data are invalid after the fork
132017-08-14 23:19:22.119000 TestFramework (INFO): Verify default node can now use witness txs
142017-08-14 23:19:22.353000 TestFramework (INFO): Verify sigops are counted in GBT with BIP141 rules after the fork
152017-08-14 23:19:22.477000 TestFramework (INFO): Non-segwit miners are able to use GBT response after activation.
162017-08-14 23:19:22.593000 TestFramework (INFO): Verify behaviour of importaddress, addwitnessaddress and listunspent
172017-08-14 23:19:26.018000 TestFramework (ERROR): JSONRPC error
18Traceback (most recent call last):
19  File "/home/jt/datafast/code/bitcoin/test/functional/test_framework/test_framework.py", line 152, in main
20    self.run_test()
21  File "/home/jt/code/bitcoin/test/functional/segwit.py", line 458, in run_test
22    spendable_txid.append(self.mine_and_test_listunspent(spendable_anytime + spendable_after_importaddress, 2))
23  File "/home/jt/code/bitcoin/test/functional/segwit.py", line 582, in mine_and_test_listunspent
24    txid = self.nodes[0].sendrawtransaction(signresults, True)
25  File "/home/jt/datafast/code/bitcoin/test/functional/test_framework/coverage.py", line 46, in __call__
26    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
27  File "/home/jt/datafast/code/bitcoin/test/functional/test_framework/authproxy.py", line 154, in __call__
28    raise JSONRPCException(response['error'])
29test_framework.authproxy.JSONRPCException: Missing inputs (-25)
302017-08-14 23:19:26.020000 TestFramework (INFO): Stopping nodes
312017-08-14 23:19:32.786000 TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_test_runner_20170815_011849/segwit_82
322017-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.

jtimon force-pushed on Aug 31, 2017

jtimon commented at 2:05 am on August 31, 2017: contributor

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.

jtimon force-pushed on Aug 31, 2017

jtimon commented at 11:30 pm on August 31, 2017: contributor

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).

jtimon force-pushed on Sep 1, 2017

jtimon commented at 0:02 am on September 1, 2017: contributor

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

jtimon force-pushed on Sep 7, 2017

jtimon commented at 3:16 am on September 7, 2017: contributor

Needed rebase

jtimon force-pushed on Sep 20, 2017

jtimon commented at 10:03 pm on September 20, 2017: contributor

Needed rebase.

jtimon force-pushed on Oct 27, 2017

jtimon commented at 8:12 pm on October 27, 2017: contributor

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

TheBlueMatt commented at 9:45 pm on November 10, 2017: member

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.

MarcoFalke commented at 6:58 pm on November 11, 2017: member

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.

jtimon commented at 7:16 pm on November 11, 2017: contributor

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.

MarcoFalke commented at 2:59 am on November 12, 2017: member

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.

in src/chainparamsbase.cpp:98 in 427531f1e0 outdated

 91@@ -91,12 +92,13 @@ std::string ChainNameFromCommandLine()
 92 {
 93     bool fRegTest = gArgs.GetBoolArg("-regtest", false);
 94     bool fTestNet = gArgs.GetBoolArg("-testnet", false);
 95+    bool fChainArgSet = gArgs.IsArgSet("-chain");
 96 
 97-    if (fTestNet && fRegTest)
 98-        throw std::runtime_error("Invalid combination of -regtest and -testnet.");
 99+    if (fChainArgSet ? (fTestNet || fRegTest) : (fTestNet && fRegTest))
100+        throw std::runtime_error("Invalid combination of -regtest, -testnet and -chain. Only one can be used.");

MarcoFalke commented at 9:11 pm on November 14, 2017:

refactoring-nit:

0if (is_chain_arg_set + fRegTest + fTestNet >= 2) {
1    throw std::runtime_eror("... Can use at most one.");

in src/chainparams.cpp:411 in b43a3cd851 outdated

370+    }
371+
372+public:
373+    CCustomParams(const std::string& chain, ArgsManager& args)
374+    {
375+        strNetworkID = chain;

MarcoFalke commented at 9:45 pm on November 14, 2017:

nit for the chain params above: Since you compare strNetworkID with CBaseChainParams, they should be set to the constant, instead of verbose copy.

jtimon commented at 9:21 pm on December 11, 2017:

The “constants” (static attributes) are not always available, see https://github.com/bitcoin/bitcoin/pull/8994/commits/52f6f43968595a769b109b9b6987ab03275c4a9d#r156194083

Apart from that, this allows chain with other names (say -chain=custom2) and they will work the same as custom except for the strNetworkID as suggested by @jnewbery . This eliminates the need for the -chainpetname argument I was using before. Note that strNetworkID is used when calling CreateGenesisBlock.

in src/qt/networkstyle.cpp:20 in b43a3cd851 outdated

19-    const char *titleAddText;
20 } network_styles[] = {
21-    {"main", QAPP_APP_NAME_DEFAULT, 0, 0, ""},
22-    {"test", QAPP_APP_NAME_TESTNET, 70, 30, QT_TRANSLATE_NOOP("SplashScreen", "[testnet]")},
23-    {"regtest", QAPP_APP_NAME_TESTNET, 160, 30, "[regtest]"}
24+    {"main", QAPP_APP_NAME_DEFAULT, 0, 0},

MarcoFalke commented at 9:49 pm on November 14, 2017:

nit: Since you compare those strings against the constants in CBaseChainParams, you might want to replace the strings with the constants.

jtimon commented at 8:47 pm on December 11, 2017:

Unfortunately, since std::string is a non-literal type, these constants are initialized in chainparamsbase.cpp after network_styles here. If these constants were macros instead of static attributes there would be no problem. Since now we sometimes do scripted diffs PRs, perhaps it’s a good time to think about s/CBaseChainParams::MAIN/CHAINPARAMS_MAIN/ and then more s/“main”/CHAINPARAMS_MAIN/ but this can be done independently from this PR (before or after).

in src/chainparams.cpp:383 in ec3235ac86 outdated

374+    {
375+        strNetworkID = chain;
376+
377+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].bit = 28;
378+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nStartTime = 0;
379+        consensus.vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].nTimeout = 999999999999ULL;

MarcoFalke commented at 10:01 pm on November 14, 2017:

Needs rebase, which raises the question why this class can not just derive from the CRegTestParams class…

in src/chainparams.cpp:352 in ec3235ac86 outdated

343+ */
344+class CCustomParams : public CChainParams {
345+
346+    void UpdateFromArgs(ArgsManager& args)
347+    {
348+        consensus.fPowAllowMinDifficultyBlocks = args.GetBoolArg("-con_fpowallowmindifficultyblocks", true);

MarcoFalke commented at 10:06 pm on November 14, 2017:

When it is possible to derive from the RegTestPrams class, you might want to put the current value (i.e. consensus.fPowAllowMinDifficultyBlocks instead of the hardcoded value here?

in src/chainparamsbase.cpp:20 in ec3235ac86 outdated

16@@ -17,11 +17,13 @@ const std::string CBaseChainParams::REGTEST = "regtest";
17 void AppendParamsHelpMessages(std::string& strUsage, bool debugHelp)
18 {
19     strUsage += HelpMessageGroup(_("Chain selection options:"));
20-    strUsage += HelpMessageOpt("-chain=<chain>", _("Use the chain <chain> (default: main). Allowed values: main, testnet, regtest"));
21+    strUsage += HelpMessageOpt("-chain=<chain>", _("Use the chain <chain> (default: main). Reserved values: main, test, regtest"));

MarcoFalke commented at 10:33 pm on November 14, 2017:

Nit: The string constants should not be translated. Mind replacing them by CBaseChainParams constants.

jtimon commented at 9:16 pm on December 11, 2017:

See https://github.com/bitcoin/bitcoin/pull/8994/commits/52f6f43968595a769b109b9b6987ab03275c4a9d#r156194083 I didn’t check if it applies to this though.

in src/chainparamsbase.cpp:25 in ec3235ac86 outdated

21+    strUsage += HelpMessageOpt("-chain=<chain>", _("Use the chain <chain> (default: main). Reserved values: main, test, regtest"));
22     strUsage += HelpMessageOpt("-testnet", _("Use the test chain"));
23     if (debugHelp) {
24         strUsage += HelpMessageOpt("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. "
25                                    "This is intended for regression testing tools and app development.");
26+        strUsage += HelpMessageGroup(_("Custom chain selection options (only for -chain=<custom> other than main, test or regtest):"));

MarcoFalke commented at 10:33 pm on November 14, 2017:

Same translation nit here.

in src/chainparamsbase.cpp:26 in ec3235ac86 outdated

22     strUsage += HelpMessageOpt("-testnet", _("Use the test chain"));
23     if (debugHelp) {
24         strUsage += HelpMessageOpt("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. "
25                                    "This is intended for regression testing tools and app development.");
26+        strUsage += HelpMessageGroup(_("Custom chain selection options (only for -chain=<custom> other than main, test or regtest):"));
27+        strUsage += HelpMessageOpt("-con_nsubsidyhalvinginterval=<int>", _("Custom subsidy halving interval."));

MarcoFalke commented at 10:34 pm on November 14, 2017:

Surely this should not be translated. Might as well remove it altogether from the help output.

jtimon commented at 9:07 pm on December 11, 2017:

ack on the translation nit since this is only for the debug options anyway. On anything in the debug options at all, I was using -con_nsubsidyhalvinginterval just as an example to catch feedback like this. I think probably all the supported options should be shown or none of them. Which also opens the question of whether all options are worth it or not. For example, -con_nsubsidyhalvinginterval may not be particularly interesting. I think activation logic for bip30 (via hardcoded block hash for bip34 activation), bip90, and bip9 are the most interesting (even though bip9 custom activation logic was already possible for regtest). Perhaps seeds or network magic makes sense too. I focused on implementing all the simple ones and documenting just one of them.

in src/chainparamsbase.cpp:72 in ec3235ac86 outdated

69+class CBaseCustomParams : public CBaseChainParams
70+{
71+public:
72+    CBaseCustomParams(const std::string& chain)
73+    {
74+        nRPCPort = 18332;

MarcoFalke commented at 10:36 pm on November 14, 2017:

Maybe set to 18553 to prevent another #10825

MarcoFalke commented at 10:38 pm on November 14, 2017: member

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

MarcoFalke commented at 10:38 pm on November 14, 2017: member

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

in src/qt/networkstyle.cpp:10 in b43a3cd851 outdated

 5@@ -6,18 +6,20 @@
 6 
 7 #include "guiconstants.h"
 8 
 9+#include "chainparamsbase.h"
10+#include "tinyformat.h"

MarcoFalke commented at 9:39 pm on November 29, 2017:

Needs rebase for the bracket change

Sjors commented at 3:43 pm on December 1, 2017: member

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.

jtimon force-pushed on Dec 11, 2017

jtimon force-pushed on Dec 12, 2017

jtimon commented at 1:00 am on December 12, 2017: contributor

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

in src/chainparamsbase.cpp:45 in 0ff8c677be outdated

45-        return std::unique_ptr<CBaseChainParams>(new CBaseRegTestParams());
46-    else
47-        throw std::runtime_error(strprintf("%s: Unknown chain %s.", __func__, chain));
48+        return std::unique_ptr<CBaseChainParams>(new CBaseChainParams("regtest", 18443));
49+
50+    return std::unique_ptr<CBaseChainParams>(new CBaseChainParams(chain, 18553));

Sjors commented at 10:37 am on December 12, 2017:

Isn’t the convention for the RPC port to be P2P port - 1?

jtimon commented at 2:17 pm on December 12, 2017:

You mean ndefault port could be, by default 18554 instead of 18444 ? Yeah, That would make it a little bit more different than regtest by default but I guess it’s alright. If @MarcoFalke hasn’t asked I would have been personally fine with leaving both the same as regtest. These are default ports that can be configured manually anyway.

Sjors commented at 10:56 am on December 12, 2017: member

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?

jtimon commented at 2:18 pm on December 12, 2017: contributor

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.

Sjors commented at 2:46 pm on December 12, 2017: member

@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.

jtimon commented at 4:45 pm on December 12, 2017: contributor

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.

jtimon force-pushed on Jan 9, 2018

jtimon commented at 3:27 am on January 9, 2018: contributor

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).

jtimon force-pushed on Jan 12, 2018

jtimon force-pushed on Jan 31, 2018

jtimon commented at 11:56 pm on January 31, 2018: contributor

Squashed the commit in which CCustomParams extends CRegTestParams and rebased.

jtimon force-pushed on Feb 1, 2018

jtimon commented at 2:00 am on February 1, 2018: contributor

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?

MarcoFalke referenced this in commit 4cad91663d on Feb 2, 2018

jtimon force-pushed on Feb 8, 2018

jtimon commented at 9:22 pm on February 8, 2018: contributor

Needed rebase

jtimon force-pushed on Feb 11, 2018

jtimon commented at 8:30 pm on February 20, 2018: contributor

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

Sjors commented at 9:17 am on February 22, 2018: member

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.

in contrib/devtools/check-doc.py:28 in 54a70ff4f8 outdated

20@@ -21,9 +21,12 @@
21 CMD_GREP_DOCS = r"egrep -r -I 'HelpMessageOpt\(\"\-[^\"=]+?(=|\")' %s" % (CMD_ROOT_DIR)
22 REGEX_ARG = re.compile(r'(?:map(?:Multi)?Args(?:\.count\(|\[)|Get(?:Bool)?Arg\()\"(\-[^\"]+?)\"')
23 REGEX_DOC = re.compile(r'HelpMessageOpt\(\"(\-[^\"=]+?)(?:=|\")')
24+
25 # list unsupported, deprecated and duplicate args as they need no documentation
26 SET_DOC_OPTIONAL = set(['-rpcssl', '-benchmark', '-h', '-help', '-socks', '-tor', '-debugnet', '-whitelistalwaysrelay', '-prematurewitness', '-walletprematurewitness', '-promiscuousmempoolflags', '-blockminsize', '-dbcrashratio', '-forcecompactdb', '-usehd'])
27 
28+SET_DOC_OPTIONAL.update(['-con_fpowallowmindifficultyblocks', '-con_fpownoretargeting', '-con_nsubsidyhalvinginterval', '-con_bip16height', '-con_bip34height', '-con_bip65height', '-con_bip66height', '-con_npowtargettimespan', '-con_npowtargetspacing', '-con_nrulechangeactivationthreshold', '-con_nminerconfirmationwindow', '-con_powlimit', '-con_bip34hash', '-con_nminimumchainwork', '-con_defaultassumevalid', '-ndefaultport', '-npruneafterheight', '-fdefaultconsistencychecks', '-frequirestandard', '-fmineblocksondemand', '-bech32_hrp'])

Sjors commented at 3:13 pm on February 26, 2018:

These might actually be worth documenting.

jtimon commented at 7:01 pm on February 26, 2018:

Happy to do so if more people feel the same way, or only some of them, whatever.

in src/chainparams.cpp:373 in 54a70ff4f8 outdated

368+        consensus.nMinerConfirmationWindow = args.GetArg("-con_nminerconfirmationwindow", consensus.nMinerConfirmationWindow);
369+
370+        consensus.nMinimumChainWork = uint256S(args.GetArg("-con_nminimumchainwork", "0x0"));
371+        consensus.defaultAssumeValid = uint256S(args.GetArg("-con_defaultassumevalid", "0x00"));
372+
373+        nDefaultPort = args.GetArg("-ndefaultport", nDefaultPort);

Sjors commented at 3:36 pm on February 26, 2018:

Redundant given port=?

Sjors commented at 3:50 pm on February 26, 2018: member

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 _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.

jtimon commented at 7:19 pm on February 26, 2018: contributor

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!

Sjors commented at 11:55 am on February 27, 2018: member

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.

jtimon commented at 9:42 pm on February 27, 2018: contributor

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.

Sjors commented at 9:26 am on February 28, 2018: member

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.

jtimon commented at 10:48 am on March 22, 2018: contributor

@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:

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)
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= 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).
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 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= 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= 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?

MarcoFalke commented at 10:58 am on March 22, 2018: member

@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.

jtimon commented at 5:05 pm on April 9, 2018: contributor

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

MarcoFalke commented at 5:08 pm on April 9, 2018: member

@jtimon Sounds fine. (Needs rebase)

MarcoFalke added the label Needs rebase on Jun 6, 2018

MarcoFalke referenced this in commit 4dac24db23 on Sep 24, 2018

fanquake commented at 2:52 am on October 10, 2018: member

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.

jtimon commented at 3:01 am on October 10, 2018: contributor

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

jtimon force-pushed on Oct 13, 2018

jtimon commented at 1:01 am on October 13, 2018: contributor

Rebased earlier than anticipated, but…

Even without running with:

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

… the following test fail:

 0feature_block.py                      | ✖ Failed  | 1 s
 1feature_cltv.py                       | ✖ Failed  | 11 s
 2feature_csv_activation.py             | ✖ Failed  | 1 s
 3feature_dersig.py                     | ✖ Failed  | 9 s
 4interface_bitcoin_cli.py              | ✖ Failed  | 1 s
 5mining_basic.py                       | ✖ Failed  | 3 s
 6p2p_invalid_block.py                  | ✖ Failed  | 2 s
 7p2p_invalid_tx.py                     | ✖ Failed  | 1 s
 8p2p_segwit.py                         | ✖ Failed  | 3 s
 9rpc_getblockstats.py                  | ✖ Failed  | 1 s
10rpc_scantxoutset.py                   | ✖ Failed  | 3 s
11wallet_disableprivatekeys.py --usecli | ✖ Failed  | 1 s
12wallet_multiwallet.py --usecli        | ✖ Failed  | 1 s

Any help welcomed

Sjors commented at 7:39 am on October 13, 2018: member

feature_block.py fails for me because it’s looking for a log file in regtest instead of regtest2:

Probably test_node.py needs a tweak:

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... :-)

in src/chainparamsbase.cpp:20 in 66d7af2aad outdated

16@@ -17,6 +17,7 @@ const std::string CBaseChainParams::REGTEST = "regtest";
17 
18 void SetupChainParamsBaseOptions()
19 {
20+    gArgs.AddArg("--chain=<chain>", "Use the chain <chain> (default: main). Reserved values: main, test, regtest", false, OptionsCategory::CHAINPARAMS);

Sjors commented at 7:48 am on October 13, 2018:

Why -- rather than -?

in test/functional/combine_logs.py:26 in 66d7af2aad outdated

22@@ -23,6 +23,7 @@ def main():
23     parser = argparse.ArgumentParser(usage='%(prog)s [options] <test temporary directory>', description=__doc__)
24     parser.add_argument('-c', '--color', dest='color', action='store_true', help='outputs the combined log with events colored by source (requires posix terminal colors. Use less -r for viewing)')
25     parser.add_argument('--html', dest='html', action='store_true', help='outputs the combined log as html. Requires jinja2. pip install jinja2')
26+    parser.add_argument('--chain', dest='chain', action='store_true', help='selected chain in the tests (default: regtest2)', const='regtest2')

ken2812221 commented at 1:30 pm on October 13, 2018:

const is an unknown argument. I assume that this is what you want:

0parser.add_argument('--chain', dest='chain', help='selected chain in the tests (default: %(default)s)', default='regtest2')

ken2812221 changes_requested

jtimon force-pushed on Oct 13, 2018

jtimon commented at 5:16 pm on October 13, 2018: contributor

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

Sjors commented at 2:45 am on October 14, 2018: member

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

jtimon force-pushed on Oct 14, 2018

jtimon commented at 12:26 pm on October 14, 2018: contributor

@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).

jtimon force-pushed on Oct 14, 2018

in src/chainparams.cpp:416 in 43614c4f79 outdated

411+        consensus.nMinerConfirmationWindow = args.GetArg("-con_nminerconfirmationwindow", consensus.nMinerConfirmationWindow);
412+
413+        consensus.nMinimumChainWork = uint256S(args.GetArg("-con_nminimumchainwork", "0x0"));
414+        consensus.defaultAssumeValid = uint256S(args.GetArg("-con_defaultassumevalid", "0x00"));
415+
416+        nPruneAfterHeight = args.GetArg("-npruneafterheight", nPruneAfterHeight);

practicalswift commented at 12:21 pm on October 15, 2018:

Make this signedness conversion explicit :-)

jtimon commented at 4:07 pm on October 16, 2018:

Will do the same for nRuleChangeActivationThreshold and nMinerConfirmationWindow.

in src/util.cpp:974 in 43614c4f79 outdated

973+    const bool fTestNet = ArgsManagerHelper::GetNetBoolArg(*this, "-testnet");
974+    const bool is_chain_arg_set = IsArgSet("-chain");
975 
976-    if (fTestNet && fRegTest)
977-        throw std::runtime_error("Invalid combination of -regtest and -testnet.");
978+    if (is_chain_arg_set + fRegTest + fTestNet > 1) {

practicalswift commented at 12:25 pm on October 15, 2018:

Make the conversion from bool to int explicit :-)

in src/chainparams.cpp:394 in 43614c4f79 outdated

386@@ -387,6 +387,80 @@ void CRegTestParams::UpdateVersionBitsParametersFromArgs(const ArgsManager& args
387     }
388 }
389 
390+/**
391+ * Custom params for testing.
392+ */
393+class CCustomParams : public CRegTestParams {
394+

practicalswift commented at 12:26 pm on October 15, 2018:

nit: Redundant blank line

in src/chainparams.cpp:428 in 43614c4f79 outdated

423+        base58Prefixes[SCRIPT_ADDRESS] = std::vector<unsigned char>(1, args.GetArg("-scriptprefix", 196));
424+        base58Prefixes[SECRET_KEY] =     std::vector<unsigned char>(1, args.GetArg("-secretprefix", 239));
425+
426+        const std::string extpubprefix = args.GetArg("-extpubkeyprefix", "043587CF");
427+        if (!IsHex(extpubprefix) || extpubprefix.size() != 8) {
428+            assert("-extpubkeyprefix must be hex string of length 8" && false);

practicalswift commented at 1:13 pm on October 15, 2018:

Nit: Move outside of the if:

0assert(IsHex(extpubprefix) && extpubprefix.size() == 8 && "-extpubkeyprefix must be hex string of length 8");

in src/chainparams.cpp:434 in 43614c4f79 outdated

429+        }
430+        base58Prefixes[EXT_PUBLIC_KEY] = ParseHex(extpubprefix);
431+
432+        const std::string extprvprefix = args.GetArg("-extprvkeyprefix", "04358394");
433+        if (!IsHex(extprvprefix) || extprvprefix.size() != 8) {
434+            assert("-extprvkeyprefix must be hex string of length 8" && false);

practicalswift commented at 1:14 pm on October 15, 2018:

Same here.

in src/chainparams.cpp:440 in 43614c4f79 outdated

435+        }
436+        base58Prefixes[EXT_SECRET_KEY] = ParseHex(extprvprefix);
437+
438+        const std::string magic_str = args.GetArg("-pchmessagestart", "FABFB5DA");
439+        if (!IsHex(magic_str) || magic_str.size() != 8) {
440+            assert("-pchmessagestart must be hex string of length 8" && false);

practicalswift commented at 1:14 pm on October 15, 2018:

Same here.

in test/functional/test_framework/test_framework.py:423 in 43614c4f79 outdated

419@@ -419,6 +420,9 @@ def _start_logging(self):
420             rpc_handler.setLevel(logging.DEBUG)
421             rpc_logger.addHandler(rpc_handler)
422 
423+    def log_filename(self, dirname, n_node, logname):

practicalswift commented at 1:16 pm on October 15, 2018:

From where is log_filename used? :-)

jtimon force-pushed on Oct 16, 2018

jtimon commented at 4:26 pm on October 16, 2018: contributor

Fixed @practicalswift ’s nits.

instagibbs referenced this in commit de842cb58f on Oct 16, 2018

DrahtBot removed the label Needs rebase on Oct 20, 2018

DrahtBot commented at 11:18 am on October 20, 2018: 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:

#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.

in src/chainparamsbase.cpp:20 in daf6b9036e outdated

16@@ -17,10 +17,12 @@ const std::string CBaseChainParams::REGTEST = "regtest";
17 
18 void SetupChainParamsBaseOptions()
19 {
20+    gArgs.AddArg("-chain=<chain>", "Use the chain <chain> (default: main). Reserved values: main, test, regtest", false, OptionsCategory::CHAINPARAMS);

MarcoFalke commented at 9:11 pm on November 1, 2018:

I am wondering if this option should be hidden like regtest…

Effectively this reverts the option sanitization check at initialization, where typos would be rejected for e.g. -tesnet. Now, -chain=tesnet is neither an error, nor does sync with testnet3.

jtimon commented at 12:25 pm on November 5, 2018:

I’m fine changing it to hidden, but I plan to stop using -testnet and use the equivalent “-chain=test” myself. “-chain=testnet” doesn’t work as you expect because the bip70 petname for testnet3 is neither “testnet” nor “testnet3” (what would have been my preference) but “test”. Note it says the reserved value is “test” and not “testnet”. “-chain=testnet” just create a new custom chain with testnet as pet name.

jtimon commented at 12:27 pm on November 5, 2018:

Presumably -testnet and -regtest could be deprecated one day, since the -chain parameter is sufficient (-chain=test is equivalent to -testnet, -chain=regtest is equivalent to -regtest).

MarcoFalke commented at 2:39 pm on November 5, 2018:

Ah, sorry. I mean -chain=tes (with a typo) would no longer throw an error nor sync with testnet. I just wanted to point out that behaviour change.

flack commented at 10:00 pm on November 20, 2018:

maybe it could log/print an extra message like “using custom chain xyz” when chain is not equal to main, test or regtest? At least that would make the potential typo easier to spot

jtimon commented at 6:40 pm on December 20, 2018:

@flack That seems simple and reasonable. @MarcoFalke would that solve your concern?

DrahtBot added the label Needs rebase on Nov 5, 2018

in test/functional/combine_logs.py:40 in daf6b9036e outdated

22@@ -23,6 +23,7 @@ def main():
23     parser = argparse.ArgumentParser(usage='%(prog)s [options] <test temporary directory>', description=__doc__)
24     parser.add_argument('-c', '--color', dest='color', action='store_true', help='outputs the combined log with events colored by source (requires posix terminal colors. Use less -r for viewing)')
25     parser.add_argument('--html', dest='html', action='store_true', help='outputs the combined log as html. Requires jinja2. pip install jinja2')
26+    parser.add_argument('--chain', dest='chain', help='selected chain in the tests (default: regtest2)', default='regtest2')

luke-jr commented at 9:50 pm on November 19, 2018:

We only need one regression test framework…

jtimon commented at 8:37 pm on November 20, 2018:

Another option that would also work would be to simply hardcode regtest2 in combine_logs.py (instead of hardcoding regtest like before). Would that look better to you?

kallewoof commented at 11:29 pm on November 20, 2018:

Why can’t you name it just “regtest”?

jtimon commented at 6:39 pm on December 20, 2018:

Because regtest is reserved for the current regtest, with a different genesis block hash. I suggested we just remove the regtest chainparams and simply use a custom chain named regtest instead (wouldn’t require this change), but someone was against it.

in test/functional/test_framework/test_node.py:62 in daf6b9036e outdated

58@@ -59,11 +59,12 @@ class TestNode():
59     To make things easier for the test writer, any unrecognised messages will
60     be dispatched to the RPC connection."""
61 
62-    def __init__(self, i, datadir, *, rpchost, timewait, bitcoind, bitcoin_cli, mocktime, coverage_dir, extra_conf=None, extra_args=None, use_cli=False):
63+    def __init__(self, i, datadir, chain, *, rpchost, timewait, bitcoind, bitcoin_cli, mocktime, coverage_dir, extra_conf=None, extra_args=None, use_cli=False):

stevenroose commented at 10:57 am on November 26, 2018:

You need to explicitly add -chain= to the arg list here. Because the config file chain variable is created before any of the tests start, so that variable defaults to the default test-framework chain. That’s why I also removed the chain= in the config file.

jtimon commented at 7:54 pm on December 20, 2018:

Not sure if this comment applies to this PR or to elements.

in test/functional/test_framework/test_framework.py:299 in daf6b9036e outdated

295@@ -295,7 +296,7 @@ def add_nodes(self, num_nodes, extra_args=None, *, rpchost=None, binary=None):
296         assert_equal(len(extra_args), num_nodes)
297         assert_equal(len(binary), num_nodes)
298         for i in range(num_nodes):
299-            self.nodes.append(TestNode(i, get_datadir_path(self.options.tmpdir, i), rpchost=rpchost, timewait=self.rpc_timewait, bitcoind=binary[i], bitcoin_cli=self.options.bitcoincli, mocktime=self.mocktime, coverage_dir=self.options.coveragedir, extra_conf=extra_confs[i], extra_args=extra_args[i], use_cli=self.options.usecli))
300+            self.nodes.append(TestNode(i, get_datadir_path(self.options.tmpdir, i), self.chain, rpchost=rpchost, timewait=self.rpc_timewait, bitcoind=binary[i], bitcoin_cli=self.options.bitcoincli, mocktime=self.mocktime, coverage_dir=self.options.coveragedir, extra_conf=extra_confs[i], extra_args=extra_args[i], use_cli=self.options.usecli))

stevenroose commented at 10:59 am on November 26, 2018:

This doesn’t seem to be flexible enough to support multiple chains per test. I had to make this change to allow for that. (Also see my suggestion to TestNode to allow that.)

jtimon commented at 6:40 pm on December 20, 2018:

This doesn’t intend to support multiple chains per test, at least not in principle. Note this comment is appearing in bitcoin/bitcoin, not elements.

stevenroose changes_requested

stevenroose commented at 11:11 am on November 26, 2018: contributor

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

Sjors commented at 1:51 pm on November 27, 2018: member

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.

jtimon force-pushed on Dec 20, 2018

jtimon commented at 7:54 pm on December 20, 2018: contributor

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

DrahtBot removed the label Needs rebase on Dec 20, 2018

DrahtBot added the label Needs rebase on Jan 3, 2019

jtimon force-pushed on Jun 8, 2019

jtimon commented at 2:51 pm on June 8, 2019: contributor

Rebased again

DrahtBot removed the label Needs rebase on Jun 8, 2019

jtimon commented at 6:29 pm on June 13, 2019: contributor

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:

0[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.

jtimon force-pushed on Jun 13, 2019

laanwj added this to the "Chasing Concept ACK" column in a project

instagibbs commented at 7:15 pm on June 13, 2019: member

concept ACK

It makes testing easier, enough said.

jtimon force-pushed on Jun 14, 2019

fanquake added the label Needs Conceptual Review on Jun 16, 2019

kallewoof commented at 4:37 am on June 17, 2019: member

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.

jtimon commented at 4:06 pm on June 17, 2019: contributor

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”.

kallewoof commented at 5:28 pm on June 17, 2019: member

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).

ajtowns commented at 5:10 am on June 18, 2019: member

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.

MarcoFalke commented at 1:57 pm on June 18, 2019: member

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?

jtimon commented at 2:41 pm on June 18, 2019: contributor

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.

Sjors commented at 2:50 pm on July 5, 2019: member

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].

jtimon commented at 4:13 pm on July 5, 2019: contributor

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 .

DrahtBot added the label Needs rebase on Jul 24, 2019

jtimon commented at 7:48 pm on August 1, 2019: contributor

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.

jtimon force-pushed on Aug 1, 2019

DrahtBot removed the label Needs rebase on Aug 1, 2019

DrahtBot added the label Needs rebase on Aug 2, 2019

MarcoFalke referenced this in commit d5ea8f4bf3 on Aug 5, 2019

jtimon force-pushed on Aug 7, 2019

jtimon commented at 3:27 am on August 7, 2019: contributor

Rebased and adapted the tests a little bit more

DrahtBot removed the label Needs rebase on Aug 7, 2019

jtimon force-pushed on Aug 7, 2019

in test/functional/test_framework/test_node.py:558 in 6b85c37fa3 outdated

554@@ -554,7 +555,7 @@ def send_cli(self, command=None, *args, **kwargs):
555         pos_args = [arg_to_cli(arg) for arg in args]
556         named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items()]
557         assert not (pos_args and named_args), "Cannot use positional arguments and named arguments in the same bitcoin-cli call"
558-        p_args = [self.binary, "-datadir=" + self.datadir] + self.options
559+        p_args = [self.binary, "-datadir=" + self.datadir, "-chain=" + self.chain] + self.options

MarcoFalke commented at 11:46 am on August 7, 2019:

Why is this required, given that you already pass the datadir, which has the conf file, which sets the chain?

jtimon commented at 9:13 am on August 8, 2019:

yeah, it seems that’s redundant, I’ll remove it.

jtimon force-pushed on Aug 8, 2019

jtimon commented at 9:32 am on August 8, 2019: contributor

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

jtimon commented at 9:38 am on August 8, 2019: contributor

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.

laanwj removed this from the "Chasing Concept ACK" column in a project

sidhujag referenced this in commit 511873dd22 on Aug 10, 2019

DrahtBot added the label Needs rebase on Aug 15, 2019

Sjors commented at 12:33 pm on August 19, 2019: member

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”.

MarcoFalke commented at 12:42 pm on August 19, 2019: member

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

Sjors commented at 2:24 pm on August 19, 2019: member

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

jtimon commented at 3:10 pm on August 19, 2019: contributor

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 .

jtimon force-pushed on Aug 22, 2019

jtimon commented at 1:56 pm on August 22, 2019: contributor

Rebased, also separated #16680 as suggested.

jtimon force-pushed on Aug 22, 2019

DrahtBot removed the label Needs rebase on Aug 22, 2019

jtimon commented at 2:19 pm on August 22, 2019: contributor

Also separated #16681

jtimon force-pushed on Aug 22, 2019

jtimon force-pushed on Aug 29, 2019

jtimon referenced this in commit 1669579f47 on Aug 30, 2019

jtimon commented at 9:24 pm on September 6, 2019: contributor

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.

jtimon force-pushed on Sep 6, 2019

MarcoFalke referenced this in commit 750c2fbf26 on Sep 10, 2019

DrahtBot added the label Needs rebase on Sep 10, 2019

sidhujag referenced this in commit c15fa2cfee on Sep 10, 2019

jtimon force-pushed on Sep 10, 2019

jtimon commented at 8:01 pm on September 10, 2019: contributor

Rebased after #16680 was merged

DrahtBot removed the label Needs rebase on Sep 10, 2019

jtimon commented at 10:32 pm on September 10, 2019: contributor

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

jtimon force-pushed on Sep 11, 2019

DrahtBot added the label Needs rebase on Sep 12, 2019

jtimon force-pushed on Sep 12, 2019

jtimon commented at 5:41 pm on September 12, 2019: contributor

Needed rebase after #16551 was merged.

DrahtBot removed the label Needs rebase on Sep 12, 2019

DrahtBot added the label Needs rebase on Sep 30, 2019

jtimon force-pushed on Oct 2, 2019

jtimon commented at 4:42 pm on October 2, 2019: contributor

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).

DrahtBot removed the label Needs rebase on Oct 2, 2019

jtimon force-pushed on Oct 2, 2019

jtimon commented at 7:43 pm on October 2, 2019: contributor

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

jtimon force-pushed on Oct 3, 2019

jtimon commented at 0:19 am on October 3, 2019: contributor

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

jtimon force-pushed on Oct 3, 2019

jtimon commented at 3:25 am on October 3, 2019: contributor

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

jtimon force-pushed on Oct 3, 2019

jtimon force-pushed on Oct 5, 2019

Tests: Use self.chain instead of 'regtest' in almost all current tests 83f08b3064

9102: Really don't validate genesis block c366558679

Config: Allow any chain/section name b3339893a9

Chainparams: Test: Make is_test_chain configurable for regtest 6ad087eeb3

Test: select chain using -chain=regtest instead of regtest=1 5326b3f5cd

Testchains: Qt: Introduce custom chains with different:

1) genesis block hash
2) default datadir
3) chain name (bip70)

all directly or indirectly configured with the -chain option

...whose constructor reads params from regular arguments (like regtests)...

Qt: Add a default purple and title for unkown chains

76c00e0701

Test: Custom chain genesis' are deterministic from the name d42efe17c8

Chainparams: Make regtest almost fully customizable 635771dfca

QA: Use resgtest2 chain instead of regtest for rpc tests (except rpc_getblockstats) 0da5e7294e

jtimon force-pushed on Oct 8, 2019

jtimon commented at 8:18 pm on October 8, 2019: contributor

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

jtimon commented at 1:38 pm on October 9, 2019: contributor

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”.

jtimon closed this on Oct 9, 2019

MarcoFalke referenced this in commit f32564f0a7 on Feb 4, 2020

sidhujag referenced this in commit e5ba378689 on Feb 9, 2020

sidhujag referenced this in commit 269521a1d5 on Nov 10, 2020

KolbyML referenced this in commit 9dfef518f7 on Jan 17, 2021

KolbyML referenced this in commit ebf48e94dd on Jan 17, 2021

UdjinM6 referenced this in commit 9a5d7fc5ac on Jan 21, 2021

PastaPastaPasta referenced this in commit e197f976e1 on Jan 22, 2021

rkarthik2k21 referenced this in commit dae58c5554 on Aug 12, 2021

rkarthik2k21 referenced this in commit 90b9a42a9b on Aug 25, 2021

UdjinM6 referenced this in commit f456edb07e on Sep 2, 2021

DrahtBot locked this on Feb 15, 2022

Testchains: Introduce custom chain whose constructor… #8994

Conflicts