💬 dergoegge commented on pull request "refactor: Continue moving application data from CNode to Peer":
(https://github.com/bitcoin/bitcoin/pull/26621#issuecomment-1635588195)
> Needs rebase if still relevant

Thanks, rebased

💬 fanquake commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1635607860)
I'll follow up with just adding more suppressions until things work on aarch64.

✅ fanquake closed a pull request: "ci: build Valgrind (3.21) from source"
(https://github.com/bitcoin/bitcoin/pull/27992)

💬 fanquake commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1635610985)
> Right. Probably not worth to spend time here,

I'll follow up with this, because it certainly shouldn't be broken, and if it is, it's not specific to valgrind etc.

🚀 fanquake merged a pull request: "ci: Add missing -O2 to valgrind tasks"
(https://github.com/bitcoin/bitcoin/pull/28071)

👍 hebasto approved a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1530008568)
ACK 31eca93a9eb8e54f856d3f558aa3c831ef181d37, I have reviewed the code and it looks OK.

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

💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1635633574)
I haven't tried, but is this still an issue on aarch64 on current master?

💬 MarcoFalke commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1635635435)
I haven't tried, but an alternative to using `clang` would be to try to set `-O1` for `gcc`, but I am not sure on how to do this cleanly?

💬 hebasto commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#issuecomment-1635642358)
Concept ACK.

📝 MarcoFalke opened a pull request: "util: Remove DirIsWritable, GetUniquePath "
(https://github.com/bitcoin/bitcoin/pull/28075)
`GetUniquePath` is only used in tests and in `DirIsWritable`. The check by `DirIsWritable` is redundant with the check done in `LockDirectory`.

Fix the redundancy by removing everything, except `LockDirectory`.

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