💬 maflcko commented on pull request "build: Minimize I/O operations in `GenerateHeaderFrom{Json,Raw}.cmake`":
(https://github.com/bitcoin/bitcoin/pull/30842#issuecomment-2346074452)
Not sure why, but for me the command took almost an hour:


```
$ ps -p 816447 --format time,rss,cmd
TIME RSS CMD
00:51:12 117640 /usr/bin/cmake -DRAW_SOURCE_PATH=_/src/bench/data/block413567.raw -DHEADER_PATH=_/bld-cmake/src/bench/data/block413567.raw.h -DRAW_NAMESPACE=benchmark::data -P _/cmake/script/GenerateHeaderFromRaw.cmake
```

Previously it took only a few seconds.

💬 pablomartin4btc commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2346078539)
> Any ideas on how to slow down `scanblocks()` enough to allow a `status` call to consistently return status of a scan in progress?

I've been playing a bit around the `invalidateblock` as I had present in my mind the recent [issue](https://github.com/bitcoin/bitcoin/pull/30808#issue-2504050692) from the assume-utxo testing but that wouldn't make the trick either. I went thru the original PRs of `scanblocks` (#23549 and #20664) but it seems there were no questions around this 'status' call te
...

💬 hebasto commented on pull request "build: Minimize I/O operations in `GenerateHeaderFrom{Json,Raw}.cmake`":
(https://github.com/bitcoin/bitcoin/pull/30842#issuecomment-2346084391)
> Not sure why, but for me the command took almost an hour:
>
>
>
>
>
> ```
>
> $ ps -p 816447 --format time,rss,cmd
>
> TIME RSS CMD
>
> 00:51:12 117640 /usr/bin/cmake -DRAW_SOURCE_PATH=_/src/bench/data/block413567.raw -DHEADER_PATH=_/bld-cmake/src/bench/data/block413567.raw.h -DRAW_NAMESPACE=benchmark::data -P _/cmake/script/GenerateHeaderFromRaw.cmake
>
> ```
>
>
>
> Previously it took only a few seconds.

What's your system?

💬 maflcko commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2346089167)
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).

💬 fjahr commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2346092294)
post-merge utACK 992f83bb6f4b29b44f4eaace1d1a2c0001d43cac

👍 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