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

ee47ca29d6 init: fix fatal error on '-wallet' negated option value (furszy)

Pull request description:

  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 https://github.com/bitcoin/bitcoin/pull/22217. Where the `GetArgs("-wallet")` call was
  replaced by `GetSettingsList("-wallet")`.

ACKs for top commit:
  achow101:
    ACK ee47ca29d6
  ryanofsky:
    Code review ACK ee47ca29d6, just adding the suggested test since last review
  TheCharlatan:
    ACK ee47ca29d6
  ismaelsadeeq:
    Tested ACK ee47ca29d6

Tree-SHA512: 5f01076f74a048019bb70791160f0accc2db7a457d969cb23687bed81ccbbdec1dda68311e7c6e2dd56250e23e8d926d4066e5014b2a99a2fc202e24ed264fbd

src/wallet/load.cpp

@@ -77,6 +77,11 @@ bool VerifyWallets(WalletContext& context)
     std::set<fs::path> wallet_paths;
 
     for (const auto& wallet : chain.getSettingsList("wallet")) {
+        if (!wallet.isStr()) {
+            chain.initError(_("Invalid value detected for '-wallet' or '-nowallet'. "
+                              "'-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets"));
+            return false;
+        }
         const auto& wallet_file = wallet.get_str();
         const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(wallet_file));
 
@@ -110,6 +115,11 @@ bool LoadWallets(WalletContext& context)
     try {
         std::set<fs::path> wallet_paths;
         for (const auto& wallet : chain.getSettingsList("wallet")) {
+            if (!wallet.isStr()) {
+                chain.initError(_("Invalid value detected for '-wallet' or '-nowallet'. "
+                                  "'-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets"));
+                return false;
+            }
             const auto& name = wallet.get_str();
             if (!wallet_paths.insert(fs::PathFromString(name)).second) {
                 continue;

test/functional/feature_config_args.py

@@ -153,6 +153,13 @@ class ConfArgsTest(BitcoinTestFramework):
             expected_msg='Error: Error parsing command line arguments: Can not set -proxy with no value. Please specify value with -proxy=value.',
             extra_args=['-proxy'],
         )
+        # Provide a value different from 1 to the -wallet negated option
+        if self.is_wallet_compiled():
+            for value in [0, 'not_a_boolean']:
+                self.nodes[0].assert_start_raises_init_error(
+                    expected_msg="Error: Invalid value detected for '-wallet' or '-nowallet'. '-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets",
+                    extra_args=[f'-nowallet={value}'],
+                )
 
     def test_log_buffer(self):
         self.stop_node(0)

test/functional/feature_settings.py

@@ -13,11 +13,32 @@ from test_framework.util import assert_equal
 
 
 class SettingsTest(BitcoinTestFramework):
+    def add_options(self, parser):
+        self.add_wallet_options(parser)
+
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
         self.wallet_names = []
 
+    def test_wallet_settings(self, settings_path):
+        if not self.is_wallet_compiled():
+            return
+
+        self.log.info("Testing wallet settings..")
+        node = self.nodes[0]
+        # Create wallet to use it during tests
+        self.start_node(0)
+        node.createwallet(wallet_name='w1')
+        self.stop_node(0)
+
+        # Verify wallet settings can only be strings. Either names or paths. Not booleans, nums nor anything else.
+        for wallets_data in [[10], [True], [[]], [{}], ["w1", 10], ["w1", False]]:
+            with settings_path.open("w") as fp:
+                json.dump({"wallet": wallets_data}, fp)
+            node.assert_start_raises_init_error(expected_msg="Error: Invalid value detected for '-wallet' or '-nowallet'. '-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets",
+                                                extra_args=[f'-settings={settings_path}'])
+
     def run_test(self):
         node, = self.nodes
         settings = node.chain_path / "settings.json"
@@ -86,6 +107,8 @@ class SettingsTest(BitcoinTestFramework):
             self.start_node(0, extra_args=[f"-settings={altsettings}"])
             self.stop_node(0)
 
+        self.test_wallet_settings(settings)
+
 
 if __name__ == '__main__':
     SettingsTest(__file__).main()