fix: mitigate crashes associated with some upgradetohd edge cases

Motivation

When filming demo footage for https://github.com/dashpay/dash/pull/6093, I realized that if I tried to create an encrypted blank legacy wallet and run upgradetohd [mnemonic], the client would crash.

dash@b9c6631a824d:/src/dash$ ./src/qt/dash-qt
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-dash'
dash-qt: wallet/scriptpubkeyman.cpp:399: void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString &, const SecureString &, CKeyingMaterial): Assertion `res' failed.
Posix Signal: Aborted
No debug information available for stacktrace. You should add debug information and then run:
dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5avkbxxg2lyebjwsz3omfwduicbmjxxe5dfmqaaa===

The expected set of operations when performing privileged operations is to first use walletpassphrase [passphrase] [time] to unlock the wallet and then perform the privileged operation. This routine that applies for almost all privileged RPCs doesn't apply here, the unlock state of the wallet has no bearing on constructing an encrypted HD chain as it needs to be encrypted with the master key stored in the wallet, which in turn is encrypted with a key derived from the passphrase (i.e., upgradetohd imports always need the passphrase, if encrypted).

You might have noticed that I used upgradetohd [mnemonic] instead of the correct syntax, upgradetohd [mnemonic] "" [passphrase] that is supposed to be used when supplying a mnemonic to an encrypted wallet, because when you run the former, you don't get told to enter the passphrase into the RPC command, you're told.

Error: Please enter the wallet passphrase with walletpassphrase first.

Which tells you to treat it like any other routine privileged operation and follow the routine as mentioned above. This is where insufficient validation starts rearing its head, we only validate the passphrase if we're supplied one even though we should be demanding one if the wallet is encrypted and it isn't supplied. We didn't supply a passphrase because we're following the normal routine, we unlocked the wallet so EnsureWalletIsUnlocked() is happy, so now the following happens.

upgradetohd()
  | Insufficient validation has allowed us to supply a blank passphrase
  | for an encrypted wallet
  |- CWallet::UpgradeToHD()
    |- CWallet::GenerateNewHDChainEncrypted()
     | We get our hands on vMasterKey by generating the key from our passphrase
     | and using it to unlock vCryptedMasterKey. 
     | 
     | There's one small problem, we don't know if the output of CCrypter::Decrypt
     | isn't just gibberish. Since we don't have a passphrase, whatever came from
     | CCrypter::SetKeyFromPassphrase isn't the decryption key, meaning, the 
     | vMasterKey we just got is gibberish
     |- LegacyScriptPubKeyMan::GenerateNewCryptedHDChain()
       |- res = LegacyScriptPubKeyMan::EncryptHDChain()
       | |- EncryptSecret()
       |   |- CCrypter::SetKey()
       |      This is where everything unravels, the gibberish key's size doesn't
       |      match WALLET_CRYPTO_KEY_SIZE, it's no good for encryption. We bail out.
       |- assert(res)
          We assume are inputs are safe so there's no real reason we should crash.
          Except our inputs aren't safe, so we crash. Welp! :c

This problem has existed for a while but didn't cause the client to crash, in v20.1.1 (19512988c6e6e8641245bd9c5fab21dd737561f0), trying to do the same thing would return you a vague error

Failed to generate encrypted HD wallet (code -4)

In the process of working on mitigating this crash, another edge case was discovered, where if the wallet was unlocked and an incorrect passphrase was provided to upgradetohd, the user would not receive any feedback that they entered the wrong passphrase and the client would similarly crash.

upgradetohd()
 | We've been supplied a passphrase, so we can try and validate it by
 | trying to unlock the wallet with it. If it fails, we know we got the
 | wrong passphrase.
 |- CWallet::Unlock()
 | | Before we bother unlocking the wallet, we should check if we're
 | | already unlocked, if we are, we can just say "unlock successful".
 | |- CWallet::IsLocked()
 | |  Wallet is indeed unlocked.
 | |- return true; 
 | The validation method we just tried to use has a bail-out mechanism
 | that we don't account for, the "unlock" succeded so I guess we have the
 | right passphrase.
 [...] (continue call chain as mentioned earlier)
       |- assert(res)
          Oh...

This pull request aims to resolve crashes caused by the above two edge cases.

Additional Information

As this PR was required me to add additional guardrails on GenerateNewCryptedHDChain() and GenerateNewHDChainEncrypted(), it was taken as an opportunity to resolve a TODO (source). The following mitigations have been implemented.

Validating vMasterKey size (any key not of WALLET_CRYPTO_KEY_SIZE size cannot be used for encryption and so, cannot be a valid key)
Validating secureWalletPassphrase's presence to catch attempts at passing a blank value (an encrypted wallet cannot have a blank passphrase)
Using Unlock() to validate the correctness of vMasterKey. (the two other instances of iterating through mapMasterKeys use Unlock(), see here and here)
- Lock()'ing the wallet before Unlock()'ing the wallet to avoid the IsLocked() bail-out condition and then restoring to the previous lock state afterwards.
Add an IsCrypted() check to see if upgradetohd's walletpassphrase is allowed to be empty.

Checklist:

[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[x] I have added or updated relevant unit/integration/functional/e2e tests
[x] I have made corresponding changes to the documentation (note: N/A)
[x] I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

dashpay / dash