Refactor: Move encryption code between KeyMan and Wallet #17369

achow101 · 2019-11-04T21:09:19Z

Let wallet class handle locked/unlocked status and master key, and let keyman
handle encrypting its data and determining whether there is encrypted data.

There should be no change in behavior, but state is tracked differently. The
fUseCrypto atomic bool is eliminated and replaced with equivalent
HasEncryptionKeys checks.

Split from #17261

DrahtBot · 2019-11-04T23:29:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16910 (wallet: reduce loading time by using unordered maps 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.

ryanofsky

Code review ACK d96925bde5c103693e44787503dc3e45d1248da8

This PR does a lot of different things:

Adds new LegacyScriptPubKeyMan::CheckDecryptionKey() method containing most of the code that was previously part of CWallet::Unlock(CKeyingMaterial). CWallet::Unlock(CKeyingMaterial) now calls LegacyScriptPubKeyMan::CheckDecryptionKey() so there is no change in behavior.
- Sidebar 1: A little confusingly, there are two CWallet::Unlock methods: CWallet::Unlock(CKeyingMaterial) and CWallet::Unlock(SecureString) and the latter unlock calls the former unlock after finding and decrypting a "master key" in CWallet::mapMasterKeys associated with the SecureString passphrase.
- Sidebar 2: "master key" throughout this code does not refer to a BIP32 master key, but to a symmetric encryption key used to encrypt the BIP32 master key and other keys. The BIP32 master key is only kept in memory as a hashed "seed id" in LegacyScriptPubKeyMan::hdChain.seed_id, and is generally referred to in wallet code as an "hd seed" or "hd chain" instead of as a master key.
Moves fDecryptionThoroughlyChecked member variable from CWallet class to LegacyScriptPubKeyMan class. This has no effect on behavior and probably should have happened in #17260, but at the time code there was put together, encryption code was still in flux. LegacyScriptPubKeyMan::fDecryptionThoroughlyChecked is only used internally by LegacyScriptPubKeyMan::CheckDecryptionKey() to avoid repeating the same checks every time the wallet is unlocked.
Adds new CWallet::HasEncryptionKeys() method. HasEncyptionKeys() returns true if CWallet::mapMasterKeys is not empty, which is true if a wallet passphrase has been ever been set, even if the wallet doesn't have any seeds or keys or address data. Again, mapMasterKeys does NOT hold BIP32 master keys, but symmetric encryption keys (see Sidebar 2 above) encrypted with passphrases.
Removes the CWallet::fUseCrypto member variable and CWallet::SetCrypted() method and changes CWallet::IsCrypted() to return HasEncryptionKeys() instead of fUseCrypto. The meaning of fUseCrypto was not describable in any simple terms: It was false by default and true if CWallet::Lock or CWallet::Unlock(CKeyingMaterial) or LegacyScriptPubKeyMan::AddCryptedKeyInner had previously been called AND if FillableSigningProvider::mapKeys was empty. I wasn't able to determine if or why there aren't bugs in the current wallet code caused by IsCrypted() returning false when it would more correctly return true, but the new implementation of IsCrypted() in this PR seems vastly simpler and safer.
Replaces if (!IsCrypted()) checks with if (!m_storage.HasEncryptionKeys()) checks. After the change described in the previous point, this is equivalent, just replacing indirect calls with direct ones.
Replaces two if (!SetCrypted()) checks with if (!mapKeys.empty() checks. These checks are equivalent because SetCrypted() always returned true unless FillableSigningProvider::mapKeys had (unencrypted) keys. I believe these checks are impossible to trigger, and should probably be asserts instead. Otherwise they could use comments explaining what their purpose is.
Replaces CWallet::vMasterKey accesses with calls to a new method CWallet::GetEncryptionKey() returning the same value. This is just to avoid the key manager accessing a wallet variable. CWallet::vMasterKey holds a master decryption key decrypted with the passphrase and is only set while the wallet is unlocked.
Renames LegacyScriptPubKeyMan::EncryptKeys() to LegacyScriptPubKeyMan::Encrypt() for fun and entertainment.
Passes a new WalletBatch* batch argument to LegacyScriptPubKeyMan::Encrypt() which is used to set the LegacyScriptPubKeyMan::encrypted_batch member while the function is running. This is equivalent to the previous code because the previous caller, CWallet::EncryptWallet(), was setting the LegacyScriptPubKeyMan::encrypted_batch member itself, but is no longer doing that because as a wallet method, it shouldn't have access to key manager internals.
Tweaks LegacyScriptPubKeyMan::Encrypt to clear FillableSigningProvider::mapKeys /before/ encrypting keys rather than after. This is equivalent to previous behavior, and the change was only made because (as a new comment there states) it is necessary to make AddCryptedKeyInner() work now that SetCrypted() is gone and it is checking for FillableSigningProvider::mapKeys to be empty.

ryanofsky · 2019-11-07T21:06:32Z

src/wallet/scriptpubkeyman.cpp

In commit "Refactor: Move encryption code between KeyMan and Wallet" (d96925bde5c103693e44787503dc3e45d1248da8)

Would be great if this check and the same check in LegacyScriptPubKeyMan::AddCryptedKeyInner could be made into asserts or CHECK_NONFATAL's, because it seems like something would have to be very wrong for these to trigger. Or it would be helpful to have a comment saying when this is expected, if it is ever expected.

Changed to asserts.

ryanofsky · 2019-11-07T21:37:18Z

src/wallet/wallet.h

In commit "Refactor: Move encryption code between KeyMan and Wallet" (d96925bde5c103693e44787503dc3e45d1248da8)

Note: Future cleanup could delete this method and just have callers call HasEncryptionKeys() directly instead.

ryanofsky

Code review ACK 4b5c54b5be86890c0f46c2b3139014c8a88011a1. No change since last review other than rebase after trivial conflict with #15931

ryanofsky · 2019-11-15T15:28:49Z

Approach ACKs for this PR? Does it look good and reviewable? (Recommend to just look at the code and ignore my overlong description above which is mostly just notes for myself about previous behavior.)

I think the changes here are great and make wallet encryption state much easier to understand than before.

This PR could potentially be split up into multiple commits based on the bullet points in #17369 (review), but IMO the current single commit is not hard to review from top to bottom once you basically understand what the code is doing.

ryanofsky · 2019-11-21T15:27:44Z

@instagibbs and @meshcollider, you may be interested to review this. This is code you previously acked when it was part of #16341

instagibbs · 2019-11-22T16:53:57Z

I think splitting up into separate commits would be beneficial for the current and future readers.

ryanofsky

Code review ACK 49e1fbd016cc0ba4ad32ddd88f8ca0b38c41f3e9. No changes since last review other than rebase after a trivial conflict with #17371

achow101 · 2019-12-05T23:30:26Z

I've split this up into several commits each that does one or two bullets from the breakdown earlier. The final diff is identical.

(Accidentally screwed up the number of commits I was editing during the rebase and now the force push history is messed up, sorry)

instagibbs · 2019-12-06T15:52:32Z

much more understandable split up, thank you.

utACK 5c8f0f8

Adds functions in WalletStorage that allow ScriptPubKeyMans to check and get encryption keys from the wallet.

CWallet::Unlock is changed to call ScriptPubKeyMan::CheckDecryptionKey and the original implementation of Unlock is renamed to CheckDecryptionKey.

Does not change behavior. Needed to make AddCryptedKeyInner() work with SetCrypted() being gone.

Removes SetCrypted() and fUseCrypto as we don't need them anymore. SetCrypted calls in LegacyScriptPubKeyMan are replaced with mapKeys.empty() IsCrypted() is changed to just call HasEncryptionKeys()

…HasEncryptionKeys

laanwj · 2019-12-12T11:13:12Z

ACK 7cecf10

7cecf10 Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow) bf64171 Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow) 77a7771 Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow) 35f962f Clear mapKeys before encrypting (Andrew Chow) 14b5efd Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow) 97c0374 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow) e576b13 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow) fd9d6ee Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow) Pull request description: Let wallet class handle locked/unlocked status and master key, and let keyman handle encrypting its data and determining whether there is encrypted data. There should be no change in behavior, but state is tracked differently. The fUseCrypto atomic bool is eliminated and replaced with equivalent HasEncryptionKeys checks. Split from #17261 ACKs for top commit: laanwj: ACK 7cecf10 Tree-SHA512: 95a997c366ca539abba0c0a7a0015f39d27b55220683d8d86344ff2d926db4724da67700d2c8ec2d82ed75d07404318c6cb81544af8aadeefab312167257e673

