Skip to content

Avoid reading the old hd master key during wallet encryption#10115

Merged
laanwj merged 1 commit intobitcoin:masterfrom
TheBlueMatt:2017-03-cleanup-sethdmasterkey
May 3, 2017
Merged

Avoid reading the old hd master key during wallet encryption#10115
laanwj merged 1 commit intobitcoin:masterfrom
TheBlueMatt:2017-03-cleanup-sethdmasterkey

Conversation

@TheBlueMatt
Copy link
Copy Markdown
Contributor

This is a minor clenaup after #9294.

This makes SetHDMasterKey responsible for maintinaing the CHDChain
version instead of always creating it with the latest version and
making EncryptWallet responsible for keeping the version from
changing.

@jonasschnelli
Copy link
Copy Markdown
Contributor

utACK fdd15f770a649be57a1c6abc346cd4d0da831493

Copy link
Copy Markdown
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

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

couple of nits, but otherwise utACK fdd15f770a649be57a1c6abc346cd4d0da831493

Comment thread src/wallet/wallet.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: remove this unused variable in this commit (it was unused before your PR, but since you're here...)

Comment thread src/wallet/wallet.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion only: combine this with the line above to:

if (!SetHDMasterKey(GenerateNewHDMasterKey()))

Also: curly braces for single line if block please.

Comment thread src/wallet/wallet.h Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Couple of grammar/typos here. Suggest you replace with: The master key version is set to the current wallet version, so the caller must ensure that current wallet version is correct before calling this function. or similar

@TheBlueMatt TheBlueMatt force-pushed the 2017-03-cleanup-sethdmasterkey branch from fdd15f7 to e2da46d Compare April 13, 2017 15:19
@TheBlueMatt
Copy link
Copy Markdown
Contributor Author

Resolved @jnewbery's minor comments.

Comment thread src/wallet/wallet.h Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: you missed one of the grammar errors: s/keys/key's/ . There's only one of them.

(sorry - normally I wouldn't nit for grammar, but this could be misleading - hence my earlier suggestion to reword)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops, sorry, missed that one

This makes SetHDMasterKey responsible for maintinaing the CHDChain
version instead of always creating it with the latest version and
making EncryptWallet responsible for keeping the version from
changing.
@TheBlueMatt TheBlueMatt force-pushed the 2017-03-cleanup-sethdmasterkey branch from e2da46d to 185c7f0 Compare April 13, 2017 15:55
@jnewbery
Copy link
Copy Markdown
Contributor

utACK 185c7f0

@laanwj laanwj merged commit 185c7f0 into bitcoin:master May 3, 2017
laanwj added a commit that referenced this pull request May 3, 2017
…ption

185c7f0 Avoid reading the old hd master key during wallet encryption (Matt Corallo)

Tree-SHA512: b744e8490c0e948355bb77b2695902bb99f89a68af46aa2be9120bd2ccf3c340eb8a56340fec117f9a935192298028945c9b18120ee6b8b23e7da8ffdb635745
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants