💬 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)
💬 luke-jr commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1947409010)
That's not guaranteed. Adding this is technically incorrect, and doesn't provide any real information, just makes it more complex.
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1947409010)
That's not guaranteed. Adding this is technically incorrect, and doesn't provide any real information, just makes it more complex.
💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2644456448)
> Given that CMake lacks any native functionality to acheive the same thing, using `-DWERROR` seems ok, especially if the alternative is implement `N` more build options (now/in future) with varying defaults for everything that may warn and needs to be caught.
> I don't like the idea of overloading the `WERROR` build option with additional functionality, as this is not what users would expect.
> As pointed in [#31724 (comment)](https://github.com/bitcoin/bitcoin/pull/31724#issuecomment-261
...
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2644456448)
> Given that CMake lacks any native functionality to acheive the same thing, using `-DWERROR` seems ok, especially if the alternative is implement `N` more build options (now/in future) with varying defaults for everything that may warn and needs to be caught.
> I don't like the idea of overloading the `WERROR` build option with additional functionality, as this is not what users would expect.
> As pointed in [#31724 (comment)](https://github.com/bitcoin/bitcoin/pull/31724#issuecomment-261
...
👍 luke-jr approved a pull request: "build: consistently use `CLIENT_NAME` in libbitcoinkernel.pc.in"
(https://github.com/bitcoin/bitcoin/pull/31820#pullrequestreview-2603255184)
utACK
(https://github.com/bitcoin/bitcoin/pull/31820#pullrequestreview-2603255184)
utACK
💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1947457946)
Can you please clarify your comment -- what isn't correct?
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1947457946)
Can you please clarify your comment -- what isn't correct?
✅ luke-jr closed a pull request: "Translate unit names & fix external signer error"
(https://github.com/bitcoin-core/gui/pull/599)
(https://github.com/bitcoin-core/gui/pull/599)
💬 hebasto commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947496645)
Thanks! Amended.
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947496645)
Thanks! Amended.