💬 maflcko commented on pull request "refactor: Construct g_verify_flag_names on first use":
(https://github.com/bitcoin/bitcoin/pull/33600#issuecomment-3425693525)
For reference, the msan false-positive can also be worked around via https://github.com/bitcoin/bitcoin/pull/33666. However, I still think this pull makes sense, because it is already reviewed and still comes with the other mentioned benefits at basically no cost. (If there were a cost, we could go with the compile-time stuff, but that diff is a bit larger, see above).

💬 maflcko commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3425916353)
(rebased)

🤔 l0rinc reviewed a pull request: "ArgsManager: support subcommand-specific options"
(https://github.com/bitcoin/bitcoin/pull/28802#pullrequestreview-3359431997)
Concept ACK, it makes sense to associate specific command-line options with specific commands - like function names with args. I can imagine this being useful in other cases as well. Given that the PR needs a rebase, I have commented on every nit I could find - and I would need full test coverage for this to be comfortable acking, but like the commit split otherwise. If you can, I would appreciate adding slightly more context to the PR description and the commit messages.

💬 l0rinc commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2447268297)
```suggestion
if (!select.contains(cmdopt_name)) continue;
```

💬 l0rinc commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2447261539)
```suggestion
if (valid_opts.contains(opts.first)) continue;
```

or even better, to avoid having the `dummy`, we could make this a predicate:
```C++
auto is_valid_opt{[&](std::string_view opt) {
return command_args != m_command_args.end() && command_args->second.contains(opt);
}};
...
if (is_valid_opt(opt_name)) continue;
```

💬 l0rinc commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2447280855)
This seems safe, just have to make sure: is it safe to add in the middle of this enum (e.g. is its index persisted anywhere?).
If the placement doesn't matter, can we add the options closer to the command values (before IPC)?

💬 l0rinc commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2447311617)
This line seems to conflict with latest `master`.

nit: is it important for the set to be sorted here?

💬 l0rinc commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2447292459)
Is this comment still useful now?

💬 l0rinc commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2447492960)
This seems more complicated than I think it should be, `strprintf` has baked-in indentation
```suggestion
return strprintf("%*s%s%s\n%*s%s\n\n",
optIndent + indent, "",option, help_param,
msgIndent + indent, "", FormatParagraph(message, screenWidth - msgIndent - indent, msgIndent + indent));
```

(Though I would have expected `FormatParagraph` to be able to apply the indentation at the beginning as well, which would enable)
```suggestion
return s
...

💬 l0rinc commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2447591467)
Most of the new code is completely uncovered: https://corecheck.dev/bitcoin/bitcoin/pulls/28802

Adding temporary exceptions inside indicates that `tool_wallet.py` and `wallet_encryption.py` cover some of the functions here.
I understand that `argsman_tests.cpp` is already very weak coverage-wise, but would be great to add some coverage for the new code at least.

💬 l0rinc commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2447499771)
slightly unrelated: maybe we could use `std::string_view` here instead

💬 l0rinc commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r2447318308)
nit: formatting

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#issuecomment-3425956618)
@theuni I hear you, maybe I should re write the PR description: The purpose is not to establish a generic socket manager but to just advance the bigger goal of replacing libevent with #32061. In that context, this PR is just the first 11 commits of the HTTP server replacement, broken off for more focused unit testing and review (which has all been great so far).

So with that in mind, what do you recommend? Is it higher level organization like removing the abstract class stuff and moving all t
...

✅ willcl-ark closed an issue: "Data corruption on MacOS when using exFAT datadir or blocksdir"
(https://github.com/bitcoin/bitcoin/issues/31454)

💬 willcl-ark commented on issue "Data corruption on MacOS when using exFAT datadir or blocksdir":
(https://github.com/bitcoin/bitcoin/issues/31454#issuecomment-3425966534)
closing since #31453

🤔 w0xlt reviewed a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3360026838)
reACK https://github.com/bitcoin/bitcoin/pull/32983/commits/b63428ac9ce2c903670409b3e47b9f6730917ae8

⚠️ rkrux opened an issue: "Unable to fuzz in local on MacOS 15.4.1"
(https://github.com/bitcoin/bitcoin/issues/33667)
Reproduction steps as per [libfuzzer](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md#quickstart-guide):
```zsh
$ brew install llvm lld
$ cmake --preset=libfuzzer \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld"
$ cmake --build build_fuzz
```


<details open>
<summary>
Getting the following error instead on the last step:
</summary>

```zsh
[ 99%] Building CXX object
...

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2447930605)
After cluster mempool, mini_miner should be modified anyway (and I think this whole MiniMinerMempoolEntry object will disappear, see for example my draft [here](https://github.com/bitcoin/bitcoin/commit/d24f02699ec4af215a4ffae3a2ae2457f71bf93f#diff-5dfb364874cfa42dd98492c3c3c1b07aafbe73ef7ecd895246d2fea00499fd6c)). So I don't think it's worth re-implementing this interface in this PR.

💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3426285363)
Yea. Still failing for me with Podman (5.6.2) on my Fedora box.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2447949351)
Can you clarify what you have in mind here?