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

💬 dergoegge commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2498817972)
> I look at fuzz tests as exploratory tools that are hard to write and run and debug and do code coverage ... not trivial to maintain ... not my go-to way to test something

There is a learning curve to writing and using fuzz testing but it is 100% worth getting in to. The maintenance aspect only relates to running them using libFuzzer on macOS.

On average, coverage guided fuzzing will be magnitudes better at finding bugs than this style of unit test. For something as small as the comparato
...

💬 yancyribbens commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2498830734)
> Are you using it as a synonym for “reduce”?

Yes, that is correct. I was using it in terms of a minimization function, that is, it is a step in the direction of a _more_ minimal solution. Agree to remove that wording though if your don't like it; I can see why it's confusing.

> Removing the OutputGroup with the lowest effective_value is a simple algorithm that increases the average effective_value per weight of the selection.

Said another way, the selection density is biased towards
...

💬 roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3497161573)
Doe this mean that, due to `AreInputsStandard`, even more UTXOs were soft-confiscated when #12460 was added?