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

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

Can you clarify how this PR is related to C++23? I don't see CMake as a blocker, if that's something we want to do.

🚀 fanquake merged a pull request: "build: no-longer allow GCC-10 in C++20 check"
(https://github.com/bitcoin/bitcoin/pull/30228)

💬 hebasto commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2151924530)
CMake's https://github.com/hebasto/bitcoin/pull/191 is built on top of this PR change.

🤔 hebasto reviewed a pull request: "build: re-enable deprecated warning copy"
(https://github.com/bitcoin/bitcoin/pull/30236#pullrequestreview-2101568286)
Post-merge ACK c3a5e8a0639ff2505adb4a4e7776db87d5ebafd3.

💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151947313)
> > An alternative/addition would be to have one CI task compile with C++23, but that can be done after the cmake transition (remind me)
>
> Can you clarify how this PR is related to C++23? I don't see CMake as a blocker, if that's something we want to do.

Ah sorry, looks like they didn't change anything about the deprecation in C++23 and it is kept as-is for now.

However, I found that this patch seems incomplete. For example, no warning is printed to catch https://eel.is/c++draft/depr.
...

💬 maflcko commented on pull request "build: re-enable deprecated warning copy":
(https://github.com/bitcoin/bitcoin/pull/30236#issuecomment-2151951796)
For reference https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2139r2.html#3.8 said:

> Strong recommendation: take decisive action early in the C++23 development cycle, so early adopters can shake out the remaining cost of updating old code. Note that this would be both an API and ABI breaking change, and it would be good to set that precedent early if we wish to allow such breakage in C++23.

But that didn't happen.