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

88b0647 wallet: Always write last hardened cache flag in migrated wallets (Ava Chow)
8a08eef tests: Check that the last hardened cache upgrade occurs (Ava Chow)

Pull request description:

  #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.

ACKs for top commit:
  cedwies:
    re-ACK 88b0647
  rkrux:
    lgtm ACK 88b0647
  pablomartin4btc:
    ACK 88b0647

Tree-SHA512: 7d0850db0ae38eedd1e6a3bfaa548c6c612182291059fb1a47279a4c4984ee7914ecd02d8c7e427ef67bf9f5e67cbc57a7ae4412fad539e1bf3e05c512a60d69

Author: glozow

src/wallet/wallet.cpp

@@ -3884,7 +3884,7 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
     m_internal_spk_managers.clear();
 
     // Setup new descriptors (only if we are migrating any key material)
-    SetWalletFlagWithDB(local_wallet_batch, WALLET_FLAG_DESCRIPTORS);
+    SetWalletFlagWithDB(local_wallet_batch, WALLET_FLAG_DESCRIPTORS | WALLET_FLAG_LAST_HARDENED_XPUB_CACHED);
     if (has_spendable_material && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
         // Use the existing master key if we have it
         if (data.master_key.key.IsValid()) {

test/functional/test_framework/test_framework.py

@@ -1110,3 +1110,15 @@ def convert_to_json_for_cli(self, text):
         if self.options.usecli:
             return json.dumps(text)
         return text
+
+    def inspect_sqlite_db(self, path, fn, *args, **kwargs):
+        try:
+            import sqlite3 # type: ignore[import]
+            conn = sqlite3.connect(path)
+            with conn:
+                result = fn(conn, *args, **kwargs)
+            conn.close()
+            return result
+        except ImportError:
+            self.log.warning("sqlite3 module not available, skipping tests that inspect the database")
+

test/functional/wallet_backwards_compatibility.py

@@ -21,9 +21,11 @@
 from test_framework.blocktools import COINBASE_MATURITY
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.descriptors import descsum_create
+from test_framework.messages import ser_string
 
 from test_framework.util import (
     assert_equal,
+    assert_greater_than,
     assert_raises_rpc_error,
 )
 
@@ -149,18 +151,13 @@ def test_v22_inactivehdchain_path(self):
         assert_equal(bad_deriv_wallet_master.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path)
         bad_deriv_wallet_master.unloadwallet()
 
-        # If we have sqlite3, verify that there are no keymeta records
-        try:
-            import sqlite3
-            wallet_db = node_master.wallets_path / wallet_name / "wallet.dat"
-            conn = sqlite3.connect(wallet_db)
-            with conn:
-                # Retrieve all records that have the "keymeta" prefix. The remaining key data varies for each record.
-                keymeta_rec = conn.execute("SELECT value FROM main where key >= x'076b65796d657461' AND key < x'076b65796d657462'").fetchone()
-                assert_equal(keymeta_rec, None)
-            conn.close()
-        except ImportError:
-            self.log.warning("sqlite3 module not available, skipping lack of keymeta records check")
+        def check_keymeta(conn):
+            # Retrieve all records that have the "keymeta" prefix. The remaining key data varies for each record.
+            keymeta_rec = conn.execute(f"SELECT value FROM main where key >= x'{ser_string(b'keymeta').hex()}' AND key < x'{ser_string(b'keymetb').hex()}'").fetchone()
+            assert_equal(keymeta_rec, None)
+
+        wallet_db = node_master.wallets_path / wallet_name / "wallet.dat"
+        self.inspect_sqlite_db(wallet_db, check_keymeta)
 
     def test_ignore_legacy_during_startup(self, legacy_nodes, node_master):
         self.log.info("Test that legacy wallets are ignored during startup on v29+")
@@ -342,6 +339,13 @@ def run_test(self):
             # Remove the wallet from old node
             wallet_prev.unloadwallet()
 
+            # Open backup with sqlite and get flags
+            def get_flags(conn):
+                flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone()
+                return int.from_bytes(flags_rec[0], byteorder="little")
+
+            old_flags = self.inspect_sqlite_db(backup_path, get_flags)
+
             # Restore the wallet to master
             load_res = node_master.restorewallet(wallet_name, backup_path)
 
@@ -378,6 +382,21 @@ def run_test(self):
 
             wallet.unloadwallet()
 
+            # Open the wallet with sqlite and inspect the flags and records
+            def check_upgraded_records(conn, old_flags):
+                flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone()
+                new_flags = int.from_bytes(flags_rec[0], byteorder="little")
+                diff_flags = new_flags & ~old_flags
+
+                # Check for last hardened xpubs if the flag is newly set
+                if diff_flags & (1 << 2):
+                    self.log.debug("Checking descriptor cache was upgraded")
+                    # Fetch all records with the walletdescriptorlhcache prefix
+                    lh_cache_recs = conn.execute(f"SELECT value FROM main where key >= x'{ser_string(b'walletdescriptorlhcache').hex()}' AND key < x'{ser_string(b'walletdescriptorlhcachf').hex()}'").fetchall()
+                    assert_greater_than(len(lh_cache_recs), 0)
+
+            self.inspect_sqlite_db(down_backup_path, check_upgraded_records, old_flags)
+
             # Check that no automatic upgrade broke downgrading the wallet
             target_dir = node.wallets_path / down_wallet_name
             os.makedirs(target_dir, exist_ok=True)

test/functional/wallet_migration.py

@@ -18,11 +18,12 @@
 from test_framework.descriptors import descsum_create
 from test_framework.key import ECPubKey
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.messages import COIN, CTransaction, CTxOut
+from test_framework.messages import COIN, CTransaction, CTxOut, ser_string
 from test_framework.script import hash160
 from test_framework.script_util import key_to_p2pkh_script, key_to_p2pk_script, script_to_p2sh_script, script_to_p2wsh_script
 from test_framework.util import (
     assert_equal,
+    assert_greater_than,
     assert_raises_rpc_error,
     find_vout_for_address,
     sha256sum_file,
@@ -140,7 +141,8 @@ def migrate_and_get_rpc(self, wallet_name, **kwargs):
         # (in which case the wallet name would be suffixed by the 'watchonly' term)
         migrated_wallet_name = migrate_info['wallet_name']
         wallet = self.master_node.get_wallet_rpc(migrated_wallet_name)
-        assert_equal(wallet.getwalletinfo()["descriptors"], True)
+        wallet_info = wallet.getwalletinfo()
+        assert_equal(wallet_info["descriptors"], True)
         self.assert_is_sqlite(migrated_wallet_name)
         # Always verify the backup path exist after migration
         assert os.path.exists(migrate_info['backup_path'])
@@ -154,6 +156,25 @@ def migrate_and_get_rpc(self, wallet_name, **kwargs):
         assert_equal(str(expected_backup_path), migrate_info['backup_path'])
         assert {"name": backup_filename} not in self.master_node.listwalletdir()["wallets"]
 
+        # Open the wallet with sqlite and verify that the wallet has the last hardened cache flag
+        # set and the last hardened cache entries
+        def check_last_hardened(conn):
+            flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{ser_string(b'flags').hex()}'").fetchone()
+            flags = int.from_bytes(flags_rec[0], byteorder="little")
+
+            # All wallets should have the upgrade flag set
+            assert_equal(bool(flags & (1 << 2)), True)
+
+            # Fetch all records with the walletdescriptorlhcache prefix
+            # if the wallet has private keys and is not blank
+            if wallet_info["private_keys_enabled"] and not wallet_info["blank"]:
+                lh_cache_recs = conn.execute(f"SELECT value FROM main where key >= x'{ser_string(b'walletdescriptorlhcache').hex()}' AND key < x'{ser_string(b'walletdescriptorlhcachf').hex()}'").fetchall()
+                assert_greater_than(len(lh_cache_recs), 0)
+
+        inspect_path = os.path.join(self.options.tmpdir, os.path.basename(f"{migrated_wallet_name}_inspect.dat"))
+        wallet.backupwallet(inspect_path)
+        self.inspect_sqlite_db(inspect_path, check_last_hardened)
+
         return migrate_info, wallet
 
     def test_basic(self):