Bitcoin Core Github
43 subscribers
122K links
Download Telegram
💬 hebasto commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3488404139)
There are no conflicts with other contributors' PRs.

Friendly ping @l0rinc @maflcko @ryanofsky @willcl-ark who reviewed https://github.com/bitcoin/bitcoin/pull/31308, and @TheCharlatan, the kernel expert :)
🤔 l0rinc requested changes to a pull request: "test: Enhance GetTxSigOpCost tests for coinbase transactions"
(https://github.com/bitcoin/bitcoin/pull/32840#pullrequestreview-3418794264)
I have gone through the cases, I think we should take this opportunity and unify the test to use BOOST_ checkers for better error messages, to split the big test into smaller self-contained tests (otherwise the first failure will break the remaining ones - though this will result in some setup-repetition, but we can add helpers for those).

I have applied the cleanup that I would like to see here, if you decide to accept any of it, please do it in multiple focused commits.

<details>
<summary>De
...
💬 l0rinc commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2492222322)
this is already a coinbase tx, basically equivalent to the above, let's put this before mutating the spending tx, it's not related to it
💬 l0rinc commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2492225315)
After we make this a coinbase, it's confusing to refer to it as a "spending" transaction...
We could add another helper that copies it and makes it a coinbase
💬 l0rinc commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2492179386)
it seems to me `scriptWitness` is actually empty here, so the comment and code are a bit confusing here.
I think a `BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull());` would document the requirements here better than a comment.
`BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), coins, flags), 0);` already checks that coinbases don't have sigops
💬 l0rinc commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2492229011)
I know you didn't introduce them, but these are basically self-contained tests that could be split out to separate `BOOST_AUTO_TEST_CASE`
* GetSigOpCount
* GetTxSigOpCost_Multisig
* GetTxSigOpCost_MultisigP2SH
* GetTxSigOpCost_P2WPKH
* GetTxSigOpCost_P2WPKH_P2SH
* GetTxSigOpCost_P2WSH
* GetTxSigOpCost_P2WSH_P2SH
💬 l0rinc commented on pull request "ci, iwyu: Fix warnings in `src/kernel` and treat them as errors":
(https://github.com/bitcoin/bitcoin/pull/33779#issuecomment-3488414728)
code review ACK a8a33bc0c0a11093418debc36db8ac63bf90e687
👍 hebasto approved a pull request: "guix: disable libsanitizer in Linux GCC build"
(https://github.com/bitcoin/bitcoin/pull/33780#pullrequestreview-3419058547)
ACK 5c41fa2918c8fee36d0e0375e753249f1efa7c07.

My Guix build:
```
x86_64
db958b4fc80e95686eca42fdba140aa3e150c003d5bdef6e3ea34872cd674c8a guix-build-5c41fa2918c8/output/aarch64-linux-gnu/SHA256SUMS.part
1f70bdae06dc815d12821b2d7ebb17bdb0b1ca84e775ce7c1bafdbb89d745d59 guix-build-5c41fa2918c8/output/aarch64-linux-gnu/bitcoin-5c41fa2918c8-aarch64-linux-gnu-debug.tar.gz
f0cb17846e91bef73901a0f9de4614f31b9881ac445370aa5183ff8a5caa1a35 guix-build-5c41fa2918c8/output/aarch64-linux-gnu/bitcoin-5c41fa
...
📝 kwsantiago opened a pull request: "build: Upgrade depends to Qt6"
(https://github.com/bitcoin/bitcoin/pull/33787)
Upgrades the depends system to build Qt 6.7.3, preparing the codebase for Qt6 compatibility.

## Changes

- Add Qt6 (6.7.3) package definitions and patches
- Update Qt build configuration to support both Qt5 and Qt6
- Add automatic Qt version detection in depends toolchain
- Add libxcb_util_cursor package for Qt6 Linux builds
- Remove obsolete Qt5-specific patches
- Add initial `wintaskbar` infrastructure for Qt6 compatibility, which is covered in https://github.com/bitcoinknots/bitcoin
...
📝 luke-jr opened a pull request: "[29.x] Backport fixes for CVE-2025 46598"
(https://github.com/bitcoin/bitcoin/pull/33788)
💬 luke-jr commented on pull request "[29.x] Backport fixes for CVE-2025 46598":
(https://github.com/bitcoin/bitcoin/pull/33788#discussion_r2492518838)
Note: Not in master commit (another approach is used for this test)
💬 luke-jr commented on pull request "[29.x] Backport fixes for CVE-2025 46598":
(https://github.com/bitcoin/bitcoin/pull/33788#discussion_r2492519413)
Note: reject_reason could be None here, so a check was added (not in master commit)
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3488760623)
Benchmarked the latest up to block 921129 and it's 16% faster :rocket:. Not as fast as some of @l0rinc's numbers but it's on a laptop with an internal NVMe SSD. This change will see the most benefit for disk IO with higher latency, like network connected storage.

| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `echo d606c36a13ca2a055d1a4eb4c623fb6aa45405b2 && /usr/bin/time ./build/bin/bitcoind -printtoconsole=0 -connect=192.168.2.171 -stopatheight=921129`
...
💬 dongcarl commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3489173537)
What a milestone. Congrats @TheCharlatan!
🤔 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
💬 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
...
🤔 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
💬 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?
📝 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)