💬 willcl-ark commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2556778530)
OK so having thought about this a bit more, my preference would be to not actually change the docs (as I have them in this PR), but to amend _depends/host/default.mk_ to better respect when `host_CC= host_CXX= ` have been set. I also posit that a setting of `host_CC` should override `CC`.
For depends, if I run `make build_CC=clang build_CXX=clang++ host_CC=clang host_CXX=clang++` I expect to be able to build all host and native packages.
I also believe that if I run `make build_CC=clang bu
...
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2556778530)
OK so having thought about this a bit more, my preference would be to not actually change the docs (as I have them in this PR), but to amend _depends/host/default.mk_ to better respect when `host_CC= host_CXX= ` have been set. I also posit that a setting of `host_CC` should override `CC`.
For depends, if I run `make build_CC=clang build_CXX=clang++ host_CC=clang host_CXX=clang++` I expect to be able to build all host and native packages.
I also believe that if I run `make build_CC=clang bu
...
💬 ryanofsky commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#discussion_r2556822053)
In commit "depends: Fix native_capnp to respect build_CC and build_CXX" (eda7d38692ddb2a935ad0b2aefd2a3076c7aa599)
I'm surprised this wasn't happening previously. The depends system should already be setting CC and CXX values here:
https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/depends/funcs.mk#L215-L217
But I guess it is not setting them correctly? It would seem better for the depends system to set these correctly and not need package definitions to override
...
(https://github.com/bitcoin/bitcoin/pull/33937#discussion_r2556822053)
In commit "depends: Fix native_capnp to respect build_CC and build_CXX" (eda7d38692ddb2a935ad0b2aefd2a3076c7aa599)
I'm surprised this wasn't happening previously. The depends system should already be setting CC and CXX values here:
https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/depends/funcs.mk#L215-L217
But I guess it is not setting them correctly? It would seem better for the depends system to set these correctly and not need package definitions to override
...
💬 maflcko commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3571494725)
> The revert doesn't work for me on wayland (Qt v6.2.4 - Ubuntu 22.04.5 LTS), the window doesn't come back to front as it does when `plugin=xcb`.
The fix is in qt6.3, so this is expected. Your options are:
* Use a pre-compiled guix release
* Use depends yourself
* Upgrade your OS
* Install/compile a more recent qt yourself and use that
* ...?
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3571494725)
> The revert doesn't work for me on wayland (Qt v6.2.4 - Ubuntu 22.04.5 LTS), the window doesn't come back to front as it does when `plugin=xcb`.
The fix is in qt6.3, so this is expected. Your options are:
* Use a pre-compiled guix release
* Use depends yourself
* Upgrade your OS
* Install/compile a more recent qt yourself and use that
* ...?
💬 Sjors commented on pull request "mining: pass missing context to createNewBlock() and checkBlock()":
(https://github.com/bitcoin/bitcoin/pull/33936#issuecomment-3571507821)
Bumping the version number would also allow clients that wish to support older Bitcoin Core version to keep two capnp files around.
(https://github.com/bitcoin/bitcoin/pull/33936#issuecomment-3571507821)
Bumping the version number would also allow clients that wish to support older Bitcoin Core version to keep two capnp files around.
💬 willcl-ark commented on pull request "depends: sqlite 3.50.4; switch to autosetup":
(https://github.com/bitcoin/bitcoin/pull/32655#discussion_r2556842459)
3.51 was [released 04/11](https://sqlite.org/chronology.html) and claims to have some performance improvements:
> Performance enhancements:
>
> Use fewer CPU cycles to commit a read transaction.
> Early detection of joins that return no rows due to one or more of the tables containing no rows.
> Avoid evaluation of scalar subqueries if the result of the subquery does not change the result of the overall expression.
> Faster window function queries when using "BETWEEN :x
...
(https://github.com/bitcoin/bitcoin/pull/32655#discussion_r2556842459)
3.51 was [released 04/11](https://sqlite.org/chronology.html) and claims to have some performance improvements:
> Performance enhancements:
>
> Use fewer CPU cycles to commit a read transaction.
> Early detection of joins that return no rows due to one or more of the tables containing no rows.
> Avoid evaluation of scalar subqueries if the result of the subquery does not change the result of the overall expression.
> Faster window function queries when using "BETWEEN :x
...
💬 maflcko commented on pull request "ci: Use latest Xcode that the minimum macOS version allows":
(https://github.com/bitcoin/bitcoin/pull/33932#issuecomment-3571520280)
> This probably doesn't help [#29415 (comment)](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550717279), the `<=>` is not fixed in [16.2](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_2-release-notes) yet, only in [16.3](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes).
Correct. This diff here does not change any behavior of the Bitcoin Core CI, as explained in the description.
> Do we have to keep those or can w
...
(https://github.com/bitcoin/bitcoin/pull/33932#issuecomment-3571520280)
> This probably doesn't help [#29415 (comment)](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550717279), the `<=>` is not fixed in [16.2](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_2-release-notes) yet, only in [16.3](https://developer.apple.com/documentation/xcode-release-notes/xcode-16_3-release-notes).
Correct. This diff here does not change any behavior of the Bitcoin Core CI, as explained in the description.
> Do we have to keep those or can w
...
💬 fanquake commented on pull request "depends: sqlite 3.50.4; switch to autosetup":
(https://github.com/bitcoin/bitcoin/pull/32655#discussion_r2556857117)
I won't be doing that update here. We'd likely want to wait for a few more point releases before that update in any case.
(https://github.com/bitcoin/bitcoin/pull/32655#discussion_r2556857117)
I won't be doing that update here. We'd likely want to wait for a few more point releases before that update in any case.
👍 brunoerg approved a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3501159396)
ACK c34bc01b2ff2fc91ed4020288c5fa15f0c5b075e
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3501159396)
ACK c34bc01b2ff2fc91ed4020288c5fa15f0c5b075e
💬 pablomartin4btc commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3571589113)
> The fix is in qt6.3, so this is expected. Your options are:
> * Use depends yourself
I think I tried that before... I'll check it again, thanks!
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3571589113)
> The fix is in qt6.3, so this is expected. Your options are:
> * Use depends yourself
I think I tried that before... I'll check it again, thanks!
💬 m3dwards commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3571664130)
Not necessarily an issue with this PR but when trying to test on Trixie with `make -C depends HOST=x86_64-w64-mingw32ucrt -j9`
I get:
```shell
make: Entering directory '/bitcoin/depends'
Configuring boost...
-- The CXX compiler identification is GNU 14.0.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - failed
-- Check for working CXX compiler: /usr/bin/x86_64-w64-mingw32ucrt-g++
-- Check for working CXX compiler: /usr/bin/x86_64-w64-mingw32ucrt-g++ - broken
...
(https://github.com/bitcoin/bitcoin/pull/33857#issuecomment-3571664130)
Not necessarily an issue with this PR but when trying to test on Trixie with `make -C depends HOST=x86_64-w64-mingw32ucrt -j9`
I get:
```shell
make: Entering directory '/bitcoin/depends'
Configuring boost...
-- The CXX compiler identification is GNU 14.0.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - failed
-- Check for working CXX compiler: /usr/bin/x86_64-w64-mingw32ucrt-g++
-- Check for working CXX compiler: /usr/bin/x86_64-w64-mingw32ucrt-g++ - broken
...
💬 monlovesmango commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3571717964)
ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
It makes sense to ensure selected coins do not have more than 1 unconfirmed ancestor to prevent selecting coins that would create a package with size greater than 2.
> I think it would be reasonable to stop this transaction from being created and throw a "invalid parameters" error, telling the user they selected inputs that can't be spent due to policy.
Still been meaning to see if I can put some code together for this.
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3571717964)
ACK dcd42d6d8f160ae8bc12c152099a6e6473658e30
It makes sense to ensure selected coins do not have more than 1 unconfirmed ancestor to prevent selecting coins that would create a package with size greater than 2.
> I think it would be reasonable to stop this transaction from being created and throw a "invalid parameters" error, telling the user they selected inputs that can't be spent due to policy.
Still been meaning to see if I can put some code together for this.
💬 l0rinc commented on pull request "ci: Use latest Xcode that the minimum macOS version allows":
(https://github.com/bitcoin/bitcoin/pull/33932#issuecomment-3571747488)
ACK fa9537cde10120b12c96061cbc3f79a7680f9d64
(https://github.com/bitcoin/bitcoin/pull/33932#issuecomment-3571747488)
ACK fa9537cde10120b12c96061cbc3f79a7680f9d64
💬 psam21 commented on pull request "depends: Fix native_capnp to respect build_CC and build_CXX":
(https://github.com/bitcoin/bitcoin/pull/33937#discussion_r2557032045)
Right to point out that the _cmake macro already passes CC and CXX on the cmake command line at funcs.mk:215-217.
The issue is that CMake's initial compiler detection happens before it processes those command-line variables. When CMake runs its configure phase, it looks for CC/CXX in the environment first. The command-line arguments passed by _cmake aren't set as environment variables, so CMake falls back to system defaults (gcc), which is what issue #33859 reported.
The config_env variabl
...
(https://github.com/bitcoin/bitcoin/pull/33937#discussion_r2557032045)
Right to point out that the _cmake macro already passes CC and CXX on the cmake command line at funcs.mk:215-217.
The issue is that CMake's initial compiler detection happens before it processes those command-line variables. When CMake runs its configure phase, it looks for CC/CXX in the environment first. The command-line arguments passed by _cmake aren't set as environment variables, so CMake falls back to system defaults (gcc), which is what issue #33859 reported.
The config_env variabl
...
💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3571793549)
I've been tracking the non-mempool transaction memory footprint for an hour now on mainnet, using fairly aggressive template update criteria (minimum fee delta 1 sat and no more than once per second). So far the footprint is minuscule, but of course this depends on the mempool weather:
<img width="1344" height="685" alt="getmemoryload-scatter" src="https://github.com/user-
attachments/assets/f46f2ca6-84c2-4571-9026-64428f1531ff" />
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3571793549)
I've been tracking the non-mempool transaction memory footprint for an hour now on mainnet, using fairly aggressive template update criteria (minimum fee delta 1 sat and no more than once per second). So far the footprint is minuscule, but of course this depends on the mempool weather:
<img width="1344" height="685" alt="getmemoryload-scatter" src="https://github.com/user-
attachments/assets/f46f2ca6-84c2-4571-9026-64428f1531ff" />
💬 brunoerg commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#issuecomment-3571972044)
reACK 8343a9ffcc752f77eb2248315d10b6dff4a5c98b
From my last review, it addresses https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2543670655.
(https://github.com/bitcoin/bitcoin/pull/33893#issuecomment-3571972044)
reACK 8343a9ffcc752f77eb2248315d10b6dff4a5c98b
From my last review, it addresses https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2543670655.
💬 plebhash commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3572028327)
any chance this could get backported?
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3572028327)
any chance this could get backported?
💬 fjahr commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557221072)
I guess that might be more informative but the comment is just removed now
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557221072)
I guess that might be more informative but the comment is just removed now
💬 fjahr commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557221500)
Fixed
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557221500)
Fixed
💬 fjahr commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557221903)
Done
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557221903)
Done
💬 fjahr commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557226776)
Yeah, that looks very good to me, I took this almost exactly as is, squashed the `NUM_WALLETS` in there because these lines are now touched as well and made you co-author of that commit. Thanks a lot!
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2557226776)
Yeah, that looks very good to me, I took this almost exactly as is, squashed the `NUM_WALLETS` in there because these lines are now touched as well and made you co-author of that commit. Thanks a lot!