[refactor] Config handling refactoring in preparation for network-specific sections #12878

ajtowns commented at 5:39 AM on April 4, 2018: member

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.

fanquake added the label Refactoring on Apr 4, 2018

in src/test/util_tests.cpp:363 in 0068900bf2 outdated

 324 | +               );
 325 | +    BOOST_CHECK(1
 326 | +                && test_args.GetArg("-h", "xxx") == "1" ); // 1st value takes precedence
 327 | +    BOOST_CHECK(1
 328 | +                && test_args.GetArg("-i", "xxx") == "0" ); // 1st value takes precedence
 329 | +

ajtowns commented at 7:22 AM on April 4, 2018:

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

ajtowns force-pushed on Apr 4, 2018

in src/util.h:312 in 7edc29f0ce outdated

 306 | @@ -305,6 +307,12 @@ class ArgsManager
 307 |      // been set. Also called directly in testing.
 308 |      void ForceSetArg(const std::string& strArg, const std::string& strValue);
 309 |  
 310 | +    /**
 311 | +     * Looks for -regtest, -testnet and returns the appropriate BIP70 chain name.
 312 | +     * @return CBaseChainParams::MAX_NETWORK_TYPES if an invalid combination is given. CBaseChainParams::MAIN by default.

jnewbery commented at 2:22 PM on April 4, 2018:

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

in src/util.cpp:740 in 7edc29f0ce outdated

 739 | +void ArgsManager::ReadConfigStream(std::istream& stream)
 740 |  {
 741 | -    fs::ifstream streamConfig(GetConfigFile(confPath));
 742 | -    if (!streamConfig.good())
 743 | -        return; // No bitcoin.conf file is OK
 744 | +    // assert(streamConfig.good());

jnewbery commented at 2:37 PM on April 4, 2018:

remove comment?

in src/test/util_tests.cpp:319 in 7edc29f0ce outdated

 316 | +       "ccc=argument\n"
 317 | +       "ccc=multiple\n"
 318 | +       "d=e\n"
 319 | +       "nofff=1\n"
 320 | +       "noggg=0\n"
 321 | +       "h=1\n"    // negated edge cases in config behave very oddly

jnewbery commented at 2:42 PM on April 4, 2018:

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

in src/test/util_tests.cpp:327 in 7edc29f0ce outdated

 324 | +       "i=1\n";
 325 | +
 326 | +    TestArgsManager test_args;
 327 | +
 328 | +    test_args.ReadConfigString(str_config);
 329 | +    // expectation: a, b, ccc, d, fff, ggg end up in map

jnewbery commented at 2:43 PM on April 4, 2018:

and h and i?

in src/test/util_tests.cpp:364 in 7edc29f0ce outdated

 361 | +                && test_args.GetArg("-h", "xxx") == "1" // 1st value takes precedence
 362 | +                && test_args.GetArg("-i", "xxx") == "0" // 1st value takes precedence
 363 | +                && test_args.GetArg("-zzz", "xxx") == "xxx"
 364 | +               );
 365 | +
 366 | +    for (int i = 0; i < 2; i++) {

jnewbery commented at 2:55 PM on April 4, 2018:

I think this is clearer in C++11:

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

ajtowns commented at 11:19 PM on April 4, 2018:

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

in src/test/util_tests.cpp:390 in 7edc29f0ce outdated

 383 | +                && test_args.GetArgs("-ccc").front() == "argument"
 384 | +                && test_args.GetArgs("-ccc").back() == "multiple");
 385 | +    BOOST_CHECK(test_args.GetArgs("-fff").size() == 1
 386 | +                && test_args.GetArgs("-fff").front() == "0");
 387 | +    BOOST_CHECK(test_args.GetArgs("-nofff").size() == 0);
 388 | +    BOOST_CHECK(test_args.GetArgs("-h").size() == 2

jnewbery commented at 3:02 PM on April 4, 2018:

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

in src/test/util_tests.cpp:405 in 7edc29f0ce outdated

 398 | +    BOOST_CHECK(!test_args.IsArgNegated("-a"));
 399 | +    BOOST_CHECK(!test_args.IsArgNegated("-b"));
 400 | +    BOOST_CHECK(!test_args.IsArgNegated("-ccc"));
 401 | +    BOOST_CHECK(!test_args.IsArgNegated("-d"));
 402 | +    BOOST_CHECK(test_args.IsArgNegated("-fff"));
 403 | +    BOOST_CHECK(test_args.IsArgNegated("-ggg")); // IsArgNegated==true when noggg=0

jnewbery commented at 3:03 PM on April 4, 2018:

IsArgNegated==true when noggg=0

yuck. Can we change that?

ajtowns commented at 9:59 PM on April 4, 2018:

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.

jnewbery commented at 2:56 PM on April 5, 2018:

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

jnewbery commented at 3:07 PM on April 4, 2018: member

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

ajtowns force-pushed on Apr 4, 2018

ajtowns commented at 11:52 PM on April 4, 2018: member

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 3467454beff8e7b2df87f9a0e29391eb14a8e6c3 on the -track branch:

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

in src/util.cpp:773 in 26d3faaa28 outdated

 785 |      if (!fs::is_directory(GetDataDir(false))) {
 786 |          throw std::runtime_error(strprintf("specified data directory \"%s\" does not exist.", gArgs.GetArg("-datadir", "").c_str()));
 787 |      }
 788 |  }
 789 |  
 790 | +std::string ArgsManager::ChainNameFromCommandLine() const

Empact commented at 1:34 AM on April 5, 2018:

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

ajtowns commented at 1:51 AM on April 5, 2018:

:+1: Probably GetChainName() would be slightly more consistent...

jnewbery commented at 2:57 PM on April 5, 2018:

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

jnewbery commented at 2:57 PM on April 5, 2018: member

utACK 26d3faaa28c794b48df2a12cdb4a9a71de3d1a96

Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName 11b6b5b86e

[tests] Add unit tests for GetChainName 834d303415

Separate out ReadConfigStream from ReadConfigFile 6d5815aad0

ReadConfigStream: assume the stream is good 087c5d2040

[tests] Add unit tests for ReadConfigStream fa27f1c23e

[tests] Check GetChainName works with config entries af173c2bec

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

ajtowns force-pushed on Apr 5, 2018

ajtowns commented at 7:00 PM on April 5, 2018: member

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: https://github.com/ajtowns/bitcoin/commit/492bbcdc558ae0edae0e2beb49a50932c8d576c6 follows on from where the tracking branch left off, and matches this branches current head, 77a733a99a75bdd04ad94df830e5bdc8a6040959:

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

laanwj added this to the "Blockers" column in a project

jtimon commented at 7:49 PM on April 5, 2018: contributor

utACK 77a733a99a75bdd04ad94df830e5bdc8a6040959

in src/util.cpp:740 in 087c5d2040 outdated

 736 | @@ -737,31 +737,31 @@ fs::path GetConfigFile(const std::string& confPath)
 737 |  
 738 |  void ArgsManager::ReadConfigStream(std::istream& stream)
 739 |  {
 740 | -    if (!stream.good())

theuni commented at 7:50 PM on April 5, 2018:

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

theuni commented at 7:52 PM on April 5, 2018: member

utACK 77a733a99a75bdd04ad94df830e5bdc8a6040959, not too concerned about the nit.

jnewbery commented at 7:53 PM on April 5, 2018: member

ACK 77a733a99a75bdd04ad94df830e5bdc8a6040959

meshcollider commented at 11:03 PM on April 5, 2018: contributor

Concept ACK, this should also help my arg refactor a bit EDIT: utACK https://github.com/bitcoin/bitcoin/pull/12878/commits/77a733a99a75bdd04ad94df830e5bdc8a6040959

jimpo commented at 10:03 PM on April 6, 2018: contributor

utACK 77a733a

jonasschnelli commented at 2:23 PM on April 8, 2018: contributor

utACK 77a733a99a75bdd04ad94df830e5bdc8a6040959

jonasschnelli merged this on Apr 8, 2018

jonasschnelli closed this on Apr 8, 2018

jonasschnelli referenced this in commit 25c56cdbe7 on Apr 8, 2018

MarcoFalke removed this from the "Blockers" column in a project

in src/test/util_tests.cpp:270 in 77a733a99a

 268 | -    BOOST_CHECK(testArgs.GetBoolArg("-foo", false) == true);
 269 | +    BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "");
 270 |  
 271 |      // A double negative is a positive.
 272 |      BOOST_CHECK(testArgs.IsArgNegated("-bar"));
 273 | -    BOOST_CHECK(testArgs.GetBoolArg("-bar", false) == true);

MarcoFalke commented at 9:10 PM on April 8, 2018:

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

ajtowns commented at 4:55 AM on April 9, 2018:

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

MarcoFalke commented at 9:10 PM on April 8, 2018: member

post merge utACK 77a733a beside question

UdjinM6 referenced this in commit 33f45f0f2c on Apr 29, 2020

PastaPastaPasta referenced this in commit 6ac5823423 on Apr 30, 2020

PastaPastaPasta referenced this in commit aaf5bf1a7e on May 9, 2020

ckti referenced this in commit 9d81d10456 on Mar 28, 2021

furszy referenced this in commit 0255df35a6 on Apr 26, 2021

gades referenced this in commit 4478a6e677 on Jun 30, 2021

MarcoFalke locked this on Sep 8, 2021