💬 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
...

💬 l0rinc commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498556752)
might be unrelated, but we have remaining `__FILE__, __LINE__, __func__` combos in one more file: https://github.com/bitcoin/bitcoin/blob/1a1f46c2285994908df9c11991c1f363c9733087/src/rest.cpp#L91-L94

Could we migrate that as well to something like:
```C++
StrFormatInternalBug("Node context not found!",
std::source_location::current()));
```

💬 l0rinc commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498559756)
Same, we don't have a [function name](https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-lambda-function-name.html) here anymore, do we still need the linter suppression?

💬 l0rinc commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498574967)
nit: we should probably use `%d` for the line number instead of `%s`
```suggestion
auto str = strprintf("%s:%d %s: Assertion `%s' failed.\n", loc.file_name(), loc.line(), loc.function_name(), assertion);
```

👋 l0rinc's pull request is ready for review: "merkle: migrate `path` arg to reference and drop unused args"
(https://github.com/bitcoin/bitcoin/pull/33805)

💬 hebasto commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3496817118)
> > Having to run the full CI either on push or locally just to fix the includes is not ideal.
>
> Something I was thinking about while review the first part of this PR was whether using CMake's `CXX_INCLUDE_WHAT_YOU_USE ${iwyu_path}` to run this as another configuration "natively" with cmake. The issue here is it's project-wide, and you have to set [`SKIP_LINTING`](https://cmake.org/cmake/help/latest/prop_sf/SKIP_LINTING.html#prop_sf:SKIP_LINTING) on those files you don't want it run on. The
...

💬 naiyoma commented on issue "importdescriptors: check for errors before rescanning":
(https://github.com/bitcoin/bitcoin/issues/33655#issuecomment-3496960469)



> ! I think a cleaner approach would be to separate the validation into a separate function entirely, so we validate in a loop first then process in a second loop. It smells better than adding an extra argument and feels more bitcoin-core-y. If you open a PR like that I'll help you get it merged ;-)

great, I'll work on this

💬 fanquake commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3496969502)
> For reference, if devs want to copy the output, the job already has the full diff, ready to apply.

I don't think this diff is "ready to apply"? Headers like `errno.h` are wrong, and should be `<cerrno>` (this is enforced by `modernize-deprecated-headers` in the tidy job)? Includes like `#include "net_types.h"` are also wrong, and should be `#include <net_types.h>`?

Given that developers working on this code, have said that this change will likely make development more difficult, should
...

💬 ryanofsky 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-3496998057)
I'd be interested to know if the test passes with GIL force-disabled with `PYTHON_GIL=0` or `-Xgil=0`. If it does, then maybe it will be trivial for a new version of pycapnp to declare it is compatible with free-threaded python and this warning will fix itself.

In any case, I think it would probably be best just to suppress this warning with [`warnings.filterwarnings`](`https://docs.python.org/3/library/warnings.html#warnings.filterwarnings) instead of skipping the test, since we are ok with
...

💬 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-3497029863)

> Leave things as they are. Developers can open a PR, and see if the CI turns green. If not, they can scroll through ~68'000 lines of CI output, to extract a diff, potentially (see above) modify the diff so it can be applied, then push again.

The massive output is just the iwyu warning/debug output. On real pull requests that run into errors, the diff should be smaller. (Should be easy to test by pushing this pr's ci changes without the code changes)

> I don't think this diff is "ready t
...