ci: merge bitcoin#24585, #25549, #26834, #26773, #30989, #32498, #32439, #32678, #33073, update channels, substitution servers, fix guix-check
#6895
Conversation
|
@coderabbitai review |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
✅ Actions performedReview triggered.
|
WalkthroughReorders Guix archive authorization keys and adds creation of a guixbuild group with multiple unprivileged guixbuilder users in contrib/containers/guix/Dockerfile; copies a pinned contrib/containers/guix/channels.scm and additional guix helper scripts into the image; creates runtime/cache and workspace directories, sets working directory to /src/dash, switches to an unprivileged user, and updates the entrypoint to build substitute URLs from a SERVERS array. guix-check now uses guix-attest to populate a temporary sigs repo and reads SHA256 sums from generated files. Adds check_source_date_epoch() to prelude.bash and invokes it from guix-build and guix-codesign. Removes contrib/install_db4.sh and updates multiple docs to prefer depends-based Berkeley DB workflows and replace Savannah Guix URLs with Codeberg. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (19)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used📓 Path-based instructions (2)contrib/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
doc/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-10-05T20:38:28.457ZApplied to files:
🪛 LanguageTooldoc/build-osx.md[grammar] ~87-~87: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) 🪛 markdownlint-cli2 (0.18.1)doc/build-freebsd.md45-45: Tables should be surrounded by blank lines (MD058, blanks-around-tables) 47-47: Link text should be descriptive (MD059, descriptive-link-text) 50-50: Tables should be surrounded by blank lines (MD058, blanks-around-tables) 52-52: Link text should be descriptive (MD059, descriptive-link-text) 53-53: Link text should be descriptive (MD059, descriptive-link-text) doc/build-osx.md83-83: Heading levels should only increment by one level at a time (MD001, heading-increment) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (16)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contrib/containers/guix/scripts/entrypoint (1)
21-26: Consider simplifying the IFS manipulation.The current approach with
IFS_OLDandIFS=' 'is correct but could be streamlined. While the implementation works, you could use a more concise approach:-IFS_OLD="${IFS}" -IFS=' ' sudo env PATH=${PATH} \ - guix-daemon --build-users-group='guixbuild' --substitute-urls="${SERVERS[*]}" < /dev/null 2>&1 | + guix-daemon --build-users-group='guixbuild' --substitute-urls="$(IFS=' '; echo "${SERVERS[*]}")" < /dev/null 2>&1 | sudo tee /var/log/guix.log > /dev/null & -IFS="${IFS_OLD}"This uses a subshell for the IFS change, eliminating the need to save and restore
IFS.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f170aed and 1ca2db9402bcf7e7352f4ee722882893d94ed856.
📒 Files selected for processing (14)
contrib/containers/guix/Dockerfile(2 hunks)contrib/containers/guix/channels.scm(1 hunks)contrib/containers/guix/scripts/entrypoint(1 hunks)contrib/containers/guix/scripts/guix-check(1 hunks)contrib/guix/INSTALL.md(3 hunks)contrib/guix/README.md(1 hunks)contrib/guix/guix-build(1 hunks)contrib/guix/guix-codesign(1 hunks)contrib/guix/libexec/build.sh(0 hunks)contrib/guix/libexec/prelude.bash(2 hunks)contrib/install_db4.sh(0 hunks)depends/README.md(1 hunks)doc/build-openbsd.md(1 hunks)doc/build-unix.md(1 hunks)
💤 Files with no reviewable changes (2)
- contrib/install_db4.sh
- contrib/guix/libexec/build.sh
🔇 Additional comments (18)
depends/README.md (1)
109-110: LGTM!The new
NO_BOOSTandNO_LIBEVENToptions follow the established pattern of other dependency options and are clearly documented. These align well with the depends-based Berkeley DB build approach introduced elsewhere in the PR.contrib/containers/guix/scripts/entrypoint (1)
8-18: Good geographic coverage for substitution servers.The SERVERS array includes both official servers and geographically distributed mirrors (Singapore, US East), which will improve fetch latency for users outside Europe. The mirrors are appropriate choices based on the PR description's reference to LibrePlanet sources.
contrib/guix/libexec/prelude.bash (2)
24-42: LGTM!The
check_source_date_epochfunction correctly guards against unintended SOURCE_DATE_EPOCH usage that could break reproducibility. The error message is clear and provides actionable guidance with two resolution paths. The exit code 1 is appropriate for this validation failure.
73-73: LGTM!The URL update to Codeberg aligns with the PR's objective to migrate from git.savannah.gnu.org to Codeberg hosting.
contrib/guix/INSTALL.md (3)
322-322: LGTM!The clone URL correctly updates to Codeberg hosting, aligning with the PR's migration objective.
610-612: LGTM!The example output correctly reflects the new Codeberg repository URL.
763-764: LGTM!The commit reference correctly updates to the Codeberg-hosted repository.
contrib/guix/README.md (2)
378-379: LGTM!The wording clarification ("authorized its signing key") is a minor editorial improvement that maintains consistency with substitution server terminology.
385-399: LGTM!Replacing the personal substitution server (dongcarl.carldong.io) with the official Bordeaux server (bordeaux.guix.gnu.org) is appropriate and aligns with using official Guix infrastructure. The PR description confirms this is one of the "now-unavailable substitution source[s]" being removed.
doc/build-unix.md (1)
239-253: LGTM!The updated Berkeley DB build instructions correctly shift from the removed
contrib/install_db4.shscript to a depends-based approach. The instructions are clear and complete:
- The
make -C dependscommand withNO_*flags appropriately excludes unneeded dependencies- The
BDB_PREFIXpath example is correct- The
BDB_LIBSandBDB_CFLAGSconfiguration is properly documentedThis aligns with the PR objective to adapt the build process following the removal of
install_db4.sh(per bitcoin#26834).doc/build-openbsd.md (1)
39-50: LGTM!The updated Berkeley DB build instructions correctly mirror the approach in
doc/build-unix.mdwhile properly using OpenBSD-specific elements:
- Uses
gmake(GNU make) instead ofmake- The output path
x86_64-unknown-openbsdis correct for OpenBSD- The
NO_*flags match the Unix documentation- The
BDB_PREFIXpath correctly points to the depends outputThis maintains consistency across the documentation while respecting platform differences.
contrib/containers/guix/channels.scm (1)
1-11: Verify the pinned commit hash and OpenPGP fingerprint.The channel configuration structure is correct and the migration to Codeberg aligns with the PR objectives. However, please verify:
- The commit hash
56344729cd07c76d5133047f2866237bbb08dcedcorresponds to the intended Guix version- The OpenPGP fingerprint
2A39 3FFF 68F4 EF7A 3D29 12AF 6F51 20A0 22FB B2D5is correct for the introduction commitRun the following to verify the commit exists and check its details:
contrib/containers/guix/scripts/guix-check (2)
19-19: Add error handling for guix-attest invocation.If
guix-attestfails, the script will continue and attempt to read from potentially non-existent files. Consider checking the exit status or usingset -e(which is already set) to ensure failures are caught.Verify that the existing
set -eo pipefailon line 3 is sufficient to abort onguix-attestfailure, or add explicit error handling:if ! ./contrib/guix/guix-attest; then echo "ERROR: guix-attest failed" >&2 exit 1 fi
18-19: Export environment variables for the child process.The variables
SIGNERandNO_SIGNare not exported, so they won't be visible to theguix-attestchild process. This could cause the attestation to fail or use unexpected defaults.Apply this diff to export the variables:
GUIX_SIGS_REPO=$(mktemp -d) trap 'rm -rf -- "$GUIX_SIGS_REPO"' EXIT -SIGNER=dummy -NO_SIGN=1 ./contrib/guix/guix-attest +export SIGNER=dummy +export NO_SIGN=1 +./contrib/guix/guix-attestLikely an incorrect or invalid review comment.
contrib/containers/guix/Dockerfile (2)
55-57: LGTM! Authorization keys properly expanded.The addition of
berlin.guix.gnu.organd explicit authorization of all three keys (Berlin, Bordeaux, CI) aligns with the PR objectives to reduce fetch latency for non-European locations by including the German official server.
75-83: LGTM! Unprivileged user setup is well-structured.The HOME environment variable, directory creation for cache/config, and ownership adjustments are correctly implemented for the unprivileged ubuntu user context.
contrib/guix/guix-build (1)
72-76: LGTM! SOURCE_DATE_EPOCH sanity check properly added.The new sanity check section for
SOURCE_DATE_EPOCHis appropriately placed after creating the version base directory and aligns with the PR objectives to address potential sources of nondeterminism encountered during rc1. The check relies oncheck_source_date_epoch()being defined inprelude.bash, similar to the implementation inguix-codesign.contrib/guix/guix-codesign (1)
70-74: LGTM! SOURCE_DATE_EPOCH sanity check properly added.The new sanity check section for
SOURCE_DATE_EPOCHis well-positioned in the validation flow and aligns with the PR objectives to address potential sources of nondeterminism. The check relies oncheck_source_date_epoch()being defined inprelude.bash.Verify that
check_source_date_epoch()is properly defined incontrib/guix/libexec/prelude.bash:
kwvg
changed the title
guix-checkguix-check
|
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6895.60c761e4. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doc/build-osx.md (1)
83-216: Legacy wallet instructions now fail without BDB env exports
brew install berkeley-db@4alone is not enough: the formula is keg-only and configure will not find headers/libs unless we export the Homebrew paths (e.g.BDB_PREFIX="$(brew --prefix berkeley-db@4)"together withBDB_LIBS/BDB_CFLAGSorCPPFLAGS/LDFLAGS). The previous doc covered this viainstall_db4.sh; with that gone, we still need to document those exports or builders with legacy wallets will hit configure failures. Please add the missing environment setup to keep the flow working.
🧹 Nitpick comments (1)
contrib/containers/guix/scripts/guix-check (1)
22-22: Remove redundant cleanup.Line 22 removes
GUIX_SIGS_REPOagain, but the trap set at line 17 already performs this cleanup on exit. The explicit removal at line 22 is redundant.Apply this diff to remove the redundant cleanup:
SHASUM_LOC="${GUIX_SIGS_REPO}/${VERSION}/${SIGNER}" cat "${SHASUM_LOC}/all.sha256sums" || cat "${SHASUM_LOC}/noncodesigned.SHA256SUMS" -rm -rf "${GUIX_SIGS_REPO}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 1ca2db9402bcf7e7352f4ee722882893d94ed856 and 60c761e4bfa3405adb3e83bbc9309ed32f2c0267.
📒 Files selected for processing (18)
Makefile.am(0 hunks)contrib/containers/guix/Dockerfile(2 hunks)contrib/containers/guix/channels.scm(1 hunks)contrib/containers/guix/scripts/entrypoint(1 hunks)contrib/containers/guix/scripts/guix-check(1 hunks)contrib/guix/INSTALL.md(3 hunks)contrib/guix/README.md(1 hunks)contrib/guix/guix-build(1 hunks)contrib/guix/guix-codesign(1 hunks)contrib/guix/libexec/build.sh(0 hunks)contrib/guix/libexec/prelude.bash(2 hunks)contrib/install_db4.sh(0 hunks)depends/README.md(1 hunks)doc/build-netbsd.md(1 hunks)doc/build-openbsd.md(1 hunks)doc/build-osx.md(1 hunks)doc/build-unix.md(1 hunks)doc/dependencies.md(1 hunks)
💤 Files with no reviewable changes (3)
- Makefile.am
- contrib/install_db4.sh
- contrib/guix/libexec/build.sh
✅ Files skipped from review due to trivial changes (1)
- doc/dependencies.md
🚧 Files skipped from review as they are similar to previous changes (6)
- depends/README.md
- contrib/containers/guix/channels.scm
- contrib/guix/INSTALL.md
- contrib/guix/libexec/prelude.bash
- contrib/containers/guix/Dockerfile
- contrib/guix/guix-codesign
🧰 Additional context used
📓 Path-based instructions (2)
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/guix/guix-buildcontrib/containers/guix/scripts/guix-checkcontrib/containers/guix/scripts/entrypointcontrib/guix/README.md
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/build-unix.mddoc/build-openbsd.mddoc/build-netbsd.mddoc/build-osx.md
🔇 Additional comments (4)
contrib/guix/guix-build (1)
72-76: LGTM!The addition of the
check_source_date_epochsanity check is a good defensive measure to ensure SOURCE_DATE_EPOCH is not unintentionally set before the build process, which helps maintain reproducibility guarantees.contrib/containers/guix/scripts/entrypoint (1)
8-22: LGTM!The introduction of a
SERVERSarray with dynamic URL construction is a maintainable approach. The list includes the German official server (berlin.guix.gnu.org) and mirrors covering North America and Asia-Pacific as described in the PR objectives, which will help reduce container fetch latency for non-European locations.contrib/guix/README.md (1)
378-379: LGTM!The documentation updates correctly reflect the migration to bordeaux.guix.gnu.org as the primary substitute server, aligning with the changes in the entrypoint script and the removal of the previously available substitution source mentioned in the PR objectives.
Also applies to: 385-385, 392-392, 399-399
contrib/containers/guix/scripts/guix-check (1)
16-21: LGTM!The rewrite to use
guix-attestfor generating checksums is a cleaner approach that reuses existing attestation tooling instead of manually computing sums. The temporary directory lifecycle management with trap and the fallback mechanism for missing files are well-implemented.
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6895.60c761e4. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6895.c39ab07b. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6895.c39ab07b. The image should be on dockerhub soon. |
|
|
This pull request has conflicts, please rebase. |
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
I get exactly the same output for c39ab07b285cf6c91a6947617671896cb1505571 locally so it works I guess (did not review individual commits yet). I had to fix it first though to be able to build fresh container (see issues below, can be fixed with e071871a91e5a666d17b3374344210045ebd91b5, gha guix build https://github.com/UdjinM6/dash/actions/runs/18607998322).
|
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6895.e765d490. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6895.e765d490. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6895.f7ae76b1. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6895.f7ae76b1. The image should be on dockerhub soon. |
|
This section should've been gone with bitcoin#21343 (ddc6fca in dash#6114)
The disclaimer needs to be more generic because * libbacktrace is an *optional* dependency if `--enable-stacktraces=no` is passed during configuration * `dashpay/bls-signatures` was made in-tree with dash#5077
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Needed to accommodate for differences in network performance due to geography as each new container instance has to re-download everything. We don't need to import new public keys as they inherit the key for the server they're mirroring (i.e. the mirror cannot modify the contents it is mirroring without _also_ invalidating it), so the risk of malicious changes are relatively mitigated.
The ad-hoc generation method we use now can't be compared against the attested checksums on dashpay/guix.sigs, so we resort to using a creative use of guix-attest to generate the exact set of checksums that an attestor would generate.
|
kwvg
changed the title
guix-checkguix-check
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6895.18d9966a. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6895.18d9966a. The image should be on dockerhub soon. |
5 participants
Motivation
Annoyances from trying to build rc1 and rc2 using our Guix container (source).
Additional Information
Guix has migrated their service provider to Codeberg and have published a timeline for sunsetting the existing
git.savannah.gnu.orgchannel (blog). This requires updating our scripts and on the upstream side this was achieved with bitcoin#32439 but requires additional changes on our end as well.trixieand have flagged theguixpackage as subject to removal, source), we need to set Codeberg as the channel source ourselves.install_db4.shwas dropped via bitcoin#26834 as bitcoin#26833 was backported (see dash#6735) and the script points to the oldgit.savannah.gnu.orgfor sources.Due to our run-in with nondeterminism in rc1, this pull request also includes backports to deal with some other potential sources of nondeterminism. As we're updating sources, a documentation update removing a no longer available substitution source has also been backported.
Both official Guix substitution servers are located in Europe (i.e. France and Germany), which makes the time and bandwidth intensive fetch that is inherent with a container with no persistence painfully slow for those located quite away from Europe, so, alongside addition of the German substitution server (
berlin.guix.gnu.org), mirrors have been added to cover North America (US East) and Asia-Pacific (Singapore).The mirrors have been sourced from LibrePlanet (source) and per their documentation, substitutes from mirrors are signed by the builder they are mirroring, not the mirror itself and as this PR only authorises Guix's official mirrors (see below), tampering risk should be mitigated.
https://github.com/dashpay/dash/blob/1ca2db9402bcf7e7352f4ee722882893d94ed856/contrib/containers/guix/Dockerfile#L55-L57
Another annoyance,
guix-checkso far used an ad-hoc method of generating checksums that cannot be (trivially) diffed against the attested checksums provided atdashpay/guix.sigs, by leveragingguix-attest's ability to generate checksums even without a signer, we can produce the exact output an attestor would without needing to be one (source).Breaking Changes
None.
Checklist