Bitcoin Core Github
44 subscribers
122K links
Download Telegram
👍 hebasto approved a pull request: "ci: remove 3rd party js from windows dll gha job"
(https://github.com/bitcoin/bitcoin/pull/32513#pullrequestreview-2877956479)
ACK ff2e35f6315cc6b912f68fe85103575d751bd2bc.

While touching this code, it seems reasonable to also address the other @davidgumberg's [comment](https://github.com/bitcoin/bitcoin/pull/32634#issuecomment-2918081325):
> Might be nice to have a CI check like [davidgumberg@1b98160](https://github.com/davidgumberg/bitcoin/commit/1b9816052b1d8ad1d9c17945530e14ead79fce33) to prevent future regressions, but OK for a separate PR if out of scope.
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2919114726)
> While this approach introduces some duplication at present, it will ultimately enable us to consolidate how the IWYU tool is executed, allowing for a choice between using the iwyu_tool.py script or CMake's built-in functionality.

Reading the PR description, it's not really clear why we'd need to retain using both tools? Either the CMake built-in should be better in some way, or at least equivalent in functionality, in which case we should just switch to using it?
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2113759162)
Is there an issue open for this upstream?
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2113759342)
Same here?
💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2919117368)
> While touching this code, it seems reasonable to also address the other @davidgumberg's [comment](https://github.com/bitcoin/bitcoin/pull/32634#issuecomment-2918081325):

I don't mind adding that.
👍 hodlinator approved a pull request: "windows: Use predefined `RC_INVOKED` macro instead of custom one"
(https://github.com/bitcoin/bitcoin/pull/32633#pullrequestreview-2877972468)
re-ACK 55f1c2ac8beb5ebebcaef0707d8cc3003e41d80e

Built x86_64-w64-mingw32 Guix build locally.
💬 hodlinator commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2919144295)
I suspect checking for manifests will currently fail for bench_bitcoin.exe as it doesn't yet have a manifest. Not sure about other binaries. Wouldn't be against adding missing manifests unless it slows down the builds noticeably.
💬 fanquake commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2919196828)
> I believe there is a regression in parsing big endian PPC64 headers in LIEF: https://github.com/lief-project/LIEF/issues/1217, but I've added a work-around to contrib/guix/security-check.py for this.

You should be able to pull in https://github.com/lief-project/LIEF/commit/a1ec0f815fb7ddefc8b53af9f7bb990884655964.
👍 TheCharlatan approved a pull request: "fuzz: Add target for coins database"
(https://github.com/bitcoin/bitcoin/pull/32602#pullrequestreview-2878087402)
ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37
📝 fanquake opened a pull request: "[28.x] Backport guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32639)
See https://guix.gnu.org/blog/2025/migrating-to-codeberg/.

When interacting with the old repo you may now also see:
```bash
warning: redirecting to https://codeberg.org/guix/guix/
```

Github-Pull: #32439
Rebased-From: c8d9baae942c94d64ce47ae8f67d3710e6a296bd
💬 fanquake commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2919256360)
Backported to 28.x in #32639.
💬 hebasto commented on pull request "Replace dead gnome link notificator.cpp":
(https://github.com/bitcoin/bitcoin/pull/32629#discussion_r2113852140)
> Agreed, the link doesn't directly help understand the code Should I remove it?

I think so.
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2113855067)
I hadn't considered the extra vector allocation, yeah I'm okay leaving this as-is then.

I did nerd-snipe myself into looking into having `AccessCoins` return a view would be quite elegant in some ways, allowing lazy evaluation, but seems like it would require quite a bit of overhaul and might have other drawbacks I've not considered yet.

```cpp
auto AccessCoins(const CTransaction& tx) const
{
return tx.vin | std::views::transform([this](const CTxIn& input) -> const Coin
...
👍 hebasto approved a pull request: "[28.x] Backport guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32639#pullrequestreview-2878132664)
ACK 5c2ba9f583e28460e618cc1119a10c06b868377c.

> When interacting with the old repo you may now also see:
>
> ```shell
> warning: redirecting to https://codeberg.org/guix/guix/
> ```

I also observed random connection issues.
💬 hebasto commented on pull request "windows: Use predefined `RC_INVOKED` macro instead of custom one":
(https://github.com/bitcoin/bitcoin/pull/32633#issuecomment-2919271405)
My Guix build:
```
aarch64
90c59c671509f759cc862b4376b374ff36707a323eda619fed3f91cf322c0de5 guix-build-55f1c2ac8beb/output/aarch64-linux-gnu/SHA256SUMS.part
890e9ac6b9c445bc1fe799378d3c7f6a9ac3c6c2d650e70d1a251a23cca88394 guix-build-55f1c2ac8beb/output/aarch64-linux-gnu/bitcoin-55f1c2ac8beb-aarch64-linux-gnu-debug.tar.gz
13e21adc0e0f487e19649d468753c3565f60b20d5e021576cd27cdf6aad16640 guix-build-55f1c2ac8beb/output/aarch64-linux-gnu/bitcoin-55f1c2ac8beb-aarch64-linux-gnu.tar.gz
ee0faf9e
...
💬 fanquake commented on pull request "windows: Use predefined `RC_INVOKED` macro instead of custom one":
(https://github.com/bitcoin/bitcoin/pull/32633#issuecomment-2919295253)
Guix Build (win):
```bash
512632ba356a2e858091dca086221a1d16b9b3318cb44c0b513dde6548379888 guix-build-55f1c2ac8beb/output/dist-archive/bitcoin-55f1c2ac8beb.tar.gz
18079f9eaa6b541fc2d4d425de55e10a8fec1fc7a549f4ac7dcf25275d632e0e guix-build-55f1c2ac8beb/output/x86_64-w64-mingw32/SHA256SUMS.part
0d548fc0b8de0a087644930b52e8ef8b6fce93e73e1a4ff30076efee9075993e guix-build-55f1c2ac8beb/output/x86_64-w64-mingw32/bitcoin-55f1c2ac8beb-win64-codesigning.tar.gz
50dfa21fba79537a7f1ddb9e870931f72fc96
...
👍 fanquake approved a pull request: "windows: Use predefined `RC_INVOKED` macro instead of custom one"
(https://github.com/bitcoin/bitcoin/pull/32633#pullrequestreview-2878165242)
ACK 55f1c2ac8beb5ebebcaef0707d8cc3003e41d80e
🚀 fanquake merged a pull request: "windows: Use predefined `RC_INVOKED` macro instead of custom one"
(https://github.com/bitcoin/bitcoin/pull/32633)
💬 strmfos commented on pull request "Replace dead gnome link notificator.cpp":
(https://github.com/bitcoin/bitcoin/pull/32629#issuecomment-2919401771)
@hebasto I removed the outdated link (comment), if I can do anything else, I'll be glad
Thank you
💬 hebasto commented on pull request "Replace dead gnome link notificator.cpp":
(https://github.com/bitcoin/bitcoin/pull/32629#issuecomment-2919418223)
Please [squash](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits) your commits and update the PR title and description.