👍 maflcko approved a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2299985303)
review ACK 129f73cc6e2ed4147f57c3d1dfe2841b8a46e9fe 💼

<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: review ACK 129f73cc6e2e
...

💬 maflcko commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1756650092)
nit: It would be nice to keep the `PeerRef` construction always at the same place as the previous `CNodeState` construction. This makes this review easier and also follow-up changes that move more state into `Peer`. Otherwise, this will have to be moved up again in the future anyway.

💬 maflcko commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1756721499)
nit: Now that C++17 is available, you can just use the equivalent `try_emplace` (5) from https://en.cppreference.com/w/cpp/container/map/try_emplace


```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 23f5265ccd..2b9147b590 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1720,7 +1720,7 @@ void PeerManagerImpl::InitializeNode(const CNode& node, ServiceFlags our_service
NodeId nodeid = node.GetId();
{
LOCK(cs_main); // For
...

💬 furszy commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1756728368)
ah, missed the extra parenthesis. Good.
Might be good to decouple it as proposed in https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752283015.

I wish there would be methods for "hasBlockData()" and "hasUndoData()" so we are not forced to use the internal `BlockStatus` enum everywhere.

🤔 furszy reviewed a pull request: "rpc, rest: Improve block rpc error handling, check header before attempting to read block data."
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2300108980)
Code review ACK 92236f43ff92c931f3a099e03d7851b890bff263

💬 hebasto commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2346120551)
> Ok, and Android support was blocked on cmake or qt6, or was it both? (Sorry for using this thread, but I couldn't find the tracking issue).

Qt 6 is a requirement for Android support.

And CMake is a requirement for Qt 6.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756741748)
Thanks, fixed!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756741980)
Thanks, fixed typo

💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756754331)
to avoid an infinite loop when the linking is incorrect (e.g. next returns itself), we could `assert(count_linked++ < count_flagged)`;

💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756759971)
Yes, absolutely! But, this PR or something else would need to be merged first, or we would need to have a method `SetFresh` because there's still a case where we have FRESH but not DIRTY.

I think we should keep the internal flags as a bitmap, since if we introduce new fields for each state it will increase the memory footprint of each `CoinsCachePair`.

💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756770827)
> because there's still a case where we have FRESH but not DIRTY.

I only see https://github.com/bitcoin/bitcoin/blob/34a101d4babd0cd2df86b414da385c3bded84027/src/coins.cpp#L204-L210 and https://github.com/bitcoin/bitcoin/blob/34a101d4babd0cd2df86b414da385c3bded84027/src/coins.cpp#L96 in production code - in both cases we're only setting `FRESH` together with `DIRTY`.
Did I miss anything in production code?

> I think we should keep the internal flags as a bitmap, since if we introduce new
...

💬 m3dwards commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#discussion_r1756773854)
I'll take it

💬 hebasto commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1756777516)
> Would be good to add a comment explaining `if(CMAKE_GENERATOR MATCHES "Ninja|Makefiles")` saying that COMPILER_LAUNCHER variable only works for these backends not other backends. Otherwise the check seems arbitrary and doesn't seem to make sense.

Thanks! Done.

🤔 maflcko reviewed a pull request: "test: Drop no longer needed workarounds"
(https://github.com/bitcoin/bitcoin/pull/30847#pullrequestreview-2300178606)
lgtm

💬 maflcko commented on pull request "test: Drop no longer needed workarounds":
(https://github.com/bitcoin/bitcoin/pull/30847#discussion_r1756777352)
Just for reference. This never worked in the first place, because "Skipping" was never picked up, because logging was disabled until it was re-added in commit f03c9420958de31fdfecec5fa3e23134aac61803

💬 fanquake commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1756779260)
When `-DBUILD_FOR_FUZZING=ON` is passed.

💬 maflcko commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#issuecomment-2346184415)
review ACK 72b46f28bf8d37a166961b5aa2a22302b932b756

👍 stickies-v approved a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2300018207)
ACK fa37d9b0fd0b8f1b23163887618b2fa1a817d596

I like the new test structure and added documentation, thanks! Left some non-blocking nits, only if you need to force-push.

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756687137)
nit: in this new new structure, `err_mix` makes more sense than `check_mix`

<details>
<summary>`s/check_/err_/g`</summary>

```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
index 4c61632fce..f398404d75 100644
--- a/src/test/util_string_tests.cpp
+++ b/src/test/util_string_tests.cpp
@@ -62,25 +62,25 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
PassFmt<2>("%2$*3$d");
PassFmt<1>("%.*f");

- auto check_mix{"Format specifiers
...

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756671121)
nit: `%8$` is not a valid specifier
```suggestion
// Positional specifier, like %8$s
```