💬 maflcko commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#issuecomment-3553708843)
review ACK 6657bcbdb4d0359c1843ca31fb3670c7c0c260d5 🐪

<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 6657bcbdb4d0
...

💬 brunoerg commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2542837356)
8a40e10c1ef4cfdd5fe19796131af2b5a34531ce: nit: I think that using `reversed` would be clearer.

```suggestion
for invalid_block in reversed(invalid_blocks):
```

💬 l0rinc commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2542843754)
I don't have Windows, it's why I asked if instead of adding a new method we can just delete the conversions, somewhat similarly to the mentioned https://github.com/bitcoin/bitcoin/pull/33550/files#diff-69423eb01bf14b3bd0d930c0b3e1fd6f4f061ffefacab579053eaa734fc22f38R65 (since the error seemed superficially similar to me).

👍 brunoerg approved a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3483863317)
ACK 8a40e10c1ef4cfdd5fe19796131af2b5a34531ce

Code lgtm. I verified that reducing one block doesn't trigger the warning and makes the test to fail.

👍 ryanofsky approved a pull request: "ArgsManager: support subcommand-specific options"
(https://github.com/bitcoin/bitcoin/pull/28802#pullrequestreview-3483679773)
Code review ACK cce7eba489d5fb8b9f603584b5939dd42ccc6e8d. Left various suggestions, but none are important so feel free to ignore.

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

Any plans to reuse this class? It would seem simpler to inline:

<details><summary>diff</summary>
<p>

```diff
--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -646,35 +646,6 @@ void ArgsManager::CheckMultipleCLIArgs() const
}
}

-namespace {
-/** Helper class for iterating over COMMAND_OPTIONS applicable to a given command */
-template <typename T>
-class CommandOp
...

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

Giving this function an `indent` parameter doesn't seem right semantically since the other parameters are higher-level parameters unrelated to formatting, and the function is still hardcoding most other formatting value internally. This also increases code complexity by requiring a separate `HelpMessageSubOpt` function, because the caller doesn't have access to the formatting constants. Would
...

💬 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