…d Wallet 7cecf10 Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow) bf64171 Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow) 77a7771 Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow) 35f962f Clear mapKeys before encrypting (Andrew Chow) 14b5efd Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow) 97c0374 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow) e576b13 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow) fd9d6ee Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow) Pull request description: Let wallet class handle locked/unlocked status and master key, and let keyman handle encrypting its data and determining whether there is encrypted data. There should be no change in behavior, but state is tracked differently. The fUseCrypto atomic bool is eliminated and replaced with equivalent HasEncryptionKeys checks. Split from bitcoin#17261 ACKs for top commit: laanwj: ACK 7cecf10 Tree-SHA512: 95a997c366ca539abba0c0a7a0015f39d27b55220683d8d86344ff2d926db4724da67700d2c8ec2d82ed75d07404318c6cb81544af8aadeefab312167257e673

…llet Summary: 7cecf10ac32af0fca206ac5f24f482bdec88cb7d Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow) bf6417142f36a2f75b3a11368bd73fe788ae1ccb Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow) 77a777118eaf78f10a439810d1c08d510a539aa0 Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow) 35f962fcf0d5107ae6a3a9348e249a9b18ff7106 Clear mapKeys before encrypting (Andrew Chow) 14b5efd66ff0afbf3bf9158a724534a9090fc7fc Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow) 97c0374a46943b2ed38ea24eeeff1f1568dd55b3 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow) e576b135d6451101d6a8219f55d80aefa216dc38 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow) fd9d6eebc1eabb4675a118d19d38283da2dead39 Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow) Pull request description: Let wallet class handle locked/unlocked status and master key, and let keyman handle encrypting its data and determining whether there is encrypted data. There should be no change in behavior, but state is tracked differently. The fUseCrypto atomic bool is eliminated and replaced with equivalent HasEncryptionKeys checks. Split from #17261 --- Depends on D7697 Backport of Core [[bitcoin/bitcoin#17369 | PR17369]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7698

