For #14138 I took a look at encrypted_batch in the wallet code. My understanding may be incorrect, as the code around it is undocumented and quite messy. But as far as I understand, CWallet::encrypted_batch is only used in these places:
- The destructor, simply to free it.
AddKeyPubKeyWithDB: If not set initially, it is set to the passed-in reference and reset tonullptrbefore exiting the function.AddCryptedKey: It is read if non-null, not changed.EncryptWallet: It is expected thatencrypted_batchis null initially. Then it is set to a newly allocated object, used, feed and reset tonullptragain before exiting the function.
To me, it seems that there is no way through which encrypted_batch can be non-null except temporarily while executing either AddKeyPubKeyWithDB or EncryptWallet. Thus I feel like this can be cleaned up and clarified quite a bit. For instance:
- Make it explicit that the member variable /
CWalletinstance never owns the associated memory; it is just a simple reference passed down toAddCryptedKey. - Accordingly, do not do anything about it in the destructor.
- Use a
std::unique_ptron the stack inEncryptWalletto hold the memory.
One could even go a step further and use RAII to set/restore encrypted_batch in both AddKeyPubKeyWithDB and EncryptWallet rather than doing it in the current “unsafe” way.
Am I missing something here, or is my understanding correct? If it is, then I’m happy to prepare a clean up PR for this.
