📝 pinheadmz converted_to_draft a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061)
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194)

It is based on #30988 but only in the sense that it copies the `Sockman` class introduced in that PR. The p2p / `Connman` refactor isn't needed for HTTP and therefore this branch can be reviewed and merged independently of the p2p changes.

Commit strategy:
- Import `sockman.{h,cpp}` from #30988
- Assert current behavior of HTTP with additional functional test
...

💬 0xB10C commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2542668058)
> This is the actual output of running the script, looks like it wasn't updated in a while

Since the script logs UTXO cache events, running it against a different node will produce different output. Not sure if worth changing the example output of the script here. The example output is just as valid just as it was before.

💬 maflcko commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2542682661)
nit: Could move the comment from the pull description to a code comment?

```py
LARGE_WORK_INVALID_CHAIN_WARNING = (
"Warning: We do not appear to fully agree with our peers " # Exclamation mark removed by SanitizeString in AlertNotify
"You may need to upgrade, or other nodes may need to upgrade."
)

💬 0xB10C commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#issuecomment-3553550433)
Looked at the UTXO tracing changes a bit. They seem fine to me. I can't really comment on the other changes, as I'm not too familiar with the utxo cache in general.

from https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2486109704:
> I was wondering how tracing is affected by this change. The trace includes the mode, and it should display it fine, so the trace should be OK.
> It would be nice to check how the trace looks for a FORCE_SYNC case as well; I guess that can be triggered b
...

💬 maflcko commented on pull request "init: Changing the rpcbind argument being ignored to a pop up warning":
(https://github.com/bitcoin/bitcoin/pull/33813#discussion_r2542697344)
> Why would you ever hit it?

the warning can be hit when `-rpcallowip was specified without -rpcbind`?

🤔 janb84 reviewed a pull request: "kernel: handle null or empty directories in implementation"
(https://github.com/bitcoin/bitcoin/pull/33867#pullrequestreview-3483631324)
ACK 6657bcbdb4d0359c1843ca31fb3670c7c0c260d5

PR Adds logic to handle null pointers and/or empty directories in the chainstate manager. + extends tests and documentation

"Also documents how BITCOINKERNEL_ARG_NONNULL should be used." I would say this also explains how to interpret it. Without this additional info seeing a parameter being described as "non-null" but not included in the Macro. The extra documentation helps with the understanding.

This change has no impact downstream (at l
...

💬 janb84 commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2542704130)
DrahtBot NIT:

```suggestion
* be associated with this kernel context for the duration of their lifetimes.
```

💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2542711002)
I guess I need to specify unsigned char for some platforms ?

```
/home/runner/work/_temp/src/util/strencodings.h:381:23: error: comparison is always false due to limited range of data type [-Werror=type-limits]
381 | if (c < 0) return c;
| ~~^~~
cc1plus: all warnings being treated as errors
```

💬 maflcko commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3553588849)
re-ACK 7dd714ae71fd18eda82ab4b43d4cecc047b87a2d 🔔

<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: re-ACK 7dd714ae71fd18eda82a
...

💬 maflcko commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3553591417)
Could use `doc:` or `build:` prefix in title?

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2542808323)
> would it be possible to invalidate the conversions you don't want instead, maybe something like

Currently implementing this and wondering if this is necessary; can you share what your IDE is confused about? The constructor of `path` that takes a `std::string` is deleted.

💬 maflcko commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2542818267)
Ah, makes sense. Passing null to nonnull can lead to UB, not to be confused with passing null to _Nonnull (https://clang.llvm.org/docs/AttributeReference.html#nonnull), which doesn't lead to UB :sweat_smile:

💬 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