💬 fanquake commented on pull request "scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r1851843891)
forgot to add pkgconf?

💬 maflcko commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851847694)
Though, I am wondering if `brew unlink pkg-config` will start failing once pkg-config is removed from the GHA image?

Wouldn't a better fix be to drop the `unlink` and the install and add a comment to add `install pkgconf` in the future?

👍 hebasto approved a pull request: "macOS: swap docs & CI from pkg-config to pkgconf"
(https://github.com/bitcoin/bitcoin/pull/31335#pullrequestreview-2451039128)
ACK 278687dcc010d0a826e55b864164cf42beddff46.

💬 fanquake commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851849029)
> Wouldn't a better fix be to drop the unlink and the install and add a comment to add install pkgconf in the future?

See https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851793270.

📝 hodlinator opened a pull request: "test: Deduplicate assert_mempool_contents()"
(https://github.com/bitcoin/bitcoin/pull/31338)
Recently added `mempool_util` implementation probably evolved in parallel with the package RBF one before being submitted as part of ephemeral dust in e2e30e89ba4b9bdbcabaf5b4346610922f0728bb (despite feedback: https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825278134, https://github.com/bitcoin/bitcoin/pull/31279#pullrequestreview-2445579323).

💬 maflcko commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851869449)
> > Wouldn't a better fix be to drop the unlink and the install and add a comment to add install pkgconf in the future?
>
> See [#31335 (comment)](https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851793270).

IIRC you still tried to install. However, my recommendation is to do neither. Otherwise, either can fail, depending on the state of the GHA image.

I tested this in https://github.com/bitcoin/bitcoin/commit/4191e4a34c7f91fe1dfc68baa91998067a144619 and it passed. What am I m
...

💬 fanquake commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851871250)
Nothing, the suggested change is already in this PR.

💬 maflcko commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2490854220)
ACK fe3457ccfff9a022d9f183e18217422e2e1f7689 🍭

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK fe3457ccfff9a022d9f183e182
...

👍 l0rinc approved a pull request: "refactor: Prepare compile-time check of bilingual format strings"
(https://github.com/bitcoin/bitcoin/pull/31295#pullrequestreview-2450986722)
ACK fa3e074304780549b1e7972217930e34fa55f59a

Lots of versions for this change, loosely coupled, but I understand they will simplify follow-ups.
I have validated manually that:
* `run-lint-format-strings.py` needs the exception 👍
* all code moves are same as before 👍
* tidy warns for `feebumper.cpp`, `salvage.cpp` and `wallet.cpp` 👎 (couldn't reproduce any of them, but manually checked it and at least it shouldn't be worse than before)

💬 l0rinc commented on pull request "refactor: Prepare compile-time check of bilingual format strings":
(https://github.com/bitcoin/bitcoin/pull/31295#discussion_r1851812567)
I tried reproducing the tidy warnings with
```bash
$ clang++ --version
Homebrew clang version 19.1.3

$ cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
&& cmake --build build -j$(nproc) \
&& run-clang-tidy -p build -j$(nproc) -checks='-*,modernize-use-emplace' src/wallet/feebumper.cpp
```

But it doesn't give me the mentioned warnings.
The change itself does make sense (forwarding the bilingual_str params directly), I just can'
...

💬 hebasto commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490932956)
Reworked per feedback.

💬 vasild commented on issue "Segmentation fault when ./bitcoind":
(https://github.com/bitcoin/bitcoin/issues/31321#issuecomment-2490945812)
To get more useful information:

```sh
gdb ./bitcoind
(gdb) run
# it will crash
(gdb) backtrace
```

👍 hebasto approved a pull request: "macOS: swap docs & CI from pkg-config to pkgconf"
(https://github.com/bitcoin/bitcoin/pull/31335#pullrequestreview-2451167289)
re-ACK fe3457ccfff9a022d9f183e18217422e2e1f7689.

👍 l0rinc approved a pull request: "test: Deduplicate assert_mempool_contents()"
(https://github.com/bitcoin/bitcoin/pull/31338#pullrequestreview-2451168273)
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638

💬 l0rinc commented on pull request "test: Deduplicate assert_mempool_contents()":
(https://github.com/bitcoin/bitcoin/pull/31338#discussion_r1851929770)
inlining `assert_mempool_contents` and removing dead code results in the exact same code as before 👍

💬 maflcko commented on pull request "refactor: Prepare compile-time check of bilingual format strings":
(https://github.com/bitcoin/bitcoin/pull/31295#discussion_r1851932559)
I don't think it matters either way in practise, because `std::string` can be moved so cheaply that an elided temporary won't be noticed one way or another. This commit is just taken from the other pulls, so that clang-tidy is happy with them.

💬 l0rinc commented on pull request "refactor: Prepare compile-time check of bilingual format strings":
(https://github.com/bitcoin/bitcoin/pull/31295#discussion_r1851938945)
Can you reproduce the warning in this PR? If not, what's the reason for the warning in other PRs (and not here). If you can, what was I doing wrong?

💬 vasild commented on issue "RFC: Adopt C++ Safe Buffers?":
(https://github.com/bitcoin/bitcoin/issues/31272#issuecomment-2490966684)
Concept ACK

> required patch is large-ish

Which patch exactly? To switch everything to use `std::array` and make clang-tidy-avoid-c-arrays happy (and similar for `std::span`)? Approach ACK on that one ;)

💬 Sjors commented on pull request "Make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#issuecomment-2490980708)
I dropped the use of `has_value()` https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851743969 as well as `value()` https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850680750. Except in the test, where throwing `std::bad_optional_access` is probably useful.

Added a comment: https://github.com/bitcoin/bitcoin/pull/31325/files#r1851747612

Dropped the word "kernel" and added a motivation to the commit message.

⚠️ vasild opened an issue: "Avoid internet traffic from tests"
(https://github.com/bitcoin/bitcoin/issues/31339)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Tests should not try to open connections to the internet because:

* they may succeed or fail unpredictably, depending on external environment
* are slow
* dox the developer to their ISP that they are running Bitcoin Core tests

### Expected behaviour

Tests should only open local connections (e.g. on the `lo` interface).

Enforce this in the CI, having it to detect non-local traffic
...