💬 hebasto commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371016620)
> > I guess this means any fs::path would have to be explicitly cast to a std::fs::path, which seems fine to do.
>
> This is one way of doing it.

Another approach would be to restore `fsbridge::ifstream` and `fsbridge::ofstream`, which were removed in 41d7166c8a598b604aad6c6b1d88ad46e23be247.

💬 hebasto commented on issue "ci: add (atleast one) *BSD job to the CI":
(https://github.com/bitcoin/bitcoin/issues/33438#issuecomment-3371042199)
Also related: https://github.com/bitcoin/bitcoin/issues/31618.

💬 hebasto commented on issue "multiprocess: `ipc_tests` fail on *BSD":
(https://github.com/bitcoin/bitcoin/issues/31618#issuecomment-3371071221)
I no longer see any issues in `ipc_tests` on *BSD systems in the [nightly builds](https://github.com/hebasto/bitcoin-core-nightly).

✅ hebasto closed an issue: "multiprocess: `ipc_tests` fail on *BSD"
(https://github.com/bitcoin/bitcoin/issues/31618)

👍 rkrux approved a pull request: "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key"
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3303950151)
lgtm ACK 8b4d4b3

Thanks for quickly addressing the comments.

```zsh
git range-diff 74521dd...8b4d4b3
```

I have tried to think if the fix covers all the cases that could possibly arise in various descriptors containing private keys partially. Essentially, the `ToPrivateString` function of the descriptor classes returns `true` even if there is a single private key available for it, whereas the newly added `HavePrivateKeys` function returns `true` when all the private keys for the descriptor ar
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3371121013)
`6aa43a298d...8725d10df5`: rebase due to conflicts

💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#issuecomment-3371163805)
Friendly ping @maflcko @fanquake :)

⚠️ fanquake opened an issue: "`test_bitcoin-qt`: segfault under LTO (CMAKE_INTERPROCEDURAL_OPTIMIZATION=ON)"
(https://github.com/bitcoin/bitcoin/issues/33548)
```bash
podman run -it ubuntu:24.04
apt install git build-essential cmake pkgconf python3 libevent-dev libboost-dev qt6-base-dev qt6-tools-dev qt6-l10n-tools qt6-tools-dev-tools libgl-dev libqrencode-dev
git clone https://github.com/bitcoin/bitcoin/
cd bitcoin
```
```bash
cmake -B build -DENABLE_IPC=OFF -DBUILD_GUI=ON -DENABLE_WALLET=OFF
cmake --build build
ctest --test-dir build # works fine
```

```bash
cmake -B build -DENABLE_IPC=OFF -DBUILD_GUI=ON -DENABLE_WALLET=OFF -DCMAKE_INTERPROCEDURAL_
...

📝 hebasto converted_to_draft a pull request: "cmake: Check user-defined `APPEND_*FLAGS` variables early"
(https://github.com/bitcoin/bitcoin/pull/32367)
For the purpose of checks performed by the build system, we strive to handle user-defined `APPEND_*FLAGS` in the same way as flags provided by other standard means. In particular, they are considered when the `try_compile()` command is used.

However, these flags are not considered during `enable_language()` command invocation due to design limitations, which might cause issues, such as https://github.com/bitcoin/bitcoin/issues/32323.

This PR addresses the issue by introducing an additional
...

💬 optout21 commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2405912840)
Why remove this? Is it just too obvious after the `while (pa != pb)`?

💬 optout21 commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2405916981)
I think what is meant here is that equal-height blocks have equal-height skip _values_, the pointer can be different (if on different forks). I suggest changing to "height skip values"

💬 janb84 commented on pull request "doc: add coverage instrumentation hint to libFuzzer quickstart":
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3371303641)
> > NACK [Developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-fuzz-coverage) already has an extensive section on generating coverage. Maintaining the same documentation in 2 places is not ideal.
>
> ok . Would you atleast like to see a reference to the developer notes in this section(assume for someone who lands here first)?

That would be better, imho (although the correct way would be to first read the contributing guide as a developer), bu
...

💬 ryanofsky commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371304309)
What is the bug exactly?

The current description is confusing because it says "constructing `std::ifstream` or `std::ofstream` from an `fs::path` object triggers its implicit conversion to `std::string`". But there aren't any unsafe impilcit conversion from paths to strings, there is only a [safe implicit conversion](https://en.cppreference.com/w/cpp/filesystem/path/native.html) to the native string type.

So what is the bug? Are there compile errors on windows? Is this the concern? Would be go
...

💬 hebasto commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371355059)
> What is the bug exactly?

The code [fails](https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3369255505) to compile using the [LLVM MinGW](https://github.com/mstorsjo/llvm-mingw) toolchain

💬 maflcko commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371355716)
> So what is the bug? Are there compile errors on windows? Is this the concern? Would be good to say what the specific bug or expected behavior is.

Yes, my understanding is there'd be compile errors, but I had difficulty as well figuring this out. My understanding is that there are no unsafe conversions or other bugs. So my suggestion was to just add an explicit cast. Your `std_path` suggestion seems fine as well.

🤔 janb84 reviewed a pull request: "Clear out space on GHA jobs"
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3304256794)
re ACK c01214e72b9876c59924c98e96f43906f5df3353

Changes sinds last ACK:
- changed precondition test for action step.
- removed superfluous line that sets a (now unused) variable

I like the solution because the alternative would be to create an custom GHA Image (as [suggested](https://github.com/orgs/community/discussions/160692#discussioncomment-10281958) by gh) which would be a more maintenance burden than this script.

💬 hebasto commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371370716)
> > > I guess this means any fs::path would have to be explicitly cast to a std::fs::path, which seems fine to do.
> >
> >
> > This is one way of doing it.
>
> I don't understand this response. What are the other ways of doing it?

https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371016620:
> > > I guess this means any fs::path would have to be explicitly cast to a std::fs::path, which seems fine to do.
> >
> >
> > This is one way of doing it.
>
> Another approach would be to
...

💬 apogio commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3371377309)
Pipeline issues should be fixed now.

💬 ryanofsky commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3371385637)
> > What is the bug exactly?
>
> The code [fails](https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3369255505) to compile using the [LLVM MinGW](https://github.com/mstorsjo/llvm-mingw) toolchain

Thanks this is very helpful. I think deleting `operator std::string` with comment like `// Disallow std::string conversion to ensure code is portable, because this operator is not defined on windows` would be a good solution to ensure the problem does not go undetected.

I wouldn't want `fs
...

💬 optout21 commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#issuecomment-3371390469)
ACK 3635d62f5a935801e26a0d5fa2cb5e2dbbb42f9b

With the new `chain_test` test, this optimization reduces the number of steps in `LastCommonAncestor` by at least 5-fold (e.g. from 5898713 to 1119703), an obvious optimization.
However, timing the tests (pre and post optimization) I could not measure a difference.
I've left two non-blocking nit comments.