💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2541801771)
nit: for consistency with other spelling, e.g. https://github.com/bitcoin/bitcoin/blob/2de0ce5cd85e1b99e318883964df318ffb615fe4/src/util/threadpool.h#L94

```suggestion
* Returns a `std::future` that provides the task's result or propagates
```

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2541836752)
some of these includes seem unused, can you please check?

<details>
<summary>iwyu_tool.py</summary>

```
python3 /opt/homebrew/bin/iwyu_tool.py -p build-iwyu src/test/threadpool_tests.cpp -- -Xiwyu --cxx17ns -Xiwyu --mapping_file=$PWD/contrib/devtools/iwyu/bitcoin.core.i
mp -Xiwyu --max_line_length=160 -Xiwyu --check_also=$PWD/src/util/threadpool.h

/Users/lorinc/IdeaProjects/bitcoin/src/util/threadpool.h should add these lines:
#include <__vector/vector.h> // for vector

/Users/
...

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2542010169)
https://corecheck.dev/bitcoin/bitcoin/pulls/33689 suggests using `std::jthread` instead, which would eliminate the need for manual joins - can you check if our infrastructure would support that?

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2541760411)
we already have individual test cases, there's no need to duplicate them here

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2542051426)
nit: we could mark this as `[[nodiscard]]` and reformat with clang-format:
```suggestion
template <class F>
[[nodiscard]] EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) auto Enqueue(F&& fn)
```

Note that this reveals that we often ignore the return value and should make it explicit if that's deliberate or not.
I also think that calling in `Enqueue` could describe its behavior better.

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2542073214)
as far as I can tell, we're ignoring the return value here and silently ignoring any thrown thread exception - which we have tested so carefully.

Since my understanding is that this is an asynchronous fire-and-forget task, we can't just `.get()` the future here, but we can likely handle the potential exceptions inside and void the method to signal that we're deliberately discarding the future (not forgetting it).

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2542192400)
```suggestion
BOOST_CHECK_EQUAL(future.wait_for(WAIT_TIMEOUT), std::future_status::ready);
```

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2542015748)
nit: comment is redundant, the code already explains it. There are few other comments that don't provide value, can you please check them?

💬 l0rinc commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2542188657)
```suggestion
UninterruptibleSleep(200ms);
```

💬 Creepdog7 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/b537a2c02a9921235d1ecf8c3c7dc1836ec68131#commitcomment-170902553)
Is this concerning #825?

💬 Sjors commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3554023671)
> It would be nice if we could ignore this problem and just treat all transactions as unique

It might be simplest thing to implement and we can improve it later. But for some rough numbers: 4MB per template, one per second for two hours, would be counted as 28GB, even if the actual footprint is a fraction of that.

💬 achow101 commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3554041212)
We could instead defer signing of the transaction until after "Send" is clicked.

> Edit: Why is the loading of PSBT upset if there are signatures? In other words, why `throw` here:

A (v0) PSBT contains the entire unsigned transaction in its own field, with signatures and scripts placed in the PSBT fields. The transaction must be unsigned in order to make further signing and finalizing possible to reason about generically. Trying to make a PSBT from a transaction that already contained signatur
...

👍 l0rinc approved a pull request: "qa: Account for unset errno in ConnectionResetError"
(https://github.com/bitcoin/bitcoin/pull/33875#pullrequestreview-3484132142)
untested code review ACK 76e0e6087d0310ec31f43d751de60adf0c0a2faa

This is a continuation of https://github.com/bitcoin/bitcoin/pull/30660/files#diff-7f22a082e3a80ca2f212e36193f87dd80237035afedb7f15b116ef7fa265f3eeR326, separating the existing `TimeoutError` to add extra details for `ConnectionResetError` as well.

💬 fjahr commented on pull request "test: Fix race condition in IPC interface block progation test":
(https://github.com/bitcoin/bitcoin/pull/33880#issuecomment-3554086620)
> Good catch. The test needs to check two things:

Thanks for explaining, I have switched the check to node0 and kept it above the sync because that feels like the most natural flow given my improved understanding of the test now. I have also added some short comments based on your explanation.

💬 D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3554088845)
with [36ae2ef](https://github.com/bitcoin/bitcoin/commit/36ae2effba30f6806a6171fa6a8018fde5215302) rebased over master.

No changes to commits...

💬 ryanofsky commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3554122176)
re: https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3553506888

> This push is mainly for a CI sanity check, then I'm going to put it in draft while I refactor for @theuni main feedback which will be removing the sockman abstraction and implementing the I/O loop directly in `HTTPServer`

I agree with theuni it should make this PR easier to review and simpler if current sockman functionality ([sockman.cpp](https://github.com/pinheadmz/bitcoin/blob/8c81bdf2532aa1c61e25d367a480ce3aa7
...

💬 hebasto commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3554161151)
> > minimum GCC version for Windows cross-compilation to [13.1](https://doc.qt.io/qt-6.8/supported-platforms.html#windows).
>
> Have you checked that it doesn't work with 12?

This [branch](https://github.com/hebasto/bitcoin/tree/251119-qt6.9) fails to cross-compile on Debian Bookworm with GCC 12.2.0:
```
/home/hebasto/bitcoin/depends/work/build/x86_64-w64-mingw32/qt/6.9.3-d5ebe8f681b/qtbase/src/3rdparty/pcre2/src/pcre2_compile.c: In function ‘pcre2_compile_16’:
/home/hebasto/bitcoin/dep
...

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2543210150)
Done. Since the `net_processing` warning is now harder to understand (half of the logged text comes from validation) I added a comment there.

💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3554211685)
[6db2551](https://github.com/bitcoin/bitcoin/commit/6db2551dc27c4a9b989e8814054c93dd9d8f1b36) to [7f28483](https://github.com/bitcoin/bitcoin/commit/7f284835be87c4d1a8a56804992043e12dae1ea1):
- addressed https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2539190197
- reworked the second `CheckForkWarningConditions` commit to unify the log message with the alernotify warning. I noticed the discrepancy due to #33893

🤔 mzumsande reviewed a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3484323098)
Concept ACK

FYI I proposed to reword the warning message in #33553 (7f284835be87c4d1a8a56804992043e12dae1ea1).