wallet: remove most asserts of `WALLET_FLAG

rkrux commented at 10:42 AM on February 4, 2026: contributor

Reasons:

Few of the wallet methods are descriptors specific as suggested by their names, these assertions seems redundant within them.
Since the legacy wallet can't be loaded anymore, few of these assertions are outdated now.
Seeing too many of these assertions in the CWallet class while reading the code is cognitively loaded, and after a while becomes annoying for the reader.

Note: Such assertions in the migration specific functions have been left untouched because they seem necessary there.

DrahtBot added the label Wallet on Feb 4, 2026

DrahtBot commented at 10:42 AM on February 4, 2026: contributor

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt by achow101)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)

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.

rkrux commented at 11:49 AM on February 4, 2026: contributor

I had incorrectly run the functional tests in a different directory in local that led to all the functional tests pass. I can reproduce the test failure locally now, putting the PR in draft for now.

rkrux marked this as a draft on Feb 4, 2026

DrahtBot added the label CI failed on Feb 4, 2026

wallet: remove few asserts of `WALLET_FLAG_DESCRIPTORS` flag

Reasons:
1. Few of the wallet methods are descriptors specific as suggested
by their names, these assertions seems redundant within them.
2. Since the legacy wallet can't be loaded anymore, few of these
assertions are outdated now.
3. Seeing too many of these assertions in the `CWallet` class while
reading the code is cognitively loaded, and after a while becomes
annoying for the reader.

Note: Such assertions in the migration specific functions have been
left untouched because they seem necessary there.

a3ff41a5a8

wallet: move out `UpgradeDescriptorCache()` wallet flag check to the callers

The `UpgradeDescriptorCache` method ends up being called via the
`CWallet::Unlock` and `WalletBatch::LoadWallet` methods, both of
which can be called for the legacy wallets within the context of
wallet migration. For the former caller, it is evident in the
`test_encrypted` test of `wallet_migration` suite.

The previous code where there is no explicit check in the callers
makes the reader believe that the callers end up being called only
for descriptor wallets, which is misleading. This move out of the
check makes it explicit for the reader that this is not the only
flow for these callers.

77778e2559

rkrux force-pushed on Feb 4, 2026

rkrux renamed this:
~~wallet: remove few asserts of `WALLET_FLAG_DESCRIPTORS` flag~~
wallet: remove most asserts of `WALLET_FLAG_DESCRIPTORS` flag
on Feb 4, 2026

DrahtBot removed the label CI failed on Feb 4, 2026

rkrux marked this as ready for review on Feb 5, 2026

frankomosh commented at 11:29 AM on February 9, 2026: contributor

Few questions from my code review;

Why remove the assertion from EncryptWallet()? I think it also does descriptor-specific work (iterates m_spk_managers, calls SetupDescriptorScriptPubKeyMans()).

You said in the other commit that Unlock() and LoadWallet() can be called during migration for legacy wallets. I wonder if EncryptWallet() can face a similar situation.

b-l-u-e commented at 12:38 PM on February 18, 2026: contributor

Why remove the assertion from EncryptWallet()? I think it also does descriptor-specific work (iterates m_spk_managers, calls SetupDescriptorScriptPubKeyMans()).

i tried testing with and without assertion seems with removing assertion leds to errors failing later added some tests in src/wallet/test/wallet_tests.cpp to see

--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -722,5 +722,32 @@ BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup)
     TestUnloadWallet(std::move(wallet));
 }
 
+BOOST_FIXTURE_TEST_CASE(encryptwallet_requires_descriptor_wallet, BasicTestingSetup)
+{
+    WalletContext context;
+    context.args = &m_args;
+    auto wallet = TestLoadWallet(context);
+    BOOST_REQUIRE(wallet);
+    BOOST_CHECK(wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
+    bool ok = wallet->EncryptWallet("test");
+    (void)ok;
+    WaitForDeleteWallet(std::move(wallet));
+}
+
+BOOST_FIXTURE_TEST_CASE(encryptwallet_on_non_descriptor_wrong_behavior, WalletTestingSetup)
+{
+    BOOST_REQUIRE(!m_wallet.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
+    BOOST_REQUIRE(m_wallet.GetAllScriptPubKeyMans().empty());
+
+    bool ok = m_wallet.EncryptWallet("test");
+
+    if (ok) {
+        BOOST_CHECK_MESSAGE(!m_wallet.GetAllScriptPubKeyMans().empty(),
+            "EncryptWallet added descriptor SPKMs via SetupDescriptorScriptPubKeyMans()");
+        BOOST_CHECK_MESSAGE(!m_wallet.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS),
+            "Wrong behavior: wallet has descriptor SPKMs but WALLET_FLAG_DESCRIPTORS was never set inconsistent state");
+    }
+}

so tests fails inside descriptor code scriptpubkeyman this is without assert

wallet/test/wallet_tests.cpp(737): Entering test case "encryptwallet_on_non_descriptor_wrong_behavior"
wallet/test/wallet_tests.cpp(739): info: check !m_wallet.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) has passed
wallet/test/wallet_tests.cpp(740): info: check m_wallet.GetAllScriptPubKeyMans().empty() has passed
test_bitcoin: wallet/scriptpubkeyman.cpp:1138: bool wallet::DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch &, const CExtKey &, OutputType, bool): Assertion `m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)' failed.
unknown location(0): fatal error: in "wallet_tests/encryptwallet_on_non_descriptor_wrong_behavior": signal: SIGABRT (application abort requested)
wallet/test/wallet_tests.cpp(740): last checkpoint
wallet/test/wallet_tests.cpp(737): Leaving test case "encryptwallet_on_non_descriptor_wrong_behavior"; testing time: 340947us
wallet/test/wallet_tests.cpp(45): Leaving test suite "wallet_tests"; testing time: 340982us
wallet/test/wallet_transaction_tests.cpp(12): Test suite "wallet_transaction_tests" is skipped because disabled
wallet/test/walletdb_tests.cpp(15): Test suite "walletdb_tests" is skipped because disabled
wallet/test/walletload_tests.cpp(14): Test suite "walletload_tests" is skipped because disabled
ipc/test/ipc_tests.cpp(11): Test suite "ipc_tests" is skipped because disabled
Leaving test module "Bitcoin Core Test Suite"; testing time: 341332us

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

