💬 frankomosh commented on pull request "doc: add coverage instrumentation hint to libFuzzer quickstart":
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3370955199)
> 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)?

💬 mikekelly commented on pull request "rpc, test: allow multiple data outputs in `createrawtransaction` & `createpsbt`":
(https://github.com/bitcoin/bitcoin/pull/32790#issuecomment-3370956828)
@yuvicc FYI - there are failing checks on this. If you make this build green then it could be proposed as part of v30 release.

💬 fanquake commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3370962466)
An `rc3` has been tagged: https://github.com/bitcoin/bitcoin/releases/tag/v30.0rc3.

💬 mikekelly commented on pull request "rpc, test: allow multiple data outputs in `createrawtransaction` & `createpsbt`":
(https://github.com/bitcoin/bitcoin/pull/32790#issuecomment-3370963517)
fwiw - I think if this is going in to 30.0 it would need some kind of warning to tell users that they may face propagation issues. If it's delayed to 30.1 then a warning probably isn't necessary.

💬 hebasto commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3370969802)
> 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 guess you are asking how to deal with the fs linter?

This, and something like `operator std::string() const = delete;` should be added to `fs::path`.

💬 mikekelly commented on issue "v30rc2 createrawtransaction unable to create txns with multiple OP_RETURNs":
(https://github.com/bitcoin/bitcoin/issues/33544#issuecomment-3370970077)
@maflcko I suppose if it's not going into 30.0 then it wont need warnings. It's a shame to not have it in 30.0 but if the ship has sailed it is what it is.

🤔 stickies-v reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3303574618)
Approach ACK 2e040d8b3c861c96c536754a82f4352b635c4d01

Intuitively it seems helpful to track something like `BLOCK_FAILED_CHILD`, but since it's been unused for so long it seems the use case isn't really there. Block validation logic is complex and critical and simplifying it is worth the effort, so I think this is the way to go.

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405483992)
In 8e6c195396304e3c66471e208b1cd59eea02c11c:

This logic seems to deal with some kind of block index corruption (e.g. unclean shutdown) where not all children of an invalid block have been marked as invalid on-disk.

Let's assume a chain of blocks `A -> B -> C`. Do we have strong guarantees that it's impossible that:
- `A` is marked `BLOCK_FAILED_VALID`
- `B` is marked `BLOCK_FAILED_CHILD`
- `C` is not marked invalid

In the above scenario, the `blockstorage.cpp` logic on `master` would
...

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405494993)
In 507b937eb39866c182b8a8939d2a71a80618f398: I think the `invalid_walk_tip` can (and if so, should) be kept local to the `while (true)` loop. Once that loop is finished, `invalid_walk_tip` should be equivalent to `pindex`? This better isolates the tip-rewinding logic.

Suggested diff (that also uses `{disconnected,new}_tip` and moved code closer to where it's used for readability):


<details>
<summary>git diff on 507b937eb3</summary>

```diff
diff --git a/src/validation.cpp b/src/vali
...

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405502780)
In 2e040d8b3c861c96c536754a82f4352b635c4d01:

It seems like we're trying to cram two different operations into one:
1. ensuring that all descendants of an invalid block are marked as invalid, in case there was any corruption/interruption
2. phasing out now-deprecated `BLOCK_FAILED_CHILD` flags

The current approach doesn't look robust to me, i.e. it won't catch all `BLOCK_FAILED_CHILD` flags. Suggested alternative that is more robust and (imo) more readable:

<details>
<summary>git diff
...

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