Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
meshcollider merged 2 commits into from
May 5, 2020
Merged

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

meshcollider merged 2 commits into from
May 5, 2020

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Apr 27, 2020 •
edited
Loading

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.

@instagibbs
Copy link
Member

instagibbs commented Apr 27, 2020

utACK 2a78098

@practicalswift practicalswift mentioned this pull request Apr 27, 2020
Merged
@fjahr
Copy link
Contributor

fjahr commented Apr 27, 2020

Code review ACK 2a78098

@maflcko
Copy link
Member

maflcko commented Apr 27, 2020

cc @achow101

@Sjors
Copy link
Member

Sjors commented Apr 27, 2020 •
edited
Loading

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.

@maflcko
Copy link
Member

maflcko commented Apr 27, 2020

would it catch if we had forgotten READWRITE(next_index)

It wouldn't

@achow101
Copy link
Member

achow101 commented Apr 27, 2020

ACK 2a78098

@practicalswift
Copy link
Contributor Author

practicalswift commented Apr 27, 2020 •
edited
Loading

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

@achow101
Copy link
Member

achow101 commented Apr 27, 2020

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 achow101 mentioned this pull request Apr 27, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 27, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Contributor Author

practicalswift commented Apr 28, 2020 •
edited
Loading

@achow101 Makes sense (saw #18787)! Thanks! Then I think this PR should be ready to go :)

@brakmic
Copy link
Contributor

brakmic commented May 1, 2020

Code review ACK 2a78098

meshcollider
meshcollider reviewed May 5, 2020
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 2a78098

@meshcollider meshcollider merged commit ec79b5f into bitcoin:master May 5, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 5, 2020
@meshcollider
Merge bitcoin#18782: wallet: Make sure no DescriptorScriptPubKeyMan o…
31e63be 
…r WalletDescriptor members are left uninitialized after construction

2a78098 wallet: Make sure no WalletDescriptor members are uninitialized after construction (practicalswift)
ff046ae wallet: Make sure no DescriptorScriptPubKeyMan members are uninitialized after construction (practicalswift)

Pull request description:

  This is a small folllow-up to bitcoin#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.

ACKs for top commit:
  instagibbs:
    utACK  bitcoin@2a78098
  fjahr:
    Code review ACK 2a78098
  Sjors:
    utACK 2a78098
  achow101:
    ACK 2a78098
  brakmic:
    Code review ACK 2a78098
  meshcollider:
    utACK 2a78098

Tree-SHA512: c98e035268fdc7f65a423b73ac0cf010b0ef7c5e679b3cf170c1813efac8ab5c657dcbaf43c746770bea59e4772bfefe4caa834f1175260c39c7f35d92946ba5
meshcollider added a commit that referenced this pull request May 22, 2020
@meshcollider
Merge #18787: wallet: descriptor wallet release notes and cleanups
df303ce 
ca2a096 Change SetType to SetInternal and remove m_address_type (Andrew Chow)
89b1ce1 Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan (Andrew Chow)
b9073c8 rpc: createwallet warning that descriptor wallets are experimental (Andrew Chow)
610030d docs: Add release notes for descriptor wallets (Andrew Chow)

Pull request description:

  Some docs and cleanup following #16528.

  * Added release notes to explain a bit of motivation for descriptor wallets, what was changed, and how users will be effected by it. Also mentions the caveats regarding multsigs and watchonly that we have discussed on IRC.
  * Adds a warning to `createwallet` that descriptor wallets are experimental.
  * Removed unused `SetCrypted` as suggestioned: #16528 (comment)
  * Removed `m_address_type` as mentioned in #18782 (comment)

ACKs for top commit:
  Sjors:
    tACK ca2a096
  instagibbs:
    utACK ca2a096
  meshcollider:
    utACK ca2a096

Tree-SHA512: 987188a912c191430e5d3f89bcef54ba6773692fc2d95b16a3ec11d9007ded210466ed980a3857e8b7196beef6422f07f9c85cc157f996c02d16f4dbde2e7b2a
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 22, 2020
@meshcollider
Merge bitcoin#18787: wallet: descriptor wallet release notes and clea…
5c5677c 
…nups

ca2a096 Change SetType to SetInternal and remove m_address_type (Andrew Chow)
89b1ce1 Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan (Andrew Chow)
b9073c8 rpc: createwallet warning that descriptor wallets are experimental (Andrew Chow)
610030d docs: Add release notes for descriptor wallets (Andrew Chow)

Pull request description:

  Some docs and cleanup following bitcoin#16528.

  * Added release notes to explain a bit of motivation for descriptor wallets, what was changed, and how users will be effected by it. Also mentions the caveats regarding multsigs and watchonly that we have discussed on IRC.
  * Adds a warning to `createwallet` that descriptor wallets are experimental.
  * Removed unused `SetCrypted` as suggestioned: bitcoin#16528 (comment)
  * Removed `m_address_type` as mentioned in bitcoin#18782 (comment)

ACKs for top commit:
  Sjors:
    tACK ca2a096
  instagibbs:
    utACK bitcoin@ca2a096
  meshcollider:
    utACK ca2a096

Tree-SHA512: 987188a912c191430e5d3f89bcef54ba6773692fc2d95b16a3ec11d9007ded210466ed980a3857e8b7196beef6422f07f9c85cc157f996c02d16f4dbde2e7b2a
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 27, 2021
@practicalswift @PiRK
wallet: Make sure no DescriptorScriptPubKeyMan or WalletDescriptor me…
b740449 
…mbers are left uninitialized after construction

Summary:
> This is a small folllow-up to [[bitcoin/bitcoin#16528 | PR16528]] ("Native Descriptor Wallets using DescriptorScriptPubKeyMan")
> 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
bitcoin/bitcoin@ff046ae

wallet: Make sure no WalletDescriptor members are uninitialized after construction
bitcoin/bitcoin@2a78098

This is a backport of Core [[bitcoin/bitcoin#18782 | PR18782]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9073
@practicalswift practicalswift deleted the uninitialized-members branch April 10, 2021 19:41
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@meshcollider meshcollider meshcollider left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@practicalswift @instagibbs @fjahr @maflcko @Sjors @achow101 @DrahtBot @brakmic @meshcollider