👍 rkrux approved a pull request: "test: move create_malleated_version() to messages.py for reuse"
(https://github.com/bitcoin/bitcoin/pull/33793#pullrequestreview-3427083745)
crACK 2bd155e6ee7e3cabd76083ac921b34bb45d98769

Seems like a common enough utility function.

📝 l0rinc opened a pull request: "merkle: migrate `path` arg to reference and drop unused args"
(https://github.com/bitcoin/bitcoin/pull/33805)
After the dead code chunks found during review in https://github.com/bitcoin/bitcoin/pull/33768 and https://github.com/bitcoin/bitcoin/pull/33786, I browsed the code coverage for more hints for dead code, this is another instance I found

### Summary
Simplifies `MerkleComputation` by removing parameters that had constant values at its only call site in `ComputeMerklePath`:

### Fixes
* Converts the `path` parameter from pointer to reference (always non-null)
* Removes `proot` and `pmutate
...

👍 hodlinator approved a pull request: "util: Allow Assert() in contexts without __func__"
(https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3427192513)
ACK fae1d99651e29341e486a10e6340335c71a2144e

#### Tested inserting `CHECK_NONFATAL` to see how output differed

Was concerned that `std::source_location::current()` inside of macros might behave slightly differently than `__func__` etc. But only expected differences where observed.

```diff
--- a/src/script/miniscript.cpp
+++ b/src/script/miniscript.cpp
@@ -17,6 +17,7 @@ namespace miniscript {
namespace internal {

Type SanitizeType(Type e) {
+ CHECK_NONFATAL(false);
int num_types =
...

💬 hodlinator commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498232714)
remark: Disabling dangling reference warnings globally is not ideal, but chances that it will not be reverted as the container is upgraded seem slim.

💬 maflcko commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498265195)
Correct, it is not ideal. Though, it is only disabled for this specific task and only temporary. Also, the GCC check is mostly or fully redundant anyway with the clang check (which works better due to lifetime annotations), as well as all the runtime sanitizers.

💬 purpleKarrot commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-3496243776)
A function that takes a `const std::span<T> coins` argument cannot modify `coins`, but it has mutable access to `coins[x]`.

💬 hodlinator commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-3496249495)
How about having an externally defined software/package name+version string as an input to kernel that can still be defined by Bitcoin Core to get the same behavior as on master? It could default to empty or be a requirement for users of kernel.

Then again, I agree with https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2096016006 that having at least the commit hash would be okay.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-3496313143)
> How about having an externally defined software/package name+version string as an input to kernel that can still be defined by Bitcoin Core to get the same behavior as on master

I recently discussed retaining the version information as a "product name" and then having separate versioning for the bitcoin kernel header. Not sure yet about that approach though. Including just the commit hash sounds like a reasonable trade off.

