Deduplicate bitcoind and bitcoin-qt init code

ryanofsky commented at 9:25 pm on February 23, 2023: contributor

Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code reading config files and creating the datadir.

Noticed the duplicate code while reviewing #27073 and want to remove it because difference in bitcoin-qt and bitcoind behavior make it hard to evaluate changes like #27073

There are a few minor changes in behavior:

In bitcoin-qt, when there is a problem reading the configuration file, the GUI error text has changed from “Error: Cannot parse configuration file:” to “Error reading configuration file:” to be consistent with bitcoind.
In bitcoind, when there is a problem reading the settings.json file, the error text has changed from “Failed loading settings file” to “Settings file could not be read” to be consistent with bitcoin-qt.
In bitcoind, when there is a problem writing the settings.json file, the error text has changed from “Failed saving settings file” to “Settings file could not be written” to be consistent with bitcoin-qt.
In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read), there is an normal error dialog showing “Error: filesystem error: status: Permission denied […/settings.json]”, instead of an uncaught exception.

DrahtBot commented at 9:25 pm on February 23, 2023: 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.

Type	Reviewers
ACK	Sjors, TheCharlatan, achow101
Concept ACK	fanquake, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26688 (Move CopyrightHolders() and LicenseInfo() into libbitcoin_common by hebasto)
#26504 (Add UNREACHABLE macro and drop -Wreturn-type/C4715 warnings suppressions by hebasto)

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.

ryanofsky commented at 9:26 pm on February 23, 2023: contributor

Leaving as draft because #27073 could be merged first, and if it is, more duplicate code can be moved to the common function.

jarolrod commented at 3:50 am on February 24, 2023: member

concept ack

fanquake commented at 8:54 am on February 24, 2023: member

Concept ACK - #27073 has now been merged.

hebasto commented at 9:20 am on February 24, 2023: member

Concept ACK.

ryanofsky force-pushed on Feb 24, 2023

ryanofsky marked this as ready for review on Feb 24, 2023

ryanofsky commented at 7:37 pm on February 24, 2023: contributor

Marking ready for review since I was able to merge the duplicate InitSettings functions after #27073.

Rebased eeff271a693baa6fb013b6b007306d0c288c811a -> e248926c73ce6f707343501fb42b31090fed202b (pr/oneconfig.1 -> pr/oneconfig.2, compare) after #27073 merge

ryanofsky force-pushed on Feb 25, 2023

ryanofsky commented at 2:07 pm on February 25, 2023: contributor

Updated e248926c73ce6f707343501fb42b31090fed202b -> 25f9cbf7191ee80a640da5f5386aef9658f1168b (pr/oneconfig.2 -> pr/oneconfig.3, compare) to fix lint spelling error https://cirrus-ci.com/task/5488540285403136

in src/test/translation_tests.cpp:16 in 25f9cbf719 outdated

11+
12+BOOST_AUTO_TEST_CASE(translation_namedparams)
13+{
14+    bilingual_str arg{"original", "translated"};
15+    bilingual_str format{"original [%s]", "translated [%s]"};
16+    bilingual_str result{strprintf(format, arg)};

fanquake commented at 2:53 pm on February 27, 2023:

https://github.com/bitcoin/bitcoin/pull/27150/checks?check_run_id=11596638131

0src/test/translation_tests.cpp: Expected 0 argument(s) after format string but found 1 argument(s): strprintf(format, arg)

ryanofsky commented at 10:33 pm on February 27, 2023:

re: #27150 (review)

Thanks! Should be fixed now

ryanofsky force-pushed on Feb 27, 2023

ryanofsky commented at 10:34 pm on February 27, 2023: contributor

Updated 25f9cbf7191ee80a640da5f5386aef9658f1168b -> 15cb0a2492d050eb61cca5a44893a5c841b7f506 (pr/oneconfig.3 -> pr/oneconfig.4, compare) fixing lint error https://cirrus-ci.com/task/6543913792569344?logs=lint#L230

in src/common/init.h:29 in 15cb0a2492 outdated

24+    ConfigStatus status;
25+    bilingual_str message{};
26+    std::vector<std::string> details{};
27+};
28+
29+//! Callback function to let the user decide to whether abort loading if

TheCharlatan commented at 6:25 am on February 28, 2023:

nit: s/to whether/whether to/

ryanofsky commented at 7:12 pm on February 28, 2023:

re: #27150 (review)

nit: s/to whether/whether to/

Thanks, fixed comment

in src/bitcoind.cpp:158 in 15cb0a2492 outdated

164-            return InitError(Untranslated(strprintf("%s", e.what())));
165+        if (auto error = common::InitConfig(args)) {
166+            return InitError(error->message, error->details);
167         }
168 
169         // Error out when loose non-argument tokens are encountered on command line

TheCharlatan commented at 7:17 am on February 28, 2023:

More a comment in case anybody else stumbles over this than a change request, but I’m surprised this is done in bitcoind and not in bitcoin-qt. Indeed I can start bitcoin-qt with any number of positional arguments, e.g. ./qt/bitcoin-qt h j k, but not so bitcoind. The historical context is that this is a leftover from when bitcoind was also used as a command line client.

TheCharlatan commented at 9:48 am on February 28, 2023: contributor

Concept ACK

in src/common/init.h:13 in 15cb0a2492 outdated

 7+
 8+#include <util/translation.h>
 9+
10+#include <optional>
11+#include <string>
12+#include <vector>

