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

💬 maflcko commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1629099295)
Another issue could be that a sister block of `invalidate_index` was activated instead, if one exists? If so, could make sense to mention that as well.

💬 maflcko commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1629101713)
style nit for new code, here and elsewhere: Could make sense to use the chainman.GetMutex() alias for chainman locking, instead of the global cs_main?

💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629131375)
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?

💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629143813)
`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.

💬 hebasto commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2151853425)
> Hmm, how best to ignore the tidy false-positive in a leveldb header?

FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.

💬 fanquake commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2151861139)
> FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.

Is that actually the behaviour we want for all subtrees? Sounds like it could just hide bugs, and no-one is running any checks in the upstream repos.

💬 maflcko commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1629164166)
Can you explain why this change in behavior is correct? Previously, if the peer was missing, it would return `true`. Now, it would return `false`.

👍 fanquake approved a pull request: "build: re-enable deprecated warning copy"
(https://github.com/bitcoin/bitcoin/pull/30236#pullrequestreview-2101490603)
ACK c3a5e8a0639ff2505adb4a4e7776db87d5ebafd3 - this is in `-Wextra` for Clang and GCC.

🚀 fanquake merged a pull request: "build: re-enable deprecated warning copy"
(https://github.com/bitcoin/bitcoin/pull/30236)

💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151869901)
An alternative would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)