wallet, tests: Expand and test when the blank wallet flag should be un/set #25634

achow101 commented at 5:37 PM on July 18, 2022: member

The blank wallet flag is used to indicate that the wallet intentionally does not have any keys, scripts, or descriptors, and it prevents the automatic generation of those things for such a wallet. Once the wallet contains any of those data, it is unnecessary, and possibly incorrect, to have blank set. This PR fixes a few places where this was not properly happening. It also adds a test for this unset behavior.

fanquake added the label Wallet on Jul 18, 2022

DrahtBot commented at 8:07 PM on July 18, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	S3RK, ryanofsky
Concept ACK	ishaanam
Stale ACK	furszy, aureleoules, Xekyo, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27827 ([Silent Payments]: Base functionality by josibake)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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 force-pushed on Jul 22, 2022

in src/wallet/rpc/wallet.cpp:379 in 79b7520a2e outdated

 343 | @@ -344,6 +344,11 @@ static RPCHelpMan createwallet()
 344 |      uint64_t flags = 0;
 345 |      if (!request.params[1].isNull() && request.params[1].get_bool()) {
 346 |          flags |= WALLET_FLAG_DISABLE_PRIVATE_KEYS;
 347 | +        // We want to set BLANK when DISABLE_PRIVATE_KEYS, but only if the user did not explicitly do blank=false
 348 | +        // It can be set when blank is not provided; when it is provided, we handle it later

aureleoules commented at 1:24 PM on September 19, 2022:

shouldn't the created wallet be blank whenever DISABLE_PRIVATE_KEYS is set?

./src/bitcoin-cli -regtest createwallet testing true false # disable keys and not blank ./src/bitcoin-cli -regtest -rpcwallet=testing getwalletinfo

{
  "walletname": "testing",
  "walletversion": 169900,
  "format": "sqlite",
  "balance": 0.00000000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 0,
  "keypoolsize": 0,
  "keypoolsize_hd_internal": 0,
  "paytxfee": 0.00000000,
  "private_keys_enabled": false,
  "avoid_reuse": false,
  "scanning": false,
  "descriptors": true,
  "external_signer": false,
  "blank": false # shouldn't this be overridden by true?
}

achow101 commented at 8:12 PM on September 19, 2022:

As noted in the OP, blank=false overrides the default behavior.

aureleoules commented at 8:18 PM on September 19, 2022:

Yes but does it make sense to create a wallet that is "not blank" and without private keys? I may be missing something

aureleoules commented at 11:51 AM on September 20, 2022:

However, for backwards compatibility, users can choose to override this in the RPC by doing blank=False.

I guess that's the reason.

achow101 commented at 5:45 PM on October 10, 2022:

The blank flag is basically a no-op for wallets without private keys. It's not necessarily a useful distinction.

hernanmarino commented at 3:37 PM on October 24, 2022:

I agree with @achow101 . Initially I was concerned that a bug in a future change might be introduced due to the apparent contradiction between disable_private_keys=true and blank=false, but after reading the tests, I understand that this might be posible after importing ADDRESSes and/or other elements. Am I right about this conclusion? Or am I missing something?

achow101 commented at 4:17 PM on October 24, 2022:

Yes, blank will be unset once anything is imported or generated. The purpose of blank is to prevent the wallet from automatically generating keys for blank wallets since they appear the same as newly created wallets to the creation/setup code that runs during wallet loading. Since disabling private keys means that nothing can be generated anyways, blank isn't actually doing anything for such wallets. Additionally, once anything is imported, blank will be unset as well.

aureleoules commented at 1:29 PM on September 19, 2022: member

Concept ACK

in test/functional/wallet_blank.py:58 in 4c936c8af2 outdated

  57 | +        eckey = ECKey()
  58 | +        eckey.generate()
  59 | +
  60 | +        wallet.importpubkey(eckey.get_pubkey().get_bytes().hex())
  61 | +        assert_equal(wallet.getwalletinfo()["blank"], False)
  62 | +

pablomartin4btc commented at 3:53 PM on October 24, 2022:

Once we import private keys, does the flag private_keys_enabled change its value to true? If so, shouldn't we also be checking for the flag private_keys_enabled is true now, here in the test?

achow101 commented at 4:12 PM on October 24, 2022:

No, WALLET_FLAG_DISABLE_PRIVATE_KEYS is a permanent flag. It cannot be changed, and attempting to import private keys into a wallet with it set will result in an error and nothing imported. This test is for importing public keys, not private keys.

pablomartin4btc commented at 3:09 PM on October 25, 2022:

Oh! Right, my bad, thanks for the clarification! I think I got confused with the following test which is test_importprivkey, but it doesn't have private_keys_enabled set to false either. I see what you're saying about the error trying to import private keys into a wallet with WALLET_FLAG_DISABLE_PRIVATE_KEYS on another test: https://github.com/bitcoin/bitcoin/blob/3c1e75ef607000b265c15b47b32485cd95c1245d/test/functional/wallet_importdescriptors.py#L225-L230

pablomartin4btc commented at 3:55 PM on October 24, 2022: member

Concept ACK, I'll perform some testing soon.

furszy approved

furszy commented at 9:18 PM on October 24, 2022: member

Code review ACK 4c936c8a

aureleoules approved

aureleoules commented at 11:09 AM on October 27, 2022: member

ACK 4c936c8af2a2a8373c8b8082d6bc4d798c8193b4

in test/functional/wallet_listdescriptors.py:107 in 44e30c5aa9 outdated

  94 |  
  95 |          self.log.info('Test list private descriptors with encrypted wallet')
  96 |          assert_raises_rpc_error(-13, 'Please enter the wallet passphrase with walletpassphrase first.', wallet.listdescriptors, True)
  97 |          wallet.walletpassphrase(passphrase="pass", timeout=1000000)
  98 | -        assert_equal(expected_private, wallet.listdescriptors(True))
  99 | +        assert expected_private["descriptors"][0] in wallet.listdescriptors(True)["descriptors"]

ishaanam commented at 10:18 PM on November 11, 2022:

In 44e30c5aa98703b68267924bbbbb745e9242e969 "wallet: Ensure that the blank wallet flag is unset after imports"

It is not clear to me how the other changes in this PR result in this behavior change. Especially since it appears to only be happening once a wallet is encrypted. After the changes in this PR, running listdescriptors before a wallet is encrypted produces drastically different results than when it is run afterwards. listdescriptors before the wallet is encrypted:

{'wallet_name': 'w2', 'descriptors': [{'desc': "wpkh([80002067/84'/1'/0']tpubDCMVLhErorrAGfApiJSJzEKwqeaf2z3NrkVMxgYQjZLzMjXMBeRw2muGNYbvaekAE8rUFLftyEar4LdrG2wXyyTJQZ26zptmeTEjPTaATts/0/*)#yz2uu6mw", 'timestamp': 1296688602, 'active': False, 'range': [0, 0], 'next': 0}]}

listdescriptors after the wallet is encrypted:

{'wallet_name': 'w2', 'descriptors': [{'desc': "wpkh([80002067/84'/1'/0']tpubDCMVLhErorrAGfApiJSJzEKwqeaf2z3NrkVMxgYQjZLzMjXMBeRw2muGNYbvaekAE8rUFLftyEar4LdrG2wXyyTJQZ26zptmeTEjPTaATts/0/*)#yz2uu6mw", 'timestamp': 1296688602, 'active': False, 'range': [0, 0], 'next': 0}, {'desc': "pkh([5dc1e209/44'/1'/0']tpubDDfjWivrLDGEU679nu8YgTDPUALX9bUppvdXXk19TDAaecSNxMH8vPZBeRRjaDqKUYgEd3UzZWS4rt5qsAWQW2qktupQZR1MnSnAZ7L85Cn/0/*)#rx0cg4z2", 'timestamp': 1668205361, 'active': True, 'internal': False, 'range': [0, 0], 'next': 0}, {'desc': "wpkh([5dc1e209/84'/1'/0']tpubDD42XKtjCmM9u18pwcFj5VNPoh4QZn6fDuHq5ypo5ZvZrsMguRRd5YvDjKX6BLKz393V9QpeouE95eHeaugv3FM6LVqTo9f2phTPQvw65xy/0/*)#q3r4f8hn", 'timestamp': 1668205361, 'active': True, 'internal': False, 'range': [0, 0], 'next': 0}, {'desc': "pkh([5dc1e209/44'/1'/0']tpubDDfjWivrLDGEU679nu8YgTDPUALX9bUppvdXXk19TDAaecSNxMH8vPZBeRRjaDqKUYgEd3UzZWS4rt5qsAWQW2qktupQZR1MnSnAZ7L85Cn/1/*)#jj2e4qjj", 'timestamp': 1668205361, 'active': True, 'internal': True, 'range': [0, 0], 'next': 0}, {'desc': "sh(wpkh([5dc1e209/49'/1'/0']tpubDCVSqsf6kfUFDo28Vq3Lsrt9FXVqtohxWFxufkxubWT4Kh9z5RfmcxZmgKPBsWvboazFWNqnA9YHmQNrtfCvxTU8mch4yzxcMshqCWXtt9u/0/*))#8s9y4eu7", 'timestamp': 1668205361, 'active': True, 'internal': False, 'range': [0, 0], 'next': 0}, {'desc': "tr([5dc1e209/86'/1'/0']tpubDDMfjHg7VtGGjDiXv3ohgm7NTpm6ue5XnXdJBx9NZ8PgrSdLAQ1DjnYnxNiQKJcB6ZaoQo7WjZo7iTXVzdBgW38KM5BRsJJzXtDUSSjehRU/0/*)#60q6ankn", 'timestamp': 1668205361, 'active': True, 'internal': False, 'range': [0, 0], 'next': 0}, {'desc': "tr([5dc1e209/86'/1'/0']tpubDDMfjHg7VtGGjDiXv3ohgm7NTpm6ue5XnXdJBx9NZ8PgrSdLAQ1DjnYnxNiQKJcB6ZaoQo7WjZo7iTXVzdBgW38KM5BRsJJzXtDUSSjehRU/1/*)#tm9mqxxt", 'timestamp': 1668205361, 'active': True, 'internal': True, 'range': [0, 0], 'next': 0}, {'desc': "sh(wpkh([5dc1e209/49'/1'/0']tpubDCVSqsf6kfUFDo28Vq3Lsrt9FXVqtohxWFxufkxubWT4Kh9z5RfmcxZmgKPBsWvboazFWNqnA9YHmQNrtfCvxTU8mch4yzxcMshqCWXtt9u/1/*))#j3tjdxfp", 'timestamp': 1668205361, 'active': True, 'internal': True, 'range': [0, 0], 'next': 0}, {'desc': "wpkh([5dc1e209/84'/1'/0']tpubDD42XKtjCmM9u18pwcFj5VNPoh4QZn6fDuHq5ypo5ZvZrsMguRRd5YvDjKX6BLKz393V9QpeouE95eHeaugv3FM6LVqTo9f2phTPQvw65xy/1/*)#39x55j8t", 'timestamp': 1668205361, 'active': True, 'internal': True, 'range': [0, 0], 'next': 0}]}

S3RK commented at 8:38 AM on November 14, 2022:

Yes. This seems like a bug. I was able to trace it back to encryption function

src/wallet/wallet.cpp:829

        // If we are using descriptors, make new descriptors with a new seed
        if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
            SetupDescriptorScriptPubKeyMans();

@achow101 why do we want to regenerate descriptors on encryption?

achow101 commented at 4:43 PM on November 14, 2022:

Hmm, yes, I'm not sure whether this is a bug as it the behavior is (mostly) expected.

We always want to rotate keys after encryption because the old keys were written to disk unencrypted and thus may have been compromised. There may also be backups of the unencrypted keys. So if the user has opted into encryption, we want to make sure any keys they use from then on were born encrypted and known to not have been stored anywhere unencrypted. We do the same for the legacy wallet as well.

The question here is whether this behavior is expected for formerly blank wallets.

S3RK commented at 8:26 AM on November 15, 2022:

I personally wouldn't expect to see new descriptors after encrypting wallet with an imported descriptor. But I also understand why rotating descriptors is generally good.

I don't think distinction of "formerly blank wallets" is good enough. The same problem can happen with non-blank wallet. Should we rotate user imported descriptors in non-blank wallets? The current code would rotate it as well.

It seems like we need to track which descriptors are automatically generated and which are manually imported. Of course, the problem is that we can't backfill the information.

achow101 commented at 4:38 PM on November 15, 2022:

Should we rotate user imported descriptors in non-blank wallets? The current code would rotate it as well.

Yes. This is considered an important aspect of encryption as it ensures that addresses used afterwards did not have their private keys written unencrypted. It is also a well documented behavior in both the RPC and GUI.

ishaanam commented at 5:51 PM on November 15, 2022:

This is considered an important aspect of encryption as it ensures that addresses used afterwards did not have their private keys written unencrypted.

This makes sense, but in this case shouldn't new private keys only be generated for the descriptors that already exist in the wallet (imported or automatically generated) instead of adding additional descriptors to the wallet that the user did not ask for? Even if rotating keys upon encryption is well documented, adding additional descriptors does not appear to be.

achow101 commented at 8:20 PM on November 15, 2022:

This makes sense, but in this case shouldn't new private keys only be generated for the descriptors that already exist in the wallet (imported or automatically generated) instead of adding additional descriptors to the wallet that the user did not ask for? Even if rotating keys upon encryption is well documented, adding additional descriptors does not appear to be.

This can get rather complicated since users can import descriptors that don't match the same pattern as our automatically generated ones, and producing ones that look similar except for the keys might be more confusing than ones that are altogether different? At least for non-single key things, it would be very obvious that the descriptor has changed.

This is also perhaps an argument for always encrypting wallets with a default passphrase which would completely sidestep that issue.

ishaanam commented at 11:36 PM on November 15, 2022:

Seeing as most of the solutions to this issue/confusion are likely out of the scope of this PR, I think that these changes should be fine with the addition of documentation about this behavior change related to importing descriptors before encrypting a wallet, since previously no new descriptors would be added.

ryanofsky commented at 8:23 PM on June 5, 2023:

Change to unset the blank flag when writing a descriptor is now reverted, so I think this makes BLANK flag documentation misleading, and could be fixed with:

--- a/src/wallet/walletutil.h
+++ b/src/wallet/walletutil.h
@@ -53,10 +53,18 @@ enum WalletFlags : uint64_t {
     //! Flag set when a wallet contains no HD seed and no private keys, scripts,
     //! addresses, and other watch only things, and is therefore "blank."
     //!
-    //! The only function this flag serves is to distinguish a blank wallet from
+    //! The main function this flag serves is to distinguish a blank wallet from
     //! a newly created wallet when the wallet database is loaded, to avoid
     //! initialization that should only happen on first run.
     //!
+    //! A secondary function of this flag, which applies to descriptor wallets
+    //! only, is to serve as an ongoing indication that descriptors in the
+    //! wallet should be created manually, and that the wallet should not
+    //! generate automatically generate new descriptors if it is later
+    //! encrypted. To support this behavior, descriptor wallets unlike legacy
+    //! wallets do not automatically unset the BLANK flag when things are
+    //! imported.
+    //!
     //! This flag is also a mandatory flag to prevent previous versions of
     //! bitcoin from opening the wallet, thinking it was newly created, and
     //! then improperly reinitializing it.

achow101 commented at 11:04 AM on June 8, 2023:

I've added a commit with this comment.

ishaanam commented at 10:41 PM on November 11, 2022: contributor

Concept ACK This looks good for the most part, I just have a question about the test.

S3RK commented at 8:27 AM on November 15, 2022: contributor

Thinking about this I've got a question: Do we even need to store a flag for a blank wallet? Can't we just check whether it has any descriptors in it or not at runtime?

achow101 commented at 4:35 PM on November 15, 2022: member

Do we even need to store a flag for a blank wallet? Can't we just check whether it has any descriptors in it or not at runtime?

No, that's how we determine whether the wallet was just created. Without the blank flag, we don't have a way of distinguishing between "blank" and "newly created".

furszy commented at 5:50 PM on November 15, 2022: member

Do we even need to store a flag for a blank wallet? Can't we just check whether it has any descriptors in it or not at runtime?

No, that's how we determine whether the wallet was just created. Without the blank flag, we don't have a way of distinguishing between "blank" and "newly created".

Hmm, wouldn't be sufficient to differentiate blank from newly created wallets by providing a flag to the CreateWallet function based on the context?

Sounds pretty odd this blank flag storage requirement. We could just add a runtime parameter that says "if we are loading any wallet from disk, don't create any new seed nor new SPKMs, just load the file into a wallet". Then, only if the context is the createwallet RPC command (or the same flow in the GUI), set the flag to true and create the new seed and related descriptors.

achow101 commented at 8:15 PM on November 15, 2022: member

Hmm, wouldn't be sufficient to differentiate blank from newly created wallets by providing a flag to the CreateWallet function based on the context?

Going forward, probably. But we still need the flag for downgrade protection. Older software determines whether a wallet is new by observing that there's nothing in it. So if a blank wallet (without the flag) were loaded into the older software, it would think the wallet was just created and generate a new seed for it. Since the flag is one of the mandatory flags, older software that see that flag (or the version number that added support for the flag) will not load or modify the wallet. We unset the flag once the wallet contains things because it is no longer needed for downgrade protection. Once the wallet has keys or descriptors, older software will not automatically generate anything for that wallet.

The other reason it's implemented this way is because at the time it was introduced, we would create a wallet specified with -wallet if it did not exist. Since the same function is used for both loading and creation and there is no (easy) way to tell whether the wallet that was loaded was just created or already existed, we needed the flag in order to know not to do anything to the wallet.

Perhaps there needs to be some distinction between "this wallet needs to start out without any automatically generated things" and "this wallet must only have things the user adds explicitly". The original reason blank was added was to support born-encrypted wallets. It was a way to make sure that a newly created wallet would not have any initial keys so that the user (or the creation process itself) can encrypt the wallet and generate keys automatically after encryption. However some users probably want to have wallets where the only things in their wallets are things that are added explicitly, e.g. only imports, or only sethdseed results. As noted in #25634 (review), blank wallets does not support that as after encrypting, a new seed or a new set of descriptors will be generated. I would argue that supporting that in blank wallets is not quite what blank wallet means and should be a separate feature.

DrahtBot added the label Needs rebase on Dec 9, 2022

achow101 force-pushed on Dec 12, 2022

DrahtBot removed the label Needs rebase on Dec 12, 2022

DrahtBot added the label Needs rebase on Dec 19, 2022

achow101 force-pushed on Dec 19, 2022

DrahtBot removed the label Needs rebase on Dec 19, 2022

in src/wallet/rpc/wallet.cpp:62 in 6ae1256483 outdated

  58 | @@ -59,6 +59,7 @@ static RPCHelpMan getwalletinfo()
  59 |                          }, /*skip_type_check=*/true},
  60 |                          {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"},
  61 |                          {RPCResult::Type::BOOL, "external_signer", "whether this wallet is configured to use an external signer such as a hardware wallet"},
  62 | +                        {RPCResult::Type::BOOL, "blank", "Whether this wallet is marked as intentionally not having any keys, scripts, or descriptors"},

murchandamus commented at 3:20 PM on March 17, 2023:

Using “not” in the context of a boolean feels always a bit like a double negation. Perhaps go with “without”:

                        {RPCResult::Type::BOOL, "blank", "whether this wallet was intentionally created without keys, scripts, or descriptors"},

achow101 commented at 7:19 PM on March 17, 2023:

Changed the wording slightly, although did not use this suggestion since it is incorrect.

in test/functional/wallet_blank.py:123 in 23403d0ed8 outdated

 111 | +
 112 | +        wallet_dump_path = os.path.join(self.nodes[0].datadir, "wallet.dump")
 113 | +        def_wallet.dumpwallet(wallet_dump_path)
 114 | +
 115 | +        wallet.importwallet(wallet_dump_path)
 116 | +        assert_equal(wallet.getwalletinfo()["blank"], False)

murchandamus commented at 3:33 PM on March 17, 2023:

You are explicitly expecting this behavior, but I’m kinda surprised that when we import a wallet that was explicitly created “blank” after the import it is now no longer blank. As far as I can tell, no keys were created, unless the import automatically does that. I’m curious why this is the expected behavior (although since you seem to explicitly expect that behavior, I don’t think there is an issue here).

achow101 commented at 5:04 PM on March 17, 2023:

We determine whether a wallet is new by checking whether it has any keys or scripts contained in it. If it is "new", then the wallet will be setup to have the automatically generated things. The blank flag is used only to indicate to that code that the wallet is intentionally blank and should not be setup as a new wallet. Once something has been added to the wallet, it's no longer blank and the setup code will no longer recognize it as being "new", so the flag should be removed. It's not intended as something that should live beyond that, it doesn't just mean that the wallet was created as blank, but rather that it is currently blank and that state is intentional.

murchandamus commented at 7:04 PM on March 17, 2023:

I see, thanks for the explanation

S3RK commented at 8:35 AM on March 20, 2023:

I have a different perspective. The flag was provided during wallet creation and captures the intent of the user.

The blank flag is used only to indicate to that code that the wallet is intentionally blank and should not be setup as a new wallet.

I'd like to take this one step further and argue that "The blank flag is used only to indicate to that code that the wallet is intentionally blank and the user is responsible for managing the keys in this wallet manually." If user created wallet intentionally blank so the software doesn't touch the keys, does the intention go away after the keys are imported in the wallet? I don't think so.

Once something has been added to the wallet, it's no longer blank and the setup code will no longer recognize it as being "new", so the flag should be removed.

We shouldn't casually flip between just blank and intentionally blank. Yes, it's not longer blank, but it's still intentionally blank because this was and is the intent of the user. I think the word blank is just misleading here. We're better off replacing intentionally blank with manual.

murchandamus commented at 3:33 PM on March 17, 2023: contributor

ACK 23403d0ed81d2365f895ab4fab50f6bb87b44ad9

DrahtBot requested review from aureleoules on Mar 17, 2023

DrahtBot requested review from furszy on Mar 17, 2023

ishaanam commented at 6:45 PM on March 17, 2023: contributor

@achow101 could you please add release notes detailing how encrypting the wallet will now rotate the imported descriptors in a descriptor wallet? Other than that this looks good.

achow101 force-pushed on Mar 17, 2023

achow101 commented at 7:19 PM on March 17, 2023: member

Added a release note.

murchandamus commented at 7:24 PM on March 17, 2023: contributor

reACK abc328c06732aa468694d6c22217a12102ece70a

S3RK commented at 8:36 AM on March 20, 2023: contributor

Concept ACK. But approach wise I'd like us to preserve the intent of the user that the wallet is intended as manual (we should pick a better term). Would it be hard to create a new flag to persist that intent and respect that (e.g on encryption)?

achow101 commented at 3:48 PM on March 20, 2023: member

But approach wise I'd like us to preserve the intent of the user that the wallet is intended as manual (we should pick a better term). Would it be hard to create a new flag to persist that intent and respect that (e.g on encryption)?

I agree, but I think that's orthogonal to this change and should be done in a followup.

S3RK commented at 8:29 AM on March 21, 2023: contributor

As far as I understand after this PR is merged we will start unsetting the blank flag and loosing the users' intent. Are we okay with loosing the intent for the wallets that used the code between this and the follow up PR?

achow101 commented at 4:58 PM on March 21, 2023: member

As far as I understand after this PR is merged we will start unsetting the blank flag and loosing the users' intent.

We were already doing that in several places, the purpose of this PR is to unify the behavior.

If you make a blank legacy wallet, importing any script, address, pubkey, or compressed private key will currently unset the blank flag. If you import an uncompressed private key and your wallet is not encrypted, we would not unset the flag. I think that's a bug, which this PR fixes. Anything that rotates the HD seed, including encrypting a wallet, will also unset the blank flag in legacy wallets.

For descriptor wallets, we currently only unset the blank flag when creating born-encrypted wallets (which necessarily have the blank flag set when the wallet is created, but it is unset after keys are generated). We do not unset it for importing a descriptor, and encrypting a descriptor wallet with the blank flag set will not rotate the descriptors. This PR changes this so that importing a descriptor will unset the blank flag, which has a side effect of encrypting later will result in new descriptors being generated. The generating on encryption was not changed, although this is an oversight.

The changes to descriptor wallets is intended to make the blank flag behavior the same for both legacy and descriptor wallets. However given that people seem to think the blank flag has more meaning behind it, I'm okay with dropping the change to descriptor wallets. This will mean that the meaning of the blank flag will be different between the wallet types. For legacy wallets, it means that a wallet that has no keys or scripts with the flag set is supposed to be blank so that loading does not treat it as new. For descriptor wallets it will mean that the user wants to manage the descriptors themselves.

ishaanam commented at 10:32 PM on March 21, 2023: contributor

However given that people seem to think the blank flag has more meaning behind it, I'm okay with dropping the change to descriptor wallets.

I think dropping the change to descriptor wallets would make more sense here. Personally I have always thought that blank for descriptor wallets meant that the wallet does not contain any wallet-generated descriptors. Additionally, as @S3RK pointed out, this would allow for maintaining the current behavior until a new flag can be added.

achow101 commented at 3:36 PM on March 22, 2023: member

For reference, the original release note in 0.18.0 for blank is as follows:

createwallet now has an optional blank argument that can be used to create a blank wallet. Blank wallets do not have any keys or HD seed. They cannot be opened in software older than 0.18. Once a blank wallet has a HD seed set (by using sethdseed) or private keys, scripts, addresses, and other watch only things have been imported, the wallet is no longer blank and can be opened in 0.17.x. Encrypting a blank wallet will also set a HD seed for it.

achow101 force-pushed on Mar 22, 2023

achow101 commented at 5:50 PM on March 22, 2023: member

It turns out that encrypting a blank legacy wallet also does not unset the blank flag nor does it generate a new seed.

The latest push removes the descriptor wallet changes and adds tests for the behavior when encrypting.

S3RK commented at 7:27 AM on March 29, 2023: contributor

Code review ACK c5fb9a4dd97b47db0e52c497a757dff943b255b1

I'm still slightly hesitating though. One question that still bothers me is whether we should expose blank flag at all. It's an implementation detail and we only need it for the functional tests. It doesn't seem like there is any observable by the user behaviour of the flag, so why test it with functional tests? Unit tests are better fit for testing internal invariants.

This is a minor detail, and shouldn't block the PR, in the worst case we deprecate the flag and remove it from getwalletinfo RPC and move tests.

DrahtBot requested review from murchandamus on Mar 29, 2023

furszy commented at 8:08 PM on March 29, 2023: member

Before the text; this is not a blocking comment. Purely about adding some ideas.

I'm still slightly hesitating though. One question that still bothers me is whether we should expose blank flag at all. It's an implementation detail and we only need it for the functional tests. It doesn't seem like there is any observable by the user behaviour of the flag, so why test it with functional tests? Unit tests are better fit for testing internal invariants.

Hmm, I also agree that exposing an implementation detail isn't the best. More when we are planning to deprecate the flag.

But.. we currently don't have a way to return a "test only" result in the RPC commands. So far, the only functionality that is "similar" to that would be to mark the RPC result as optional and only return it on regtest/testnet (e.g. https://github.com/furszy/bitcoin-core/commit/4cfe14cb2a6e4ec9cd7e836917c7323c1ce607d5), which isn't the best but it works. Another idea, cleaner but a bit more complex. Would be to create a getwalletinfo wrapper: make a proxy function that calls getwalletinfo and adds the blank flag at the end only for regtest. Which would let us add more implementation details for testing only purposes in the future.

pablomartin4btc commented at 9:54 PM on April 5, 2023: member

re-ACK https://github.com/bitcoin/bitcoin/commit/c5fb9a4dd97b47db0e52c497a757dff943b255b1

DrahtBot added the label Needs rebase on May 2, 2023

achow101 force-pushed on May 2, 2023

DrahtBot removed the label Needs rebase on May 2, 2023

S3RK commented at 6:35 AM on May 8, 2023: contributor

re code review ACK 7baa0971527ba3c8d87b82c7a2a4eabf6abbb2b7

My previous comment about exposing blank flag is still valid, but I don't think it's blocking. I like furszy's suggestion to return the flag only for regtest

DrahtBot requested review from pablomartin4btc on May 8, 2023

DrahtBot added the label CI failed on May 30, 2023

in src/wallet/rpc/wallet.cpp:379 in 5f3c64c3fd outdated

 374 | @@ -375,6 +375,11 @@ static RPCHelpMan createwallet()
 375 |      uint64_t flags = 0;
 376 |      if (!request.params[1].isNull() && request.params[1].get_bool()) {
 377 |          flags |= WALLET_FLAG_DISABLE_PRIVATE_KEYS;
 378 | +        // We want to set BLANK when DISABLE_PRIVATE_KEYS, but only if the user did not explicitly do blank=false
 379 | +        // It can be set when blank is not provided; when it is provided, we handle it later

ryanofsky commented at 7:06 PM on June 5, 2023:

In commit "rpc, wallet: Set blank when disable private keys in createwallet" (5f3c64c3fd845228227e28b7cefa3e6c5c0ba7ef)

The comment is saying "We want to set BLANK when DISABLE_PRIVATE_KEYS" without saying why.

I think I would make the comment try to explain why to make the code understandable. Would suggest something like:

By default, try to set the BLANK flag when the DISABLE_PRIVATE_KEYS flag is set, to reflect the fact that the new wallet will not contain any keys or scripts or descriptors. The user can avoid setting the BLANK flag by explicitly passing a blank=false parameter. Setting the BLANK flag is not actually important, because the point of the flag is to prevent keys from being automatically generated, and the DISABLE_PRIVATE_KEYS flags already does that. Leaving the BLANK flag unset can be useful for compatibility, to allow the wallet to be loaded by old software that only supports DISABLE_PRIVATE_KEYS and not BLANK.

achow101 commented at 11:05 AM on June 8, 2023:

Done as suggested

in src/wallet/scriptpubkeyman.cpp:758 in 215694947a outdated

 754 | @@ -755,12 +755,12 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& s
 755 |          RemoveWatchOnly(script);
 756 |      }
 757 |  
 758 | +    m_storage.UnsetBlankWalletFlag(batch);

ryanofsky commented at 7:19 PM on June 5, 2023:

In commit "wallet: Ensure that the blank wallet flag is unset after imports" (215694947a5dce9e2f2617b7ee688256366d011e)

This commit doesn't seem to have any test coverage. Maybe that's ok because the change doesn't really effect visible behavior. But I was wondering if this hard to test for some other reason

achow101 commented at 11:06 AM on June 8, 2023:

It's tested in the new test. This should be more apparent with the tests being introduced per commit rather than all at the end.

in test/functional/wallet_blank.py:20 in 7baa097152 outdated

  15 | +    assert_equal,
  16 | +)
  17 | +from test_framework.wallet_util import bytes_to_wif
  18 | +
  19 | +
  20 | +class WalletBlankTest(BitcoinTestFramework):

