💬 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

💬 l0rinc commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2490998686)
Having no fully end-to-end tests sounds a bit dangerous, since it's part of the fundamental behavior of Bitcoin, not sure we should completely mock that away (i.e. certain bugs could creep in that work locally only - no idea how that would be possible, but that's exactly the point).

💬 fanquake commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491001441)
Which tests are you seeing this from? Neither the unit nor functional tests should be making outside connections. This has certainly been discussed/avoided previously so may be a recent regression.

💬 vasild commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491003878)
I have the idea to log non local traffic with `iptables` and fail the CI. But I do not understand why nothing is logged - I added some quick and dirty commands to `ci/test/03_test_script.sh`:

```sh
iptables -A OUTPUT -m addrtype \! --dst-type LOCAL -j LOG --log-prefix "eeeee1" --log-level emerg
iptables -A OUTPUT -j LOG --log-prefix "eeeee2" --log-level emerg
... generate some traffic with telnet and ping ...
# confirm some packets matched:
iptables -v -x -n -L
...
2024-11-21T11:38:4
...

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1851965340)
Good point. Just tested that that's indeed the case. I'll mention that in the commit message.

cc @hebasto

🤔 hebasto reviewed a pull request: "build: Enable -Wbidi-chars=any"
(https://github.com/bitcoin/bitcoin/pull/31315#pullrequestreview-2451227488)
Post-merge ACK fa7857ccda5d82739bba16eeb7244433978d28f3. This compiler option is recommended in the [Compiler Options Hardening Guide for C and C++](https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html).