wallet: Set descriptor cache upgraded flag for migrated wallets #33031

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:lasthardened-cache-migratewallet changing 4 files +67 −15
  1. achow101 commented at 10:33 pm on July 21, 2025: member

    #32597 set the descriptor cache upgraded flag for newly created wallets, but migrated wallets still did not have the flag set when they are migrated. For consistency, and to avoid an unnecessary upgrade, we should be setting this flag for migrated wallets.

    The flag would end up being set anyways at the end of migration when the wallet is reloaded as it would perform the automatic upgrade at that time. However, this is unnecessary and we should just set it from the get go.

    This PR also adds a couple tests to verify that the flag is being set, and that the upgrade is being performed.

  2. DrahtBot added the label Wallet on Jul 21, 2025
  3. DrahtBot commented at 10:33 pm on July 21, 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/33031.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK rkrux, pablomartin4btc
    Stale ACK w0xlt, cedwies

    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:

    • #33230 (cli: Handle arguments that can be either JSON or string by achow101)
    • #33034 (wallet: Store transactions in a separate sqlite table 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.

  4. achow101 force-pushed on Jul 21, 2025
  5. DrahtBot added the label CI failed on Jul 21, 2025
  6. DrahtBot commented at 11:11 pm on July 21, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46428278739 LLM reason (✨ experimental): The CI failure is caused by lint errors due to undefined ‘self’ references in Python test files.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  7. DrahtBot removed the label CI failed on Jul 22, 2025
  9. rkrux commented at 1:06 pm on July 25, 2025: contributor
    I deleted my previous updated suggestion regarding importing pysqlite3 because it is not considerably better than the current implementation.
  10. cedwies commented at 9:08 pm on July 25, 2025: none
    • Unit tests (test_bitcoin) passed in 91.67 s (macOS Debug build)

    questions/observations:

    1. why set WALLET_FLAG_DESCRIPTORS and WALLET_FLAG_LAST_HARDENED_XPUB_CACHED in a single db write?
    2. If the xpub cache flag was already true, could re-setting it trigger any unexpected behavior?
    3. nit: please update the comment above the SetWalletFlagWithDB call to mention that it now also caches the last-hardened xpub.

    ACK 4e5faab

  11. DrahtBot added the label Needs rebase on Jul 29, 2025
  12. achow101 force-pushed on Jul 29, 2025
  13. DrahtBot removed the label Needs rebase on Jul 29, 2025
  14. in test/functional/wallet_backwards_compatibility.py:409 in 3854d6b56c outdated 
    404+
405+                    # Check for last hardened xpubs if the flag is newly set
406+                    if diff_flags & (1 << 2):
407+                        self.log.debug("Checking descriptor cache was upgraded")
408+                        # Fetch all records with the walletdescriptorlhcache prefix
409+                        lh_cache_recs = conn.execute("SELECT value FROM main where key >= x'1777616c6c657464657363726970746f726c686361636865' AND key < x'1777616c6c657464657363726970746f726c686361636866'").fetchall()
    rkrux commented at 10:34 am on July 30, 2025:

    In 3854d6b56c50b26f7b61dfb37f5230726d561cf9 “tests: Check that the last hardened cache upgrade occurs”

    At the stage where there are multiple such instances of hardcoding the hex representation of the database keys in the tests, I would introduce a get_db_key_hex utility. Mentioned in overall review comment.

    achow101 commented at 11:25 pm on July 30, 2025:
    I’ve changed this to use the existing ser_string utility which is more accurate.
    rkrux commented at 12:06 pm on July 31, 2025:
    I see, yes ser_string seems thorough.
  15. in test/functional/wallet_migration.py:41 in 3854d6b56c outdated 
    32@@ -32,6 +33,13 @@
