💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3534660813)
My guix build hashes:

```x86_64
fddfcf1d14219e7df35dd4c25546b99df2edb1fc29856c7ae1f7a9f27bfb24b1 guix-build-2594d5a189e5/output/aarch64-linux-gnu/bitcoin-2594d5a189e5-aarch64-linux-gnu-debug.tar.gz
488d6c14892a1753495091c8a49a00d1f0e3d2702d9d7b3536fa98e714e217ea guix-build-2594d5a189e5/output/aarch64-linux-gnu/bitcoin-2594d5a189e5-aarch64-linux-gnu.tar.gz
a02136d07e0aab9f6c0352fe280226db51bb34000f6db9163a64ca7ee05902b0 guix-build-2594d5a189e5/output/aarch64-linux-gnu/SHA256SUMS.part
c0
...

💬 vostrnad commented on issue "Default ASmap file path is not used unless -asmap is set":
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3534768830)
@fjahr Yes, I think removing the default altogether is probably the best way forward, especially in light of a possible future release with embedded ASmap data, and it should remove any potential for confusion around how the option works.

💬 theStack commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2529085149)
these two lines can be dropped; `expect_disconnect` was removed in #33050 (commit 876dbdfb4702410dfd4037614dc9298a0c09c63e) and `valid_in_block` is set to False by default anyways
```suggestion
```
other than that, lgtm

💬 vostrnad commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529117675)
I also don't like the inconsistent `(default: none)`. I think it's pretty clear that if no default is mentioned, the feature is disabled. The help text for `-i2psam` should be fixed.

💬 vostrnad commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529118333)
nit: `strprintf` isn't needed anymore.

💬 151henry151 commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-3534886629)
It looks to me like GUI’s “Create unsigned” path still signs the transaction, so legacy inputs end up with non-empty `scriptSig` and the PSBT parser rejects them. Perhaps we could call `CreateTransaction` with signing disabled or explicitly clear each `scriptSig`/`scriptWitness` before building the PSBT, or maybe it would be better to reuse the RPC PSBT builder so the GUI and RPC share the same logic.

I haven't attempted to reproduce yet, just read through the issue and thinking about possible
...

💬 Christewart commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2529334053)
Done in a7c96f8

👍 theStack approved a pull request: "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`"
(https://github.com/bitcoin/bitcoin/pull/31823#pullrequestreview-3467369575)
ACK a7c96f874de13ace9814da92fd6160a126a97ebf

📝 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]);
-
...