util: Disallow network-qualified command line options #17482

ryanofsky · 2019-11-14T20:34:03Z

Previously these were allowed but ignored.

This change implements one of the settings simplifications listed in #17508. Change includes release notes.

ryanofsky · 2019-11-14T20:41:43Z

src/test/util_tests.cpp

-    BOOST_CHECK_EQUAL(out_sha_hex, "b835eef5977d69114eb039a976201f8c7121f34fe2b7ea2b73cafb516e5c9dc8");
+    BOOST_CHECK_EQUAL(out_sha_hex, "8fd4877bb8bf337badca950ede6c917441901962f160e52514e06a60dea46cde");


In commit "util: Disallow network-qualified command line options" (baf24fc)

Effect of this change is to remove a lot of lines that contain network-qualified command line options from test output:

-net=test -main.server=a1 -main.server=a2 noserver=1 main.server=c1 main.server=c2 server=c3 server=c4 soft || c3 | c3 c4

Lines without network-qualified command line options are unaffected:

net=test -server=a1 -server=a2 main.server=c1 main.server=c2 noserver=1 server=c3 server=c4 soft || a2 | a1 a2 c3 c4

DrahtBot · 2019-11-14T21:56:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

promag

Concept ACK, seems fine to do this.

src/util/system.cpp

promag · 2019-11-14T22:05:29Z

src/test/util_tests.cpp

+
+    const char* argv[] = {"ignored", "-registered"};
+    std::string error;
+    BOOST_CHECK(test.ParseParameters(2, (char**)argv, error));


nit, success case seems unnecessary.

re: #17482 (comment)

nit, success case seems unnecessary.

It helps ensure the test passes for right reason. That ParseParameters calls below aren't returning false for some other reason. Generally it's nice when test cases vary one input at a time, so you can be sure about what causes the observed output.

promag · 2019-11-14T22:07:05Z

src/util/system.cpp

-            // error below.
-            if (section.empty()) {
-                m_settings.command_line_options[key].push_back(value);
-            }


Previously a warning could make sense.

laanwj · 2019-11-15T09:48:19Z

Being explicit on invalid arguments is always better. And I don't see any reason to implement them, so Concept ACK.

ariard

Code review and tested ACK 124841f

Tested network-qualified command line args with/without this patchset and errors are now thrown as expected.

ryanofsky

Updated 124841f -> 5e4ef31 (pr/wdqual.2 -> pr/wdqual.3, compare) just adding suggested comment

ryanofsky · 2019-11-15T13:28:23Z

src/test/util_tests.cpp

+
+    const char* argv[] = {"ignored", "-registered"};
+    std::string error;
+    BOOST_CHECK(test.ParseParameters(2, (char**)argv, error));


re: #17482 (comment)

nit, success case seems unnecessary.

It helps ensure the test passes for right reason. That ParseParameters calls below aren't returning false for some other reason. Generally it's nice when test cases vary one input at a time, so you can be sure about what causes the observed output.

src/util/system.cpp

Previously these were allowed but ignored.

ajtowns · 2020-01-09T02:38:20Z

Concept ACK

laanwj · 2020-02-05T14:53:20Z

ACK 900d8f6

900d8f6 util: Disallow network-qualified command line options (Russell Yanofsky) Pull request description: Previously these were allowed but ignored. This change implements one of the settings simplifications listed in #17508. Change includes release notes. ACKs for top commit: laanwj: ACK 900d8f6 Tree-SHA512: ab020a16a86c1e8ec709fbf798d533879d32c565eceeb7eb785c33042c49c6b4d1108c5453d8166e4a2abffc2c8802fbb6d3b895e0ddeefa8f274fd647e3c8ad

…tions 900d8f6 util: Disallow network-qualified command line options (Russell Yanofsky) Pull request description: Previously these were allowed but ignored. This change implements one of the settings simplifications listed in bitcoin#17508. Change includes release notes. ACKs for top commit: laanwj: ACK 900d8f6 Tree-SHA512: ab020a16a86c1e8ec709fbf798d533879d32c565eceeb7eb785c33042c49c6b4d1108c5453d8166e4a2abffc2c8802fbb6d3b895e0ddeefa8f274fd647e3c8ad

Summary: > Command line options prefixed with main/test/regtest network names like -main.port=8333 -test.server=1 previously were allowed but ignored. Now they trigger "Invalid parameter" errors on startup. This is a backport of Core [[bitcoin/bitcoin#17482 | PR17482]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8739

ryanofsky commented Nov 14, 2019

View reviewed changes

ryanofsky force-pushed the pr/wdqual branch from baf24fc to 124841f Compare November 14, 2019 20:48

DrahtBot added Docs Tests Utils/log/libs labels Nov 14, 2019

fanquake removed the Docs label Nov 14, 2019

promag reviewed Nov 14, 2019

View reviewed changes

ryanofsky mentioned this pull request Nov 18, 2019

Simpler settings interpretation #17508

Open

ariard approved these changes Nov 19, 2019

View reviewed changes

ryanofsky force-pushed the pr/wdqual branch from 124841f to 5e4ef31 Compare November 21, 2019 22:43

ryanofsky commented Nov 21, 2019

View reviewed changes

DrahtBot added the Needs rebase label Dec 19, 2019

util: Disallow network-qualified command line options

900d8f6

Previously these were allowed but ignored.

ryanofsky force-pushed the pr/wdqual branch from 5e4ef31 to 900d8f6 Compare December 19, 2019 21:29

DrahtBot removed the Needs rebase label Dec 19, 2019

laanwj merged commit 900d8f6 into bitcoin:master Feb 5, 2020

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 Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

util: Disallow network-qualified command line options #17482

util: Disallow network-qualified command line options #17482

Uh oh!

ryanofsky commented Nov 14, 2019 •

edited

Loading

Uh oh!

ryanofsky Nov 14, 2019 •

edited

Loading

Uh oh!

DrahtBot commented Nov 14, 2019 •

edited

Loading

Uh oh!

promag left a comment

Uh oh!

Uh oh!

promag Nov 14, 2019

Uh oh!

ryanofsky Nov 15, 2019

Uh oh!

promag Nov 14, 2019

Uh oh!

laanwj commented Nov 15, 2019 •

edited

Loading

Uh oh!

ariard left a comment

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Nov 15, 2019

Uh oh!

Uh oh!

ajtowns commented Jan 9, 2020

Uh oh!

laanwj commented Feb 5, 2020

Uh oh!

Uh oh!

		BOOST_CHECK_EQUAL(out_sha_hex, "b835eef5977d69114eb039a976201f8c7121f34fe2b7ea2b73cafb516e5c9dc8");
		BOOST_CHECK_EQUAL(out_sha_hex, "8fd4877bb8bf337badca950ede6c917441901962f160e52514e06a60dea46cde");

util: Disallow network-qualified command line options #17482

util: Disallow network-qualified command line options #17482

Uh oh!

Conversation

ryanofsky commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky Nov 14, 2019 • 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!

DrahtBot commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

promag Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajtowns commented Jan 9, 2020

Uh oh!

laanwj commented Feb 5, 2020

Uh oh!

Uh oh!

ryanofsky commented Nov 14, 2019 •

edited

Loading

ryanofsky Nov 14, 2019 •

edited

Loading

DrahtBot commented Nov 14, 2019 •

edited

Loading

laanwj commented Nov 15, 2019 •

edited

Loading