test: Add importdescriptors rpc error coverage #35179

Question: Should the submitted import future be checked here?

The second RPC call is made after the rescan-start log is observed, so this does exercise the "already rescanning" error path. But the background importdescriptors future is not stored or checked, and leaving the ThreadPoolExecutor context waits for it to finish before abortrescan() is called. So the later abortrescan() call likely does not actually abort this rescan.

Would it be clearer to either assert the background import result, or move the abort/check inside the executor block if the intention is to stop the rescan early?

But the background importdescriptors future is not stored or checked

It doesn't matter, it is not what this test is doing. There are plenty of tests testing "normal" imports. This test only wants to make sure that if a rescan is going on, trying to import a descriptor to that wallet will fail.

You are right about the abortrescan(), before it was inside the thread, I guess I moved it by mistake, but given the test is pretty fast, it can just be deleted.

nit: the testing framework has an util function for this:

        def make_proxy():
            return get_rpc_proxy(f"{self.nodes[0].url}/wallet/{wallet_name}", self.nodes[0].index)

I could use get_rpc_proxy but there's no much difference because I cannot use self.nodes[0].url it returns None if --usecli is used.

If I use get_rpc_proxy the diff would look like:

<details> <summary>diff</summary>

$ git diff
diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index cd18bf4474..ef31bf5eec 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -23,10 +23,10 @@ from test_framework.blocktools import COINBASE_MATURITY
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.descriptors import descsum_create
 from test_framework.script import SEQUENCE_LOCKTIME_TYPE_FLAG
-from test_framework.authproxy import AuthServiceProxy
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
+    get_rpc_proxy,
     rpc_url,
 )
 from test_framework.wallet_util import (
@@ -833,7 +833,8 @@ class ImportDescriptorsTest(BitcoinTestFramework):
         w_rescan = self.nodes[0].get_wallet_rpc(wallet_name)
 
         def make_proxy():
-            return AuthServiceProxy(rpc_url(self.nodes[0].datadir_path, self.nodes[0].index, self.nodes[0].chain, self.nodes[0].rpchost) + "/wallet/" + wallet_name)
+            return get_rpc_proxy(rpc_url(self.nodes[0].datadir_path, self.nodes[0].index, self.nodes[0].chain, self.nodes[0].rpchost) + f"/wallet/{wallet_name}",
+             self.nodes[0].index)
 
         with concurrent.futures.ThreadPoolExecutor(max_workers=1) as thread:
             slow_desc = descsum_create("pkh(" + xpriv + "/1h/*h)")

</details>

Which looks even a bit longer so it's less readable. I will let as is unless someone suggest a cleaner way of doing it.

Cool, didn't know about the --usecli implication. Leaving as-is makes sense

you are side-stepping the timeout and the coverage. This may be fine, but I think it is cleaner to have a function here. Maybe faab8e28db0bf205451ff3d30f269167b73f24c0 (if you want to use the rpc), or alternatively, use the cli(), like the other tests that spin up a new thread and need a new rpc socket for that thread.

Done, used cli()