💬 maflcko commented on pull request "qa: Account for unset errno in ConnectionResetError":
(https://github.com/bitcoin/bitcoin/pull/33875#issuecomment-3668757736)
review ACK 76e0e6087d0310ec31f43d751de60adf0c0a2faa 🌬

<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: review ACK 76e0e6087d03
...

💬 maflcko commented on pull request "qa: Account for unset errno in ConnectionResetError":
(https://github.com/bitcoin/bitcoin/pull/33875#issuecomment-3668758592)
tested via:

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 6cf47eba73..a43436ef9c 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -263,6 +263,7 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m)
/** HTTP request callback */
static void http_request_cb(struct evhttp_request* req, void* arg)
{
+ assert(false);
evhttp_connection* conn{evhttp_request_get_connection(req)};
// Track active requests
{

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2629994409)
> The change seems unsafe because none of the other functions currently calling `value()` are switched to use `operator*` instead, so they will now throw instead of assert if the value is not set. This would make it dangerous to swap the `util::Expected` class with `std::expected` in the future.

I understand that switching `util::Expected::operator*` to `std::expected::operator*` is unsafe, but this is documented in https://en.cppreference.com/w/cpp/utility/expected/operator%252A.html:

> I
...

💬 stringintech commented on pull request "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation":
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3669209803)
9f946a7 to 6b92a69 - rebased, resolved conflict with #30214, and addressed https://github.com/bitcoin/bitcoin/pull/33604#discussion_r2628258598. Also PR and first commit descriptions are updated.

💬 Chand-ra commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3669342761)
I performed a quick `grep '\.count()'` and it looks like there are quite a few instances that seem eligible for this change but aren't included here, is there some reason for doing so? For example, I found [1](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550/src/net.cpp#L2017), [2](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550/src/net.cpp#L2018), [3](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550
...

💬 janb84 commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3669409766)
> I performed a quick `grep '\.count()'` and it looks like there are quite a few instances that seem eligible for this change but aren't included here, is there some reason for doing so? For example, I found [1](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550/src/net.cpp#L2017), [2](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550/src/net.cpp#L2018), [3](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a5
...

💬 stickies-v commented on pull request "log: Use `__func__` for -logsourcelocations":
(https://github.com/bitcoin/bitcoin/pull/34088#discussion_r2630483108)
> If someone really wants the long name, a SourceLocation::function_name_long() could be added in the future.

Sounds good to me, and I agree that this should only be added when necessary. My concern was more about needlessly converting code that doesn't care about the function name being long or short, but I see your point about type safety. Can be marked as resolved.

💬 stickies-v commented on pull request "log: Use `__func__` for -logsourcelocations":
(https://github.com/bitcoin/bitcoin/pull/34088#issuecomment-3669610759)
ACK facd3d56ccbe2414a5f2b75be7132cd8b904f1e9

💬 maflcko commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3669632035)
> Task `lint`: https://github.com/bitcoin/bitcoin/actions/runs/20319772315/job/58372602822

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

💬 rkrux commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3669679934)
> Is this really gui-specific? Does it not happen with bitcoind?

Yes, it seems to be GUI specific, and doesn't happen with bitcoind alone.

> Can you check the directory permissions please

I did and noticed that the regtest and testnet subdirectories didn't have the r|x permissions for group and others, unlike the data directory. I gave those permissions to them but I don't believe it was the issue - when I was looking into the directory permissions, I also noticed that one more regtest subdir
...

💬 sedited commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2628854781)
I'm not sure what you mean here. Afaict this is moved over from `ConnectBlock`.

💬 sedited commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2630573322)
It is important that the BIP30 validation is done before we spend the coins. The current split makes sense to me for this reason. Can you elaborate a bit more what you think might not be optimal here?

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