chore: merge bitcoin#28347, partial #30527, #26257, bump to Python 3.10, mypy 0.981, fix Docker group assignment, minor housekeeping (misc. changes: part 2) #6987
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request updates the project Python runtime from 3.9.18 to 3.10.14 in multiple build/config files, bumps development tooling versions (mypy, pyzmq, vulture), removes contrib/guix/Dockerfile and updates references to a new contrib/containers/guix path, adds USER_ID/GROUP_ID build args in docker-compose, tightens shell quoting in configure.ac, refactors Windows stack-unwinding frame skipping in src/stacktraces.cpp, and switches the lint dependency check from pkg_resources to importlib.metadata. Sequence Diagram(s)mermaid mermaid mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 2
🧹 Nitpick comments (4)
contrib/guix/INSTALL.md (1)
59-62: Path verified; consider improving link descriptiveness.The new Dockerfile path
contrib/containers/guix/Dockerfileis correct and exists. The link text "here" could be improved for clarity and accessibility. Consider updating the link text to something more descriptive like "Dockerfile" or the full path reference instead of the generic "here".contrib/containers/develop/docker-compose.yml (1)
7-9: Document how users should override the default USER_ID and GROUP_ID values.The hardcoded defaults (1000:1000) may not match the host user's actual UID/GID, which could lead to permission issues. Consider documenting in a comment or README that users should pass these values, e.g.,
USER_ID=$(id -u) GROUP_ID=$(id -g) docker-compose up --build.Verify that the target Dockerfile (
./develop/Dockerfile) actually consumes these build arguments:#!/bin/bash # Check if the Dockerfile uses the USER_ID and GROUP_ID build arguments echo "=== Checking if Dockerfile consumes USER_ID and GROUP_ID ===" fd -t f "Dockerfile" contrib/containers/develop/ --exec grep -l "ARG USER_ID\|ARG GROUP_ID" {} \; echo -e "\n=== Showing usage context ===" fd -t f "Dockerfile" contrib/containers/develop/ --exec grep -A 3 "ARG USER_ID\|ARG GROUP_ID" {} \;contrib/containers/ci/ci-slim.Dockerfile (1)
129-131: Consider removing redundant chown command.Line 130 (
chown ${USER_ID}:${GROUP_ID} /src) appears redundant since line 131 performs the same operation recursively. The non-recursive chown on line 130 doesn't add value.Apply this diff to remove the redundant command:
usermod -u ${USER_ID} -md /home/dash -l dash ubuntu; \ chown ${USER_ID}:${GROUP_ID} -R /home/dash; \ mkdir -p /src/dash && \ - chown ${USER_ID}:${GROUP_ID} /src && \ chown ${USER_ID}:${GROUP_ID} -R /srctest/lint/lint-python.py (1)
15-17: Consider removing extra blank line for consistency.The import section now has an extra blank line (lines 16-17). While not incorrect, typical Python style uses a single blank line between import groups.
Apply this diff:
from importlib.metadata import metadata, PackageNotFoundError - - + DEPS = ['flake8', 'lief', 'mypy', 'pyzmq']
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.python-version(1 hunks)ci/lint/04_install.sh(1 hunks)configure.ac(1 hunks)contrib/containers/ci/ci-slim.Dockerfile(3 hunks)contrib/containers/develop/docker-compose.yml(1 hunks)contrib/guix/Dockerfile(0 hunks)contrib/guix/INSTALL.md(1 hunks)doc/dependencies.md(1 hunks)src/stacktraces.cpp(3 hunks)test/lint/lint-python.py(2 hunks)
💤 Files with no reviewable changes (1)
- contrib/guix/Dockerfile
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
📚 Learning: 2025-02-11T19:25:05.487Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.487Z
Learning: The Dash Core deploy container runs a single process and doesn't require complex init systems, service supervision, or system utilities that Phusion baseimage provides, making vanilla Ubuntu a more suitable base image choice.
Applied to files:
contrib/guix/INSTALL.mdcontrib/containers/develop/docker-compose.ymlcontrib/containers/ci/ci-slim.Dockerfile
📚 Learning: 2025-02-11T19:25:05.487Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.487Z
Learning: The deploy container for Dash Core is designed to be minimal, requiring only basic container functionality without complex init systems or service management.
Applied to files:
contrib/guix/INSTALL.mdcontrib/containers/develop/docker-compose.yml
📚 Learning: 2025-01-06T09:51:03.167Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6516
File: depends/patches/gmp/include_ldflags_in_configure.patch:557-621
Timestamp: 2025-01-06T09:51:03.167Z
Learning: The `GMP_GCC_ARM_UMODSI` macro checks only the compiler version, and `GMP_GCC_MIPS_O32` relies on the `-mabi=32` flag. Therefore, `$LDFLAGS` is irrelevant to these tests.
Applied to files:
configure.ac
📚 Learning: 2025-07-14T10:11:05.011Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6754
File: contrib/containers/guix/docker-compose.yml:18-19
Timestamp: 2025-07-14T10:11:05.011Z
Learning: In the Guix build process for Dash Core, the `guix.sigs` directory requires write access as signatures are written to it during the build process, and `dash-detached-sigs` may be updated with `git pull` operations, so both directories need rw permissions in the Docker volume mounts.
Applied to files:
contrib/containers/develop/docker-compose.yml
🪛 markdownlint-cli2 (0.18.1)
contrib/guix/INSTALL.md
62-62: Link text should be descriptive
(MD059, descriptive-link-text)
🔇 Additional comments (7)
.python-version (1)
1-1: LGTM! Python version bump is consistent with PR objectives.The upgrade from 3.9.18 to 3.10.14 aligns with the documented changes across the repository and represents a stable Python release.
doc/dependencies.md (1)
13-13: LGTM! Documentation properly updated for Python 3.10 requirement.The minimum Python version update is consistent with the runtime changes and properly references the upstream PR.
configure.ac (1)
1270-1270: LGTM! Proper shell quoting prevents potential syntax errors.Quoting
$LINK_WRAP_SUPPORTEDprotects against word splitting and glob expansion, following shell scripting best practices.contrib/containers/ci/ci-slim.Dockerfile (2)
58-58: LGTM! Python version consistent with project-wide upgrade.The update to Python 3.10.14 aligns with changes in
.python-versionand other configuration files.
81-83: LGTM! Package versions consistent with CI lint configuration.The mypy, pyzmq, and vulture version bumps match those in
ci/lint/04_install.sh, maintaining consistency across CI environments.test/lint/lint-python.py (1)
101-105: LGTM! Modernized dependency checking using importlib.metadata.Switching from deprecated
pkg_resourcestoimportlib.metadata(Python 3.8+) is the correct approach. The new implementation is cleaner and maintains the same behavior.src/stacktraces.cpp (1)
170-170: Approved—the code is logically correct and follows expected backport practices.The refactoring separates the immutable
skipparameter from a mutableskip_framescounter. The logic is sound:
skip_framesis initialized fromskip(line 170)- Incremented when skipping the current frame (line 210)
- Used consistently to filter collected frames (line 228)
The
volatilequalifier is unusual but not problematic. While the upstream rationale cannot be verified through public search, the code contains no bugs, undefined behavior, or compilation issues. Per backport practices, such unusual patterns should be preserved to maintain upstream fidelity unless they introduce genuine defects, which this does not.
…encies includes: - 1e5e87c (portions)
kwvg
force-pushed
the
misc_changes_p2
branch
from
137c10d to
82723dc
Compare
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contrib/guix/INSTALL.md (1)
61-62: Use descriptive link text instead of "here".Markdown best practices recommend descriptive link text for accessibility and clarity. Replace the generic placeholder "here" with descriptive text that conveys the link's purpose.
Consider this change:
-Please refer to Docker image -[here](https://github.com/dashpay/dash/tree/master/contrib/containers/guix/Dockerfile). +Please refer to the [guix Docker image](https://github.com/dashpay/dash/tree/master/contrib/containers/guix/Dockerfile).Or alternatively:
-Please refer to Docker image -[here](https://github.com/dashpay/dash/tree/master/contrib/containers/guix/Dockerfile). +Please refer to the [Dockerfile](https://github.com/dashpay/dash/tree/master/contrib/containers/guix/Dockerfile) for building the guix Docker image.The path update from
contrib/guix/Dockerfiletocontrib/containers/guix/Dockerfileis correct and aligns with the container reorganization in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.python-version(1 hunks)ci/lint/04_install.sh(1 hunks)configure.ac(1 hunks)contrib/containers/ci/ci-slim.Dockerfile(3 hunks)contrib/containers/develop/docker-compose.yml(1 hunks)contrib/guix/Dockerfile(0 hunks)contrib/guix/INSTALL.md(1 hunks)doc/dependencies.md(1 hunks)src/stacktraces.cpp(3 hunks)test/lint/lint-python.py(2 hunks)
💤 Files with no reviewable changes (1)
- contrib/guix/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (5)
- .python-version
- src/stacktraces.cpp
- test/lint/lint-python.py
- contrib/containers/ci/ci-slim.Dockerfile
- configure.ac
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
ci/lint/04_install.sh
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
ci/lint/04_install.sh
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Applied to files:
ci/lint/04_install.sh
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
ci/lint/04_install.sh
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
ci/lint/04_install.sh
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
ci/lint/04_install.sh
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
ci/lint/04_install.sh
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Applied to files:
ci/lint/04_install.sh
📚 Learning: 2025-02-11T19:25:05.487Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.487Z
Learning: The deploy container for Dash Core is designed to be minimal, requiring only basic container functionality without complex init systems or service management.
Applied to files:
contrib/containers/develop/docker-compose.ymlcontrib/guix/INSTALL.md
📚 Learning: 2025-02-11T19:25:05.487Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6564
File: contrib/containers/deploy/Dockerfile:1-1
Timestamp: 2025-02-11T19:25:05.487Z
Learning: The Dash Core deploy container runs a single process and doesn't require complex init systems, service supervision, or system utilities that Phusion baseimage provides, making vanilla Ubuntu a more suitable base image choice.
Applied to files:
contrib/containers/develop/docker-compose.ymlcontrib/guix/INSTALL.md
📚 Learning: 2025-07-14T10:11:05.011Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6754
File: contrib/containers/guix/docker-compose.yml:18-19
Timestamp: 2025-07-14T10:11:05.011Z
Learning: In the Guix build process for Dash Core, the `guix.sigs` directory requires write access as signatures are written to it during the build process, and `dash-detached-sigs` may be updated with `git pull` operations, so both directories need rw permissions in the Docker volume mounts.
Applied to files:
contrib/containers/develop/docker-compose.yml
🪛 markdownlint-cli2 (0.18.1)
contrib/guix/INSTALL.md
62-62: Link text should be descriptive
(MD059, descriptive-link-text)
⏰ 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). (4)
- GitHub Check: linux64_multiprocess-test / Test source
- GitHub Check: linux64_sqlite-test / Test source
- GitHub Check: linux64-test / Test source
- GitHub Check: linux64_tsan-test / Test source
🔇 Additional comments (3)
ci/lint/04_install.sh (1)
39-41: Package version bumps align with Python 3.10 upgrade.The version specifications (mypy 0.981, pyzmq 24.0.1, vulture 2.6) are consistent with the Python 3.10.14 upgrade across the codebase and have been verified for compatibility.
doc/dependencies.md (1)
13-13: Documentation reflects Python 3.10 minimum requirement.This update is consistent with the runtime upgrade throughout the PR and aligns with upstream Bitcoin Core PR bitcoin#30527.
contrib/containers/develop/docker-compose.yml (1)
7-9: No issues found. The USER_ID and GROUP_ID build arguments are properly declared incontrib/containers/ci/ci-slim.Dockerfile(lines 123-124 with defaults of 1000) and correctly inherited through the image hierarchy todevelop/Dockerfile. The docker-compose.yml additions properly expose these arguments for override with clear documentation.
kwvg
changed the title
4 participants
Additional Information
Dependency for ci: merge bitcoin#31529, #30730, partial bitcoin#33185, use Guix for sourcing non-default GCC, bump to GCC 15 #6988
Python 3.9's security support elapsed on 31st Oct '25 (source), in response to that we are switching over to Python 3.10.
Due to python/mypy#13627 arising as a result of this bump, we also had to upgrade
mypyto 0.981. Note that this is a divergence from upstream as they opted to upgrade tomypy1.x (see bitcoin#28009) but the changes needed to do so are too disruptive given this PR's larger context.Future backports should feel free to overwrite the mypy version and realign with upstream.
CI identified potential clobbering (build) in Windows stacktraces code that started to build after dash#6966 (see below). Additionally, C++20 has deprecated certain operators like {in,de}crementation courtesy of P1152, which required additional adaptation.
Build error:
The
AM_CONDITIONALforCRASH_HOOKS_WRAPPED_CXX_ABIwasn't evaluated due to missing quotations (see below), this has been resolved.Configure:
Currently we offer two containers for Guix builds meant for developers who aren't on Linux, one was introduced by dash#5285 (based on fanquake/core-review's container, source) and the other introduced by dash#5449, the former does not seem to get much use and has been out of sync with its upstream source for a while.
As the second container is used by some developers and is updated and maintained to fit Dash's specific needs, the first container has been dropped and documentation has been updated to reflect the same.
As Docker has matured with the WSL2 backend on Windows and the Apple Virtualization framework on macOS, boot2docker has been deprecated (see boot2docker/boot2docker#1408) and the comment referencing it has been dropped.
To avoid permissions issues with mounting directories, containers come with
USER_IDandGROUP_IDargs (source) that need to be specified at build time if the mount needs different permissions (as is often the case on macOS).To make that option more explicitly clear, it has been specified in
docker-compose.ymlwith default values filled in.dash#6929 introduced a fix to prevent unexpected container build failure due to conflicting groups (most commonly GID 20 that on macOS is
staffbut on Linux isdialout) but the fix did not assign the default user to that group, that has been resolved here.Breaking Changes
None expected.
Checklist