init: fix init fatal error on invalid negated option value

furszy commented at 2:32 pm on August 20, 2024: member

Currently, if users provide a double negated value such as ‘-nowallet=0’ or a non-boolean convertible value to a negated option such as ‘-nowallet=not_a_boolean’, the initialization process results in a fatal error, causing an unclean shutdown and displaying a poorly descriptive error message: “JSON value of type bool is not of expected type string.” (On bitcoind. The GUI does not display any error msg - upcoming PR -).

This PR fixes the issue by ensuring that only string values are returned in the the “wallet” settings list, failing otherwise. It also improves the clarity of the returned error message.

Note: This bug was introduced in #22217. Where the GetArgs("-wallet") call was replaced by GetSettingsList("-wallet").

DrahtBot commented at 2:32 pm on August 20, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, TheCharlatan, ismaelsadeeq, achow101

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

Conflicts

No conflicts as of last run.

furszy force-pushed on Aug 20, 2024

ryanofsky commented at 10:29 pm on August 20, 2024: contributor

Nice catch! I think the changes in 744d7242239b8c09c2626e5ef7f90c2a91eed0fa might have some benefits, but might not be the best solution to this problem for a few reasons:

They are not a complete fix. While they prevent an uncaught exception when you pass -nowallet=not_a_bool they do not prevent a similar uncaught exception when you pass -nowallet=0
The new error message “Invalid ‘-no%s=%s’: Value must be a boolean (0/1 or false/true)” seems to suggest that false/true values would be allowed but they do not actually seem to be.
They introduce a new way of parsing boolean variables. Before this PR there was one way to parse boolean values, and it was kind of weird, but now there are two different ways of parsing values, one for negated values, and one for non-negated.
They change what values are allowed to be passed to all bitcoin settings when the problem this is trying to fix is limited to one specific setting which is is parsed in an unusual way
They are not backwards compatible. Probably it would be good thing to restrict what values are allowed, but there could be working configurations using values be broken by this change, so it’s worth thinking about whether actually we want to break these to fix a bug which doesn’t require breaking anything.

So I think a better fix for the issue described would look more like the following:

 0--- a/src/wallet/load.cpp
 1+++ b/src/wallet/load.cpp
 2@@ -77,6 +77,7 @@ bool VerifyWallets(WalletContext& context)
 3     std::set<fs::path> wallet_paths;
 4 
 5     for (const auto& wallet : chain.getSettingsList("wallet")) {
 6+        if (!wallet.isStr()) continue;
 7         const auto& wallet_file = wallet.get_str();
 8         const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(wallet_file));
 9 
