💬 ryanofsky commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1628436308)
In commit "p2p: For assumeutxo, download snapshot chain before background chain" (ac547ea43beb1d66d2c11e7e8abb76dd1bfc2883)
It seems like the `pindexLastCommonBlock->nHeight < snapshot_height` check is not strict enough because the snapshot block might not necessarily be an ancestor the peer's best known block.
In this case, the `pindexLastCommonBlock` assignments on line 1406 and 1411 will reset it back to the fork point between the two nodes each time `FindNextBlocksToDownload` is called
...
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1628436308)
In commit "p2p: For assumeutxo, download snapshot chain before background chain" (ac547ea43beb1d66d2c11e7e8abb76dd1bfc2883)
It seems like the `pindexLastCommonBlock->nHeight < snapshot_height` check is not strict enough because the snapshot block might not necessarily be an ancestor the peer's best known block.
In this case, the `pindexLastCommonBlock` assignments on line 1406 and 1411 will reset it back to the fork point between the two nodes each time `FindNextBlocksToDownload` is called
...
🤔 cbergqvist reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2089208116)
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::N
...
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2089208116)
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::N
...
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628300902)
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.
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1628300902)
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.
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1621331017)
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,
};
```
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1621331017)
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,
};
```
💬 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