wallet: Set descriptor cache upgraded flag for migrated wallets #33031
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33031. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
7cd2ddf to
4e5faab
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
I deleted my previous updated suggestion regarding importing |
There was a problem hiding this comment.
- Unit tests (test_bitcoin) passed in 91.67 s (macOS Debug build)
questions/observations:
- why set WALLET_FLAG_DESCRIPTORS and WALLET_FLAG_LAST_HARDENED_XPUB_CACHED in a single db write?
- If the xpub cache flag was already true, could re-setting it trigger any unexpected behavior?
- nit: please update the comment above the SetWalletFlagWithDB call to mention that it now also caches the last-hardened xpub.
ACK 4e5faab
4e5faab to
aa08126
Compare
There was a problem hiding this comment.
tACK aa08126
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.
Diff
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 639421d3ae..15b5f05966 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -952,6 +952,17 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
except ImportError:
raise SkipTest("sqlite3 module not available.")
+ def inspect_database(self, path, queries_executor, *args, **kwargs):
+ try:
+ import sqlite3 # type: ignore[import]
+ conn = sqlite3.connect(path)
+ with conn:
+ result = queries_executor(conn, *args, **kwargs)
+ # Connection needs to be manually closed: https://docs.python.org/3/library/sqlite3.html#sqlite3-connection-context-manager
+ conn.close()
+ return result
+ except ImportError:
+ self.log.warning("sqlite3 module not available, skipping tests that inspect the database")
+
def skip_if_no_python_bcc(self):
"""Attempt to import the bcc package and skip the tests if the import fails."""
try:
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index e5a5938f07..1bc713a12b 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -619,3 +619,6 @@ def wallet_importprivkey(wallet_rpc, privkey, timestamp, *, label=""):
}]
import_res = wallet_rpc.importdescriptors(req)
assert_equal(import_res[0]["success"], True)
+
+def get_db_key_hex(db_key_string):
+ return (f"{len(db_key_string):02x}{db_key_string.encode().hex()}")
diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py
index 0ce836d850..c07fd2b5ec 100755
--- a/test/functional/wallet_backwards_compatibility.py
+++ b/test/functional/wallet_backwards_compatibility.py
@@ -26,15 +26,9 @@ from test_framework.util import (
assert_equal,
assert_greater_than,
assert_raises_rpc_error,
+ get_db_key_hex,
)
-SQLITE3_IMPORTED = False
-try:
- import sqlite3 # type: ignore[import]
- SQLITE3_IMPORTED = True
-except ImportError:
- pass
-
LAST_KEYPOOL_INDEX = 9 # Index of the last derived address with the keypool size of 10
class BackwardsCompatibilityTest(BitcoinTestFramework):
@@ -58,9 +52,6 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
self.skip_if_no_wallet()
self.skip_if_no_previous_releases()
- if not SQLITE3_IMPORTED:
- self.log.warning("sqlite3 module not available, skipping tests that inspect the database")
-
def setup_nodes(self):
self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[
None,
@@ -161,14 +152,12 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
bad_deriv_wallet_master.unloadwallet()
# If we have sqlite3, verify that there are no keymeta records
- if SQLITE3_IMPORTED:
- 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()
+ def assert_no_keymeta_records(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'{get_db_key_hex('keymeta')}' AND key < x'{get_db_key_hex('keymetb')}'").fetchone()
+ assert_equal(keymeta_rec, None)
+
+ self.inspect_database(node_master.wallets_path / wallet_name / "wallet.dat", assert_no_keymeta_records)
def test_ignore_legacy_during_startup(self, legacy_nodes, node_master):
self.log.info("Test that legacy wallets are ignored during startup on v29+")
@@ -351,12 +340,11 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
wallet_prev.unloadwallet()
# Open backup with sqlite and check flags
- if SQLITE3_IMPORTED:
- conn = sqlite3.connect(backup_path)
- with conn:
- flags_rec = conn.execute("SELECT value FROM main WHERE key = x'05666c616773'").fetchone()
- old_flags = int.from_bytes(flags_rec[0], byteorder="little")
- conn.close()
+ def retrieve_flags_of_backup(conn):
+ flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{get_db_key_hex('flags')}'").fetchone()
+ return int.from_bytes(flags_rec[0], byteorder="little")
+
+ old_flags = self.inspect_database(backup_path, retrieve_flags_of_backup)
# Restore the wallet to master
load_res = node_master.restorewallet(wallet_name, backup_path)
@@ -395,21 +383,17 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
wallet.unloadwallet()
# Open the wallet with sqlite and inspect the flags and records
- if SQLITE3_IMPORTED:
- conn = sqlite3.connect(down_backup_path)
- with conn:
- flags_rec = conn.execute("SELECT value FROM main WHERE key = x'05666c616773'").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("SELECT value FROM main where key >= x'1777616c6c657464657363726970746f726c686361636865' AND key < x'1777616c6c657464657363726970746f726c686361636866'").fetchall()
- assert_greater_than(len(lh_cache_recs), 0)
-
- conn.close()
+ def assert_flags_records(conn, old_flags):
+ flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{get_db_key_hex('flags')}'").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")
+ 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()
+ assert_greater_than(len(lh_cache_recs), 0)
+
+ self.inspect_database(down_backup_path, assert_flags_records, old_flags)
# Check that no automatic upgrade broke downgrading the wallet
target_dir = node.wallets_path / down_wallet_name
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 9ee27a5765..9b9a3b6b48 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -27,19 +27,13 @@ from test_framework.util import (
assert_raises_rpc_error,
find_vout_for_address,
sha256sum_file,
+ get_db_key_hex,
)
from test_framework.wallet_util import (
get_generate_key,
generate_keypair,
)
-SQLITE3_IMPORTED = False
-try:
- import sqlite3 # type: ignore[import]
- SQLITE3_IMPORTED = True
-except ImportError:
- pass
-
BTREE_MAGIC = 0x053162
@@ -54,9 +48,6 @@ class WalletMigrationTest(BitcoinTestFramework):
self.skip_if_no_wallet()
self.skip_if_no_previous_releases()
- if not SQLITE3_IMPORTED:
- self.log.warning("sqlite3 module not available, skipping tests that inspect the database")
-
def setup_nodes(self):
self.add_nodes(
self.num_nodes,
@@ -165,26 +156,22 @@ class WalletMigrationTest(BitcoinTestFramework):
# Open the wallet with sqlite and verify that the wallet has the last hardened cache flag
# set and the last hardened cache entries
- if SQLITE3_IMPORTED:
- inspect_path = os.path.join(self.options.tmpdir, os.path.basename(f"{migrated_wallet_name}_inspect.dat"))
- wallet.backupwallet(inspect_path)
-
- conn = sqlite3.connect(inspect_path)
-
- with conn:
- flags_rec = conn.execute("SELECT value FROM main WHERE key = x'05666c616773'").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("SELECT value FROM main where key >= x'1777616c6c657464657363726970746f726c686361636865' AND key < x'1777616c6c657464657363726970746f726c686361636866'").fetchall()
- assert_greater_than(len(lh_cache_recs), 0)
-
- conn.close()
+ def check_last_hardened_data(conn, wallet_info):
+ flags_rec = conn.execute(f"SELECT value FROM main WHERE key = x'{get_db_key_hex('flags')}'").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'{get_db_key_hex('walletdescriptorlhcache')}' AND key < x'{get_db_key_hex('walletdescriptorlhcachf')}'").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_database(inspect_path, check_last_hardened_data, wallet_info)
return migrate_info, wallet
aa08126 to
da84d66
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
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
da84d66 to
88b0647
Compare
|
|
||
| def inspect_sqlite_db(self, path, fn, *args, **kwargs): | ||
| try: | ||
| import sqlite3 # type: ignore[import] |
There was a problem hiding this comment.
In 8a08eef "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.
There was a problem hiding this comment.
Multiple import of the same module is a no-op, it uses whatever is already imported.
There was a problem hiding this comment.
ACK 88b0647
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.
#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.