💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3553832028)
> > To compute the short channel Id we simply call `getrawtransaction` which gives us the block hash for the block that confirmed the tx, then load that block to get the tx position in that block.
>
> So you need txindex and make 3 RPC calls? ISTM storing the position in this new index would make it much more efficient and more useful to other people.

I could easily return the block hash along with the spending tx, txindex would not be needed anymore but users would still need to call `get
...

💬 l0rinc commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2542915769)
Thank you for checking!
The headers have changed and the column widths, so I had to regenerate the example to be accurate:
```patch
-Duration (µs) Mode Coins Count Memory Usage Prune
+Duration (µs) Mode Coins Count Memory Usage Flush for Prune
```

💬 ryanofsky commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3553938834)
> [@ryanofsky](https://github.com/ryanofsky) how would the node go about tracking the memory footprint of clients?

The way I imagined this working is that `MinerImpl` class would have an `size_t m_block_templates_size` member, which the `BlockTemplateImpl` constructor would increment and the destructor would decrement. Then `createNewBlock` and `waitNext` calls could check this and throw an overloaded exception before they do anything if the the size exceeds some cap.

But I guess a problem wi
...

🤔 vasild reviewed a pull request: "refactor: interfaces, make 'createTransaction' less error-prone"
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-3484023723)
Almost ACK 0b92ee2bc7, modulo:

* Needs rebase. I did rebase it and extended the newly added functional test with some edge cases here: https://github.com/vasild/bitcoin/commits/gui/807/

* The second commit a4007a447ed9a6c21812fe5b3d468cf6f7039a5b `gui: remove unreachable AmountWithFeeExceedsBalance error` implies that https://github.com/bitcoin/bitcoin/pull/25269 was merged, but it was not. Maybe easiest would be to drop that commit from here, so that this PR is not tied to the fate of #25269?
...

💬 vasild commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone":
(https://github.com/bitcoin-core/gui/pull/807#discussion_r2543008409)
I am not sure why this is needed.

🤔 l0rinc requested changes to a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3482342931)
After the tests, I have reviewed the `ThreadPool` as well now - first iteration, have a few questions, I expect a few more rounds of review.
I left some modernization suggestions and fine-grained commit requests to make the change easily reviewable.

I still share many of @andrewtoth's concerns regarding RAII vs Start/Stop/Interrupt and I think we can modernize the pool from `std::condition_variable` with locks to C++20 `std::counting_semaphore`. Besides my suggestions below I think we should
...

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