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

💬 hebasto commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#issuecomment-3426298790)
> Here are the recent updates from the upstream projects:
>
> 1. Clang has [merged](https://github.com/llvm/llvm-project/commit/10be254587da24d56e2c6817b382beaca612b6c3) the fix and [backported](https://github.com/llvm/llvm-project/commit/570c4c9443387b756ed3e4cb94ca708841f2472a) it to 21.x.

Available in the recent release: https://github.com/llvm/llvm-project/releases/tag/llvmorg-21.1.4.

💬 ajtowns commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3426393945)
I don't think it makes much sense to split this out; it doesn't reduce the size of the parent PR meaningfully, and renaming a function doesn't really seem like it justifies a PR on its own.

💬 maflcko commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3426413950)
> which risks unintentional disclosure.
>
> Realistically, though, no one’s going to sift through a wall of LLM-generated “vibe” commits to check disclosure tags.

I'd say this is one of the use-cases to cover. If someone does not want to disclose their vibe-coding, but they fail to read their own submission of code (and the corresponding commit messages), the error is on their part. The requirement for pull request authors to understand their own changes is documented in essence in the pre
...

💬 kanzure commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3426491048)
One of the norms ought to be minimizing the waste of time of the project's
contributors. Whether such a waste is achieved by LLM (or not) does not
matter. Using autogenerated code is not itself negligent or malicious, just
like using any other tools could theoretically be productive.

💬 maflcko commented on issue "Unable to fuzz in local on MacOS 15.4.1":
(https://github.com/bitcoin/bitcoin/issues/33667#issuecomment-3426502657)
There are some prior issues/pulls/discussions:

* https://github.com/bitcoin/bitcoin/pull/32084
* https://github.com/bitcoin/bitcoin/issues/32089

Though, this one seems to be a new link-time error.