wallet: Make sure no DescriptorScriptPubKeyMan or WalletDescriptor members are left uninitialized after construction #18782

1. 
   practicalswift commented at 2:29 pm on April 27, 2020: contributor

   This is a small folllow-up to #16528 (“Native Descriptor Wallets using DescriptorScriptPubKeyMan”) which was merged in to master a couple of hours ago.

   Make sure no DescriptorScriptPubKeyMan or WalletDescriptor members are left uninitialized after construction.

   Before this change bool m_internal was left uninitialized when using the DescriptorScriptPubKeyMan(WalletStorage&, WalletDescriptor&) ctor.

   The same goes for the now initialized integers which were left uninitialized when using the WalletDescriptor() ctor.

2. wallet: Make sure no DescriptorScriptPubKeyMan members are uninitialized after construction ff046aeeba
3. wallet: Make sure no WalletDescriptor members are uninitialized after construction 2a78098098
4. 
   instagibbs commented at 2:31 pm on April 27, 2020: member

   utACK https://github.com/bitcoin/bitcoin/pull/18782/commits/2a780980983f4b4aaae75817e57e7ed308713561
5. 
   fjahr commented at 2:40 pm on April 27, 2020: member

   Code review ACK 2a780980983f4b4aaae75817e57e7ed308713561
6. 
   MarcoFalke commented at 2:54 pm on April 27, 2020: member

   cc @achow101
7. 
   Sjors commented at 2:59 pm on April 27, 2020: member

   utACK 2a78098

   The change in DescriptorScriptPubKeyMan is consistent with LegacyScriptPubKeyMan which initialises everything.

   As for WalletDescriptor, there’s not really a pattern for classes that use ADD_SERIALIZE_METHODS which variables we do and don’t initialise. Afaik we only use the empty WalletDescriptor() constructor when loading a database record. In that case everything is initialised with READWRITE.

   I don’t know if valgrind tools can reason through that; e.g, would it catch if we had forgotten READWRITE(next_index)? If so then maybe not initialising is smart (see recurring discussion elsewhere). But if it wouldn’t catch that bug, then I don’t see any downside to initialising them right away.

8. DrahtBot added the label Wallet on Apr 27, 2020
9. 
   MarcoFalke commented at 3:48 pm on April 27, 2020: member

   would it catch if we had forgotten READWRITE(next_index)

   It wouldn’t

10. 
    achow101 commented at 4:05 pm on April 27, 2020: member

    ACK 2a780980983f4b4aaae75817e57e7ed308713561
11. 
    practicalswift commented at 4:54 pm on April 27, 2020: contributor

    Also: what about OutputType m_address_type (in DescriptorScriptPubKeyMan)? :)
12. 
    achow101 commented at 6:50 pm on April 27, 2020: member

    Also: what about OutputType m_address_type (in DescriptorScriptPubKeyMan)? :)

    I don’t think giving that a default really makes sense. Maybe we should just infer the type from the descriptor instead as descriptors have a GetOutputType. In fact, I think m_address_type could be eliminated entirely. I’ll investigate.

13. 
    DrahtBot commented at 11:07 pm on April 27, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18787 (wallet: descriptor wallet release notes and cleanups 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.

14. 
    practicalswift commented at 6:17 am on April 28, 2020: contributor

    @achow101 Makes sense (saw #18787)! Thanks! Then I think this PR should be ready to go :)
15. 
    brakmic commented at 10:24 am on May 1, 2020: contributor

    Code review ACK 2a780980983f4b4aaae75817e57e7ed308713561
16. 
    meshcollider commented at 3:42 am on May 5, 2020: contributor

    utACK 2a780980983f4b4aaae75817e57e7ed308713561
17. meshcollider merged this on May 5, 2020
18. meshcollider closed this on May 5, 2020

    ---

19. sidhujag referenced this in commit 31e63beaae on May 5, 2020
20. meshcollider referenced this in commit df303ceb65 on May 22, 2020
21. sidhujag referenced this in commit 5c5677cfef on May 22, 2020
22. Fabcien referenced this in commit b740449f35 on Jan 27, 2021
23. practicalswift deleted the branch on Apr 10, 2021
24. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift instagibbs fjahr MarcoFalke Sjors achow101 DrahtBot brakmic meshcollider

Labels
Wallet