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

🤔 Sjors reviewed a pull request: "Implement BIP 370 PSBTv2"
(https://github.com/bitcoin/bitcoin/pull/21283#pullrequestreview-2601346881)
Studied the other commits as well.

Perhaps there's an opportunity to split this PR by first introducing the internal refactoring that anticipates PSBTv2. But other that I don't think it can be made much smaller.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946295044)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", nit: calling this param `psbt_version` makes it clear that inputs and outputs don't have their own versioning. And consistent with the eventual variable name.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946298433)
In https://github.com/bitcoin/bitcoin/commit/2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", μnit `const`.