ryanofsky commented at 7:48 PM on June 5, 2023:

In commit "tests: Test that the blank wallet flag is un/set as expected" (7baa0971527ba3c8d87b82c7a2a4eabf6abbb2b7)

Would be nice to add the test earlier as the second commit or part of the first commit, so it would be easily possible to see how other commits in the PR affect the test cases

achow101 commented at 11:33 AM on June 8, 2023:

I've split it up so that tests are introduced in the commits that fix the issues.

ryanofsky approved

ryanofsky commented at 8:38 PM on June 5, 2023: contributor

Code review ACK 7baa0971527ba3c8d87b82c7a2a4eabf6abbb2b7, but only lightly reviewed test. I suggested some documentation updates, but these are obviously not critical.

This change does add good test coverage and seems like an improvement over the status quo. I think I'm not really clear on the benefits of setting the blank flag in createwallet when the disable private keys flag is set, though. I wonder if we considered changing the GUI behavior to match RPC behavior instead of vice versa, and not setting the blank flag unnecessarily. I think even in the GUI something like a like a "Generate private keys" / "Do not generate private keys" / "Disallow private keys" radio box would be clearer than existing "Disable private keys" and "Make Blank Wallet" checkboxes.

DrahtBot removed review request from pablomartin4btc on Jun 5, 2023

