🤔 pablomartin4btc reviewed a pull request: "test: move create_malleated_version() to messages.py for reuse"
(https://github.com/bitcoin/bitcoin/pull/33793#pullrequestreview-3426229391)
cr ACK 2bd155e6ee7e3cabd76083ac921b34bb45d98769

💬 maflcko commented on pull request "test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set":
(https://github.com/bitcoin/bitcoin/pull/33795#issuecomment-3495381887)
As mentioned in the issue, I don't see the problem that free threading solves for us. In fact, it is likely going to cause more issues that are not worth it to debug nor fix.

Free threading could make sense if there is a heavy multithreaded production workload. However, I don't see this in the functional test, which are single-threaded, or at most double threaded?

I'd say we should just require the GIL for all Python code. I can't see Python removing the GIL any time soon without controve
...

💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2497861709)
Did the same on master:
```
git log -1
commit 5c5704e730796c6f31e2d7891bf6334674a04219 (HEAD, upstream/master, upstream/HEAD, origin/master, origin/HEAD)
```
and unfortunately I'm getting the same:
```
time ./build/bin/bitcoind -stopatheight=921129 -dbcache=45000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0\
&& time ./build/bin/bitcoind -stopatheight=921129 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0\
&& time ./build/bin/bitcoind -stopathei
...

💬 purpleKarrot commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3495683770)
> Maybe a better approach would be to run the enforced sections in a separate, faster job?

With such a job, it is possible to copy the output and pipe it through [`fix_includes.py`](https://github.com/include-what-you-use/include-what-you-use/blob/master/fix_includes.py) locally. Full reproducibility is nice to have (and can be achieved with a dev container that contains all the necessary tools in the right version), but for a development workflow it is also possible to ignore IWYU (just rely
...

💬 purpleKarrot commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3495699926)
ACK a8a33bc0c0a11093418debc36db8ac63bf90e687

💬 maflcko commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3495782091)
> With such a job, it is possible to copy the output and pipe it through [`fix_includes.py`](https://github.com/include-what-you-use/include-what-you-use/blob/master/fix_includes.py) locally.

For reference, if devs want to copy the output, the job already has the full diff, ready to apply. So there is no need to take an extra step. Example https://github.com/bitcoin/bitcoin/actions/runs/19076699870/job/54494252522?pr=33779#step:9:41045:

```diff
IWYU edited 873 files on your behalf.

+ g
...

👍 hodlinator approved a pull request: "net: make m_nodes_mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32394#pullrequestreview-3426944143)
re-ACK 1a9274f391df0465c17b6b6664988af70910e39b

Changes since previous review (https://github.com/bitcoin/bitcoin/pull/32394#pullrequestreview-3353773054):
* Resolves conflict with #32983.

💬 l0rinc commented on pull request "[kernel] Expose `CheckTransaction` consensus validation function":
(https://github.com/bitcoin/bitcoin/pull/33796#discussion_r2498106807)
shouldn't this be bigger than 12 (or could maybe be 0) so that new values can be added in the future?

👍 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://
...