#34502 wallet: remove most asserts of `WALLET_FLAG_DESCRIPTORS` flag
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #34502 wallet: remove most asserts of `WALLET_FLAG_DESCRIPTORS` flag | Ready | 0 | Neutral | 8 | |||
|
|
|
|
| |||
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
Removes redundant assertions verifying WALLET_FLAG_DESCRIPTORS across descriptor-specific CWallet methods (EncryptWallet, LoadActiveScriptPubKeyMan, AddWalletDescriptor, GetHDPubKeys, and GetKey). It also relocates the descriptor flag check out of UpgradeDescriptorCache() directly into its callers, CWallet::Unlock and WalletBatch::LoadWallet.
Problem: Because legacy wallets can no longer be loaded for normal operations, asserting the descriptor wallet flag across descriptor-specific methods is obsolete and creates cognitive overhead for developers. In addition, checking the flag inside UpgradeDescriptorCache() obscured the fact that its callers can still encounter legacy wallets in migration workflows.
Evidence of importance:
- Author states that legacy wallets cannot be loaded directly anymore, making assertions redundant in descriptor-specific methods and adding cognitive clutter (PR description).
- Author notes that CWallet::Unlock and WalletBatch::LoadWallet are called on legacy wallets during migration (demonstrated in test_encrypted of the wallet_migration suite), so checking the flag at the call sites clarifies that descriptor wallets are not the only callers (commit ba4ae462ed).
Risk of a mistake: low (wallet). Mistakes could theoretically allow non-descriptor wallet paths to invoke descriptor logic without failing early, but legacy wallets are already blocked from loading outside migration workflows.
Changed since the previous assessment: Rebased with no substantive changes to the underlying approach or affected functions.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: frankomosh and b-l-u-e to review author's responses 2026-10-26
- reviewer: frankomosh and b-l-u-e to evaluate author's replies regarding EncryptWallet 2026-10-26
Agreement
Neutral
objections settled, nobody has spoken for the PR
Thread notes: Author replied to both frankomosh and b-l-u-e regarding the EncryptWallet assertion and rebased over master. / Author rebased and replied to objections from frankomosh and b-l-u-e regarding EncryptWallet.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| frankomosh (contributor) | objection, correctness | EncryptWallet executes descriptor-specific work and may be called during legacy wallet migration, making the assertion necessary | answered · author replied that EncryptWallet is not called during migration and only applies to descriptor wallets | answered | yes | 2026-02-09: 'Why remove the assertion from `EncryptWallet()`? I think it also does descriptor-specific work' Status from: 2026-10-26: rkrux: 'I don't see EncryptWallet being called from the migration workflow. Since it can't be called for legacy wallets and only for descriptor wallets, there seems no need to have this assertion within it.' |
| b-l-u-e (contributor) | objection, correctness | Removing the assertion in EncryptWallet causes SIGABRT later in DescriptorScriptPubKeyMan or leaves the wallet in an inconsistent state | answered · author replied that the test scenario calling EncryptWallet on a non-descriptor wallet is contrived | answered | yes | 2026-02-18: 'seems with removing assertion leds to errors failing later' Status from: 2026-10-26: rkrux: 'This seems quite a contrived test case where EncryptWallet is being made to be called from a non-descriptor wallet. Unless this scenario happens in a real world use case, I don't believe this contrived test case failure needs to be considered.' |
Participants:
- frankomosh (contributor, objection, 1 statements 2026-02-09 to 2026-02-09): questioned removing the assert from EncryptWallet since it does descriptor-specific work and may be called during migration
- b-l-u-e (contributor, objection, 2 statements 2026-02-18 to 2026-02-18): demonstrated that removing assert in EncryptWallet leads to later assertion failures and cited descriptor-specific calls
Files
0 lines under test/bench/ci.
- src/wallet/wallet.cpp +5/-16
- src/wallet/walletdb.cpp +3/-1
Card
The short text the ranking pass compares across PRs.
Removes redundant assertions checking WALLET_FLAG_DESCRIPTORS across descriptor-specific CWallet methods and hoists the descriptor check from UpgradeDescriptorCache() into its callers. Because legacy wallets can no longer be loaded for standard operation, the author notes that these assertions clutter the codebase without providing safety value. Moving the check to callers such as CWallet::Unlock also clarifies which code paths may still encounter legacy wallets during wallet migration. This is an internal code cleanup with no user-facing changes.
Data
thread read through 2026-10-26 (6 statements), head ba4ae462ed
labels: Wallet
Categories
Wallet
P4 · cleanup
Removes obsolete assertions and relocates an internal flag check to reduce developer cognitive overhead. The change addresses internal code clarity following legacy wallet deprecation, with no direct user-facing impact or bug fix.
Membership: Matches Wallet label and modifies wallet implementation files src/wallet/wallet.cpp and src/wallet/walletdb.cpp.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 0