wallet: remove GetScriptPubKeyMan spam

[klementtan]

fixes: #21716

Problem:
In AddWalletDescriptor we iterate through every combination of internal/external and OutputType. For each combination we call GetScriptPubKeyMan and when the combination is invalid we would log a warning scriptPubKey Manager for output type and thus causing the spam in the logs.

My approach
Since there is only one ScriptPubKeyMan that has the same id as old_spk_man_id, we would instead iterate through both the map and erase the ScriptPubKeyMan once we find it.

The code that causes the spam: src/wallet/wallet.cpp

ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const OutputType& type, bool internal) const
{
    const std::map<OutputType, ScriptPubKeyMan*>& spk_managers = internal ? m_internal_spk_managers : m_external_spk_managers;
    std::map<OutputType, ScriptPubKeyMan*>::const_iterator it = spk_managers.find(type);
    if (it == spk_managers.end()) {
        WalletLogPrintf("%s scriptPubKey Manager for output type %d does not exist\n", internal ? "Internal" : "External", static_cast<int>(type));
        return nullptr;
    }
    return it->second;
}
        auto old_spk_man_id = old_spk_man->GetID();
        for (bool internal : {false, true}) {
            for (OutputType t : OUTPUT_TYPES) {
                auto active_spk_man = GetScriptPubKeyMan(t, internal);
                if (active_spk_man && active_spk_man->GetID() == old_spk_man_id) {
                    if (internal) {
                        m_internal_spk_managers.erase(t);
                    } else {
                        m_external_spk_managers.erase(t);
                    }
                    break;
                }
            }
        }

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• wallet: importdescriptors update existing #19651 (wallet: importdescriptors update existing by S3RK)

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.

[fanquake]

@S3RK You might be interested in this.

[S3RK]

Updating descriptors by removing SPKmanager from the map and creating a new one has been shown to be problematic — see #19651. I'd suggest to review #19651 as it removes the code it question altogether and it had one ACK already.

[achow101]

Agree with @S3RK that we should focus on #19651 which removes this code and replaces it with proper updating that also happens to not call GetScriptPubKeyMan a bunch.

[fanquake]

Going to close this in favour of #19651. @klementtan would you like to review that PR instead?

[klementtan]

Agreed. #19651 is a better solution than this. Will take a look at it.