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 detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | furszy |
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.
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.