and then with assert it fails at the boundary Encryptwallet

wallet/test/wallet_tests.cpp(739): info: check !m_wallet.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) has passed
wallet/test/wallet_tests.cpp(740): info: check m_wallet.GetAllScriptPubKeyMans().empty() has passed
wallet/wallet.cpp:812 bool wallet::CWallet::EncryptWallet(const SecureString &): Assertion `IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)' failed.
unknown location(0): fatal error: in "wallet_tests/encryptwallet_on_non_descriptor_wrong_behavior": signal: SIGABRT (application abort requested)
wallet/test/wallet_tests.cpp(740): last checkpoint
wallet/test/wallet_tests.cpp(737): Leaving test case "encryptwallet_on_non_descriptor_wrong_behavior"; testing time: 13719us
wallet/test/wallet_tests.cpp(45): Leaving test suite "wallet_tests"; testing time: 13750us
wallet/test/wallet_transaction_tests.cpp(12): Test suite "wallet_transaction_tests" is skipped because disabled
wallet/test/walletdb_tests.cpp(15): Test suite "walletdb_tests" is skipped because disabled
wallet/test/walletload_tests.cpp(14): Test suite "walletload_tests" is skipped because disabled
ipc/test/ipc_tests.cpp(11): Test suite "ipc_tests" is skipped because disabled
Leaving test module "Bitcoin Core Test Suite"; testing time: 14055us

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

b-l-u-e commented at 1:50 PM on February 18, 2026: contributor

i think here states that its descriptor-specific

git show a3ff41a5a8:src/wallet/wallet.cpp | sed -n '865,868p'
        if (!IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
            SetupDescriptorScriptPubKeyMans();
        }
        Lock();

rkrux marked this as a draft on Mar 5, 2026

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

Reviews

Conflicts