💬 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
(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?
(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` ?
(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
...
(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?
(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
...
(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
(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.
(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
...
(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()`
...
(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
(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?
(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.
(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
(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
...
(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.
(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
...
(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.
(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`.
(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
...
(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
...