#36031 wallet: Remove mapMasterKeys and enforce that only one encryption key can exist

ReviewabilityReviewsAgreementSize
#36031 achow101 wallet: Remove mapMasterKeys and enforce that only one encryption key can existWaiting on author0 (+1)Strong24 + 2 tests
  • Remove dead multi-passphrase handling logic from the wallet encryption system
  • Prevent inconsistent wallet states and simplify key management for wallet maintainers
  • Ready for review
  • Broad agreement that keeping only one encryption key simplifies maintenance (rkrux, jeanpablojp)
  • Verified backward compatibility with nonstandard master key identifiers through testing (vicjuma)
  • Considered safe since corrupted edge-case wallets failing to load is acceptable (davidgumberg)
  • Concept approval with open questions on unloadable wallets and safety assertions (polespinasa)
  • 24 lines added or modified outside tests
  • 2 lines added or modified in tests
  • 40 lines removed in total
  • 6 files, 3 commits
Wallet#69 of 92
P3 · cleanup
  • P3 because it cleans up obsolete multi-passphrase design that was never implemented
  • Improves code maintainability by eliminating redundant loops without changing wallet features
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Replaces the in-memory master key map and ID counter in CWallet with an optional single master key. If a wallet database contains more than one encryption key record, loading now fails with a database corruption error. Key unlocking, passphrase updating, and database serialization are simplified to operate on this single key directly while preserving existing database record formats.

