wallet: bugfix, invalid crypted key "checksum_valid" set #26532

pull furszy wants to merge 4 commits into bitcoin:master from furszy:2022_wallet_fix_ckeys_checksum changing 12 files +206 −103
  1. furszy commented at 2:55 PM on November 18, 2022: member

    At wallet load time, the crypted key "checksum_valid" variable is always set to false. Which, on every wallet decryption call, forces the process to re-write all the ckeys to db when it's not needed.

    Note: The first commit fixes the issue, the two commits in the middle are cleanups so DuplicateMockDatabase can be used without duplicating code. And, the last one is pure test coverage for the crypted keys loading process.

    Includes test coverage for the following scenarios:

    1. "All ckeys checksums valid" test: Loads an encrypted wallet with all the crypted keys with a valid checksum and verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.

      (we force a complete ckeys re-write if we find any missing crypted key checksum during the wallet loading process)

    2. "Missing checksum in one ckey" test: Verifies that loading up a wallet with, at least one, 'ckey' with no checksum triggers a complete re-write of the crypted keys.

    3. "Invalid ckey checksum error" test: Verifies that loading up a ckey with an invalid checksum stops the wallet loading process with a corruption error.

    4. "Invalid ckey pubkey error" test: Verifies that loading up a ckey with an invalid pubkey stops the wallet loading process with a corruption error.

  2. wallet: bugfix, invalid crypted key "checksum_valid" set
    At wallet load time, we set the crypted key "checksum_valid" variable always to false.
Which, on every wallet decryption call, forces the process to re-write the entire ckeys to db when
it's not needed.
    cc5a5e8121
  3. DrahtBot commented at 2:55 PM on November 18, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK aureleoules, achow101
    Stale ACK luke-jr

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26153 (Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa)
    • #25909 (wallet: coverage for receiving txes with same id but different witness data by furszy)
    • #25766 (wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101)
    • #25361 (BIP324: Cipher suite by dhruv)
    • #25325 (Add pool based memory resource by martinus)
    • #24914 (wallet: Load database records in a particular order by achow101)
    • #23561 (BIP324: Handshake prerequisites by dhruv)
    • #23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

    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.

  4. DrahtBot added the label Wallet on Nov 18, 2022
  5. achow101 commented at 4:31 PM on November 18, 2022: member

    Good catch! Do you think it's possible to have a test for this?

  6. DrahtBot cross-referenced this on Nov 19, 2022 from issue wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101
  7. DrahtBot cross-referenced this on Nov 19, 2022 from issue wallet: Load database records in a particular order by achow101
  8. bitcoin deleted a comment on Nov 19, 2022
  9. furszy commented at 2:02 PM on November 19, 2022: member

    Good catch! Do you think it's possible to have a test for this?

    Yep :), added test coverage for it and more inside 4db80c90.

    Covered the following scenarios

    1. "All ckeys checksums valid" test: Loads an encrypted wallet with all the crypted keys with a valid checksum and verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.

      (we force a complete ckeys re-write if we find any missing crypted key checksum during the wallet loading process)

    2. "Missing checksum in one ckey" test: Verifies that loading up a wallet with, at least one, 'ckey' with no checksum triggers a complete re-write of the crypted keys.

    3. "Invalid ckey checksum error" test: Verifies that loading up a ckey with an invalid checksum stops the wallet loading process with a corruption error.

    4. "Invalid ckey pubkey error" test: Verifies that loading up a ckey with an invalid pubkey stops the wallet loading process with a corruption error.

    Extra note: The two commits in the middle are cleanups so I can use the DuplicateMockDatabase function without duplicating code.

    Going to update the PR description with this too.

  10. furszy force-pushed on Nov 19, 2022
  11. furszy force-pushed on Nov 19, 2022
  12. bitcoin deleted a comment on Nov 19, 2022
  13. DrahtBot cross-referenced this on Nov 20, 2022 from issue Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa
  14. DrahtBot cross-referenced this on Nov 20, 2022 from issue wallet: coverage for receiving txes with same id but different witness data by furszy
  15. in src/Makefile.test_util.include:22 in cd62eddbcf outdated 
      18 | @@ -19,7 +19,7 @@ TEST_UTIL_H = \
  19 |    test/util/transaction_utils.h \
  20 |    test/util/txmempool.h \
  21 |    test/util/validation.h \
  22 | -  test/util/wallet.h
  23 | +  wallet/test/util.h
    achow101 commented at 7:43 PM on November 21, 2022:

    In cd62eddbcfba652a238374d62f70181cb3ad9075 "refactor: unify test/util/wallet.h with wallet/test/util.h"

    nit: ISTM these should be guarded by ENABLE_WALLET? They aren't used by non-wallet benchmarks or tests, so it doesn't break anything.

    This would also remove the need to have #ifdef ENABLE_WALLET in those files too.

    furszy commented at 8:26 PM on November 21, 2022:

    hmm yeah, could also remove them from BITCOIN_TEST_SUITE as TEST_UTIL_H is already included there.

  16. in src/wallet/test/util.cpp:16 in cd62eddbcf outdated 
      10 | @@ -11,10 +11,11 @@
  11 |  #include <wallet/wallet.h>
  12 |  #include <wallet/walletdb.h>
  13 |  
  14 | -#include <boost/test/unit_test.hpp>
  15 | -
  16 |  #include <memory>
  17 |  
  18 | +const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj";
    achow101 commented at 7:46 PM on November 21, 2022:

    In cd62eddbcfba652a238374d62f70181cb3ad9075 "refactor: unify test/util/wallet.h with wallet/test/util.h"

    nit: Although this were originally in src/test/util/wallet.h, it doesn't really make sense to be in the wallet's utils since it is not a wallet specific address. Could probably go in src/test/util/script.h or src/test/util/transaction_utils.h

    furszy commented at 8:27 PM on November 21, 2022:

    Could also just move it to wallet_balance.cpp. It's not used anywhere else.

  17. achow101 commented at 7:53 PM on November 21, 2022: member

    ACK 4db80c90b02b70a71a3ac1d614d779d222561525

  18. furszy force-pushed on Nov 21, 2022
  19. refactor: unify test/util/wallet.h with wallet/test/util.h
    files share the same purpose, and we shouldn't have wallet code
