wallet: include a checksum of encrypted private keys #16946

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:wallet-enckey-checksum changing 3 files +39 −6
  1. achow101 commented at 10:24 PM on September 23, 2019: member

    Adds a checksum to the encrypted key record in the wallet database so that encrypted keys can be checked for corruption on wallet loading, in the same way that unencrypted keys are. This allows for us to skip the full decryption of keys upon the first unlocking of the wallet in that session as any key corruption will have already been detected. The checksum is just the double SHA256 of the encrypted key and it is appended to the record after the encrypted key itself.

    This is backwards compatible as old wallets will be able to read the encrypted key and ignore that there is more data in the stream. Additionally, old wallets will be upgraded upon their first unlocking (so that key decryption is checked before we commit to a checksum of the encrypted key) and a wallet flag set indicating that. The presence of the wallet flag lets us skip the full decryption as if fDecryptionThoroughlyChecked were true.

    This does mean that the first time an old wallet is unlocked in a new version will take much longer, but subsequent unlocks will be instantaneous. Furthermore, corruption will be detected upon loading rather than on trying to send so wallet corruption will be detected sooner.

    Fixes #12423

  2. fanquake added the label Wallet on Sep 23, 2019
  3. DrahtBot commented at 11:37 PM on September 23, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  4. achow101 force-pushed on Sep 24, 2019
  5. achow101 force-pushed on Sep 24, 2019
  6. achow101 force-pushed on Sep 25, 2019
  7. achow101 force-pushed on Sep 25, 2019
  8. practicalswift commented at 10:02 PM on September 25, 2019: contributor

    I'm afraid the current version of this PR will introduce a bug in WalletBatch::WriteCryptedKey:

    https://github.com/bitcoin/bitcoin/blob/2b79a270e7442309ab167b161384060d93808695/src/wallet/walletdb.cpp#L118-L128

    Please note the first return false; which makes the re-try logic unreachable :)

  9. achow101 force-pushed on Sep 25, 2019
  10. achow101 commented at 10:09 PM on September 25, 2019: member

    I'm afraid the current version of this PR will introduce a bug in WalletBatch::WriteCryptedKey:

    Please note the first return false; which makes the re-try logic unreachable :)

    Whoops, fixed.

  11. laanwj added the label Feature on Sep 30, 2019
  12. laanwj commented at 7:27 PM on October 28, 2019: member

    Concept ACK

  13. laanwj added this to the milestone 0.20.0 on Oct 28, 2019
  14. DrahtBot added the label Needs rebase on Oct 29, 2019
  15. achow101 force-pushed on Oct 29, 2019
  16. DrahtBot removed the label Needs rebase on Oct 29, 2019
  17. luke-jr commented at 2:18 AM on November 3, 2019: member

    a wallet flag set indicating that. The presence of the wallet flag lets us skip the full decryption as if fDecryptionThoroughlyChecked were true.

    What happens if I open with new version, then switch back to an old version, generate some new keys, then switch back to the new?

    Is a wallet flag really needed at all? Shouldn't verifying checksums also reveal if there are checksums missing?

  18. DrahtBot added the label Needs rebase on Nov 4, 2019
  19. achow101 force-pushed on Nov 4, 2019
  20. achow101 commented at 6:44 PM on November 4, 2019: member

    This is currently getting messed up by the ScriptPubKeyMan changes. Will fix, rebase, and address comments after ScriptPubKeyMan is fully introduced.

  21. achow101 force-pushed on Nov 4, 2019
  22. achow101 force-pushed on Dec 4, 2019
  23. achow101 commented at 12:26 AM on December 4, 2019: member

    I've rebased onto master and included #17369 as a prerequisite. I've also reworked it according to @luke-jr's suggestions. Instead of a wallet flag, we reuse fDecryptionThoroughlyChecked to let as avoid the decryption check. This also makes it easier for us to write (or rewrite) the checksums if they don't exist (or are invalid).

  24. DrahtBot removed the label Needs rebase on Dec 4, 2019
  25. DrahtBot added the label Needs rebase on Dec 6, 2019
  26. achow101 force-pushed on Dec 6, 2019
  27. DrahtBot removed the label Needs rebase on Dec 6, 2019
  28. laanwj commented at 12:23 PM on December 12, 2019: member

    Might want to rebase to get rid of the commits from #17369 and make it easier to review.

  29. achow101 force-pushed on Dec 12, 2019
  30. achow101 commented at 4:13 PM on December 12, 2019: member

    Rebased

  31. DrahtBot added the label Needs rebase on Jan 30, 2020
  32. Read and write a checksum for encrypted keys a8334f7ac3
  33. Set fDecryptionThoroughlyChecked based on whether crypted key checksums are valid
    Change fDecryptionThoroughlyChecked to default to true so that it can
latch to false when an invalid checksum is seen. Checksums may be invalid
if the wallet does not have checksums or if the wallet became corrupted.

It is safe to default fDecryptionThoroughlyChecked to true because any
existing wallet without a checksum will set it to false. Any new or
blank wallet where encrypted keys are added will then set this to true
when the first encrypted key is generated by virtue of CheckDecryptionKey
doing that during the initial Unlock prior to keys being added.
    c9a9ddb414
  34. Upgrade or rewrite encrypted key checksums
    If fDecryptionThoroughlyChecked is false, after a key has been checked,
write (or rewrite) its checksum. This serves to upgrade wallets
and correct those which have the checksum corrupted but not the key.
    d67055e00d
  35. achow101 force-pushed on Jan 30, 2020
  36. DrahtBot removed the label Needs rebase on Jan 30, 2020
  37. jonatack commented at 5:31 PM on March 24, 2020: member

    Code review ACK d67055e00dd90f504384e5c3f229fc95306d5aac

    Rebased on current master @ 5236b2e, built, ran tests, ran bitcoind.

    Could use test coverage.

  38. laanwj removed this from the milestone 0.20.0 on Mar 26, 2020
  39. laanwj added this to the milestone 0.21.0 on Mar 26, 2020
  40. meshcollider commented at 2:24 AM on April 16, 2020: contributor

    Code review ACK d67055e00dd90f504384e5c3f229fc95306d5aac

  41. achow101 commented at 11:14 PM on April 27, 2020: member

    ping @instagibbs

  42. laanwj added this to the "Blockers" column in a project
  43. laanwj commented at 6:44 PM on May 21, 2020: member

    code review ACK d67055e00dd90f504384e5c3f229fc95306d5aac

  44. laanwj removed this from the "Blockers" column in a project
  45. laanwj merged this on May 21, 2020
  46. laanwj closed this on May 21, 2020
  47. sidhujag referenced this in commit 50a72191b0 on May 21, 2020
  48. ComputerCraftr referenced this in commit f6ee1c3482 on Jun 10, 2020
  49. ComputerCraftr referenced this in commit 9a51a27080 on Jun 10, 2020
  50. Fabcien referenced this in commit a7bb5a014c on Feb 4, 2021
  51. DrahtBot locked this on Feb 15, 2022
Contributors
achow101DrahtBotpracticalswiftlaanwjluke-jrjonatackmeshcollider
Labels
FeatureWallet

Milestone
0.21.0

Linked (view graph)
#12423 initial walletpassphrase slow and freezing
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: 2026-04-17 09:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me