DrahtBot requested review from pablomartin4btc on Jun 5, 2023

DrahtBot removed review request from pablomartin4btc on Jun 5, 2023

DrahtBot requested review from pablomartin4btc on Jun 5, 2023

DrahtBot removed the label CI failed on Jun 6, 2023

achow101 force-pushed on Jun 8, 2023

rpc, wallet: Include information about blank flag

This allows us to test that the blank flag is being set appropriately.

e9379f1ffa

achow101 force-pushed on Jun 8, 2023

DrahtBot added the label CI failed on Jun 8, 2023

in test/functional/wallet_blank.py:30 in 37dd054d9c outdated

  26 | @@ -27,11 +27,23 @@ def skip_test_if_missing_module(self):
  27 |      def add_options(self, options):
  28 |          self.add_wallet_options(options)
  29 |  
  30 | +    def test_createwallet(self):

ryanofsky commented at 1:17 PM on June 9, 2023:

In commit "rpc, wallet: Set blank when disable private keys in createwallet" (37dd054d9c7d58e829338c88df836241c0259f99)

These tests aren't actually being invoked in the current version of the PR. Need to call test_createwallet and test_createwallet_override functions in run_test below

achow101 commented at 7:55 PM on June 13, 2023:

Removed this test so this is no longer relevant.

