🤔 w0xlt reviewed a pull request: "net_processing: reorder the code that handles the VERSION message"
(https://github.com/bitcoin/bitcoin/pull/33792#pullrequestreview-3461290288)
Thank you for the detailed explanation.
ACK https://github.com/bitcoin/bitcoin/pull/33792/commits/52149b0e2b7897cc2ea059a9b50c4cb3b9fcd722

👍 ryanofsky approved a pull request: "test: Ignore error message give from python because of PYTHON_GIL"
(https://github.com/bitcoin/bitcoin/pull/33795#pullrequestreview-3461291850)
Code review ACK ef268ee288c04a31dd802c4bb729a0c2e6e64786. IMO, this is the most direct fix possible for #33582, and I think it's a better fix than skipping the test or forcing the GIL to be enabled long-term. It can be reverted later if pycapnp is updated to work without the GIL, or if we decide we do want to turn on the GIL explicitly.

💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2524678860)
I can also add `PKHash`, but for watch only loading I don't think worth adding more types. Wouldn't reflect its usage in practice.

👍 hebasto approved a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3461366311)
re-ACK 96963b888e5a10f4024fa0449c60c02e3bed6245, rebased, addressed my comments and adjusted formatting in `symbol-check.py` since my recent [review](https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3456081353).

💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#issuecomment-3529446791)
Force-pushed [6436ad7](https://github.com/bitcoin/bitcoin/commit/6436ad7f2f93774cb719c89965800b51f546420c) to [6227441](https://github.com/bitcoin/bitcoin/commit/6227441b782b50403571d3d97e8aa174c95fc511):

- Added `PKHash` for `LoadWatchOnly`
- Addressed https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2521415641
- Addressed https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2521400460

🤔 ryanofsky reviewed a pull request: "init: Require explicit -asmap filename"
(https://github.com/bitcoin/bitcoin/pull/33770#pullrequestreview-3461345406)
Thanks for the reviews! Made all suggested changes

<!-- begin push-4 -->
Updated 44717da29b626bfdcb20a32318689d74c4113b7b -> deac31dd779d28a8c01f1c3153650f5827cfe75d ([`pr/asmapd.3`](https://github.com/ryanofsky/bitcoin/commits/pr/asmapd.3) -> [`pr/asmapd.4`](https://github.com/ryanofsky/bitcoin/commits/pr/asmapd.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/asmapd.3..pr/asmapd.4))<!-- end --> updating files.md and adding a more specific error message for a missing filename

💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2524678779)
re: https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2518375913

Thanks, added a more specific error message for this case.

💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2524680342)
re: https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2518380662

> nit: I was going to say that the unspecified file and the missing file test could be deduplicated since they are currently similar but I think the better change would be to have an explicit error message for the empty `-asmap` arg.

Make sense, implemented the separate error message

💬 yancyribbens commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2524738408)
I was pointing out you where using `&&` when it should have been `||` since `data_dir == nullptr && data_dir_len > 0` will not return early if `data_dir` is null but `data_dir_len` is greater than zero. Looks like you made that change though :)

💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2524753751)
Shouldn't this and the one below be `LogWarning` to clearly highlight that something is wrong?

🤔 w0xlt reviewed a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3461743341)
ACK https://github.com/bitcoin/bitcoin/pull/33872/commits/0aebdac95da9a7d476264424c0107bd806ce5362

💬 w0xlt commented on pull request "init: completely remove `-maxorphantx` option":
(https://github.com/bitcoin/bitcoin/pull/33872#discussion_r2524978802)
nit:
```suggestion
previously deprecated and has no effect anymore since v30.0. (#33872).
Passing it now causes startup failure.
```

👍 TheCharlatan approved a pull request: "kernel: handle null or empty directories in implementation"
(https://github.com/bitcoin/bitcoin/pull/33867#pullrequestreview-3461861782)
ACK 6657bcbdb4d0359c1843ca31fb3670c7c0c260d5

💬 stringintech commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3529907874)
Thanks @ryanofsky for explaining the trade-offs!

I think I also prefer we push this change after we support separate logger instances per connection though, and at this point keep the API consistent with what it's currently capable of. I'm not sure how big of an impact delaying this would have on bindings/users, especially if we're assuming their use cases are mostly limited to a single logging connection.

👍 pablomartin4btc approved a pull request: "Added test coverage for qt gui#901 console history filter"
(https://github.com/bitcoin-core/gui/pull/910#pullrequestreview-3462041401)
ACK 310e4979b36cbcf1e9e01dd90c14e2e9997343a0

💬 w0xlt commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#issuecomment-3530135641)
Concept ACK

💬 Sammie05 commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#issuecomment-3530190674)
Tested on Windows and the Qt test runs successfully without requiring manual QT_PLUGIN_PATH environment setup.

Approved

💬 stickies-v commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2525371442)
> will not return early if `data_dir` is null but `data_dir_len` is greater than zero

that's literally English for `data_dir == nullptr && data_dir_len > 0` evaluating to `true`, therefore satisfying the `if` condition, therefore triggering the early return.

> you where using `&&` when it should have been `||`

Sorry, but... no. If the statement was `if ((data_dir == nullptr || data_dir_len > 0) || (blocks_dir == nullptr || blocks_dir_len > 0))` as you suggest, we'd do an early nullptr r
...

💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3530425864)
Thanks for the feedback @stringintech. Can you explain what specifically you may be looking for which this PR does not provide? The API defined here is consistent and logical. It just happens to be compatible with new features like per-connection options and separate log streams (already implemented in #30342) instead of being pointlessly incompatible with them. It also eliminates the btck_logging_disable() function which is unsafe and confusing. And it gets rid of the kernel's inefficient defau
...

💬 151henry151 commented on pull request "doc: gen-manpages.py should check build options":
(https://github.com/bitcoin/bitcoin/pull/33085#issuecomment-3530698712)
Hi @BrandonOdiwuor; I did some work on this in #33828, perhaps you might find some ideas there to implement here. Feel free to use any of it that might help.

💬 danielabrozzoni commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#issuecomment-3530733454)
post-merge concept ACK, thanks for working on this! :)