💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542581787)
re: https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542278811

Thanks for the reply. I think you have all the facts right, and I accept all the points you are making, even if we might prefer different tradeoffs & possible solutions. My point was just that `RPCConvertNamedValues` was using a poor heuristic before this PR, and now it is using a better one, without requiring every parameter to be listed in the table. If you do want to list every parameter in the table and simplify the
...

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2542621198)
My bad, I must have tested a slightly different comparator. The suggested ones are deterministic.

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

<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 8a40e10c1ef4cfdd5fe19
...

💬 maflcko commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2542640064)
```suggestion
height = self.nodes[0].getblockcount() + 1
block_time = self.nodes[0].getblock(tip)['time'] + 1

invalid_blocks = []
for i in range(7): # invalid chain must be at least 6 blocks longer to trigger warning
block = create_block(int(tip, 16), create_coinbase(height), block_time)
```

small nit: Could ensure time and height are incremented in the same places?

💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2542654939)
I added comments where `any_success` was used.

💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3553506888)
Push to 8c81bdf2532aa1c61e25d367a480ce3aa71362ca:

- Rebase on #32747 which has gotten lots of good feedback, and is also rebased on master
- Fix four nits from @vasild

This push is mainly for a CI sanity check, then I'm going to put it in draft while I refactor for @theuni main feedback which will be removing the sockman abstraction and implementing the I/O loop directly in `HTTPServer`

💬 ryanofsky commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3553507404)
> Should I cut and paste what you wrote below into the PR or commit description?

Feel free to use & change any of it. I mostly wrote that for myself to be able to remember what changes this PR was making. I do think the current description is perfectly ok and accurate, but I skipped over this PR many times not really knowing what it is was doing or if I should take the time to find out, so adding some more details could help this get more attention.

📝 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
...