💬 MarcoFalke commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1263696589)
I'd say no. There should be never a need to include `kernel/notifications_interface.h` when you've included `node/kernel_notifications.h`. And I don't see an outcome where `node/kernel_notifications.h` was ever changed to not include `kernel/notifications_interface.h`. Thus, iwyu pragma `export` should be used.

👍 MarcoFalke approved a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1530242589)
lgtm ACK 31eca93a9eb8e54f856d3f558aa3c831ef181d37 🕷

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 31eca93a9eb8e54f
...

💬 MarcoFalke commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635845208)
@hebasto Let us know if you'll pick this up again or if you want someone else to do it?

💬 hebasto commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635845857)
> @hebasto Let us know if you'll pick this up again or if you want someone else to do it?

Almost ready :)

📝 hebasto reopened a pull request: "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions"
(https://github.com/bitcoin/bitcoin/pull/26504)
The `UNREACHABLE` macro was suggested in https://github.com/bitcoin/bitcoin/pull/24812, but during reviewing it has been [dropped](https://github.com/bitcoin/bitcoin/pull/24812#discussion_r851597471):
> This is unused and on a second thought I wonder if there is any value in it. The current code can already use `assert(false)` just fine. Introducing another way to write `assert(false)` will just lead to bike-shedding without any benefit.

Apparently, this macro prevents compiler warnings -- `
...

💬 hebasto commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635858237)
re https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263587638
> Maybe drop this change and the one to `src/script/interpreter.cpp` for now and only use the second and third commit?

Done. Rebased.

👋 hebasto's pull request is ready for review: "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions"
(https://github.com/bitcoin/bitcoin/pull/26504)

💬 NateWarncoin commented on issue "bitcoind dumps core when deriveaddresses is called with index 2147483647 (2^31-1)":
(https://github.com/bitcoin/bitcoin/issues/26274#issuecomment-1635862689)
`doc/release-notes/release-notes-25.0.md`

👍 MarcoFalke approved a pull request: "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions"
(https://github.com/bitcoin/bitcoin/pull/26504#pullrequestreview-1530302258)
Nice. Review lgtm

💬 MarcoFalke commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263732536)
nit: Can remove the empty lines in this file, and other files?

💬 MarcoFalke commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635866926)
Looks like you forgot to include the doc change? https://github.com/bitcoin/bitcoin/commit/abacaef3f3b541ead566532eaae9f5db31ccb7ff#diff-5e319039d67254bed6ca82562ec5f560f102004aa4806e4ca77f5d0065c65fbb

Also, could update title, description and commit message?

💬 jonatack commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263749946)
A potential downside with this change is the `util/check.h` code is added to a fair number of compilation units (unless the header was already missing). Also, it looks like some cases haven't been updated. Maybe only use it for new code?

🤔 jonatack reviewed a pull request: "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions"
(https://github.com/bitcoin/bitcoin/pull/26504#pullrequestreview-1530329568)
If this is done, IIUC the developer notes should be updated.

💬 MarcoFalke commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1635896789)
lgtm ACK 1cd45d4e08c3dfd1d6423620c79169f1404ac12b 🌂

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 1cd45d4e08c3dfd1
...

📝 MarcoFalke opened a pull request: "util: Replace filesystem include with util/fs.h include "
(https://github.com/bitcoin/bitcoin/pull/28076)
Using `std::filesystem` is problematic:

* There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
* Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.

Fix all issues by removing use of it and adding a linter to avoid using it again in the future.

💬 hebasto commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263770200)
Done.

💬 hebasto commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635907218)
@MarcoFalke
> Also, could update title, description and commit message?

The title an the description have been updated. What update for commit messages would you like to see?

💬 jonatack commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263776033)
```suggestion
*Rationale*: The comment documents skipping the `default:` case and it complies with `clang-format` rules. The `UNREACHABLE` macro prevents firing of `-Wreturn-type`/`C4715` warnings on some compilers. In the RPC code, the `NONFATAL_UNREACHABLE` macro might be more appropriate instead.
```

💬 hebasto commented on pull request "Add `UNREACHABLE` macro":
(https://github.com/bitcoin/bitcoin/pull/26504#discussion_r1263779434)
Thanks! Updated.

👍 brunoerg approved a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-1530384275)
reACK e010fddf15a6d4ff84bfebdcb33c9c1d9cea7f0a