[refactor] Config handling refactoring in preparation for network-specific sections by ajtowns · Pull Request #12878 · bitcoin/bitcoin

ajtowns · 2018-04-04T05:39:28Z

This does a bit of refactoring of the configuration handling code in order to add additional tests to make adding support for [test]/[regtest] sections in the config file in #11862 easier. Should not cause any behaviour changes.

ajtowns · 2018-04-04T07:22:37Z

src/test/util_tests.cpp

+                && test_args.GetArg("-h", "xxx") == "1" ); // 1st value takes precedence
+    BOOST_CHECK(1
+                && test_args.GetArg("-i", "xxx") == "0" ); // 1st value takes precedence
+


Oops, these probably should be merged back into the previous check

jnewbery

Looks really good. Thanks for all the additional test coverage!

jnewbery · 2018-04-04T14:22:26Z

src/util.h


+    /**
+     * Looks for -regtest, -testnet and returns the appropriate BIP70 chain name.
+     * @return CBaseChainParams::MAX_NETWORK_TYPES if an invalid combination is given. CBaseChainParams::MAIN by default.


This comment is wrong (since 55a8975 when MAX_NETWORK_TYPES was removed). May as well remove it now that you're moving the function.

jnewbery · 2018-04-04T14:37:01Z

src/util.cpp

-    fs::ifstream streamConfig(GetConfigFile(confPath));
-    if (!streamConfig.good())
-        return; // No bitcoin.conf file is OK
+    // assert(streamConfig.good());


remove comment?

jnewbery · 2018-04-04T14:42:10Z

src/test/util_tests.cpp

+       "d=e\n"
+       "nofff=1\n"
+       "noggg=0\n"
+       "h=1\n"    // negated edge cases in config behave very oddly


I don't understand this comment. Can you make it more explicit about what you mean, or remove?

jnewbery · 2018-04-04T14:43:44Z

src/test/util_tests.cpp

+    TestArgsManager test_args;
+
+    test_args.ReadConfigString(str_config);
+    // expectation: a, b, ccc, d, fff, ggg end up in map


and h and i?

jnewbery · 2018-04-04T14:55:25Z

src/test/util_tests.cpp

+                && test_args.GetArg("-zzz", "xxx") == "xxx"
+               );
+
+    for (int i = 0; i < 2; i++) {


I think this is clearer in C++11:

for (auto def : { false, true }) {

Yeah, it is. But no point to auto instead of bool :)

jnewbery · 2018-04-04T15:02:16Z

src/test/util_tests.cpp

+    BOOST_CHECK(test_args.GetArgs("-fff").size() == 1
+                && test_args.GetArgs("-fff").front() == "0");
+    BOOST_CHECK(test_args.GetArgs("-nofff").size() == 0);
+    BOOST_CHECK(test_args.GetArgs("-h").size() == 2


Why no: BOOST_CHECK(test_args.GetArgs("-ggg").size() == 1); ?

jnewbery · 2018-04-04T15:03:10Z

src/test/util_tests.cpp

+    BOOST_CHECK(!test_args.IsArgNegated("-ccc"));
+    BOOST_CHECK(!test_args.IsArgNegated("-d"));
+    BOOST_CHECK(test_args.IsArgNegated("-fff"));
+    BOOST_CHECK(test_args.IsArgNegated("-ggg")); // IsArgNegated==true when noggg=0


IsArgNegated==true when noggg=0

yuck. Can we change that?

It's changed in #11862 (and easy to change on its own as well). I think it makes sense for this PR to just refactor/add tests rather than change functionality though.

sounds good. Keep this PR as refactor only and do behaviour changes in #11862.

ajtowns · 2018-04-04T23:52:01Z

Bumped to address @jnewbery's suggestions. Unsquashed changes are at https://github.com/ajtowns/bitcoin/commits/netconf-refactor-track with commit 26d3faaa28c794b48df2a12cdb4a9a71de3d1a96 here having the same tree as commit 3467454 on the -track branch:

$ for a in netconf-refactor{,-track}; do git log -1 --pretty="%h -> %T" $a; done
26d3faaa28 -> ef5b209a05c28afb98e0ce8e05e9cbc54dfebeaf
3467454bef -> ef5b209a05c28afb98e0ce8e05e9cbc54dfebeaf

Empact · 2018-04-05T01:34:05Z

src/util.cpp

Could just call this ChainName given gArgs is all about the command line / config files already.

👍 Probably GetChainName() would be slightly more consistent...

Agree that GetChainName() is a better name. Can be done in this PR or a future PR.

jnewbery · 2018-04-05T14:57:44Z

utACK 26d3faaa28c794b48df2a12cdb4a9a71de3d1a96

…Name

ajtowns · 2018-04-05T19:00:45Z

Rebased due to conflict with #10244, and renamed ChainNameFromCommandLine to GetChainName. Not sure how useful the -track branch is since it's now got a merge with master, but it still works: ajtowns@492bbcd follows on from where the tracking branch left off, and matches this branches current head, 77a733a:

$ for a in netconf-refactor{,-track}; do git log -1 --pretty="%h -> %T" $a; done
77a733a99a -> 4b6b60ed983260f4f3219ad28825d209ff5a7957
492bbcdc55 -> 4b6b60ed983260f4f3219ad28825d209ff5a7957

jtimon · 2018-04-05T19:49:30Z

utACK 77a733a

theuni · 2018-04-05T19:50:15Z

src/util.cpp


 void ArgsManager::ReadConfigStream(std::istream& stream)
 {
-    if (!stream.good())


Replace this with an assertion instead, even if just to self-document assumptions?

theuni · 2018-04-05T19:52:58Z

utACK 77a733a, not too concerned about the nit.

jnewbery · 2018-04-05T19:53:53Z

ACK 77a733a

meshcollider · 2018-04-05T23:03:30Z

Concept ACK, this should also help my arg refactor a bit
EDIT: utACK 77a733a

jimpo · 2018-04-06T22:03:47Z

utACK 77a733a

jonasschnelli · 2018-04-08T14:23:25Z

utACK 77a733a

…or network-specific sections 77a733a [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) af173c2 [tests] Check GetChainName works with config entries (Anthony Towns) fa27f1c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 087c5d2 ReadConfigStream: assume the stream is good (Anthony Towns) 6d5815a Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) 834d303 [tests] Add unit tests for GetChainName (Anthony Towns) 11b6b5b Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName (Anthony Towns) Pull request description: This does a bit of refactoring of the configuration handling code in order to add additional tests to make adding support for [test]/[regtest] sections in the config file in #11862 easier. Should not cause any behaviour changes. Tree-SHA512: 8d2ce1449fc180de03414e7e569d1a21ba1e9f6564e13d3faf3961f710adc725fa0d4ab49b89ebd2baa11ea36ac5018377f693a84037d386a8b8697c9d6db3e9

maflcko · 2018-04-08T21:10:20Z

src/test/util_tests.cpp


    // A double negative is a positive.
    BOOST_CHECK(testArgs.IsArgNegated("-bar"));
-    BOOST_CHECK(testArgs.GetBoolArg("-bar", false) == true);


In the test case util_GetBoolArgEdgeCases you seem to be removing all calls to GetBoolArg. Is this intentional?

Yeah; I think the edge cases are more clearly tested with GetArg and a clearly distinct default value. The IsArgNegated/GetBoolArg cases are still tested in util_GetBoolArg (via -a and -b).

maflcko · 2018-04-08T21:10:58Z

post merge utACK 77a733a beside question

…ation for network-specific sections 77a733a [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) af173c2 [tests] Check GetChainName works with config entries (Anthony Towns) fa27f1c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 087c5d2 ReadConfigStream: assume the stream is good (Anthony Towns) 6d5815a Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) 834d303 [tests] Add unit tests for GetChainName (Anthony Towns) 11b6b5b Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName (Anthony Towns) Pull request description: This does a bit of refactoring of the configuration handling code in order to add additional tests to make adding support for [test]/[regtest] sections in the config file in bitcoin#11862 easier. Should not cause any behaviour changes. Tree-SHA512: 8d2ce1449fc180de03414e7e569d1a21ba1e9f6564e13d3faf3961f710adc725fa0d4ab49b89ebd2baa11ea36ac5018377f693a84037d386a8b8697c9d6db3e9

8817d76 Add config changes to release notes (random-zebra) 988d428 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns) 7b72630 ArgsManager: special handling for -regtest and -testnet (Anthony Towns) 51c4aa1 [tests] Unit tests for network-specific config entries (Anthony Towns) 30f3bab ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns) 1bddffe ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns) ef505ea [Tests] Fix and re-activate rpc_users.py functional test (random-zebra) 0eacce7 [RPC] Add rpcauth startup flag for multiple RPC users (random-zebra) e79ff3c [Tests] Fix and re-activate feature_config_args functional test (random-zebra) 3fcaaa4 Replace cookie auth in tests (random-zebra) b907f33 rpc: Generate auth cookie in hex instead of base64 (random-zebra) bf443a9 [tests] Use regtest section in functional tests configs (Anthony Towns) 7857bcd [tests] Unit tests for config file sections (Anthony Towns) 4cf2ee6 ArgsManager: support config file sections (Anthony Towns) 2658771 ArgsManager: drop m_negated_args (Anthony Towns) 4ee69b5 ArgsManager: keep command line and config file arguments separate (random-zebra) 4f416b8 [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) 5d1200b [tests] Check GetChainName works with config entries (Anthony Towns) 157da7c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 23a4633 ReadConfigStream: assume the stream is good (Anthony Towns) 56ea59e Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) a3438a2 Test datadir in conf file exists (MeshCollider) 459c4ad [tests] Add unit tests for GetChainName (Anthony Towns) 0ab3e99 Move ChainNameFromCommandLine into ArgsManager, rename to GetChainName (Anthony Towns) 1ea7ce6 Globals: Decouple GetConfigFile and ReadConfigFile from global mapArgs (random-zebra) Pull request description: It is now possible for a single configuration file to set different options for different networks. This is done by using sections or by prefixing the option with the network, such as: ``` main.uacomment=pivx test.uacomment=pivx-testnet regtest.uacomment=regtest [main] mempoolsize=300 [test] mempoolsize=100 [regtest] mempoolsize=20 ``` The `addnode=`, `connect=`, `port=`, `bind=`, `rpcport=`, `rpcbind=`, and `wallet=` options will only apply to mainnet when specified in the configuration file, unless a network is specified. Also - fix cookie-based authentication for the functional tests, and re-enable `feature_config_args.py` - add `-rpcauth` startup flag, for multiple RPC users, and re-enable `rpc_users.py`. Backports relevant commits from: - bitcoin#8856 Globals: Decouple GetConfigFile and ReadConfigFile from global mapArgs - bitcoin#11829 Test datadir specified in conf file exists - bitcoin#12878 Config handling refactoring in preparation for network-specific sections - bitcoin#11862 Network specific conf sections - bitcoin#10533 [tests] Use cookie auth instead of rpcuser and rpcpassword - bitcoin#8858 Generate auth cookie in hex instead of base64 ACKs for top commit: Fuzzbawls: Tested ACK 8817d76 furszy: utACK 8817d76 Tree-SHA512: 0d23dbf3d254b186a5378576601cf43f8322abe036b0b135a39b22e13ffa2e299cb1323160d87c7d8284e6aaa229c47f54c8a3a3ebf07cc298e644fb8bd69dc0

…ation for network-specific sections 77a733a [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) af173c2 [tests] Check GetChainName works with config entries (Anthony Towns) fa27f1c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 087c5d2 ReadConfigStream: assume the stream is good (Anthony Towns) 6d5815a Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) 834d303 [tests] Add unit tests for GetChainName (Anthony Towns) 11b6b5b Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName (Anthony Towns) Pull request description: This does a bit of refactoring of the configuration handling code in order to add additional tests to make adding support for [test]/[regtest] sections in the config file in bitcoin#11862 easier. Should not cause any behaviour changes. Tree-SHA512: 8d2ce1449fc180de03414e7e569d1a21ba1e9f6564e13d3faf3961f710adc725fa0d4ab49b89ebd2baa11ea36ac5018377f693a84037d386a8b8697c9d6db3e9

fanquake added the Refactoring label Apr 4, 2018

ajtowns commented Apr 4, 2018

View reviewed changes

ajtowns force-pushed the netconf-refactor branch from 4bf826f to 7edc29f Compare April 4, 2018 07:48

ajtowns mentioned this pull request Apr 4, 2018

Network specific conf sections #11862

Merged

jnewbery reviewed Apr 4, 2018

View reviewed changes

ajtowns force-pushed the netconf-refactor branch from 7edc29f to 26d3faa Compare April 4, 2018 23:47

ajtowns mentioned this pull request Apr 5, 2018

Test ReadConfigFile #12812

Closed

Empact reviewed Apr 5, 2018

View reviewed changes

Empact mentioned this pull request Apr 5, 2018

Store the current config file at read time, and return it from GetConfigFile if set #12765

Closed

ajtowns added 7 commits April 6, 2018 04:46

Move ChainNameFromCommandLine into ArgsManager and rename to GetChain…

11b6b5b

…Name

[tests] Add unit tests for GetChainName

834d303

Separate out ReadConfigStream from ReadConfigFile

6d5815a

ReadConfigStream: assume the stream is good

087c5d2

[tests] Add unit tests for ReadConfigStream

fa27f1c

[tests] Check GetChainName works with config entries

af173c2

[tests] Add additional unit tests for -nofoo edge cases

77a733a

ajtowns force-pushed the netconf-refactor branch from 26d3faa to 77a733a Compare April 5, 2018 18:50

theuni reviewed Apr 5, 2018

View reviewed changes

jonasschnelli merged commit 77a733a into bitcoin:master Apr 8, 2018

maflcko reviewed Apr 8, 2018

View reviewed changes

random-zebra mentioned this pull request Apr 19, 2021

[Core] Network specific configuration sections PIVX-Project/PIVX#2324

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

ajtowns commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 5, 2018

Uh oh!

ajtowns commented Apr 5, 2018

Uh oh!

jtimon commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Apr 5, 2018

Uh oh!

jnewbery commented Apr 5, 2018

Uh oh!

meshcollider commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimpo commented Apr 6, 2018

Uh oh!

jonasschnelli commented Apr 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

meshcollider commented Apr 5, 2018 •

edited

Loading