💬 stickies-v commented on pull request "rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1263578714)
Are you still going to make these changes?

💬 fanquake commented on pull request "util: Remove DirIsWritable, GetUniquePath":
(https://github.com/bitcoin/bitcoin/pull/28075#issuecomment-1635664090)
```bash
make[2]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-arm-linux-gnueabihf/src'
/usr/bin/ccache arm-linux-gnueabihf-g++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -fmacro-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-arm-linux-gnueabihf=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO -D_FILE_OFFSET_BITS=64 -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /tmp/cir
...

💬 MarcoFalke commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(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?

💬 MarcoFalke commented on pull request "Add `UNREACHABLE` macro and drop `-Wreturn-type`/`C4715` warnings suppressions":
(https://github.com/bitcoin/bitcoin/pull/26504#issuecomment-1635684211)
Concept ACK on `UNREACHABLE`. I find it frustrating to play compiler golf against g++ in the CI on every code push that uses a switch-case on an enum class.

First push is `-Werror=return-type`, the second push is `Assert(false);`, but g++ failing to understand it, then the third push is `assert(false);`

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1263674110)
> This requires to `#include "kernel/notifications_interface.h"`, no?

It is included from the header, and is not a new dependency since this file is subclassing that one, but yes according to IWYU's reasoning it should be included directly here as well. I'll add the include if I need to update this PR for another reason.

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