💬 sedited commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2630691600)
I don't think this is quite analog to the `GetPrevious` method. The complete genesis header is serialized with a 0 prev block, but I think that has a specific meaning. Then again, we do skip the prev block field in `getblock`'s output too if it is 0. I tend towards keeping it as is, because external applications can then just directly use this to initialize their own network-serializable block header, without needing to apply extra logic.

🚀 fanquake merged a pull request: "fuzz: doc: remove any mention to `address_deserialize_v2`"
(https://github.com/bitcoin/bitcoin/pull/34091)

🚀 fanquake merged a pull request: "qa: Account for unset errno in ConnectionResetError"
(https://github.com/bitcoin/bitcoin/pull/33875)

💬 l0rinc commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2630727791)
Thanks for checking @maflcko, that's why I asked.
The vectorized operations are obviously supported, but for some reason were not triggered for me either.
@theuni, is this just an emulation anomaly or we were just too cautious?
I understdood that @achow101 has access to real big-endian power9 machine that we might be able to test this on when it's ready.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2630736129)
This seems a bit more complicated - even if it makes sense theoretically.
I expect this will have follow-ups, I suggest we do it in a different PR after this is merged.

💬 stringintech commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2630766312)
> but maybe it is still better to add it for defensive reasons?

I am not against having the try-catch block, but seeing it next to `btck_chainstate_manager_process_block` (which doesn't have try-catch) made me wonder why `btck_chainstate_manager_process_block_header` gets the special treatment. Is there something that makes `ProcessNewBlockHeaders` a better fit for this?

💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2630789857)
1ff59a15407cb5fdc2ae06358e45134166db2448: Besides a unit test for the aggregate, I think a fuzz testing would be nice, e.g.:

```cpp
// Copyright (c) 2025-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <random.h>
#include <test/fuzz/FuzzedDataProvider.h>
#include <test/fuzz/fuzz.h>
#include <test/fuzz/util.h>
#include <test/util/setup_common.h>
#in
...

🚀 fanquake merged a pull request: "log: Use `__func__` for -logsourcelocations"
(https://github.com/bitcoin/bitcoin/pull/34088)

💬 sedited commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2630839850)
I think that is fair. We are doing a bunch of disk reads that may throw when processing new blocks, so adding a try-catch there too seems like a good idea.

💬 fanquake commented on pull request "ci: bump actions/checkout version to v6":
(https://github.com/bitcoin/bitcoin/pull/34094#issuecomment-3670021971)
ACK cd98caea438a61f1e2c6ea8331afa545478f8f94

🚀 fanquake merged a pull request: "ci: bump actions/checkout version to v6"
(https://github.com/bitcoin/bitcoin/pull/34094)

📝 billymcbip opened a pull request: "test: Improve code coverage for pubkey checks"
(https://github.com/bitcoin/bitcoin/pull/34099)
Cover these branches in `IsCompressedOrUncompressedPubKey` and `IsCompressedPubKey`:
- `Non-canonical public key: invalid length for uncompressed key`
- `Non-canonical public key: invalid length for compressed key`
- `Non-canonical public key: invalid prefix for compressed key`

See the missed branches here: https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html

`script_tests` succeed on my end.

💬 billymcbip commented on pull request "test: Improve code coverage for pubkey checks":
(https://github.com/bitcoin/bitcoin/pull/34058#issuecomment-3670079572)
@rkrux I created a new PR with the exact same changes: https://github.com/bitcoin/bitcoin/pull/34099. I hope the code coverage job will run correctly on that PR.

👍 rkrux approved a pull request: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095#pullrequestreview-3592744915)
lgtm, code review ACK 31c8ef3d9c885fbe810d5454ba1041ee87efa917

Nice, I missed reviewing the predecessor PR.

💬 Chand-ra commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670127663)
> The change is not about removing `.count()` everywhere. But to change it in cases where the `count()` statement is used to count the number of occurrences to indicate if something is in the collection. The example you reference eg `bool has_received{last_recv.count() != 0};` is a "correct" usage of `count()` (is it not empty)

Makes sense! I seem to have mistaken `count()` in `std::chrono` with the ones for container classes.

ACK [31c8ef3](https://github.com/bitcoin/bitcoin/pull/34095/com
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3670158639)
code review diff ACK 89372213048adf37a47427112a1ff836ee84c50e

Github diff seems broken - https://github.com/bitcoin/bitcoin/compare/662270f5b8d96434faec9e6f55f566f4f2ba1261..89372213048adf37a47427112a1ff836ee84c50e

`git fetch origin 89372213048adf37a47427112a1ff836ee84c50e && git range-diff 662270f5b8d96434faec9e6f55f566f4f2ba1261...89372213048adf37a47427112a1ff836ee84c50e` is also very verbose and confusing, so ended up checking out old branch, rebasing it locally and soft resetting it on
...

📝 anuragchvn-blip opened a pull request: "doc: Use multipath descriptors in descriptors.md and linked test"
(https://github.com/bitcoin/bitcoin/pull/34100)
Updates documentation and `wallet_miniscript_decaying_multisig_descriptor_psbt.py` to use single multipath descriptors with `<0;1>` syntax instead of separate external/internal descriptors.

## Changes
- **doc/descriptors.md**: Update examples (lines 70-71) to use `/<0;1>/*` multipath syntax
- **doc/descriptors.md**: Update Basic Multisig Example instructions (line 179) to use single multipath descriptor
- **test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py**: Refactor to us
...

💬 l0rinc commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670192777)
Thanks for checking @Chand-ra - your mentioned examples are not "contains" alternatives but "exists" alternatives for `chrono::duration` (which is orthogonal to this change and doesn't seem to exist in C++20 anyway).

> Looks like GitHub nuked all historic commits and historic CI runs here?

I did push a few versions while CI was running, maybe that was confusing - is there anything you need me to do here?

💬 rkrux commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2631043584)
Thanks for the suggestions, updated PR to accommodate both of them.

💬 maflcko commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3670243771)
Actually, parsing the conf file should clear the old/stale datadir value:

```
src/common/config.cpp- // If datadir is changed in .conf file:
src/common/config.cpp: ClearPathCache();
src/common/config.cpp- if (!CheckDataDirOption(*this)) {
src/common/config.cpp- error = strprintf("specified data directory \"%s\" does not exist.", GetArg("-datadir", ""));
src/common/config.cpp- return false;
src/common/config.cpp- }
```

Can you reproduce this on Linux?

💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670247073)
Still fails for me running the CI (via Podman 5.7.1) on my Fedora box (this PR rebased on master):
```bash
ALL | ✓ Passed | 6980 s (accumulated)
Runtime: 1243 s

+ traffic_monitor_end functional
+ test_name=functional
++ get_interfaces
++ set -o pipefail
++ ifconfig
++ awk -F ':| ' '/^[^[:space:]]/ { if (!match($1, /^lo/)) { print $1 } }'
++ set +o pipefail
+ for ifname in $(get_interfaces)
++ tcpdump_file functional eth0
++ loca
...