backport: merge bitcoin#24681, #25244, #24291, #25687, #25813, #25852, #26056, #26089, #26086, #26099, #26833, #25974, #27057, #27027, #27118, #27406, #27387, #27696 (build backports: part 2) #6735
Conversation
WalkthroughThis set of changes updates the build system, dependency management, and some internal testing infrastructure. The build configuration scripts and Makefiles now support new flags for selectively disabling Boost, libevent, and hardening features, and the minimum required libevent version is raised to 2.1.8 throughout the codebase and documentation. Compiler and linker flags are reorganized, especially regarding Boost and hardening macros, and the handling of ZMQ and fuzzing build targets is refined. Several test and utility files are updated to use a new shared JSON parsing utility, centralizing the ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
src/test/util/json.h (1)
5-14: Header is fine ‒ one minor nitpick aboutread_jsonvisibilityEverything here is concise and correct. Consider adding
[[nodiscard]]toread_jsonso accidental ignoring of the return value is caught at compile-time.-UniValue read_json(const std::string& jsondata); +[[nodiscard]] UniValue read_json(const std::string& jsondata);src/test/key_io_tests.cpp (1)
12-12: Include path is correct, but ordering is now slightly offProject headers (
<test/util/json.h>) usually go after system/third-party headers and before the test framework include.
Consider moving this include below the Boost include block to keep ordering consistent with the rest of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
configure.ac(15 hunks)depends/Makefile(5 hunks)depends/README.md(2 hunks)depends/config.site.in(1 hunks)depends/gen_id(2 hunks)depends/packages/libevent.mk(1 hunks)depends/packages/packages.mk(1 hunks)doc/dependencies.md(1 hunks)src/Makefile.am(8 hunks)src/Makefile.bench.include(3 hunks)src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(1 hunks)src/Makefile.test.include(5 hunks)src/Makefile.test_fuzz.include(1 hunks)src/Makefile.test_util.include(2 hunks)src/bench/data.cpp(1 hunks)src/bitcoin-cli.cpp(1 hunks)src/httpserver.cpp(2 hunks)src/httpserver.h(1 hunks)src/rpc/misc.cpp(1 hunks)src/test/addrman_tests.cpp(3 hunks)src/test/base58_tests.cpp(1 hunks)src/test/evo_trivialvalidation.cpp(1 hunks)src/test/fuzz/http_request.cpp(0 hunks)src/test/governance_validators_tests.cpp(1 hunks)src/test/key_io_tests.cpp(1 hunks)src/test/script_tests.cpp(1 hunks)src/test/sighash_tests.cpp(1 hunks)src/test/transaction_tests.cpp(1 hunks)src/test/util/json.cpp(1 hunks)src/test/util/json.h(1 hunks)src/wallet/test/bip39_tests.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/fuzz/http_request.cpp
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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: kwvg
PR: dashpay/dash#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.
src/test/base58_tests.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#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.
src/test/transaction_tests.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#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: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/bench/data.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
depends/packages/libevent.mk (1)
Learnt from: kwvg
PR: dashpay/dash#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.
src/test/sighash_tests.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#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: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
src/test/governance_validators_tests.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#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.
depends/config.site.in (1)
Learnt from: kwvg
PR: dashpay/dash#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.
src/Makefile.test_fuzz.include (2)
Learnt from: kwvg
PR: dashpay/dash#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.
Learnt from: kwvg
PR: dashpay/dash#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.
depends/gen_id (1)
Learnt from: kwvg
PR: dashpay/dash#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.
src/wallet/test/bip39_tests.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#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.
depends/packages/packages.mk (1)
Learnt from: kwvg
PR: dashpay/dash#6516
File: ci/test/00_setup_env_mac.sh:11-11
Timestamp: 2025-01-02T08:33:26.751Z
Learning: The removal of DMG support in the macOS packaging process eliminates the need for python-based scripts or python3-setuptools in the build environment. The PACKAGES variable in ci/test/00_setup_env_mac.sh is vestigial due to the use of a general-purpose Docker container defined in contrib/containers/ci/Dockerfile.
depends/README.md (1)
Learnt from: kwvg
PR: dashpay/dash#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.
src/Makefile.bench.include (1)
Learnt from: kwvg
PR: dashpay/dash#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.
src/Makefile.test.include (2)
Learnt from: kwvg
PR: dashpay/dash#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: kwvg
PR: dashpay/dash#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.
src/test/addrman_tests.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
src/bitcoin-cli.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#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: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/Makefile.am (2)
Learnt from: kwvg
PR: dashpay/dash#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: kwvg
PR: dashpay/dash#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.
depends/Makefile (2)
Learnt from: kwvg
PR: dashpay/dash#6516
File: ci/test/00_setup_env_mac.sh:11-11
Timestamp: 2025-01-02T08:33:26.751Z
Learning: The removal of DMG support in the macOS packaging process eliminates the need for python-based scripts or python3-setuptools in the build environment. The PACKAGES variable in ci/test/00_setup_env_mac.sh is vestigial due to the use of a general-purpose Docker container defined in contrib/containers/ci/Dockerfile.
Learnt from: kwvg
PR: dashpay/dash#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.
configure.ac (1)
Learnt from: kwvg
PR: dashpay/dash#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.
🧬 Code Graph Analysis (1)
src/rpc/misc.cpp (2)
src/httpserver.cpp (2)
UpdateHTTPServerLogging(447-453)UpdateHTTPServerLogging(447-447)src/httpserver.h (1)
UpdateHTTPServerLogging(35-35)
🪛 GitHub Actions: Clang Diff Format Check
src/test/evo_trivialvalidation.cpp
[error] 9-16: Clang format differences found. The file does not conform to the expected code style. Please run clang-format to fix formatting issues.
🪛 LanguageTool
depends/README.md
[uncategorized] ~129-~129: Loose punctuation mark.
Context: ...lated options to *FLAGS. - NO_HARDEN=1: Don't use hardening options when buildi...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (49)
doc/dependencies.md (1)
21-21: LGTM – dependency version bump looks consistentThe minimum-required libevent version is now 2.1.8, matching the checks in
configure.acand the removed conditional code. No further action needed.src/test/governance_validators_tests.cpp (1)
9-9: Good change – centralised JSON helper usedSwitching to
<test/util/json.h>removes duplicate forward declarations and keeps tests consistent.src/test/base58_tests.cpp (1)
8-8: LGTM! Good refactoring to centralize JSON utilities.This change properly consolidates the
read_jsonfunction declaration into a shared header, eliminating redundant forward declarations across multiple test files.src/test/sighash_tests.cpp (1)
13-13: LGTM! Consistent with JSON utility centralization.This change follows the same pattern as other test files, properly using the centralized
read_jsonfunction from the shared header.src/Makefile.test_fuzz.include (1)
17-17: Build flag standardization looks good.This change aligns the fuzz test build environment with other test builds by using
BOOST_CPPFLAGSinstead of the previous combination of specific flags. This standardization should improve build consistency.Please verify that fuzz tests still compile correctly after this flag change:
#!/bin/bash # Description: Check if fuzz test targets can be built successfully # Expected: Clean compilation without errors # Check if fuzz test library can be built make libtest_fuzz.a 2>&1 | grep -E "(error|Error|ERROR)" || echo "Fuzz test library builds successfully"src/bench/data.cpp (1)
11-11: Namespace simplification looks clean.Removing the
raw_bench::qualifier simplifies the code. This change aligns with the build system updates for raw data handling mentioned in the AI summary.Please verify that
block813851_rawis still properly accessible after removing the namespace qualifier:#!/bin/bash # Description: Check if benchmark data compiles correctly # Expected: Clean compilation without undefined symbol errors # Check if benchmark data compiles make src/bench/data.o 2>&1 | grep -E "(undefined|error|Error|ERROR)" || echo "Benchmark data compiles successfully"depends/packages/libevent.mk (1)
20-22: LGTM! Proper conditional hardening implementation.This change correctly implements conditional security hardening by adding
_FORTIFY_SOURCE=3only when hardening is not disabled. The conditional logic is properly structured and aligns with the build system enhancements mentioned in the PR objectives.depends/config.site.in (2)
81-83: LGTM: Hardening conditional follows established pattern.The new conditional block for
enable_hardeningfollows the same pattern as other feature toggles in this file and integrates properly with the broader build system changes for theNO_HARDENflag.
87-89: LGTM: PKG_CONFIG variable changes align with build system updates.The removal of
exportkeywords fromPKG_CONFIG_PATHandPKG_CONFIG_LIBDIRappears intentional and coordinated with theconfigure.acchanges mentioned in the summary that improve handling of these variables.src/test/transaction_tests.cpp (1)
21-21: LGTM: JSON utility consolidation improves code organization.The inclusion of
<test/util/json.h>properly centralizes theread_jsonfunction declaration, eliminating the need for manual forward declarations and improving maintainability across test files.depends/gen_id (2)
5-5: LGTM: Usage comment properly updated.The usage comment correctly includes the new
NO_HARDENenvironment variable alongside existing options.
72-74: LGTM: NO_HARDEN block follows established pattern.The new
NO_HARDENoutput block follows the exact same structure as the existingLTOblock and properly includes the variable in build ID generation for cache invalidation purposes.src/wallet/test/bip39_tests.cpp (1)
10-10: LGTM: Consistent JSON utility consolidation.The inclusion of
<test/util/json.h>follows the same consolidation pattern applied across multiple test files, properly centralizing theread_jsonfunction declaration.src/Makefile.test_util.include (2)
13-13: LGTM: JSON utility files properly added to build.The addition of
test/util/json.handtest/util/json.cppto their respective lists properly supports the JSON utility consolidation effort across test files.Also applies to: 32-32
27-27: LGTM: CPPFLAGS change aligns with Boost standardization.The change from
MINIUPNPC_CPPFLAGS,EVENT_CFLAGS, andEVENT_PTHREADS_CFLAGStoBOOST_CPPFLAGSaligns with the broader build system standardization effort mentioned in the summary.src/httpserver.h (1)
34-35: LGTM: Function signature simplification aligns with libevent version requirement updates.The change from
booltovoidreturn type removes conditional error handling that was previously needed for older libevent versions. This simplification is appropriate given the PR's objective to raise the minimum libevent version requirement and remove version-dependent code paths.src/Makefile.qttest.include (1)
32-33: LGTM: Consistent addition of Boost compilation flags.Adding
$(BOOST_CPPFLAGS)ensures Boost headers and macros are available during Qt test compilation, aligning with the broader build system updates that consistently add Boost flags across multiple Makefiles.src/test/util/json.cpp (1)
12-17: LGTM: Well-implemented JSON utility function for tests.The function correctly:
- Parses JSON using UniValue::read()
- Validates both parsing success and array type in a single assertion
- Returns the extracted array
This centralization of JSON parsing logic improves maintainability by eliminating redundant code across multiple test files.
depends/README.md (2)
116-117: LGTM: Clear documentation for new build flags.The documentation for
NO_NATPMPandNO_USDTflags follows the established format and clearly explains their purpose.
129-129: LGTM: Appropriate documentation for hardening control flag.The
NO_HARDEN=1documentation is clear and consistent with other build options. The static analysis hint about punctuation appears to be a false positive - the colon formatting is appropriate for this list context.src/test/addrman_tests.cpp (3)
558-558: LGTM: AS map data reference update.The change from
raw_tests::asmaptoasmap_rawaligns with the broader refactoring of raw data handling mentioned in the build system updates. The test logic remains unchanged.
612-612: LGTM: Consistent AS map data reference update.This change maintains consistency with the AS map data naming updates across the test cases.
690-690: LGTM: Final AS map data reference update.Completes the consistent renaming of AS map data references throughout the addrman serialization tests.
src/Makefile.qt.include (1)
374-374: LGTM: Systematic Boost flags integrationThis addition of
$(BOOST_CPPFLAGS)to the Qt library compilation flags is consistent with the broader systematic integration of Boost compilation flags across build components mentioned in the AI summary. The change ensures proper preprocessor flag availability for Qt GUI components.src/test/script_tests.cpp (1)
15-15: LGTM: JSON utility consolidationThe inclusion of
<test/util/json.h>replaces the localread_jsonfunction implementation with a centralized utility. This consolidation improves code reuse and maintainability across test files.depends/packages/packages.mk (1)
1-5: LGTM: Enhanced dependency configurabilityThe refactoring of package variables effectively separates
boostandlibeventinto dedicated variables (boost_packagesandlibevent_packages) while removing them from the mainpackageslist. This enables conditional inclusion/exclusion via the newNO_BOOSTandNO_LIBEVENTbuild flags, providing enhanced build system configurability.src/bitcoin-cli.cpp (1)
776-778: LGTM: Style improvement for consistency.The change from a single-line if statement to a block with braces improves code consistency and maintainability without affecting functionality.
src/rpc/misc.cpp (1)
1251-1251: LGTM: Consistent with libevent modernization effort.The removal of error handling around
UpdateHTTPServerLogging()aligns with the function signature change frombooltovoid. This simplification is appropriate given the raised minimum libevent version requirement.src/Makefile.test.include (4)
10-12: LGTM: Improved test inclusion logic.The change from
!ENABLE_FUZZtoENABLE_TESTSmakes the test inclusion condition more explicit and semantically correct, removing the implicit negative dependency on fuzzing.Also applies to: 409-428
237-237: LGTM: Target-specific Boost flags addition.Adding
$(BOOST_CPPFLAGS)to specific test targets aligns with the broader refactoring that moves from global to target-specific compiler flags, providing better dependency control.Also applies to: 256-256
241-241: LGTM: Wallet-specific BDB flags.Conditionally adding
$(BDB_CPPFLAGS)when wallet is enabled follows the pattern of making build dependencies more targeted and explicit.
445-453: LGTM: Generalized JSON header generation rule.Removing the
test/data/prefix from the pattern rule makes it more flexible and allows it to work with JSON files in any location, improving build system maintainability.src/Makefile.bench.include (2)
60-82: LGTM: Enhanced benchmark build configuration.The addition of
$(BOOST_CPPFLAGS)and comprehensive linker flags/libraries aligns with the systematic refactoring to make build dependencies more explicit and target-specific.
92-92: LGTM: Conditional wallet library inclusion.Adding wallet-specific libraries only when
ENABLE_WALLETis set maintains consistency with the targeted dependency approach used throughout the build system.src/httpserver.cpp (1)
403-404: LGTM: Simplified libevent logging integration.The removal of conditional compilation and error handling in
UpdateHTTPServerLoggingappropriately reflects the updated minimum libevent version requirement (2.1.8), eliminating legacy compatibility code.Also applies to: 447-453
src/Makefile.am (3)
41-41: LGTM: Systematic flag redistribution for better dependency control.Moving
$(BOOST_CPPFLAGS)and$(BDB_CPPFLAGS)from globalBITCOIN_INCLUDESto specific library targets provides clearer dependency management and better build organization. The conditional inclusion based on feature flags is appropriate.Also applies to: 439-439, 571-571, 578-578, 591-591, 634-634, 732-732, 814-814
1099-1107: LGTM: Centralized raw file header generation.Adding the centralized
.raw.hpattern rule improves maintainability by consolidating binary-to-header conversion logic that was previously duplicated across multiple makefiles.
1116-1116: LGTM: Simplified test makefile inclusion.Unconditional inclusion of
Makefile.test.includeis appropriate since test building is now controlled by theENABLE_TESTSflag within the test makefile itself.depends/Makefile (3)
35-36: LGTM: New optional build configuration flags.Adding
NO_BOOST,NO_LIBEVENT, andNO_HARDENflags with empty defaults provides useful build flexibility while maintaining backward compatibility.Also applies to: 48-48
150-155: LGTM: Systematic integration of conditional build flags.The integration of
NO_HARDENinto build ID generation and conditional package selection for boost/libevent provides the intended configurability while maintaining proper build identity tracking.Also applies to: 172-172
252-252: LGTM: Configuration template integration.Adding the
@no_harden@substitution ensures theNO_HARDENflag is properly propagated to the autotools configuration system viaconfig.site.configure.ac (8)
22-31: LGTM! Good fix for the config.site variable handling.The approach of prefixing
PKG_CONFIGwithenvto preserve the environment variables is correct and ensures--config-cacheworks properly with the depends build system.
489-498: LGTM! Appropriate compiler-specific warning suppression.The conditional suppression of
-Wcppwarnings for GCC 11 and below is necessary to support_FORTIFY_SOURCE=3while maintaining-Werrorcompatibility. The compiler detection logic correctly targets only affected GCC versions.
590-590: LGTM! Correct flag ordering for CPU feature detection.Prepending CPU feature flags ensures they take precedence over existing CXXFLAGS, which is the correct approach for feature detection tests.
Also applies to: 612-612, 627-627, 642-642, 663-663, 682-682
1354-1354: LGTM! Consistent with fuzzing build configuration.Explicitly disabling tests when fuzzing is enabled ensures a clean fuzzing build without conflicting targets.
1488-1488: LGTM! Updated libevent requirements.The minimum version bump to 2.1.8 and the inclusion of
enable_fuzz_binaryin the condition check are appropriate updates that align with the codebase requirements.Also applies to: 1549-1552
1608-1609: LGTM! Improved code organization.Moving the
AM_CONDITIONALforENABLE_ZMQimmediately after the ZMQ check improves code readability by keeping related logic together.
1819-1819: LGTM! Consistent target checking.Including
enable_fuzz_binaryin the "No targets" check is consistent with the earlier changes and ensures fuzz binary builds are properly recognized as valid targets.
1041-1046: LGTM! Enhanced security hardening with _FORTIFY_SOURCE=3.Upgrading to
_FORTIFY_SOURCE=3provides additional runtime security checks. This aligns with the-Wcppsuppression added earlier for GCC 11 compatibility.What are the differences between _FORTIFY_SOURCE levels 2 and 3 in GCC?
src/test/evo_trivialvalidation.cpp
Outdated
| #include <evo/providertx.h> | ||
| #include <primitives/transaction.h> | ||
|
|
||
| #include <test/util/json.h> |
There was a problem hiding this comment.
clang-format failure – run formatter on this hunk
CI flagged a formatting difference in this area. Running git clang-format (or clang-format -i src/test/evo_trivialvalidation.cpp) should auto-fix include ordering/spacing and unblock the pipeline.
🤖 Prompt for AI Agents
In src/test/evo_trivialvalidation.cpp at line 12, the include directive
formatting does not comply with clang-format standards. Run the clang-format
tool on this file or specifically on this line using `clang-format -i
src/test/evo_trivialvalidation.cpp` to automatically fix the include ordering
and spacing issues and ensure the code style matches the project's formatting
rules.
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
depends/README.md (2)
116-118: Alphabetical ordering nitpick
NO_NATPMPandNO_USDTare correctly documented, but they break the alphabetical ordering of theNO_*options list, making future look-ups slightly harder. Consider re-ordering the bullets to keep the list sorted.... - `NO_UPNP`: Don't download/build/cache packages needed for enabling UPnP +# Keep list alphabetical +- `NO_USDT`: Don't download/build/cache packages needed for enabling USDT tracepoints - `NO_NATPMP`: Don't download/build/cache packages needed for enabling NAT-PMP ...
129-129: Inconsistent flag syntaxAll other boolean flags are documented without an explicit
=1suffix, e.g.NO_QT,NO_UPNP, etc.
NO_HARDEN=1deviates from this pattern, which can be confusing because the surrounding text already states “can be set when running make:make FOO=bar”. Align with the existing convention for consistency:- `NO_HARDEN=1`: Don't use hardening options when building packages + `NO_HARDEN`: Don't use hardening options when building packages(If the implementation truly requires
=1, consider adding a short note such as “(value must be 1)” instead.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
configure.ac(15 hunks)depends/Makefile(5 hunks)depends/README.md(2 hunks)depends/config.site.in(1 hunks)depends/gen_id(2 hunks)depends/packages/libevent.mk(1 hunks)depends/packages/packages.mk(1 hunks)doc/dependencies.md(1 hunks)src/Makefile.am(8 hunks)src/Makefile.bench.include(3 hunks)src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(1 hunks)src/Makefile.test.include(5 hunks)src/Makefile.test_fuzz.include(1 hunks)src/Makefile.test_util.include(2 hunks)src/bench/data.cpp(1 hunks)src/bitcoin-cli.cpp(1 hunks)src/httpserver.cpp(2 hunks)src/httpserver.h(1 hunks)src/rpc/misc.cpp(1 hunks)src/test/addrman_tests.cpp(3 hunks)src/test/base58_tests.cpp(1 hunks)src/test/evo_trivialvalidation.cpp(1 hunks)src/test/fuzz/http_request.cpp(0 hunks)src/test/governance_validators_tests.cpp(1 hunks)src/test/key_io_tests.cpp(1 hunks)src/test/script_tests.cpp(1 hunks)src/test/sighash_tests.cpp(1 hunks)src/test/transaction_tests.cpp(1 hunks)src/test/util/json.cpp(1 hunks)src/test/util/json.h(1 hunks)src/wallet/test/bip39_tests.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/fuzz/http_request.cpp
✅ Files skipped from review due to trivial changes (3)
- src/test/key_io_tests.cpp
- src/test/governance_validators_tests.cpp
- src/test/base58_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (27)
- src/test/evo_trivialvalidation.cpp
- doc/dependencies.md
- depends/config.site.in
- src/Makefile.qttest.include
- src/test/util/json.h
- src/test/sighash_tests.cpp
- src/test/script_tests.cpp
- src/bench/data.cpp
- depends/packages/libevent.mk
- src/test/addrman_tests.cpp
- src/wallet/test/bip39_tests.cpp
- src/Makefile.test_fuzz.include
- src/Makefile.test_util.include
- src/test/transaction_tests.cpp
- depends/gen_id
- src/httpserver.h
- src/Makefile.bench.include
- src/test/util/json.cpp
- src/rpc/misc.cpp
- src/Makefile.test.include
- src/Makefile.qt.include
- depends/packages/packages.mk
- src/httpserver.cpp
- depends/Makefile
- src/bitcoin-cli.cpp
- src/Makefile.am
- configure.ac
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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: kwvg
PR: dashpay/dash#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.
depends/README.md (1)
Learnt from: kwvg
PR: dashpay/dash#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.
🪛 LanguageTool
depends/README.md
[uncategorized] ~129-~129: Loose punctuation mark.
Context: ...lated options to *FLAGS. - NO_HARDEN=1: Don't use hardening options when buildi...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6735.a4acf951. 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-devpr6735.a4acf951. The image should be on dockerhub soon. |
Should match convention established by bitcoin#16702
When we split out test_fuzz and test_util, NAT-PMP support wasn't available yet, so we never had those unused CPPFLAGS, so here we remove the unused miniUPnPc flags that we have instead.
Let's also move `qt_linux_packages` downward to match upstream ordering
Also, move `NO_NATPMP` to correct position to match upstream
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6735.bad71590. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
depends/README.md (2)
116-118: Document-style nit: keep wording consistent with existingNO_*togglesEvery other boolean toggle is documented as
`NO_FOOBAR`: <explanation>without showing an explicit=1. For the sake of uniformity, consider wording the two new entries the same way:-`NO_NATPMP`: Don't download/build/cache packages needed for enabling NAT-PMP -`NO_USDT`: Don't download/build/cache packages needed for enabling USDT tracepoints +`NO_NATPMP`: Don't download/build/cache packages needed for enabling NAT-PMP +`NO_USDT`: Don't download/build/cache packages needed for enabling USDT trace-points(No functional change—purely documentation polish.)
129-129: Rename bullet toNO_HARDENfor consistencyAll other feature-gate options are introduced without the
=1suffix, e.g.NO_QT,NO_UPNP, etc. Dropping the literal assignment keeps the list visually uniform while still allowingmake NO_HARDEN=1in practice:-`NO_HARDEN=1`: Don't use hardening options when building packages +`NO_HARDEN`: Don't use hardening options when building packages (set to `1` to disable hardening, e.g. `make NO_HARDEN=1`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
configure.ac(15 hunks)depends/Makefile(5 hunks)depends/README.md(2 hunks)depends/config.site.in(1 hunks)depends/gen_id(2 hunks)depends/packages/libevent.mk(1 hunks)depends/packages/packages.mk(1 hunks)src/Makefile.am(8 hunks)src/Makefile.bench.include(3 hunks)src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(1 hunks)src/Makefile.test.include(5 hunks)src/Makefile.test_fuzz.include(1 hunks)src/Makefile.test_util.include(2 hunks)src/bench/data.cpp(1 hunks)src/test/addrman_tests.cpp(3 hunks)src/test/base58_tests.cpp(1 hunks)src/test/evo_trivialvalidation.cpp(1 hunks)src/test/governance_validators_tests.cpp(1 hunks)src/test/key_io_tests.cpp(1 hunks)src/test/script_tests.cpp(1 hunks)src/test/sighash_tests.cpp(1 hunks)src/test/transaction_tests.cpp(1 hunks)src/test/util/json.cpp(1 hunks)src/test/util/json.h(1 hunks)src/wallet/test/bip39_tests.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- src/test/governance_validators_tests.cpp
- src/test/key_io_tests.cpp
- src/wallet/test/bip39_tests.cpp
- src/test/sighash_tests.cpp
- src/test/base58_tests.cpp
- src/test/transaction_tests.cpp
- src/test/addrman_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (18)
- depends/packages/libevent.mk
- src/test/evo_trivialvalidation.cpp
- src/Makefile.test_fuzz.include
- depends/config.site.in
- src/bench/data.cpp
- depends/gen_id
- src/test/util/json.h
- src/Makefile.test_util.include
- src/Makefile.qttest.include
- src/test/util/json.cpp
- src/test/script_tests.cpp
- depends/packages/packages.mk
- src/Makefile.qt.include
- src/Makefile.bench.include
- src/Makefile.test.include
- depends/Makefile
- src/Makefile.am
- configure.ac
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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: kwvg
PR: dashpay/dash#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.
depends/README.md (1)
Learnt from: kwvg
PR: dashpay/dash#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.
🪛 LanguageTool
depends/README.md
[uncategorized] ~129-~129: Loose punctuation mark.
Context: ...lated options to *FLAGS. - NO_HARDEN=1: Don't use hardening options when buildi...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: check_merge
- GitHub Check: predict_conflicts
- GitHub Check: predict_conflicts
- GitHub Check: Build slim container / Build container
- GitHub Check: Build container / Build container
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6735.bad71590. The image should be on dockerhub soon. |
…itcoin#26773, bitcoin#30989, bitcoin#32498, bitcoin#32439, bitcoin#32678, bitcoin#33073, update channels, substitution servers, fix `guix-check` 18d9966 ci: make `guix-check` print out the set of hashes compared against (Kittywhiskers Van Gogh) 90f2581 ci: add mirrors to Bordeaux substitution server (Kittywhiskers Van Gogh) 0ae477b ci: add `berlin.guix.gnu.org` to substitute URLs list (Kittywhiskers Van Gogh) 4891135 ci: ensure that Codeberg is used as the `guix` channel (Kittywhiskers Van Gogh) 24b390d merge bitcoin#33073: warn SOURCE_DATE_EPOCH set in guix-codesign (Kittywhiskers Van Gogh) f976956 merge bitcoin#32678: warn and abort when SOURCE_DATE_EPOCH is set (Kittywhiskers Van Gogh) 2e0b209 merge bitcoin#32439: accomodate migration to codeberg (Kittywhiskers Van Gogh) a2d0e4a merge bitcoin#32498: remove Carls substitute server from Guix docs (Kittywhiskers Van Gogh) 2e0e44a merge bitcoin#30989: Drop no longer needed PATH modification (Kittywhiskers Van Gogh) 2d2f220 merge bitcoin#26773: FreeBSD build doc updates to reflect removal of install_db4.sh (Kittywhiskers Van Gogh) 12e04f5 merge bitcoin#26834: remove install_db4.sh (Kittywhiskers Van Gogh) f3f2fd3 doc: update disclaimer in `build-netbsd.md` (Kittywhiskers Van Gogh) 2ca9984 merge bitcoin#24585: mention that BDB is for the legacy wallet in build-osx.md (Kittywhiskers Van Gogh) 080d4b2 docs: remove extra Berkeley DB fragment in `build-osx.md` (Kittywhiskers Van Gogh) Pull request description: ## Motivation Annoyances from trying to build rc1 and rc2 using our Guix container ([source](https://github.com/dashpay/dash/blob/f170aed19ec399ea68ddf003a39e22be7c5d6e8e/contrib/containers/guix/Dockerfile)). ## Additional Information * Guix has migrated their service provider to Codeberg and have published a timeline for sunsetting the existing `git.savannah.gnu.org` channel ([blog](https://guix.gnu.org/en/blog/2025/migrating-to-codeberg/)). This requires updating our scripts and on the upstream side this was achieved with [bitcoin#32439](bitcoin#32439) but requires additional changes on our end as well. * As we rely on Ubuntu's distribution of Guix and staleness is a known problem (Debian no longer ships Guix with `trixie` and have flagged the `guix` package as subject to removal, [source](https://lwn.net/Articles/1035491/)), we need to set Codeberg as the channel source ourselves. * `install_db4.sh` was dropped via [bitcoin#26834](bitcoin#26834) as [bitcoin#26833](bitcoin#26833) was backported (see [dash#6735](#6735)) and the script points to the old `git.savannah.gnu.org` for 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](https://libreplanet.org/wiki/Group:Guix/Mirrors)) 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-check` so far used an ad-hoc method of generating checksums that cannot be (trivially) diffed against the attested checksums provided at [`dashpay/guix.sigs`](https://github.com/dashpay/guix.sigs), by leveraging `guix-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](https://github.com/dashpay/dash/blob/f170aed19ec399ea68ddf003a39e22be7c5d6e8e/contrib/guix/guix-attest#L101-L104)). ## Breaking Changes None. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: ACK 18d9966 Tree-SHA512: 9964b957aacd85b7e193c3019656ea70b8987031bb6ed6189c5933b24f47346fff01a4e37ac23314f802a2084e96a91443e309d5b7b83c79940a5d3202134be2
5 participants
Additional Information
In order to allow backporting bitcoin#27027,
-Werror=cpphas been suppressed for GCC 11 as_FORTIFY_SOURCE=3was introduced in GCC 12 (source) and thelinux64_sqlitebuild variant errored (build) due to our enablement of-Werror.The warning emitted by GCC 11 has been acknowledged in the comments of bitcoin#27027 as well (comment).
Breaking Changes
None expected.
Checklist