💬 maflcko commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3669714977)
I don't think `datadir` is supposed to include the `regtest` portion?

Otherwise, the dir structure will end with `regtest/regtest`, no?

💬 maflcko commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3669746699)
At least that is what I get when running:

```
$ ./bld-cmake/bin/bitcoin-qt -regtest -datadir=/tmp/regtest/ -printtoconsole | grep 'Using data'
2025-12-18T11:03:50Z Using data directory /tmp/regtest/regtest
```

You are setting regtest in the conf file, so that is different, and it may be good to check what the expected interaction is there:

```
2025-12-17T10:22:16Z Default data directory /Users/rkrux/Library/Application Support/Bitcoin
2025-12-17T10:22:16Z Using data directory /Users/rkrux/Lib
...

💬 rkrux commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3669772191)
> I don't think datadir is supposed to include the regtest portion?

If I run without it, I get the same error. If I run without it and without the `-walletdir` as well, then too the same error.
I suspect that the GUI always tries to create the regtest subdirectory and errors out if it's already present - at least with my combination of the arguments and configuration file.

✅ billymcbip closed a pull request: "test: Improve code coverage for pubkey checks"
(https://github.com/bitcoin/bitcoin/pull/34058)

💬 rkrux commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3669785378)
From the debug log:

```
2025-12-18T11:05:02Z Default data directory /Users/rkrux/Library/Application Support/Bitcoin
2025-12-18T11:05:02Z Using data directory /Users/rkrux/Library/ApplicationSupport/Bitcoin/regtest/regtest
2025-12-18T11:05:02Z Config file: /Users/rkrux/Library/ApplicationSupport/Bitcoin/bitcoin-reg.conf
2025-12-18T11:05:02Z Config file arg: regtest="1"
2025-12-18T11:05:02Z Config file arg: [regtest] bind="127.0.0.1:28334"
2025-12-18T11:05:02Z Config file arg: [regtest] bind="12
...

📝 maflcko opened a pull request: "test: [move-only] Move lint functions into modules"
(https://github.com/bitcoin/bitcoin/pull/34098)
The single, large `main.rs` file is fine, but at some point it becomes harder to read.

So reduce the size by pulling functions out into modules.

This can be reviewed with the git option: `--color-moved=dimmed-zebra`

💬 vasild commented on pull request "netif: fix compilation warning in QueryDefaultGatewayImpl()":
(https://github.com/bitcoin/bitcoin/pull/34093#issuecomment-3669832624)
I am not sure why there is no "comparison of integers of different signs" on Linux:

```cpp
#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len <= (len))
```
We pass `int64_t` for `len`. The first comparison checks it against `(int)sizeof(...`, but the second comparison checks it against `nlmsg_len` which is `__u32`, so no matter what type is passed for
...

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