💬 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
...
💬 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.
(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
...
(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
...
(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.
(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
(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.
(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.
(https://github.com/bitcoin/bitcoin/pull/31820#pullrequestreview-2602880373)
ACK f5b9a2f68c95182b139e8a3447b4b5888ecb4f9f.
💬 theuni commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947223395)
Right, my comment was about trying to keep them out of the gui menus.
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947223395)
Right, my comment was about trying to keep them out of the gui menus.
💬 hebasto commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947236719)
Ah, I see. In that case, wouldn't me be easier and consistent with the other code to mark these both variables as [advanced](https://cmake.org/cmake/help/latest/command/mark_as_advanced.html)?
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947236719)
Ah, I see. In that case, wouldn't me be easier and consistent with the other code to mark these both variables as [advanced](https://cmake.org/cmake/help/latest/command/mark_as_advanced.html)?
🤔 sipa reviewed a pull request: "TxOrphanage: account for size of orphans and count announcements"
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2602898410)
utACK e107bf78f9d722fcdeb5c1fba5a784dd7747e12f
Just nits.
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2602898410)
utACK e107bf78f9d722fcdeb5c1fba5a784dd7747e12f
Just nits.
💬 sipa commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1947250869)
If we want to (eventually) call this function in functional tests or so, outside of fuzz tests, using `Assert()` is better than `Assume()` (as the the latter won't do anything outside of test builds).
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1947250869)
If we want to (eventually) call this function in functional tests or so, outside of fuzz tests, using `Assert()` is better than `Assume()` (as the the latter won't do anything outside of test builds).
💬 sipa commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1947241463)
Similar comment as for the global usage account: if that becomes a 64-bit integer, it may be best to do the same here (because when the global limit is not reached, nothing will prevent a single peer from consuming the full global limit).
Similarly, can be done in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1947241463)
Similar comment as for the global usage account: if that becomes a 64-bit integer, it may be best to do the same here (because when the global limit is not reached, nothing will prevent a single peer from consuming the full global limit).
Similarly, can be done in a follow-up.
💬 sipa commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1947225159)
If we permit 4MWu per-peer reservation, and have a 1000 peers, we're not very far away from the `unsigned int` limit $2^{32}-1$. We were thinking about using only 1/10 of that for inbound, so we're really still a factor 10x away, but that still feels close enough that perhaps this should be an `int64_t` instead.
(can be done as a follow-up, since this isn't a concern with the current orphan eviction policies)
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1947225159)
If we permit 4MWu per-peer reservation, and have a 1000 peers, we're not very far away from the `unsigned int` limit $2^{32}-1$. We were thinking about using only 1/10 of that for inbound, so we're really still a factor 10x away, but that still feels close enough that perhaps this should be an `int64_t` instead.
(can be done as a follow-up, since this isn't a concern with the current orphan eviction policies)
💬 theuni commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947251136)
Whoops, of course! That's what I meant to suggest, not Internal. Advanced please :)
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947251136)
Whoops, of course! That's what I meant to suggest, not Internal. Advanced please :)
💬 fjahr commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2644292890)
Yeah, I'm using macOS Sequoia 15.3 and I am also using clang but I have already been using `GCOV_EXECUTABLE` from the start.
Checking some of the other versions:
```
$ gcovr --version | head -n1
gcovr 8.3
$ clang --version | head -n1
Homebrew clang version 19.1.6
$ bash --version
GNU bash, version 5.2.37(1)-release (aarch64-apple-darwin24.0.0)
```
The built-in wc doesn't seem to provide version information but it is supposed to be taken from BSD tools.
I tried compiling with G
...
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2644292890)
Yeah, I'm using macOS Sequoia 15.3 and I am also using clang but I have already been using `GCOV_EXECUTABLE` from the start.
Checking some of the other versions:
```
$ gcovr --version | head -n1
gcovr 8.3
$ clang --version | head -n1
Homebrew clang version 19.1.6
$ bash --version
GNU bash, version 5.2.37(1)-release (aarch64-apple-darwin24.0.0)
```
The built-in wc doesn't seem to provide version information but it is supposed to be taken from BSD tools.
I tried compiling with G
...
🤔 mzumsande reviewed a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2600169358)
Concept ACK
I verified that this would solve the issues from #31824, only looked at the first couple commits in detail so far.
With respect to the above discussion, I think that the concept of delayed flushing only makes sense if it applies to all relevant data for a given block connection /disconnection (which applies to the indexes, for example). But when other changes to wallet txns are synced to disk immediately, it seems wrong to delay flushing the locator - this will lead to a mixed
...
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2600169358)
Concept ACK
I verified that this would solve the issues from #31824, only looked at the first couple commits in detail so far.
With respect to the above discussion, I think that the concept of delayed flushing only makes sense if it applies to all relevant data for a given block connection /disconnection (which applies to the indexes, for example). But when other changes to wallet txns are synced to disk immediately, it seems wrong to delay flushing the locator - this will lead to a mixed
...