In addition to #35179 (already merged) this adds more missing test coverage that was detected while rebasing #34861.
The three tests added checks:
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35630.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--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.
Co-authored-by: w0xlt <woltx@protonmail.com>
Also adds global_error to test_importdesc to make it able to test per-item errors or global RPC errors
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
ACK 84bc7db250efb0fc253b2359a8ac879c6e3dd724
158 | + }, False), 159 | + ] 160 | + 161 | + descriptors, expected = map(list, zip(*cases)) 162 | + result = wallet.importdescriptors(descriptors) 163 | + assert_equal([r["success"] for r in result], expected)
Since the main goal here is to check that the results are returned in the same order as passed in the request, I think we should check the error messages rather than wheter if it was successfull or not. The way we are currently doing it, if the descriptors that must fail get swapped with each other, the test will still pass. Checking the returned error messages would also catch that case.
The same applies to the descriptors that are expected to succeed. If those get swapped, we won't be able to detect it either. However, in that case, I don't think there is a way to solve this..
succes_cases = [
({
"desc": descsum_create(f"pkh({get_generate_key().privkey})"),
"timestamp": 1,
"label": "Valid descriptor1",
}, True),
({
"desc": descsum_create(f"pkh({get_generate_key().privkey})"),
"timestamp": "now",
"internal": True,
}, True),
]
descriptors, expected = map(list, zip(*succes_cases))
result = wallet.importdescriptors(descriptors)
assert_equal([r["success"] for r in result], expected)
whitespace_pubkey = f" {get_generate_key().pubkey}"
failed_cases = [
({
"timestamp": "now"
}, 'Descriptor not found.'),
({
"desc": descsum_create(f"pkh({get_generate_key().pubkey})"),
"timestamp": "now",
"label": "Invalid descriptor 2",
"internal": True,
}, 'Internal addresses should not have a label'),
({
"desc": descsum_create(f"pkh({whitespace_pubkey})"),
"timestamp": "now",
"internal": True,
}, f"pkh(): Key '{whitespace_pubkey}' is invalid due to whitespace"),
]
descriptors, errors = map(list, zip(*failed_cases))
result = wallet.importdescriptors(descriptors)
for r,err in zip(result, errors):
assert_equal(r["success"], False)
assert_equal(r["error"]["message"], err)
Concept ACK. Left a suggestion that I think can make the test in 84bc7db250efb0fc253b2359a8ac879c6e3dd724 more robust.