ryanofsky commented at 3:49 PM on June 13, 2023: contributor

This PR has a lot of review (6 stale acks), and adds some very good test coverage. So maybe with an update and few reacks it could be merged.

I had a question about 2 test functions that didn't appear to be called in: #25634 (review)

I also don't think the change setting BLANK flank when DISABLE_PRIVATE_KEYS flag is set is a good one. BLANK seems like a confusing flag would be good to use less frequently rather than more frequently. Setting it by default when DISABLE_PRIVATE_KEYS is set has a minor practical drawback of making wallets that are supposed to be legacy wallet incompatible with older software. And I think the only claimed benefit of setting it is that it makes the RPC interface more similar to the GUI. However, it seems to me the GUI interface is actually pretty bad and confusing. The GUI would be simpler and clearer if it made blank and watch-only wallets separate choices instead of overlapping options.

So I'd be happier if the PR kept all the new test coverage and bugfixes but dropped the second commit "Set blank when disable private keys in createwallet" 37dd054d9c7d58e829338c88df836241c0259f99. I don't think the second commit is harmful if you really want to keep it, but it seems like it just adds complexity and a backwards compatibility corner case for no clear reason.

wallet: Ensure that the blank wallet flag is unset after imports 43310200dc

wallet: Document blank flag use in descriptor wallets cdba23db35

