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

ee47ca2 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 bitcoin/bitcoin#22217. Where the `GetArgs("-wallet")` call was
  replaced by `GetSettingsList("-wallet")`.

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

Tree-SHA512: 5f01076f74a048019bb70791160f0accc2db7a457d969cb23687bed81ccbbdec1dda68311e7c6e2dd56250e23e8d926d4066e5014b2a99a2fc202e24ed264fbd

Author: achow101

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 @@ def test_invalid_command_line_options(self):
             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 @@
 
 
 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 @@ def run_test(self):
             self.start_node(0, extra_args=[f"-settings={altsettings}"])
             self.stop_node(0)
 
+        self.test_wallet_settings(settings)
+
 
 if __name__ == '__main__':
     SettingsTest(__file__).main()