⚠️ davidgumberg opened an issue: "IBD stalls if addnode peer is in IBD"
(https://github.com/bitcoin/bitcoin/issues/34096)
Starting with two fresh nodes A and B. If you start node A and before it completes IBD start a second node B with `-addnode=$ADDRESS_OF_NODE_A`, node B will stall and not proceed in IBD.

```bash
mkdir -p /tmp/nodea /tmp/nodeb

cat > /tmp/nodea/bitcoin.conf << EOF
prune=1000
bind=127.0.0.1:8333
EOF

cat > /tmp/nodeb/bitcoin.conf << EOF
prune=1000
rpcport=18331
bind=127.0.0.1:18335
addnode=127.0.0.1:8333
EOF
```

and then start both nodes however you prefer, e.g.

```
bitcoind -datadir=/tmp/nodea
...

⚠️ Svki77777 opened an issue: "good"
(https://github.com/bitcoin/bitcoin/issues/34097)
gg

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2629585169)
Correct we don't run into exceptions here but its better to keep for defensive reason. see https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508041569

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2629656192)
I think the previous approach seems like a better match and consistent with the rest of the codebase.

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2629659019)
friendly ping! @sedited.

💬 maflcko commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629716761)
Actually, I ran the CI config locally, but it didn't catch this, as the platform opts out. The CI doesn't run the ` sed -i "s/# define CHACHA20_VEC_DISABLE_STATES_2/\/\/# define CHACHA20_VEC_DISABLE_STATES_2/" src/crypto/chacha20_vec_base.cpp && \` portion, so it would not have caught this, even if the task was run.

💬 maflcko commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-3668646866)
merging is not done in this repo to solve conflicts. Please use a rebase, as explained in the (now deleted) drahtbot comment:

> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).

💬 maflcko commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3668673323)
Only change is replacing the sed commands with a large patch.

review ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8 🌄

<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+krxU1
...

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