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

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946353102)
To avoid overlaps. In `master`, without this PR, this test could generate nodes with duplicate `id`s.

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

✅ fanquake closed an issue: "fuzz: oss-fuzz coverage build is failing"
(https://github.com/bitcoin/bitcoin/issues/31770)

🚀 fanquake merged a pull request: "depends: Avoid using the `-ffile-prefix-map` compiler option"
(https://github.com/bitcoin/bitcoin/pull/31800)

💬 rishkwal commented on pull request "test: test_inv_block, use mocktime instead of waiting":
(https://github.com/bitcoin/bitcoin/pull/31811#issuecomment-2642617401)
tACK 2706c5b

Tested multiple times. Previously took anywhere between 36 to 96 seconds. Now consistently taking around 35 to 37 seconds. Agree that `test_inv_block` does not need real-time delay.

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946367832)
Ah, and we can't make `CConnman` `final` while we have `struct ConnmanTestMsg : public CConnman`, got it. Should have tested before suggesting.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946368965)
It is for every function, not just "thread" ones - `doc/developer-notes.md` says:

> - Combine annotations in function declarations with run-time asserts in
> function definitions ...

This means:

```cpp
void f() EXCLUSIVE_LOCKS_REQUIRED(!m);

...

void f()
{
AssertLockNotHeld(m);
...
}
```

The reasoning is that the annotation `EXCLUSIVE_LOCKS_REQUIRED` can be ignored. It is nice that the check is done at compile time, but if the compiler is not clang then it wil
...

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946371426)
`SocketsAndThreads` would be clearer, but I think the current name is fine.

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946377084)
We open/close sockets, we don't start/join them, so I prefer the current name over adding "And".

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1946381130)
Ah, so it's done as a general rule even if it makes less logical sense for the function at the root of a thread (as nothing can have taken the lock before it unless the thread is wrapped in some kind of utility that is weird enough to take locks).

💬 fanquake commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2642699952)
> Of course would be nice to test this on windows but it should work

@hebasto ?