👍 stickies-v approved a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2974825230)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d

💬 stickies-v commented on pull request "doc: clarify that the "-j N" goes after the "--build build" part":
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177208310)
review note: `gmake` seems fine with placing `-j` earlier, so technically this change isn't necessary, but it's probably better to be consistent anyway.

💬 hebasto commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3023345698)
> Is this PR still an alternative to #31669.

It's related, but I wouldn't consider it an alternative.

💬 bigspider commented on pull request "doc: clarify that the "-j N" goes after the "--build build" part":
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177223248)
Good point. Same for `ctest`, I think.

👍 dergoegge approved a pull request: "fuzz: Make process_message(s) more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-2974855838)
utACK fa30966d7d881f5f299a531bd83a666fa15f4563

🤔 hebasto reviewed a pull request: "depends: fix libevent `_WIN32_WINNT` usage"
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-2974868613)
My Guix build:
```
aarch64
70a6898fa1a63cccb6926d319fbd1be2264f19a5763e7064060964581eda25b6 guix-build-336cab98eb04/output/aarch64-linux-gnu/SHA256SUMS.part
2519f262eaf2e96f3060b41c79d90ff01225939503247bae772d851a4eb092af guix-build-336cab98eb04/output/aarch64-linux-gnu/bitcoin-336cab98eb04-aarch64-linux-gnu-debug.tar.gz
13b69188ff681af2e6dabc9fb2c79112993ca7752348224f0776ce0b2e5d1720 guix-build-336cab98eb04/output/aarch64-linux-gnu/bitcoin-336cab98eb04-aarch64-linux-gnu.tar.gz
69b05e8a
...

💬 purpleKarrot commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3023424198)
ACK 67dc7523f3e103c8359b546d38f28c1feb2b9b34

The issue is specific to python. I leave it up to you and other reviewers whether that should be reworded or not.

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2177272359)
Took this suggestion. After staring at `txrequest` some more, I agree that it makes more sense conceptually to keep functions that are only used to look up announcements as `uint256`. You're right that there is already a distinction made on master between a `GenTxid` and a `txhash` so I don't see a reason to change that in this PR (other than making the actual `Announcement` type safe).

💬 stickies-v commented on pull request "doc: clarify that the "-j N" goes after the "--build build" part":
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177273789)
Oh you're right!

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2177284171)
Used your commits, thanks. I agree it's a lot cleaner and a minimal change so it makes sense as part of this refactor. Also, making `GetIter` type safe was going to happen in the next PR anyway.

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2177289598)
Took this suggestion too and used `concepts` for the templates. Definitely better to not have to repeat these two two functions.

💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#issuecomment-3023473293)
Thanks for reviewing @hodlinator and @stickies-v! Pretty sure I addressed all comments, but let me know if I missed anything in the most recent push (very possible I did). After taking your suggestions, it's looking better and cleaner now for sure. The `txrequest` changes should be quite a bit easier to review now as well.

💬 purpleKarrot commented on pull request "doc: clarify that the "-j N" goes after the "--build build" part":
(https://github.com/bitcoin/bitcoin/pull/32846#issuecomment-3023510964)
This may be unrelated, but cmake also respects some [environment variables](https://cmake.org/cmake/help/latest/manual/cmake-env-variables.7.html).
So instead of passing `-j` to each command, you could just set `CMAKE_BUILD_PARALLEL_LEVEL` in bashrc or whatever your favorite shell uses. In fish, I use the following:

```fish
function cmake
set -x CMAKE_C_COMPILER_LAUNCHER (which ccache)
set -x CMAKE_CXX_COMPILER_LAUNCHER (which ccache)
set -x CMAKE_BUILD_PARALLEL_LEVEL (nproc)
se
...

💬 purpleKarrot commented on pull request "doc: clarify that the "-j N" goes after the "--build build" part":
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177295699)
`cmake --install` supports `-j` too!

💬 hebasto commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#discussion_r2177343557)
Isn't this change from another commit https://github.com/libevent/libevent/commit/dda8968c71f684235abb3cf6c26810751bf2c31a?

🤔 janb84 reviewed a pull request: "cmake: Improve robustness and usability"
(https://github.com/bitcoin/bitcoin/pull/31233#pullrequestreview-2975096506)
tACK 67dc7523f3e103c8359b546d38f28c1feb2b9b34

The PR changes the usage of the `Python3_EXECUTABLE` to the (indeed) more modern target-based approach of `TARGET Python3::Interpreter`

test outcome:
<details>

no python installed:


`cmake -S . -B build ` output gives warning:

```shell
CMake Warning at CMakeLists.txt:710 (message):
Minimum required Python not found. Rpcauth tests are disabled.
```

`ctest --test-dir build -j 8` skips / disables `util_rpcauth_test` (as int
...

💬 hebasto commented on pull request "cmake: Improve Python robustness and test usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3023674710)
Updated the PR title.

🤔 danielabrozzoni reviewed a pull request: "test: Fix wait_for_getheaders() call in test_outbound_eviction_blocks_relay_only()"
(https://github.com/bitcoin/bitcoin/pull/32823#pullrequestreview-2975234206)
ACK ec004cdb86e6471915e1033f390c76ee0428e415

Code looks good to me, I like the rephrasing of the comments, the test is easier to follow now :)

💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2177503856)
actually this is wrong, the test is checking that `timelock_tx_id` is *not* in mempool. If we don't use `invalidateblock` it's essentially impossible to trigger this behavior since the chain should not move backwards in height ever.

Re-read this PR after writing up https://github.com/bitcoin/bitcoin/issues/32838

Let me think about how to best do this test case and enhancing it possibly

🤔 janb84 reviewed a pull request: "docs: add guidance on initialism capitalisation in PascalCase identifiers."
(https://github.com/bitcoin/bitcoin/pull/32720#pullrequestreview-2975291425)
reACK b6f3b335002a6fa5095bedb8ad22a71fce5d35fb

Changes sinds last ACK:
- Author fixed some typo's