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

💬 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`?

💬 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()`.)

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

...

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

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

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

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

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

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

💬 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

💬 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

💬 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

💬 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

💬 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

💬 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

🤔 tdb3 reviewed a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2100747176)
Concept ACK.
Mind sharing how you see this fitting in with Erlay rework?

Will review in more depth soon.

💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151578901)
ACK c3a5e8a0639ff2505adb4a4e7776db87d5ebafd3

💬 maflcko commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2151583284)
```
libtool: link: /usr/bin/ccache clang++-15 -stdlib=libc++ -std=c++20 -g -O2 -fvisibility=hidden -fdebug-prefix-map=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu=. -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimpl
...

👋 Eunovo's pull request is ready for review: "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict"
(https://github.com/bitcoin/bitcoin/pull/29680)