wallet: Use util::Error throughout AddWalletDescriptor instead of returning nullptr for some errors #32475

1. 
   achow101 commented at 1:28 am on May 13, 2025: member

   #32023 changed AddWalletDescriptor to return util::Error, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use util::Error so that all callers handle AddWalletDescriptor errors in the same way.

   The encapsulated return type is changed from ScriptPubKeyMan* to std::reference_wrapper<DescriptorScriptPubKeyMan>. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of ScriptPubKeyMan that can come out of AddWalletDescriptor is a DescriptorScriptPubKeyMan anyways.

2. 
   DrahtBot commented at 1:28 am on May 13, 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/32475.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	ryanofsky, furszy, Sjors
   Concept ACK	pablomartin4btc
   Stale ACK	w0xlt

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
   • #32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
   • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
   • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
   • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory 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.

3. DrahtBot added the label Wallet on May 13, 2025
4. 
   fanquake commented at 8:32 am on May 13, 2025: member

   0 /Users/runner/work/bitcoin/bitcoin/src/wallet/test/wallet_tests.cpp:69:10: error: unused variable 'spk_manager' [-Werror,-Wunused-variable]
   1    auto spk_manager = *Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false));
   2         ^
   31 error generated.
   

5. fanquake requested review from furszy on May 13, 2025
6. achow101 force-pushed on May 13, 2025
7. 
   achow101 commented at 8:19 pm on May 13, 2025: member

   CI should be fixed
8. 
   w0xlt commented at 6:33 pm on May 15, 2025: contributor

   ACK https://github.com/bitcoin/bitcoin/pull/32475/commits/f17945b347f6a46dee3b56f86a557eaccec1bc72
9. 
   furszy commented at 8:15 pm on May 15, 2025: member

   Code review ACK f17945b347f6a46dee3b56f86a557eaccec1bc72

   Interesting that no CI job was failing. It smells we need more tests.

10. DrahtBot added the label Needs rebase on May 16, 2025
11. achow101 force-pushed on May 16, 2025
12. DrahtBot removed the label Needs rebase on May 16, 2025
13. in src/wallet/test/util.h:119 in 218db39973 outdated

    115@@ -116,7 +116,7 @@ class MockableDatabase : public WalletDatabase
    116 std::unique_ptr<WalletDatabase> CreateMockableWalletDatabase(MockableData records = {});
    117 MockableDatabase& GetMockableDatabase(CWallet& wallet);
    118 
    119-ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success);
    120+std::optional<std::reference_wrapper<DescriptorScriptPubKeyMan>> CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success);
    

    ---

    ---

    

    ryanofsky commented at 8:23 pm on May 19, 2025:

    In commit “wallet: Use util::Error throughout AddWalletDescriptor” (218db39973736ac50e912bec440b1e512586d3b0)

    I think I understand the need to use std::reference_wrapper with util::Result. But it would seem good to replace std::optional<std::reference_wrapper<DescriptorScriptPubKeyMan>> here and other places with just DescriptorScriptPubKeyMan* for simplicity and avoid reference_wrapper except in the cases where it’s really needed. It seems like doing this would make a lot of the test changes here unnecessary and the tests themselves more readable.

    I think use of reference_wrapper could to go away later with #25665 and normal references could be used.

    ---

    

    achow101 commented at 1:12 am on May 20, 2025:

    Done as suggested

    ---

    

    pablomartin4btc commented at 3:07 pm on May 20, 2025:

    I guess reference_wrapper then is still needed in CWallet::AddWalletDescriptor?

    ---

    

    ryanofsky commented at 4:01 pm on May 20, 2025:

    re: #32475 (review)

    I guess reference_wrapper then is still needed in CWallet::AddWalletDescriptor?

    Yes it’s still probably better to use reference_wrapper there instead of using a pointer type which could be null. I think more ideally util::Result would support reference types, but it doesn’t currently. (When I wrote previous comment I though #25665 would allow util::Result to support reference types, but that’s not actually true. It could help, but would not be sufficient on its own because the implementation uses a union and references aren’t allowed in unions.)

14. 
    ryanofsky commented at 8:24 pm on May 19, 2025: contributor

    Concept ACK 218db39973736ac50e912bec440b1e512586d3b0
15. wallet: Use util::Error throughout AddWalletDescriptor

    32023 changed AddWalletDescriptor to return util::Error, but did not
    change all of the failure cases to do so. This may result in some
    callers continuing when there was actually an error. Unify all of the
    failure cases to use util::Error so that all callers handle
    AddWalletDescriptor errors in the same way.
    
    The encapsulated return type is changed from ScriptPubKeyMan* to
    std::reference_wrapper<DescriptorScriptPubKeyMan>. This avoids having a
    value that can be interpreted as a bool, and also removes the need to
    constantly dynamic_cast the returned value. The only kind of
    ScriptPubKeyMan that can come out of AddWalletDescriptor is a
    DescriptorScriptPubKeyMan anyways.

    785e1407b0
16. achow101 force-pushed on May 20, 2025
17. ryanofsky approved
18. 
    ryanofsky commented at 3:03 pm on May 20, 2025: contributor

    Code review ACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
19. DrahtBot requested review from w0xlt on May 20, 2025
20. DrahtBot requested review from furszy on May 20, 2025
21. 
    pablomartin4btc commented at 3:06 pm on May 20, 2025: member

    Concept ACK
22. 
    furszy commented at 5:20 pm on May 20, 2025: member

    Code review ACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
23. DrahtBot requested review from pablomartin4btc on May 20, 2025
24. 
    Sjors commented at 11:54 am on May 21, 2025: member

    utACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
25. fanquake merged this on May 21, 2025
26. fanquake closed this on May 21, 2025

    ---

Contributors
achow101 DrahtBot fanquake w0xlt furszy ryanofsky pablomartin4btc Sjors

Review Requested
w0xlt pablomartin4btc

Labels
Wallet