#34502 wallet: remove most asserts of `WALLET_FLAG_DESCRIPTORS` flag

full analysis

https://github.com/bitcoin/bitcoin/pull/34502 · rkrux · +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.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T15:50, confidence high, input hash 968075286ee095f1