wallet: Always set descriptor cache upgraded flag for new wallets

achow101 commented at 0:39 am on May 23, 2025: member

Newly created wallets will always have an upgraded descriptor cache, so set those.

Also, to verify this behavior, add a new flags field to getwalletinfo and check that in the functional tests.

Split from #32489

wallet: Set upgraded descriptor cache flag for newly created wallets

Although WalletBatch::LoadWallet performs the descriptor cache upgrade,
because new wallets do not have the descriptor flag set yet, the upgrade
does not run and set the flag.

Since new wallets will always being using the upgraded cache, there's no
reason to wait to set the flag, so set it when the wallet flags are
being initialized for new wallets.

69f588a99a

wallet: Add GetWalletFlags bc2a26b296

wallet, rpc: Output wallet flags in getwalletinfo 47237cd193

DrahtBot commented at 0:39 am on May 23, 2025: contributor

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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, w0xlt, rkrux
Concept ACK	adyshimony

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

DrahtBot added the label Wallet on May 23, 2025

adyshimony commented at 10:16 pm on May 26, 2025: none

ACK

47237cd

Manual tests:

 0
 1./bitcoin-cli createwallet "test_32597_wallet" false false "" false true
 2{
 3  "name": "test_32597_wallet",
 4  "warnings": [
 5    "Empty string given as passphrase, wallet will not be encrypted."
 6  ]
 7}
 8
 9
10
11./bitcoin-cli -rpcwallet=test_32597_wallet getwalletinfo
12{
13  "walletname": "test_32597_wallet",
14  "walletversion": 169900,
15  "format": "sqlite",
16  "balance": 0.00000000,
17  "unconfirmed_balance": 0.00000000,
18  "immature_balance": 0.00000000,
19  "txcount": 0,
20  "keypoolsize": 4000,
21  "keypoolsize_hd_internal": 4000,
22  "paytxfee": 0.00000000,
23  "private_keys_enabled": true,
24  "avoid_reuse": false,
25  "scanning": false,
26  "descriptors": true,
27  "external_signer": false,
28  "blank": false,
29  "birthtime": 1748289166,
30  "flags": [
31    "last_hardened_xpub_cached",
32    "descriptor_wallet"
33  ],
34  "lastprocessedblock": {
35    "hash": "0000000000000000000104185d6d17742c6f4e4a7b005af0e14c55176037ad95",
36    "height": 898492
37  }
38}

Functional tests:

 0build/test/functional/wallet_avoidreuse.py
 12025-05-26T20:24:39.668000Z TestFramework (INFO): PRNG seed is: 1859031226962638324
 22025-05-26T20:24:39.669000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_b3fqengx
 32025-05-26T20:24:40.193000Z TestFramework (INFO): Test wallet files persist avoid_reuse flag
 42025-05-26T20:24:40.761000Z TestFramework (INFO): Test immutable wallet flags
 52025-05-26T20:24:42.050000Z TestFramework (INFO): Test that change doesn't turn into non-change when spent
 62025-05-26T20:24:42.135000Z TestFramework (INFO): Test sending from reused address with avoid_reuse=false
 72025-05-26T20:24:42.289000Z TestFramework (INFO): Test sending from reused legacy address fails
 82025-05-26T20:24:43.382000Z TestFramework (INFO): Test sending from reused p2sh-segwit address fails
 92025-05-26T20:24:43.452000Z TestFramework (INFO): Test sending from reused bech32 address fails
