BerkeleyRODatabase which is an independent parser for BDB files. This PR uses this in legacy wallet migration so that migration will continue to work once the legacy wallet and BDB are removed. LegacyDataSPKM is introduced to have the minimum data and functions necessary for a legacy wallet to be loaded for migration.
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 |
|---|---|
| ACK | furszy |
| Concept ACK | theStack |
| Stale ACK | cbergqvist |
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.
I think it would be easier to review if you made a separate PR for BerkeleyRODatabase and have it work as a regular, albeit read-only, wallet database backend.
I’m pleasantly surprised at how small the implementation is.
I don’t see how reimplementing BDB is better than just continuing to use BDB.
The reimplementation is quite compact because it is for only reading BDB files, only the features that we use, and none of the actual DB system stuff. BDB itself contains a ton of code, a lot of which we don’t use. Maintaining it as a dependency will eventually be more burdensome - we already need to patch it in order to compile in the depends system. This reimplementation does not carry those issues - it implements a format that isn’t going to change, and it only depends on things that are already being used elsewhere in the codebase.
🚧 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.
4464@@ -4438,6 +4465,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4465 // both before and after reloading. This ensures the set is complete even if one of the wallets
4466 // fails to reload.
4467 std::set<fs::path> wallet_dirs;
4468+ options.require_format = std::nullopt;
Why reset require_format here? Seems options is not going to be read by anything after this line and none of the database types is holding a reference to it.
Avoiding that would mean that you could introduce more immutability around line 4386:
0 const DatabaseOptions options{
1 .require_existing = true,
2 .require_format = DatabaseFormat::BERKELEY_RO,
3 };
reload_wallet calls which capture it.
"Reset require_format since reload_wallet will only be called on non-Berkeley DB sub-wallets from here on." or something more correct?
3646+ // Only create base LegacyDataSPKM if using BERKELEY_RO
3647+ if (m_database->Format() == "bdb_ro") {
3648+ spk_manager = std::unique_ptr<ScriptPubKeyMan>(new LegacyDataSPKM(*this));
3649+ } else {
3650+ spk_manager = std::unique_ptr<ScriptPubKeyMan>(new LegacyScriptPubKeyMan(*this, m_keypool_size));
3651+ }
0 // Only create base LegacyDataSPKM if using BERKELEY_RO
1 std::unique_ptr<ScriptPubKeyMan> spk_manager = m_database->Format() == "bdb_ro" ?
2 std::make_unique<LegacyDataSPKM>(*this) :
3 std::make_unique<LegacyScriptPubKeyMan>(*this, m_keypool_size);
4374+
4375+ if (wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
4376+ return util::Error{_("Error: This wallet is already a descriptor wallet")};
4377+ }
4378+ // Unload
4379+ wallet.reset();
275@@ -276,31 +276,106 @@ static const std::unordered_set<OutputType> LEGACY_OUTPUT_TYPES {
276
277 class DescriptorScriptPubKeyMan;
278
279-class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider
280+class LegacyDataSPKM : public ScriptPubKeyMan, public FillableSigningProvider
LegacyScriptPubKeyMan below could do with comments summarizing their responsibilities since breaking them apart and naming one LegacyDataSPKM makes things less clear.
386- bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey);
387- bool AddCryptedKeyInner(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret);
388+ bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey) override;
389
390 /**
391 * Private version of AddWatchOnly method which does not accept a
AddWatchOnly(const CScript&) is an inherited virtual method which appears untrue. Maybe add an extra commit ahead of the others that tidies up that comment?
353@@ -354,7 +354,7 @@ bool LoadKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::stri
354 strErr = "Error reading wallet database: CPrivKey corrupt";
355 return false;
356 }
357- if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadKey(key, vchPubKey))
358+ if (!pwallet->GetOrCreateLegacyDataSPKM()->LoadKey(key, vchPubKey))
359 {
360 strErr = "Error reading wallet database: LegacyScriptPubKeyMan::LoadKey failed";
"Error reading wallet database: LegacyDataSPKM::LoadKey failed", same for LoadCryptedKey and LoadCScript below.
960@@ -961,6 +961,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
961 //! Get the LegacyScriptPubKeyMan which is used for all types, internal, and external.
962 LegacyScriptPubKeyMan* GetLegacyScriptPubKeyMan() const;
963 LegacyScriptPubKeyMan* GetOrCreateLegacyScriptPubKeyMan();
964+ LegacyDataSPKM* GetLegacyDataSPKM() const;
965+ LegacyDataSPKM* GetOrCreateLegacyDataSPKM();
966
967 //! Make a LegacyScriptPubKeyMan and set it for all types, internal, and external.
"Make a *LegacyDataSPKM and set it...". Similar need for update in wallet.cpp around line 3019 before call to SetupLegacyScriptPubKeyMan().)
LegacyScriptPubKeyMan in normal operation.
LegacyScriptPubKeyMan is a LegacyDataSPKM, so the comment would be more correct, but it could be corrected later when removing LegacyScriptPubKeyMan - unless you plan to rename LegacyDataSPKM back into LegacyScriptPubKeyMan once the deletion of the latter is complete.
I think the meaning is well understood, both before and after LegacyScriptPubKeyMan is deleted. SPKM is an abbreviation for ScriptPubKeyMan anyways.
Changed it to Legacy(Data)SPKM.
2922@@ -2923,7 +2923,7 @@ bool CWallet::EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestinatio
2923 return true;
2924 }
2925
2926-std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string)
2927+util::Result<fs::path> GetWalletPath(const std::string& name)
static?
1885@@ -1887,7 +1886,6 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
1886 for (const CScript& spk : desc_spks) {
1887 size_t erased = spks.erase(spk);
1888 assert(erased == 1);
1889- assert(IsMine(spk) == ISMINE_SPENDABLE);
LegacyDataSPKM does not have an IsMine() function, so this call needs to be removed otherwise it will not compile.
No.
LegacyDataSPKMdoes not have anIsMine()function, so this call needs to be removed otherwise it will not compile.
I think the outcome would be worst. It would compile using the base ScriptPubKeyMan::IsMine function. Returning ISMINE_NO for all IsMine calls.
Still, I think we should state why this assertion removal is ok in the commit description too.
499@@ -500,7 +500,6 @@ class LegacyScriptPubKeyMan : public LegacyDataSPKM
500
501 /* Set the HD chain model (chain child index counters) and writes it to the database */
502 void AddHDChain(const CHDChain& chain);
503- const CHDChain& GetHDChain() const { return m_hd_chain; }
1952@@ -1812,7 +1953,7 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
1953 WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
1954
1955 // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
1956- auto desc_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size));
1957+ auto desc_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(m_storage, w_desc, /*keypool_size=*/0));
keypool_size=0? I’m guessing there are still cases where we generate some pubkeys despite having a zero pool size, otherwise you would have removed some of the code and the for-loop below?
nit: Might as well use make_unique on the lines you modify:
0 auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
keypool_size only matters for ranged descriptors. Non-HD keys do not make ranged descriptors, so the value here is ignored. The loop below is for an entirely different set of descriptors and is unrelated to this.
Haven’t fully grokked the code yet, so I understand if you’d rather not answer my questions, but if you want:
I don’t think this needs to have any additional explanation. keypool_size only matters for ranged descriptors. Non-HD keys do not make ranged descriptors, so the value here is ignored.
We’re in an HD chain for-loop, how is this not involving HD keys?
Surely legacy wallets could be HD wallets.
The loop below is for an entirely different set of descriptors and is unrelated to this.
I’m referring to the loop over desc_spks which is using the pubkeys from the DescriptorScriptPubKeyMan that’s just been created and comes from DescriptorScriptPubKeyMan::m_map_script_pub_keys which is filled out in DescriptorScriptPubKeyMan::TopUpWithDB() which is affected by m_keypool_size.
Would m_keypool_size already always be 0 before this change?
The line this comment is on is in the one that goes over the non-HD keys. However, I see that you’re confused about the similar line in the loop below that iterates the hd chains.
The keypool_size here is a runtime value that depends on the startup options. It’s is set when the DescriptorScriptPubKeyman is instantiated on loading, and the value is never directly persisted to disk. It ends up written as part of range_end when range_end is updated during TopUp(), but range_end can also be set directly.
The value that does matter is the range_end stored in WalletDescriptor. When we are going over the hd chains, we set range_end to match the chain counter stored in the CHDChain that is being migrated. This results in a descriptor that will produce exactly the same scriptPubKeys as the original hd chain, including it’s keypool. We are enforcing that the migrated descriptors produce exactly the same scriptPubKeys (no more, no less). Setting keypool_size could mean that TopUp() would generate more scriptPubKeys than we need/expect, which would cause migration to assert and crash.
762@@ -763,7 +763,7 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch,
763
764 // nTimeFirstKey is only reliable if all keys have metadata
765 if (pwallet->IsLegacy() && (key_res.m_records + ckey_res.m_records + watch_script_res.m_records) != (keymeta_res.m_records + watch_meta_res.m_records)) {
766- auto spk_man = pwallet->GetOrCreateLegacyScriptPubKeyMan();
767+ auto spk_man = pwallet->GetLegacyScriptPubKeyMan();
OrCreate here any longer?
IsLegacy() ensures the existence of a LegacyScriptPubKeyMan, so there’s no need to try to create one.
Code reviewed e11fa4524723aebc0d879ee083c2d6d46849d89a.
Overall concept: Not fully convinced about the benefit of breaking out LegacyDataSPKM, seems to add complexity. Is the goal to delete LegacyScriptPubKeyMan in the future when ripping out BDB and only keep LegacyDataSPKM?
Why perform the inlining of IsMine() in 31e918afc3920a2f8aa367ad323f590b4856aa26 before the function is removed? Seems risky in case of other PRs modifying it prior to removal etc.
LegacyScriptPubKeyMan::NewKeyPool() only touches data from LegacyDataSPKM, should it be moved to that class as public (or protected and then exposed in the subclass?). (If LegacyScriptPubKeyMan is slated for removal I can understand you want to signal that method will no longer be needed). (Have not checked if there are other methods along the same lines).
Assuming LegacyDataSPKM name is kept, comment in src/script/signingprovider.h could be updated to refer to LegacyDataSPKM::setWatchOnly instead.
Is the goal to delete
LegacyScriptPubKeyManin the future when ripping out BDB and only keepLegacyDataSPKM?
Yes.
We are trying to remove the legacy wallet in its entirety, leaving only behind the absolute minimum necessary to perform migration.
Why perform the inlining of
IsMine()in 31e918a before the function is removed? Seems risky in case of other PRs modifying it prior to removal etc.
IsMine() is unlikely to change. Besides the fact that any changes to the legacy wallet are likely to be closed instead of seriously considered, IsMine() has not had any material changes in the last several years. It could be included in #28710 but that PR is big enough as it is.
LegacyScriptPubKeyMan::NewKeyPool()only touches data fromLegacyDataSPKM, should it be moved to that class as public (or protected and then exposed in the subclass?)
No. The separation exists now to indicate which things can be removed at the end. LegacyDataSPKM is the bare minimum that must stick around for migration, everything else remains in LegacyScriptPubKeyMan and will be removed at the same time.
373+ /** Delete all the records ofthis LegacyScriptPubKeyMan from disk*/
374+ bool DeleteRecords();
375+};
376+
377+// Implements the full legacy wallet behavior
378+class LegacyScriptPubKeyMan : public LegacyDataSPKM
0// Implements the full legacy wallet behavior
1// Slated for deletion once Berkeley DB library dependency is removed.
2class LegacyScriptPubKeyMan : public LegacyDataSPKM
If the code is unable to communicate why it is like it is, it’s good to explain the otherwise surprising state of it. It would hopefully limit the amount of questions like my prior one from #26596#pullrequestreview-2089208116:
LegacyScriptPubKeyMan::NewKeyPool()only touches data fromLegacyDataSPKM, should it be moved to that class as public (or protected and then exposed in the subclass?).
But I guess you’re fairly confident it will be deleted in time for the next release anyway.
ACK a8f8a2d5d0e2328cbe2e569c7e12f653d18f456d
(Ran tests on 31632d18ea122def82820770f14d9aa009726114 but only comments changed since then).
Functional tests including --extended passed (a few automatically skipped).
make check passed.
4370+ if (!wallet_path) {
4371+ return util::Error{util::ErrorString(wallet_path)};
4372+ }
4373+ if (!IsBDBFile(BDBDataFile(*wallet_path))) {
4374+ return util::Error{_("Error: This wallet is already a descriptor wallet")};
4375+ }
in commit f732495191d346b877ffe8402ec68761713eaaf7: now that the legacy-wallet-check is done without loading the wallet, is the check for the descriptor flag below still needed? (can a BDB wallet ever have this flag set?)
0 // Before anything else, check if there is something to migrate.
1 if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
2 if (was_loaded) {
3 reload_wallet(local_wallet);
4 }
5 return util::Error{_("Error: This wallet is already a descriptor wallet")};
6 }
It is theoretically possible to have a descriptor wallet using BDB under 2 scenarios, however this is still an unsupported configuration:
1832@@ -1833,7 +1833,7 @@ std::optional<MigrationData> LegacyScriptPubKeyMan::MigrateToDescriptor()
1833 WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0);
1834
1835 // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys
1836- auto desc_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(m_storage, w_desc, m_keypool_size));
1837+ auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);
Not really an issue but this works because TopUp sets the descriptor range_end to 1 on the first run when the descriptor is not ranged.
As this would fail if we ever change that, what if we set the range properly in WalletDescriptor constructor?
E.g.
0WalletDescriptor w_desc(std::move(desc), creation_time, /*range_start=*/0, /*range_end=*/1, /*next_index=*/0)
Also, this change doesn’t seem to be related to c653f4fdbfe06 description? –> nvm about this last point, theStack just made me notice that this is because m_keypool_size is not accesible from LegacyDataSPKM.
Just a small comment; you don’t have to take it.
I believe this PR could get merged quite fast if you leave 89503710386f52d9162f67fcd707eceffa954faa for a follow-up and make IsMine compatible with LegacyDataSPKM in this PR.
I’m suggesting this because this PR starts testing the newly introduced BDB reader class and allows people to use it, which is great since we want it polished for the next release. The IsMine removal is also nice but I wouldn’t miss a deadline for it.
Still trying to fully grasp the IsMine inlining commit 89503710386f52d9162f67fcd707eceffa954faa, meanwhile a few comments:
BerkeleyRODatabase which was already merged in #26606LegacyDataSPKM is supposed to never write to the database (as BerkeleyRODatabase obviously doesn’t support that), but LegacyDataSPKM::CheckDecryptionKey contains a code-path for that (for a necessary rewrite):
https://github.com/achow101/bitcoin/blob/a8f8a2d5d0e2328cbe2e569c7e12f653d18f456d/src/wallet/scriptpubkeyman.cpp#L254-L255
Should CheckDecryptionKey stay in LegacyScriptPubKeyMan?
- To my understanding, the class
LegacyDataSPKMis supposed to never write to the database (asBerkeleyRODatabaseobviously doesn’t support that), butLegacyDataSPKM::CheckDecryptionKeycontains a code-path for that (for a necessary rewrite): https://github.com/achow101/bitcoin/blob/a8f8a2d5d0e2328cbe2e569c7e12f653d18f456d/src/wallet/scriptpubkeyman.cpp#L254-L255 ShouldCheckDecryptionKeystay inLegacyScriptPubKeyMan?
It isn’t really an issue because CheckDecryptionKey does not verify the outcome of the write call (which would be “successful” because the read-only bdb class return true for all write operations). But.. for the sake of correctness I see two possible paths to follow:
As this write is done exclusively on wallets with no checksum on crypted keys, we could move the upgrade code to the loading code in walletdb.cpp; removing fDecryptionThoroughlyChecked entirely and requiring such ancient wallets to provide the passphrase during load to perform the upgrade (adding a “passphrase” arg to the loadwallet RPC command).
Introduce a new WalletBatch::IsReadOnly() method to avoid calling the write inside CheckDecryptionKey.
The first approach will make the legacy wallet code simpler while the second one is a minimal change.
I believe this PR could get merged quite fast if you leave 8950371 for a follow-up and make
IsMinecompatible withLegacyDataSPKMin this PR.
Moved to #30328
The
IsMineremoval is also nice but I wouldn’t miss a deadline for it.
I think it needs to be in the next release as it is a critical part of migration. I would prefer to have it in a release that people can use before deleting the legacy wallet.
- The PR description is outdated, as it still mentions adding
BerkeleyRODatabasewhich was already merged in [wallet: Implement independent BDB parser #26606](https://github.com/bitcoin/bitcoin/pull/26606)
Updated
- in c653f4f: typo in commit subject (s/MigrateToLegacy/MigrateToDescriptor/)
Fixed
- To my understanding, the class
LegacyDataSPKMis supposed to never write to the database (asBerkeleyRODatabaseobviously doesn’t support that), butLegacyDataSPKM::CheckDecryptionKeycontains a code-path for that (for a necessary rewrite): https://github.com/achow101/bitcoin/blob/a8f8a2d5d0e2328cbe2e569c7e12f653d18f456d/src/wallet/scriptpubkeyman.cpp#L254-L255 ShouldCheckDecryptionKeystay inLegacyScriptPubKeyMan?
No, we should not be migrating a wallet if we cannot decrypt the keys. BerkeleyRODatabase always returns true for all write operations so it doesn’t matter. Also, I don’t think it is useful to invert that and have to have specific bypasses for BerkeleyRODatabase in all functions that attempts to write to it.
DatabaseFormat::BERKELEY format.
4460@@ -4438,6 +4461,8 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4461 // both before and after reloading. This ensures the set is complete even if one of the wallets
4462 // fails to reload.
4463 std::set<fs::path> wallet_dirs;
4464+ // Reset options.require_format as options are used by reload_wallet which may reload wallets of any format.
4465+ options.require_format = std::nullopt;
In f732495191d346b8:
The reset call needs to be moved above the reload_wallet function declaration. Otherwise, the wallet will be reloaded in read-only mode for any failure occurring in-between the reload_wallet declaration and this line, such as failures during backup creation or the decryption procedure.
I have written a test to trigger this misbehavior: furszy@c24b673 (feel free to pull it here). The test passes when the fix is cherry-picked on top: furszy@2d5d9a4.
reload_wallet directly, it doesn’t need to live outside of it.
When we reopen the wallet to do the migration, instead of opening using
BDB, open it using the BerkeleyRO implementation.
In order to load the necessary data for migrating a legacy wallet
without the full LegacyScriptPubKeyMan, move the data storage and
loading components to LegacyDataSPKM. LegacyScriptPubKeyMan now
subclasses that.
IsMine is necessary for migration. It should be inlined with migration
when the legacy wallet is removed.
In SetupLegacyScriptPubKeyMan, a base LegacyDataSPKM will be created if
the database has the format "bdb_ro" (i.e. the wallet was opened only
for migration purposes).
All of the loading functions are now called with a LegacyDataSPKM object
instead of LegacyScriptPubKeyMan.
The migration process reloads the wallet after all failures.
This commit tests the behavior by trying to obtain a new address
after a decryption failure during migration.
810+ LegacyDataSPKM::LoadScriptMetadata(script_id, meta);
811 UpdateTimeFirstKey(meta.nCreateTime);
812- m_script_metadata[script_id] = meta;
813+}
814+
815+bool LegacyDataSPKM::AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey)
nano nit:
0bool LegacyDataSPKM::AddKeyPubKeyInner(const CKey& key, const CPubKey& pubkey)
(In response to #26596 (review))
This made me cringe when first looking at the code too, but I guess retaining the inconsistency makes it easier to search for AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey) and find all 4 places it exists. Changing it in this PR would arguably add noise (on the other hand - if not now, then when?).
815+bool LegacyDataSPKM::AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey)
816+{
817+ // This function should only be called during loading of a legacy to be migrated.
818+ // As such, the wallet should not be encrypted if this is called.
819+ LOCK(cs_KeyStore);
820+ assert(!m_storage.HasEncryptionKeys());
HasEncryptionKeys should always return false but not for the reasons stated here.
I think calling IsLocked() would be more precise and make more sense then. It seems confusing to say that the wallet can still be encrypted but expect no encryption keys (we understand it because we know the loading procedure but others might not understand it).
Other than that, what if instead of creating this new LegacyDataSPKM::AddKeyPubKeyInner method you place this three lines inside the LegacyDataSPKM::LoadKey which is only called during load?
Now that the IsMine changes were moved to another PR, could reorder the commits just so 0cce574 is introduced before c3373a39a1acc94dbf644103fab2e477d49da671. This will let us keep the IsMine assertions inside MigrateToDescriptor for now.
Other than that, code review ACK 9700c21.
git range-diff master a8f8a2d 9700c217
Agree with @furszy on reordering the commits to make it possible to retain the 3 asserts in MigrateToDescriptor().