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.
| Type | Reviewers |
|---|---|
| ACK | nebula-21, Bicaru20, brunoerg |
| Stale ACK | davidgumberg |
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>
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)
taken with a few tweeks
Concept ACK. Left a suggestion that I think can make the test in 84bc7db250efb0fc253b2359a8ac879c6e3dd724 more robust.
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/30255489290/job/89942991968</sub>
<sub>LLM reason (✨ experimental): CI failed because the Python lint step (ruff) reported an error (E712) in test/functional/wallet_importdescriptors.py.</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
125 | @@ -118,6 +126,45 @@ def test_import_unused_noprivs(self): 126 | wallet=wallet) 127 | wallet.unloadwallet() 128 | 129 | + def test_per_item_errors_are_reported_in_order(self): 130 | + self.log.info("Test that imports results are in the same order as the original request")
self.log.info("Test that import results are in the same order as the original request")
136 | + "timestamp": "now" 137 | + }, [False, "Descriptor not found."]), 138 | + ({ 139 | + "desc": descsum_create(f"pkh({get_generate_key().privkey})"), 140 | + "timestamp": 1, 141 | + "label": "Valid descriptor1",
"label": "Valid descriptor 1",
ACK e9570adcf8efd087e1ea4fe48fb9765056e9cf3e
left two nits
ACK e9570adcf8.
Commit e9570adcf8efd087e1ea4fe48fb9765056e9cf3e can be tested with the following mutant:
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 396be62825..d38ab9f76e 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -477,6 +477,13 @@ RPCMethod importdescriptors()
}
}
+ std::vector<UniValue> reordered = response.getValues();
+ response.clear();
+ response.setArray();
+ for (auto it = reordered.rbegin(); it != reordered.rend(); ++it) {
+ response.push_back(*it);
+ }
+
return response;
},
};
Just ran all the tests and the added one in e9570adcf8efd087e1ea4fe48fb9765056e9cf3e is the only one that detects it.
227 | @@ -220,6 +228,25 @@ def run_test(self): 228 | error_code=-8, 229 | error_message='Descriptor not found.') 230 | 231 | + # # Test import fails if one timestamp is invalid or missing
nit:
-# #
+#
ACK e9570adcf8efd087e1ea4fe48fb9765056e9cf3e
happy to re-ack if nits are addressed.
@polespinasa do you wanna fix the typos here?
Also adds global_error to test_importdesc to make it able to test per-item errors or global RPC errors
Co-Authored-By: Bicaru20 <bicaru2@gmail.com>
Force pushed to just fix the typos
ACK 3ac8b806a699a1d518ce225b39aa504f84b4cc06
re-ACK 3ac8b806a699a1d518ce225b39aa504f84b4cc06
reACK 3ac8b806a699a1d518ce225b39aa504f84b4cc06