refactor: Use name constants in chainparams initialization #17306

jtimon · 2019-10-29T19:32:09Z

I thought this wouldn't work for some reason, but it seems it does.
Just a little bit more consistency. I'm still not able to use them in qt/networkstyle.cpp though, not sure why.

maflcko · 2019-10-29T19:36:14Z

ACK 37b8475

hebasto

ACK 37b8475, I have reviewed the code and it looks OK, I agree it can be merged.

fjahr

ACK 37b8475

fjahr · 2019-10-29T19:46:25Z

src/chainparams.cpp

@@ -254,7 +254,7 @@ class CTestNetParams : public CChainParams {
 class CRegTestParams : public CChainParams {
 public:
    explicit CRegTestParams(const ArgsManager& args) {
-        strNetworkID = "regtest";
+        strNetworkID =  CBaseChainParams::REGTEST;


There seems to be an unnecessary white space here.

You may install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.

DrahtBot · 2019-10-29T20:05:56Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17032 (Tests: Chainparams: Make regtest almost fully customizable by jtimon)
#16411 (Signet support by kallewoof)

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.

practicalswift · 2019-10-29T23:34:57Z

Concept ACK

FWIW:

$ git grep -E '("main"|"test"|"regtest")' -- "*.cpp" "*.h" 
src/chainparams.cpp:        strNetworkID = "main";
src/chainparams.cpp:        strNetworkID = "test";
src/chainparams.cpp:        strNetworkID = "regtest";
src/chainparamsbase.cpp:const std::string CBaseChainParams::MAIN = "main";
src/chainparamsbase.cpp:const std::string CBaseChainParams::TESTNET = "test";
src/chainparamsbase.cpp:const std::string CBaseChainParams::REGTEST = "regtest";
src/chainparamsbase.cpp:        return MakeUnique<CBaseChainParams>("regtest", 18443);
src/qt/bitcoin.cpp:    util::ThreadSetInternalName("main");
src/qt/networkstyle.cpp:    {"main", QAPP_APP_NAME_DEFAULT, 0, 0},
src/qt/networkstyle.cpp:    {"test", QAPP_APP_NAME_TESTNET, 70, 30},
src/qt/networkstyle.cpp:    {"regtest", QAPP_APP_NAME_REGTEST, 160, 30}
src/qt/test/apptests.cpp:    QCOMPARE(value["chain"].get_str(), std::string("regtest"));
src/qt/test/rpcnestedtests.cpp:    { "test", "rpcNestedTest", &rpcNestedTest_rpc, {} },
src/qt/test/rpcnestedtests.cpp:    QVERIFY(result=="main");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "main");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "regtest");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
src/test/util_tests.cpp:    test_args.SelectConfigNetwork("test");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
src/test/util_tests.cpp:// - Testing "main" and "test" network values to make sure settings from network
src/test/util_tests.cpp:        parser.AddArg("-regtest", "regtest", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
src/test/util_tests.cpp:    BOOST_CHECK_EQUAL(FormatParagraph("test", 79, 0), "test");
src/wallet/db.cpp:                            "main",             // Logical db name
src/wallet/db.cpp:                            fMockDb ? strFilename.c_str() : "main",   // Logical db name
src/wallet/db.cpp:                                            "main",             // Logical db name
src/wallet/rpcwallet.cpp:    // Release the "main" shared pointer and prevent further notifications.
src/wallet/test/wallet_crypto_tests.cpp:    TestCrypter::TestPassphrase(ParseHex("0000deadbeef0000"), "test", 25000, \

laanwj · 2019-10-30T11:22:25Z

ACK 37b8475

37b8475 Chainparams: Use name constants in chainparams initialization (Jorge Timón) Pull request description: I thought this wouldn't work for some reason, but it seems it does. Just a little bit more consistency. I'm still not able to use them in qt/networkstyle.cpp though, not sure why. ACKs for top commit: MarcoFalke: ACK 37b8475 laanwj: ACK 37b8475 hebasto: ACK 37b8475, I have reviewed the code and it looks OK, I agree it can be merged. fjahr: ACK 37b8475 Tree-SHA512: d9fa5df5650e10c645ac1f3afe831674a47f35d4a649e18a3d2aee1d04b08e6896aff6f1bbed0630d28775c51f989f9daaa9e405c9f3d7dca30e639a6f9008f0

jtimon · 2019-10-30T12:37:49Z

This is one of the few times when I feel you guys merged this too fast.
I would have happily removed the extra white space.
I would also had happily added more cases like @practicalswift seems to be suggesting.

laanwj · 2019-10-30T12:43:19Z

I didn't see that the whitespace nit wasn't solved yet.

I would also had happily added more cases like @practicalswift seems to be suggesting.

I think it's a bad idea to extend the scope of a PR after two people have ACKed it. It's a trivially correct change and people agree with it, so it should be merged quickly.

maflcko · 2019-10-30T12:46:56Z

It is not a disaster to have this merged, mistakes can always happen. Let's just fix up the nits in a follow-up.

Chainparams: Use name constants in chainparams initialization

37b8475

jtimon mentioned this pull request Oct 29, 2019

Tests: Chainparams: Make regtest almost fully customizable #17032

Closed

fanquake added the Refactoring label Oct 29, 2019

maflcko changed the title ~~Use name constants in chainparams initialization~~ refactor: Use name constants in chainparams initialization Oct 29, 2019

hebasto approved these changes Oct 29, 2019

View reviewed changes

fjahr approved these changes Oct 29, 2019

View reviewed changes

maflcko added the Waiting for author label Oct 30, 2019

laanwj merged commit 37b8475 into bitcoin:master Oct 30, 2019

laanwj removed the Waiting for author label Oct 30, 2019

jtimon deleted the b20-chain-constants branch October 30, 2019 12:50

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Use name constants in chainparams initialization #17306

refactor: Use name constants in chainparams initialization #17306

Uh oh!

jtimon commented Oct 29, 2019

Uh oh!

maflcko commented Oct 29, 2019

Uh oh!

hebasto left a comment

Uh oh!

fjahr left a comment

Uh oh!

fjahr Oct 29, 2019

Uh oh!

maflcko Oct 29, 2019

Uh oh!

DrahtBot commented Oct 29, 2019

Uh oh!

practicalswift commented Oct 29, 2019

Uh oh!

laanwj commented Oct 30, 2019

Uh oh!

jtimon commented Oct 30, 2019

Uh oh!

laanwj commented Oct 30, 2019 •

edited

Loading

Uh oh!

maflcko commented Oct 30, 2019

Uh oh!

Uh oh!

refactor: Use name constants in chainparams initialization #17306

refactor: Use name constants in chainparams initialization #17306

Uh oh!

Conversation

jtimon commented Oct 29, 2019

Uh oh!

maflcko commented Oct 29, 2019

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Oct 29, 2019

Conflicts

Uh oh!

practicalswift commented Oct 29, 2019

Uh oh!

laanwj commented Oct 30, 2019

Uh oh!

jtimon commented Oct 30, 2019

Uh oh!

laanwj commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 30, 2019

Uh oh!

Uh oh!

laanwj commented Oct 30, 2019 •

edited

Loading