102025-05-26T20:24:43.531000Z TestFramework (INFO): Test getbalances used category
112025-05-26T20:24:45.734000Z TestFramework (INFO): Test that full destination groups are preferred in coin selection
122025-05-26T20:24:47.031000Z TestFramework (INFO): Test that all destination groups are used
132025-05-26T20:24:49.956000Z TestFramework (INFO): Stopping nodes
142025-05-26T20:24:50.010000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_b3fqengx on exit
152025-05-26T20:24:50.010000Z TestFramework (INFO): Tests successful

 0build/test/functional/wallet_createwallet.py
 12025-05-26T20:25:07.813000Z TestFramework (INFO): PRNG seed is: 2684982768405465258
 22025-05-26T20:25:07.814000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_p9897jvk
 32025-05-26T20:25:08.134000Z TestFramework (INFO): Run createwallet with invalid parameters.
 42025-05-26T20:25:08.150000Z TestFramework (INFO): Test disableprivatekeys creation.
 52025-05-26T20:25:08.169000Z TestFramework (INFO): Test that private keys cannot be imported
 62025-05-26T20:25:08.182000Z TestFramework (INFO): Test blank creation with private keys disabled.
 72025-05-26T20:25:08.199000Z TestFramework (INFO): Test blank creation with private keys enabled.
 82025-05-26T20:25:08.255000Z TestFramework (INFO): Test blank creation with privkeys enabled and then encryption
 92025-05-26T20:25:08.691000Z TestFramework (INFO): Test blank creation with privkeys disabled and then encryption
102025-05-26T20:25:08.711000Z TestFramework (INFO): New blank and encrypted wallets can be created
112025-05-26T20:25:09.108000Z TestFramework (INFO): Test creating a new encrypted wallet.
122025-05-26T20:25:09.547000Z TestFramework (INFO): Test making a wallet with avoid reuse flag
132025-05-26T20:25:09.563000Z TestFramework (INFO): Using a passphrase with private keys disabled returns error
142025-05-26T20:25:09.564000Z TestFramework (INFO): Test that legacy wallets cannot be created
152025-05-26T20:25:09.565000Z TestFramework (INFO): Check that the version number is being logged correctly
162025-05-26T20:25:09.658000Z TestFramework (INFO): Stopping nodes
172025-05-26T20:25:09.810000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_p9897jvk on exit
182025-05-26T20:25:09.810000Z TestFramework (INFO): Tests successful

bitcoin-cli help shows the new flags field:

 0build/bin/bitcoin-cli help getwalletinfo
 1getwalletinfo
 2
 3Returns an object containing various wallet state info.
 4
 5Result:
 6{                                         (json object)
 7  "walletname" : "str",                   (string) the wallet name
 8  "walletversion" : n,                    (numeric) the wallet version
 9  "format" : "str",                       (string) the database format (only sqlite)
10  "balance" : n,                          (numeric) DEPRECATED. Identical to getbalances().mine.trusted
11  "unconfirmed_balance" : n,              (numeric) DEPRECATED. Identical to getbalances().mine.untrusted_pending
12  "immature_balance" : n,                 (numeric) DEPRECATED. Identical to getbalances().mine.immature
13  "txcount" : n,                          (numeric) the total number of transactions in the wallet
14  "keypoololdest" : xxx,                  (numeric, optional) the UNIX epoch time of the oldest pre-generated key in the key pool. Legacy wallets only.
15  "keypoolsize" : n,                      (numeric) how many new keys are pre-generated (only counts external keys)
16  "keypoolsize_hd_internal" : n,          (numeric, optional) how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)
17  "unlocked_until" : xxx,                 (numeric, optional) the UNIX epoch time until which the wallet is unlocked for transfers, or 0 if the wallet is locked (only present for passphrase-encrypted wallets)
18  "paytxfee" : n,                         (numeric) the transaction fee configuration, set in BTC/kvB
19  "private_keys_enabled" : true|false,    (boolean) false if privatekeys are disabled for this wallet (enforced watch-only wallet)
20  "avoid_reuse" : true|false,             (boolean) whether this wallet tracks clean/dirty coins in terms of reuse
21  "scanning" : {                          (json object) current scanning details, or false if no scan is in progress
22    "duration" : n,                       (numeric) elapsed seconds since scan start
23    "progress" : n                        (numeric) scanning progress percentage [0.0, 1.0]
24  },
25  "descriptors" : true|false,             (boolean) whether this wallet uses descriptors for output script management
26  "external_signer" : true|false,         (boolean) whether this wallet is configured to use an external signer such as a hardware wallet
27  "blank" : true|false,                   (boolean) Whether this wallet intentionally does not contain any keys, scripts, or descriptors
28  "birthtime" : xxx,                      (numeric, optional) The start time for blocks scanning. It could be modified by (re)importing any descriptor with an earlier timestamp.
29  "flags" : [                             (json array) The flags currently set on the wallet
30    "str",                                (string) The name of the flag
31    ...
32  ],
33  "lastprocessedblock" : {                (json object) hash and height of the block this information was generated on
34    "hash" : "hex",                       (string) hash of the block this information was generated on
35    "height" : n                          (numeric) height of the block this information was generated on
36  }
37}