achow101 force-pushed on Jun 13, 2023

achow101 commented at 7:56 PM on June 13, 2023: member

The GUI would be simpler and clearer if it made blank and watch-only wallets separate choices instead of overlapping options.

I've done that in https://github.com/bitcoin-core/gui/pull/739

So I'd be happier if the PR kept all the new test coverage and bugfixes but dropped the second commit "Set blank when disable private keys in createwallet" 37dd054

Dropped it.

S3RK commented at 7:33 AM on June 14, 2023: contributor

reACK cdba23db353a1beff831ff4fc83d01ed64e8c2a9

Appreciate the perseverance in addressing all the comments and arriving at consensus. Thank for working on this.

DrahtBot removed review request from pablomartin4btc on Jun 14, 2023

DrahtBot requested review from pablomartin4btc on Jun 14, 2023

DrahtBot requested review from ryanofsky on Jun 14, 2023

ryanofsky commented at 1:22 PM on June 14, 2023: contributor

Appreciate the perseverance in addressing all the comments and arriving at consensus. Thank for working on this.

Agreed, thanks @achow101. I do think the end result is better, but that was a very long process... and the PR description is now slightly out of date too 😕.

I'm also wondering if the earlier discussion about adding a "manual" flag (https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1476485625) is still relevant. Is the blank flag already acting like manual flag for descriptor wallets now, or are there other effects setting a manual flag would have for descriptor wallets? (I assume we don't care about adding a manual flag for legacy wallets.)

