Repository: bitcoin/bitcoin
File: src/wallet/wallet.cpp (C++)

Code before:
            error = strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n"
                                "The wallet might have been tampered with or created with malicious intent.\n"), walletFile);
            return nullptr;
        } else if (nLoadWalletRet == DBErrors::LEGACY_WALLET) {
            error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), walletFile);
            return nullptr;
        } else {
            error = strprintf(_("Error loading %s"), walletFile);
            return nullptr;
        }
    }

    // This wallet is in its first run if there are no ScriptPubKeyMans and it isn't blank or no privkeys
    const bool fFirstRun = walletInstance->m_spk_managers.empty() &&
                     !walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) &&
                     !walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
    if (fFirstRun)
    {
        LOCK(walletInstance->cs_wallet);

        // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
        walletInstance->SetMinVersion(FEATURE_LATEST);

        // Init with passed flags.
        // Always set the cache upgrade flag as this feature is supported from the beginning.
        walletInstance->InitWalletFlags(wallet_creation_flags | WALLET_FLAG_LAST_HARDENED_XPUB_CACHED);

        // Only descriptor wallets can be created
        assert(walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));

        if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) || !(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET))) {
            if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
                walletInstance->SetupDescriptorScriptPubKeyMans();
                // SetupDescriptorScriptPubKeyMans already calls SetupGeneration for us so we don't need to call SetupGeneration separately
            } else {
                // Legacy wallets need SetupGeneration here.
                for (auto spk_man : walletInstance->GetActiveScriptPubKeyMans()) {
                    if (!spk_man->SetupGeneration()) {
                        error = _("Unable to generate initial keys");
                        return nullptr;
                    }
                }
            }
        }

        if (chain) {
            std::optional<int> tip_height = chain->getHeight();
            if (tip_height) {
                walletInstance->SetLastBlockProcessed(*tip_height, chain->getBlockHash(*tip_height));
            }
        }

Review comment (performance):
In 69f588a99a7a79d1d72300bc0f2c8475f95f6c6a

> Since new wallets will always being using the upgraded cache, there's no
reason to wait to set the flag, so set it when the wallet flags are
being initialized for new wallets.

Conceptually, it makes sense to me that the `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` is set at the time of wallet creation.

> Although WalletBatch::LoadWallet performs the descriptor cache upgrade,
because new wallets do not have the descriptor flag set yet, the upgrade
does not run and set the flag.

However, as mentioned above^ and as I checked in the code as well, since the cache upgrade is not run, it therefore is not recorded in the DB as well. If so far the presence of this flag leads to a presence of the corresponding DB entry, it'd be preferable if this behaviour is preserved here as well, which I believe is not happening. I will double check if this can lead to errors in some other parts of the wallet flows.


Code after:
            error = strprintf(_("Unrecognized descriptor found. Loading wallet %s\n\n"
                                "The wallet might had been created on a newer version.\n"
                                "Please try running the latest software version.\n"), walletFile);
            return nullptr;
        } else if (nLoadWalletRet == DBErrors::UNEXPECTED_LEGACY_ENTRY) {
            error = strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n"
                                "The wallet might have been tampered with or created with malicious intent.\n"), walletFile);
            return nullptr;
        } else if (nLoadWalletRet == DBErrors::LEGACY_WALLET) {
            error = strprintf(_("Error loading %s: Wallet is a legacy wallet. Please migrate to a descriptor wallet using the migration tool (migratewallet RPC)."), walletFile);
            return nullptr;
        } else {
            error = strprintf(_("Error loading %s"), walletFile);
            return nullptr;
        }
    }

    // This wallet is in its first run if there are no ScriptPubKeyMans and it isn't blank or no privkeys
    const bool fFirstRun = walletInstance->m_spk_managers.empty() &&
                     !walletInstance->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) &&
                     !walletInstance->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
    if (fFirstRun)
    {
        LOCK(walletInstance->cs_wallet);

        // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
        walletInstance->SetMinVersion(FEATURE_LATEST);

        // Init with passed flags.
        // Always set the cache upgrade flag as this feature is supported from the beginning.
        walletInstance->InitWalletFlags(wallet_creation_flags | WALLET_FLAG_LAST_HARDENED_XPUB_CACHED);

        // Only descriptor wallets can be created
        assert(walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));

        if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) || !(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET))) {
            if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
                walletInstance->SetupDescriptorScriptPubKeyMans();
                // SetupDescriptorScriptPubKeyMans already calls SetupGeneration for us so we don't need to call SetupGeneration separately
            } else {
                // Legacy wallets need SetupGeneration here.
                for (auto spk_man : walletInstance->GetActiveScriptPubKeyMans()) {
                    if (!spk_man->SetupGeneration()) {
                        error = _("Unable to generate initial keys");
                        return nullptr;
                    }
                }
            }
        }

        if (chain) {