💬 ajtowns commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2447506448)
(Adding `using enum TxBroadcastMethod` would keep the type safety without requiring the prefix)

💬 waketraindev commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3425662444)
Why not simply create a label to flag them if their behavior is causing offense? What’s the purpose of disclosure, particularly if it’s an inadvertent one the user didn’t intend or consent to?

📝 maflcko opened a pull request: "ci: Drop libFuzzer from msan fuzz task"
(https://github.com/bitcoin/bitcoin/pull/33666)
libFuzzer is mostly unmaintained (https://llvm.org/docs/LibFuzzer.html#status), and it isn't really needed by the CI tasks. While it provides some additional stats like rss or the max input byte size, they are not essential.

Also, there seems to be a history of intermittent false-positive msan warnings (https://github.com/bitcoin/bitcoin/pull/33600#issuecomment-3391921802).

It is unclear what exactly is causing the false-positives, so just disable libFuzzer in this task for now, to work ar
...

💬 optout21 commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3425689192)
Concept ACK c9bfd298be422de7e989fe244fb4281c507068a3

💬 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