DrahtBot removed review request from pablomartin4btc on Jun 14, 2023

DrahtBot requested review from pablomartin4btc on Jun 14, 2023

ryanofsky approved

ryanofsky commented at 1:26 PM on June 14, 2023: contributor

Code review ACK cdba23db353a1beff831ff4fc83d01ed64e8c2a9. Only change since last review is dropping the commit which makes createwallet RPC set BLANK flag automatically when DISABLE_PRIVATE_KEYS flag is set

DrahtBot removed review request from pablomartin4btc on Jun 14, 2023

DrahtBot requested review from pablomartin4btc on Jun 14, 2023

ryanofsky commented at 1:28 PM on June 14, 2023: contributor

I edited the PR description to drop the second paragraph #25634#issue-1308298704:

Additionally it unifies the behavior of the RPC with the GUI in that setting disable_private_keys will also automatically set blank. However, for backwards compatibility, users can choose to override this in the RPC by doing blank=False.

DrahtBot removed review request from pablomartin4btc on Jun 14, 2023

DrahtBot requested review from pablomartin4btc on Jun 14, 2023

DrahtBot removed review request from pablomartin4btc on Jun 14, 2023

DrahtBot requested review from pablomartin4btc on Jun 14, 2023

ryanofsky merged this on Jun 14, 2023

