Refactor: Move encryption code between KeyMan and Wallet #17369

pull achow101 wants to merge 8 commits into bitcoin:master from achow101:wallet-box-pr-4 changing 4 files +75 −64
  1. achow101 commented at 9:09 pm on November 4, 2019: member

    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

  2. fanquake added the label Wallet on Nov 4, 2019
  3. fanquake added the label Refactoring on Nov 4, 2019
  4. DrahtBot commented at 11:29 pm on November 4, 2019: member

    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.

  5. ryanofsky added this to the "PRs" column in a project

  6. in src/wallet/scriptpubkeyman.cpp:209 in d96925bde5 outdated
    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())
    


    ryanofsky commented at 9:06 pm on November 7, 2019:

    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.


    achow101 commented at 0:30 am on December 6, 2019:
    Changed to asserts.
  7. in src/wallet/wallet.h:742 in d96925bde5 outdated
    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;
    


    ryanofsky commented at 9:37 pm on November 7, 2019:

    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.

  8. ryanofsky approved
  9. ryanofsky commented at 9:37 pm on November 7, 2019: member

    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.

  10. DrahtBot added the label Needs rebase on Nov 8, 2019
  11. achow101 force-pushed on Nov 8, 2019
  12. ryanofsky approved
  13. ryanofsky commented at 5:05 pm on November 8, 2019: member
    Code review ACK 4b5c54b5be86890c0f46c2b3139014c8a88011a1. No change since last review other than rebase after trivial conflict with #15931
  14. DrahtBot removed the label Needs rebase on Nov 8, 2019
  15. ryanofsky commented at 3:28 pm on November 15, 2019: member

    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.

  16. ryanofsky commented at 3:27 pm on November 21, 2019: member
    @instagibbs and @meshcollider, you may be interested to review this. This is code you previously acked when it was part of #16341
  17. instagibbs commented at 4:53 pm on November 22, 2019: member
    I think splitting up into separate commits would be beneficial for the current and future readers.
  18. DrahtBot added the label Needs rebase on Nov 22, 2019
  19. achow101 force-pushed on Nov 22, 2019
  20. DrahtBot removed the label Needs rebase on Nov 22, 2019
  21. ryanofsky approved
  22. ryanofsky commented at 11:06 pm on November 25, 2019: member
    Code review ACK 49e1fbd016cc0ba4ad32ddd88f8ca0b38c41f3e9. No changes since last review other than rebase after a trivial conflict with #17371
  23. ryanofsky moved this from the "PRs" to the "Done" column in a project

  24. ryanofsky moved this from the "Done" to the "PRs" column in a project

  25. achow101 force-pushed on Dec 5, 2019
  26. achow101 commented at 11:30 pm on December 5, 2019: member

    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)

  27. achow101 force-pushed on Dec 5, 2019
  28. achow101 force-pushed on Dec 5, 2019
  29. achow101 force-pushed on Dec 6, 2019
  30. instagibbs commented at 3:52 pm on December 6, 2019: member
  31. DrahtBot added the label Needs rebase on Dec 6, 2019
  32. Add GetEncryptionKey() and HasEncryptionKeys() to WalletStorage
    Adds functions in WalletStorage that allow ScriptPubKeyMans to check
    and get encryption keys from the wallet.
    fd9d6eebc1
  33. Replace LegacyScriptPubKeyMan::vMasterKey with GetDecryptionKey() e576b135d6
  34. Move Unlock implementation to LegacyScriptPubKeyMan
    CWallet::Unlock is changed to call ScriptPubKeyMan::CheckDecryptionKey
    and the original implementation of Unlock is renamed to CheckDecryptionKey.
    97c0374a46
  35. Move fDecryptionThoroughlyChecked from CWallet to LegacyScriptPubKeyMan 14b5efd66f
  36. Clear mapKeys before encrypting
    Does not change behavior. Needed to make AddCryptedKeyInner() work
    with SetCrypted() being gone.
    35f962fcf0
  37. Rename EncryptKeys to Encrypt and pass in the encrypted batch to use 77a777118e
  38. Remove SetCrypted() and fUseCrypto; Change IsCrypted()'s implementation
    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()
    bf6417142f
  39. Replace LegacyScriptPubKeyMan::IsCrypted with LegacyScriptPubKeyMan::HasEncryptionKeys 7cecf10ac3
  40. achow101 force-pushed on Dec 6, 2019
  41. DrahtBot removed the label Needs rebase on Dec 6, 2019
  42. laanwj commented at 11:13 am on December 12, 2019: member
    ACK 7cecf10ac32af0fca206ac5f24f482bdec88cb7d
  43. laanwj referenced this in commit 0192bd0652 on Dec 12, 2019
  44. laanwj merged this on Dec 12, 2019
  45. laanwj closed this on Dec 12, 2019

  46. sidhujag referenced this in commit baae132c23 on Dec 12, 2019
  47. fanquake moved this from the "PRs" to the "Done" column in a project

  48. jasonbcox referenced this in commit ffc617b7dd on Oct 1, 2020
  49. sidhujag referenced this in commit d0aed0e3d1 on Nov 10, 2020
  50. DrahtBot locked this on Dec 16, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 09:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me