test: Add importdescriptors rpc error test coverage #35630

In addition to #35179 (already merged) this adds more missing test coverage that was detected while rebasing #34861.

The three tests added checks:

• Locked wallet throws because of being locked if giving an empty importdescriptors request.
• Invalid or missing timestamp throws as a top level RPC error and not a per-item error.
• The order of the requests and the response is the same, even if failing or succeeding.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35630.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Why is it desirable to enforce the order in which these errors are enforced?

Why is it desirable to enforce the order in which these errors are enforced?

+1

Why is it desirable to enforce the order in which these errors are enforced?

Because the importdescriptors RPC response is an array. Not a json with key-value elements that can be used to match requests and responses. If the order is not kept if an import fail you cannot know which descriptor is the one that failed.

@polespinasa That makes sense. What I mean is e.g. in https://github.com/bitcoin/bitcoin/pull/35630/changes/2d536455829b904138c8853d9aed636916373e94 it seems that the test is checking that even when the importdescriptors call is erroneous because it's passed an empty list of descriptors, that the error is for being locked and not for being an empty list of descriptors. I guess what I'm asking is why does it matter to enforce which error gets checked for first, that seems like an implementation detail, but maybe I have misunderstood what is being tested for there.

Oh sorry, I completely misunderstood your question.

it seems that the test is checking that even when the importdescriptors call is erroneous because it's passed an empty list of descriptors, that the error is for being locked and not for being an empty list of descriptors.

An empty list of descriptors is not an error. We do accept that in the current implementation. The following diff allows to trigger it:

$ git diff
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 396be62825..3d2d7f89c8 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -399,7 +399,7 @@ RPCMethod importdescriptors()
     int64_t lowest_timestamp = 0;
     bool rescan = false;
     UniValue response(UniValue::VARR);
-    {
+    if (!requests.empty()) {
         LOCK(pwallet->cs_wallet);
         EnsureWalletIsUnlocked(*pwallet);
 

This test is ensuring that even if we don't provide a list of descriptors to import, the wallet-state related errors are still enforced. We could argue if this behavior is the correct one or not, we could just throw at the really beginning of the RPC call because it does not make sense, but the current state is the one defined in the test. And that is not covered by the tests (before this PR) as I almost silently break it in #34861 as can be seen in: #34861 (review)

In the last force-push I have added the other case so this expected behavior is clear in the test itself.

$ git diff 6ca82938bbbe43deeabb65fdbf649687620e3b5e..84bc7db250efb0fc253b2359a8ac879c6e3dd724
diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index b48db8dbfd..f92b34c7dd 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -909,6 +909,11 @@ class ImportDescriptorsTest(BitcoinTestFramework):
         assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.",
             encrypted_wallet.importdescriptors, [])
 
+        self.log.info("An unlocked wallet accepts an empty importdescriptors request")
+        self.nodes[0].createwallet("unencrypted_wallet", blank=True)
+        unencrypted_wallet = self.nodes[0].get_wallet_rpc("unencrypted_wallet")
+        assert_equal(unencrypted_wallet.importdescriptors([]), [])
+
         descriptor["timestamp"] = 0
         descriptor["next_index"] = 0