fix: resolve or sidestep compiler warnings, re-enable -Werror for Clang, merge bitcoin#13633
#6638
Conversation
WalkthroughThis set of changes includes updates to both build/test scripts and various C++ source files. In the CI environment setup scripts, the export of the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (27)
💤 Files with no reviewable changes (8)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (15)
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:
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.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/wallet/wallet.cpp (1)
3705-3709:⚠️ Potential issueCode conditionally disabled with a hardcoded false expression
This change effectively disables the code block by using
/* DISABLES CODE */ (false) &&at the beginning of the condition. This means the HD wallet upgrade restriction that previously forced users to use theupgradetohdRPC is now bypassed.This approach of disabling code with a hardcoded false condition is not optimal for long-term maintainability. Consider either:
- Properly removing the code if it's no longer needed
- Adding a clear comment explaining why this code is being disabled rather than removed
- Using a configuration flag if this is meant to be conditionally enabled/disabled
The current approach makes it unclear whether this is a temporary change or a permanent one, and future developers might be confused about the intention.
🧹 Nitpick comments (6)
src/script/interpreter.cpp (1)
1524-1524: Appropriate use of [[maybe_unused]] attribute to suppress warningAdding the
[[maybe_unused]]attribute to theHandleMissingDatastatic function is appropriate as this function appears to be a utility that might not be directly called in all code paths. This change helps suppress compiler warnings while keeping the function available for use.Note that this function doesn't have a return statement for all paths. When the
MissingDataBehaviorvalue is not one of the known cases, it will assert but not return a value. Modern compilers would flag this issue.You might want to add a return statement after the assert to ensure all code paths return a value:
[[maybe_unused]] static bool HandleMissingData(MissingDataBehavior mdb) { switch (mdb) { case MissingDataBehavior::ASSERT_FAIL: assert(!"Missing data"); break; case MissingDataBehavior::FAIL: return false; } assert(!"Unknown MissingDataBehavior value"); + return false; // Unreachable, but ensures all paths return a value }src/wallet/scriptpubkeyman.cpp (1)
222-224: Optional refactor: simplify HD chain check
The condition(m_hd_chain.IsNull() || (!m_hd_chain.IsNull() && !m_hd_chain.IsCrypted()))is logically equivalent to
!m_hd_chain.IsCrypted(). You can simplify theifto:-if (!keyPass && !accept_no_keys && (m_hd_chain.IsNull() || (!m_hd_chain.IsNull() && !m_hd_chain.IsCrypted()))) { +if (!keyPass && !accept_no_keys && !m_hd_chain.IsCrypted()) { return false; }This retains the same behavior while improving readability.
configure.ac (3)
213-218: Flag inversion may break existing automation – keep the old spelling as an aliasThe option now defaults to “suppress” and exposes
--disable‑suppress‑external‑warnings. CI scripts were updated, but third‑party builds that still pass the old--enable‑suppress‑external‑warningswill silently do the opposite of what they intend.Consider mapping the legacy flag to the new semantics:
AC_ARG_ENABLE([suppress-external-warnings-old-alias], [AS_HELP_STRING([--enable-suppress-external-warnings], [DEPRECATED: retained for backwards compatibility])], [suppress_external_warnings=yes])This keeps the new default while avoiding accidental warning explosions in downstream builds.
460-482: Large unconditional warning list: verify configure time on older toolchainsMoving the
AX_CHECK_COMPILE_FLAGblock outside anyenable_werrorguard means every configure run now probes ~20 extra flags. While feature‑tested, this slows configuration noticeably on slower CI runners and very old compilers.If the intent is merely to enable these warnings when available, you can:
- Guard the expensive probes behind
if test "$CXXFLAG_WERROR" != ""(Clang/GCC only), or- Cache results via
AS_VAR_APPEND([WARN_CACHED_FLAGS], …)to avoid re‑testing in re‑runs.Not critical, but worth a follow‑up to keep configure snappy.
488-490: Default path now hides-Wdeprecated-copydiagnosticsBecause suppression is now on by default, the
-Wno-deprecated-copyexemption will not be added, and external C++17‑incompatible headers (e.g. Boost < 1.80) may spew warnings that break--enable-werrorbuilds.If that flood becomes noisy again, you may want to invert the predicate:
if test "$suppress_external_warnings" = "yes"; then NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-copy" fiJust something to keep in mind.
ci/test/00_setup_env_native_nowallet.sh (1)
15-15: NO_WERROR flag introduced – remove duplicated compiler overrides
NO_WERROR=1is a sensible addition for faster green builds.
A minor clean‑up:CC=gcc-14 CXX=g++-14is specified both inDEP_OPTSand inBITCOIN_CONFIG. Unless the double export is intentional, trimming one copy avoids confusion when investigators grep for the active compiler.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
ci/dash/build_src.sh(1 hunks)ci/test/00_setup_env_arm.sh(1 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/00_setup_env_native_fuzz_with_valgrind.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)ci/test/00_setup_env_native_nowallet.sh(1 hunks)ci/test/00_setup_env_native_qt5.sh(1 hunks)ci/test/00_setup_env_native_sqlite.sh(1 hunks)ci/test/00_setup_env_native_valgrind.sh(1 hunks)ci/test/00_setup_env_win64.sh(1 hunks)configure.ac(2 hunks)doc/developer-notes.md(1 hunks)src/coinjoin/client.cpp(4 hunks)src/coinjoin/client.h(2 hunks)src/coinjoin/coinjoin.h(1 hunks)src/evo/creditpool.cpp(1 hunks)src/evo/creditpool.h(1 hunks)src/evo/deterministicmns.cpp(0 hunks)src/evo/deterministicmns.h(1 hunks)src/node/chainstate.cpp(1 hunks)src/node/coinstats.cpp(0 hunks)src/qt/networkstyle.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/script/bitcoinconsensus.cpp(1 hunks)src/script/interpreter.cpp(1 hunks)src/script/sign.cpp(1 hunks)src/test/script_tests.cpp(1 hunks)src/versionbits.h(1 hunks)src/wallet/hdchain.h(1 hunks)src/wallet/rpcwallet.cpp(2 hunks)src/wallet/scriptpubkeyman.cpp(1 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/walletdb.cpp(1 hunks)
💤 Files with no reviewable changes (2)
- src/evo/deterministicmns.cpp
- src/node/coinstats.cpp
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/script/bitcoinconsensus.cpp (3)
src/script/interpreter.cpp (2)
maybe_unused(85-95)maybe_unused(1524-1534)src/test/script_tests.cpp (3)
maybe_unused(285-289)maybe_unused(291-295)maybe_unused(297-301)src/streams.h (1)
m_version(163-163)
src/wallet/spend.cpp (3)
src/wallet/coinselection.cpp (2)
SelectCoinsBnB(66-171)SelectCoinsBnB(66-66)src/wallet/coinselection.h (1)
SelectCoinsBnB(182-182)src/wallet/wallet.h (1)
nTargetValue(550-550)
src/test/script_tests.cpp (2)
src/script/bitcoinconsensus.cpp (1)
maybe_unused(51-51)src/script/interpreter.cpp (2)
maybe_unused(85-95)maybe_unused(1524-1534)
src/node/chainstate.cpp (4)
src/txmempool.h (1)
dmnman(591-591)src/llmq/commitment.h (1)
dmnman(69-70)src/llmq/debug.h (2)
dmnman(80-81)dmnman(93-94)src/dbwrapper.h (1)
make_unique(709-711)
src/script/interpreter.cpp (2)
src/script/bitcoinconsensus.cpp (1)
maybe_unused(51-51)src/test/script_tests.cpp (3)
maybe_unused(285-289)maybe_unused(291-295)maybe_unused(297-301)
src/wallet/wallet.cpp (1)
src/wallet/scriptpubkeyman.cpp (4)
IsHDEnabled(594-598)IsHDEnabled(594-594)IsHDEnabled(2085-2089)IsHDEnabled(2085-2085)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/creditpool.cpp
[error] 235-236: Clang format differences found. Code formatting does not match the expected style.
src/evo/deterministicmns.h
[error] 571-574: Clang format differences found. Code formatting does not match the expected style.
src/coinjoin/client.cpp
[error] 1014-1015: Clang format differences found. Code formatting does not match the expected style.
[error] 1914-1916: Clang format differences found. Code formatting does not match the expected style.
🔇 Additional comments (32)
src/evo/creditpool.h (1)
78-78:✅ Verification successful
Public access to consensus parameters
You've changed
paramsfrom private to public. This seems to be part of refactoring the initialization pattern, as the constructor in the CPP file no longer initializes this member directly.Was this member made public to allow external initialization? If so, please ensure that the member is properly initialized before use.
🏁 Script executed:
#!/bin/bash # Check if this member is initialized anywhere else in the codebase grep -r "creditPoolDiff.*params" --include="*.cpp" .Length of output: 557
Confirm proper initialization of
paramsin CreditPoolDiffThe
paramsmember was made public so that callers ofGetCreditPoolDiffForBlock(...)can inject the consensus parameters at construction time. All existing call sites (e.g. in specialtxman.cpp and miner.cpp) now passchainparams.GetConsensus()orm_consensus_paramsinto that factory function, which in turn initializes theparamsfield.No other direct usages of
creditPoolDiff.paramswere found, and every instantiation path supplies a validConsensus::Paramsreference. No further changes are required here—paramsis consistently set before any use.src/evo/deterministicmns.h (1)
574-574: Simplified CDeterministicMNManager constructorYou've removed the dependency on
CChainState& chainstatefrom the constructor, which reduces coupling between components. This is a good architectural improvement.However, please fix the Clang format issue reported in the pipeline failures for lines 571-574.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 571-574: Clang format differences found. Code formatting does not match the expected style.
src/node/chainstate.cpp (1)
216-216: Updated CDeterministicMNManager constructionThe construction of
CDeterministicMNManagerhas been updated to match the simplified constructor signature, removing the active chainstate argument. This change correctly aligns with the updated constructor definition.src/script/bitcoinconsensus.cpp (1)
51-51: Appropriate use of [[maybe_unused]] attribute to suppress unused function warningAdding the
[[maybe_unused]]attribute to theGetVersion()method prevents compiler warnings about this function not being used while still preserving it for potential future use.src/qt/networkstyle.cpp (1)
27-27: Fix for struct initialization consistencyAdding the missing fifth field
titleAddTextwith an empty string value to the "test" and "regtest" network style entries ensures proper initialization consistent with the struct definition on line 19. This prevents potential undefined behavior or compiler warnings related to incomplete initialization.Also applies to: 29-29
src/wallet/hdchain.h (1)
125-125: Appropriate use of [[maybe_unused]] attribute for class memberAdding the
[[maybe_unused]]attribute to the privatenVersionmember inCHDPubKeyis appropriate since it appears to only be used for serialization but not accessed elsewhere in the class. This change suppresses compiler warnings while maintaining compatibility with serialized data structures.src/wallet/spend.cpp (1)
376-377: Improved clarity by adding comment about BnB algorithm disablementThe code previously had a silent
falsecondition that disabled the Branch and Bound (BnB) coin selection algorithm. This change adds a clear comment explaining why the algorithm is disabled, making the code intention more explicit.src/versionbits.h (1)
110-113: Added proper default destructor and constructor to base classAdding a virtual destructor to
AbstractEHFManageris a good practice since it's a base class for an inheritance hierarchy. This ensures proper cleanup when derived objects are deleted through base class pointers.src/rpc/evo.cpp (1)
183-192:✅ Verification successful
Function moved outside the ENABLE_WALLET conditional block
The
ParseBLSSecretKeyfunction has been moved outside the wallet conditional compilation block, making it available regardless of wallet support. This is appropriate since the function is a utility for parsing BLS secret keys and doesn't depend on wallet functionality.
🏁 Script executed:
#!/bin/bash # Check if the function is used outside wallet contexts rg "ParseBLSSecretKey" --type cpp | grep -v "src/rpc/evo.cpp"Length of output: 59
LGTM:
ParseBLSSecretKeyproperly decoupled from walletNo references to
ParseBLSSecretKeywere found outsidesrc/rpc/evo.cpp, so moving it out of theENABLE_WALLETblock poses no risks.src/test/script_tests.cpp (3)
285-289: Add[[maybe_unused]]toPush(const std::string&)overload
This marks theTestBuilder::Push(const std::string&)method as potentially unused, correctly suppressing Clang’s-Wunused-functionwarnings for this test helper. The attribute placement and syntax are appropriate.
291-295: Add[[maybe_unused]]toPush(const uint256&)overload
The[[maybe_unused]]attribute is correctly applied here to silence warnings for this overload. No functional change is introduced.
297-301: Add[[maybe_unused]]toPush(const CScript&)overload
Consistent with the otherPushoverloads, the attribute placement is correct and effectively suppresses unused-function warnings.src/wallet/rpcwallet.cpp (2)
955-956: Enforce wallet lock at ListReceived entry
AddingAssertLockHeld(wallet.cs_wallet);ensures that at runtime the requiredcs_walletlock is held when enteringListReceived. This complements theEXCLUSIVE_LOCKS_REQUIREDannotation, catching any misuse early.
1034-1037: Annotate and assert lock in addressbook lambda
The inline lambda is now markedEXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)and invokesAssertLockHeld(wallet.cs_wallet);, guaranteeing thread-safety when processing address book entries. This matches the project's lock‑safety conventions.src/script/sign.cpp (1)
242-244: Improved safety by restricting Stacks struct constructionThe Stacks struct has been made more restrictive by explicitly deleting the default and copy constructors, forcing users to only create instances with the explicit constructor from SignatureData.
This change improves type safety by enforcing a specific usage pattern for the Stacks struct. By removing the ability to default-construct or copy Stacks instances, the code prevents potential misuse and makes the intended usage clearer. This aligns with modern C++ practices of making interfaces more explicit and preventing unintended usage patterns.
src/coinjoin/coinjoin.h (1)
319-321: Add virtual destructor to base session class
Addingvirtual ~CCoinJoinBaseSession() = default;ensures that derived session classes are properly destroyed when handled through base-class pointers, preventing potential resource leaks or undefined behavior.ci/test/00_setup_env_native_sqlite.sh (1)
13-15: Disable-Werrorin native SQLite CI environment
ExportingNO_WERROR=1here aligns this script with other CI configurations, preventing warnings from being treated as errors and avoiding build failures due to emerging compiler warnings.ci/test/00_setup_env_native_qt5.sh (1)
18-20: Disable-Werrorin native Qt5 CI environment
SettingNO_WERROR=1ensures consistency with other CI containers and prevents build failures from compiler warnings elevated to errors.ci/test/00_setup_env_native_multiprocess.sh (1)
17-19: Suppress documentation warnings as errors
Appending-Wno-error=documentationtoCXXFLAGSrelaxes strict error treatment for documentation warnings, aligning with the broader effort to disable-Werrorin CI builds.doc/developer-notes.md (2)
217-217: Remove explicit--enable-suppress-external-warningsfrom clang-tidy instructions.The
./configurecommand no longer needs the--enable-suppress-external-warningsflag since suppression is now the default behavior.
221-221: Retain denoising note to reflect default suppression.The description "The output is denoised of errors from external dependencies." remains accurate given the inverted default behavior.
ci/test/00_setup_env_arm.sh (1)
28-29: Drop--enable-suppress-external-warningsand addNO_WERROR.The
BITCOIN_CONFIGhas been updated to remove the explicit suppression flag, andNO_WERROR=1ensures warnings aren’t treated as errors in this ARM CI environment.ci/test/00_setup_env_native_fuzz_with_valgrind.sh (1)
17-17: Omit external warnings suppression in fuzz+Valgrind config.The
BITCOIN_CONFIGnow focuses solely on fuzzing and sanitizer options, dropping the--enable-suppress-external-warningsflag.ci/test/00_setup_env_native_valgrind.sh (1)
14-14: Remove suppression flag from native Valgrind configuration.The
BITCOIN_CONFIGspecification no longer includes--enable-suppress-external-warnings, aligning with the new default behavior.ci/test/00_setup_env_win64.sh (3)
16-18: Document MinGW header workaround context.The added comments clearly explain why specific warnings need suppression on mingw-w64 < 11.0.0, improving future traceability.
19-19: ExtendBITCOIN_CONFIGwith MinGW-specific warning suppressions.Inclusion of
-Wno-return-type,-Wno-error=maybe-uninitialized, and-Wno-error=array-boundsaddresses known header deficiencies during cross-compilation.
21-21: IntroduceNO_WERRORfor Windows CI builds.Exporting
NO_WERROR=1disables treating warnings as errors, ensuring the build passes despite external header warnings.src/coinjoin/client.h (2)
203-206: Refactoring: Simplified constructor signature by removing wallet manager dependency.The constructor signature has been updated to remove the
CoinJoinWalletManager& walletmanparameter, removing the direct dependency on the wallet manager from the client session class. This is part of a broader refactoring effort to simplify the interface and reduce coupling between components.
305-307: Refactoring: Simplified constructor by removing wallet manager dependency.The constructor signature has been updated to remove the
CoinJoinWalletManager& walletmanparameter, which aligns with the same change made to theCCoinJoinClientSessionclass. This reduces coupling and simplifies the interface between components.ci/test/00_setup_env_native_fuzz.sh (1)
18-18: Build configuration simplified by removing suppression flag.The
--enable-suppress-external-warningsflag has been removed from theBITCOIN_CONFIGenvironment variable. This change is part of the broader effort mentioned in the PR description to re-enable-Werror-enabled builds for Clang, which treats warnings as errors during compilation.src/coinjoin/client.cpp (2)
108-109: Removed unnecessary wallet manager indirection in lambda.The lambda no longer captures
thisto use the wallet manager instance, directly using them_walletmanmember variable instead. This simplifies the code and improves readability while maintaining the same functionality.
166-169: Updated constructor implementation to match header change.The constructor implementation has been updated to match the signature change in the header file, removing the
CoinJoinWalletManager& walletmanparameter. This is part of the refactoring to reduce coupling between components.
src/wallet/walletdb.cpp
Outdated
| #ifdef USE_BDB | ||
| return MakeBerkeleyDatabase(path, options, status, error); | ||
| #endif | ||
| #else | ||
| error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path))); | ||
| status = DatabaseStatus::FAILED_BAD_FORMAT; | ||
| return nullptr; | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same remark for Berkeley DB guard
Analogous to the SQLite branch above – consider a dedicated UNSUPPORTED_FORMAT status to avoid conflating build‑time capabilities with data corruption.
src/wallet/walletdb.cpp
Outdated
| if (format == DatabaseFormat::SQLITE) { | ||
| #ifdef USE_SQLITE | ||
| return MakeSQLiteDatabase(path, options, status, error); | ||
| #endif | ||
| #else | ||
| error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); | ||
| status = DatabaseStatus::FAILED_BAD_FORMAT; | ||
| return nullptr; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Gracefully handle unsupported SQLite builds but reconsider status code
The new #else branch prevents undefined‐symbol errors when the wallet is compiled without USE_SQLITE, which is great.
However, returning DatabaseStatus::FAILED_BAD_FORMAT may be misleading for callers – the on‑disk format is perfectly fine, it is the build that lacks the necessary backend. A more explicit status such as FAILED_UNSUPPORTED_FORMAT (or a new enum value) would let front‑ends distinguish “file corrupted” from “feature not built”.
Diff sketch:
- status = DatabaseStatus::FAILED_BAD_FORMAT;
+ status = DatabaseStatus::FAILED_UNSUPPORTED_FORMAT;[tagged accordingly]
🏁 Script executed:
#!/bin/bash
set -e
# Locate the definition of DatabaseStatus
rg -n "enum.*DatabaseStatus" -A20
# Enumerate all current DatabaseStatus values and usages
rg -n "DatabaseStatus::" -A10Length of output: 20199
Introduce an explicit FAILED_UNSUPPORTED_FORMAT status for unsupported database backends
To distinguish “build lacks backend” from “on‐disk format is invalid,” add a new enum value and use it in both the SQLite and Berkeley‐DB unsupported branches, then update the RPC mapping so front‑ends can handle it appropriately.
• In src/wallet/db.h, insert a new status:
enum class DatabaseStatus {
SUCCESS,
FAILED_BAD_PATH,
FAILED_BAD_FORMAT,
+ FAILED_UNSUPPORTED_FORMAT,
FAILED_ALREADY_LOADED,
…
};• In src/wallet/walletdb.cpp, replace FAILED_BAD_FORMAT with FAILED_UNSUPPORTED_FORMAT in both unsupported‐backend branches:
if (format == DatabaseFormat::SQLITE) {
#ifdef USE_SQLITE
return MakeSQLiteDatabase(path, options, status, error);
#else
error = Untranslated(...);
- status = DatabaseStatus::FAILED_BAD_FORMAT;
+ status = DatabaseStatus::FAILED_UNSUPPORTED_FORMAT;
return nullptr;
#endif
}
…
#else // USE_BDB
error = Untranslated(...);
- status = DatabaseStatus::FAILED_BAD_FORMAT;
+ status = DatabaseStatus::FAILED_UNSUPPORTED_FORMAT;
return nullptr;
#endif• In src/wallet/rpcwallet.cpp, add a case for the new status so callers can react differently:
case DatabaseStatus::FAILED_NOT_FOUND:
code = RPC_WALLET_NOT_FOUND;
break;
+ case DatabaseStatus::FAILED_UNSUPPORTED_FORMAT:
+ code = RPC_WALLET_UNSUPPORTED_FORMAT; // or another fitting RPC code
+ break;
case DatabaseStatus::FAILED_ALREADY_LOADED:
code = RPC_WALLET_ALREADY_LOADED;
break;This makes it clear to UIs and RPC clients that the wallet file is valid but the build lacks the necessary backend.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (format == DatabaseFormat::SQLITE) { | |
| #ifdef USE_SQLITE | |
| return MakeSQLiteDatabase(path, options, status, error); | |
| #endif | |
| #else | |
| error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); | |
| status = DatabaseStatus::FAILED_BAD_FORMAT; | |
| return nullptr; | |
| #endif | |
| } | |
| if (format == DatabaseFormat::SQLITE) { | |
| #ifdef USE_SQLITE | |
| return MakeSQLiteDatabase(path, options, status, error); | |
| #else | |
| error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path))); | |
| status = DatabaseStatus::FAILED_UNSUPPORTED_FORMAT; | |
| return nullptr; | |
| #endif | |
| } |
ci/dash/build_src.sh
Outdated
| BITCOIN_CONFIG_ALL="--disable-dependency-tracking --prefix=$DEPENDS_DIR/$HOST --bindir=$BASE_OUTDIR/bin --libdir=$BASE_OUTDIR/lib" | ||
| if [ -z "$NO_WERROR" ]; then | ||
| BITCOIN_CONFIG_ALL="${BITCOIN_CONFIG_ALL} --enable-werror" | ||
| fi |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify removal of --enable-suppress-external-warnings
The updated BITCOIN_CONFIG_ALL no longer includes --enable-suppress-external-warnings. Please confirm that no CI scripts or build logic still expect this flag.
🏁 Script executed:
#!/bin/bash
# Check for any remaining references to --enable-suppress-external-warnings in CI directories
grep -R -- '--enable-suppress-external-warnings' -n ci/Length of output: 91
Correcting grep syntax to properly search for any occurrences of the deprecated flag:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the deprecated flag in CI scripts
grep -R -n -- '--enable-suppress-external-warnings' ci/
# Search for the deprecated flag anywhere in the repository
grep -R -n -- '--enable-suppress-external-warnings' .
# Search for the identifier without the leading dashes as a sanity check
grep -R -n 'suppress-external-warnings' .Length of output: 75
The script exited early because grep returns exit code 1 when no matches are found under set -euo pipefail. Let’s rerun the search without aborting on no‐match so we cover all three checks:
🏁 Script executed:
#!/bin/bash
# Allow grep to return 1 without exiting
set +e
# 1) In CI scripts
grep -R -n -- '--enable-suppress-external-warnings' ci/
# 2) Anywhere in the repo
grep -R -n -- '--enable-suppress-external-warnings' .
# 3) Sanity check without leading dashes
grep -R -n 'suppress-external-warnings' .Length of output: 300
Clean up remaining suppress-external-warnings entries
We still have the AC_ARG_ENABLE([suppress-external-warnings], …) block in configure.ac, even though no CI scripts pass --enable-suppress-external-warnings. Either drop this argument entirely or reconcile its default with your new build logic:
• In configure.ac (around lines 213–214):
- AC_ARG_ENABLE([suppress-external-warnings],
- [AS_HELP_STRING([--disable-suppress-external-warnings],
- [suppress external include warnings in build output])],
- …)Remove or update the block to match your intended default (suppress on/off).
Committable suggestion skipped: line range outside the PR's diff.
-Werror-enabled builds for Clang, merge bitcoin#13633-Werror for Clang, merge bitcoin#13633
|
This pull request has conflicts, please rebase. |
…bitcoin#29486, bitcoin#25972 (ensure `WARN_CXXFLAGS` are populated to ensure expected `--enable-werror` behavior) af14f23 ci: don't build using `-Werror` when using Clang (Kittywhiskers Van Gogh) 3b837c8 ci: don't build using `-Werror` when using GCC (Kittywhiskers Van Gogh) 04e036e revert: make fuzzing builds stricter by enabling -Werror by default (Kittywhiskers Van Gogh) 29090a0 merge bitcoin#25972: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set (Kittywhiskers Van Gogh) d0548f8 merge bitcoin#29486: remove -Wdocumentation conditional (Kittywhiskers Van Gogh) 0f4812f merge bitcoin#27872: suppress external warnings by default (Kittywhiskers Van Gogh) 7100780 merge bitcoin#28999: Enable -Wunreachable-code (Kittywhiskers Van Gogh) 5471c58 merge bitcoin#28092: document that -Wreturn-type has been fixed upstream (mingw-w64) (Kittywhiskers Van Gogh) 9098c9c build: always enable -Wsuggest-override (Kittywhiskers Van Gogh) Pull request description: ## Motivation While working on [dash#6633](#6633), I had built `develop` (5e4a892) but the build _failed_ locally due to a `-Wthread-safety` warning (see below) that was introduced by [bitcoin#25337](bitcoin#25337) ([commit](a7d4127)). It was caught because I use additional `CXXFLAGS` on my local system but it _should_ have been caught by CI, especially since [bitcoin#20182](bitcoin#20182) ([commit](14a67ee)) made `--enable-werror` the default for CI and the sole exception (the Windows build) was remedied with [bitcoin#20586](bitcoin#20586) ([commit](750447e)), so every build variant should've run with `-Werror`. <details> <summary>Compile error:</summary> ``` CXX wallet/libbitcoin_wallet_a-sqlite.o wallet/rpcwallet.cpp:1038:36: error: calling function 'IsMine' requires holding mutex 'wallet.cs_wallet' exclusively [-Werror,-Wthread-safety-analysis] 1038 | isminefilter mine = wallet.IsMine(address); | ^ ``` </details> But that didn't happen. Till [bitcoin#23149](70ed6b4), there were a separate set of warnings (overridable by `CXXFLAGS`) and errors (overridable by `CXXFLAG_WERROR`). _Before_ the backport, coverage was as expected ([build](https://gitlab.com/dashpay/dash/-/jobs/9221165750#L786), search for `-Werror=thread-safety`) but _after_ the backport, `thread-safety` (and other expected warnings) were no longer being evaluated ([build](https://gitlab.com/dashpay/dash/-/jobs/9238308844#L740), search for `-Werror` and `-Wthread-safety`, only the former is present). Expected `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wreorder -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wunreachable-code-loop-increment -Wimplicit-fallthrough -Wno-unused-parameter -Wno-self-assign -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` Actual `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` This happened because `CXXFLAGS` are overridden by default which results in none of the warnings making it to the final `CXXFLAGS`, which reduced the effectiveness of `-Werror` substantially (while `CXXFLAG_WERROR` was left undisturbed, which allowed pre-backport builds to include coverage). This is remedied by backporting [bitcoin#25972](bitcoin#25972) (done by this PR), which will ensure that `WARN_CXXFLAGS` are included _even if `CXXFLAGS` are overridden_ and is possible because [bitcoin#24391](bitcoin#24391) ([commit](11323c3)) is already in `develop`. ## Additional Information * Dependency for #6638 * Dependency for #6639 * Because the warnings (converted to errors with `-Werror`) cast a wider net than the older set of error flags, `develop` in its current state does not compile. To allow CI to bless this PR, `-Werror` for both Clang and GCC-based builds have been **tentatively** disabled and will be re-enabled by the dependent PRs listed above. It is recommended to read the pull request description for both dependents while reviewing this PR. * `-Wsuggest-override` was made unconditional in [bitcoin#28348](bitcoin#28348) ([commit](c71e3df)) **but** there were two such conditional checks, they were deduplicated in [bitcoin#23149](bitcoin#23149) but the former was merged before the latter (i.e. out-of-order) and one conditional check lingered around. This lingering check has been removed as we don't support GCC 9.2. * `CXXFLAGS` set for the fuzz build ([commit](184bd60)) that enabled `-Werror` are made redundant with [bitcoin#20182](bitcoin#20182) and therefore, have been removed. ## 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK af14f23 Tree-SHA512: ccdaf71cf79eb3aec2468c4c1eaa696cd120c03e9665a3c4b56da6ef17cca9585ef8c66ac1625f2ba243c7f80f15e92a336c0bd90b5f11969fabb3adde3c8125
Also, add `AssertLockHeld` to corresponding `EXCLUSIVE_LOCKS_REQUIRED`
Required to avoid `-Wunused-function` warnings when building with wallet support disabled.
Code is unused but inherited by upstream and therefore, has to stick around even if we do nothing with it.
…n#27872, bitcoin#29486, bitcoin#25972 (ensure `WARN_CXXFLAGS` are populated to ensure expected `--enable-werror` behavior) af14f23 ci: don't build using `-Werror` when using Clang (Kittywhiskers Van Gogh) 3b837c8 ci: don't build using `-Werror` when using GCC (Kittywhiskers Van Gogh) 04e036e revert: make fuzzing builds stricter by enabling -Werror by default (Kittywhiskers Van Gogh) 29090a0 merge bitcoin#25972: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set (Kittywhiskers Van Gogh) d0548f8 merge bitcoin#29486: remove -Wdocumentation conditional (Kittywhiskers Van Gogh) 0f4812f merge bitcoin#27872: suppress external warnings by default (Kittywhiskers Van Gogh) 7100780 merge bitcoin#28999: Enable -Wunreachable-code (Kittywhiskers Van Gogh) 5471c58 merge bitcoin#28092: document that -Wreturn-type has been fixed upstream (mingw-w64) (Kittywhiskers Van Gogh) 9098c9c build: always enable -Wsuggest-override (Kittywhiskers Van Gogh) Pull request description: ## Motivation While working on [dash#6633](dashpay#6633), I had built `develop` (5e4a892) but the build _failed_ locally due to a `-Wthread-safety` warning (see below) that was introduced by [bitcoin#25337](bitcoin#25337) ([commit](dashpay@a7d4127)). It was caught because I use additional `CXXFLAGS` on my local system but it _should_ have been caught by CI, especially since [bitcoin#20182](bitcoin#20182) ([commit](dashpay@14a67ee)) made `--enable-werror` the default for CI and the sole exception (the Windows build) was remedied with [bitcoin#20586](bitcoin#20586) ([commit](dashpay@750447e)), so every build variant should've run with `-Werror`. <details> <summary>Compile error:</summary> ``` CXX wallet/libbitcoin_wallet_a-sqlite.o wallet/rpcwallet.cpp:1038:36: error: calling function 'IsMine' requires holding mutex 'wallet.cs_wallet' exclusively [-Werror,-Wthread-safety-analysis] 1038 | isminefilter mine = wallet.IsMine(address); | ^ ``` </details> But that didn't happen. Till [bitcoin#23149](dashpay@70ed6b4), there were a separate set of warnings (overridable by `CXXFLAGS`) and errors (overridable by `CXXFLAG_WERROR`). _Before_ the backport, coverage was as expected ([build](https://gitlab.com/dashpay/dash/-/jobs/9221165750#L786), search for `-Werror=thread-safety`) but _after_ the backport, `thread-safety` (and other expected warnings) were no longer being evaluated ([build](https://gitlab.com/dashpay/dash/-/jobs/9238308844#L740), search for `-Werror` and `-Wthread-safety`, only the former is present). Expected `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wreorder -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wunreachable-code-loop-increment -Wimplicit-fallthrough -Wno-unused-parameter -Wno-self-assign -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` Actual `CXXFLAGS`: ``` CXXFLAGS = -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=. -Werror -pipe -std=c++20 -O2 -O0 -g0 ``` This happened because `CXXFLAGS` are overridden by default which results in none of the warnings making it to the final `CXXFLAGS`, which reduced the effectiveness of `-Werror` substantially (while `CXXFLAG_WERROR` was left undisturbed, which allowed pre-backport builds to include coverage). This is remedied by backporting [bitcoin#25972](bitcoin#25972) (done by this PR), which will ensure that `WARN_CXXFLAGS` are included _even if `CXXFLAGS` are overridden_ and is possible because [bitcoin#24391](bitcoin#24391) ([commit](dashpay@11323c3)) is already in `develop`. ## Additional Information * Dependency for dashpay#6638 * Dependency for dashpay#6639 * Because the warnings (converted to errors with `-Werror`) cast a wider net than the older set of error flags, `develop` in its current state does not compile. To allow CI to bless this PR, `-Werror` for both Clang and GCC-based builds have been **tentatively** disabled and will be re-enabled by the dependent PRs listed above. It is recommended to read the pull request description for both dependents while reviewing this PR. * `-Wsuggest-override` was made unconditional in [bitcoin#28348](bitcoin#28348) ([commit](dashpay@c71e3df)) **but** there were two such conditional checks, they were deduplicated in [bitcoin#23149](bitcoin#23149) but the former was merged before the latter (i.e. out-of-order) and one conditional check lingered around. This lingering check has been removed as we don't support GCC 9.2. * `CXXFLAGS` set for the fuzz build ([commit](dashpay@184bd60)) that enabled `-Werror` are made redundant with [bitcoin#20182](bitcoin#20182) and therefore, have been removed. ## 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK af14f23 Tree-SHA512: ccdaf71cf79eb3aec2468c4c1eaa696cd120c03e9665a3c4b56da6ef17cca9585ef8c66ac1625f2ba243c7f80f15e92a336c0bd90b5f11969fabb3adde3c8125
| EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) | ||
| { | ||
| AssertLockHeld(wallet.cs_wallet); | ||
|
|
There was a problem hiding this comment.
better to remove duplicated IsMine check below, no need to add annotation then
| @@ -219,7 +219,7 @@ bool LegacyScriptPubKeyMan::CheckDecryptionKey(const CKeyingMaterial& master_key | |||
| if (keyFail) { | |||
| return false; | |||
| } | |||
| if (!keyPass && !accept_no_keys && (m_hd_chain.IsNull() || !m_hd_chain.IsNull() && !m_hd_chain.IsCrypted())) { | |||
| if (!keyPass && !accept_no_keys && (m_hd_chain.IsNull() || (!m_hd_chain.IsNull() && !m_hd_chain.IsCrypted()))) { | |||
There was a problem hiding this comment.
Better to simplify to
if (!keyPass && !accept_no_keys && (m_hd_chain.IsNull() || !m_hd_chain.IsCrypted()))) {
Proof. Both functions:
bool f(bool a, bool b, bool c) {
return !a && (b || !b && !c);
}
bool g(bool a, bool b, bool c) {
return !a && (b || !c);
}
are compiled to same binary:
xor edx, 1
xor eax, eax
or edx, esi
test dil, dil
cmove eax, edx
ret
…ray-bounds` and `-Wdangling-reference`, re-enable `-Werror` for GCC 40caa7d revert: don't build using `-Werror` when using GCC (Kittywhiskers Van Gogh) 96e494c build: constrain `-Wstringop-over{read,flow}` suppression to GCC only (Kittywhiskers Van Gogh) 6bff03b build: don't error on `-Wattributes` with GCC <14 (Kittywhiskers Van Gogh) 9aab60b build: don't error on `-Warray-bounds` and `-Wdangling-reference` with GCC (Kittywhiskers Van Gogh) 18fcb26 fix: resolve `-Wunused-function` warning (Kittywhiskers Van Gogh) 7a65315 fix: resolve `-Wmaybe-uninitialized` warnings (Kittywhiskers Van Gogh) b208018 fix: resolve `-Wignored-qualifiers` warning (Kittywhiskers Van Gogh) 7dd0daa fix: resolve `-Woverloaded-virtual` warning (Kittywhiskers Van Gogh) c587838 refactor: use `GetTransactionBlock()` as narrow `GetTransaction()` proxy (Kittywhiskers Van Gogh) d9828a9 refactor: move `ToJson` definitions relied by `TxToUniv` to source file (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6637 * Depends on #6638 * `TxToUniv()` is a function defined in `core_write.cpp`, which is a part of `libbitcoin_common` that relies on `ToJson()` definitions in various classes (`CCbTx`, `CProRegTx` and others) to print out of contents of transaction payload. The problem is, those various classes have their source files a part of `libbitcoin_server`, so when binaries like `dash-tx` (which do not use `libbitcoin_server`) need to be linked, the necessary `ToJson()` implementations, were they defined in source files, wouldn't be found and this would result in a linking error. This is worked around by defining them in the header instead but this creates a new problem where these headers are included by `core_write` but constructing the UniValue objects returned by `ToJson()` needs something from `core_write` (i.e. there is a circular dependency). This was worked around in [dash#6229](#6229) ([commit](b330318)) with redefinition, which was fine but is now verboten with `-Wredundant-decls` enforcement. * To work around this, all `ToJson()` definitions from those headers relied on by `TxToUniv()` have been moved to their own source file (`evo/core_write.cpp`) and included in `libbitcoin_common`. This takes care of the linking issue and avoids circular dependencies. * `GetTransaction()` is extensively used in Dash-specific code and a redefinition in `validation.cpp` is used to reduce the amount of reported circular dependencies, removing that circular dependency in order to satisfy `-Wredundant-decls` results in the following (see below). <details> <summary>Circular dependencies:</summary> Removed: ``` "evo/deterministicmns -> validation -> txmempool -> evo/deterministicmns" ``` Added: ``` "node/transaction -> validation -> node/transaction" "evo/deterministicmns -> node/transaction -> net_processing -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/blockstorage -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/context -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> txmempool -> evo/deterministicmns" "evo/chainhelper -> llmq/chainlocks -> node/transaction -> node/context -> evo/chainhelper" "evo/deterministicmns -> node/transaction -> net_processing -> evo/mnauth -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/context -> governance/governance -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> net_processing -> llmq/dkgsession -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> net_processing -> llmq/dkgsessionmgr -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> net_processing -> llmq/quorums -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/blockstorage -> masternode/node -> evo/deterministicmns" "governance/governance -> governance/object -> node/transaction -> node/context -> governance/governance" "llmq/chainlocks -> node/transaction -> node/context -> llmq/context -> llmq/chainlocks" "llmq/context -> llmq/instantsend -> node/transaction -> node/context -> llmq/context" "coinjoin/context -> coinjoin/server -> evo/deterministicmns -> node/transaction -> node/context -> coinjoin/context" "evo/cbtx -> evo/deterministicmns -> node/transaction -> node/context -> evo/creditpool -> evo/cbtx" "evo/deterministicmns -> node/transaction -> net_processing -> llmq/dkgsession -> llmq/debug -> evo/deterministicmns" "evo/deterministicmns -> node/transaction -> node/context -> evo/mnhftx -> llmq/signing -> llmq/signing_shares -> evo/deterministicmns" ``` Diff: **+18** </details> To avoid this, alongside some header adjustments, a proxy to `GetTransaction()` has been introduced in `evo/chainhelper.cpp`, `GetTransactionBlock()`. This _reduces_ the number of circular dependencies (see below). <details> <summary>Circular dependencies:</summary> Removed: ``` "consensus/tx_verify -> evo/assetlocktx -> validation -> consensus/tx_verify" "consensus/tx_verify -> evo/assetlocktx -> validation -> txmempool -> consensus/tx_verify" "evo/assetlocktx -> validation -> txmempool -> evo/assetlocktx" "evo/deterministicmns -> validation -> evo/deterministicmns" "evo/deterministicmns -> validation -> txmempool -> evo/deterministicmns" ``` Added: ``` "evo/chainhelper -> llmq/chainlocks -> evo/chainhelper" "evo/chainhelper -> llmq/instantsend -> evo/chainhelper" "evo/chainhelper -> evo/specialtxman -> evo/deterministicmns -> evo/chainhelper" "evo/chainhelper -> node/transaction -> node/context -> evo/chainhelper" "evo/deterministicmns -> llmq/commitment -> validation -> evo/deterministicmns" "consensus/tx_verify -> evo/assetlocktx -> llmq/commitment -> validation -> consensus/tx_verify" "evo/assetlocktx -> llmq/signing -> net_processing -> txmempool -> evo/assetlocktx" "evo/chainhelper -> masternode/payments -> governance/classes -> governance/object -> evo/chainhelper" "evo/deterministicmns -> llmq/commitment -> validation -> txmempool -> evo/deterministicmns" "consensus/tx_verify -> evo/assetlocktx -> llmq/signing -> net_processing -> txmempool -> consensus/tx_verify" ``` Diff: **+5** </details> To differentiate it from `GetTransaction()`, since all Dash-specific invocations either rely on the transaction index or the mempool, we can safely trim down the number of arguments and add a fast-fail. * Even with `-Werror` enabled, we have to downgrade `-Warray-bounds` to warnings (i.e. use `-Wno-error=array-bounds`) as there are two sources of the warning, `immer` (see [arximboldi/immer#223](arximboldi/immer#223)) and serialization code (fixed with [bitcoin#30765](bitcoin#30765)). As due to the former, we need to suppress it anyways, the latter has not been backported in the interest of brevity. * Similar to the above, we have to downgrade `-Wdangling-reference` as it is caused by UniValue-adjacent code that is fixed by [bitcoin#27605](bitcoin#27605) but would require an out-of-order backport that pulls in multiple dependent PRs in order to be complete (or alternatively, backport it as-is and then make future backporting annoying). The simpler solution to downgrade the warning was opted for instead. * We need to annotate `CHDPubKey::nVersion` with `[[maybe_unused]]` to avoid upsetting Clang due to `-Wunused-private-field`. GCC 11 doesn't emit a warning for that variable and therefore, finds the annotation unnecessary, triggering an error due to `-Wattributes` ([build](https://gitlab.com/dashpay/dash/-/jobs/9777475240#L1581)). Since both Clang 18 and GCC 14 agree with the `-Wunused-private-field` assessment, we are better off downgrading `-Wattributes` to a warning for GCC 11. ## 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: knst: ACK 40caa7d UdjinM6: utACK 40caa7d Tree-SHA512: 4fb383f4e2142db24ffedffd023cc075804c07e499abfcfc5708047d64fd981599cdb7479d92f501bedede6d94b76cc16f77f1372dd5bce7e1906ddb374679c3
Additional Information
Depends on build: merge bitcoin#28092, #28999, #27872, #29486, #25972 (ensure
WARN_CXXFLAGSare populated to ensure expected--enable-werrorbehavior) #6637Dependency for fix: resolve compiler warnings, suppress errors on
-Warray-boundsand-Wdangling-reference, re-enable-Werrorfor GCC #6639While bitcoin#13633 was originally marked as DNM in the backports spreadsheet for being SegWit-related, it's not SegWit-dependent and is for dead code removal, which is necessary due to the enforcement of
-Wunused-function. It has therefore been included.EraseOldDBData()fell into disuse after dash#6579 but wasn't removed then. It has been removed now due to-Wunused-functionenforcement.An extraneous
ApplyStats()definition was left over when backporting bitcoin#19145. It has been removed now due to-Wunused-functionenforcement.bitcoin#25337 (commit) did not include a lock annotation for the lambda expression in
ListReceived(). It has been added alongsideAssertLockHelds for both the lambda expression and the function. This was done to pass-Wthread-safetyenforcement.ParsePubKeyIDFromAddress(),ParseBLSPubKey()andValidatePlatformPort()have been moved inside theENABLE_WALLETmacro conditional to avoid-Wunused-functionwarnings if building with wallets disabled.Some dead code cannot be removed because they are either part of partial backports that may be completed in the future or complicate backports in the future that may expect their presence. To keep
-Wunused-functionhappy, they have been marked with[[maybe_unused]]and this annotation should be removed when they are used.Breaking Changes
None expected.
Checklist