Deduplicate bitcoind and bitcoin-qt init code #27150

ryanofsky · 2023-02-23T21:25:39Z

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 · 2023-02-23T21:25:42Z

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 · 2023-02-23T21:26:54Z

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

jarolrod · 2023-02-24T03:50:36Z

concept ack

fanquake · 2023-02-24T08:54:26Z

Concept ACK - #27073 has now been merged.

hebasto · 2023-02-24T09:20:42Z

Concept ACK.

ryanofsky · 2023-02-24T19:37:44Z

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

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

ryanofsky · 2023-02-25T14:07:45Z

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

src/test/translation_tests.cpp

ryanofsky

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

src/test/translation_tests.cpp

TheCharlatan

Concept ACK

src/common/init.h

TheCharlatan · 2023-02-28T07:17:43Z

src/bitcoind.cpp

-        } catch (const std::exception& e) {
-            return InitError(Untranslated(strprintf("%s", e.what())));
+        if (auto error = common::InitConfig(args)) {
+            return InitError(error->message, error->details);
        }

        // Error out when loose non-argument tokens are encountered on command line


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.

hebasto

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?

src/common/init.h

src/common/init.cpp

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-

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.

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.

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

ryanofsky

Thanks for the reviews!

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

src/common/init.cpp

src/common/init.h

Sjors · 2023-03-01T14:12:52Z

Light review ACK 802cc1e

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 · 2023-03-01T14:57:58Z

@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:

src/bitcoind -regtest -conf=bad
src/qt/bitcoin-qt -regtest -conf=bad
src/qt/bitcoin-qt -regtest
echo bad > ~/.bitcoin/regtest/settings.json
sudo chown root ~/.bitcoin/regtest
sudo chmod 755 ~/.bitcoin/regtest
sudo chmod 700 ~/.bitcoin/regtest
sudo chown 1000:1000 ~/.bitcoin/regtest

TheCharlatan · 2023-03-01T17:42:47Z

ACK 802cc1e

achow101 · 2023-03-07T17:55:20Z

ACK 802cc1e

This was referenced Feb 24, 2023

Move CopyrightHolders() and LicenseInfo() into libbitcoin_common #26688

Closed

Add UNREACHABLE macro #26504

Closed

ryanofsky force-pushed the pr/oneconfig branch 2 times, most recently from ebc1659 to e248926 Compare February 24, 2023 19:28

ryanofsky marked this pull request as ready for review February 24, 2023 19:37

ryanofsky force-pushed the pr/oneconfig branch from e248926 to 25f9cbf Compare February 25, 2023 14:05

fanquake reviewed Feb 27, 2023

View reviewed changes

src/test/translation_tests.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 27, 2023

refactor: Stop using gArgs global in system.cpp #27170

Merged

ryanofsky force-pushed the pr/oneconfig branch from 25f9cbf to 15cb0a2 Compare February 27, 2023 22:30

ryanofsky commented Feb 27, 2023

View reviewed changes

src/test/translation_tests.cpp Show resolved Hide resolved

TheCharlatan reviewed Feb 28, 2023

View reviewed changes

hebasto reviewed Feb 28, 2023

View reviewed changes

src/common/init.h Show resolved Hide resolved

src/common/init.cpp Outdated Show resolved Hide resolved

ryanofsky added 4 commits February 28, 2023 12:04

DrahtBot added the Needs rebase label Feb 28, 2023

ryanofsky force-pushed the pr/oneconfig branch from 15cb0a2 to 802cc1e Compare February 28, 2023 19:15

ryanofsky commented Feb 28, 2023

View reviewed changes

src/common/init.cpp Outdated Show resolved Hide resolved

src/common/init.h Show resolved Hide resolved

src/common/init.h Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label Feb 28, 2023

fanquake requested a review from pinheadmz March 4, 2023 08:01

achow101 merged commit fc037c8 into bitcoin:master Mar 7, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 7, 2023

Merge bitcoin#27150: Deduplicate syscoind and syscoin-qt init code

181b413

hebasto mentioned this pull request Mar 13, 2023

Bitcoin ignores datadir and blocksdir parameter in .conf #27246

Closed

1 task

hebasto mentioned this pull request Mar 31, 2023

Rebased cmake-staging branch (post PR#10) hebasto/bitcoin#11

Closed

bitcoin locked and limited conversation to collaborators Mar 6, 2024

Deduplicate bitcoind and bitcoin-qt init code #27150

Deduplicate bitcoind and bitcoin-qt init code #27150

Uh oh!

Conversation

ryanofsky commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

ryanofsky commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod commented Feb 24, 2023

Uh oh!

fanquake commented Feb 24, 2023

Uh oh!

hebasto commented Feb 24, 2023

Uh oh!

ryanofsky commented Feb 24, 2023

Uh oh!

ryanofsky commented Feb 25, 2023

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheCharlatan Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 1, 2023

Uh oh!

TheCharlatan commented Mar 1, 2023

Uh oh!

achow101 commented Mar 7, 2023

Uh oh!

Uh oh!

ryanofsky commented Feb 23, 2023 •

edited

Loading

DrahtBot commented Feb 23, 2023 •

edited

Loading

ryanofsky commented Feb 23, 2023 •

edited

Loading

Sjors commented Mar 1, 2023 •

edited

Loading