💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946227366)
Changed to `LogError()` and removed the newline. I did not know the trailing newlines are now unnecessary (since bbbb2e43ee95c9a8866aa1f65e3f001f752dfed2).

💬 dergoegge commented on pull request "ci: Bump fuzz task timeout":
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642410797)
ACK faca7ac13215fd88b420feea8f06d7404f8fd067

💬 dergoegge commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2642418075)
> corecheck is back up and integrated into the DrahtBot comment, so is anything left to be done here?

I think the only thing missing is an alert system or even just a better overview over all benchmarks? Seems difficult to spot regressions in here manually: https://corecheck.dev/benchmarks

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946240012)
Done.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946244171)
Done.

💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#issuecomment-2642428740)
tahnks for your time @hodlinator!

🚀 fanquake merged a pull request: "ci: Bump fuzz task timeout"
(https://github.com/bitcoin/bitcoin/pull/31814)

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946265398)
:facepalm: makes total sense, I was thinking about the remote sockets.

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946267787)
Yeah, sorry, me confusing sockets again. An `Assume()` would be nice though.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946271517)
Done.

💬 maflcko commented on issue "Continuous benchmark tracking":
(https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-2642488461)
Good point. I guess there could be a comment (or the summary comment could be edited) to encourage people to check the result link of a pull request if the difference is larger than some percent.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946284407)
Removed `struct` from `BindAndStartListening()` in commit `style: modernize the style of SockMan::BindListenPort()`.

`GetBindAddress()` was just moved verbatim without any mods from `net.cpp` to `sockman.cpp` in another commit. This makes it easier to review with
```
[diff]
colorMoved = dimmed-zebra
colorMovedWS = allow-indentation-change
```
in `~/.gitconfig`. Will leave it as it is. Don't want to bloat this PR further with one more "style changing" commit.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946299108)
In general, less indentation makes the code more readable, so I prefer to have `return;` and then the subsequent code to be with less indentation.

Anyway this code was changed and now uses `switch` (see `CConnman::EventI2PStatus()` after I push).

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946303871)
```
error: delete called on non-final 'CConnman' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-abstract-non-virtual-dtor]
```

💬 fanquake commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2642520881)
> Submitted patch in https://github.com/capnproto/capnproto/pull/2235

Thanks. Pulled that into #29796.

💬 maflcko commented on pull request "build: fix MSVC ccache reporting":
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2642527478)
Looks like there are several general problems:

* The summary is just AI generated to repeat the code changes (which are also AI generated?). This is problematic, because if someone wanted to look at AI summaries or generations, they could just ask an LLM themselves.
* The summary repeats the code changes. This is problematic, because the code changes should be explained: Why they are the correct approach (in light of alternatives) and why they fix the problem.
* All tests are failing. This
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946311849)
There are multiple sockets and multiple threads. "socket threads" looks like there is one socket and multiple threads. Will leave it as it is.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946320421)
Less indentation makes the code more readable in general. So I prefer:

```
if (A) {
return;
}
CODE;
```
over:
```
if (A) {
CODE;
} else {
return;
}
```

Reduced scope is nice, but I find `if (auto it{m_nodes.find(id)}; it != m_nodes.end()) {` hard to read. Sometimes I confuse it for a for-loop: `for (foo; bar; baz) {`. Also in this particular case the function ends right after this, so the scope of `it` is the same.

Will leave it as it is.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946344669)
There should be no change in behavior. Note that the original code in `master` was very subtle - it required that `ForEachNode()` would iterate the nodes in increasing order of `id`:

```cpp
std::pair<NodeId, std::chrono::seconds> youngest_peer{-1, 0}, next_youngest_peer{-1, 0};

m_connman.ForEachNode([&](CNode* pnode) {
if (!pnode->IsBlockOnlyConn() || pnode->fDisconnect) return;
if (pnode->GetId() > youngest_peer.first) {
next_youngest_peer = youngest_peer;
you
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946347206)
Done.