💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542570737)
> I don't understand why my suggestion does not work. `my=` is not a valid named argument for `createwallet`, so the check `"my=wallet".starts_with("wallet_name=")` evaluates to false for all args, and the argument is correctly identified as a positional arg. Conversely, if you were passing `wallet_name=bla`, the check would also correctly identify the named arg.

That's what something similar to the current code is doing internally but with more checks and conditions. So one of the reasons yo
...

💬 plebhash commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3553407353)
we're discussing this on a Sv2 call and @GitGab19 pointed out that whatever default value is set for `-maxtemplatepoolsize` should be a reasonable number

the main concern is having values that are too low, which would make templates be discarded too soon

we're not sure what the ideal value should be, but just writing this to bring awareness to this

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