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

💬 maflcko commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#discussion_r1628919530)
```suggestion
v -= auto(v);
```

nit: An alternative would be to create a copy (C++23), or with C++20:

```
v -= arith_uint256{v};
```

But either seems fine.

💬 maflcko commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1628935823)
`reverse_iterate` is a helper in this repo, not in `std::`. You'll have to include it to use it.

Also, it looks like taking a `std::span` from a `Span` is not possible, at least on clang-15, according to the CI.

I think if you instead rebase on fabc9b02331ad6d5cbae3d351721e7e5d9585df0, it should fix itself, as that removes `Span`.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2151786093)
Rebased

> My impression is that this PR is good enough as is, unless there is a specific issue with invalidate/reconsider block that I am not aware of.

> If the rollback functionality gets implemented after this PR is merged, it seems straightforward to change this code

Yes, either way, this PR should be an improvement. I am still planning to do the rollback in a cleaner way but if this PR is merged before that is ready, there should be no overhead resulting from that. It would become a
...

💬 maflcko commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2151786786)
Could add a patch to the leveldb subtree?