…d Wallet 7cecf10 Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys (Andrew Chow) bf64171 Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation (Andrew Chow) 77a7771 Rename EncryptKeys to Encrypt and pass in the encrypted batch to use (Andrew Chow) 35f962f Clear mapKeys before encrypting (Andrew Chow) 14b5efd Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan (Andrew Chow) 97c0374 Move Unlock implementation to LegacyScriptPubKeyMan (Andrew Chow) e576b13 Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() (Andrew Chow) fd9d6ee Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage (Andrew Chow) Pull request description: Let wallet class handle locked/unlocked status and master key, and let keyman handle encrypting its data and determining whether there is encrypted data. There should be no change in behavior, but state is tracked differently. The fUseCrypto atomic bool is eliminated and replaced with equivalent HasEncryptionKeys checks. Split from bitcoin#17261 ACKs for top commit: laanwj: ACK 7cecf10 Tree-SHA512: 95a997c366ca539abba0c0a7a0015f39d27b55220683d8d86344ff2d926db4724da67700d2c8ec2d82ed75d07404318c6cb81544af8aadeefab312167257e673

fanquake added Wallet Refactoring labels Nov 4, 2019

achow101 mentioned this pull request Nov 4, 2019

Make ScriptPubKeyMan an actual interface and the wallet to have multiple #17261

Merged

ryanofsky approved these changes Nov 7, 2019

View reviewed changes

ryanofsky mentioned this pull request Nov 7, 2019

wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet #17373

Merged

DrahtBot added the Needs rebase label Nov 8, 2019

achow101 force-pushed the wallet-box-pr-4 branch from d96925b to 4b5c54b Compare November 8, 2019 16:38

ryanofsky approved these changes Nov 8, 2019

View reviewed changes

DrahtBot removed the Needs rebase label Nov 8, 2019

ryanofsky mentioned this pull request Nov 14, 2019

Refactoring: Clarify code using encrypted_batch in CWallet #14144

Closed

DrahtBot added the Needs rebase label Nov 22, 2019

achow101 force-pushed the wallet-box-pr-4 branch from 4b5c54b to 49e1fbd Compare November 22, 2019 20:51

DrahtBot removed the Needs rebase label Nov 22, 2019

ryanofsky approved these changes Nov 25, 2019

View reviewed changes

achow101 mentioned this pull request Dec 4, 2019

wallet: include a checksum of encrypted private keys #16946

Merged

achow101 force-pushed the wallet-box-pr-4 branch from 49e1fbd to c079263 Compare December 5, 2019 23:29

achow101 force-pushed the wallet-box-pr-4 branch 3 times, most recently from 3a55e9e to 5c8f0f8 Compare December 6, 2019 00:30

DrahtBot added the Needs rebase label Dec 6, 2019

achow101 added 4 commits December 6, 2019 15:05

Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage

fd9d6ee

Adds functions in WalletStorage that allow ScriptPubKeyMans to check and get encryption keys from the wallet.

Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey()

e576b13

Move Unlock implementation to LegacyScriptPubKeyMan

97c0374

CWallet::Unlock is changed to call ScriptPubKeyMan::CheckDecryptionKey and the original implementation of Unlock is renamed to CheckDecryptionKey.

Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan

14b5efd

achow101 added 4 commits December 6, 2019 15:05

Clear mapKeys before encrypting

35f962f

Does not change behavior. Needed to make AddCryptedKeyInner() work with SetCrypted() being gone.

Rename EncryptKeys to Encrypt and pass in the encrypted batch to use

77a7771

Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation

bf64171

Removes SetCrypted() and fUseCrypto as we don't need them anymore. SetCrypted calls in LegacyScriptPubKeyMan are replaced with mapKeys.empty() IsCrypted() is changed to just call HasEncryptionKeys()

Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::…

7cecf10

…HasEncryptionKeys

achow101 force-pushed the wallet-box-pr-4 branch from 5c8f0f8 to 7cecf10 Compare December 6, 2019 20:08

DrahtBot removed the Needs rebase label Dec 6, 2019

laanwj merged commit 7cecf10 into bitcoin:master Dec 12, 2019

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Refactor: Move encryption code between KeyMan and Wallet #17369

Refactor: Move encryption code between KeyMan and Wallet #17369

Uh oh!

Conversation

achow101 commented Nov 4, 2019

Uh oh!

DrahtBot commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 15, 2019

Uh oh!

ryanofsky commented Nov 21, 2019

Uh oh!

instagibbs commented Nov 22, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Dec 6, 2019

Uh oh!

laanwj commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Nov 4, 2019 •

edited

Loading

ryanofsky Nov 7, 2019 •

edited

Loading

achow101 commented Dec 5, 2019 •

edited

Loading

laanwj commented Dec 12, 2019 •

edited

Loading