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

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946310826)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version": somewhere later in the PR, we should add a target for version 2?

Or better: any version, which would detect that the `PSBTInput` constructor doesn't prevent version 1 (not currently a problem, but could creep in if we build functionality to add inputs to a PSBT).

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946376405)
In c6743bf7137615eb20145c6eb3e2da5c82903b1e "Enforce PSBT version constraints": this is hard to read. Would prefer two separate checks based on the PSBT version.

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946305833)
In 2dc96bd4dc5169f692b94ae98eeb6bcd6b0f1ee9 "Have PSBTInput and PSBTOutput know the PSBT's version", nit: `/*psbt_version=*/` would be useful here, but this goes away in a later commit (and before the default is changed to 2).

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1946391356)
In 0c459e43ba1ce74d2e88dde352e28a63743f4746 "Add PSBT::CacheUnsignedTxPieces": this seems fine, but it would be good to have a method that verifies that for v0 PSBTs these two representations are in sync. You could then enable that method in debug builds and call it in a bunch of places.

The could e.g call `GetUnsignedTx()` (introduced later), skip its `if (tx != std::nullopt)` early return, and compare.