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

👍 rkrux approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2450922162)
tACK 878b6c85466366c4ae5f454ec49b5a5f561e0ed2

Successful make, unit and functional tests. Definitely a value-add, I like how the problem a user faces has been described in the PR description, and then how this feature solves it.

I reviewed the range diff along with reviewing again the PR as whole.
```
git range-diff f383db7...878b6c8
```

I tested it on mainnet on a quite active address `bc1qryhgpmfv03qjhhp2dj8nw8g4ewg08jzmgy3cyx` for blocks `870930` and `870931`. Sharing an observati
...

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851782089)
Nesting `outspk` worked out well, I was not a fan of the special check for `desc` in the loop above earlier but at that moment seemed unavoidable.

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851787388)
Nit: Although only one usage right now, but making `201` dependent on `200` above in the setup seems more forward facing: https://github.com/bitcoin/bitcoin/pull/30708/files#diff-dfe8598636b25e8bef111dc785f4cfc330e936191d9adb95b66add813a926839R23.

```python
GENERATED_BLOCKS = 200
...
...
'height': GENERATED_BLOCKS + 1,
```

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851877237)
Thinking out loud, not necessary to be a change atm: The crux of these 4 lines is to return a `CBlock` given `chainman.m_blockman, *blockindex` . `block_data` and `block_stream` are not used beyond these 4 lines and seem internal to this intent, a sign these can be encapsulated in a function if we anticipate this pattern to be replicated.

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851772090)
Can also test here for duplicated addresses at the end of `test_multiple_addresses`.

```python
# Duplicated descriptors do not return duplicated results
result = node.getdescriptoractivity([blockhash], [f"addr({addr_1})", f"addr({addr_1})"], True)
assert_equal(len(result['activity']), 1)
```

💬 l0rinc commented on pull request "Make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#issuecomment-2490989399)
ACK 2fff10a656edb8bec7d45433bf431fd233d0fd81