in src/wallet/wallet.cpp:2904 in 69f588a99a outdated

2903@@ -2904,7 +2904,9 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2904         // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key

rkrux commented at 5:50 pm on May 27, 2025:

In 69f588a99a7a79d1d72300bc0f2c8475f95f6c6a

Since new wallets will always being using the upgraded cache, there’s no reason to wait to set the flag, so set it when the wallet flags are being initialized for new wallets.

Conceptually, it makes sense to me that the WALLET_FLAG_LAST_HARDENED_XPUB_CACHED is set at the time of wallet creation.

Although WalletBatch::LoadWallet performs the descriptor cache upgrade, because new wallets do not have the descriptor flag set yet, the upgrade does not run and set the flag.

However, as mentioned above^ and as I checked in the code as well, since the cache upgrade is not run, it therefore is not recorded in the DB as well. If so far the presence of this flag leads to a presence of the corresponding DB entry, it’d be preferable if this behaviour is preserved here as well, which I believe is not happening. I will double check if this can lead to errors in some other parts of the wallet flows.

achow101 commented at 6:38 pm on May 27, 2025:

There can be no DB entries when upgrade is run on a newly created wallet as the descriptors don’t even exist yet.

in src/wallet/wallet.cpp:2909 in 69f588a99a outdated

2903@@ -2904,7 +2904,9 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
2904         // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
2905         walletInstance->SetMinVersion(FEATURE_LATEST);
2906 
2907-        walletInstance->InitWalletFlags(wallet_creation_flags);
2908+        // Init with passed flags.
2909+        // Always set the cache upgrade flag as this feature is supported from the beginning.
2910+        walletInstance->InitWalletFlags(wallet_creation_flags | WALLET_FLAG_LAST_HARDENED_XPUB_CACHED);

rkrux commented at 6:02 pm on May 27, 2025:

I don’t suppose it’s a big issue but while going through the creation flow it seems odd that this flag is set prior to even the setup of the SPKMs. I have not tried it but from a code flow perspective a call to UpgradeDescriptorCache can come right after SetupDescriptorScriptPubKeyMans that will set both the cache keys in the database along with this flag.

achow101 commented at 6:41 pm on May 27, 2025:

The only behavior that is conditional on this flag is whether the upgrade needs to be run. It exists only as a startup optimization to ensure that we aren’t re-running the same upgrade over and over. The DescriptorScriptPubKeyMan will always write the records for the upgraded cache, regardless of whether the flag exists. There is no need to upgrade the cache either as the upgraded cache is always used regardless of the flag.

rkrux commented at 7:00 pm on June 17, 2025:

I finally got a chance to dive deeper and found that the corresponding caches values are recorded in the DB via TopUpWithDB inside SetupDescriptorGeneration through SetupDescriptorScriptPubKeyMans in the first run of the wallet, i.e., creation. So, as answered by your reply, there is no need to call UpgradeDescriptorCache right after, contrary to what I suggested in my first comment. https://github.com/bitcoin/bitcoin/blob/5e6dbfd14ea9eace1c7e5ee76b140be46a0ec855/src/wallet/scriptpubkeyman.cpp#L1050-L1054

