-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
backport: merge bitcoin#23060, #24164, #23565, #24337, #27682, #29208, #29091, #29165, #29934, #30261, partial bitcoin#27662, #28210, #28348, #30263 (bump minimum compiler) #6389
Conversation
This pull request has conflicts, please rebase. |
, bitcoin#28622, bitcoin#28880, bitcoin#29185, bitcoin#29170, bitcoin#29233, bitcoin#29298, bitcoin#29598, bitcoin#29732, bitcoin#29890, bitcoin#29739, bitcoin#30074, bitcoin#30198, bitcoin#29072 (toolchain backports: part 2) 1506d9d merge bitcoin#29072: use `-no_exported_symbols` on macOS (Kittywhiskers Van Gogh) 9247960 merge bitcoin#30198: qt 5.15.14 and fix macOS build with Clang 18 (Kittywhiskers Van Gogh) 5585e7a merge bitcoin#30074: use ENV flags in get_arch (Kittywhiskers Van Gogh) decd420 merge bitcoin#29739: swap cctools otool for llvm-objdump (Kittywhiskers Van Gogh) 0f8c420 merge bitcoin#29890: remove some tools when cross-compiling for macOS (Kittywhiskers Van Gogh) 936da1a merge bitcoin#29732: qt 5.15.13 (Kittywhiskers Van Gogh) c294b47 revert: patch qt to make placeholders differ from actual text (Kittywhiskers Van Gogh) af7090c merge bitcoin#29598: don't use -h with touch on OpenBSD (Kittywhiskers Van Gogh) ebf8ff2 merge bitcoin#29298: patch libtool out of libnatpmp/miniupnpc (Kittywhiskers Van Gogh) 070b876 merge bitcoin#29233: depends move macOS C(XX) FLAGS out of C & CXX (Kittywhiskers Van Gogh) d838481 revert dash#2398: Force fvisibility=hidden when compiling on macos (Kittywhiskers Van Gogh) 59a18f9 merge bitcoin#29170: add macho branch protection check (Kittywhiskers Van Gogh) cb024d9 merge bitcoin#29185: remove `--enable-lto` (Kittywhiskers Van Gogh) 6d75a81 merge bitcoin#28880: switch to using LLVM 17.x for macOS builds (Kittywhiskers Van Gogh) 7b0a1f2 merge bitcoin#28622: use macOS 14 SDK (Xcode 15.0) (Kittywhiskers Van Gogh) 02eb735 merge bitcoin#24948: fix typo in permissions (Kittywhiskers Van Gogh) 2739107 merge bitcoin#24534: make gen-sdk deterministic (Kittywhiskers Van Gogh) ab10bf9 merge bitcoin#24241: cleanup doc on need of Developer Account to obtain macOS SDK (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6384 * Dependency for #6389 * The Qt patch introduced in [dash#5596](#5596), `fix_qt_placeholders.patch`, was a portion of a suggested workaround for QTBUG-92199 ([source](https://bugreports.qt.io/browse/QTBUG-92199?focusedId=669719&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-669719)) but since then, a fix ([here](https://codereview.qt-project.org/c/qt/qtbase/+/434310)) has made its way to 5.15.12 and we are upgrading to 5.15.14 from 5.15.11. So we can safely remove this patch. ## Breaking Changes None expected ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 1506d9d PastaPastaPasta: utACK 1506d9d Tree-SHA512: df8e4ea0ce9e7b269d248518698f0566b5eca1a54cdfb53f5b213b90fb5177e5a5df44eaeb6f3fc014cd93351c9245736bb2fd52bc2af4ae274d8fa93e601b07
WalkthroughThe changes span CI configuration, build scripts, documentation, and source code. In CI files, the container image was updated from CentOS 8 to CentOS 9 along with a corresponding task name change, and a retry wrapper was removed from installation commands. Build macros for checking multiplication overflow and std::filesystem support were removed from auxiliary m4 files and the configure script was adjusted accordingly. The dependency documentation was restructured: detailed lists were replaced with references to an external dependencies file, and instructions for macOS were updated to emphasize LLVM installation. In the source code, the return type in a network function was simplified, built-in overflow tests were removed from fuzz tests, and a MinGW64-specific workaround for file renaming was eliminated. Additionally, linking flags for fuzz testing were simplified and release notes were updated with new minimum compiler version requirements. Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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 (4)
src/net.cpp (1)
1610-1648
: LGTM! Consider adding debug logging for invalid cases.The message type decoding implementation is correct and handles both short and long encodings properly. The input validation and error handling are thorough.
Consider adding debug logging when returning nullopt to help troubleshoot invalid message types:
if (contents.size() == 0) { + LogPrint(BCLog::NET, "V2Transport::GetMessageType: empty contents\n"); return std::nullopt; // Empty contents } // ... if (contents.size() < CMessageHeader::COMMAND_SIZE) { + LogPrint(BCLog::NET, "V2Transport::GetMessageType: insufficient bytes for long encoding (%d)\n", contents.size()); return std::nullopt; // Long encoding needs 12 message type bytes. }doc/dependencies.md (1)
1-6
: Refine Introductory Text for Clarity
The opening lines now explain the dependency overview. Consider shortening the phrase “in reference to the release binaries” (e.g. “for release binaries”) to reduce wordiness as hinted by the static analysis.🧰 Tools
🪛 LanguageTool
[style] ~5-~5: ‘in reference to’ might be wordy. Consider a shorter alternative.
Context: .... "Runtime" and "Version Used" are both in reference to the release binaries. | Compiler | Min...(EN_WORDINESS_PREMIUM_IN_REFERENCE_TO)
.cirrus.yml (1)
81-88
: Update Task for CentOS 9 Environment
The task name update to'32-bit + dash [gui] [CentOS 9]'
and the container image change tocentos:9
ensure consistency with the new CI requirements. Additionally, settingPACKAGE_MANAGER_INSTALL: "yum install -y"
is appropriate for the CentOS environment. Please double-check that YAML formatting (e.g. spacing before colons) complies with linting guidelines.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 82-82: too many spaces before colon
(colons)
doc/build-osx.md (1)
55-68
: Clarify LLVM Upgrade Instructions
The added instructions for installing a more recent version of LLVM and setting the appropriate environment variables are clear and useful. Given that this update supports the new minimum compiler requirements (GCC 11.1 / Clang 16.0+), consider adding a brief note explaining the motivation behind using LLVM (e.g., improved language feature support, compatibility with new C++ standards, etc.). This extra context could help users understand the necessity of the change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.cirrus.yml
(1 hunks)build-aux/m4/bitcoin_runtime_lib.m4
(0 hunks)build-aux/m4/l_filesystem.m4
(0 hunks)ci/test/04_install.sh
(1 hunks)ci/test/05_before_script.sh
(1 hunks)configure.ac
(2 hunks)doc/build-freebsd.md
(2 hunks)doc/build-osx.md
(1 hunks)doc/build-unix.md
(1 hunks)doc/dependencies.md
(1 hunks)doc/release-notes-6389.md
(1 hunks)src/Makefile.test.include
(1 hunks)src/net.cpp
(1 hunks)src/test/fuzz/addition_overflow.cpp
(0 hunks)src/test/fuzz/multiplication_overflow.cpp
(0 hunks)src/util/hasher.h
(1 hunks)src/util/system.cpp
(0 hunks)
💤 Files with no reviewable changes (5)
- src/test/fuzz/multiplication_overflow.cpp
- src/test/fuzz/addition_overflow.cpp
- build-aux/m4/bitcoin_runtime_lib.m4
- build-aux/m4/l_filesystem.m4
- src/util/system.cpp
✅ Files skipped from review due to trivial changes (1)
- doc/release-notes-6389.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/build-freebsd.md
67-67: Heading levels should only increment by one level at a time
Expected: h5; Actual: h6
(MD001, heading-increment)
🪛 YAMLlint (1.35.1)
.cirrus.yml
[warning] 82-82: too many spaces before colon
(colons)
🪛 LanguageTool
doc/dependencies.md
[style] ~5-~5: ‘in reference to’ might be wordy. Consider a shorter alternative.
Context: .... "Runtime" and "Version Used" are both in reference to the release binaries. | Compiler | Min...
(EN_WORDINESS_PREMIUM_IN_REFERENCE_TO)
🔇 Additional comments (11)
src/util/hasher.h (1)
42-42
: LGTM! Documentation URL updated to latest GCC version.The URL reference has been correctly updated to point to the latest GCC 13.2.0 documentation.
src/Makefile.test.include (1)
246-246
: LGTM! Simplified linking flags for fuzz testing binary.Removed
$(RUNTIME_LDFLAGS)
as it's no longer needed with the updated compiler requirements.configure.ac (2)
481-482
: LGTM! Added -Wsuggest-override warning flag.Added compiler warning flag to encourage use of override specifiers in C++ code, which improves code clarity and helps catch inheritance-related bugs.
1373-1390
: LGTM! Improved fuzz binary configuration logic.Restructured the fuzz binary configuration to:
- Only check for main function when fuzz binary is enabled
- Improved conditional logic and error handling
- Added proper messaging
ci/test/05_before_script.sh (1)
28-33
: Simplify SHELL_OPTS Assignment for CentOS
The removal of theLC_ALL=en_US.UTF-8
part and settingSHELL_OPTS="CONFIG_SHELL=/bin/bash"
now simplifies the environment configuration for CentOS. Please verify that none of the downstream commands need the explicit UTF-8 locale setting in the new CentOS 9 environment.doc/dependencies.md (2)
7-54
: Enhanced Structured Documentation
The reorganization into clear sections – "Required", "Optional", "GUI", "Networking", "Notifications", and "Wallet" – with corresponding version and runtime details greatly improves readability and maintainability. All the version references appear consistent with the updated build requirements.
55-56
: Note Formatting Review
The note regarding libgmp’s minimum supported version on macOS is clear. Make sure any cross-references elsewhere (e.g. in platform-specific build docs) are updated to reflect this change.doc/build-freebsd.md (1)
17-18
: Centralize Dependency Information
Replacing the embedded dependency details with a reference todependencies.md
is a good move toward centralizing and maintaining documentation. Ensure that platform-specific nuances are still properly addressed in any separate guides if needed.ci/test/04_install.sh (1)
61-64
: Remove Retry Mechanism for Yum Install Commands
The removal of${CI_RETRY_EXE}
in front of the yum install commands aligns the script with the CentOS 9 setup. Please verify that the removal of the retry logic won’t negatively affect the robustness of package installations, especially during transient network issues in CI.doc/build-osx.md (1)
49-50
: Centralizing Dependency Information
Using a reference to dependencies.md streamlines the documentation by avoiding redundancy. This helps ensure that all dependency details are maintained in a single, authoritative source.doc/build-unix.md (1)
31-32
: Reference to Centralized Dependencies
Adding the line to refer users to dependencies.md for a complete overview is consistent with the updated documentation in other build guides. This change reduces duplication and keeps dependency details in one place for easier maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 0861692
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 0861692
…oble`), merge bitcoin#29985 1599cc6 fix: suppress `float-cast-overflow` UBSan error from `qRound(double)` (Kittywhiskers Van Gogh) a57d128 ci: drop i386 conditional altogether, we don't need `wine32` anymore (Kittywhiskers Van Gogh) b862411 ci: drop conflicting `g++-multilib` package and related workaround (Kittywhiskers Van Gogh) 7b1caa9 fix: remove now-invalid package entry (Kittywhiskers Van Gogh) 9f9e965 fix: use default non-root user `ubuntu` introduced in Ubuntu 22.10 (Kittywhiskers Van Gogh) 9b4c95e ci: update containers and CI to use Ubuntu 24.04 LTS (`noble`) (Kittywhiskers Van Gogh) 8aec25b merge bitcoin#29985: Fix build of Qt for 32-bit platforms with recent glibc (Kittywhiskers Van Gogh) Pull request description: ## Motivation Since [dash#6389](#6389), the minimum supported version of GCC has been 11.1 or later. Our current base, Ubuntu 22.04 LTS (`jammy`) ships with GCC 11.2 ([source](https://packages.ubuntu.com/jammy/gcc)). The cross-compilation package for `arm-linux-gnueabihf` is GCC 11.2 as well ([source](https://packages.ubuntu.com/jammy/gcc-arm-linux-gnueabihf)). Unfortunately, this isn't the case for `mingw-w64-x86-64`, which ships with GCC 10.3 ([source](https://packages.ubuntu.com/jammy/gcc-mingw-w64-x86-64-posix)). So far, an workaround was utilized to allow GCC 10.3 to pass muster upstream through bitcoin#28379 ([source](bitcoin@fa67f09#diff-0baeabda402ee522682c25cef25a248ff6d9e2904f6d66f93f6babf55b576675L986)). Dash Core's enablement of C++20 experimental support in [dash#4600](#4600) was relatively even more permissive ([source](https://github.com/dashpay/dash/pull/4600/files#diff-0baeabda402ee522682c25cef25a248ff6d9e2904f6d66f93f6babf55b576675R168)). Though since then, enforcement of those newer minimum requirements through backports like [bitcoin#30228](bitcoin#30228) are frustrated by the version of GCC shipped for Windows cross-compilation. This can be resolved by bumping the image to the next available LTS release, Ubuntu 24.04 (`noble`), which ships with GCC 13.2 natively ([source](https://packages.ubuntu.com/noble/gcc)), for ARM Linux ([source](https://packages.ubuntu.com/noble/gcc-arm-linux-gnueabihf)) and AMD64 Windows ([source](https://packages.ubuntu.com/noble/gcc-mingw-w64-x86-64-posix)), moreover, it is acknowledged as a _de facto_ lowest supported distro needed for Windows cross compilation by [bitcoin#30580](bitcoin#30580 (comment)). Skipping the enforcement of newer minimum requirements is inadvisable, primarily because it will eventually conflict with upcoming code changes ([comment](bitcoin#30228 (comment))) like [dash#6378](#6378). ## Additional Information * Dependency for #6380 * A default unprivileged user named `ubuntu` was introduced in Ubuntu 22.10 ([source](https://bugs.launchpad.net/cloud-images/+bug/2005129)) with UID `1000` and GID `1000`. We allow specifying UID and GID to ensure they match with the ownership of directories expected to be mounted to avoid permissions issues (especially on platforms like macOS where the user can have a UID of `501` and GID of `20`). * To retain this behavior, the `ubuntu` user and the `ubuntu` group will have its UID and GID updated. This is a no-op if defaults are selected. * In some cases where it may be undesirable to have the username or home directory deviate from the present name (`dash`), it will be additionally renamed from `ubuntu`. * GitLab is unhappy if the container doesn't have a user named `dash` ([build](https://gitlab.com/dashpay/dash/-/jobs/9082688485#L24)) and it results in a runner failure. * Due to a conflict between `gcc-multilib` and `gcc-arm-linux-gnueabihf`, installation of the latter will result in the uninstallation of the former. This has been documented behavior for a while now ([source](https://bugs.launchpad.net/ubuntu/+source/gcc-defaults/+bug/1300211)) and continues till today (see below). <details> <summary>Error message:</summary> ``` ubuntu@ec46b76eeb2c:/src/dash$ sudo apt install gcc-multilib gcc-arm-linux-gnueabihf Reading package lists... Done Building dependency tree... Done Reading state information... Done gcc-arm-linux-gnueabihf is already the newest version (4:13.2.0-7ubuntu1). gcc-arm-linux-gnueabihf set to manually installed. Some packages could not be installed. This may mean that you have requested an impossible situation or if you are using the unstable distribution that some required packages have not yet been created or been moved out of Incoming. The following information may help to resolve the situation: The following packages have unmet dependencies: gcc-arm-linux-gnueabihf : Depends: gcc-13-arm-linux-gnueabihf (>= 13.2.0-11~) but it is not installable E: Unable to correct problems, you have held broken packages. ``` </details> Since [dash#5372](#5372), we have stopped supporting i686 and dropped support for 32-bit Windows even earlier. There doesn't seem to be much reason to keep `gcc-multilib` around, especially since it _cannot_ be around and has been silently uninstalled for a while now, so the package has now been dropped. * Since Wine 7.0, WoW64 support has been included ([source](https://www.winehq.org/announce/7.0)), which allows for some applications to run without a corresponding `wine32` setup ([source](https://wiki.debian.org/Wine#Step_1:_Enable_multiarch)). Support has improved furthermore in Wine 9.0 ([source](https://gitlab.winehq.org/wine/wine/-/releases/wine-9.0)), which is available in `noble` ([source](https://packages.ubuntu.com/noble/wine64)). This allows us to drop the i386 conditional altogether. * `libncurses5`, while a valid package on `jammy` ([source](https://packages.ubuntu.com/jammy/libncurses5)), is not in any future version, including `noble` ([source](https://packages.ubuntu.com/noble/libncurses5), error page) though `libncurses5-dev` continues to remain a valid package ([source](https://packages.ubuntu.com/noble/libncurses5-dev)) and remains used as a Python dependency ([source](https://github.com/dashpay/dash/blob/1930572b05c53d2d8335bcfc22270871d5b0bf2f/contrib/containers/ci/Dockerfile#L91)). As a result, `libncurses5` has been dropped. * A bump in distro base also causes a bump in glibc version, which causes zlib, a Qt dependency to fail a compile-time check ([build](https://github.com/dashpay/dash/actions/runs/13220255380/job/36904408129?pr=6564#step:6:8050)), this is avoided by backporting [bitcoin#29985](bitcoin#29985). * Due to [QTBUG-133261](https://bugreports.qt.io/browse/QTBUG-133261), UBSan will raise a `float-cast-overflow` originating from `qRound(double)` ([build](https://gitlab.com/dashpay/dash/-/jobs/9083558880#L3132)), as of this writing, a fix is currently underway ([source](https://codereview.qt-project.org/c/qt/qtbase/+/621931)) but it is unclear when it would be available in the next Qt 5.15 revision. As this doesn't seem to be a regression ([source](https://bugreports.qt.io/browse/QTBUG-133261?focusedId=860311&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-860311)) despite being undesirable behavior, we can suppress the alarm. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 1599cc6 UdjinM6: utACK 1599cc6 Tree-SHA512: 92f786e9da1440830057729a5d9cdf6448ef9da8c66e0d4c3b80da2e22dd599353f44a31dcbe0ed103febdf627d2a1a332d72de1c3ec312b6ceb665f4bbcb5d1
…s, GCC 11 for building `sqlite` source 7089045 ci: install GCC 11 and use it on sqlite build (Kittywhiskers Van Gogh) 95e24f5 ci: install GCC 14 and use it on nowallet depends and build (Kittywhiskers Van Gogh) Pull request description: ## Motivation This PR is a spiritual continuation of [dash#5375](#5375) to help catch issues like the ones resolved in [dash#5064](#5064) during CI. ## Additional Information * Earlier iterations of this PR built GCC 14 as the toolchain PPA wouldn't go higher than GCC 12 for `jammy` ([source](https://launchpad.net/~ubuntu-toolchain-r/+archive/ubuntu/ppa?field.series_filter=jammy)) (GCC 13 if we were willing to use the `test` branch, [source](https://launchpad.net/~ubuntu-toolchain-r/+archive/ubuntu/test?field.series_filter=jammy)) and we ruled out upgrading our base image to `noble` (which does offer GCC 14 even without a PPA, [source](https://packages.ubuntu.com/noble/g++-14)) so we could test against the lowest supported LTS. * Unfortunately, staying on `jammy` was no longer tenable as we require at least GCC 11.1 (since [dash#6389](#6389)) but `jammy` shipped GCC 10.3 for the Windows cross-compiler ([source](https://packages.ubuntu.com/jammy/gcc-mingw-w64-x86-64-posix)) and this proved to be a blocker for backports. See [dash#6564](#6564) for more information. * Since we've upgraded to `noble` now, we not only can use GCC 14 (the latest major version of GCC as of this writing), we can also use older versions and GCC 11.4 is readily available on `noble` ([source](https://packages.ubuntu.com/noble/g++-11)), which can help us test for unexpected regressions on the lowest supported version and versions likely to be used by rolling release distros. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 7089045; will run my own CI and confirm functionality UdjinM6: utACK 7089045 Tree-SHA512: 29c46be91efa384860be9560b95f49835c7c3dfbbeae8ce6095fcc6ace336ed7d38e8aa7c9a93843c350b95a489562fddfb2bb61a74798cc0c4955ac3d9d62cf
Additional Information
Breaking Changes
GCC 11.1 or later, or Clang 16.0 or later, are now required to compile Dash Core.
Checklist: