Skip to content

refactor: Use name constants in chainparams initialization#17306

Merged
laanwj merged 1 commit intobitcoin:masterfrom
jtimon:b20-chain-constants
Oct 30, 2019
Merged

refactor: Use name constants in chainparams initialization#17306
laanwj merged 1 commit intobitcoin:masterfrom
jtimon:b20-chain-constants

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Oct 29, 2019

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.

@jtimon jtimon mentioned this pull request Oct 29, 2019
Closed
@maflcko maflcko changed the title Use name constants in chainparams initialization refactor: Use name constants in chainparams initialization Oct 29, 2019
@maflcko
Copy link
Member

maflcko commented Oct 29, 2019

ACK 37b8475

hebasto
hebasto approved these changes Oct 29, 2019
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fjahr
fjahr approved these changes Oct 29, 2019
View reviewed changes
Copy link
Contributor

@fjahr fjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 37b8475

src/chainparams.cpp
public:
explicit CRegTestParams(const ArgsManager& args) {
strNetworkID = "regtest";
strNetworkID = CBaseChainParams::REGTEST;
Copy link
Contributor

@fjahr fjahr Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be an unnecessary white space here.

Copy link
Member

@maflcko maflcko Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

DrahtBot commented Oct 29, 2019

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
Copy link
Contributor

practicalswift commented Oct 29, 2019

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
Copy link
Member

laanwj commented Oct 30, 2019

ACK 37b8475

laanwj added a commit that referenced this pull request Oct 30, 2019
@laanwj
Merge #17306: refactor: Use name constants in chainparams initialization
f129170 
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
@laanwj laanwj merged commit 37b8475 into bitcoin:master Oct 30, 2019
@jtimon
Copy link
Contributor Author

jtimon commented Oct 30, 2019

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
Copy link
Member

laanwj commented Oct 30, 2019 •
edited
Loading

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
Copy link
Member

maflcko commented Oct 30, 2019

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

@jtimon jtimon deleted the b20-chain-constants branch October 30, 2019 12:50
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@maflcko maflcko maflcko left review comments

@fjahr fjahr fjahr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jtimon @maflcko @DrahtBot @practicalswift @laanwj @fjahr @hebasto @fanquake