💬 theuni commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643704920)
I didn't mean CXXFLAGS the env var, I meant CXXFLAGS the concept (as-in, what ultimately gets passed to the compiler). There are just some things that either we set internally or that CMake sets that are impossible to override without affecting something else (by using a None build type for example).`-Ox` is one of them.

So we provide these vars with guaranteed append semantics.

💬 theuni commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643708491)
Btw, if your argument is "`CXXFLAGS=-DDEBUG_LOCKCONTENTION` would work for the sake of this PR", I agree with you there.

💬 dedyshkaPexto commented on pull request "fixing-link-Update dependencies.md":
(https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2643712433)
> The old link successfully redirects me to the new one, what's the purpose of changing it?

view of the new link. The old one also still redirects.

💬 theuni commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#discussion_r1947001175)
Agreed. Mind addressing this @hebasto?

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1947016668)
done

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#issuecomment-2643744332)
> nit: I believe that CheckInvariants() could be outside of the loop in the txdownloadman_impl harness
the checks are now out of the loop, just like core devs

💬 theuni commented on pull request "cmake: Copy `cov_tool_wrapper.sh.in` to the build tree":
(https://github.com/bitcoin/bitcoin/pull/31722#discussion_r1947027251)
Could this not be relative to `${PROJECT_SOURCE_DIR}` instead to avoid the two-step copy/process?

💬 theuni commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947036941)
I think these should be tested for existence first (in case set by the user), and set to `INTERNAL` ?

💬 l0rinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947042506)
Modern C++ integer division always [truncates toward zero](https://en.cppreference.com/w/c/language/operator_arithmetic#Division). For positive numbers, that’s equivalent to rounding down, but for negatives it rounds up.
Since we need controlled rounding (toward negative infinity when `round_down` is true, or toward positive infinity otherwise), we need to take the sign of the numerator into account to adjust the result when necessary

For example, in [feefrac_tests.cpp](https://github.com/bi
...

💬 l0rinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947049915)
This is [always used now](https://github.com/bitcoin/bitcoin/commit/0606e66fdbb914f984433d8950f0c32b5fb871f3#diff-eca436ae09a12f8a59ac888c7f4ee6f36e4d12f38c2dab47fee0377ce69fe660R18), right?

⚠️ mzumsande opened an issue: "wallet: wrong balance and crash after reorg and unclean shutdown"
(https://github.com/bitcoin/bitcoin/issues/31824)
The wallet can show a wrong balance and/or hit an assert crash in case of a unclean shutdown after a reorg:

Initial state: Wallet and Node are synced to disk (e.g. a Flush just happened), the coinbase of the tip is a wallet tx.
1.) Tip is disconnected due to a reorg. This will change the status of affected wallet txns to Inactive (which will be synced with the wallet db immediately) - but neither the chainstate nor the wallet locator will be flushed.
2.) The node has an unclean shutdown for an
...

🤔 marcofleon reviewed a pull request: "TxOrphanage: account for size of orphans and count announcements"
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2602619218)
reACK e107bf78f9d722fcdeb5c1fba5a784dd7747e12f

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