importdescriptors: check for errors before rescanning #33655

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.

Reference: #31514, #32376, #33614

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:

diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp                                                                                                                      
index 4b4e432176..342f27dc13 100644                                                                                                                                                     
--- a/src/wallet/rpc/backup.cpp                                                                                                                                                         
+++ b/src/wallet/rpc/backup.cpp                                                                                                                                                         
@@ -137,7 +137,7 @@ static int64_t GetImportTimestamp(const UniValue& data, int64_t now)                                                                                                
     throw JSONRPCError(RPC_TYPE_ERROR, "Missing required timestamp field for key");                                                                                                    
 }                                                                                                                                                                                      
                                                                                                                                                                                        
-static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)                                     
+static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, const int64_t timestamp, bool validate_only = false) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {                                                                                          
     UniValue warnings(UniValue::VARR);      
     UniValue result(UniValue::VOBJ);
@@ -224,8 +224,9 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
             throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
         }
                                                                                            
+                                                                                           
         for (size_t j = 0; j < parsed_descs.size(); ++j) {      
-            auto parsed_desc = std::move(parsed_descs[j]);                                                                                                                             
+            const auto& parsed_desc = parsed_descs[j];
             if (parsed_descs.size() == 2) {
                 desc_internal = j == 1;
             } else if (parsed_descs.size() > 2) {
@@ -259,6 +260,20 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
                    warnings.push_back("Not all private keys provided. Some wallet functionality may return unexpected errors");
                }
             }
+        }
+        }
+        if (validate_only) {
+            result.pushKV("success", UniValue(true));
+            PushWarnings(warnings, result);
+            return result;
+        }
+        for (size_t j = 0; j < parsed_descs.size(); ++j) {
+            auto parsed_desc = std::move(parsed_descs[j]);
+            bool desc_internal = internal.has_value() && internal.value();
+            if (parsed_descs.size() == 2) {
+                desc_internal = j == 1;
+            } else if (parsed_descs.size() > 2) {
+                CHECK_NONFATAL(!desc_internal);
+            }
  
             WalletDescriptor w_desc(std::move(parsed_desc), timestamp, range_start, range_end, next_index);
  
@@ -379,6 +394,15 @@ RPCHelpMan importdescriptors()
  
         CHECK_NONFATAL(pwallet->chain().findBlock(pwallet->GetLastBlockHash(), FoundBlock().time(lowest_timestamp).mtpTime(now)));
  
+        for (const UniValue& request : requests.getValues()) {
+        const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
+        const UniValue result = ProcessDescriptorImport(*pwallet, request, timestamp, /*validate_only=*/true);
+        // If any validation fails, throw immediately and abort everything
+        if (!result["success"].get_bool()) {
+            throw result["error"];
+        }
+    }
+
         // Get all timestamps and extract the lowest timestamp

@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 ;-)

! 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