More verbose warning for multiple network argument error. #26028

amovfx commented at 2:58 AM on September 7, 2022: none

This PR has bitcoin print a more verbose message when a network argument collisions happens. I'm new to using the bitcoin software and was reconfiguring an old node. I also have limited experience at coding, this is my first PR on an open-source project, so any feedback on how to improve this process is welcomed.

If a chain argument is in the bitcoin.conf and then a bitcoin command has a network flag or a chain argument, such as : bitcoind -regtest currently you are presented with the message:

"Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one."

As I am a new to this software, with not knowing that the bitcoin.conf was being merge in the command line, I was at a loss until I got guidance in the bitcoin-core-dev irc.

In this PR, if a user makes this error, they will be notified to check their conf files.

The new error is a bit more verbose for the command bitcoind -regtest if the chain argument is set in the command line.

"Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one. Please check if chain is specified in config file."

In regards to the unit test util_tests/util_ChainMerge:

I had to write a script to check if the resulting output of master branch was the same output as the test from my branch and then update the hash accordingly. It is a simple compare of two txt files.

You can download the scripts here: https://github.com/amovfx/btc-argument-collision-warning.git

I understand that your time is valuable and this isn't a significant change but I thought it was something within my ability I could contribute. Thank you for your time. Also, thanks to everyone that helped me get to this point to even submit a PR. Jonas at chain code, the blockchains commons project, programming bitcoin by Jimmy Song, Achow101's twitch stream, the many hosts of the bitcoin-pr-review club and the devs in the bitcoin-core-dev and bitcoin stack exchange answering my questions.

maflcko commented at 7:37 AM on September 7, 2022: member

What about just modifying the error string to encourage the user to re-check the command line and config file?

amovfx force-pushed on Sep 7, 2022

amovfx commented at 12:23 AM on September 8, 2022: none

Hi @MarcoFalke, Well that would be simpler for sure. I am open to that if you think it is a good idea. Perhaps I went overboard. I can modify that string, ditch the feature test and make sure the unit test still works.

I'm trying to get past this listing error on python -utf8. If you have any insights that would be appreciated.

amovfx force-pushed on Sep 8, 2022

amovfx commented at 5:29 PM on September 8, 2022: none

@MarcoFalke I made your suggested changes, doesn't need the additional functional test. LInting error solved.

amovfx marked this as ready for review on Sep 8, 2022

in src/util/system.cpp:1064 in a73caa9bc6 outdated

1060 | @@ -1061,7 +1061,7 @@ std::string ArgsManager::GetChainName() const
1061 |          LOCK(cs_args);
1062 |          util::SettingsValue value = util::GetSetting(m_settings, /* section= */ "", SettingName(arg),
1063 |              /* ignore_default_section_config= */ false,
1064 | -            /*ignore_nonpersistent=*/false,
1065 | +            /* ignore_nonpersistent= */ false,

fanquake commented at 9:49 AM on September 9, 2022:

You can remove this, and the rest of the unrelated code changes.

in src/util/system.cpp:1079 in a73caa9bc6 outdated

1076 | +        throw std::runtime_error("Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one. Please check if chain is specified in config file.");
1077 |      }
1078 |      if (fRegTest)
1079 |          return CBaseChainParams::REGTEST;
1080 | -    if (fSigNet) {
1081 | +    if (fSigNet)

luke-jr commented at 11:25 PM on September 10, 2022:

This is a regression in coding style. If you want to update these, they should probably all be one liners.

eg

    if (fSigNet) return CBaseChainParams::SIGNET;

amovfx commented at 12:03 AM on September 13, 2022: none

Thanks @luke-jr @MarcoFalke I'm not sure the protocol for submitting suggested changes. I'm squashing the commit down. Or would it be better to have the suggested changes into a separate commit for better transparency for future reviewers?

amovfx force-pushed on Sep 13, 2022

More verbose warning for multiple network argument error. 90f5c531da

amovfx force-pushed on Sep 20, 2022

maflcko approved

amovfx commented at 3:17 PM on September 21, 2022: none

As small of a change this was, and as difficult as it was I'm super happy to have this approved. Thank you to all the reviewers for their time.

DrahtBot commented at 4:07 AM on September 23, 2022: contributor

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26489 (test: Split overly large util_tests.cpp file by MarcoFalke)

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/util/system.cpp:1075 in 90f5c531da

1071 | @@ -1072,16 +1072,11 @@ std::string ArgsManager::GetChainName() const
1072 |      const bool is_chain_arg_set = IsArgSet("-chain");
1073 |  
1074 |      if ((int)is_chain_arg_set + (int)fRegTest + (int)fSigNet + (int)fTestNet > 1) {
1075 | -        throw std::runtime_error("Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.");
1076 | +        throw std::runtime_error("Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one. Please check if chain is specified in config file.");

jonatack commented at 8:25 AM on September 23, 2022:

        throw std::runtime_error("Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one. Please check your config options on the command line and in the bitcoin.conf file.");

jonatack commented at 8:30 AM on September 23, 2022: contributor

I'm not sure the protocol for submitting suggested changes. I'm squashing the commit down. Or would it be better to have the suggested changes into a separate commit for better transparency for future reviewers?

Yes, in this project it's generally preferred to squash the changes, if needed, into their final form. See CONTRIBUTING.md for more info.

DrahtBot commented at 9:00 PM on November 15, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Nov 15, 2022

fanquake commented at 4:26 PM on February 6, 2023: member

@amovfx are you going to follow up here? This is not currently mergable.

fanquake marked this as a draft on Feb 16, 2023

maflcko commented at 8:07 PM on April 4, 2023: member

Closing for now due to lack of activity. Let us know when the should be reopened.

maflcko closed this on Apr 4, 2023

bitcoin locked this on Apr 3, 2024