refactor: Return BResult from restoreWallet #25594

1. 
   MarcoFalke commented at 1:37 pm on July 12, 2022: member

   This avoids the error in-out param (and if warnings is added to BResult, it will avoid passing that in-out param as well).

   Also, as it is needed for this change, prepare BResult for non-copyable types.

2. MarcoFalke force-pushed on Jul 12, 2022
3. fanquake added the label Wallet on Jul 12, 2022
4. w0xlt approved
5. 
   w0xlt commented at 1:58 pm on July 12, 2022: contributor

   Tested ACK https://github.com/bitcoin/bitcoin/pull/25594/commits/fa869c6df15a54dae0e8d6ba385514801ddd345f

   Perhaps this change can be split into two commits: one for BResult and one for restoreWallet.

6. MarcoFalke force-pushed on Jul 12, 2022
7. Prepare BResult for non-copyable types fa8de09edc
8. refactor: Return BResult from restoreWallet fa475e9c79
9. MarcoFalke force-pushed on Jul 12, 2022
10. in src/util/result.h:22 in fa8de09edc outdated

    18@@ -18,9 +19,9 @@ class BResult {
    19     std::variant<bilingual_str, T> m_variant;
    20 
    21 public:
    22-    BResult() : m_variant(Untranslated("")) {}
    23-    BResult(const T& _obj) : m_variant(_obj) {}
    

    ---

    ---

    

    ryanofsky commented at 6:52 pm on July 12, 2022:

    In commit “refactor: Return BResult from restoreWallet” (fa869c6df15a54dae0e8d6ba385514801ddd345f)

    I think dropping const T& constructor drops support for noncopyable and nonmovable objects.

    Maybe want:

    0BResult(const T& obj) : m_variant{obj} {}
    1BResult(T&& obj) : m_variant{std::move(obj)} {}
    

    ---

    

    MarcoFalke commented at 7:22 am on July 13, 2022:

    drops support

    This was never supported, unless I am missing something?

    Regardless, it seems wrong to return memory that is not owned. Though, if you really want to, it might be better to make the type T explicit. That is, make it a a raw pointer/std::ref/span/string_view/whatever.

    ---

    

    ryanofsky commented at 12:46 pm on July 13, 2022:

    drops support

    This was never supported, unless I am missing something?

    You’re right. It didn’t work previously or in the version I suggested either. It would be possible, but would have to call one of the std::in_place_* variant constructors here.

    Regardless, it seems wrong to return memory that is not owned.

    The idea would not to have BResult point to an outside value but for BResult to contain a value that is constructed in-place. But regardless, this is an edge case not worth considering here. I might support with it in my followup for #25308 and add a test if I do

11. ryanofsky approved
12. 
    ryanofsky commented at 6:56 pm on July 12, 2022: contributor

    Code review ACK fa869c6df15a54dae0e8d6ba385514801ddd345f
13. w0xlt approved
14. 
    w0xlt commented at 7:19 pm on July 12, 2022: contributor

    reACK https://github.com/bitcoin/bitcoin/pull/25594/commits/fa475e9c7977a952617738f2ee8cf600c07d4df8

    The previous commit, although it failed some CI checks, had worked on Ubuntu 21.10 clang version 13.0.0-2.

    Maybe shared_ptr<CWallet> src/wallet/wallet.h:std::RestoreWallet(..,bilingual_str& error,...) might also be changed to BResult<CWallet> src/wallet/wallet.h:std::RestoreWallet(..)

15. 
    w0xlt commented at 2:49 am on July 13, 2022: contributor

    I proposed some changes to BResult in #25601 that might make it easier to use in src/wallet/wallet.h:{RestoreWallet(...),CreateWallet(...),LoadWallet(...)}.
16. 
    MarcoFalke commented at 7:24 am on July 13, 2022: member

    Maybe shared_ptr src/wallet/wallet.h:std::RestoreWallet(..,bilingual_str& error,…) might also be changed to BResult src/wallet/wallet.h:std::RestoreWallet(..)

    Good idea. Feel free to ping me for review on a follow-up. For now I am trying to keep the changes here to a minimum.

17. ryanofsky approved
18. 
    ryanofsky commented at 12:50 pm on July 13, 2022: contributor

    Code review ACK fa475e9c7977a952617738f2ee8cf600c07d4df8. Changes since last review were replacing auto with explicit type and splitting commits
19. 
    DrahtBot commented at 4:07 am on July 14, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25608 (BResult improvements by ryanofsky)

    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.

20. MarcoFalke merged this on Jul 14, 2022
21. MarcoFalke closed this on Jul 14, 2022

    ---

22. bitcoin deleted a comment on Jul 14, 2022
23. MarcoFalke deleted the branch on Jul 14, 2022
24. sidhujag referenced this in commit b6ff16e53b on Jul 14, 2022
25. MarcoFalke referenced this in commit f5e96ecef5 on Aug 10, 2022
26. sidhujag referenced this in commit 8ba56bdfd0 on Aug 11, 2022
27. bitcoin locked this on Jul 16, 2023

Contributors
MarcoFalke w0xlt ryanofsky DrahtBot

Labels
Wallet