💬 ryanofsky commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2542857143)
In commit "ArgsManager: support command-specific options" (e2debfa5d38ef06f0676049310088c9e7cf69c34)

Might be worth noting that options with this category will be excluded from help output unless they are referenced by an `AddCommand` call.

Relatedly it might be good to add a check that every option in the `COMMAND_OPTIONS` category is actually referenced by an AddCommand call.

💬 ryanofsky commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2542895141)
In commit "tests: Add some test coverage for ArgsManager::AddCommand" (cce7eba489d5fb8b9f603584b5939dd42ccc6e8d)

Might be good to check details is empty on success, nonempty on failure

💬 ryanofsky commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2542866964)
In commit "ArgsManager: support command-specific options" (e2debfa5d38ef06f0676049310088c9e7cf69c34)

Should the options list be validated against known options? Seems like it could be easy to make a typo and pass a bad list of options to this function, causing help output to be incomplete without it being obvious.

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