Merge bitcoin/bitcoin#26480: test: Remove wallet option from non-wallet tests

fa10f193b5 test: Set default in add_wallet_options if only one type can be chosen (MacroFake)
555519d082 test: Remove wallet option from non-wallet tests (MacroFake)
fac8d59d31 test: Set -disablewallet when no wallet has been compiled (MacroFake)
fa68937b89 test: Make requires_wallet private (MacroFake)

Pull request description:

  The tests have several issues:

  * Some tests that are wallet-type specific offer the option to run the test with the incompatible type

  For example, `wallet_dump.py` offers `--descriptors` and on current master fails with `JSONRPCException: Invalid public key`. After the changes here, it fails with a clear error: `unrecognized arguments: --descriptors`.

  * Tests that don't use the wallet at all offer the option to run it with a wallet type. This is confusing and wastes developers time if they are "tricked" into running the test for both wallet types, even though no wallet code is executed at all.

  For example, `feature_addrman.py` will happily accept and run with `--descriptors` or `--legacy-wallet`. After the changes here, it no longer silently ignores the flag, but reports a clear error: `unrecognized arguments`.

ACKs for top commit:
  achow101:
    ACK fa10f193b5

Tree-SHA512: a5784da7305f4ec58c0013f433289000d94fc3d434b00fc329ffa37b812e2cd1da0071e34c3462bf79d904808564f2ae6d3d582f6b86b26215f9b07391b58460

test/functional/test_framework/test_framework.py

@@ -113,7 +113,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         self.wallet_names = None
         # By default the wallet is not required. Set to true by skip_if_no_wallet().
         # When False, we ignore wallet_names regardless of what it is.
-        self.requires_wallet = False
+        self._requires_wallet = False
         # Disable ThreadOpenConnections by default, so that adding entries to
         # addrman will not result in automatic connections to them.
         self.disable_autoconnect = True
@@ -194,12 +194,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
                             help="set a random seed for deterministically reproducing a previous test run")
         parser.add_argument('--timeout-factor', dest="timeout_factor", type=float, default=1.0, help='adjust test timeouts by a factor. Setting it to 0 disables all timeouts')
 
-        group = parser.add_mutually_exclusive_group()
-        group.add_argument("--descriptors", action='store_const', const=True,
-                            help="Run test using a descriptor wallet", dest='descriptors')
-        group.add_argument("--legacy-wallet", action='store_const', const=False,
-                            help="Run test using legacy wallets", dest='descriptors')
-
         self.add_options(parser)
         # Running TestShell in a Jupyter notebook causes an additional -f argument
         # To keep TestShell from failing with an "unrecognized argument" error, we add a dummy "-f" argument
@@ -212,7 +206,13 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         config.read_file(open(self.options.configfile))
         self.config = config
 
-        if self.options.descriptors is None:
+        if "descriptors" not in self.options:
+            # Wallet is not required by the test at all and the value of self.options.descriptors won't matter.
+            # It still needs to exist and be None in order for tests to work however.
+            # So set it to None to force -disablewallet, because the wallet is not needed.
+            self.options.descriptors = None
+        elif self.options.descriptors is None:
+            # Some wallet is either required or optionally used by the test.
             # Prefer BDB unless it isn't available
             if self.is_bdb_compiled():
                 self.options.descriptors = False
@@ -221,6 +221,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             else:
                 # If neither are compiled, tests requiring a wallet will be skipped and the value of self.options.descriptors won't matter
                 # It still needs to exist and be None in order for tests to work however.
+                # So set it to None, which will also set -disablewallet.
                 self.options.descriptors = None
 
         PortSeed.n = self.options.port_seed
@@ -412,7 +413,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             extra_args = self.extra_args
         self.add_nodes(self.num_nodes, extra_args)
         self.start_nodes()
-        if self.requires_wallet:
+        if self._requires_wallet:
             self.import_deterministic_coinbase_privkeys()
         if not self.setup_clean_chain:
             for n in self.nodes:
@@ -446,6 +447,19 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 
     # Public helper methods. These can be accessed by the subclass test scripts.
 
+    def add_wallet_options(self, parser, *, descriptors=True, legacy=True):
+        group = parser.add_mutually_exclusive_group()
+        kwargs = {}
+        if descriptors + legacy == 1:
+            # If only one type can be chosen, set it as default
+            kwargs["default"] = descriptors
+        if descriptors:
+            group.add_argument("--descriptors", action='store_const', const=True, **kwargs,
+                               help="Run test using a descriptor wallet", dest='descriptors')
+        if legacy:
+            group.add_argument("--legacy-wallet", action='store_const', const=False, **kwargs,
+                               help="Run test using legacy wallets", dest='descriptors')
+
     def add_nodes(self, num_nodes: int, extra_args=None, *, rpchost=None, binary=None, binary_cli=None, versions=None):
         """Instantiate TestNode objects.
 
@@ -866,7 +880,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 
     def skip_if_no_wallet(self):
         """Skip the running test if wallet has not been compiled."""
-        self.requires_wallet = True
+        self._requires_wallet = True
         if not self.is_wallet_compiled():
             raise SkipTest("wallet has not been compiled.")
         if self.options.descriptors:

test/functional/test_framework/test_node.py

@@ -105,6 +105,9 @@ class TestNode():
             "-debugexclude=rand",
             "-uacomment=testnode%d" % i,
         ]
+        if self.descriptors is None:
+            self.args.append("-disablewallet")
+
         if use_valgrind:
             default_suppressions_file = os.path.join(
                 os.path.dirname(os.path.realpath(__file__)),