wallet: Some cleanups around LegacyScriptPubKeyMan #20580

pull promag wants to merge 3 commits into bitcoin:master from promag:2020-12-wallet-legacy changing 2 files +8 −15

promag commented at 7:09 PM on December 5, 2020: member

No description provided.
wallet: Simplify CWallet::IsLegacy caf68e292c
wallet: Avoid IsLegacy before GetLegacyScriptPubKeyMan fcd2bd3e13
wallet: Use const LegacyScriptPubKeyMan 0fe4d02b09
promag force-pushed on Dec 5, 2020
DrahtBot added the label Wallet on Dec 5, 2020
fanquake requested review from achow101 on Dec 5, 2020
in src/wallet/wallet.cpp:4475 in caf68e292c outdated
```
4471 | @@ -4472,11 +4472,7 @@ void CWallet::LoadActiveScriptPubKeyMan(uint256 id, OutputType type, bool intern
4472 |  
4473 |  bool CWallet::IsLegacy() const
4474 |  {
4475 | -    if (m_internal_spk_managers.count(OutputType::LEGACY) == 0) {
4476 | -        return false;
4477 | -    }
4478 | -    auto spk_man = dynamic_cast<LegacyScriptPubKeyMan*>(m_internal_spk_managers.at(OutputType::LEGACY));
4479 | -    return spk_man != nullptr;
4480 | +    return (m_internal_spk_managers.count(OutputType::LEGACY) == 1);
```
achow101 commented at 10:41 PM on December 5, 2020:

This change is simply incorrect. The original checked for LegacyScriptPubKeyMan. This now just checks for the presence of a ScriptPubKeyMan that provides legacy change addresses. That is completely not what this is for. OutputType::LEGACY is unrelated to LegacyScriptPubKeyMan, this could have used OutputType::BECH32 and not been any different.

promag commented at 11:08 PM on December 5, 2020:

Got it, now I understand the reason of the cast. But ATM all m_internal_spk_managers are the same LegacyScriptPubKeyMan instance right?

achow101 commented at 12:11 AM on December 6, 2020:

But ATM all m_internal_spk_managers are the same LegacyScriptPubKeyMan instance right?

Only for legacy wallets. For legacy wallets, there's only one ScriptPubKeyMan for all spk_managers.

However for descriptor wallets, there are multiple DescriptorScriptPubKeyMans. This change is incorrect for descriptor wallets.
achow101 commented at 10:44 PM on December 5, 2020: member

Tend to NACK.

Besides the 1st commit being completely incorrect, I don't really see the point. It cleans up a little bit but the stuff that is already there isn't currently confusing. Furthermore, the change to auto in the 3rd commit actually makes it slightly less clear about the type.
promag commented at 11:21 PM on December 5, 2020: member

It cleans up a little bit but the stuff that is already there isn't currently confusing.

The point was to avoid double checking m_internal_spk_managers, not because it's confusing.

Furthermore, the change to auto in the 3rd commit actually makes it slightly less clear about the type.

Kind of, but the idea was not to use auto, but to make it const.

Thanks @achow101 for reviewing. I'll hold on these nits if something else comes up.
promag closed this on Dec 5, 2020
promag deleted the branch on Dec 5, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

promag

achow101

Labels

Wallet