wallet: refactor ProcessDescriptorImport #33874

1. 
   naiyoma commented at 6:15 pm on November 14, 2025: contributor

   Fixes: #33655

   Currently, we validate and import descriptors in the same loop. If any descriptor fails validation, an error is added to the response array, but the complete response (including that error) is only returned to after a rescan completes—potentially wasting significant time when validation error could have been caught immediately.

   This PR separates validation from processing: validate all descriptors first, and process in the second loop

2. DrahtBot added the label Wallet on Nov 14, 2025
3. 
   DrahtBot commented at 6:15 pm on November 14, 2025: contributor

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

   Code Coverage & Benchmarks

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

   Reviews

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)
   • #31668 (Added rescan option for import descriptors by saikiran57)
   • #29136 (wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor by achow101)

   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.

4. DrahtBot added the label CI failed on Nov 14, 2025
5. naiyoma marked this as a draft on Nov 14, 2025
6. naiyoma renamed this:
   wallet: refactor ProcessDescriptorImport , validate descriptors before rescan
   wallet: refactor ProcessDescriptorImport
   on Nov 14, 2025
7. DrahtBot renamed this:
   wallet: refactor ProcessDescriptorImport
   wallet: refactor ProcessDescriptorImport
   on Nov 14, 2025
8. naiyoma force-pushed on Nov 17, 2025
9. wallet:refactor ProcessDescriptorImport separate validation logic 04e6704029
10. naiyoma force-pushed on Nov 18, 2025
11. DrahtBot removed the label CI failed on Nov 18, 2025
12. in src/wallet/rpc/backup.cpp:418 in 04e6704029

    415-
    416-        // Get all timestamps and extract the lowest timestamp
    417         for (const UniValue& request : requests.getValues()) {
    418+            DescriptorImportData validated = ValidateDescriptorImport(*pwallet, request);
    419+            UniValue result(UniValue::VOBJ);
    420+            result.pushKV("success", UniValue(validated.success));
    

    ---

    ---

    

    achow101 commented at 10:06 pm on November 19, 2025:

    This is dangerous, we should never return success: true if the descriptor hasn’t been fully processed yet.

    Given how this RPC behaved previously, this can lead to users thinking that their descriptor has actually been imported when it was not.

13. 
    achow101 commented at 10:10 pm on November 19, 2025: member

    Concept meh

    importdescriptors was modeled after the old importmulti, which IIRC part of the point was that even if some things couldn’t be imported that everything else that can be imported was.

    But I do see how exiting early and doing nothing if something can’t be imported could be useful, but this is pretty big semantic change and we need to be careful about return values to avoid any possible confusion.

Contributors
naiyoma DrahtBot achow101

Labels
Wallet