Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

(extra) small batch of trivial backports

How Has This Been Tested?

CI

Breaking Changes

None

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@PastaPastaPasta PastaPastaPasta added this to the 23 milestone May 20, 2025
@coderabbitai
Copy link

coderabbitai bot commented May 20, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

The updates include several targeted changes across build scripts, patches, and test files. In the symbol checking script, a version check was updated to require a newer tool version. The Guix build environment was adjusted by pinning to a new commit hash and introducing a helper function to apply a reproducibility patch to the MinGW winpthreads package, with corresponding updates to the cross-toolchain creation logic. Two patches were added or modified to the GCC and winpthreads Makefiles to append -ffile-prefix-map flags, ensuring debug symbols do not embed architecture-dependent Guix store paths, thereby improving build reproducibility. Additionally, a redundant block of coin selection test code was removed from the wallet test suite.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 257b1b7 and 6a0b179.

📒 Files selected for processing (6)
  • contrib/devtools/symbol-check.py (1 hunks)
  • contrib/guix/libexec/prelude.bash (1 hunks)
  • contrib/guix/manifest.scm (1 hunks)
  • contrib/guix/patches/gcc-remap-guix-store.patch (1 hunks)
  • contrib/guix/patches/winpthreads-remap-guix-store.patch (1 hunks)
  • src/wallet/test/coinselector_tests.cpp (0 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
contrib/devtools/symbol-check.py (1)

254-257: Necessary version update for MACHO tool validation

This update to the expected LLVM linker (LLD) version check makes sense as part of keeping the toolchain requirements current.

Consider simplifying this function by returning the condition directly:

def check_MACHO_lld(binary) -> bool:
-    if binary.build_version.tools[0].version == [18, 1, 8]:
-        return True
-    return False
+    return binary.build_version.tools[0].version == [18, 1, 8]
🧰 Tools
🪛 Ruff (0.11.9)

255-257: Return the condition directly

Inline condition

(SIM103)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between dc6c35b and c97367f.

📒 Files selected for processing (6)
  • contrib/devtools/symbol-check.py (1 hunks)
  • contrib/guix/libexec/prelude.bash (1 hunks)
  • contrib/guix/manifest.scm (1 hunks)
  • contrib/guix/patches/gcc-remap-guix-store.patch (1 hunks)
  • contrib/guix/patches/winpthreads-remap-guix-store.patch (1 hunks)
  • src/wallet/test/coinselector_tests.cpp (0 hunks)
💤 Files with no reviewable changes (1)
  • src/wallet/test/coinselector_tests.cpp
🧰 Additional context used
🪛 Ruff (0.11.9)
contrib/devtools/symbol-check.py

255-257: Return the condition directly

Inline condition

(SIM103)

🔇 Additional comments (5)
contrib/guix/libexec/prelude.bash (1)

54-54: Update to newer Guix version looks good

This change updates the pinned Guix commit hash to a newer version, supporting the PR's goal of improving build reproducibility.

contrib/guix/patches/winpthreads-remap-guix-store.patch (1)

1-18: Excellent reproducibility enhancement for debug symbols

This patch improves build reproducibility by ensuring debug symbols don't contain architecture-specific Guix store paths. Instead, all paths under /gnu/store are remapped to /usr using the -ffile-prefix-map compiler flag.

The approach is well-documented in the patch header and consistent with similar changes for other components (like the GCC patch).

contrib/guix/patches/gcc-remap-guix-store.patch (1)

1-21: Excellent reproducibility enhancement for libgcc

This patch correctly implements path remapping for libgcc debug symbols, ensuring that architecture-specific Guix store paths don't affect build reproducibility. The implementation is clean and mirrors the approach used in the winpthreads patch.

The consistent approach across multiple components demonstrates good systematic thinking about the build reproducibility problem.

contrib/guix/manifest.scm (2)

131-133: Well-structured helper function addition

The new winpthreads-patches function follows the same pattern as the existing patch helper functions, providing a clean way to apply the reproducibility patch to the MinGW winpthreads package.


139-141: Good integration with existing code

This change properly wraps the make-mingw-w64 call with the newly defined winpthreads-patches function, maintaining the original functionality while adding the reproducibility improvement. This approach improves build reproducibility by ensuring debug symbols don't embed architecture-dependent Guix store paths.

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK c97367f (works with #6682 applied)

achow101 and others added 3 commits May 30, 2025 22:45
fb1c6c1 test: Remove redundant test (yancy)

Pull request description:

  I can't think of any reason to keep this test case around labeled [fix me](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L242).  The test was originally added [here](bitcoin@4566ab7) however there was never an assertion about the coins that should be selected, only that a solution is found (which is a redundant solution to the test [above](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L222)).  The comment was later added here to [fix](bitcoin@3842732) it, however it's unclear what exactly it's testing.  A test was later added [here](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L366) where if the [long term fee](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L357) is less than the current [fee](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L356), then select fewer UTXOs, which may have been the original intent.

ACKs for top commit:
  S3RK:
    ACK fb1c6c1
  Zero-1729:
    Concept ACK fb1c6c1
  achow101:
    ACK fb1c6c1

Tree-SHA512: bce2cdae669c144ffaa130237a1643e3b6728e13d603cebf5d9493c4c7c68b3635868e4d93d210783c2ded2a871f185ca09a2053168c05b26a1e056ff6edf68f
…f23802cfe75a737963c

6ee000e guix: bump time-machine to efc26826400762207cde9f23802cfe75a737963c (fanquake)
cbeb2c2 guix: patch /gnu/store paths out of winpthreads (fanquake)

Pull request description:

  Needed for bitcoin#30210. This doesn't switch runtimes, because upstream is
  still configured to use the old runtime. See:
  https://git.savannah.gnu.org/cgit/guix.git/commit/?id=17188be0f723e00377b21b767f5447d7938a116e.

  git-mimimal `2.45.1` -> `2.45.2`
  Kernel Headers `6.1.92` -> `6.1.100`
  LLVM `18.1.6` -> `18.1.8`
  mingw-w64 `11.0.1` -> `12.0.0`
  NSIS `3.09` -> `3.10`
  patch `2.7.6` -> `2.7.6-0.f144b35`

ACKs for top commit:
  TheCharlatan:
    ACK 6ee000e

Tree-SHA512: f4f99d16dd8cab5b2b7c5d82111af86de20e1669cc3b4054d72ab4a67b2956757df170f0df28c96d18653c1c9d2ebdd0ecc441005726a20cd963d98513b4a851
…f93ae88fbef25427252

eca20be guix: bump time-machine to 7bf1d7aeaffba15c4f680f93ae88fbef25427252 (fanquake)

Pull request description:

  Includes:
  https://git.savannah.gnu.org/cgit/guix.git/commit/?id=d428237642e1e4ac8fda4597205ffec89926c0ec.

  which removes the need to build Python2, and OpenSSL `1.x` (which has historically caused issues) when building for Windows (Python2 (with a dependency on OpenSSL `1.x`) used to be a dependency of NSIS).

  Linux Kernel Headers `6.1.100` -> `6.1.102`

  ```bash
  d079858fb1bc526217ee06f312d97a56c34986440e5f9e108af66eaecacea073  guix-build-eca20bead2da/output/aarch64-linux-gnu/SHA256SUMS.part
  2db780ffe39210a3ba113e52362d94840449218ac1747e3a3484606cc36acead  guix-build-eca20bead2da/output/aarch64-linux-gnu/bitcoin-eca20bead2da-aarch64-linux-gnu-debug.tar.gz
  b56b602bd87e73b11a6b68147c52c6dfa53f0ec4bac52ac749765025e7b43bc9  guix-build-eca20bead2da/output/aarch64-linux-gnu/bitcoin-eca20bead2da-aarch64-linux-gnu.tar.gz
  d56a9a6ac683da2e347d2ea71fab0cd54a126604ac1c9cc4d8fa89f6343ddb52  guix-build-eca20bead2da/output/arm-linux-gnueabihf/SHA256SUMS.part
  6555b6d837605b35c5cf72ab4e5728d9205f8481e3e4bc9e1bbe44e09a1aa1a3  guix-build-eca20bead2da/output/arm-linux-gnueabihf/bitcoin-eca20bead2da-arm-linux-gnueabihf-debug.tar.gz
  d310e9a532d035db238552ad3fa3779b93d062e655f8e477ff029682af4f7cf4  guix-build-eca20bead2da/output/arm-linux-gnueabihf/bitcoin-eca20bead2da-arm-linux-gnueabihf.tar.gz
  eab39d953890e2d36b92b53e43a711ab615090d5b9030861229386e4a41344d8  guix-build-eca20bead2da/output/arm64-apple-darwin/SHA256SUMS.part
  394473362a4d4895431d9250702e9e87f8ccb880a2b723eeb0cfd8eed1635518  guix-build-eca20bead2da/output/arm64-apple-darwin/bitcoin-eca20bead2da-arm64-apple-darwin-unsigned.tar.gz
  3e72306a34e69647200fe5b3d5e8da3bc3b75105d3eeb9f5d5b5332de5d8e464  guix-build-eca20bead2da/output/arm64-apple-darwin/bitcoin-eca20bead2da-arm64-apple-darwin-unsigned.zip
  1b0515ab24a57706278bde37b88a0dc51188373154876997dcbf61f2c9cf2c65  guix-build-eca20bead2da/output/arm64-apple-darwin/bitcoin-eca20bead2da-arm64-apple-darwin.tar.gz
  fba7470833ff076c01de5f2ed4a4d697f8a2e06cec49098f89d40e84c4798357  guix-build-eca20bead2da/output/dist-archive/bitcoin-eca20bead2da.tar.gz
  935cc2ebec9b595da67bcde22a395060817fdf16b3a31f14e6b2252cb5073640  guix-build-eca20bead2da/output/powerpc64-linux-gnu/SHA256SUMS.part
  85ee64aa6d0ab6d3431b7c0af3e26a9bfdb365343ba1b5198e321ae0f6778d33  guix-build-eca20bead2da/output/powerpc64-linux-gnu/bitcoin-eca20bead2da-powerpc64-linux-gnu-debug.tar.gz
  52a1a8e8fdb48589cfb00c35001ae8765f6127d472d11ad03c3faa3621e45032  guix-build-eca20bead2da/output/powerpc64-linux-gnu/bitcoin-eca20bead2da-powerpc64-linux-gnu.tar.gz
  72ccb2577dd11342dfce124346d359b19d8bc4af12cd445447e0568321dd39b9  guix-build-eca20bead2da/output/riscv64-linux-gnu/SHA256SUMS.part
  80a0d80c3adb8e2de27605ed0b2bd7f5442c8316397a53a3e0e840a14587b057  guix-build-eca20bead2da/output/riscv64-linux-gnu/bitcoin-eca20bead2da-riscv64-linux-gnu-debug.tar.gz
  62de93f8defc9e561caed1586bee20e208be1d66cdc8bf593f5e09e3c28d03a6  guix-build-eca20bead2da/output/riscv64-linux-gnu/bitcoin-eca20bead2da-riscv64-linux-gnu.tar.gz
  e5af6b6bb63d88f7797531f855ad0a8fb5bf0e4645a7b2f83516b4cb26bf1da4  guix-build-eca20bead2da/output/x86_64-apple-darwin/SHA256SUMS.part
  3743509c6ad5fb3837bd8d420885b49221515e89d1bba12f8816f401879bee7a  guix-build-eca20bead2da/output/x86_64-apple-darwin/bitcoin-eca20bead2da-x86_64-apple-darwin-unsigned.tar.gz
  cc6fb35a57506250790ddea4aaa7888aa9d1db66f8ce3f09c830898f83f2f39f  guix-build-eca20bead2da/output/x86_64-apple-darwin/bitcoin-eca20bead2da-x86_64-apple-darwin-unsigned.zip
  4b4d2096e87ca10847e5a543ff32f002325c882856523f0fc5d70564009f9244  guix-build-eca20bead2da/output/x86_64-apple-darwin/bitcoin-eca20bead2da-x86_64-apple-darwin.tar.gz
  1989106147fc5f77bc27a08886bb2120ff0c49cbe6ea97b9e234752740ec81ad  guix-build-eca20bead2da/output/x86_64-linux-gnu/SHA256SUMS.part
  bdb48a649f9ca026e6bbab28159f716a1ad4b84257588e1a12bf4467e4c7acb6  guix-build-eca20bead2da/output/x86_64-linux-gnu/bitcoin-eca20bead2da-x86_64-linux-gnu-debug.tar.gz
  aff4717e841508bd6284d846d8c6da7da3622bf54d68a8919e3fd95814beb309  guix-build-eca20bead2da/output/x86_64-linux-gnu/bitcoin-eca20bead2da-x86_64-linux-gnu.tar.gz
  fdc8346e0b0f03648399b74a0d38d961c985c5ec8128193443be0b7208632f06  guix-build-eca20bead2da/output/x86_64-w64-mingw32/SHA256SUMS.part
  a50f517e3f2467e5931349315bbe0968e190e8bcdbb024e3a8d4c37333938155  guix-build-eca20bead2da/output/x86_64-w64-mingw32/bitcoin-eca20bead2da-win64-debug.zip
  38223484c214a90193f88f8c60743b376ce0c80f9401ec863ccb36a1337c85a2  guix-build-eca20bead2da/output/x86_64-w64-mingw32/bitcoin-eca20bead2da-win64-setup-unsigned.exe
  07fca2496b2c59ea928684c4bf4ef163686f8fb11934117c6c37407a3a374363  guix-build-eca20bead2da/output/x86_64-w64-mingw32/bitcoin-eca20bead2da-win64-unsigned.tar.gz
  8e71711db17c69000627c44965eb7070fc92788f792ea3d0778a55bdadf36cdb  guix-build-eca20bead2da/output/x86_64-w64-mingw32/bitcoin-eca20bead2da-win64.zip
  ```

ACKs for top commit:
  TheCharlatan:
    Nice, ACK eca20be

Tree-SHA512: eb1795dcc82bd975b76631326c72c4456d71be8b8cf509195295021c0581abee6da36b55d7faddb440f953e24996ee70b01b35ae9e945fcbb0799efb2e29b916
@PastaPastaPasta PastaPastaPasta merged commit 9ef4783 into dashpay:develop May 31, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants