💬 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
...
💬 mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r1945560499)
> this is okay even if there is an unclean shutdown since reloading the wallet will still rescan this block
I don't think that this is the case. We will be temporarily in a state in which the wallet thinks it has scanned the block (locator is written to disk) but haven't actually done the `SyncTransaction` work yet. This is fragile, because in case of an unclean shutdown a future rescan may not include this block if it thinks, based on the locator, that the block has already been scanned.
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r1945560499)
> this is okay even if there is an unclean shutdown since reloading the wallet will still rescan this block
I don't think that this is the case. We will be temporarily in a state in which the wallet thinks it has scanned the block (locator is written to disk) but haven't actually done the `SyncTransaction` work yet. This is fragile, because in case of an unclean shutdown a future rescan may not include this block if it thinks, based on the locator, that the block has already been scanned.
...
💬 mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r1947184556)
To deal with unclean shutdowns, during disconnect the reverse order compared to blockConnected would be better:
If the locator is updated first, an unclean shutdown will lead to a rescan of the disconnected block (which might do nothing but correct the locator).
But if txns are set to Inactive first and have an unclean shutdown after that, the transactions will be missing from the wallet / balance.
As mentioned above, doing it all in one batch would resolve this in a more elegant way.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r1947184556)
To deal with unclean shutdowns, during disconnect the reverse order compared to blockConnected would be better:
If the locator is updated first, an unclean shutdown will lead to a rescan of the disconnected block (which might do nothing but correct the locator).
But if txns are set to Inactive first and have an unclean shutdown after that, the transactions will be missing from the wallet / balance.
As mentioned above, doing it all in one batch would resolve this in a more elegant way.
💬 yancyribbens commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947389283)
> 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.
However property tests, which like fuzz tests generate random input, although the inputs are truly random, which may be desirable for testing program correctness. From what I understand about fuzz tests, the random inputs are generate
...
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947389283)
> 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.
However property tests, which like fuzz tests generate random input, although the inputs are truly random, which may be desirable for testing program correctness. From what I understand about fuzz tests, the random inputs are generate
...
💬 davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2644420333)
> I'm wondering if a minimum-diff fix which simply replaces the `memset` calls in the dtors with `memory_cleanse` would be largely sufficient here? In [#31166 (comment)](https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2446055905) one argument for not needing secure allocators was the short-lived nature of the secrets.
It seems right to me that this structure is always short lived, but I also felt in #31166 `secure_allocator` should have been used over just `memory_cleanse()`. I fe
...
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2644420333)
> I'm wondering if a minimum-diff fix which simply replaces the `memset` calls in the dtors with `memory_cleanse` would be largely sufficient here? In [#31166 (comment)](https://github.com/bitcoin/bitcoin/pull/31166#issuecomment-2446055905) one argument for not needing secure allocators was the short-lived nature of the secrets.
It seems right to me that this structure is always short lived, but I also felt in #31166 `secure_allocator` should have been used over just `memory_cleanse()`. I fe
...
👋 davidgumberg's pull request is ready for review: "build: Make config warnings fatal if -DWERROR=ON"
(https://github.com/bitcoin/bitcoin/pull/31665)
(https://github.com/bitcoin/bitcoin/pull/31665)