#34502 wallet: remove most asserts of `WALLET_FLAG_DESCRIPTORS` flag
https://github.com/bitcoin/bitcoin/pull/34502 · · +8/-17 in 2 files, 2 commits · labels: Wallet, Needs rebase · draft
Goal
- Reduce cognitive noise for developers reading wallet code by dropping redundant descriptor assertions
This pull request removes several `WALLET_FLAG_DESCRIPTORS` assertions in `CWallet` functions, arguing they are redundant now that legacy wallets cannot be loaded outside migration. It also moves the descriptor flag check out of `UpgradeDescriptorCache()` into its call sites (`Unlock()` and `LoadWallet()`).
Problem: The author finds multiple assertion checks for the descriptor flag cognitively noisy when reading wallet code and considers some redundant since legacy wallets are mostly obsolete.
Category: Wallet (#83 of 84)
P4 · cleanup
- P4 because it is minor internal cleanup to simplify reading wallet code
- Offers no functional, security, or user-facing benefits
P4 because this is cosmetic cleanup aimed at reducing cognitive load while reading `CWallet`. Reviewers demonstrated that removing the assertions removes an intentional safety boundary, causing crashes deeper inside descriptor code.
Membership: Touches `src/wallet/wallet.cpp` to remove wallet flag assertions and adjust migration call paths.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0
Reviewability: Stale: Needs rebase
- Needs rebase and remains in draft while the author investigates test failures
The PR has merge conflicts, is in draft, and the author has been inactive for 225 days after reproducing test failures.
Author status: silent since 2026-02-04 after marking the PR as draft due to local test failures
Open concerns:
- frankomosh questioned removing the assert in `EncryptWallet()`, pointing out it executes descriptor-specific setup.
- b-l-u-e provided test cases showing that removing the assert in `EncryptWallet()` causes an assertion abort deeper in `DescriptorScriptPubKeyMan` or leaves the wallet in an inconsistent state.
Agreement: Mild
- Unaddressed objection: removing the assertion leads to crashes or inconsistent state (b-l-u-e)
- Questioned removing the check because encryption executes descriptor-specific setup (frankomosh)
- Author has not responded to the technical objections
Mild: frankomosh and b-l-u-e demonstrated that removing the assertion leads to inconsistent state and crashes; author has not replied.
Reviewers raised technical objections with proof that removing the assertions allows non-descriptor wallets into paths that fail deeper down in scriptpubkeyman. The author has not responded.
- frankomosh asked why remove the assertion from `EncryptWallet()` as it calls `SetupDescriptorScriptPubKeyMans()`
- b-l-u-e demonstrated via test case that removing the assertion in `EncryptWallet()` leads to `SIGABRT` in `scriptpubkeyman.cpp` or inconsistent state
Review verdicts (DrahtBot): 0
Files
File list not available for this run.
Card
PR 34502 proposes removing several `WALLET_FLAG_DESCRIPTORS` assertions in `CWallet` functions, claiming they are redundant and clutter the code now that legacy wallets cannot be loaded. Reviewers demonstrated that removing these assertions exposes callers to invalid state and deeper crashes in descriptor code. The author put the PR into draft on creation day after reproducing test failures and has been silent for 225 days. The PR has merge conflicts and lacks support.