👍 ryanofsky approved a pull request: "cmake: Move IPC tests to `ipc/test`"
(https://github.com/bitcoin/bitcoin/pull/33774#pullrequestreview-3461148887)
Code review ACK 866bbb98fd365962840ee99df0d9f7ad557cc025, just adding back the suggested comment, and also fixing bad include arguments passed to target_capnp_sources. It would probably be a little better if the include fix was done in an earlier commit, since it's not really related to the other changes in the last commit, but would also be ok to make both changes at the same time.

As mentioned below i am confused about include paths used to build the `bitcoin_ipc` librariy which somehow loo
...

💬 ryanofsky commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2524524789)
In commit "cmake, test: Improve locality of `bitcoin_ipc_test` library description" (866bbb98fd365962840ee99df0d9f7ad557cc025)

This change should be correct, but I'm confused about how it was working before. This change causes includes in generated files such `build/src/ipc/capnp/common.capnp.proxy.h` to be generated correctly relative to `src/`:

```diff
-#include <src/ipc/capnp/common.capnp.h> // IWYU pragma: keep
+#include <capnp/common.capnp.h> // IWYU pragma: keep
```

So that see
...

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