inside the test directory.

This later is needed to use wallet util functions in the bench
and test binaries without be forced to duplicate them.
    ee7a984f85
  20. refactor: move DuplicateMockDatabase to wallet/test/util.h 373c99633e
  21. test: load wallet, coverage for crypted keys
    Adds test coverage for the wallet's crypted key loading from db process.
The following scenarios are covered:

1) "All ckeys checksums valid" test:
   Loads an encrypted wallet with all the crypted keys with a valid checksum and
   verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.

   (we force a complete ckeys re-write if we find any missing crypted key checksum
    during the wallet loading process)

2) "Missing checksum in one ckey" test:
   Verifies that loading up a wallet with, at least one, 'ckey' with no checksum
   triggers a complete re-write of the crypted keys.

3) "Invalid ckey checksum error" test:
   Verifies that loading up a ckey with an invalid checksum stops the wallet loading
   process with a corruption error.

4) "Invalid ckey pubkey error" test:
   Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
   process with a corruption error.
    13d9760829
  22. furszy force-pushed on Nov 21, 2022
  23. furszy commented at 8:34 PM on November 21, 2022: member

    Updated per feedback, thanks achow101.

    Small Diff:

    1. Added ENABLE_WALLET guard for the wallet/test/util.h and util.cpp files at the test_util makefile level.
    2. Moved ADDRESS_BCRT1_UNSPENDABLE from wallet/test/util.h to wallet_balance.cpp (it's only used there).
  24. DrahtBot cross-referenced this on Nov 22, 2022 from issue BIP324: Cipher suite by dhruv
  25. DrahtBot cross-referenced this on Nov 22, 2022 from issue Add pool based memory resource by martinus
  26. DrahtBot cross-referenced this on Nov 22, 2022 from issue BIP324: Enable v2 P2P encrypted transport by dhruv
  27. DrahtBot cross-referenced this on Nov 22, 2022 from issue BIP324: Handshake prerequisites by dhruv
  28. DrahtBot cross-referenced this on Nov 22, 2022 from issue BIP324: Add encrypted p2p transport {de}serializer by dhruv
  29. in src/wallet/test/walletload_tests.cpp:54 in 13d9760829 
      50 | @@ -50,5 +51,139 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup)
  51 |      }
  52 |  }
  53 |  
  54 | +bool HasAnyRecordOfType(WalletDatabase& db, const std::string& key)
    aureleoules commented at 2:48 PM on November 22, 2022:

    13d97608297bd56ed033d0e754d2e50447b02af0 nit:

    bool HasAnyRecordOfType(WalletDatabase& db, std::string_view key)
    furszy commented at 9:03 PM on November 23, 2022:

    thanks, will do if have to retouch

  30. in src/wallet/test/walletload_tests.cpp:85 in 13d9760829 
      80 | +        return db;
  81 | +    };
  82 | +
  83 | +    {   // Context setup.
  84 | +        // Create and encrypt legacy wallet
  85 | +        std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", m_args, CreateMockWalletDatabase()));
    aureleoules commented at 2:55 PM on November 22, 2022:

    13d97608297bd56ed033d0e754d2e50447b02af0 nit: (same for other instances)

            auto wallet{std::make_shared<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase())};
    furszy commented at 8:59 PM on November 23, 2022:

    k thanks, will do if have to retouch.

  31. aureleoules approved
  32. aureleoules commented at 3:19 PM on November 22, 2022: member

    ACK 13d97608297bd56ed033d0e754d2e50447b02af0

    Cherry-picked the last three commits on master and verified the tests do not pass. I also verified that the tests correctly test the behavior.

  33. achow101 commented at 8:52 PM on November 23, 2022: member

    ACK 13d97608297bd56ed033d0e754d2e50447b02af0

  34. fanquake requested review from Sjors on Nov 23, 2022
  35. luke-jr approved
  36. luke-jr commented at 1:25 AM on November 29, 2022: member

    utACK cc5a5e81217506ec6f9fff34056290f8f40a7396 by itself (did not review refactors or new test)

  37. achow101 merged this on Nov 29, 2022
  38. achow101 closed this on Nov 29, 2022
  39. sidhujag referenced this in commit f2e4361419 on Dec 1, 2022
  40. furszy deleted the branch on May 27, 2023
  41. UdjinM6 referenced this in commit 17b8dd6c61 on Dec 10, 2023
  42. UdjinM6 referenced this in commit d7bae33f57 on Dec 10, 2023
  43. PastaPastaPasta referenced this in commit 985da9ab93 on Dec 11, 2023
  44. ogabrielides referenced this in commit 8364e4b001 on Dec 22, 2023
  45. PastaPastaPasta referenced this in commit f0feb36ce3 on Dec 24, 2023
  46. bitcoin locked this on May 26, 2024
Contributors
furszyDrahtBotachow101aureleoulesluke-jr


Sjors

Labels
Wallet
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-05-20 06:53 UTC