hebasto commented at 1:12 pm on February 28, 2023:

0#include <functional>
1#include <optional>
2#include <string>
3#include <vector>

ryanofsky commented at 7:12 pm on February 28, 2023:

re: #27150 (review)

Thanks, applied change

in src/common/init.cpp:14 in 15cb0a2492 outdated

0@@ -0,0 +1,69 @@
1+// Copyright (c) 2023 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <common/init.h>
6+#include <chainparams.h>
7+#include <util/system.h>
8+
9+#include <optional>

hebasto commented at 1:15 pm on February 28, 2023:

0#include <common/init.h>
1#include <chainparams.h>
2#include <fs.h>
3#include <tinyformat.h>
4#include <util/system.h>
5#include <util/translation.h>
6
7#include <algorithm>
8#include <exception>
9#include <optional>

ryanofsky commented at 7:11 pm on February 28, 2023:

re: #27150 (review)

Thanks, applied change

hebasto commented at 1:18 pm on February 28, 2023: member

Maybe it makes sense from the beginning to add src/common/init.cpp to the list of files which are processed by IWYU in ci/test/06_script_b.sh?

scripted-diff: Remove double newlines after some init errors

Some InitError calls had trailing \n characters, causing double newlines in
error output. After this change InitError calls consistently output one newline
instead of two. Appearance of messages in the GUI does not seem to be affected.
Can be tested with:

  src/bitcoind -regtest -datadir=noexist
  src/qt/bitcoin-qt -regtest -datadir=noexist

-BEGIN VERIFY SCRIPT-
git grep -l InitError src/ | xargs sed -i 's/\(InitError(.*\)\\n"/\1"/'
-END VERIFY SCRIPT-

c361df90b9

Extend bilingual_str support for tinyformat

Previous bilingual_str tinyformat::format accepted bilingual format strings,
but not bilingual arguments. Extend it to accept both. This is useful when
embedding one translated string inside another translated string, for example:
`strprintf(_("Error: %s"), message)` which would fail previously if `message`
was a bilingual_str.

3db2874bd7

Add InitError(error, details) overload

This is only used in the current PR to avoid ugly
`strprintf(Untranslated("%s:\n%s"), str, MakeUnorderedList(details)`
boilerplate in init code. But in the future the function could be extended and
more widely used to include more details in GUI error messages or display them
in a more readable way, see code comment.

d172b5c671

Deduplicate bitcoind and bitcoin-qt init code

Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code
reading config files and creating the datadir.

There are a few minor changes in behavior:

- In bitcoin-qt, when there is a problem reading the configuration file, the
  GUI error text has changed from "Error: Cannot parse configuration file:" to
  "Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the
  error text has changed from "Failed loading settings file" to "Settings
  file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the
  error text has changed from "Failed saving settings file" to "Settings file
  could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read),
  there is an normal error dialog showing "Error: filesystem error: status:
  Permission denied [.../settings.json]", instead of an uncaught exception

802cc1ef53

DrahtBot added the label Needs rebase on Feb 28, 2023

ryanofsky force-pushed on Feb 28, 2023

ryanofsky commented at 7:16 pm on February 28, 2023: contributor

Thanks for the reviews!

Rebased 15cb0a2492d050eb61cca5a44893a5c841b7f506 -> 802cc1ef536e11944608fe9ab782d3e962037703 (pr/oneconfig.4 -> pr/oneconfig.5, compare) due to conflict with #27170 and applied suggestions

DrahtBot removed the label Needs rebase on Feb 28, 2023

Sjors commented at 2:12 pm on March 1, 2023: member

Light review ACK 802cc1ef536e11944608fe9ab782d3e962037703

I’m not super familiar with the init code, but the changes look sane to me. I tested some of the error messages on macOS.

Regarding d172b5c, any hints on how to produce a fatal error?

ryanofsky commented at 2:57 pm on March 1, 2023: contributor

@sjors

Regarding d172b5c, any hints on how to produce a fatal error?

I don’t know of a way to trigger the AbortError calls in that commit offhand but I think it should be straightforward to verify that replacing them with InitError has no effect, because the previous definition of AbortError was constexpr auto AbortError = InitError; (The commit dropped the AbortError alias because it seemed pointless, and dropping it made overloading InitError easier).

If it helps, I used the following commands for testing the other errors in the PR:

0src/bitcoind -regtest -conf=bad
1src/qt/bitcoin-qt -regtest -conf=bad
2src/qt/bitcoin-qt -regtest
3echo bad > ~/.bitcoin/regtest/settings.json
4sudo chown root ~/.bitcoin/regtest
5sudo chmod 755 ~/.bitcoin/regtest
6sudo chmod 700 ~/.bitcoin/regtest
7sudo chown 1000:1000 ~/.bitcoin/regtest

TheCharlatan commented at 5:42 pm on March 1, 2023: contributor

ACK 802cc1ef536e11944608fe9ab782d3e962037703

fanquake requested review from pinheadmz on Mar 4, 2023

achow101 commented at 5:55 pm on March 7, 2023: member

ACK 802cc1ef536e11944608fe9ab782d3e962037703

achow101 merged this on Mar 7, 2023

achow101 closed this on Mar 7, 2023

sidhujag referenced this in commit 181b413d19 on Mar 7, 2023

bitcoin locked this on Mar 6, 2024

Deduplicate bitcoind and bitcoin-qt init code #27150

Reviews

Conflicts