wallet, rpc: log instead of returning false #35966

Summary

  if (!Unlock(strWalletPassphrase)) {
            return false;
    }

The above code src/wallet/wallet.cpp:890: runs after the wallet encryption process is already complete. Making the failed unlock operation to return false leads to a misleading RPC error message, just in case the encryption happens but the unlock operation fails. Probably it was added there because the function should always return a boolean.

Before change

This is the result of a failed unlock, i.e making the function to always return false to mimic the unlock failure

Mutated function

if (Unlock(plain_master_key)) {
        // Now that we've unlocked, upgrade the descriptor cache
        // UpgradeDescriptorCache();
         return false;
    }

Output

ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ ./bitcoin-cli createwallet testwallet
{
  "name": "testwallet"
}
ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ ./bitcoin-cli encryptwallet "testpass"
error code: -16
error message:
Error: Failed to encrypt the wallet.
ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ ./bitcoin-cli encryptwallet "testpass"
error code: -15
error message:
Error: running with an encrypted wallet, but encryptwallet was called.
ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ 

Solution

The failed unlock is inconsequential to the encryption process. It can even just be written as

Lock();
Unlock(strWalletPassphrase)

SetupWalletGeneration();

Lock();

Instead of returning false, which leads to a misleading error message to the user, probably just logging a message might be necessary

 if (!Unlock(strWalletPassphrase)) {
    WalletLogPrintf("Unlocking the encrypted wallet failed\n");
 }

After the change

ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ ./bitcoin-cli createwallet testwallet
{
 "name": "testwallet"
}
ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ ./bitcoin-cli encryptwallet "testpass"
wallet encrypted; The keypool has been flushed and a new HD seed was generated. You need to make a new backup with the backupwallet RPC.
ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ ./bitcoin-cli encryptwallet "testpass"
error code: -15
error message:
Error: running with an encrypted wallet, but encryptwallet was called.
ratedg@0xratedg:~/Desktop/bitcoin/build/bin$ 

The nature of the log is the one to be probably determined or the essence of the conditional statement

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35966.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Is this consistent with PR 28333? Can GetDatabase().Rewrite(); get called in a dangeruz way?

Is this consistent with PR 28333? Can GetDatabase().Rewrite(); get called in a dangeruz way?

Thanks for the comment. Both GetDatabase().Rewrite(); and !Unlock(strWalletPassphrase) are called after the encryption is already successful. This PR does not look at the database rewrite in any way because its result does not also change anything. Whether the rewrite is a success or not, the encryption has already happened. Same as whether the unlock is successful or not. That's why returning false raises the wrong error message as I have demonstrated above.

Is this consistent with PR 28333? Can GetDatabase().Rewrite(); get called in a dangeruz way?

This PR you are mentioning added the conditional statement, this PR tends to maintein it, only this time logging instead of returning. It is the simplest way if the function is to return a bool. Changing the function to return an enum class variant is also possible, like

enum class EncryptWalletResul {SUCCESS, FAILED, SUCCESS_UNLOCK_FAILED, SUCCESS_REWRITE_FAILED};

This is however complicated solution for the simple problem

I know I'm missing most of the thing here, but. If encryption of right-just-created wallets is currently broken, as I think your bitcoin-cli flow implies, isnt this change a lazy workaround fix instead of what would be a thorough elaborate fix? I can't see how this would be equivalent.

I know I'm missing most of the thing here, but. If encryption of right-just-created wallets is currently broken, as I think your bitcoin-cli flow implies, isnt this change a lazy workaround fix instead of what would be a thorough elaborate fix? I can't see how this would be equivalent.

It is not a break in encryption, its just the wrong error reporting. The process is Encryption -> Unlock Wallet -> Rewrite Database. Now returning false in the place of unlock is what results in

error code: -16
error message:
Error: Failed to encrypt the wallet.

which should not be the case because the encryption process has already happened. You can maybe try to reproduce it in regtest so that you see what is currently happening. The PR description shows the steps to reproduce it