Merge bitcoin/bitcoin#35179: test: Add importdescriptors rpc error coverage

ed11dd6a5f test: add coverage for importdescriptors when manually interrupting a wallet rescan (Pol Espinasa)
d90d7f0a55 test: add coverage for importdescriptors errors when using assumeutxo (Pol Espinasa)
ad388bf254 test: add coverage for importdescriptors while wallet is rescanning (Pol Espinasa)
84d07e471c test: add coverage for importdescriptor with an encrypted wallet (Pol Espinasa)

Pull request description:

  The current tests for `importdescriptors` RPC do not check for cases where RPC errors should be thrown.

  This PR adds coverage for _importing a descriptor when the wallet is encrypted_ , for _importing a descriptor while the wallet is rescanning_ and _importing a descriptor while using assumeutxo_

  For context, this lack of coverage was found while implementing #34861 when a reviewer found that this was being silently broken in the PR.

  I am not sure if the "rescanning test" approach is the optimal solution, I am open to suggestions.

ACKs for top commit:
  achow101:
    ACK ed11dd6a5f
  w0xlt:
    ACK ed11dd6a5f

Tree-SHA512: 18e7111314ff003d39538d53899a3e2261027f5f965945f259eec4b56ece5c22706faf2891694c47575f3a5089ca02c80ea0bd05c453c4e072335d4a45ab8edd

test/functional/wallet_assumeutxo.py

@@ -215,12 +215,18 @@ class AssumeutxoTest(BitcoinTestFramework):
         wallet_name = "w1"
         n1.createwallet(wallet_name, disable_private_keys=True)
         key = get_generate_key()
-        time = n1.getblockchaininfo()['time']
+        block_info = n1.getblockchaininfo()
+        time = block_info["time"]
+        def expected_rescan_error(timestamp):
+            return f"Rescan failed for descriptor with timestamp {timestamp}. There was an error reading a block from time {time}, which is after or within 7200 seconds of key creation, and could contain transactions pertaining to the desc. As a result, transactions and coins using this desc may not appear in the wallet. This error is likely caused by an in-progress assumeutxo background sync. Check logs or getchainstates RPC for assumeutxo background sync progress and try again later."
         timestamp = 0
-        expected_error_message = f"Rescan failed for descriptor with timestamp {timestamp}. There was an error reading a block from time {time}, which is after or within 7200 seconds of key creation, and could contain transactions pertaining to the desc. As a result, transactions and coins using this desc may not appear in the wallet. This error is likely caused by an in-progress assumeutxo background sync. Check logs or getchainstates RPC for assumeutxo background sync progress and try again later."
         result = self.import_descriptor(n1, wallet_name, key, timestamp)
         assert_equal(result[0]['error']['code'], -1)
-        assert_equal(result[0]['error']['message'], expected_error_message)
+        assert_equal(result[0]['error']['message'], expected_rescan_error(timestamp))
+        now_timestamp = block_info["mediantime"]
+        result = self.import_descriptor(n1, wallet_name, get_generate_key(), "now")
+        assert_equal(result[0]['error']['code'], -1)
+        assert_equal(result[0]['error']['message'], expected_rescan_error(now_timestamp))
 
         self.log.info("Test that rescanning blocks from before the snapshot fails when blocks are not available from the background sync yet")
         w1 = n1.get_wallet_rpc(wallet_name)

test/functional/wallet_importdescriptors.py