Problem: Since 2011 (PR #352), the wallet stored encryption keys in a map intended for multiple passphrases, a feature that was never implemented. This left loops and redundant key ID tracking across unlock and passphrase modification routines, and allowed inconsistent states if unexpected key records existed.

Evidence of importance:

  • PR description: "mapMasterKeys was introduced in #352 without a clear rationale. Likely it was to allow the user to have multiple passphrases... However, this functionality was never implemented, and current code enforces in a few places (inconsistently) that there is only one encryption key."
  • Commit 8e3e423921: "By removing mapMasterKeys and replacing it with a single m_encryption_key, we can remove this confusion and simplify encryption key handling. No one should have a wallet that has more than one encrypion key."

Scope: Touches wallet encryption data structures and wallet database loading and writing logic.

Risk of a mistake: high (wallet, wallet_storage). A flaw in master key serialization or loading logic could corrupt the wallet database or render encrypted wallets permanently unreadable, causing fund loss.

Changed since the previous assessment: No substantial functional changes from the previous version; the PR structure and implementation remain the same.

Reviewability

Paused · Waiting on author

Open objections from polespinasa with no author reply for 31 days; review would repeat what has been asked.

Waiting on (from the thread read):

  • author: author reply to questions and review suggestions from polespinasa 2026-10-01

Agreement

Strong

substantive support, no open objection (jeanpablojp, vicjuma, rkrux, davidgumberg)

Thread notes: Added polespinasa as a participant with a Concept ACK along with three inline suggestions and one question regarding multiple encryption keys.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
jeanpablojp (contributor)objection, correctnessWallets created blank and encrypted twice in v0.18/0.19 have two mkey records and will fail to load or migrate with 'Wallet corrupted'answered · author replied that this rare edge case is not worth addressing · v1, not yet re-checkedansweredyes2026-08-22: 'On this branch, migratewallet rejects the legacy wallet and loadwallet rejects the already migrated descriptor wallet with Wallet corrupted.'
Status from: 2026-08-24: achow101: 'I don't think that this is worth addressing as I expect no one to actually be affected by it.'
polespinasa (member)question, approachWhat happens if a wallet has more than one encryption key, and does this change make it unloadable?openansweredno2026-10-01: 'What if someone does? IIUC think this PR makes the wallet unloadable.'
polespinasa (member)suggestion, interfaceHave the function take kMasterKey for consistency with WriteMasterKeyopenansweredno2026-10-01: 'Can't this function also take the `kMasterKey` so it is consistent with `WriteMasterKey`??'
polespinasa (member)suggestion, safetyAdd an assert or return an error if m_encryption_key is empty to guard against undefined behavioropenansweredno2026-10-01: 'I think an `assert(m_encryption_key.hasvalue())` would make the function safer. Or as the function is `Expected` maybe return an error if it does not have value.'
polespinasa (member)suggestion, correctnessEnsure the silently ignored return value in wallet_encrypt benchmark returns trueopenansweredno2026-10-01: 'Not introduced by this PR but, this return is silently ignored. Maybe worth ensure it returns true?'

Support:

  • jeanpablojp (contributor) (Concept ACK, 2026-08-22) [read]: keeping a single key makes sense
  • theStack (member) (Concept ACK, 2026-08-24): (no reason given) [not substantive]
  • vicjuma (contributor) (Approach ACK, 2026-08-24) [tested]: Verified that nonstandard master key IDs remain backward compatible through testing
  • rkrux (member) (Concept ACK, 2026-08-26): I don't see a good reason to have multiple keys or passphrases to encrypt the wallet going forward. And the corresponding code cleanup is necessary for long term wallet maintainability and usability.
  • w0xlt (contributor) (Concept ACK, 2026-09-08): (no reason given) [not substantive]
  • davidgumberg (member) (ACK, 2026-09-08) [read]: From a high-level this change is safe: the worst that can happen is that some wallet edge case has not been taken into account and will fail to load on startup, which is fine.
  • polespinasa (member) (Concept ACK, 2026-10-01) [read]: (no reason given) [not substantive]

Participants:

  • jeanpablojp (contributor, objection, 2 statements 2026-08-22 to 2026-08-22): Concept ACK, but noted blank wallets encrypted twice under v0.18/0.19 fail to load as corrupt
  • theStack (member, support, 1 statements 2026-08-24 to 2026-08-24): Concept ACK
  • vicjuma (contributor, support, 3 statements 2026-08-24 to 2026-08-24): Approach ACK after testing master key backward compatibility with a nonstandard ID
  • rkrux (member, support, 1 statements 2026-08-26 to 2026-08-26): Strong Concept ACK noting cleanup is necessary for long-term maintainability
  • davidgumberg (member, support, 2 statements 2026-09-08 to 2026-09-08): crACK after analyzing failure modes
  • w0xlt (contributor, support, 1 statements 2026-09-08 to 2026-09-08): Concept ACK
  • polespinasa (member, support, 4 statements 2026-10-01 to 2026-10-01): Concept ACK; asked what happens if multiple keys exist and suggested safety assertions and consistency improvements

Files

5 lines under test/bench/ci.

  • src/wallet/walletdb.cpp +12/-15
  • src/wallet/wallet.cpp +8/-18
  • src/bench/wallet_encrypt.cpp +2/-3
  • src/wallet/wallet.h +1/-3
  • src/wallet/crypter.h +2/-0
  • src/wallet/walletdb.h +1/-1

Card

The short text the ranking pass compares across PRs.

This pull request removes the mapMasterKeys collection in CWallet and replaces it with a single optional encryption key member. It addresses dead architectural complexity left over from an unfinished 2011 multi-passphrase concept that was never completed, causing unlock and passphrase-change routines to loop unnecessarily over maps. Loading a wallet with multiple master key records is now treated explicitly as database corruption. The author notes that Bitcoin Core already assumed a single encryption key in several inconsistent places, making this a simplification of wallet internals.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-02T01:25, confidence high, input hash 75b652d1f433cde8
thread read through 2026-10-01 (15 statements), head 8e3e423921
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Wallet, Needs rebase

Categories

Wallet

P3 · cleanup

Simplifies wallet encryption state and enforces consistency by removing unused multi-passphrase infrastructure. Replaces mapMasterKeys with a single master key and treats multiple key records as database corruption, removing confusing loops and dead code across key management routines.

Membership: Wallet label and modifications across src/wallet/ touching key crypter, database loading, and wallet state.

Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1