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

pull rkrux wants to merge 2 commits into bitcoin:master from rkrux:descriptors-assertion changing 2 files +8 −17
  1. rkrux commented at 10:42 AM on February 4, 2026: contributor

    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.

    <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

  2. DrahtBot added the label Wallet on Feb 4, 2026
  3. DrahtBot commented at 10:42 AM on February 4, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. 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.

  5. rkrux marked this as a draft on Feb 4, 2026
  6. DrahtBot added the label CI failed on Feb 4, 2026
  7. 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
  8. 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
  9. rkrux force-pushed on Feb 4, 2026
  10. 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
  11. DrahtBot removed the label CI failed on Feb 4, 2026
  12. rkrux marked this as ready for review on Feb 5, 2026
  13. 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.

  14. 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"
  15. 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();
  16. rkrux marked this as a draft on Mar 5, 2026
Contributors
rkruxDrahtBotfrankomoshb-l-u-e
Labels
Wallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me