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

furszy · 2022-11-18T14:55:54Z

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:

"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)
"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.
"Invalid ckey checksum error" test:
Verifies that loading up a ckey with an invalid checksum stops the wallet loading
process with a corruption error.
"Invalid ckey pubkey error" test:
Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
process with a corruption error.

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.

DrahtBot · 2022-11-18T14:55:56Z

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

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.

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.

achow101 · 2022-11-18T16:31:12Z

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

furszy · 2022-11-19T14:02:43Z

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

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

Covered the following scenarios

"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)
"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.
"Invalid ckey checksum error" test:
Verifies that loading up a ckey with an invalid checksum stops the wallet loading
process with a corruption error.
"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.

achow101

ACK 4db80c9

src/Makefile.test_util.include

src/wallet/test/util.cpp

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.

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.

furszy · 2022-11-21T20:34:57Z

Updated per feedback, thanks achow101.

Small Diff:

Added ENABLE_WALLET guard for the wallet/test/util.h and util.cpp files at the test_util makefile level.
Moved ADDRESS_BCRT1_UNSPENDABLE from wallet/test/util.h to wallet_balance.cpp (it's only used there).

aureleoules

ACK 13d9760

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.

src/wallet/test/walletload_tests.cpp

achow101 · 2022-11-23T20:52:03Z

ACK 13d9760

luke-jr

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

…lid" set

…cksum_valid" set backports cc5a5e8 obly ``` 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. ```

…cksum_valid" set backports cc5a5e8 only ``` 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. ```

backport: partial merge bitcoin#26532: wallet: bugfix, invalid crypted key "checksum_valid" set

wallet: bugfix, invalid crypted key "checksum_valid" set

cc5a5e8

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.

DrahtBot added the Wallet label Nov 18, 2022

This was referenced Nov 19, 2022

wallet: Include a signature with encrypted keys to mitigate a wallet scam #25766

Closed

wallet: Load database records in a particular order #24914

Merged

bitcoin deleted a comment Nov 19, 2022

furszy force-pushed the 2022_wallet_fix_ckeys_checksum branch 2 times, most recently from 17a499a to 4db80c9 Compare November 19, 2022 14:33

bitcoin deleted a comment Nov 19, 2022

This was referenced Nov 20, 2022

Reduce wasted pseudorandom bytes in ChaCha20 + various improvements #26153

Merged

wallet: coverage for receiving txes with same id but different witness data #25909

Closed

achow101 reviewed Nov 21, 2022

View reviewed changes

src/Makefile.test_util.include Outdated Show resolved Hide resolved

src/wallet/test/util.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_wallet_fix_ckeys_checksum branch from 4db80c9 to bb71809 Compare November 21, 2022 20:29

furszy added 3 commits November 21, 2022 17:30

refactor: unify test/util/wallet.h with wallet/test/util.h

ee7a984

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.

refactor: move DuplicateMockDatabase to wallet/test/util.h

373c996

furszy force-pushed the 2022_wallet_fix_ckeys_checksum branch from bb71809 to 13d9760 Compare November 21, 2022 20:30

This was referenced Nov 22, 2022

BIP324: Cipher suite #25361

Closed

Add pool based memory resource #25325

Merged

BIP324: Enable v2 P2P encrypted transport #24545

Closed

BIP324: Handshake prerequisites #23561

Closed

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

aureleoules approved these changes Nov 22, 2022

View reviewed changes

src/wallet/test/walletload_tests.cpp Show resolved Hide resolved

src/wallet/test/walletload_tests.cpp Show resolved Hide resolved

fanquake requested a review from Sjors November 23, 2022 22:00

luke-jr approved these changes Nov 29, 2022

View reviewed changes

achow101 merged commit 5690848 into bitcoin:master Nov 29, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 1, 2022

Merge bitcoin#26532: wallet: bugfix, invalid crypted key "checksum_va…

f2e4361

…lid" set

furszy deleted the 2022_wallet_fix_ckeys_checksum branch May 27, 2023 01:47

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Dec 11, 2023

Merge pull request #5759 from UdjinM6/bp26532

985da9a

backport: partial merge bitcoin#26532: wallet: bugfix, invalid crypted key "checksum_valid" set

ogabrielides pushed a commit to ogabrielides/dash that referenced this pull request Dec 22, 2023

Merge pull request dashpay#5759 from UdjinM6/bp26532

8364e4b

backport: partial merge bitcoin#26532: wallet: bugfix, invalid crypted key "checksum_valid" set

PastaPastaPasta added a commit to ogabrielides/dash that referenced this pull request Dec 24, 2023

Merge pull request dashpay#5759 from UdjinM6/bp26532

f0feb36

backport: partial merge bitcoin#26532: wallet: bugfix, invalid crypted key "checksum_valid" set

bitcoin locked and limited conversation to collaborators May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

furszy commented Nov 18, 2022 •

edited

Loading

DrahtBot commented Nov 18, 2022 •

edited

Loading

achow101 commented Nov 18, 2022

furszy commented Nov 19, 2022 •

edited

Loading

achow101 left a comment

furszy commented Nov 21, 2022

aureleoules left a comment

achow101 commented Nov 23, 2022

luke-jr left a comment

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

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

Conversation

furszy commented Nov 18, 2022 • edited Loading

DrahtBot commented Nov 18, 2022 • edited Loading

Reviews

Conflicts

achow101 commented Nov 18, 2022

furszy commented Nov 19, 2022 • edited Loading

Covered the following scenarios

achow101 left a comment

Choose a reason for hiding this comment

furszy commented Nov 21, 2022

aureleoules left a comment

Choose a reason for hiding this comment

achow101 commented Nov 23, 2022

luke-jr left a comment

Choose a reason for hiding this comment

furszy commented Nov 18, 2022 •

edited

Loading

DrahtBot commented Nov 18, 2022 •

edited

Loading

furszy commented Nov 19, 2022 •

edited

Loading