📝 hebasto opened a pull request: "doc: Update NetBSD Build Guide"
(https://github.com/bitcoin/bitcoin/pull/33876)
The `py310-zmq` binary package is not available by default on NetBSD 10.1. It has been updated to `py313-zmq`, and the `python310` package is updated accordingly.

See: https://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/index-all.html.

📝 stringintech opened a pull request: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877)
Remove the int parameter from `btck_chainstate_manager_options_*_in_memory` functions and rename them from "update" to "set". These functions now only set the options to `true` for enabling in-memory mode. This, combined with explicit default initialization to `false`, removes ambiguity about what happens when these functions are not called and makes the typical use case (disk-based storage) require no action from the user.

📝 stringintech converted_to_draft a pull request: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877)
Remove the int parameter from `btck_chainstate_manager_options_*_in_memory` functions and rename them from "update" to "set". These functions now only set the options to `true` for enabling in-memory mode. This, combined with explicit default initialization to `false`, removes ambiguity about what happens when these functions are not called and makes the typical use case (disk-based storage) require no action from the user.

👍 hebasto approved a pull request: "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3468081115)
ACK 2594d5a189e52052c2019faccaa47f2affdc48e1.

👋 stringintech's pull request is ready for review: "kernel: Rename in-memory DB option setters and simplify API"
(https://github.com/bitcoin/bitcoin/pull/33877)

💬 hebasto commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3536509285)
https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3534660813

> My guix build hashes:

@151henry151

To make hashes easily comparable, they are sorted using `env LC_ALL=C sort`. The full command to retrieve the hashes is as follows:
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
```

💬 hebasto commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3536510041)
cc @purpleKarrot

🤔 hodlinator reviewed a pull request: "Embed default ASMap as binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895)
Reviewed e00deb2cfa60e846ea3de48bed35c7d3992db4bd

Seems like this PR has evolved a bit since before CMake. Pretty good shape now (using CMake byte header generation instead of Python script etc).

Left a bunch of nits but also some more in-depth suggestions & questions.

Skipped aspects addressed in other PR with similar commits (https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3441618331).

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514801922)
nit: We tend to drop `const...&` for spans, similar to how we don't use `const int&`.
```suggestion
uint32_t Interpret(std::span<const std::byte> asmap, std::span<const std::byte> ip);
```
As the style in:

https://github.com/bitcoin/bitcoin/blob/e00deb2cfa60e846ea3de48bed35c7d3992db4bd/src/test/fuzz/asmap_direct.cpp#L15

(One can always add `const` for the function definition in the .cpp file, as it is the concern of the implementation).

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514815293)
nit: `NO` might work, but isn't `OFF` a clearer inversion of `ON`?
```suggestion
export BITCOIN_CONFIG="-DREDUCE_EXPORTS=ON -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_KERNEL_TEST=ON -DBUILD_SHARED_LIBS=ON -DWITH_EMBEDDED_ASMAP=OFF"
```

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514782960)
Why change this? Various tools expect an empty line before EOF (although I admit it's probably rarely relevant for these generated files).

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2515345175)
nit: Could replace `for`s with `copy_n()`:
```diff
--- a/src/netgroup.cpp
+++ b/src/netgroup.cpp
@@ -89,9 +89,8 @@ uint32_t NetGroupManager::GetMappedAS(const CNetAddr& address) const
std::vector<std::byte> ip_bits(16);
if (address.HasLinkedIPv4()) {
// For lookup, treat as if it was just an IPv4 address (IPV4_IN_IPV6_PREFIX + IPv4 bits)
- for (int8_t byte_i = 0; byte_i < 12; ++byte_i) {
- ip_bits[byte_i] = std::byte(IPV4_IN_IPV6_PREFIX[byte_i]);
-
...

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2515564991)
nit: Align with style of prior line (`sep_pos`)?
```suggestion
const size_t ip_len{buffer.size() - sep_pos - 1};
```

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514789998)
A variant of `BitsToBytes` from src/merkleblock.cpp already exists, but it seems okay to make more modern divisionless version (albeit dividing by 8 probably gets optimized as 3 right shifts). I understand not wanting to touch & reuse consensus code for now.

nit: Clearer parameter name?
```suggestion
std::vector<std::byte> BitsToBytes(std::span<const uint8_t> bits) noexcept
```

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2522389508)
Side note: the documentation below indicates that a failure should be returned if no default ASN has been set, but the current code (also on master) returns zero:
https://github.com/bitcoin/bitcoin/blob/e00deb2cfa60e846ea3de48bed35c7d3992db4bd/contrib/asmap/asmap.py#L158-L165

I think it would be good to update the documentation or the code to match, but understand if others don't want to do it in the context of this PR.

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2515426420)
nit: Could replace `for` with:
```C++
std::ranges::copy(std::as_bytes(buffer.subspan(1, asmap_size)), std::back_inserter(asmap));
```

💬 hodlinator commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2522547981)
remark in 35e6c5bdd467e8342ed606f338c3b4a363c2ef35 - "refactor: Operate on bytes instead of bits in Asmap code":
#### Endian-inversion on a bit-level

The old `vector<bool>` data on master converts to `"dfc0...."_hex` (`df` being the bit-endian-inversion of `fb`, and `c0` being the inversion of `03`).
But the old code would endian-invert each byte while loading from disk or from a byte-buffer:

https://github.com/bitcoin/bitcoin/blob/8f7e02a08626a3141b64df09dfeb38d7d9e2974c/src/util/asmap.
...

💬 purpleKarrot commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3536621101)
ACK 2594d5a189e52052c2019faccaa47f2affdc48e1

Thanks for working on this!

💬 purpleKarrot commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#issuecomment-3536624391)
ACK 0dd8d5c237e2fdb0168d9ca644e7f2f5a8b6ed72

💬 purpleKarrot commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#discussion_r2529975644)
> it returns the path to the “Release” plugin when building with `--config Debug`.

Interesting. I put that in my backlog. Low priority, because your approach is working. I just want to know why the `LOCATION` property is not set correctly.