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

💬 fjahr commented on issue "Default ASmap file path is not used unless -asmap is set":
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3497169826)
> Remove the default value to prevent any confusion.

@vostrnad since you reported this issue and [#33770](https://github.com/bitcoin/bitcoin/pull/33770) implements this suggestion of your, could you say if the removal would be your preferred resolution or if you would like to keep the option around? I think you haven't said if you wanted to actually use the default path or if you just happened to stumble upon this. Thanks!

💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3497216753)
> [P]eople 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

To be sure, I think it could make sense to have a default path that is loaded by default. I think it just to confusing to have a default path that is not loaded by default, and I think https://github.com/bitcoin/bitcoin/issues/33386 shows I'm not the only one who finds the current situation confusing.

Also if #
...

💬 maflcko commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498944152)
I think a temporary workaround, local to two CI tasks, is fine. I don't think there is value in doing more here. See https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498265195. Leaving as-is for now.

💬 maflcko commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498945053)
Thx, done for all in a new, unrelated, commit.

💬 maflcko commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498945189)
My preference is to just use `%s` for all args, unless there is a reason not to. So I'll leave this as-is for now.

Now that the libc++ minimum version is 17, which includes `std::format`, in about 12 months, we can probably bump GCC to 13 as well. Then `{}` can be used for all args, unless there is a reason not to.

💬 maflcko commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498947280)
thx, ran clang-format while changing the `{}` initialization.

💬 maflcko commented on pull request "util: Allow Assert() in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2498948494)
thx, removed redundant and fixed NOLINT