10@@ -110,6 +111,7 @@ bool LoadWallets(WalletContext& context)
11     try {
12         std::set<fs::path> wallet_paths;
13         for (const auto& wallet : chain.getSettingsList("wallet")) {
14+            if (!wallet.isStr()) continue;
15             const auto& name = wallet.get_str();
16             if (!wallet_paths.insert(fs::PathFromString(name)).second) {
17                 continue;

These changes are needed anyway to fix -nowallet=0. The more generic changes in 744d7242239b8c09c2626e5ef7f90c2a91eed0fa could be useful too, but I think they would be better in different PR not tied to the -wallet setting

furszy commented at 9:55 pm on August 22, 2024: member

Hmm yeah ok, nice catch. I missed that getSettingsList return a list of json that can be checked against the string type. One drawback of your suggestion is that we currently fail (crash) when the invalid value is provided and with your suggestion, we would ignore it. So, by following-up this fix with new args restrictions (which I think we should do) we would be changing behavior twice. Unless we don’t backport the crash fix and merge both PRs during the same release window or.. we fail with a more general error for the time being at the two if (!wallet.isStr()) lines (we can’t report the correct error there because the negation information gets lost after parsing. A double negated value is interpreted as true -> which is not detected as a negation in any of the SettingsValue/SettingsSpan/etc classes).

furszy commented at 10:06 pm on August 22, 2024: member

Found #16545 at the ArgsManager::Flags enum when started implementing the same idea. Cool stuff. Happy to move there on this release cycle. Will report a general error at the two if (!wallet.isStr()) lines to fix the crash and keep the init failure here.

ryanofsky commented at 10:37 pm on August 22, 2024: contributor

I’m not sure I understand the drawback of the suggested fix in #30684 (comment). You are concerned that a writing -nowallet=0 or -nowallet=not_a_bool would be ignored instead of treated as an error, and then potentially be treated as an error again at some future date?

This doesn’t seem like big problem to me because -nosetting=0 -nosetting=not_a_bool is already allowed most other places and it is just treated -setting=1. So it seems like not a big deal that a double negative here would just reverse the effect of any previous negation and do nothing.

If you want to add special code to make this an error it seems ok, but it also seems ok to ignore it and handle it more like most other settings.

furszy force-pushed on Aug 22, 2024

furszy commented at 10:55 pm on August 22, 2024: member

I’m not sure I understand the drawback of the suggested fix in #30684 (comment). You are concerned that a writing -nowallet=0 or -nowallet=not_a_bool would be ignored instead of treated as an error, and then potentially be treated as an error again at some future date?

Yeah, I’m just trying to avoid users placing something like this in the config file, forgetting about it, and then coming back in the future (after our changes) asking why their node doesn’t start anymore. May prevent some headaches and it does not require that many lines more.

Yet, if we don’t find a good wording for the new error message, I could also remove it.

DrahtBot added the label CI failed on Aug 22, 2024

furszy force-pushed on Aug 22, 2024

DrahtBot removed the label CI failed on Aug 23, 2024

in src/wallet/load.cpp:81 in e56e2f5180 outdated

76@@ -77,6 +77,11 @@ bool VerifyWallets(WalletContext& context)
77     std::set<fs::path> wallet_paths;
78 
79     for (const auto& wallet : chain.getSettingsList("wallet")) {
80+        if (!wallet.isStr()) {
81+            chain.initError(_("Invalid value detected for '-wallet' or '-nowallet'. "

ryanofsky commented at 1:59 am on August 23, 2024:

In commit “init: fix fatal error on ‘-wallet’ negated option value” (e56e2f51803b276945c74a479d30d884256d7ffc)

If desired, you could include the value in the error message like strprintf(_("Invalid -wallet value %s detected"), wallet.write())

furszy commented at 2:49 pm on August 23, 2024:

In commit “init: fix fatal error on ‘-wallet’ negated option value” (e56e2f5)

If desired, you could include the value in the error message like strprintf(_("Invalid -wallet value %s detected"), wallet.write())

Sadly, that wouldn’t help much and could end up confusing users. The error output for -nowallet=not_a_boolean would be -wallet=true, which does not correspond to what the user inputted. I think leaving the more general error until #16545 is fine.

ryanofsky approved

ryanofsky commented at 2:00 am on August 23, 2024: contributor

Code review ACK e56e2f51803b276945c74a479d30d884256d7ffc.

Thanks for the fix! Could update the description and note that this bug was caused by #22217. Before that PR specifying -nowallet=0 or -nowallet=not_a_bool would be equivalent to specifying -wallet=1, after that PR it triggers an uncaught exception, and now it triggers a clearer error. I think this error could also be triggered by editing settings.json and adding a non-string wallet name.

furszy force-pushed on Aug 23, 2024

furszy commented at 2:54 pm on August 23, 2024: member

Could update the description and note that this bug was caused by #22217.

Sure. Done!.

I think this error could also be triggered by editing settings.json and adding a non-string wallet name.

Yes, nice. Added coverage for it too.

in test/functional/feature_config_args.py:160 in 0df52e2436 outdated

152@@ -153,6 +153,12 @@ def test_invalid_command_line_options(self):
153             expected_msg='Error: Error parsing command line arguments: Can not set -proxy with no value. Please specify value with -proxy=value.',
154             extra_args=['-proxy'],
155         )
156+        # Provide a non-boolean-convertible arg to a negated option
157+        if self.is_wallet_compiled():
158+            self.nodes[0].assert_start_raises_init_error(
159+                expected_msg="Error: Invalid value detected for '-wallet' or '-nowallet'. '-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets",
160+                extra_args=['-nowallet=not_a_boolean'],

ryanofsky commented at 8:49 pm on August 26, 2024:

In commit “init: fix fatal error on ‘-wallet’ negated option value” (0df52e24363f729210887ca20a47824ba8073617)

I think this should be extended to test -nowallet=0. Maybe consider making this a loop to test both -nowallet=0 -nowallet=not_a_boolean.

Reason: the simplest and most direct way to trigger the uncaught exception is to write -nowallet=0. The -nowallet=not_a_boolean case is a more indirect way to trigger it because it relies on IntepretBool interpreting the string not_a_boolean as false.

furszy commented at 9:31 pm on August 26, 2024:

absolutely. Pushed.

ryanofsky approved

ryanofsky commented at 8:52 pm on August 26, 2024: contributor

Code review ACK 0df52e24363f729210887ca20a47824ba8073617

init: fix fatal error on '-wallet' negated option value

Because we don't have type checking for command-line/settings/config
args, strings are interpreted as 'false' for non-boolean args.
By convention, this "forces" us to interpret negated strings as 'true',
which conflicts with the negated option definition in all the settings
classes (they expect negated options to always be false and ignore any
other value preceding them). Consequently, when retrieving all "wallet"
values from the command-line/settings/config, we also fetch the negated
string boolean value, which is not of the expected 'string' type.

This mismatch leads to an internal fatal error, resulting in an unclean
shutdown during initialization. Furthermore, this error displays a poorly
descriptive error message:
"JSON value of type bool is not of expected type string"

This commit fixes the fatal error by ensuring that only string values are
returned in the "wallet" settings list, failing otherwise. It also improves
the clarity of the returned error message.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>

ee47ca29d6

furszy force-pushed on Aug 26, 2024

achow101 added this to the milestone 28.0 on Aug 29, 2024

achow101 removed this from the milestone 28.0 on Aug 29, 2024

ryanofsky approved

ryanofsky commented at 6:22 pm on September 4, 2024: contributor

Code review ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef, just adding the suggested test since last review

ryanofsky commented at 6:25 pm on September 4, 2024: contributor

May want to update the pr description to say this bug also when happens passing a double negated value, since the not_a_boolean case is really just a special case of that

furszy commented at 8:58 pm on September 4, 2024: member

May want to update the pr description to say this bug also when happens passing a double negated value, since the not_a_boolean case is really just a special case of that

yeah, thanks. Done. PR description updated.

ismaelsadeeq commented at 10:56 pm on September 5, 2024: member

Concept ACK will review

TheCharlatan approved

TheCharlatan commented at 10:44 am on September 6, 2024: contributor

ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef

DrahtBot requested review from ismaelsadeeq on Sep 6, 2024

in test/functional/feature_config_args.py:158 in ee47ca29d6

152@@ -153,6 +153,13 @@ def test_invalid_command_line_options(self):
153             expected_msg='Error: Error parsing command line arguments: Can not set -proxy with no value. Please specify value with -proxy=value.',
154             extra_args=['-proxy'],
155         )
156+        # Provide a value different from 1 to the -wallet negated option
157+        if self.is_wallet_compiled():
158+            for value in [0, 'not_a_boolean']:

ismaelsadeeq commented at 11:40 am on September 6, 2024:

adding negative value to the values makes this test to fail. should we also prevent passing negative integers?

ryanofsky commented at 1:12 pm on September 6, 2024:

adding negative value to the values makes this test to fail. should we also prevent passing negative integers?

-nowallet=-1 should be treated the same as -nowallet=1 which is the same as -nowallet which just clears the lists of wallets to load and is not expected to be an error.

This test is testing what happens when -nowallet=0 is passed. Previously before #22217 -nowallet=0 was the treated the same as -wallet=1, and would cause a wallet called “1” to be loaded. But #22217 accidentally made this into an uncaught exception, so this PR is just preventing the exception and printing a clearer error.

-nowallet=not_a_boolean is treated the same as -nowallet=0 due to behavior of InterpretBool and I’m not really sure it is a great idea to test it here because it makes the test extra confusing and there is already a lot of coverage for InterpretBool function elsewhere. But it’s also fine to keep.

Note: a lot of the behavior described above is a little insane, but has a certain internal logic and is not specific to the -wallet option, applying to all options.

ismaelsadeeq commented at 1:27 pm on September 8, 2024:

Ah, I see. For users, it will be surprising to see that passing a convertible value like -1 is treated the same as 1.

To prevent confusion, this should be documented elsewhere if it isn’t already, or the error message should be modified to indicate this behavior “that boolean convertible value that will result in true is also accepted”.

DrahtBot requested review from ismaelsadeeq on Sep 6, 2024

ismaelsadeeq approved

ismaelsadeeq commented at 1:29 pm on September 8, 2024: member

Tested ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef

I’ve verified that on master, passing a non-boolean convertible value and 0 causes a fatal error. This PR provides a much clearer error message.

achow101 commented at 4:38 pm on September 9, 2024: member

ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef

achow101 merged this on Sep 9, 2024

achow101 closed this on Sep 9, 2024

init: fix init fatal error on invalid negated option value #30684

Code Coverage

Reviews

Conflicts