Preparations for more testchains #16680

pull jtimon wants to merge 2 commits into bitcoin:master from jtimon:b19-testchains-qt changing 11 files +29 −24

jtimon commented at 1:53 PM on August 22, 2019: contributor

Separated from #8994 as suggested by MarcoFalke and Sjors in #8994 (comment)

You can't really test the qt changes on their own, so to test them, use #8994 .
fanquake added the label Refactoring on Aug 22, 2019
DrahtBot commented at 2:11 PM on August 22, 2019: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #16411 (Signet support by kallewoof)
- #15934 (Separate settings merging from parsing by ryanofsky)
- #8994 (Testchains: Introduce custom chain whose constructor... by jtimon)
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.
jtimon force-pushed on Aug 22, 2019

in src/chainparamsbase.cpp:20 in 5a21981b13 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). Allowed values: main, testnet, regtest", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);

MarcoFalke commented at 4:16 PM on August 25, 2019:

I don't think testnet is an allowed value

    gArgs.AddArg("-chain=<chain>", "Use the chain <chain> (default: main). Allowed values: main, test, regtest", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);

Alternatively, you can use

Join({
  CBaseChainParams::MAIN,
  CBaseChainParams::TESTNET,
  CBaseChainParams::REGTEST,
}, ", ");

jtimon force-pushed on Aug 29, 2019
jtimon commented at 12:14 PM on August 29, 2019: contributor

Fixed @MarcoFalke 's nit and slightly improve documentation.
Sjors commented at 2:15 PM on August 29, 2019: member

ACK dba00d2

It was fairly easy to cherry-pick these commits onto the signet PR (https://github.com/Sjors/bitcoin/commit/df1846c8ae97f28165c0d0b44c6cd7ae6b1fcbba & https://github.com/bitcoin/bitcoin/commit/124ee3786802b19e81351808f3145ed4e80dc32c, cc @kallewoof). By dropping the signet lines from the QT commit, I indeed get a purple client.
kallewoof commented at 2:25 PM on August 29, 2019: member

Nice!
jtimon commented at 6:27 PM on August 30, 2019: contributor

@Sjors sure, that's another way to test it, thanks.
in src/qt/networkstyle.cpp:94 in dba00d2b04 outdated
```
  87 | @@ -85,8 +88,8 @@ const NetworkStyle *NetworkStyle::instantiate(const QString &networkId)
  88 |                      network_styles[x].appName,
  89 |                      network_styles[x].iconColorHueShift,
  90 |                      network_styles[x].iconColorSaturationReduction,
  91 | -                    network_styles[x].titleAddText);
  92 | +                    titleAddText.c_str());
  93 |          }
  94 |      }
  95 | -    return nullptr;
  96 | +    return new NetworkStyle(strprintf("%s-%s", QAPP_APP_NAME_CUSTOM, networkId).c_str(), 250, 30, titleAddText.c_str());
```
MarcoFalke commented at 2:49 PM on September 6, 2019:

in commit dba00d2b04c3bf0fd4d31cca662d262c459f2ebb:

This is dead code and I'd prefer if the "default purple" and "custom chain" concept were left for the testchains pull request. This is not required for signet and is only adding dead code.

I am fine with the titleAddText change in this commit, though. So you can either remove the commit or strip it of the concept of a custom chain.

jtimon commented at 6:58 PM on September 6, 2019:

I guess my idea was for signet to reuse this color, perhaps change the tittle from custom to signet if it comes first, and if not just reuse the custom name for all custom chains signet or not.

EDIT: I mean, or if it's about the color, we can change to the signet one, I couldn't care less. Or I can separate it as you sasy, it shouldn't take long.

MarcoFalke commented at 7:37 PM on September 6, 2019:

Wouldn't that also require 2f5796504a5d984276931da974436a8a5d352f74?

jtimon commented at 8:17 PM on September 6, 2019:

hopefully fixed
MarcoFalke dismissed

MarcoFalke commented at 2:51 PM on September 6, 2019: member

ACK 36f9573e82d370af76fed0789fe3d671546e6e85 (also run the test to verify that only error messages are responsible for change the hash)

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash fe315895de9bcfebf0a22dee8eb70c8cd08dd7ba0736e8d0c931542ea93a4bd7 -

</details>

MarcoFalke added this to the milestone 0.19.0 on Sep 6, 2019
Testchains: Generic selection with -chain=<str> in addition of -testnet and -regtest 052c54ecb0
Testchains: Qt: Simplify network/chain styles 3bf9d8cac0
jtimon force-pushed on Sep 6, 2019