wallet: Change wallet validation order #24859

w0xlt commented at 6:50 AM on April 15, 2022: contributor

In the current code, the database is created before the last validation, which checks that passphrase is set and private keys are disabled.

Therefore, if this validation fails, it will result in an empty database and the user will not be able to recreate a wallet with the same name and with the correct parameters.

Behavior on the master branch:

$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_01" disable_private_keys=true passphrase="passphrase"
error code: -4
error message:
Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.

$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_01"
error code: -4
error message:
Wallet file verification failed. Failed to create database path '/home/w/.bitcoin/regtest/wallets/invalid_wallet'. Database already exists.

Behavior on the PR branch:

$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_02" disable_private_keys=true passphrase="passphrase"
error code: -4
error message:
Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.

$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_02"
{
  "name": "invalid_wallet_01",
  "warning": ""
}

Change wallet validation order

In the current code, the database is created before the last validation,
which checks that passphrase is set and private keys are disabled.

Therefore, if this validation fails, it will result in an empty database
and the user will not be able to recreate a wallet with the same name
and with the correct parameters.

0359d9b6a3

w0xlt renamed this:
~~Change wallet validation order~~
wallet: Change wallet validation order
on Apr 15, 2022

MarcoFalke commented at 6:54 AM on April 15, 2022: member

It would be good to add a test for this if this is a bug fix

DrahtBot added the label Wallet on Apr 15, 2022

vincenzopalazzo commented at 9:27 PM on April 15, 2022: none

Concept ACK, the change looks trivial but I think to add a test unit will avoid trouble in the future if any change will introduce

vincenzopalazzo approved

vincenzopalazzo commented at 10:43 PM on April 15, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/24859/commits/5db4e4fd7f4f2eab5b45cd0c4192ebb8f9328768

w0xlt commented at 10:44 PM on April 15, 2022: contributor

Added https://github.com/bitcoin/bitcoin/pull/24859/commits/5db4e4fd7f4f2eab5b45cd0c4192ebb8f9328768 containing the test.

This test reproduces the behavior mentioned in the PR description and fails in the master branch.

aureleoules commented at 3:18 AM on April 16, 2022: member

tACK 5db4e4fd7f4f2eab5b45cd0c4192ebb8f9328768 Verified the behavior on master and this branch, nice fix!

test: Add a test that creates a wallet with invalid parameters

Invalid parameters must not prevent a new wallet with the same name
from being created with the correct parameters

6f29409ad1

in test/functional/wallet_createwallet.py:31 in 5db4e4fd7f outdated

  25 | @@ -26,6 +26,12 @@ def run_test(self):
  26 |          node = self.nodes[0]
  27 |          self.generate(node, 1) # Leave IBD for sethdseed
  28 |  
  29 | +        self.log.info("Run createwallet with invalid parameters.")
  30 | +        # Run createwallet with invalid parameters. This must not prevent a new wallet with the same name from being created with the correct parameters.
  31 | +        # See PR #24859 for details.

MarcoFalke commented at 7:13 AM on April 16, 2022:

I don't think we need to add obscure #nnn to the source code. If anything this should be a full url, but I don't think it adds any value, so best to remove it.

vincenzopalazzo commented at 7:38 AM on April 16, 2022:

+1

w0xlt commented at 7:47 AM on April 16, 2022:

Done.

w0xlt force-pushed on Apr 16, 2022

DrahtBot commented at 5:30 AM on April 17, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24897 ([Draft / POC] Silent Payments by w0xlt)

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.

achow101 commented at 3:27 PM on April 18, 2022: member

ACK 6f29409ad180ef00998ac05997f0fa03f98cd066

achow101 merged this on Apr 18, 2022

achow101 closed this on Apr 18, 2022

sidhujag referenced this in commit 4a5c0d0b98 on Apr 19, 2022

w0xlt deleted the branch on Apr 25, 2022

luke-jr referenced this in commit 47a4548943 on May 21, 2022

luke-jr referenced this in commit e04ee88451 on May 21, 2022

DrahtBot locked this on Apr 25, 2023