More verbose warning for multiple network argument error. #26028

amovfx · 2022-09-07T02:58:15Z

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 · 2022-09-07T07:37:48Z

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

amovfx · 2022-09-08T00:23:34Z

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 · 2022-09-08T17:29:25Z

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

fanquake · 2022-09-09T09:49:28Z

src/util/system.cpp

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


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

luke-jr · 2022-09-10T23:25:01Z

src/util/system.cpp

    }
    if (fRegTest)
        return CBaseChainParams::REGTEST;
-    if (fSigNet) {
+    if (fSigNet)


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

eg

Suggested change

if (fSigNet)

if (fSigNet) return CBaseChainParams::SIGNET;

amovfx

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 · 2022-09-21T15:17:26Z

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 · 2022-09-23T04:07:25Z

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.

jonatack · 2022-09-23T08:25:53Z

src/util/system.cpp

@@ -1072,16 +1072,11 @@ std::string ArgsManager::GetChainName() const
    const bool is_chain_arg_set = IsArgSet("-chain");

    if ((int)is_chain_arg_set + (int)fRegTest + (int)fSigNet + (int)fTestNet > 1) {
-        throw std::runtime_error("Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.");
+        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.");


Suggested change

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

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 · 2022-09-23T08:30:27Z

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 · 2022-11-15T21:00:38Z

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

fanquake · 2023-02-06T16:26:00Z

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

maflcko · 2023-04-04T20:07:40Z

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

fanquake mentioned this pull request Sep 7, 2022

Chain Argument collision warning #26027

Closed

amovfx force-pushed the network-argument-collision-verbosity branch 6 times, most recently from 5b4f6c7 to 4b0de90 Compare September 7, 2022 19:04

amovfx force-pushed the network-argument-collision-verbosity branch 2 times, most recently from 8ed8ac3 to a73caa9 Compare September 8, 2022 16:55

amovfx marked this pull request as ready for review September 8, 2022 18:39

fanquake reviewed Sep 9, 2022

View reviewed changes

luke-jr reviewed Sep 10, 2022

View reviewed changes

amovfx commented Sep 13, 2022

View reviewed changes

amovfx force-pushed the network-argument-collision-verbosity branch from a73caa9 to 17df428 Compare September 13, 2022 00:04

More verbose warning for multiple network argument error.

90f5c53

amovfx force-pushed the network-argument-collision-verbosity branch from 17df428 to 90f5c53 Compare September 20, 2022 17:17

maflcko approved these changes Sep 20, 2022

View reviewed changes

jonatack reviewed Sep 23, 2022

View reviewed changes

DrahtBot mentioned this pull request Oct 11, 2022

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

DrahtBot mentioned this pull request Nov 11, 2022

test: Split overly large util_tests.cpp file #26489

Merged

DrahtBot added the Needs rebase label Nov 15, 2022

fanquake marked this pull request as draft February 16, 2023 12:30

maflcko closed this Apr 4, 2023

bitcoin locked and limited conversation to collaborators Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

More verbose warning for multiple network argument error. #26028

More verbose warning for multiple network argument error. #26028

Uh oh!

amovfx commented Sep 7, 2022 •

edited

Loading

Uh oh!

maflcko commented Sep 7, 2022

Uh oh!

amovfx commented Sep 8, 2022

Uh oh!

amovfx commented Sep 8, 2022

Uh oh!

fanquake Sep 9, 2022

Uh oh!

luke-jr Sep 10, 2022

Uh oh!

amovfx left a comment •

edited

Loading

Uh oh!

amovfx commented Sep 21, 2022

Uh oh!

DrahtBot commented Sep 23, 2022 •

edited

Loading

Uh oh!

jonatack Sep 23, 2022

Uh oh!

jonatack commented Sep 23, 2022

Uh oh!

DrahtBot commented Nov 15, 2022

Uh oh!

fanquake commented Feb 6, 2023

Uh oh!

maflcko commented Apr 4, 2023

Uh oh!

Uh oh!

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

More verbose warning for multiple network argument error. #26028

More verbose warning for multiple network argument error. #26028

Uh oh!

Conversation

amovfx commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 7, 2022

Uh oh!

amovfx commented Sep 8, 2022

Uh oh!

amovfx commented Sep 8, 2022

Uh oh!

fanquake Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

luke-jr Sep 10, 2022

Choose a reason for hiding this comment

Uh oh!

amovfx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amovfx commented Sep 21, 2022

Uh oh!

DrahtBot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

jonatack Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack commented Sep 23, 2022

Uh oh!

DrahtBot commented Nov 15, 2022

Uh oh!

fanquake commented Feb 6, 2023

Uh oh!

maflcko commented Apr 4, 2023

Uh oh!

Uh oh!

amovfx commented Sep 7, 2022 •

edited

Loading

amovfx left a comment •

edited

Loading

DrahtBot commented Sep 23, 2022 •

edited

Loading