💬 fanquake commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3496352566)
This issue is a `/gnu/store/path/`:
```diff
2c2
< bitcoin_x86_64: file format elf64-x86-64
---
> bitcoin_aarch64: file format elf64-x86-64
123244,123246c123244,123246
< 0x00001000 746f7265 2f696763 73676164 6b763679 tore/igcsgadkv6y
< 0x00001010 6b6e3766 686e3032 36716763 6b316e73 kn7fhn026qgck1ns
< 0x00001020 69726a7a 722d676c 6962632d 63726f73 irjzr-glibc-cros
---
> 0x00001000 746f7265 2f327163 3263697a 30373867 tore/2qc2ciz078g
> 0x00001010 38776b31 6b696263 7a72
...

💬 l0rinc commented on pull request "test: move create_malleated_version() to messages.py for reuse":
(https://github.com/bitcoin/bitcoin/pull/33793#issuecomment-3496528310)
ACK 2bd155e6ee7e3cabd76083ac921b34bb45d98769

Recreated locally, ended up with the same.
The move is correct, only the method name is changed. The imports are correctly sorted and adjusted.

💬 vasild commented on pull request "fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName()":
(https://github.com/bitcoin/bitcoin/pull/32109#issuecomment-3496547789)
What about setting `errno` when returning `-1`? From [`getsockname(2)`](https://man7.org/linux/man-pages/man2/getsockname.2.html):

> On error, -1 is returned, and [errno](https://man7.org/linux/man-pages/man3/errno.3.html) is set to indicate the error.

The fuzz-mocked getsockname in `master` does not set `errno` when returning `-1`:

```cpp
if (bytes.size() < (int)sizeof(sockaddr)) return -1;
```
and this PR, among other things, fixed that as well.

⚠️ l0rinc opened an issue: "malloc: Failed to allocate segment from range group - out of space"
(https://github.com/bitcoin/bitcoin/issues/33806)
While reviewing and benchmarking https://github.com/bitcoin/bitcoin/pull/31132 I noticed that I was getting these (separately):
```
bench_bitcoin(10874,0x207c84800) malloc: Failed to allocate segment from range group - out of space
bitcoind(70369,0x16f5ff000) malloc: Failed to allocate segment from range group - out of space
bitcoind(71239,0x170f2f000) malloc: Failed to allocate segment from range group - out of space
```
I have checked the same against master and it seems to reproduce: https://
...

💬 hodlinator commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498505073)
Tested running...
```shell
₿ env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh'
```
...without the modification to the env shell scripts. Got the expected error:
```
C++ compiler .......................... GNU 13.0.0, /bin/x86_64-w64-mingw32-g++-posix
...
/ci_container_base/src/test/blockmanager_tests.cpp: In member function ‘void blockmanager_tests::blockmanager_scan_unlink_already_pruned_files::test_meth
...

💬 TheCharlatan commented on pull request "[kernel] Expose `CheckTransaction` consensus validation function":
(https://github.com/bitcoin/bitcoin/pull/33796#issuecomment-3496583496)
Concept ACK on adding more checks. I am not sure how useful this check by itself is though, since it lacks finality, inputs, sigops, amount + fee, and script checks. Are you planning on adding these too and if not, what is the purpose served from surfacing the context-free checks, but not the others?

I think the unit tests are going a bit too far. We don't have to verify again that our validation logic works internally and should instead just verify that the function's contract is correct. I
...

💬 maflcko commented on pull request "fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName()":
(https://github.com/bitcoin/bitcoin/pull/32109#issuecomment-3496640005)
no strong opinion (removing myself for review for now)

💬 hebasto commented on pull request "ci: Add Windows + UCRT jobs for cross-compiling and native testing":
(https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3496698032)
@fanquake

You [comment](https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3480170953) was parsed by @DrahtBot into a NACK in the second-to-top [comment](https://github.com/bitcoin/bitcoin/pull/33764#issuecomment-3478048157), which might affect some reviewers :)

🤔 l0rinc requested changes to a pull request: "util: Allow Assert() in contexts without __func__"
(https://github.com/bitcoin/bitcoin/pull/33785#pullrequestreview-3427593936)
Concept ACK

It seems to me we should do a deeper cleanup now that `__func__` isn't used, apply it to a few more cases and not use a global suppression but adjust the call-sites instead.

💬 l0rinc commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498552804)
is the comment still accurate now that there's no `__func__` here anymore?
And do we need it in the first place, given that it [seems to have been introduced](https://github.com/bitcoin/bitcoin/pull/28737) because of it?

💬 l0rinc commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498525502)
nit: can we align the formatting - either in [column](https://github.com/bitcoin/bitcoin/blob/28d679bad9fda3f180ab0f7d34353e1fa9294d68/src/logging.h#L384-L390) or at the [end of each line](https://github.com/bitcoin/bitcoin/blob/af78d36512999be0ea59715ac5de8cb8d0b3d004/src/util/check.h#L111-L112), but this in-between looks weird

💬 l0rinc commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498620712)
I'm also not a fan of globally disabling these, they could hide [real problems](https://github.com/bitcoin/bitcoin/pull/32313/files#diff-bca63b54ad7fcc2df12f3237fd8775810c1b849c49110f702920655778fcc248).

Doing a `git grep -n "const auto&.*Assert("` shows there are only a few of these
```
src/index/coinstatsindex.cpp:204: const auto& tx_undo{Assert(block.undo_data)->vtxundo.at(i - 1)};
src/init.cpp:2006: const auto& tip{*Assert(chainman.ActiveTip())};
src/node/miner.c
...