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

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:uninitialized-members changing 2 files +5 −5

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.
wallet: Make sure no DescriptorScriptPubKeyMan members are uninitialized after construction ff046aeeba
wallet: Make sure no WalletDescriptor members are uninitialized after construction 2a78098098
instagibbs commented at 2:31 PM on April 27, 2020: member

utACK https://github.com/bitcoin/bitcoin/pull/18782/commits/2a780980983f4b4aaae75817e57e7ed308713561
practicalswift cross-referenced this on Apr 27, 2020 from issue Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101
fjahr commented at 2:40 PM on April 27, 2020: contributor

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

cc @achow101
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.
DrahtBot added the label Wallet on Apr 27, 2020
MarcoFalke commented at 3:48 PM on April 27, 2020: member

would it catch if we had forgotten READWRITE(next_index)

It wouldn't
achow101 commented at 4:05 PM on April 27, 2020: member

ACK 2a780980983f4b4aaae75817e57e7ed308713561
practicalswift commented at 4:54 PM on April 27, 2020: contributor

Also: what about OutputType m_address_type (in DescriptorScriptPubKeyMan)? :)
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.
achow101 cross-referenced this on Apr 27, 2020 from issue wallet: descriptor wallet release notes and cleanups by achow101
DrahtBot commented at 11:07 PM on April 27, 2020: 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:
- #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.
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 :)
brakmic commented at 10:24 AM on May 1, 2020: contributor

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

utACK 2a780980983f4b4aaae75817e57e7ed308713561
meshcollider merged this on May 5, 2020
meshcollider closed this on May 5, 2020
sidhujag referenced this in commit 31e63beaae on May 5, 2020
meshcollider referenced this in commit df303ceb65 on May 22, 2020
sidhujag referenced this in commit 5c5677cfef on May 22, 2020
Fabcien referenced this in commit b740449f35 on Jan 27, 2021
practicalswift deleted the branch on Apr 10, 2021
bitcoin locked this on Aug 16, 2022