💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628376667)
*`"Error reading wallet database: LegacyDataSPKM::LoadKey failed"`, same for `LoadCryptedKey` and `LoadCScript` below.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628376667)
*`"Error reading wallet database: LegacyDataSPKM::LoadKey failed"`, same for `LoadCryptedKey` and `LoadCScript` below.
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1621407661)
```suggestion
// 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);
```
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1621407661)
```suggestion
// 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);
```
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628371045)
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?
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628371045)
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?
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628438776)
In commit 46520aacc43dd42a7968587ab9ece488bbe9503d: Should this line have been removed in the following commit (31e918afc3920a2f8aa367ad323f590b4856aa26)?
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628438776)
In commit 46520aacc43dd42a7968587ab9ece488bbe9503d: Should this line have been removed in the following commit (31e918afc3920a2f8aa367ad323f590b4856aa26)?
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628492436)
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?
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628492436)
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?
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628408816)
Could be marked `static`?
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628408816)
Could be marked `static`?
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628391489)
(`"Make a *LegacyDataSPKM and set it..."`. Similar need for update in wallet.cpp around line 3019 before call to `SetupLegacyScriptPubKeyMan()`.)
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628391489)
(`"Make a *LegacyDataSPKM and set it..."`. Similar need for update in wallet.cpp around line 3019 before call to `SetupLegacyScriptPubKeyMan()`.)
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#issuecomment-2151172675)
> Is the goal to delete `LegacyScriptPubKeyMan` in the future when ripping out BDB and only keep `LegacyDataSPKM`?
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](https://github.com/bitcoin/bitcoin/commit/31e918afc3920a2f8aa367ad323f590b4856aa26) before the function is removed? Seems risky in case of other PRs modifying it prior to removal etc.
...
(https://github.com/bitcoin/bitcoin/pull/26596#issuecomment-2151172675)
> Is the goal to delete `LegacyScriptPubKeyMan` in the future when ripping out BDB and only keep `LegacyDataSPKM`?
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](https://github.com/bitcoin/bitcoin/commit/31e918afc3920a2f8aa367ad323f590b4856aa26) before the function is removed? Seems risky in case of other PRs modifying it prior to removal etc.
...
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628597865)
It's used by the `reload_wallet` calls which capture it.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628597865)
It's used by the `reload_wallet` calls which capture it.
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628598230)
I don't think it's necessary to add changes that correct this documentation when it's all going to be deleted soon anyways.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628598230)
I don't think it's necessary to add changes that correct this documentation when it's all going to be deleted soon anyways.
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628598636)
The function still creates a `LegacyScriptPubKeyMan` in normal operation.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628598636)
The function still creates a `LegacyScriptPubKeyMan` in normal operation.
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628600303)
No. `LegacyDataSPKM` does not have an `IsMine()` function, so this call needs to be removed otherwise it will not compile.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628600303)
No. `LegacyDataSPKM` does not have an `IsMine()` function, so this call needs to be removed otherwise it will not compile.
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628603057)
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.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628603057)
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.
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628610626)
It's redundant since `IsLegacy()` ensures the existence of a `LegacyScriptPubKeyMan`, so there's no need to try to create one.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628610626)
It's redundant since `IsLegacy()` ensures the existence of a `LegacyScriptPubKeyMan`, so there's no need to try to create one.
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612108)
Done as suggested
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612108)
Done as suggested
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612254)
Add a comment to each
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612254)
Add a comment to each
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612380)
Done as suggested
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612380)
Done as suggested
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612502)
Done
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612502)
Done
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612617)
Done
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612617)
Done
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612697)
Done as suggested
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628612697)
Done as suggested