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

💬 0xB10C commented on pull request "ci: Bump fuzz task timeout":
(https://github.com/bitcoin/bitcoin/pull/31814#issuecomment-2642711802)
fwiw: duration of the `fuzzer,address,undefined,integer, no depends` task over the last few months in MM:SS

📝 ryanofsky opened a pull request: "multiprocess: Lock CapnpProtocol::m_loop with mutex"
(https://github.com/bitcoin/bitcoin/pull/31815)
This change is intended to fix intermittent failures in `ipc_tests` unit test and `rpc_misc.py` functional tests that happen on mac os, which sometimes fail with error `terminating due to uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument` as reported https://github.com/chaincodelabs/libmultiprocess/issues/154

This error could happen when [`m_loop.reset();`](https://github.com/bitcoin/bitcoin/blob/81eb6cc2c609b2fb3af53a53749e6162fecb34e2/src/ipc/capnp/proto
...