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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Reviewers, this pull request conflicts with the following ones:
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.
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
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
0{
1 "walletname": "testing",
2 "walletversion": 169900,
3 "format": "sqlite",
4 "balance": 0.00000000,
5 "unconfirmed_balance": 0.00000000,
6 "immature_balance": 0.00000000,
7 "txcount": 0,
8 "keypoolsize": 0,
9 "keypoolsize_hd_internal": 0,
10 "paytxfee": 0.00000000,
11 "private_keys_enabled": false,
12 "avoid_reuse": false,
13 "scanning": false,
14 "descriptors": true,
15 "external_signer": false,
16 "blank": false # shouldn't this be overridden by true?
17}
blank=false overrides the default behavior.
However, for backwards compatibility, users can choose to override this in the RPC by doing blank=False.
I guess that’s the reason.
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.
57+ eckey = ECKey()
58+ eckey.generate()
59+
60+ wallet.importpubkey(eckey.get_pubkey().get_bytes().hex())
61+ assert_equal(wallet.getwalletinfo()["blank"], False)
62+
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?
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.
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
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"]
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:
0{'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:
0{'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}]}
Yes. This seems like a bug. I was able to trace it back to encryption function
src/wallet/wallet.cpp:829
0 // If we are using descriptors, make new descriptors with a new seed
1 if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
2 SetupDescriptorScriptPubKeyMans();
@achow101 why do we want to regenerate descriptors on encryption?
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.
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.
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.
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.
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.
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:
0--- a/src/wallet/walletutil.h
1+++ b/src/wallet/walletutil.h
2@@ -53,10 +53,18 @@ enum WalletFlags : uint64_t {
3 //! Flag set when a wallet contains no HD seed and no private keys, scripts,
4 //! addresses, and other watch only things, and is therefore "blank."
5 //!
6- //! The only function this flag serves is to distinguish a blank wallet from
7+ //! The main function this flag serves is to distinguish a blank wallet from
8 //! a newly created wallet when the wallet database is loaded, to avoid
9 //! initialization that should only happen on first run.
10 //!
11+ //! A secondary function of this flag, which applies to descriptor wallets
12+ //! only, is to serve as an ongoing indication that descriptors in the
13+ //! wallet should be created manually, and that the wallet should not
14+ //! generate automatically generate new descriptors if it is later
15+ //! encrypted. To support this behavior, descriptor wallets unlike legacy
16+ //! wallets do not automatically unset the BLANK flag when things are
17+ //! imported.
18+ //!
19 //! This flag is also a mandatory flag to prevent previous versions of
20 //! bitcoin from opening the wallet, thinking it was newly created, and
21 //! then improperly reinitializing it.
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”.
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.
Hmm, wouldn’t be sufficient to differentiate blank from newly created wallets by providing a flag to the
CreateWalletfunction 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.
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"},
Using “not” in the context of a boolean feels always a bit like a double negation. Perhaps go with “without”:
0 {RPCResult::Type::BOOL, "blank", "whether this wallet was intentionally created without keys, scripts, or descriptors"},
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)
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.
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)?
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.
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.
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.
For reference, the original release note in 0.18.0 for blank is as follows:
createwalletnow has an optionalblankargument 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 usingsethdseed) 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.
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.
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.
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
blankflag 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.
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
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
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:
754@@ -755,12 +755,12 @@ bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& s
755 RemoveWatchOnly(script);
756 }
757
758+ m_storage.UnsetBlankWalletFlag(batch);
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
15+ assert_equal,
16+)
17+from test_framework.wallet_util import bytes_to_wif
18+
19+
20+class WalletBlankTest(BitcoinTestFramework):
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
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.
This allows us to test that the blank flag is being set appropriately.
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):
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
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.
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.
reACK cdba23db353a1beff831ff4fc83d01ed64e8c2a9
Appreciate the perseverance in addressing all the comments and arriving at consensus. Thank for working on this.
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.)
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_keyswill also automatically setblank. However, for backwards compatibility, users can choose to override this in the RPC by doingblank=False.
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.
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?
Now when
blankflag 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.