@@ -16,6 +16,7 @@ variants.
   and test the values returned."""
 
 import concurrent.futures
+import threading
 import time
 
 from test_framework.blocktools import COINBASE_MATURITY
@@ -117,6 +118,82 @@ class ImportDescriptorsTest(BitcoinTestFramework):
                              wallet=wallet)
         wallet.unloadwallet()
 
+    def test_rescan_fails_import(self):
+        xpriv = "tprv8ZgxMBicQKsPeuVhWwi6wuMQGfPKi9Li5GtX35jVNknACgqe3CY4g5xgkfDDJcmtF7o1QnxWDRYw4H5P26PXq7sbcUkEqeR4fg3Kxp2tigg"
+
+        self.log.info("Test importdescriptors fails when wallet is already rescanning")
+        wallet_name = "rescan_wallet"
+        self.nodes[0].createwallet(wallet_name=wallet_name, blank=True)
+        other_desc = descsum_create("pkh(" + get_generate_key().privkey + ")")
+
+        w_import = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / f"wallet/{wallet_name}"
+
+        with concurrent.futures.ThreadPoolExecutor(max_workers=2) as thread:
+            w_rescan = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / f"wallet/{wallet_name}"
+            w_conflict = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / f"wallet/{wallet_name}"
+            # Use an xprv with timestamp=0 and a large key-range to trigger a slow full rescan that stays in-flight
+            slow_desc = [{"desc": descsum_create("pkh(" + xpriv + "/0h/*h)"),
+            "timestamp": 0, "range": [0, 10000]}]
+            conflicting_desc = [{"desc": descsum_create("pkh(" + xpriv + "/1h/*h)"),
+            "timestamp": 0, "range": [0, 10000]}]
+
+            start = threading.Barrier(3)
+
+            def import_after_barrier(wallet, descriptors):
+                start.wait(timeout=10)
+                return wallet.importdescriptors(descriptors)
+
+            imports = [
+                thread.submit(import_after_barrier, w_rescan, slow_desc),
+                thread.submit(import_after_barrier, w_conflict, conflicting_desc),
+            ]
+            start.wait(timeout=10)
+
+            # One importdescriptor call must hold WalletRescanReserver while the other fails immediately.
+            num_errors = 0
+            num_success = 0
+            for future in concurrent.futures.as_completed(imports, timeout=30 * self.options.timeout_factor):
+                try:
+                    assert_equal(future.result(), [{'success': True}])
+                    num_success += 1
+                except JSONRPCException as e:
+                    assert_equal(e.error["code"], -4)
+                    assert_equal(e.error["message"], "Wallet is currently rescanning. Abort existing rescan or wait.")
+                    num_errors += 1
+
+            assert_equal(num_success, 1)
+            assert_equal(num_errors, 1)
+
+        # After the rescan finishes, any importdescriptors should succeed.
+        result = w_import.importdescriptors([{"desc": other_desc, "timestamp": "now"}])
+        assert_equal(result[0]['success'], True)
+
+        self.log.info("Aborting an importdescriptors rescan should fail the RPC call")
+        wallet_name = "abort_import_wallet"
+        self.nodes[0].createwallet(wallet_name, blank=True)
+
+        with concurrent.futures.ThreadPoolExecutor(max_workers=1) as thread:
+            w_import = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / f"wallet/{wallet_name}"
+            abort_rpc = self.nodes[0].create_new_rpc_connection(mode="AUTHPROXY") / f"wallet/{wallet_name}"
+            descriptor = [{"desc": descsum_create("pkh(" + xpriv + "/2h/*h)"),
+            "timestamp": 0, "range": [0, 4000]}]
+
+            importing = thread.submit(w_import.importdescriptors, descriptor)
+
+            # Keep trying because an abort before ScanForWalletTransactions starts
+            # is reset when the scan loop begins.
+            abort_succeeded = False
+            abort_deadline = time.time() + 30 * self.options.timeout_factor
+            while not importing.done() and time.time() < abort_deadline:
+                abort_succeeded = abort_rpc.abortrescan() or abort_succeeded
+
+            assert_equal(abort_succeeded, True)
+            try:
+                importing.result(timeout=30 * self.options.timeout_factor)
+                raise AssertionError("importdescriptors unexpectedly succeeded")
+            except JSONRPCException as e:
+                assert_equal(e.error["code"], -1)
+                assert_equal(e.error["message"], "Rescan aborted by user.")
 
     def run_test(self):
         self.log.info('Setting up wallets')
@@ -762,6 +839,10 @@ class ImportDescriptorsTest(BitcoinTestFramework):
         self.nodes[0].createwallet("encrypted_wallet", blank=True, passphrase="passphrase")
         encrypted_wallet = self.nodes[0].get_wallet_rpc("encrypted_wallet")
 
+        self.log.info("Wallet must be unlocked to import a descriptor")
+        assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.",
+            encrypted_wallet.importdescriptors, [descriptor])
+
         descriptor["timestamp"] = 0
         descriptor["next_index"] = 0
 
@@ -878,6 +959,7 @@ class ImportDescriptorsTest(BitcoinTestFramework):
         self.test_import_unused_key()
         self.test_import_unused_key_existing()
         self.test_import_unused_noprivs()
+        self.test_rescan_fails_import()
 
 if __name__ == '__main__':
     ImportDescriptorsTest(__file__).main()