importdescriptors: check for errors before rescanning

pinheadmz commented at 12:49 pm on October 18, 2025: member

I tried to import two descriptors with labels but one was an internal descriptor. I didn’t realize that was not allowed until the entire rescan (for the first descriptor) completed and the return object included success false with that error message, for the second descriptor.

waketraindev commented at 4:40 pm on October 18, 2025: contributor

Reference: #31514, #32376, #33614

naiyoma commented at 11:52 am on November 5, 2025: contributor

Currently, if there are any validation errors, they’re added in the result and returned after a rescan . I think we should validate all descriptors first without modifying the wallet. If any descriptor fails validation, throw an error immediately and abort— no rescan. A possible fix for this:

 0diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp                                                                                                                      
 1index 4b4e432176..342f27dc13 100644                                                                                                                                                     
 2--- a/src/wallet/rpc/backup.cpp                                                                                                                                                         
 3+++ b/src/wallet/rpc/backup.cpp                                                                                                                                                         
 4@@ -137,7 +137,7 @@ static int64_t GetImportTimestamp(const UniValue& data, int64_t now)                                                                                                
 5     throw JSONRPCError(RPC_TYPE_ERROR, "Missing required timestamp field for key");                                                                                                    
 6 }                                                                                                                                                                                      
 7                                                                                                                                                                                        
 8-static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)                                     
 9+static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, const int64_t timestamp, bool validate_only = false) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
10 {                                                                                          
11     UniValue warnings(UniValue::VARR);      
12     UniValue result(UniValue::VOBJ);
13@@ -224,8 +224,9 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
14             throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
15         }
16                                                                                            
17+                                                                                           
18         for (size_t j = 0; j < parsed_descs.size(); ++j) {      
19-            auto parsed_desc = std::move(parsed_descs[j]);                                                                                                                             
20+            const auto& parsed_desc = parsed_descs[j];
21             if (parsed_descs.size() == 2) {
22                 desc_internal = j == 1;
23             } else if (parsed_descs.size() > 2) {
24@@ -259,6 +260,20 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
25                    warnings.push_back("Not all private keys provided. Some wallet functionality may return unexpected errors");
26                }
27             }
28+        }
29+        }
30+        if (validate_only) {
31+            result.pushKV("success", UniValue(true));
32+            PushWarnings(warnings, result);
33+            return result;
34+        }
35+        for (size_t j = 0; j < parsed_descs.size(); ++j) {
36+            auto parsed_desc = std::move(parsed_descs[j]);
37+            bool desc_internal = internal.has_value() && internal.value();
38+            if (parsed_descs.size() == 2) {
39+                desc_internal = j == 1;
40+            } else if (parsed_descs.size() > 2) {
41+                CHECK_NONFATAL(!desc_internal);
42+            }
43  
44             WalletDescriptor w_desc(std::move(parsed_desc), timestamp, range_start, range_end, next_index);
45  
46@@ -379,6 +394,15 @@ RPCHelpMan importdescriptors()
47  
48         CHECK_NONFATAL(pwallet->chain().findBlock(pwallet->GetLastBlockHash(), FoundBlock().time(lowest_timestamp).mtpTime(now)));
49  
50+        for (const UniValue& request : requests.getValues()) {
51+        const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
52+        const UniValue result = ProcessDescriptorImport(*pwallet, request, timestamp, /*validate_only=*/true);
53+        // If any validation fails, throw immediately and abort everything
54+        if (!result["success"].get_bool()) {
55+            throw result["error"];
56+        }
57+    }
58+
59         // Get all timestamps and extract the lowest timestamp

pinheadmz commented at 2:09 pm on November 5, 2025: member

@naiyoma thanks for taking an interest in this and digging in! I think a cleaner approach would be to separate the validation into a separate function entirely, so we validate in a loop first then process in a second loop. It smells better than adding an extra argument and feels more bitcoin-core-y. If you open a PR like that I’ll help you get it merged ;-)

naiyoma commented at 12:14 pm on November 6, 2025: contributor

! I think a cleaner approach would be to separate the validation into a separate function entirely, so we validate in a loop first then process in a second loop. It smells better than adding an extra argument and feels more bitcoin-core-y. If you open a PR like that I’ll help you get it merged ;-)

great, I’ll work on this

importdescriptors: check for errors before rescanning #33655