💬 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.
💬 davidgumberg commented on pull request "build: consistently use `CLIENT_NAME` in libbitcoinkernel.pc.in":
(https://github.com/bitcoin/bitcoin/pull/31820#issuecomment-2644549805)
utACK https://github.com/bitcoin/bitcoin/pull/31820/commits/f5b9a2f68c95182b139e8a3447b4b5888ecb4f9f
(https://github.com/bitcoin/bitcoin/pull/31820#issuecomment-2644549805)
utACK https://github.com/bitcoin/bitcoin/pull/31820/commits/f5b9a2f68c95182b139e8a3447b4b5888ecb4f9f
💬 hebasto commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2644560045)
Could the `CAPNP_EXECUTABLE`, `CAPNPC_CXX_EXECUTABLE` and all `CapnProto_*` CMake variables be marked as advanced to avoid cluttering the CMake cache view when inspected by the user?
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2644560045)
Could the `CAPNP_EXECUTABLE`, `CAPNPC_CXX_EXECUTABLE` and all `CapnProto_*` CMake variables be marked as advanced to avoid cluttering the CMake cache view when inspected by the user?
💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2644725633)
> > Did anyone test this on Windows?
>
> Of course would be nice to test this on windows but it should work there according to cmake release notes: https://cmake.org/cmake/help/v3.13/release/3.13.html?highlight=release%20notes#command-line
The success of the `cmake -E create_symlink` command on Windows depends on the the "Create symbolic links" privilege ([`SeCreateSymbolicLinkPrivilege`](https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-10/security/threat-protecti
...
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2644725633)
> > Did anyone test this on Windows?
>
> Of course would be nice to test this on windows but it should work there according to cmake release notes: https://cmake.org/cmake/help/v3.13/release/3.13.html?highlight=release%20notes#command-line
The success of the `cmake -E create_symlink` command on Windows depends on the the "Create symbolic links" privilege ([`SeCreateSymbolicLinkPrivilege`](https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-10/security/threat-protecti
...
💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2644734444)
Additionally, 433aa99d238740bbdcb07b53b17f639170bd326d does not work for any multi-config generators.
For example:
```
$ cmake -B build -G "Ninja Multi-Config"
$ cmake --build build
$ file build/src/bitcoind
build/src/bitcoind: broken symbolic link to /home/hebasto/git/bitcoin/build/bin/bitcoind
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2644734444)
Additionally, 433aa99d238740bbdcb07b53b17f639170bd326d does not work for any multi-config generators.
For example:
```
$ cmake -B build -G "Ninja Multi-Config"
$ cmake --build build
$ file build/src/bitcoind
build/src/bitcoind: broken symbolic link to /home/hebasto/git/bitcoin/build/bin/bitcoind
📝 bhandariarun opened a pull request: "Bitcoin"
(https://github.com/bitcoin/bitcoin/pull/31825)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31825)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ hebasto closed a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/31825)
(https://github.com/bitcoin/bitcoin/pull/31825)
📝 hebasto locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/31825)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31825)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 hebasto commented on pull request "cmake: Copy `cov_tool_wrapper.sh.in` to the build tree":
(https://github.com/bitcoin/bitcoin/pull/31722#discussion_r1947553413)
With the current design, the required `gcov` or `llvm-cov` tools are searched during the `Coverage{Fuzz}.cmake` script invocation. This two-step processing could potentially be avoided by moving these tool checks into the main buildsystem script. However, there is no reliable way to gate them depending on whether the build configuration is 'Coverage' or not.
(https://github.com/bitcoin/bitcoin/pull/31722#discussion_r1947553413)
With the current design, the required `gcov` or `llvm-cov` tools are searched during the `Coverage{Fuzz}.cmake` script invocation. This two-step processing could potentially be avoided by moving these tool checks into the main buildsystem script. However, there is no reliable way to gate them depending on whether the build configuration is 'Coverage' or not.
💬 hebasto commented on pull request "guix: remove test-security/symbol-check scripts":
(https://github.com/bitcoin/bitcoin/pull/31818#issuecomment-2644785548)
> In the pre-Guix days, these were valuable to sanity-check the environment, because we were pulling that pre-built from Ubuntu, with little control. At this point, it's less clear what these scripts are (sanity) checking.
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31818#issuecomment-2644785548)
> In the pre-Guix days, these were valuable to sanity-check the environment, because we were pulling that pre-built from Ubuntu, with little control. At this point, it's less clear what these scripts are (sanity) checking.
Concept ACK.