🤔 mzumsande reviewed a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2602622517)
Concept ACK - I opened #31757, which shows that this can actually happen in production.

The suggested fix seems good from a defensive programming point of view, although it doesn't address the root problem why double disconnections can happen.

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2643832134)
Rebased 4ca2462bf516846c5552378345e243bc819553a3 -> 658176160026a3e44c5eaa941880aed34f5926d4 ([`pr/subtree.9`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.9) -> [`pr/subtree.10`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.9-rebase..pr/subtree.10)) dropping std::format calls to address https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2643619428
Updated 658176160026a3e44c5eaa941880aed34f5926
...

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2643842804)
`7866c736c8...7d84f431f9`: rebase due to conflicts and address suggestions

Interface changes:

`SockMan::CloseSockets()` renamed to
`SockMan::StopListening()`

`SockMan::GetNewNodeId()` renamed to
`SockMan::GetNewId()`

`NodeId` renamed to
`SockMan::Id`

`SockMan::EventI2PListen()` renamed to
`SockMan::EventI2PStatus()` and instead of `bool` it now takes an `enum I2PStatus` argument

`SockMan::EventIOLoopCompletedForNode()` renamed to
`SockMan::EventIOLoopCompletedForOne()`


...

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#issuecomment-2643844830)
reACK https://github.com/bitcoin/bitcoin/pull/31810/commits/e107bf78f9d722fcdeb5c1fba5a784dd7747e12f

💬 stickies-v commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#discussion_r1947087440)
> I guess we need to actually set components for the other bins first.

I don't think we do? This patch picks up a component if defined, and otherwise just ignores it. So PR ordering shouldn't matter?

💬 furszy commented on issue "wallet: wrong balance and crash after reorg and unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2643931620)
Nice finding! Cool functional test. Glad to see #31757 fixing the crash.

> Ideally, the SyncTransaction() updates and locator updates due to block connection / disconnection should be done atomically, in one batch, if possible.

I actually implemented this in #25297, three years ago :). Will see how much work is to revive it next week.

🤔 BrandonOdiwuor reviewed a pull request: "cmake: Install man pages for configured targets only"
(https://github.com/bitcoin/bitcoin/pull/31765#pullrequestreview-2602730166)
Concept ACK

👍 pinheadmz approved a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2602735125)
ACK 386eecff5f14d508688e6e7374b67cb54aaa7249

Minor changes since last ACK, addressing my comments:
- Use consensus values instead of policy
- Define `DEFAULT_BLOCK_RESERVED_WEIGHT`
- Release notes updates, these have been bikeshed quite a bit. I'm happy with them but will continue to review changes there is others disagree.
<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 386eecff5f14d508688e6e7374b67cb54aaa7249
-----BEGIN PGP SI
...

💬 ryanofsky commented on pull request "multiprocess: Lock CapnpProtocol::m_loop with mutex":
(https://github.com/bitcoin/bitcoin/pull/31815#issuecomment-2644043194)
So far as of baea320be01eb68ff7ba47d0ca0e00b64366a880 it seems like this change does not solve the problem it was supposed to fix.

There is a new "libc++abi: terminating due to uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument" failure https://github.com/bitcoin/bitcoin/actions/runs/13206947916/job/36872233303?pr=30975#step:7:2433 in 553a7e2f66649590864604ed187bf5bf2e796297 which includes a cherry-picked version of this fix.

💬 Sjors commented on pull request "multiprocess: Lock CapnpProtocol::m_loop with mutex":
(https://github.com/bitcoin/bitcoin/pull/31815#issuecomment-2644043850)
This might have a bug somewhere:

```
test/ipc_tests.cpp:11: Entering test suite "ipc_tests"
test/ipc_tests.cpp:12: Entering test case "ipc_tests"
libc++abi: terminating due to uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument
2025-02-07T19:59:17.387846Z [unknown] [test/util/random.cpp:48] [SeedRandomStateForTest] Setting random seed for current tests to RANDOM_CTX_SEED=32db8d22914609b5d53f87f6947ca780fea50790b141c9ccc134e7a13f971121
2025-02-07T19:59:1
...

💬 hebasto commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#discussion_r1947142490)
Thanks! Reworked.

💬 hebasto commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947151056)
From CMake [docs](https://cmake.org/cmake/help/latest/command/set.html#set-cache-entry):
> Since cache entries are meant to provide user-settable values this does not overwrite existing cache entries by default.

It overrides only when `FORCE` is provided or the type is `INTERNAL`, which implies `FORCE`.

💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947178260)
Right. The variables this code operates on are `arith_uint256`, which represent integers in range $[0, 2^{256}-1]$, and whose division operator rounds down (whether that means towards $-\infty$ or towards 0 does not matter, since only non-negative numbers can be represented anyway).

However, these values reason about the *absolute value* of what `Div` is expected to do. If we want to round down (towards $-\infty$), but the value being divided is negative, that means the absolute value of the
...

💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947184910)
> As a side note, it's interesting that fuzz tests are used to test for program correctness where I thought the main objective for fuzz tests was to find crashes and panics.

They're an amazing tool for this purpose. Any time you write a well-contained, well-specified piece of code, you can essentially write a less efficient simulator for its behavior, and compare the two in a fuzz test. We have many such examples in the codebase.

💬 sipa commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2644123324)
Some chatter from IRC:

```
17:21:41 < darosior> It might be confusing to release both a bitcoin-wallet utility and a bitcoin-wallet binary as part of multiprocess?
17:24:08 < darosior> We could rename the utility, but then it would be nice to at least have one deprecation cycle. Given recent momentum i estimate it's possible we might release multiprocess in
30.0, which means if we want to deprecate the bitcoin-wallet utility name we should do it.. now?
17:33:31 < sip
...

👍 hodlinator approved a pull request: "test: expect that files may disappear from /proc/PID/fd/"
(https://github.com/bitcoin/bitcoin/pull/31614#pullrequestreview-2602844005)
ACK b2e9fdc00f5c40c241a37739f7b73b74c2181e39

### Concept

Since there is no trivial platform-independent way in Python to take a snapshot of the filesystem to my knowledge, suppressing exceptions stemming from file descriptors being closed right under our noses seems like the best solution.

### Testing

Modified [test reported to have failed on CI](https://github.com/bitcoin/bitcoin/pull/31614#issuecomment-2574971127) to provoke the issue.

```diff
diff --git a/test/functional/rpc_b
...

💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1947195581)
> `BITCOIN_BUILD_INTERNAL`: I'm a component of Core who's not building against the kernel at all, I just happen to be seeing this header.

`BITCOIN_INTERNAL_BUILD` was in my draft of the updated branch this morning :)

Will rework the PR shortly.

💬 sipa commented on pull request "test: expect that files may disappear from /proc/PID/fd/":
(https://github.com/bitcoin/bitcoin/pull/31614#issuecomment-2644148298)
utACK b2e9fdc00f5c40c241a37739f7b73b74c2181e39

💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1947210404)
Thanks! Reworked.

👍 hebasto approved a pull request: "build: consistently use `CLIENT_NAME` in libbitcoinkernel.pc.in"
(https://github.com/bitcoin/bitcoin/pull/31820#pullrequestreview-2602880373)
ACK f5b9a2f68c95182b139e8a3447b4b5888ecb4f9f.