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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
206+bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key, bool accept_no_keys)
207 {
208 {
209 LOCK(cs_KeyStore);
210- if (!SetCrypted())
211+ if (!mapKeys.empty())
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.
722@@ -732,11 +723,9 @@ class CWallet final : public WalletStorage, private interfaces::Chain::Notificat
723 {
724 // Should not have slots connected at this point.
725 assert(NotifyUnload.empty());
726- delete encrypted_batch;
727- encrypted_batch = nullptr;
728 }
729
730- bool IsCrypted() const { return fUseCrypto; }
731+ bool IsCrypted() const;
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.
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.
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#pullrequestreview-313636620, but IMO the current single commit is not hard to review from top to bottom once you basically understand what the code is doing.
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)
much more understandable split up, thank you.
utACK https://github.com/bitcoin/bitcoin/pull/17369/commits/5c8f0f81783b185f6dfa0797d91319f50e64324d
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()