But it does not set the wallet flag, and hence the need of this PR.

It exists only as a startup optimization to ensure that we aren’t re-running the same upgrade over and over.

I wanted to see how the flow would run the same upgrade over and over for which I checked out master, created a new wallet, unloaded it, & then loaded it - then repeated the unload/load activity. I found that after creation, the flag is not set which is incorrect and this PR will fix it. On the first load after creation, the CWallet::UpgradeDescriptorCache is run because of absence of the flag and the DescriptorScriptPubKeyMan::UpgradeDescriptorCache is also entered but the actual descriptor expansion and cache insertion in DB is not done because of the below check. Even though the flag was not set during creation but the m_last_hardened_xpubs was still populated. https://github.com/bitcoin/bitcoin/blob/5e6dbfd14ea9eace1c7e5ee76b140be46a0ec855/src/wallet/scriptpubkeyman.cpp#L1545-L1548

And on the second load, CWallet::UpgradeDescriptorCache early returns because the flag was set in the previous load. So, I don’t suppose the upgrade would run over and over?

achow101 commented at 9:38 pm on June 17, 2025:

UpgradeDescriptorCache is always called, so it always runs. However, the check for the flag is also always done first, so the actual upgrade itself will be skipped.

rkrux commented at 6:04 pm on May 27, 2025: contributor

Concept ACK, code review 69f588a99a7a79d1d72300bc0f2c8475f95f6c6a

Newly created wallets will always have an upgraded descriptor cache, so set those.

Besides the logical accuracy, is there any other advantage of doing this that is used in #32489? Probably can mention it in the PR description.

Sjors commented at 12:35 pm on June 5, 2025: member

ACK 47237cd1938058b29fdec242c3a37611e255fda0

Also, to verify this behavior, add a new flags field to getwalletinfo and check that in the functional tests.

I checked that reverting the first commit breaks the new tests.

DrahtBot requested review from rkrux on Jun 5, 2025

in src/wallet/wallet.h:163 in 47237cd193

166-    {"key_origin_metadata", WALLET_FLAG_KEY_ORIGIN_METADATA},
167-    {"last_hardened_xpub_cached", WALLET_FLAG_LAST_HARDENED_XPUB_CACHED},
168-    {"disable_private_keys", WALLET_FLAG_DISABLE_PRIVATE_KEYS},
169-    {"descriptor_wallet", WALLET_FLAG_DESCRIPTORS},
170-    {"external_signer", WALLET_FLAG_EXTERNAL_SIGNER}
171+static const std::map<WalletFlags, std::string> WALLET_FLAG_TO_STRING{

w0xlt commented at 9:48 pm on June 11, 2025:

nit: string_view can be used, as it is read-only access to a string and can avoid unnecessary memory allocation and copying. In this case, you also would need to do:

0static RPCHelpMan setwalletflag()
1{
2            std::string flags;
3            for (auto& it : STRING_TO_WALLET_FLAG)
4                if (it.second & MUTABLE_WALLET_FLAGS)
5-                    flags += (flags == "" ? "" : ", ") + it.first;             
6+                    flags += (flags == "" ? "" : ", ") + std::string(it.first);

0static const std::map<WalletFlags, std::string_view> WALLET_FLAG_TO_STRING{

w0xlt commented at 9:48 pm on June 11, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32597/commits/47237cd1938058b29fdec242c3a37611e255fda0

rkrux approved

rkrux commented at 7:31 pm on June 17, 2025: contributor

ACK 47237cd1938058b29fdec242c3a37611e255fda0

I have verified and agree that this flag should be setup during wallet creation. The addition of the wallet flags in the RPC response in this PR is an added bonus that also closes the loop on the wallet flag integer values & their corresponding string representations.

wallet: Always set descriptor cache upgraded flag for new wallets #32597

Code Coverage & Benchmarks

Reviews