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

💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3497066592)
Thanks @ryanofsky for the ping and for moving the conversation forward.

I am -0 on this change for now because in the recollection of conversations about ASMap usage whenever the conversation touched this (which was rarely) then either people were indifferent about the default path or found it nice to have. I don’t recollect anyone expressing negativity about it, as in it doesn’t make sense or it won’t be used. I can not point at specific people or conversations that were documented for this,
...