backport: trivial 2025 04 22 #6641
Conversation
…nclude/c++/ suppression fae55f9 test: Add implicit-signed-integer-truncation:*/include/c++/ suppression (MarcoFalke) Pull request description: Needed for aarch64. Steps to test on aarch64: ``` lscpu | grep Arch FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh ``` ACKs for top commit: fanquake: ACK fae55f9 - reproduced the failure: Tree-SHA512: b5058873118d285cc5d678a572cf4b890f8d68a24e1ac0987490f1b4123469a2b4456b08474f372e6aa49bb0d69e16f2c8277208b1cde3222a317f000beb5056
…s for `src/secp256k1` subtree a747774 Add UBSan `-fsanitize=integer` suppressions for `src/secp256k1` subtree (Hennadii Stepanov) Pull request description: Required for bitcoin#27991 (see the [comment](bitcoin#27991 (comment))) and for the upcoming CMake-based build system. ACKs for top commit: MarcoFalke: lgtm ACK a747774 Tree-SHA512: 602fa3ad22d3b0f6981a51358677d2347c92c4c9f59626b497af10f7ba828ede37227d8ee717f089bf33bde5efe0854d53acc89bea46f0955e62b7f22c454d05
…dows builds 96f2cf8 build: use -muse-unaligned-vector-move for Windows (fanquake) Pull request description: We currently work around a longstanding GCC issue with aligned vector instructions, by patching the behaviour we want into GCC (see discussion in bitcoin#24736). Possibly in response to the GCC thread (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412#c40), a new option was [introduced into the binutils assembler](https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=c8480b58e1968f209b6365af7422678f348222c2) with the 2.38 release: ``` x86: Add -muse-unaligned-vector-move to assembler Unaligned load/store instructions on aligned memory or register are as fast as aligned load/store instructions on modern Intel processors. Add a command-line option, -muse-unaligned-vector-move, to x86 assembler to encode encode aligned vector load/store instructions as unaligned vector load/store instructions. ``` ACKs for top commit: theuni: ACK 96f2cf8. Tree-SHA512: f5aaa125922bb17bbb51454103b3394d293184214b0dea554c36c2f130488a3ede2f39678044ec846fa0fdf4cd441d4227f4565c29d380f5a73b50bf6f3b9a67
WalkthroughThis set of changes introduces updates across build configuration scripts, documentation, test infrastructure, and sanitizer suppressions. In the build system, new compile flag checks are added for Windows targets to handle vector instruction alignment, and the runtime check for the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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: 0
🧹 Nitpick comments (5)
doc/offline-signing-tutorial.md (5)
7-7: Use a relative link for PSBT docs.Instead of linking to the GitHub master branch, consider referencing the local
doc/psbt.mdto keep documentation self‑contained:-[Partially Signed Bitcoin Transactions](https://github.com/bitcoin/bitcoin/blob/master/doc/psbt.md) +[Partially Signed Bitcoin Transactions](psbt.md)
18-19: Refine sentence for readability.Add a comma and use a more direct construct:
-In order to spend these coins we'll create an unsigned PSBT using the `watch_only_wallet`, sign it offline, and broadcast it online. +To spend these coins, we'll create an unsigned PSBT using the `watch_only_wallet`, sign it offline, and broadcast it online.🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Consider a shorter alternative to avoid wordiness.
Context: ...ill receive some coins into the wallet. In order to spend these coins we'll create an unsig...(IN_ORDER_TO_PREMIUM)
[typographical] ~18-~18: It seems that a comma is missing.
Context: ...nto the wallet. In order to spend these coins we'll create an unsigned PSBT using the...(IN_ORDER_TO_VB_COMMA)
51-59: Minor comma omission.Add a comma after "watch_only_wallet" for consistency:
-On the online machine create a blank watch-only wallet which has private keys disabled and is named `watch_only_wallet`. +On the online machine, create a blank watch-only wallet with private keys disabled named `watch_only_wallet`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~53-~53: Possible missing comma found.
Context: ...ewatch_only_wallet1. On the online machine create a blank watch-only wallet which ...(AI_HYDRA_LEO_MISSING_COMMA)
144-154: Improve flow of PSBT creation step.Consider adding a comma after "address" and rephrasing for clarity:
-Get a destination address for the transaction. In this tutorial we'll be sending funds to the address `tb1q9k5w0nhnhyeh78snpxh0t5t7c3lxdeg3erez32`, but if you don't need the coins for further testing you could send the coins back to the faucet. +Get a destination address for the transaction. In this tutorial, we'll send funds to `tb1q9k5w0nhnhyeh78snpxh0t5t7c3lxdeg3erez32`; you can return coins to the faucet if testing isn’t required.🧰 Tools
🪛 LanguageTool
[typographical] ~146-~146: It appears that a comma is missing.
Context: ...on address for the transaction. In this tutorial we'll be sending funds to the address `...(DURING_THAT_TIME_COMMA)
255-256: Add a trailing newline.Ensure the file ends with a newline character to comply with POSIX file conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
configure.ac(2 hunks)contrib/guix/manifest.scm(1 hunks)doc/README.md(1 hunks)doc/multisig-tutorial.md(0 hunks)doc/offline-signing-tutorial.md(1 hunks)src/init.cpp(1 hunks)src/init/common.cpp(1 hunks)src/test/fuzz/fuzz.cpp(2 hunks)test/functional/p2p_add_connections.py(3 hunks)test/sanitizer_suppressions/ubsan(2 hunks)
💤 Files with no reviewable changes (1)
- doc/multisig-tutorial.md
🧰 Additional context used
🪛 LanguageTool
doc/offline-signing-tutorial.md
[style] ~18-~18: Consider a shorter alternative to avoid wordiness.
Context: ...ill receive some coins into the wallet. In order to spend these coins we'll create an unsig...
(IN_ORDER_TO_PREMIUM)
[typographical] ~18-~18: It seems that a comma is missing.
Context: ...nto the wallet. In order to spend these coins we'll create an unsigned PSBT using the...
(IN_ORDER_TO_VB_COMMA)
[uncategorized] ~25-~25: Possible missing comma found.
Context: ...the offline_wallet 1. On the offline machine create a wallet named offline_wallet ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~53-~53: Possible missing comma found.
Context: ...e watch_only_wallet 1. On the online machine create a blank watch-only wallet which ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~146-~146: It appears that a comma is missing.
Context: ...on address for the transaction. In this tutorial we'll be sending funds to the address `...
(DURING_THAT_TIME_COMMA)
🪛 Gitleaks (8.21.2)
doc/offline-signing-tutorial.md
130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (34)
src/init.cpp (2)
592-592: Simplified UPnP help textThe help text for the
-upnpcommand line argument has been simplified by directly usingstrprintfwithout the translation marker_(). This is consistent with other networking options and avoids unnecessary translation overhead for technical terms.
597-597: Simplified NAT-PMP help textSimilar to the UPnP change, the help text for the
-natpmpcommand line argument has been simplified by removing the translation marker. This maintains consistency with the UPnP option and other networking-related parameters.src/init/common.cpp (1)
70-70: Improved-loglevelhelp text clarity and completenessThe help text for the
-loglevelargument has been significantly improved:
- It now clearly explains that this affects logging categories enabled with
-debugor the logging RPC- Uses
LogInstance().LogLevelsString()to dynamically list all possible values- Shows the default log level
- Clarifies which levels are always logged (error, warning, info)
- Better explains category-specific overrides
The flags were also updated to prevent negation and elision, which is appropriate for this parameter.
test/functional/p2p_add_connections.py (3)
2-2: Updated copyright year rangeChanged copyright year from a specific year to a range "2020-present" which is good practice for ongoing development.
14-20: AddedVersionSenderclass for P2P protocol testingNew test class that overrides the default P2P connection behavior to immediately send a version message upon connection, rather than waiting to receive one first. This allows testing of early version message handling.
The class properly validates that
on_connection_send_msgis set before sending and cleared afterward.
118-130: Added test for proper version message handling protocolThis new test case validates important P2P behavior - ensuring that the node properly handles receiving a version message before it sends its own. The test confirms that the node:
- Sends its own version message first
- Then processes the early version message from the peer
- Finally sends a verack message to complete the handshake
This is essential for ensuring protocol compliance with other nodes in the network.
doc/README.md (1)
80-80: Added link to Offline Signing TutorialAdded a new entry in the Miscellaneous section linking to the offline-signing-tutorial.md document. This is a valuable addition as offline signing is an important security practice for handling high-value transactions.
configure.ac (2)
746-748: Good addition of Windows-specific compiler flagThe added check for
-Wa,-muse-unaligned-vector-movespecifically for Windows targets helps avoid issues with aligned vector instructions. This is a good mitigation for the GCC bug mentioned in the comment (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412).
1024-1024: Removal of sys/sysctl.h from header checksThe removal of
sys/sysctl.hfrom the list of headers checked byAC_CHECK_HEADERSstreamlines the configuration process. This aligns with the pattern in configure.ac where header checks are kept to the necessary minimum.test/sanitizer_suppressions/ubsan (1)
22-22: Added secp256k1 suppressions for sanitizer checksThe addition of
secp256k1/to various sanitizer suppression categories is appropriate for cryptographic libraries that may contain intentional behavior that triggers sanitizer warnings. This change ensures clean sanitizer runs by suppressing known benign issues.Also applies to: 31-31, 34-34, 37-37, 44-44
src/test/fuzz/fuzz.cpp (2)
80-96: Good implementation of coverage counter reset functionalityThe newly added
ResetCoverageCounters()function is well-implemented with proper conditional compilation and weak symbol linking. This allows the code to work correctly regardless of the compilation environment.
150-150: Appropriate usage of the new counter reset functionAdding the call to
ResetCoverageCounters()at this point in the initialization process ensures that coverage counters are reset before fuzzing begins, leading to more accurate coverage metrics during fuzzing runs.contrib/guix/manifest.scm (1)
430-431: Improved SSP enablement for mingw-w64 GCC packageThe change from using make flags (
gcc_cv_libc_provides_ssp=yes) to using a configure flag (--enable-default-ssp=yes) is a more modern and direct approach to enabling Stack Smashing Protector. This aligns with the changes inconfigure.acthat removed the runtimelibsspcheck.doc/offline-signing-tutorial.md (21)
1-6: Well structured introduction and clear goal.The title and opening paragraphs succinctly explain the purpose and security benefits of an air‑gapped offline signing workflow.
9-11: Admonition style is clear.The
[!NOTE]block correctly highlights the mainnet vs signet distinction.
12-17: Overview section is comprehensive.The heading, description, and bullet list clearly outline the two hosts and their roles.
20-22: Requirements list is clear.Specifying
jqas optional is helpful for users who prefer JSON CLI tools.
23-35: Code block formatting looks good.The shell commands for creating the offline wallet are properly fenced and annotated.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~25-~25: Possible missing comma found.
Context: ...theoffline_wallet1. On the offline machine create a wallet namedoffline_wallet...(AI_HYDRA_LEO_MISSING_COMMA)
37-39: Helpful security reminder.Emphasizing wallet encryption is important. The link to deeper guidance is appropriate.
40-46: Descriptor export steps are clear.Using
jqto extract descriptors is a nice touch for readability and portability.
48-50: Note on descriptor transfer is useful.Reminding users to use physical media reinforces the air‑gap concept.
61-68: Watch‑only wallet creation commands are well formatted.The use of
-namedflags and JSON response examples help onboard new users.
70-74: Descriptor import command is accurate.Using
$(cat /path/to/descriptors.json)within quotes is correct to handle JSON arrays.
76-100: Import result JSON is illustrative.Showing multiple
successresponses clarifies why several descriptors are imported.
102-104: Helpful note on multiple descriptors.Explaining the significance of multiple imports aids comprehension.
105-115: Funding instructions are straightforward.Detailing both offline and online wallet address generation reinforces the key point.
121-141: Unspent output example is comprehensive.The JSON sample covers all relevant fields, showcasing
listunspentresults.🧰 Tools
🪛 Gitleaks (8.21.2)
130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
168-194: Decoding and analysis examples are clear.Providing both
decodepsbtandanalyzepsbtoutputs helps users understand the missing signatures section.
198-206: Signing workflow instructions are accurate.Unlocking, processing, signing, and finalizing are clearly broken into steps.
211-215: Final PSBT export command is correct.Capturing
.hexand redirecting output provides a ready-to-broadcast file.
218-224: Broadcast example is well demonstrated.Using
sendrawtransactionon the online node completes the workflow.
231-244: Balance confirmation steps are helpful.Showing both balance and last processed block gives a full picture post‑broadcast.
250-255: Concluding transaction list command wraps up the tutorial.Encouraging users to inspect transaction history rounds out the guide.
1-255: Ensure README linkage is updated.This tutorial must be referenced in
doc/README.mdunder "Miscellaneous" for discoverability. Please verify and add the link if missing.🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Consider a shorter alternative to avoid wordiness.
Context: ...ill receive some coins into the wallet. In order to spend these coins we'll create an unsig...(IN_ORDER_TO_PREMIUM)
[typographical] ~18-~18: It seems that a comma is missing.
Context: ...nto the wallet. In order to spend these coins we'll create an unsigned PSBT using the...(IN_ORDER_TO_VB_COMMA)
[uncategorized] ~25-~25: Possible missing comma found.
Context: ...theoffline_wallet1. On the offline machine create a wallet namedoffline_wallet...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~53-~53: Possible missing comma found.
Context: ...ewatch_only_wallet1. On the online machine create a blank watch-only wallet which ...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~146-~146: It appears that a comma is missing.
Context: ...on address for the transaction. In this tutorial we'll be sending funds to the address `...(DURING_THAT_TIME_COMMA)
🪛 Gitleaks (8.21.2)
130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
f95af98 guix: default ssp for Windows GCC (fanquake) 95d55b9 guix: remove ssp workaround from Windows GCC (fanquake) 8f43302 build: remove explicit libssp linking from Windows build (fanquake) Pull request description: I was expecting this to fail to compile somewhere, maybe in the CI, but that doesn't seem to be the case? Seems workable given the SSP related changes in the newer mingw-w64 headers (which are in Guix): > Implement some of the stack protector functions/variables so -lssp is now optional when _FORTIFY_SOURCE or -fstack-protector-strong is used. However I think this would still be broken in some older environments, so we might have to wait for a compiler bump, or similar. The optional -lssp also seems to work when using older headers, which doesn't make sense. Would fix bitcoin#28104. ACKs for top commit: hebasto: ACK f95af98, I've verified binaries from `bitcoin-f95af98128f1-win64.zip` on Windows 11 Pro 23H2. TheCharlatan: ACK f95af98 Tree-SHA512: 71169ec513cfe692dfa7741d2bf37b45da05627c0af1cbd50cf8c3c04cc21c4bf88f3284532bddc1e3e648391ec78dbaca5170987a13c21ac204a7bcaf27f349
92f88a9 doc: fixup NAT-PMP help doc (fanquake) 02395ed init: remove redundant upnp #ifdef (fanquake) Pull request description: This is a very belated followup to bitcoin#26896 (which removed the configure options for setting the upnp and natpmp runtime default) and corrects the `-help` docs for `-upnp` and `-natpmp`. ACKs for top commit: davidgumberg: ACK bitcoin@92f88a9 hernanmarino: ACK 92f88a9 Tree-SHA512: 795dc8a8703bf322b5831d845de85f2428ee0dd45d3064b48ff47d147147381af26c0a9d00c596db12009b254763844b209989daf4e7470d20e8a1753b640966
949abeb [fuzz] Avoid collecting initialization coverage (dergoegge) Pull request description: Our coverage reports include coverage of initialization code, which can be misleading when trying to evaluate the coverage a fuzz harness achieves through fuzzing alone. This PR proposes to make fuzz coverage reports more accurate by resetting coverage counters after initialization code has been run. This makes it easier to evaluate which code was actually reached through fuzzing (e.g. to spot fuzz blockers). ACKs for top commit: maflcko: utACK 949abeb brunoerg: nice, utACK 949abeb Tree-SHA512: c8579bda4f3d71d199b9331fbe6316fce375a906743d0bc216bb94958dc03fdc9a951ea50cfeb487494a75668ae3c16471a82f7e5fdd912d781dc29d063e2c5b
c0b5ea5 build: Drop redundant `sys/sysctl.h` header check (Hennadii Stepanov) Pull request description: The `AC_CHECK_HEADERS` macro defines `HAVE_SYS_SYSCTL_H` if the `sys/sysctl.h` header is found. However, in the source code, this header is guarded by `HAVE_SYSCTL` and `HAVE_SYSCTL_ARND` macros, which have their own checks. Since `HAVE_SYS_SYSCTL_H` is not used, we can skip the `AC_CHECK_HEADERS(... sys/sysctl.h ...)` check. ACKs for top commit: laanwj: ACK c0b5ea5 fanquake: ACK c0b5ea5 - we could got the other way, and add nested #defs, but that doesn't seem worthwhile. Tree-SHA512: 73bc4bbfc5c457cd2c38e40f8e57d2a70c06ef661d76d4148d683d262be45b9405b8cda1958ac611c312ca7d9e2f9624cf2cac1b61f1008af0856875c62f0eac
faed5d3 test: Non-Shy version sender (MarcoFalke) Pull request description: After `add_outbound_p2p_connection`, the test framework normally sends a version message only in reply to a received version. This is fine, but the protocol does not require this and tolerates a version to be sent earlier. However, this is untested, and the missing test coverage leads to bugs being missed. For example bitcoin#30394 (review) Fix it by adding a test. ACKs for top commit: brunoerg: ACK faed5d3 tdb3: ACK faed5d3 theStack: tACK faed5d3 glozow: ACK faed5d3 Tree-SHA512: dbf527a39c932e994a1e8248ba78058000811a4bf69275278f1fd1e545716ac4d2d3be5dcf362976bbafa2a49f91d13e3601daf71d29e9c556179b01af62c03c
…and link to closed PR 3cd24aa doc: remove obsolete mention and link to closed PR (Marnix) Pull request description: Remove the mention and link as the PR (bitcoin#22341) is closed and the description is wrong/outdated anyway. ACKs for top commit: BrandonOdiwuor: ACK 3cd24aa tdb3: ACK 3cd24aa Tree-SHA512: 5cd97029337f0cdfe81b6be9401adc4fe51ae2868f8fcadcb03828531a38380a587c32840850a924b6428f62df7d20a1e16ef7414d4078e7bb2c4e359b1fae40
0e835b3 to
1e04c56
Compare
|
This pull request has conflicts, please rebase. |
…roundup" acddde7 Revert "Merge bitcoin#28461: build: Windows SSP roundup" (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Backporting [28461](ea32090) in #6641 broke Guix build. ## What was done? Revert ea32090. ## How Has This Been Tested? develop: https://github.com/UdjinM6/dash/actions/runs/15217141785 this PR: https://github.com/UdjinM6/dash/actions/runs/15219875769 ## Breaking Changes n/a ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: ACK acddde7 PastaPastaPasta: utACK acddde7 Tree-SHA512: 657d707dc7c46333265322bc87ea86ff42d43f7a9ec905ac3921642f7ba599ffd2ea62a7a234544500e9e471862511b4fffefc3e197c91bb5323ffee91436777
Issue being fixed or feature implemented
Batch of trivial backports
How Has This Been Tested?
Built locally
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.