wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

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:

    const DatabaseOptions options{
        .require_existing = true,
        .require_format = DatabaseFormat::BERKELEY_RO,
    };

It's used by the reload_wallet calls which capture it.

Thanks, sorry for missing that. Worth an explaining comment like "Reset require_format since reload_wallet will only be called on non-Berkeley DB sub-wallets from here on." or something more correct?

Added a comment.

    // Only create base LegacyDataSPKM if using BERKELEY_RO
    std::unique_ptr<ScriptPubKeyMan> spk_manager = m_database->Format() == "bdb_ro" ?
        std::make_unique<LegacyDataSPKM>(*this) :
        std::make_unique<LegacyScriptPubKeyMan>(*this, m_keypool_size);

Done as suggested

Loading the entire wallet just to obtain the descriptors flag shouldn't be needed. Could just check the file type instead. We want a bdb file here. So, something like https://github.com/furszy/bitcoin-core/commit/7a03a039ae99e69504fe9df95b04f5e326c45a70 would work well. Yet, the code isn't the best but it includes a test that verifies the behavior.

Good point, I think the original intention was to be able to handle the legacy-sqlite case, but that's moot since we require BDB_RO later. I've changed this to check for just look for BDB files, and also added your test.

This class and possibly LegacyScriptPubKeyMan below could do with comments summarizing their responsibilities since breaking them apart and naming one LegacyDataSPKM makes things less clear.

Add a comment to each

This comment goes on to say 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?

I don't think it's necessary to add changes that correct this documentation when it's all going to be deleted soon anyways.

*"Error reading wallet database: LegacyDataSPKM::LoadKey failed", same for LoadCryptedKey and LoadCScript below.

Done as suggested

("Make a *LegacyDataSPKM and set it...". Similar need for update in wallet.cpp around line 3019 before call to SetupLegacyScriptPubKeyMan().)

The function still creates a 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.

Could be marked static?

Done

In commit 46520aacc43dd42a7968587ab9ece488bbe9503d: Should this line have been removed in the following commit (31e918afc3920a2f8aa367ad323f590b4856aa26)?

No. LegacyDataSPKM does not have an IsMine() function, so this call needs to be removed otherwise it will not compile.

No. LegacyDataSPKM does not have an IsMine() 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.

This was removed in e11fa4524723aebc0d879ee083c2d6d46849d89a but probably shouldn't have been included in 84a5fb96382960300ebbd2cf2fe8756142bea572 (verified through compiling on that commit + with it removed).

Done

Worth adding a comment about the consequences of setting 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:

        auto desc_spk_man = std::make_unique<DescriptorScriptPubKeyMan>(m_storage, w_desc, /*keypool_size=*/0);

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. The loop below is for an entirely different set of descriptors and is unrelated to this.

Done as suggested

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.

Thanks for taking the time to clarify this matter!

Why not OrCreate here any longer?

It's redundant since IsLegacy() ensures the existence of a LegacyScriptPubKeyMan, so there's no need to try to create one.

// Implements the full legacy wallet behavior
// Slated for deletion once Berkeley DB library dependency is removed.
class LegacyScriptPubKeyMan : public LegacyDataSPKM

I don't think it's necessary to have a comment saying such.

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 from LegacyDataSPKM, 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.

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?)

    // Before anything else, check if there is something to migrate.
    if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
        if (was_loaded) {
            reload_wallet(local_wallet);
        }
        return util::Error{_("Error: This wallet is already a descriptor wallet")};
    }

https://github.com/achow101/bitcoin/blob/a8f8a2d5d0e2328cbe2e569c7e12f653d18f456d/src/wallet/wallet.cpp#L4425-L4431

It is theoretically possible to have a descriptor wallet using BDB under 2 scenarios, however this is still an unsupported configuration:

1. The descriptor wallet was created on an unreleased version after #16528 but before #19077 (there was a period of about 6 months where this could have happened)
2. The user manipulated the database using external tooling where they created a BDB file and manually inserted descriptor wallet records

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.

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

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.

Fixed and pulled the test. I decided to just move this into reload_wallet directly, it doesn't need to live outside of it.

nano nit:

bool 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?).

Done

(Now grepping "AddKeyPubKeyInner(const CKey& key, const CPubKey& pubkey)" gives 1 result. grepping "AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey)" gives 3 results.

It would be nice to align style within the same class between .CPP/.H).

Not going to change existing code, especially code that will be deleted, to match style

(Agreed, but then I would skip doing the suggested change in the .CPP file).

I'm not sure this is asserting what you expect. In the loading process, we load the encryption keys after the key records. So the encryption keys map should always be empty and HasEncryptionKeys should always return false but not for the reasons stated here.

Yes, that's what the comment is saying. It is because this function is only called during loading, and because legacy wallet records are loaded first, that this assertion makes sense. After all, to be migrated legacy wallets can still be encrypted.

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?

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?

We can't do that because LoadKey needs to call LegacyScriptPubKeyMan::AddKeyPubKeyInner for legacy wallets that are being loaded normally. Since migration cannot do all of the things in LegacyScriptPubKeyMan::AddKeyPubKeyInner, we need to have LegacyDataSPKM::AddKeyPubKeyInner.

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

Actually, looking at this more closely, your original comment was correct. This function is only reached during legacy migration wallet migration loading for unencrypted keys only. Since the encryption keys are loaded later, this check doesn't actually do anything useful.

Removed the assert and the comment.

nit: was about to suggest to deduplicate shared code between this method and ::GetLegacyScriptPubKeyMan above, but since the latter is removed soon anyway (in PR #28710, commit https://github.com/bitcoin/bitcoin/pull/28710/commits/5652a9c59f9ec793886eff00126c382003e4ce02#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8), it's probably not worth it.

If you re-touch it again. Could:

LegacyDataSPKM* CWallet::GetLegacyDataSPKM() const
{
    if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { return nullptr; }
    return dynamic_cast<LegacyDataSPKM*>(GetScriptPubKeyMan(OutputType::LEGACY, /*internal=*/true));
}

In 7461d0c006c92:

At this point, just a non-blocking nit but you are locking cs_KeyStore twice. One here, and another one inside LegacyDataSPKM::LoadKeyMetadata.

Same for LoadScriptMetadata.

In https://github.com/bitcoin/bitcoin/commit/7461d0c006c92ede2f2595b79a5509eaf3509fb7:

FillableSigningProvider::AddKeyPubKey already locks cs_Keystore internally.