A bug in 0.21.x and 22.x resulted in some wallets storing invalid derivation paths that are the concatenation of two derivation paths. This corruption follows a rigid pattern and can be easily detected and repaired.
The bug that resulted in such wallets was fixed in #23304 but wallets that already ran into it would still have bad data stored. This PR should fix such wallets.
Fixes #29109
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29124.
See 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.
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.
694 return DBErrors::LOAD_OK;
695 });
696 result = std::max(result, keymeta_res.m_result);
697
698+ // Update any changed metadata
699+ if (updated_metas.size()) {
!updated_metas.empty()
526@@ -527,7 +527,7 @@ static LoadResult LoadRecords(CWallet* pwallet, DatabaseBatch& batch, const std:
527 return LoadRecords(pwallet, batch, key, prefix, load_func);
528 }
529
530-static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, int last_client) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
531+static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, WalletBatch& w_batch, int last_client) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
batch and w_batch when w_batch contains the batch object.
m_batch public so that we can just pass the WalletBatch but still use the underlying batch object. However I’m not sure if that’s something we want to do in the long term, but I think it should be okay.
I’ve made
m_batchpublic so that we can just pass theWalletBatchbut still use the underlying batch object. However I’m not sure if that’s something we want to do in the long term, but I think it should be okay.
Yeah.. I’m not happy reading all the the batch.m_batch usages neither but np.
699+ if (updated_metas.size()) {
700+ LegacyScriptPubKeyMan* spkm = pwallet->GetLegacyScriptPubKeyMan();
701+ Assert(spkm);
702+ LOCK(spkm->cs_KeyStore);
703+ for (const auto& pubkey : updated_metas) {
704+ w_batch.WriteKeyMetadata(spkm->mapKeyMetadata.at(pubkey.GetID()), pubkey, true);
WriteKeyMetadata return value.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
ACK 1c3462e908994f0d63c1f39b5908f48d083dd10e
Could minimize the first commit diff https://github.com/furszy/bitcoin-core/commit/145737e46e71859608334ab738817f7b323df367.
150+ with open(dump_path, encoding="utf8") as f:
151+ for line in f:
152+ if "hdkeypath=m/0'/0'/9'" in line:
153+ addr = line.split(" ")[4].split("=")[1]
154+ elif " hdseed=1 " in line:
155+ seed = line.split(" ")[0]
647+ // See https://github.com/bitcoin/bitcoin/issues/29109
648+ // Markers for bad derivation:
649+ // - 6 indexes
650+ // - path[5] == path[2] + 1
651+ // - path[0:2] == path[3:5]
652+ // - path[3:6] matches hdKeypath
DeriveNewChildKey function answers it: https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/scriptpubkeyman.cpp#L1129
147+ wallet.dumpwallet(dump_path)
148+ addr = None
149+ seed = None
150+ with open(dump_path, encoding="utf8") as f:
151+ for line in f:
152+ if "hdkeypath=m/0'/0'/9'" in line:
I spent some time trying to understand why this one is picked here only to realise later that 9 is the last index at this chain because of keypool size set to 10 at the top of this test here: https://github.com/bitcoin/bitcoin/pull/29124/commits/e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc#diff-b5055a65809b46e2775f6e28aa6eb8171e3fcc7a535cce5687a548b9ff233669R44
This 9 and 10 can be tied to each other with a constant for better readability.
164+ self.sync_all(nodes=[self.nodes[0], node_master, node_v22])
165+ node_v22.loadwallet(wallet_name)
166+
167+ # Dump again to find bad hd keypath
168+ bad_deriv_path = "m/0'/0'/9'/0'/0'/10'"
169+ good_deriv_path = "m/0'/0'/10'"
148+ addr = None
149+ seed = None
150+ with open(dump_path, encoding="utf8") as f:
151+ for line in f:
152+ if "hdkeypath=m/0'/0'/9'" in line:
153+ addr = line.split(" ")[4].split("=")[1]
180+ # Verify that this addr is actually at m/0'/0'/10' by making a new wallet with the same seed
181+ node_v22.createwallet(wallet_name="path_verify", descriptors=False, blank=True)
182+ verify_wallet = node_v22.get_wallet_rpc("path_verify")
183+ verify_wallet.sethdseed(True, seed)
184+ # Bad addr is after keypool, so need to generate it by refilling
185+ verify_wallet.keypoolrefill(12)
195+
196+ # Check persistence
197+ wallet_master.unloadwallet()
198+ with node_master.assert_debug_log(expected_msgs=[], unexpected_msgs=["Repairing derivation path for"]):
199+ node_master.loadwallet(wallet_name)
200+ assert_equal(wallet_master.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path)
189+ backup_path = node_v22.wallets_path / f"{wallet_name}.bak"
190+ wallet.backupwallet(backup_path)
191+ with node_master.assert_debug_log(expected_msgs=["Repairing derivation path for"]):
192+ node_master.restorewallet(wallet_name, backup_path)
193+ wallet_master = node_master.get_wallet_rpc(wallet_name)
194+ assert_equal(wallet_master.getaddressinfo(bad_path_addr)["hdkeypath"], good_deriv_path)
0'pubkey': '029edc63efe154f7f0da891f0def969b65cc4706893f8277580093eccbd09cb11d', 'ischange': True, 'timestamp': 1738665397, 'hdkeypath': "m/0'/0'/10'",
I found it a little weird that an external keychain address is categorised as change. I am assuming that internal keychain addresses are for changes.
ischange does not actually look at the derivation path. Instead it looks at some heuristics which result in derived-but-not-yet-requested external addresses being identified as change.
301@@ -302,8 +302,8 @@ class WalletBatch
302 //! Registers db txn callback functions
303 void RegisterTxnListener(const DbTxnListener& l);
304
305-private:
306 std::unique_ptr<DatabaseBatch> m_batch;
307+private:
tACK e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc
Thanks for this PR, I enjoyed reviewing it. I ran the tests and debugged the compatibility test.
Can / Will this also be back-ported to the affected version or is the affected version too far back to be back-ported into?
A bug in 0.21.x and 22.x resulted in some wallets having invalid
derivation paths that are the concatenation of two derivation paths.
This corruption follows a rigid pattern and can be easily detected and
repaired.
40@@ -41,7 +41,7 @@ def set_test_params(self):
41 ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v25.0
42 ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v24.0.1
43 ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v23.0
44- ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1"], # v22.0
45+ ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-whitelist=noban@127.0.0.1", "-keypool=10"], # v22.0
179+ # Verify that this addr is actually at m/0'/0'/10' by making a new wallet with the same seed
180+ node_v22.createwallet(wallet_name="path_verify", descriptors=False, blank=True)
181+ verify_wallet = node_v22.get_wallet_rpc("path_verify")
182+ verify_wallet.sethdseed(True, seed)
183+ # Bad addr is after keypool, so need to generate it by refilling
184+ verify_wallet.keypoolrefill(LAST_KEYPOOL_INDEX + 2)
reACK 78cb0df4d5b9cc30dbe60a6cf24ab6cac5d3f421
I reviewed the range diff.
0git range-diff e92dc1a3d36be02a8b16eed26159a43cfbe3dbcc...78cb0df4d5b9cc30dbe60a6cf24ab6cac5d3f421
Newer changes are related to using the constant LAST_KEYPOOL_INDEX as suggested and use migratewallet instead of restorewallet due to which the format of good_deriv_path is changed from ' to ‘h’. Using migratewallet is fine because it also ends up calling LoadLegacyWalletRecords internally where the bug fix lies, and additionally it also ensures the fix works for the newer wallets.
