🤔 trevarj reviewed a pull request: "guix: Use UCRT runtime for Windows release binaries"
(https://github.com/bitcoin/bitcoin/pull/33593#pullrequestreview-3419999713)
@hebasto thanks for working in my suggestion!
ACK dbb8359
(https://github.com/bitcoin/bitcoin/pull/33593#pullrequestreview-3419999713)
@hebasto thanks for working in my suggestion!
ACK dbb8359
💬 purpleKarrot commented on pull request "depends: add zeromq patch to fix mingw CMake file install location":
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3489804291)
> The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
>
> UPD. ... on Windows, but not when cross-compiling.
When taking cross-compiling into account, there are not two, but three scenarios how mingw can be used:
1. Building on Windows
2. Building on Linux, but packaging for Windows
3. Building on and packaging for Linux
CMake searches for packages in different paths depending on the **build hos
...
(https://github.com/bitcoin/bitcoin/pull/33778#issuecomment-3489804291)
> The current path is valid per the CMake [docs](https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure).
>
> UPD. ... on Windows, but not when cross-compiling.
When taking cross-compiling into account, there are not two, but three scenarios how mingw can be used:
1. Building on Windows
2. Building on Linux, but packaging for Windows
3. Building on and packaging for Linux
CMake searches for packages in different paths depending on the **build hos
...
🤔 maflcko reviewed a pull request: "guix: use GCC 14.3.0 over 13.3.0"
(https://github.com/bitcoin/bitcoin/pull/33775#pullrequestreview-3420453767)
concept ack. 14.3 should be sufficiently propagated and tested by now, to rely on it for release builds
(https://github.com/bitcoin/bitcoin/pull/33775#pullrequestreview-3420453767)
concept ack. 14.3 should be sufficiently propagated and tested by now, to rely on it for release builds
💬 maflcko commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2493421654)
url is wrong, and the `maybe-uninitialized` below can be removed?
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2493421654)
url is wrong, and the `maybe-uninitialized` below can be removed?
📝 frankomosh opened a pull request: "doc: add cmake help option in Windows build docs"
(https://github.com/bitcoin/bitcoin/pull/33789)
Follow-up to #33088.
Adds `cmake -B build -LH` documentation to Windows build guides, similar to Unix build documentation.
Based on the suggestion and example provided by @stickies-v in #33088, with minor adjustment to match existing indented code block format in `build-windows.md`.
Tested for:
- WSL Ubuntu with mingw-w64 cross-compilation
- Windows 11 with Visual Studio 2022 (MSVC)
(https://github.com/bitcoin/bitcoin/pull/33789)
Follow-up to #33088.
Adds `cmake -B build -LH` documentation to Windows build guides, similar to Unix build documentation.
Based on the suggestion and example provided by @stickies-v in #33088, with minor adjustment to match existing indented code block format in `build-windows.md`.
Tested for:
- WSL Ubuntu with mingw-w64 cross-compilation
- Windows 11 with Visual Studio 2022 (MSVC)
💬 yuvicc commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3489978539)
ACK a9ec7cdc0a1f6d4a3bd532649e79459074ed9f3b
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3489978539)
ACK a9ec7cdc0a1f6d4a3bd532649e79459074ed9f3b
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3490078075)
> The latest https://corecheck.dev/bitcoin/bitcoin/pulls/31713 Sonar warnings seem like false positives.
Took a closer look at https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&branch=31713-a1df8547ca0711d99f05d1b31bc758a8068ba512&id=aureleoules_bitcoin&open=AZpLftZOTlsoQg18lt0A
`miniscript::Node` on master and in the PR violates the "Rule of Zero" (https://sonarcloud.io/organizations/aureleoules/rules?open=cpp%3AS3624&rule_key=cpp%3AS3624&tab=why) where if one has a cust
...
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3490078075)
> The latest https://corecheck.dev/bitcoin/bitcoin/pulls/31713 Sonar warnings seem like false positives.
Took a closer look at https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&branch=31713-a1df8547ca0711d99f05d1b31bc758a8068ba512&id=aureleoules_bitcoin&open=AZpLftZOTlsoQg18lt0A
`miniscript::Node` on master and in the PR violates the "Rule of Zero" (https://sonarcloud.io/organizations/aureleoules/rules?open=cpp%3AS3624&rule_key=cpp%3AS3624&tab=why) where if one has a cust
...
💬 janb84 commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3490171034)
should `src/test/kernel` also be moved to `src/kernel/test` to keep the project structure Consistent. ?
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3490171034)
should `src/test/kernel` also be moved to `src/kernel/test` to keep the project structure Consistent. ?
👍 willcl-ark approved a pull request: "guix: disable libsanitizer in Linux GCC build"
(https://github.com/bitcoin/bitcoin/pull/33780#pullrequestreview-3420936380)
utACK 5c41fa2918c8fee36d0e0375e753249f1efa7c07
(https://github.com/bitcoin/bitcoin/pull/33780#pullrequestreview-3420936380)
utACK 5c41fa2918c8fee36d0e0375e753249f1efa7c07
💬 fanquake commented on pull request "Modernize custom filtering":
(https://github.com/bitcoin-core/gui/pull/899#issuecomment-3490255856)
Backported to `30.x` in https://github.com/bitcoin/bitcoin/pull/33609
(https://github.com/bitcoin-core/gui/pull/899#issuecomment-3490255856)
Backported to `30.x` in https://github.com/bitcoin/bitcoin/pull/33609
🚀 fanquake merged a pull request: "guix: disable libsanitizer in Linux GCC build"
(https://github.com/bitcoin/bitcoin/pull/33780)
(https://github.com/bitcoin/bitcoin/pull/33780)
👍 rkrux approved a pull request: "test: remove obsolete `get_{key,multisig}` helpers from wallet_util.py"
(https://github.com/bitcoin/bitcoin/pull/33782#pullrequestreview-3420985296)
crACK ec8516ceb7568d7b09836b830023978bd37f8462
(https://github.com/bitcoin/bitcoin/pull/33782#pullrequestreview-3420985296)
crACK ec8516ceb7568d7b09836b830023978bd37f8462
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2493783865)
I had a f2f discussion with somebody (forgot who?) of what happens if a transaction is submitted for private broadcast and is:
1. previously submitted but not yet in the mempool
2. in the mempool (and thus not in the for-private-broadcast list)
This part of the test answers 1. - if a transaction is already in the internal for-private-broadcast list then `sendrawtransaction` will error with "Ignoring unnecessary request...".
"2." is not exercised in the test, but what will happen is tha
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2493783865)
I had a f2f discussion with somebody (forgot who?) of what happens if a transaction is submitted for private broadcast and is:
1. previously submitted but not yet in the mempool
2. in the mempool (and thus not in the for-private-broadcast list)
This part of the test answers 1. - if a transaction is already in the internal for-private-broadcast list then `sendrawtransaction` will error with "Ignoring unnecessary request...".
"2." is not exercised in the test, but what will happen is tha
...
🚀 fanquake merged a pull request: "clang-tidy: Remove no longer needed NOLINT"
(https://github.com/bitcoin/bitcoin/pull/33781)
(https://github.com/bitcoin/bitcoin/pull/33781)
💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2493801897)
> and the maybe-uninitialized below can be removed?
Fixed the URL, I don't think we can drop it yet: https://github.com/bitcoin/bitcoin/actions/runs/19074211079/job/54485090675?pr=33775#step:8:3723.
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2493801897)
> and the maybe-uninitialized below can be removed?
Fixed the URL, I don't think we can drop it yet: https://github.com/bitcoin/bitcoin/actions/runs/19074211079/job/54485090675?pr=33775#step:8:3723.
💬 hebasto commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3490333396)
> should `src/test/kernel` also be moved to `src/kernel/test` to keep the project structure Consistent. ?
Maybe.
Moving IPC tests provides additional benefits. I'm not sure the same justification applies to the kernel tests, so let's leave them for a follow-up.
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3490333396)
> should `src/test/kernel` also be moved to `src/kernel/test` to keep the project structure Consistent. ?
Maybe.
Moving IPC tests provides additional benefits. I'm not sure the same justification applies to the kernel tests, so let's leave them for a follow-up.
👍 TheCharlatan approved a pull request: "test: Add bitcoin-chainstate test for assumeutxo functionality"
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3421048286)
re-ACK 67740dfe6cfe3f3f91aaad41018a7da9004dcd8c
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3421048286)
re-ACK 67740dfe6cfe3f3f91aaad41018a7da9004dcd8c
👍 willcl-ark approved a pull request: "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors"
(https://github.com/bitcoin/bitcoin/pull/33779#pullrequestreview-3421197426)
ACK a8a33bc0c0a11093418debc36db8ac63bf90e687
Indeed a good time to get this change in :)
(https://github.com/bitcoin/bitcoin/pull/33779#pullrequestreview-3421197426)
ACK a8a33bc0c0a11093418debc36db8ac63bf90e687
Indeed a good time to get this change in :)
💬 maflcko commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3490481643)
lgtm ACK 25ede968badadfeeaaa8358b38c9a8cfff7189f0
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3490481643)
lgtm ACK 25ede968badadfeeaaa8358b38c9a8cfff7189f0
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2493940857)
> In this implementation the reserved memory is kept in between blocks
K, so let's reserve outside and clear inside.
Now that missing values aren't failures, we can experiment with shortids - since a missing value isn't a failure anymore (even though I wouldn't expect any collisions in 64 bits either, assuming uniform distribution. But even if the distribution isn't uniform, we can likely store it safely).
64 bit ids for internal spends would mean that in case of some collision we will atte
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2493940857)
> In this implementation the reserved memory is kept in between blocks
K, so let's reserve outside and clear inside.
Now that missing values aren't failures, we can experiment with shortids - since a missing value isn't a failure anymore (even though I wouldn't expect any collisions in 64 bits either, assuming uniform distribution. But even if the distribution isn't uniform, we can likely store it safely).
64 bit ids for internal spends would mean that in case of some collision we will atte
...