ryanofsky closed this on Jun 14, 2023

ryanofsky commented at 1:39 PM on June 14, 2023: contributor

I went ahead and merged this despite latest version only having 2 acks, because previous versions had more acks, and the only significant change dropping a commit (37dd054d9c7d58e829338c88df836241c0259f99 "Set blank when disable private keys in createwallet"). If anybody disagrees with dropping that, we can bring it back in another PR, but I thought it'd be good to merge the changes that seemed to have full agreement.

sidhujag referenced this in commit 4abc5f1752 on Jun 15, 2023

S3RK commented at 7:26 AM on June 19, 2023: contributor

I'm also wondering if the earlier discussion about adding a "manual" flag (#25634 (comment)) is still relevant. Is the blank flag already acting like manual flag for descriptor wallets now, or are there other effects setting a manual flag would have for descriptor wallets? (I assume we don't care about adding a manual flag for legacy wallets.)

Unless I'm missing something, the blank flag is already acting like manual for descriptors.

It's not unset on imports
It preservers descriptors on encryption

Now when blank flag diverged in meaning for descriptors wallet vs legacy. Do we want to add a highlight to release notes? Or change some other documentation?

bitcoin deleted a comment on Jun 19, 2023

ryanofsky commented at 7:16 PM on June 19, 2023: contributor

Now when blank flag diverged in meaning for descriptors wallet vs legacy. Do we want to add a highlight to release notes?

As far as I know, difference was introduced when descriptor wallets were introduced, so there would not be a need for release notes. But it would be good to make sure RPCs are properly documented, so if you encrypt a wallet it is clear in what cases new keys will be generated. Also if we have any documentation contrasting legacy wallets and descriptor wallets we could have it mention this difference. I don't think it would be good for documentation to make a lot of references to the BLANK flag specifically. Better to just be document when keys will and won't be generated, and only mention about flag if it actually helps clarify behavior.

hernanmarino commented at 4:30 PM on June 20, 2023: contributor

post merge ACK . Also +1 to @ryanofsky latest comment.

pablomartin4btc commented at 4:00 PM on June 23, 2023: member

post-merge re-ACK with dropping 3rd commit (which only had 2 ACKs and there was the debate on: setting BLANK flag on RPC automatically when DISABLE_PRIVATE_KEYS flag is set), going to review at the GUI's follow-up on this by @achow101 that would fix the inconsistency there.

luke-jr referenced this in commit 9dd03e144e on Aug 16, 2023

luke-jr referenced this in commit 8c52125976 on Aug 16, 2023

bitcoin locked this on Jun 22, 2024