33     generate_keypair,
34 )
35 
36+SQLITE3_IMPORTED = False
37+try:
38+    import sqlite3 # type: ignore[import]
39+    SQLITE3_IMPORTED = True
40+except ImportError:
41+    pass
    rkrux commented at 10:37 am on July 30, 2025:

    In 3854d6b56c50b26f7b61dfb37f5230726d561cf9 “tests: Check that the last hardened cache upgrade occurs”

    I think I have found a way of getting rid of this kind of duplicated try-import-except-setfileconstant instances in multiple files by introducing a inspect_database function in the test framework. Mentioned in overall review comment.

    I think it would be quite nice to have this inspect function but I don’t feel like holding the ack on the PR only for this.

    achow101 commented at 11:25 pm on July 30, 2025:
    Done as suggested.
  16. rkrux commented at 10:40 am on July 30, 2025: contributor

    tACK aa08126836f9acdf17790837aaa099500f2a5863

    This builds on a similar change done in #32597. Adding the tests before the flag change on the migrated wallet ensures this flag was added before as well but at a slightly later state in the migration flow.

    Combined diff of the inline-comments here that passes on my system.

      0diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
  1index 639421d3ae..15b5f05966 100755
  2--- a/test/functional/test_framework/test_framework.py
  3+++ b/test/functional/test_framework/test_framework.py
  4@@ -952,6 +952,17 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
  5         except ImportError:
  6             raise SkipTest("sqlite3 module not available.")
  7 
  8+    def inspect_database(self, path, queries_executor, *args, **kwargs):
  9+        try:
 10+            import sqlite3 # type: ignore[import]
 11+            conn = sqlite3.connect(path)
 12+            with conn:
 13+                result = queries_executor(conn, *args, **kwargs)
 14+            # Connection needs to be manually closed: https://docs.python.org/3/library/sqlite3.html#sqlite3-connection-context-manager
 15+            conn.close()
 16+            return result
 17+        except ImportError:
 18+            self.log.warning("sqlite3 module not available, skipping tests that inspect the database")
 19+
 20     def skip_if_no_python_bcc(self):
 21         """Attempt to import the bcc package and skip the tests if the import fails."""
 22         try:
 23diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
 24index e5a5938f07..1bc713a12b 100644
 25--- a/test/functional/test_framework/util.py
 26+++ b/test/functional/test_framework/util.py
 27@@ -619,3 +619,6 @@ def wallet_importprivkey(wallet_rpc, privkey, timestamp, *, label=""):
 28     }]
 29     import_res = wallet_rpc.importdescriptors(req)
 30     assert_equal(import_res[0]["success"], True)
 31+
 32+def get_db_key_hex(db_key_string):
 33+    return (f"{len(db_key_string):02x}{db_key_string.encode().hex()}")
 34diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py
 35index 0ce836d850..c07fd2b5ec 100755
 36--- a/test/functional/wallet_backwards_compatibility.py
 37+++ b/test/functional/wallet_backwards_compatibility.py
 38@@ -26,15 +26,9 @@ from test_framework.util import (
 39     assert_equal,
 40     assert_greater_than,
 41     assert_raises_rpc_error,
 42+    get_db_key_hex,
 43 )
 44 
 45-SQLITE3_IMPORTED = False
 46-try:
 47-    import sqlite3 # type: ignore[import]
 48-    SQLITE3_IMPORTED = True
 49-except ImportError:
 50-    pass
 51-
 52 LAST_KEYPOOL_INDEX = 9 # Index of the last derived address with the keypool size of 10
 53 
 54 class BackwardsCompatibilityTest(BitcoinTestFramework):
 55@@ -58,9 +52,6 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
 56         self.skip_if_no_wallet()
 57         self.skip_if_no_previous_releases()
 58 
 59-        if not SQLITE3_IMPORTED:
 60-            self.log.warning("sqlite3 module not available, skipping tests that inspect the database")
 61-
 62     def setup_nodes(self):
 63         self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[
 64             None,
 65@@ -161,14 +152,12 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
 66         bad_deriv_wallet_master.unloadwallet()
 67 
 68         # If we have sqlite3, verify that there are no keymeta records
 69-        if SQLITE3_IMPORTED:
 70-            wallet_db = node_master.wallets_path / wallet_name / "wallet.dat"
 71-            conn = sqlite3.connect(wallet_db)
 72-            with conn:
 73-                # Retrieve all records that have the "keymeta" prefix. The remaining key data varies for each record.
 74-                keymeta_rec = conn.execute("SELECT value FROM main where key >= x'076b65796d657461' AND key < x'076b65796d657462'").fetchone()
 75-                assert_equal(keymeta_rec, None)
 76-            conn.close()
 77+        def assert_no_keymeta_records(conn):
 78+            # Retrieve all records that have the "keymeta" prefix. The remaining key data varies for each record.
 79+            keymeta_rec = conn.execute(f"SELECT value FROM main where key >= x'{get_db_key_hex('keymeta')}' AND key < x'{get_db_key_hex('keymetb')}'").fetchone()
 80+            assert_equal(keymeta_rec, None)
 81+
 82+        self.inspect_database(node_master.wallets_path / wallet_name / "wallet.dat", assert_no_keymeta_records)
 83 
 84     def test_ignore_legacy_during_startup(self, legacy_nodes, node_master):
 85         self.log.info("Test that legacy wallets are ignored during startup on v29+")
 86@@ -351,12 +340,11 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
 87             wallet_prev.unloadwallet()
 88 
 89             # Open backup with sqlite and check flags
 90-            if SQLITE3_IMPORTED:
 91-                conn = sqlite3.connect(backup_path)
 92-                with conn:
 93-                    flags_rec = conn.execute("SELECT value FROM main WHERE key = x'05666c616773'").fetchone()
 94-                    old_flags = int.from_bytes(flags_rec[0], byteorder="little")
 95-                conn.close()
 96+            def retrieve_flags_of_backup(conn):
 97+                    flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{get_db_key_hex('flags')}'").fetchone()
 98+                    return int.from_bytes(flags_rec[0], byteorder="little")
 99+
100+            old_flags = self.inspect_database(backup_path, retrieve_flags_of_backup)
101 
102             # Restore the wallet to master
103             load_res = node_master.restorewallet(wallet_name, backup_path)
104@@ -395,21 +383,17 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
105             wallet.unloadwallet()
106 
107             # Open the wallet with sqlite and inspect the flags and records
108-            if SQLITE3_IMPORTED:
109-                conn = sqlite3.connect(down_backup_path)
110-                with conn:
111-                    flags_rec = conn.execute("SELECT value FROM main WHERE key = x'05666c616773'").fetchone()
112-                    new_flags = int.from_bytes(flags_rec[0], byteorder="little")
113-                    diff_flags = new_flags & ~old_flags
114-
115-                    # Check for last hardened xpubs if the flag is newly set
116-                    if diff_flags & (1 << 2):
117-                        self.log.debug("Checking descriptor cache was upgraded")
118-                        # Fetch all records with the walletdescriptorlhcache prefix
119-                        lh_cache_recs = conn.execute("SELECT value FROM main where key >= x'1777616c6c657464657363726970746f726c686361636865' AND key < x'1777616c6c657464657363726970746f726c686361636866'").fetchall()
120-                        assert_greater_than(len(lh_cache_recs), 0)
121-
122-                conn.close()
123+            def assert_flags_records(conn, old_flags):
124+                flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{get_db_key_hex('flags')}'").fetchone()
125+                new_flags = int.from_bytes(flags_rec[0], byteorder="little")
126+                diff_flags = new_flags & ~old_flags
127+                # Check for last hardened xpubs if the flag is newly set
128+                if diff_flags & (1 << 2):
129+                    self.log.debug("Checking descriptor cache was upgraded")
130+                    lh_cache_recs = conn.execute(f"SELECT value FROM main where key >= x'{get_db_key_hex('walletdescriptorlhcache')}' AND key < x'{get_db_key_hex('walletdescriptorlhcachf')}'").fetchall()
131+                    assert_greater_than(len(lh_cache_recs), 0)
132+
133+            self.inspect_database(down_backup_path, assert_flags_records, old_flags)
134 
135             # Check that no automatic upgrade broke downgrading the wallet
136             target_dir = node.wallets_path / down_wallet_name
137diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
138index 9ee27a5765..9b9a3b6b48 100755
139--- a/test/functional/wallet_migration.py
140+++ b/test/functional/wallet_migration.py
141@@ -27,19 +27,13 @@ from test_framework.util import (
142     assert_raises_rpc_error,
143     find_vout_for_address,
144     sha256sum_file,
145+    get_db_key_hex,
146 )
147 from test_framework.wallet_util import (
148     get_generate_key,
149     generate_keypair,
150 )
151 
152-SQLITE3_IMPORTED = False
153-try:
154-    import sqlite3 # type: ignore[import]
155-    SQLITE3_IMPORTED = True
156-except ImportError:
157-    pass
158-
159 BTREE_MAGIC = 0x053162
160 
161 
162@@ -54,9 +48,6 @@ class WalletMigrationTest(BitcoinTestFramework):
163         self.skip_if_no_wallet()
164         self.skip_if_no_previous_releases()
165 
166-        if not SQLITE3_IMPORTED:
167-            self.log.warning("sqlite3 module not available, skipping tests that inspect the database")
168-
169     def setup_nodes(self):
170         self.add_nodes(
171             self.num_nodes,
172@@ -165,26 +156,22 @@ class WalletMigrationTest(BitcoinTestFramework):
173 
174         # Open the wallet with sqlite and verify that the wallet has the last hardened cache flag
175         # set and the last hardened cache entries
176-        if SQLITE3_IMPORTED:
177-            inspect_path = os.path.join(self.options.tmpdir, os.path.basename(f"{migrated_wallet_name}_inspect.dat"))
178-            wallet.backupwallet(inspect_path)
179-
180-            conn = sqlite3.connect(inspect_path)
181-
182-            with conn:
183-                flags_rec = conn.execute("SELECT value FROM main WHERE key = x'05666c616773'").fetchone()
184-                flags = int.from_bytes(flags_rec[0], byteorder="little")
185-
186-                # All wallets should have the upgrade flag set
187-                assert_equal(bool(flags & (1 << 2)), True)
188-
189-                # Fetch all records with the walletdescriptorlhcache prefix
190-                # if the wallet has private keys and is not blank
191-                if wallet_info["private_keys_enabled"] and not wallet_info["blank"]:
192-                    lh_cache_recs = conn.execute("SELECT value FROM main where key >= x'1777616c6c657464657363726970746f726c686361636865' AND key < x'1777616c6c657464657363726970746f726c686361636866'").fetchall()
193-                    assert_greater_than(len(lh_cache_recs), 0)
194-
195-            conn.close()
196+        def check_last_hardened_data(conn, wallet_info):
197+            flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{get_db_key_hex('flags')}'").fetchone()
198+            flags = int.from_bytes(flags_rec[0], byteorder="little")
199+
200+            # All wallets should have the upgrade flag set
201+            assert_equal(bool(flags & (1 << 2)), True)
202+
203+            # Fetch all records with the walletdescriptorlhcache prefix
204+            # if the wallet has private keys and is not blank
205+            if wallet_info["private_keys_enabled"] and not wallet_info["blank"]:
206+                lh_cache_recs = conn.execute(f"SELECT value FROM main where key >= x'{get_db_key_hex('walletdescriptorlhcache')}' AND key < x'{get_db_key_hex('walletdescriptorlhcachf')}'").fetchall()
207+                assert_greater_than(len(lh_cache_recs), 0)
208+
209+        inspect_path = os.path.join(self.options.tmpdir, os.path.basename(f"{migrated_wallet_name}_inspect.dat"))
210+        wallet.backupwallet(inspect_path)
211+        self.inspect_database(inspect_path, check_last_hardened_data, wallet_info)
212 
213         return migrate_info, wallet
214
  17. DrahtBot requested review from cedwies on Jul 30, 2025
  18. DrahtBot requested review from w0xlt on Jul 30, 2025
  19. achow101 force-pushed on Jul 30, 2025
  20. DrahtBot added the label CI failed on Jul 31, 2025
  21. DrahtBot commented at 1:14 am on July 31, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/47076642836 LLM reason (✨ experimental): The CI failure was caused by an unfixable linting error detected by ruff due to an unnecessary semicolon in the Python code.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  22. in test/functional/test_framework/test_framework.py:1091 in da84d666e3 outdated 
    1086+    def inspect_sqlite_db(self, path, fn, *args, **kwargs):
1087+        try:
1088+            import sqlite3 # type: ignore[import]
1089+            conn = sqlite3.connect(path)
1090+            with conn:
1091+                result = fn(conn, *args, **kwargs);
    rkrux commented at 12:07 pm on July 31, 2025:
    Linter fails here because of ;
    achow101 commented at 5:30 pm on July 31, 2025:
    oops fixed
  23. rkrux approved
  24. rkrux commented at 12:14 pm on July 31, 2025: contributor
    lgtm ACK da84d666e375b82960962ddf840f331473149999 modulo py-lint error.
  25. tests: Check that the last hardened cache upgrade occurs 
    When loading an older wallet without the last hardened cache, an
automatic upgrade should be performed. Check this in
wallet_backwards_compatibility.py

When migrating a wallet, the migrated wallet should always have the last
hardened cache, so verify in wallet_migration.py
    8a08eef645
  26. wallet: Always write last hardened cache flag in migrated wallets 88b0647f02
  27. achow101 force-pushed on Jul 31, 2025
  28. DrahtBot removed the label CI failed on Jul 31, 2025
  29. in test/functional/test_framework/test_framework.py:1088 in 88b0647f02 
    1081@@ -1082,3 +1082,15 @@ def convert_to_json_for_cli(self, text):
1082         if self.options.usecli:
1083             return json.dumps(text)
1084         return text
1085+
1086+    def inspect_sqlite_db(self, path, fn, *args, **kwargs):
1087+        try:
1088+            import sqlite3 # type: ignore[import]
    rkrux commented at 8:37 am on August 1, 2025:

    In 8a08eef645eeb3e1991a80480c5ee232bfceeb37 “tests: Check that the last hardened cache upgrade occurs”

    An argument can be made that sqlite3 is imported every time this function is called, for which an alternative could be to import once at the start of this file and use the corresponding boolean, like it was done in this PR previously. The import would still be limited to one file unlike it was done previously.

    But I don’t think it’s a big deal and the current implementation looks fine to me.

    achow101 commented at 11:48 pm on August 1, 2025:
    Multiple import of the same module is a no-op, it uses whatever is already imported.
  30. rkrux approved
  31. rkrux commented at 8:37 am on August 1, 2025: contributor
    lgtm ACK 88b0647f027a608acb61ec32329d19f8e5b0a9fd
  32. pablomartin4btc approved
  33. pablomartin4btc commented at 11:04 pm on August 1, 2025: member

    ACK 88b0647f027a608acb61ec32329d19f8e5b0a9fd

    It makes sense to set the flag as soon as the DB is created during migration and not leaving it to the reload of the, later migrated, wallet (in void CWallet::UpgradeDescriptorCache()) at the end of the entire process.


achow101 DrahtBot w0xlt rkrux cedwies pablomartin4btc


w0xlt cedwies

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-29 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me