backport: bitcoin#21551, #22585, #22650, #22798, #22994, #23014, #23126, bitcoin-core/gui#336, gui#342, gui#430 #6569
Conversation
97292b1 ci: Drop AppVeyor CI integration (Hennadii Stepanov) 1fb7079 ci: Add Windows task to Cirrus CI (Hennadii Stepanov) Pull request description: This PR: - gets rid off unreliable AppVeyor CI - places all CI configs in one place - allows to enable functional tests in the future (using a persistent worker) | no populated `vcpkg` cache | populated `vcpkg` cache | |---|---| |  |  | Currently, AppVeyor builds take about 44..48 minutes. ACKs for top commit: sipsorcery: re-ACK 97292b1. Tree-SHA512: 3af50d9fd68eb12f39724810dacf948e4068573b5dfd0dbaeb05d19d4bd6f10bd9a432656dcc32b742684b438d31305eace85c602296d7a1bf84b2f1fcc06f02
We don't have native windows builds, no need to track these changes
252d1a7 ci: use Debian Bullseye in ARM CI (fanquake) Pull request description: This works around an issue when trying to use `std::filesystem::remove_all` with the ARM GCC on Buster. Has been split out of bitcoin#20744. See commentary starting here: bitcoin#20744 (comment). Also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93201. ACKs for top commit: MarcoFalke: cr ACK 252d1a7 hebasto: ACK 252d1a7, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: ca71f5cb07fe06c1c7f0160935e667ffeb62bd6a1a89b54124b5633c5c176347a2207aaa5eca68938ed89db9778d357e42b677115d4ed386fa2d7d2ffa5025ad
WalkthroughThe pull request applies wide-ranging modifications across configuration files, source code, documentation, and tests. Changes in CI/CD configurations include a renaming of templates and an update to container images (for example, changing from Debian buster to bullseye) along with modifications to environment variable settings in test setup scripts. Documentation updates revise developer notes, fuzz testing guides, release notes, and subtree maintenance instructions to reflect current project identifiers. In the core code, several function signatures have been streamlined by removing deprecated parameters, and obsolete functions related to multi-destination extraction have been removed. The Qt user interface now dynamically allocates dialogs and uses new utility methods to manage them, while new signals were introduced to improve inter-component communication. The RPC interfaces have been revised to mark multiple fields as optional and to update response structures. Additionally, test cases have been adjusted or removed to match the updated functionality and extraction logic. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/qt/walletcontroller.cpp (1)
335-352: Consider adding error handling for wallet loading.
While loading wallets asynchronously, it might be helpful to detect or log any potential load failures for user feedback or debugging.src/qt/sendcoinsdialog.cpp (1)
479-482: Improve dialog management with non-blocking show()The dialog management has been improved by using dynamic allocation and Qt::WA_DeleteOnClose. However, as noted in the TODO comment, the blocking exec() call should be replaced with a non-blocking show() for better UI responsiveness.
.cirrus.yml (1)
13-16: Fix YAML formatting.The YAML formatting can be improved by fixing the spacing before colons.
-filter_template: &FILTER_TEMPLATE +filter_template: &FILTER_TEMPLATE skip: $CIRRUS_REPO_FULL_NAME == "bitcoin-core/gui" && $CIRRUS_PR == "" # No need to run on the read-only mirror, unless it is a PR. https://cirrus-ci.org/guide/writing-tasks/#conditional-task-execution stateful: false # https://cirrus-ci.org/guide/writing-tasks/#stateful-tasksdoc/fuzzing.md (2)
274-275: Improve command output documentation.The commands in the documentation should show their expected output to help users verify they're on the right track.
$ git clone https://github.com/bitcoin/bitcoin +# Expected output: Cloning into 'bitcoin'... $ cd bitcoin/Also applies to: 287-291
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
274-274: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
275-275: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
327-329: Add warning about potential disk space issues.The documentation should warn users about the potential disk space requirements when running Eclipser for extended periods.
Note that fuzzing with Eclipser on certain targets (those that create 'full nodes', e.g. `process_message*`) will, for now, slowly fill `/tmp/` with improperly cleaned-up files, which will cause spurious crashes. See [this proposed patch](https://github.com/bitcoin/bitcoin/pull/22472) for more information. + +Warning: Extended fuzzing sessions can consume significant disk space. Ensure you have sufficient free space in your `/tmp` directory +and consider monitoring disk usage during long fuzzing runs.doc/developer-notes.md (3)
1135-1138: Improve clarity of subtree maintenance instructions.The sentence structure can be improved to avoid repetition and enhance readability.
-Some of these are maintained by active developers of Dash Core, in which case -changes should go directly upstream without being PRed directly against the project. +Some of these are maintained by active developers of Dash Core. For these subtrees, +changes should be submitted upstream rather than being PRed against the project. They will be merged back in the next subtree merge.🧰 Tools
🪛 LanguageTool
[style] ~1136-~1136: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...go directly upstream without being PRed directly against the project. They will be merge...(ADVERB_REPETITION_PREMIUM)
1140-1142: Fix comma usage in conjunction.Remove the unnecessary comma before "so that" to improve readability.
-to these should also be sent upstream, but bugfixes may also be prudent to PR against -a Dash Core subtree, so that they can be integrated quickly. Cosmetic changes +to these should also be sent upstream, but bugfixes may also be prudent to PR against +a Dash Core subtree so that they can be integrated quickly. Cosmetic changes should be taken upstream.🧰 Tools
🪛 LanguageTool
[typographical] ~1141-~1141: The conjunction “so that” does not have a comma in front.
Context: ...rudent to PR against a Dash Core subtree, so that they can be integrated quickly. Cosmeti...(SO_THAT_UNNECESSARY_COMMA)
1150-1170: Format URLs consistently.Use consistent URL formatting with proper markdown links instead of bare URLs.
- - Subtree at https://github.com/bitcoin-core/leveldb-subtree ; maintained by Core contributors. - - Upstream at https://github.com/google/leveldb ; maintained by Google. Open + - Subtree at [bitcoin-core/leveldb-subtree](https://github.com/bitcoin-core/leveldb-subtree); maintained by Core contributors. + - Upstream at [google/leveldb](https://github.com/google/leveldb); maintained by Google. Open🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1150-1150: Bare URL used
null(MD034, no-bare-urls)
1151-1151: Bare URL used
null(MD034, no-bare-urls)
1158-1158: Bare URL used
null(MD034, no-bare-urls)
1159-1159: Bare URL used
null(MD034, no-bare-urls)
1162-1162: Bare URL used
null(MD034, no-bare-urls)
1165-1165: Bare URL used
null(MD034, no-bare-urls)
1168-1168: Bare URL used
null(MD034, no-bare-urls)
1169-1169: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.cirrus.yml(2 hunks).editorconfig(1 hunks)ci/test/00_setup_env_native_valgrind.sh(1 hunks)ci/test/00_setup_env_s390x.sh(1 hunks)doc/developer-notes.md(1 hunks)doc/fuzzing.md(1 hunks)doc/release-notes-22650.md(1 hunks)src/bitcoin-tx.cpp(1 hunks)src/core_io.h(1 hunks)src/core_write.cpp(2 hunks)src/qt/addressbookpage.cpp(1 hunks)src/qt/bitcoin.cpp(5 hunks)src/qt/bitcoin.h(1 hunks)src/qt/bitcoingui.cpp(6 hunks)src/qt/bitcoingui.h(1 hunks)src/qt/forms/optionsdialog.ui(2 hunks)src/qt/guiutil.cpp(2 hunks)src/qt/guiutil.h(2 hunks)src/qt/optionsdialog.cpp(1 hunks)src/qt/optionsdialog.h(1 hunks)src/qt/sendcoinsdialog.cpp(2 hunks)src/qt/transactionview.cpp(2 hunks)src/qt/walletcontroller.cpp(2 hunks)src/qt/walletcontroller.h(2 hunks)src/qt/walletframe.cpp(1 hunks)src/qt/walletview.cpp(2 hunks)src/rpc/blockchain.cpp(4 hunks)src/rpc/blockchain.h(0 hunks)src/rpc/mining.cpp(1 hunks)src/rpc/misc.cpp(2 hunks)src/rpc/net.cpp(2 hunks)src/rpc/rawtransaction.cpp(14 hunks)src/script/standard.cpp(0 hunks)src/script/standard.h(1 hunks)src/test/fuzz/script.cpp(2 hunks)src/test/fuzz/transaction.cpp(1 hunks)src/test/script_standard_tests.cpp(0 hunks)src/wallet/rpcwallet.cpp(17 hunks)test/functional/feature_dip3_deterministicmns.py(1 hunks)test/functional/rpc_addresses_deprecation.py(0 hunks)test/functional/test_runner.py(0 hunks)test/lint/README.md(1 hunks)
💤 Files with no reviewable changes (5)
- test/functional/test_runner.py
- test/functional/rpc_addresses_deprecation.py
- src/script/standard.cpp
- src/test/script_standard_tests.cpp
- src/rpc/blockchain.h
✅ Files skipped from review due to trivial changes (4)
- .editorconfig
- src/qt/forms/optionsdialog.ui
- test/lint/README.md
- doc/release-notes-22650.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/fuzzing.md
274-274: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
275-275: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
276-276: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
277-277: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
278-278: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
279-279: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
287-287: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
288-288: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
289-289: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
290-290: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
291-291: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
297-297: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
298-298: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
299-299: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
300-300: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
301-301: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
302-302: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
303-303: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
304-304: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
305-305: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
306-306: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
315-315: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
323-323: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
324-324: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
doc/developer-notes.md
1150-1150: Bare URL used
null
(MD034, no-bare-urls)
1151-1151: Bare URL used
null
(MD034, no-bare-urls)
1158-1158: Bare URL used
null
(MD034, no-bare-urls)
1159-1159: Bare URL used
null
(MD034, no-bare-urls)
1162-1162: Bare URL used
null
(MD034, no-bare-urls)
1165-1165: Bare URL used
null
(MD034, no-bare-urls)
1168-1168: Bare URL used
null
(MD034, no-bare-urls)
1169-1169: Bare URL used
null
(MD034, no-bare-urls)
🪛 Ruff (0.8.2)
test/functional/feature_dip3_deterministicmns.py
180-181: Use a single if statement instead of nested if statements
(SIM102)
🪛 YAMLlint (1.35.1)
.cirrus.yml
[warning] 18-18: too many spaces before colon
(colons)
[warning] 64-64: too many spaces before colon
(colons)
🪛 LanguageTool
doc/developer-notes.md
[style] ~1136-~1136: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...go directly upstream without being PRed directly against the project. They will be merge...
(ADVERB_REPETITION_PREMIUM)
[typographical] ~1141-~1141: The conjunction “so that” does not have a comma in front.
Context: ...rudent to PR against a Dash Core subtree, so that they can be integrated quickly. Cosmeti...
(SO_THAT_UNNECESSARY_COMMA)
🔇 Additional comments (56)
src/script/standard.h (2)
124-126: Documentation update accurately reflects current functionality.The updated documentation clarifies that
ExtractDestinationonly works for P2PK, P2PKH, and P2SH scripts, which aligns with the PR's objective of deprecating certain address-related functionality.
1-143: Verify impact of removingExtractDestinationsfunction.The removal of the
ExtractDestinationsfunction is a breaking change that aligns with the PR objectives. This change affects several RPCs that will no longer return theaddressesandreqSigsfields.Let's verify the impact on dependent code:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining references to ExtractDestinations # Search for direct function calls echo "Searching for direct function calls to ExtractDestinations..." rg "ExtractDestinations\(" --type cpp # Search for references in comments echo "Searching for references in comments..." rg "ExtractDestinations" --type cpp # Search for potentially affected RPC implementations echo "Searching for affected RPC implementations..." rg -A 5 "gettxout|getrawtransaction|decoderawtransaction|decodescript|gettransaction" --type cppLength of output: 25974
RPC/Address Extraction Update Verified: No Residual Dependencies
Our grep searches for direct calls or comments referencing
ExtractDestinationsreturned no hits in the codebase. This confirms that its removal does not leave any dangling dependencies, and the impacted RPCs have been updated accordingly to drop theaddressesandreqSigsfields.src/qt/walletcontroller.cpp (4)
185-185: Good use of deleteLater for automatic cleanup.
Connecting thefinishedsignal todeleteLateris a standard and safe way to ensure this activity object cleans itself up when done.
190-193: Confirm ownership and lifetime of the progress dialog.
Creating a newQProgressDialogwithWA_DeleteOnCloseensures memory is freed on close. Just ensure no further references toprogress_dialogare made after it is closed.
194-199: UI configuration looks good.
All essential properties (title, label text, range, modality) are set, which should give a clear progress indication to the user.
202-202: Maintaining consistency with Qt progress dialog behavior.
CallingsetValue(0)is a known workaround to properly start the duration estimator. Great attention to detail.src/qt/optionsdialog.h (1)
85-85: New signal adds flexibility for reset handling.
IntroducingquitOnResetis useful for external code to handle any additional teardown logic.src/qt/bitcoin.h (1)
70-71: Reinstated shutdown request method enhances clarity.
Bringing backrequestShutdownclarifies the shutdown flow. Verify it's correctly connected wherever shutdown is triggered to avoid partial or incomplete shutdown.src/qt/walletcontroller.h (2)
89-89: LGTM: Using default destructor aligns with modern C++ practices.The change to use
= defaultfor the virtual destructor is a good practice as it allows the compiler to generate the appropriate destructor code.
147-155: LGTM: New LoadWalletsActivity class follows established patterns.The new class follows the existing activity pattern in the codebase, inheriting from
WalletControllerActivityand providing wallet loading functionality.src/qt/walletframe.cpp (1)
281-283: LGTM: Improved dialog memory management.The changes improve dialog handling by:
- Using dynamic allocation with
new- Using
GUIUtil::ShowModalDialogAndDeleteOnClosefor automatic cleanupsrc/qt/addressbookpage.cpp (1)
188-195: LGTM: Consistent dialog memory management.The changes follow the new pattern for dialog management:
- Dynamic allocation of dialog
- Using
GUIUtil::ShowModalDialogAndDeleteOnClosefor automatic cleanupsrc/qt/walletview.cpp (3)
299-302: LGTM: Enhanced encryption dialog management with proper signal handling.The changes improve the encryption dialog by:
- Using dynamic allocation
- Connecting the finished signal to handle encryption status changes
- Using
ShowModalDialogAndDeleteOnClosefor automatic cleanup
327-329: LGTM: Consistent dialog management for password changes.The changes align with the new dialog management pattern using dynamic allocation and automatic cleanup.
335-339: LGTM: Improved unlock dialog management.The changes maintain consistency with the new dialog management pattern while preserving the existing unlock logic.
src/qt/bitcoingui.h (1)
257-257: LGTM! Good addition of the quit signal.The new
quitRequestedsignal enhances the shutdown process by providing a clean way for the GUI to notify other components about quit requests.src/qt/optionsdialog.cpp (1)
409-410: LGTM! Improved shutdown sequence.The change enhances the shutdown process by:
- Properly closing the dialog first
- Using signal/slot mechanism for quit requests instead of direct quit calls
src/qt/guiutil.h (1)
613-616: LGTM! Good addition of dialog management utility.The new
ShowModalDialogAndDeleteOnClosefunction improves memory management by ensuring proper cleanup of modal dialogs.src/qt/transactionview.cpp (2)
229-244: LGTM! Improved third-party URL handling.The changes enhance the context menu creation by:
- Adding separator only once when needed
- Using a more descriptive action label with the host name
- Using a lambda function for cleaner action handling
513-520: LGTM! Better dialog memory management.The changes improve dialog handling by:
- Using dynamic allocation for EditAddressDialog
- Utilizing ShowModalDialogAndDeleteOnClose for proper cleanup
Also applies to: 524-529
src/qt/bitcoin.cpp (3)
60-60: LGTM!Added QWindow header for enhanced window management functionality.
326-328: LGTM!Added a loop to hide all top-level windows before proceeding with shutdown, improving the shutdown sequence.
415-416: LGTM!Changed to use requestShutdown instead of quit for a more structured shutdown process.
src/qt/sendcoinsdialog.cpp (1)
941-943: LGTM!Improved dialog lifecycle management using GUIUtil::ShowModalDialogAndDeleteOnClose.
src/qt/guiutil.cpp (1)
1953-1958: LGTM!Added a utility function that provides a consistent way to manage dialog lifecycles by:
- Setting Qt::WA_DeleteOnClose attribute
- Setting Qt::ApplicationModal behavior
- Showing the dialog
src/qt/bitcoingui.cpp (4)
477-477: LGTM: Improved quit handlingChanged quit action to emit signal instead of directly calling quit, allowing for proper cleanup and confirmation before application exit.
903-904: LGTM: Added wallet loading activityAdded LoadWalletsActivity with proper parent-child relationship for memory management.
1096-1097: LGTM: Improved dialog memory managementChanged HelpMessageDialog handling to use ShowModalDialogAndDeleteOnClose utility function, ensuring proper cleanup of dialog resources.
1703-1703: LGTM: Consistent quit signal emissionChanged to emit quitRequested signal, consistent with the quit action handling change.
src/core_io.h (3)
48-48: LGTM! Parameter rename and default value addition.The parameter rename from
fIncludeHextoinclude_heximproves readability, and adding a default value forinclude_addressprovides better backward compatibility.
49-49: LGTM! Simplified function signature.Removing the
include_addressparameter aligns with the PR's objective to deprecate address-related functionality.
50-50: LGTM! Updated function signature.The changes:
- Remove the deprecated
include_addressesparameter- Add
serialize_flagsparameter with a default value- Maintain backward compatibility with optional parameters
src/test/fuzz/transaction.cpp (1)
97-98: LGTM! Updated function calls.The calls to
TxToUnivhave been correctly updated to remove the deprecatedinclude_addressesparameter, maintaining test coverage while aligning with the new interface.src/test/fuzz/script.cpp (2)
70-82: LGTM! Simplified destination extraction logic.The code now uses single destination extraction with appropriate assertions, aligning with the deprecation of multiple address extraction.
102-106: LGTM! Updated function calls.The calls to
ScriptPubKeyToUnivandScriptToUnivhave been correctly updated to match the new interfaces.src/core_write.cpp (3)
155-158: LGTM! Simplified implementation.The function now delegates to
ScriptPubKeyToUniv, reducing code duplication and improving maintainability.
160-174: LGTM! Streamlined address handling.The implementation now:
- Uses clearer parameter names
- Handles single address extraction
- Maintains consistent type information
176-326: LGTM! Updated transaction serialization.The implementation:
- Removes deprecated address handling
- Adds serialize flags support
- Maintains comprehensive transaction information including special transaction types
test/functional/feature_dip3_deterministicmns.py (1)
180-181: LGTM! Change aligns with deprecation of 'addresses' field.The update correctly modifies the test to check for the new single 'address' field instead of the deprecated 'addresses' array, consistent with the PR's breaking change that removes the
-deprecatedrpc=addressesconfiguration option.🧰 Tools
🪛 Ruff (0.8.2)
180-181: Use a single
ifstatement instead of nestedifstatements(SIM102)
src/bitcoin-tx.cpp (1)
694-694: LGTM! Change aligns with removal of addresses field.The update correctly modifies the
TxToUnivfunction call by removing theinclude_addressesparameter, consistent with the PR's breaking change that removes address-related fields.src/rpc/mining.cpp (1)
598-601: LGTM! Enhanced API response with capabilities information.The addition of the 'capabilities' field in the RPC result structure improves the API by explicitly listing supported features like 'proposal'. This helps clients better understand the server's capabilities.
src/rpc/net.cpp (1)
109-110: LGTM! Improved API robustness by marking fields as optional.The changes correctly mark several fields as optional in the RPC result structure, making the API more flexible and resilient. This is particularly beneficial for fields that may not always be available, such as bind addresses, local addresses, and ping-related information.
Also applies to: 112-112, 133-135
src/rpc/misc.cpp (1)
246-249: LGTM! Improved response structure by making fields optional.The changes make the
address,scriptPubKey, andisscriptfields optional in thevalidateaddressresponse, while adding a new optionalerrorfield. This is a good improvement as it:
- Makes the response structure more flexible
- Allows for proper error reporting
- Aligns with the PR objectives of streamlining RPC responses
src/rpc/rawtransaction.cpp (4)
151-193: LGTM! Improved getrawtransaction response structure.The changes make several fields optional in the
getrawtransactionresponse:
in_active_chainblockhashconfirmationsblocktimetimeaddressin scriptPubKeyThis is a good improvement as these fields may not always be available, especially for mempool transactions.
740-743: LGTM! Improved decoderawtransaction response structure.The changes make input fields optional in the
decoderawtransactionresponse:
coinbasetxidvoutscriptSigThis is a good improvement as these fields may not be present in certain transaction types.
1113-1121: LGTM! Improved testmempoolaccept response structure.The changes make several fields optional in the
testmempoolacceptresponse:
package-errorallowedvsizefeesreject-reasonThis is a good improvement as these fields may not be present when transactions cannot be fully validated due to failures in other transactions in the package.
1053-1054: LGTM! Improved analyzepsbt response structure.The changes make the
inputsarray optional in theanalyzepsbtresponse. This is appropriate since a PSBT may not always have inputs to analyze.src/rpc/blockchain.cpp (3)
172-172: LGTM: Removal of deprecated parameter.The removal of the
include_addressesparameter fromTxToUnivcall aligns with the PR objectives of removing deprecated RPC fields.
1067-1070: LGTM: Enhanced API flexibility with optional fields.The modification to mark fields as optional in
gettxoutsetinfoRPC result improves API flexibility, particularly when using coinstatsindex. This change provides better handling of cases where certain data might not be available.
1406-1427: LGTM: Well-documented optional fields.The documentation clearly indicates which fields are optional and under what conditions they appear in the response. This improves API usability and helps clients handle responses correctly.
src/wallet/rpcwallet.cpp (4)
348-348: LGTM! Field renaming improves consistency.The renaming of fields from "fee reason" to "fee_reason" improves API consistency by using underscores instead of spaces in JSON field names.
Also applies to: 793-793
1102-1103: LGTM! Documentation improvements for optional fields.The documentation has been enhanced by explicitly marking optional fields with the
/* optional */ trueflag, improving API clarity for users.Also applies to: 1153-1154, 1290-1301
4104-4108: LGTM! Improved RPCResult documentation.The RPCResult fields for the send RPC have been correctly marked as optional using
/* optional */ true, providing better clarity about which fields may be omitted in the response.
1659-1659: Verify the impact of removing deprecated parameters.The call to TxToUniv() has been updated to remove deprecated parameters. This aligns with the PR objectives but we should verify the change doesn't break existing functionality.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other calls to TxToUniv to ensure consistent parameter usage rg "TxToUniv" -A 2 -B 2Length of output: 4683
Verified Impact of Removing Deprecated Parameters
- The
TxToUnivfunction insrc/core_io.his declared with default parameters for the deprecated arguments.- In
src/wallet/rpcwallet.cpp, the callTxToUniv(*wtx.tx, uint256(), decoded, false);now omits the deprecated parameters, relying on their default values.- A scan of the repository shows that other usages either pass all parameters explicitly or also rely on the default values, ensuring consistent usage without breaking existing functionality.
ci/test/00_setup_env_s390x.sh (1)
22-22:⚠️ Potential issueFix typo in test name.
There is a typo in the excluded test name:
fature_bind_extrashould befeature_bind_extra.-export TEST_RUNNER_EXTRA="--exclude rpc_bind,fature_bind_extra" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547 +export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547Likely an incorrect or invalid review comment.
.cirrus.yml (1)
63-67: Verify ARM container image compatibility.The ARM task has been updated to use Debian Bullseye. Please ensure that all required dependencies and build tools are available in this newer version.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the required packages are available in Debian Bullseye for ARM architecture. # Get list of available packages for ARM architecture curl -s https://deb.debian.org/debian/dists/bullseye/main/binary-arm64/Packages.gz | gunzip | grep -A 10 "^Package:"Length of output: 69751
ARM container image compatibility appears verified.
The output from fetching Bullseye’s ARM (arm64) package list shows a rich repository containing many required libraries, build tools, and dependencies. This indicates that the Debian Bullseye image for ARM should support the necessary packages for unit tests and builds.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 64-64: too many spaces before colon
(colons)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/feature_dip3_deterministicmns.py (1)
180-181: Simplify nested conditions.The nested
ifstatements can be combined for better readability.- if 'address' in out['scriptPubKey']: - if expected_payee in out['scriptPubKey']['address'] and out['valueSat'] == expected_amount: + if ('address' in out['scriptPubKey'] and + expected_payee in out['scriptPubKey']['address'] and + out['valueSat'] == expected_amount):🧰 Tools
🪛 Ruff (0.8.2)
180-181: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
doc/release-notes-22650.md(1 hunks)src/bitcoin-tx.cpp(1 hunks)src/core_io.h(1 hunks)src/core_write.cpp(2 hunks)src/rpc/blockchain.cpp(4 hunks)src/rpc/blockchain.h(0 hunks)src/rpc/rawtransaction.cpp(14 hunks)src/script/standard.cpp(0 hunks)src/script/standard.h(1 hunks)src/test/fuzz/script.cpp(2 hunks)src/test/fuzz/transaction.cpp(1 hunks)src/test/script_standard_tests.cpp(0 hunks)src/wallet/rpcwallet.cpp(17 hunks)test/functional/feature_dip3_deterministicmns.py(1 hunks)test/functional/rpc_addresses_deprecation.py(0 hunks)test/functional/test_runner.py(0 hunks)
💤 Files with no reviewable changes (5)
- src/test/script_standard_tests.cpp
- test/functional/test_runner.py
- test/functional/rpc_addresses_deprecation.py
- src/script/standard.cpp
- src/rpc/blockchain.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/bitcoin-tx.cpp
- doc/release-notes-22650.md
- src/test/fuzz/transaction.cpp
- src/core_io.h
🧰 Additional context used
🪛 Ruff (0.8.2)
test/functional/feature_dip3_deterministicmns.py
180-181: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (25)
src/script/standard.h (1)
124-126: LGTM! Documentation update accurately reflects supported script types.The documentation now correctly specifies that
ExtractDestinationonly supports P2PK, P2PKH, and P2SH scripts, aligning with the removal of multi-destination extraction functionality.src/test/fuzz/script.cpp (1)
70-82: LGTM! Test cases updated to reflect single destination extraction.The test cases have been correctly updated to use
ExtractDestinationinstead ofExtractDestinations, with appropriate assertions for different script types (PUBKEY, NONSTANDARD, NULL_DATA, MULTISIG).src/core_write.cpp (3)
155-158: LGTM! SimplifiedScriptToUnivimplementation.The function now delegates to
ScriptPubKeyToUniv, reducing code duplication and improving maintainability.
160-174: LGTM! UpdatedScriptPubKeyToUnivto handle single address.The function has been simplified to handle single address extraction, aligning with the removal of multi-destination support.
176-326: LGTM! UpdatedTxToUnivsignature and implementation.The function signature has been updated to include
serialize_flagsand removeinclude_addresses, reflecting the changes in address handling.src/rpc/rawtransaction.cpp (7)
99-99: LGTM: Improved parameter passing to TxToUnivThe change from
0tonullptrfor thetxundoparameter makes the code more explicit and type-safe.
151-194: LGTM: Enhanced RPC result documentationThe changes correctly mark several fields as optional in the
getrawtransactionRPC result:
in_active_chain: Only present with explicit "blockhash" argumentblockhash,confirmations,blocktime,time: Optional fieldsaddress: Optional field for well-defined addresses only
740-743: LGTM: Improved RPC result documentation for decoderawtransactionThe changes correctly mark input fields as optional in the
decoderawtransactionRPC result:
coinbase,txid,vout,scriptSig: Optional fields for transaction inputs
804-804: LGTM: Enhanced RPC result documentation for decodescriptThe change correctly marks the
addressfield as optional in thedecodescriptRPC result, indicating it's only present for well-defined addresses.
2053-2054: LGTM: Enhanced RPC result documentation for analyzepsbtThe change correctly marks the
inputsarray as optional in theanalyzepsbtRPC result.
1700-1701: LGTM: Enhanced RPC result documentation for finalizepsbtThe changes correctly mark the following fields as optional in the
finalizepsbtRPC result:
psbt: Present when the transaction is not extractedhex: Present when the transaction is extracted
1477-1477: Verify the impact of ScriptToUniv function callsThe function calls to
ScriptToUnivhave been modified to use a simpler signature. Ensure this change is consistent with the function's declaration.Also applies to: 1575-1575
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for ScriptToUniv function declaration rg -A 5 "ScriptToUniv" src/core_io.hLength of output: 336
Consistent ScriptToUniv Signature
Both calls in
src/rpc/rawtransaction.cpp(lines 1477 and 1575) now pass two arguments—which matches the declaration insrc/core_io.h:void ScriptToUniv(const CScript& script, UniValue& out);No inconsistencies were found.
src/rpc/blockchain.cpp (4)
1067-1070: Fields marked as optional in gettxoutsetinfo RPC result.The following fields have been marked as optional in the RPC result:
transactions: Only available when coinstatsindex is not useddisk_size: Only available when coinstatsindex is not usedtotal_unspendable_amount: Only available when coinstatsindex is used
1406-1407: Fields marked as optional in getblockchaininfo RPC result.The following fields have been marked as optional in the RPC result:
bip9object: Only present for "bip9" typebitfield: Only present for "started" and "locked_in" statusstatisticsobject: Only present for "started" and "locked_in" statusthresholdfield: Only present for "started" statuspossiblefield: Only present for "started" statusheightfield: Only present for "buried" type or "bip9" type with "active" statusAlso applies to: 1410-1410, 1417-1418, 1420-1420, 1423-1424, 1426-1427
1927-1959: Fields marked as optional in getblockstats RPC result.All fields in the getblockstats RPC result have been marked as optional, which is correct since they depend on the requested stats parameter. This includes fields like avgfee, avgfeerate, avgtxsize, blockhash, etc.
172-172: Verify the updated TxToUniv function call.The function call has been updated to use the new signature, removing the
include_addressesparameter and adding a0as theserialize_flagsparameter. This aligns with the removal of the deprecatedaddressesfield mentioned in the PR objectives.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Verify the TxToUniv function signature in core_io.h rg -A 2 "void TxToUniv" src/core_io.hLength of output: 282
TxToUniv Parameter Update Verified
The updated function call correctly reflects the new signature from
src/core_io.h, with the removal of the deprecatedinclude_addressesparameter and the explicit0provided forserialize_flags. No issues were found.src/wallet/rpcwallet.cpp (9)
348-348: LGTM! Improved field name consistencyThe field name change from
fee reasontofee_reasonin sendtoaddress RPC result follows the standard naming convention of using underscores for field names.
793-793: LGTM! Consistent field name changeThe same field name change from
fee reasontofee_reasonis applied to sendmany RPC result, maintaining consistency across the API.
1102-1103: LGTM! Improved optional field documentationThe
involvesWatchonlyfield in listreceivedbyaddress RPC result is now properly marked as optional with a clear description.
1153-1154: LGTM! Consistent optional field documentationThe same improvement to
involvesWatchonlyfield documentation is applied to listreceivedbylabel RPC result.
1321-1322: LGTM! Enhanced optional fields documentationThe listtransactions RPC result documentation has been improved by:
- Properly marking
involvesWatchonlyas optional- Adding optional flag for
labelfield- Adding optional flag for
feefield- Properly marking
abandonedfield as optional with clear descriptionAlso applies to: 1334-1338, 1341-1342
1438-1439: LGTM! Comprehensive optional fields documentationThe listsinceblock RPC result documentation has been enhanced with:
- Proper optional flags for
involvesWatchonlyandfeefields- Optional flags for
abandonedandlabelfields- Clear documentation for the
removedarray fieldAlso applies to: 1450-1452, 1455-1457, 1460-1462
1571-1572: LGTM! Improved field documentationThe gettransaction RPC result documentation has been enhanced with:
- Optional flag for
feefield with clear description- Optional flags for
involvesWatchonly,address, andlabelfields- Optional flags for
feeandabandonedfields with descriptionsAlso applies to: 1580-1582, 1591-1596
1605-1607: LGTM! Enhanced optional fields documentationThe send RPC result documentation has been improved by properly marking
txid,hex, andpsbtfields as optional with clear descriptions.
1659-1659: Verify the impact of removing thehashBlockparameterThe change to TxToUniv call removes the
hashBlockparameter. This could affect transaction details in the decoded output.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for other TxToUniv calls to verify consistency rg "TxToUniv.*hashBlock" -A 2Length of output: 988
Action: Confirm Inconsistent tx decoding due to default hashBlock
The output confirms that while many TxToUniv call sites (e.g. in src/rest.cpp and src/test/fuzz/transaction.cpp) supply an explicit block hash value, the call in src/wallet/rpcwallet.cpp now passes a default-constructeduint256()(i.e. an empty hash). This means that block-related information in the decoded transaction output will be absent for wallet RPC responses.Please verify that this omission is intentional, as downstream consumers of decoded RPC output may expect block hash details when available.
|
This pull request has conflicts, please rebase. |
…tra for two configurations fa660de ci: Update valgrind config (MarcoFalke) fad5dbc ci: Update s390x config (MarcoFalke) Pull request description: (See commit messages) ACKs for top commit: fanquake: ACK fa660de Tree-SHA512: b5bf7c89042d5d8f49a92c59b934185447ae42bd055ccb1c6edb98fe104a35eb2d6c26d810bec9f52d545d09599f3d33fc8606fbfb793862610d276b2a4bac6c
…n third-party link action 2ccde2f qt: hyphenate usage of third-party modifier (Jarol Rodriguez) 8177578 qt: ensure seperator when adding third-party transaction links (Jarol Rodriguez) a70a980 qt: improve text for open third-party tx url action (Jarol Rodriguez) 9980f4a qt, refactor: simplify third-party tx url action through overload (Jarol Rodriguez) Pull request description: [dashpay#4092](bitcoin#4092) introduced the ability to open up a transaction in a block explorer. This improves the related code by simplifying the addition and connection of the action through an [overloaded](https://doc.qt.io/archives/qt-5.9/qmenu.html#addAction-5) `addAction` function and prepends action description text to the host, "Show in". The reason to add this text is to make it clear what the action does. It also creates a clearer mental correlation between a user doing the work to add the 3rd-party tx link and this new menu action popping up. This updates the setting text so that "third-party" is hyphenated. It should be hyphenated because it is being used as a modifier of both "URL" and "transaction URLs". Additionally, this fixes dashpay#431 by ensuring that the seperator will be added before creating action. Screenshots of visual changes: **Context menu actions** | master | pr | |--------------|--------| | <img width="248" alt="3pt-master" src="https://user-images.githubusercontent.com/23396902/134618354-00278ac6-5094-44ee-8ba7-fe648fdcb7d2.png"> | <img width="248" alt="3pt-pr" src="https://user-images.githubusercontent.com/23396902/134618364-ddb64269-e5ee-40af-a2a6-1922001b6f4e.png"> | **Setting text** (tooltip text containing usage of "third-party" is also properly hyphenated) | master | pr | |--------------|--------| |  |  | ACKs for top commit: stratospher: tested ACK 2ccde2f. promag: Code view ACK 2ccde2f. hebasto: ACK 2ccde2f Tree-SHA512: 8dfcd539a1d41c8abf3c8208d150d1480d4ef81a008de826299e8bad1dfa6e3c49dc76d041c5946fafcf0b033eebb9b9fbd3d49ba6d8af93dd388c488e92f143
2b90eae doc: update developer docs for subtree renaming (fanquake) Pull request description: Update the developer docs after the [recent subtree renaming](bitcoin#22646 (comment)). ACKs for top commit: hebasto: ACK 2b90eae Tree-SHA512: ed0eec8db888e60595c07f4fad0a506673e4b10345fb2dd6d1a98d785da22bddf1fe8896aa52fd67f5e1688e3c91c6b642739e08646f1b920f50f0d35037d961
…during shutdown 451ca24 qt, refactor: Drop intermediate BitcoinApplication::shutdownResult slot (Hennadii Stepanov) f3a17bb qt: Do not exit and re-enter main event loop during shutdown (Hennadii Stepanov) b4e0d2c qt, refactor: Allocate SendConfirmationDialog instances on heap (Hennadii Stepanov) 332dea2 qt, refactor: Keep HelpMessageDialog in the main event loop (Hennadii Stepanov) c8bae37 qt, refactor: Keep PSBTOperationsDialog in the main event loop (Hennadii Stepanov) 7fa91e8 qt, refactor: Keep AskPassphraseDialog in the main event loop (Hennadii Stepanov) 6f6fde3 qt, refactor: Keep EditAddressDialog in the main event loop (Hennadii Stepanov) 59f7ba4 qt, refactor: Keep CoinControlDialog in the main event loop (Hennadii Stepanov) 7830cd0 qt, refactor: Keep OptionsDialog in the main event loop (Hennadii Stepanov) 13f6188 qt: Add GUIUtil::ShowModalDialogAndDeleteOnClose (Hennadii Stepanov) Pull request description: On master (1ef34ee) during shutdown `QApplication` exits the main event loop, then re-enter again. This PR streamlines shutdown process by removing the need to interrupt the main event loop, that is required for dashpay#59. Also, blocking [`QDialog::exec()`](https://doc.qt.io/qt-5/qdialog.html#exec) calls are replaced with safer [`QDialog::show()`](https://doc.qt.io/qt-5/qwidget.html#show), except for `SendConfirmationDialog` as that change is not trivial (marked as TODO). The [`QDialog::open()`](https://doc.qt.io/qt-5/qdialog.html#open) was not used because the actual modality mode (application modal or window modal) of a dialog depends on whether it has a parent. This PR does not change behavior, and all touched dialogs are still application modal. As a follow up, a design research could suggest to make some dialogs window modal. NOTE for reviewers: quitting app while a dialog is open (e.g., via systray icon menu) must work fine. ACKs for top commit: laanwj: Code review and lighly tested ACK 451ca24 promag: ACK 451ca24, just changed signal to `quitRequested`. Tree-SHA512: ef01ab6ed803b202e776019a4e1f592e816f7bc786e00574b25a0bf16be2374ddf9db21f0a26da08700df7ef0ab9e879550df46dcfe3b6d940f5ed02ca5f8447
… main GUI thread 2fe69ef qt, wallet: Drop no longer used WalletController::getOpenWallets() (Hennadii Stepanov) f6991cb qt, wallet: Add LoadWalletsActivity class (Hennadii Stepanov) 4a024fc qt, wallet, refactor: Move connection to QObject::deleteLater to ctor (Hennadii Stepanov) f9b633e qt, wallet: Move activity progress dialog from data member to local (Hennadii Stepanov) Pull request description: This PR improves the GUI responsiveness during initial wallets loading at startup (especially ones that have tons of txs), and shows a standard progress dialog for long loading:  Fixes dashpay#247. ACKs for top commit: ryanofsky: Code review ACK 2fe69ef. Just suggested changes since last review: squashing commits and dropping unused method (thanks!) shaavan: reACK 2fe69ef promag: Code review ACK 2fe69ef. Tree-SHA512: 2ac3cb48886e0005fc36b3fd0c2b35abd557186be16db3145d753c34d94188e4f4ff14dc07fb0fb7558944f84498204a3988f8284fd56c6d85b47bc9081e71a6
6e1150e fuzz: add guide to fuzzing with Eclipser v1.x (Alex Groce) Pull request description: MarcoFalke and practicalswift here's an Eclipser guide, reconstructed from their documentation and my docker history getting it up and running. It might be good if someone confirmed it actually works for them in a fresh ubuntu 20.04. ACKs for top commit: practicalswift: ACK 6e1150e Tree-SHA512: ca855932fd7a2c1d1005d572ab5fabc26f42d779f9baf279783f08a43dd72ec60f57239135d30c2a82781e593626fec2c96bb19fb91e1b777cef2d83a54eba35
BACKPORT NOTICE: missing: - field `signet_challenge` in rpc/mining.cpp - changes in `rpc/external_signer.cpp` fa10fbc doc: Fix RPC result documentation (MarcoFalke) Pull request description: Fix: * Incorrectly named fields * Add missing ones * Add missing optional flag ACKs for top commit: fanquake: ACK fa10fbc Tree-SHA512: 2b302e6f7ac8253a55882bc032ddda1932b728abddd12b0adb5fba814b12fb998a67b91e6dd124ebbe0ac16dccdace01332ade01c1dc00a3768647118dd3a2d2
…onding code/logic 43cd6b8 doc: add release notes for removal of the -deprecatedrpc=addresses flag (Michael Dietz) 2b1fdc2 refactor: minor styling, prefer snake case and same line if (Michael Dietz) d64deac refactor: share logic between ScriptPubKeyToUniv and ScriptToUniv (Michael Dietz) 8721638 rpc: remove deprecated addresses and reqSigs from rpc outputs (Michael Dietz) Pull request description: Resolves bitcoin#21797 now that we've branched-off to v23 ("addresses" and "reqSigs" deprecated) "ExtractDestinations" should be removed. `-deprecatedrpc=addresses` was initially added in this PR bitcoin#20286 (which resolved the original issue bitcoin#20102). Some chunks of code and logic are no longer used/necessary with the removal of this, and therefore some minor refactoring is done in this PR as well (separated commits) ACKs for top commit: MarcoFalke: re-ACK 43cd6b8 🐉 meshcollider: Code review ACK 43cd6b8 jonatack: ACK 43cd6b8 per `git range-diff a9d0cec 92dc5e9 43cd6b8`, also rebased to latest master, debug built + quick re-review of each commit to bring back context, and ran tests locally at the final commit Tree-SHA512: fba83495e396d3c06f0dcf49292f14f4aa6b68fa758f0503941fade1a6e7271cda8378e2734af1faea550d1b43c85a36c52ebcc9dec0732936f9233b4b97901c
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/functional/feature_dip3_deterministicmns.py (1)
28-28:⚠️ Potential issueRemove deprecated RPC flag.
The
-deprecatedrpc=addressesflag is being removed as per the PR objectives. This test should be updated to work without the deprecated flag.Apply this diff to remove the deprecated flag:
- self.extra_args = ["-deprecatedrpc=addresses"] + self.extra_args = []
🧹 Nitpick comments (7)
test/lint/README.md (1)
30-33: Fix markdown formatting issues.The unordered list items use asterisks instead of dashes, and the URLs are not properly formatted with angle brackets.
Apply this diff to fix the formatting:
-* for `src/leveldb`: https://github.com/bitcoin-core/leveldb-subtree.git (branch bitcoin-fork) -* for `src/univalue`: https://github.com/bitcoin-core/univalue-subtree.git (branch master) -* for `src/crypto/ctaes`: https://github.com/bitcoin-core/ctaes.git (branch master) -* for `src/crc32c`: https://github.com/bitcoin-core/crc32c-subtree.git (branch bitcoin-fork) +- for `src/leveldb`: <https://github.com/bitcoin-core/leveldb-subtree.git> (branch bitcoin-fork) +- for `src/univalue`: <https://github.com/bitcoin-core/univalue-subtree.git> (branch master) +- for `src/crypto/ctaes`: <https://github.com/bitcoin-core/ctaes.git> (branch master) +- for `src/crc32c`: <https://github.com/bitcoin-core/crc32c-subtree.git> (branch bitcoin-fork)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
30-30: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
30-30: Bare URL used
null(MD034, no-bare-urls)
31-31: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
31-31: Bare URL used
null(MD034, no-bare-urls)
32-32: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
32-32: Bare URL used
null(MD034, no-bare-urls)
33-33: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
33-33: Bare URL used
null(MD034, no-bare-urls)
doc/developer-notes.md (3)
1135-1138: Improve clarity of subtree maintenance policy.The sentence contains repetitive use of "directly". Consider rephrasing for better readability.
-Some of these are maintained by active developers of Dash Core, in which case changes should go directly upstream without being PRed directly against the project. +Some of these are maintained by active developers of Dash Core, in which case changes should be submitted upstream without being PRed against the project.🧰 Tools
🪛 LanguageTool
[style] ~1136-~1136: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...go directly upstream without being PRed directly against the project. They will be merge...(ADVERB_REPETITION_PREMIUM)
1139-1143: Fix grammatical issue in external subtree policy.Remove the unnecessary comma before "so that" to improve readability.
-to these should also be sent upstream, but bugfixes may also be prudent to PR against a Dash Core subtree, so that they can be integrated quickly. +to these should also be sent upstream, but bugfixes may also be prudent to PR against a Dash Core subtree so that they can be integrated quickly.🧰 Tools
🪛 LanguageTool
[typographical] ~1141-~1141: The conjunction “so that” does not have a comma in front.
Context: ...rudent to PR against a Dash Core subtree, so that they can be integrated quickly. Cosmeti...(SO_THAT_UNNECESSARY_COMMA)
1149-1170: Fix bare URLs in subtree references.The URLs should be properly formatted as markdown links for better readability and to follow markdown best practices.
- - Subtree at https://github.com/bitcoin-core/leveldb-subtree ; maintained by Core contributors. - - Upstream at https://github.com/google/leveldb ; maintained by Google. Open + - Subtree at [bitcoin-core/leveldb-subtree](https://github.com/bitcoin-core/leveldb-subtree); maintained by Core contributors. + - Upstream at [google/leveldb](https://github.com/google/leveldb); maintained by Google. Open - - Subtree at https://github.com/bitcoin-core/crc32c-subtree ; maintained by Core contributors. - - Upstream at https://github.com/google/crc32c ; maintained by Google. + - Subtree at [bitcoin-core/crc32c-subtree](https://github.com/bitcoin-core/crc32c-subtree); maintained by Core contributors. + - Upstream at [google/crc32c](https://github.com/google/crc32c); maintained by Google. - - Upstream at https://github.com/bitcoin-core/secp256k1/ ; maintained by Core contributors. + - Upstream at [bitcoin-core/secp256k1](https://github.com/bitcoin-core/secp256k1/); maintained by Core contributors. - - Upstream at https://github.com/bitcoin-core/ctaes ; maintained by Core contributors. + - Upstream at [bitcoin-core/ctaes](https://github.com/bitcoin-core/ctaes); maintained by Core contributors. - - Subtree at https://github.com/bitcoin-core/univalue-subtree ; maintained by Core contributors. - - Deviates from upstream https://github.com/jgarzik/univalue. + - Subtree at [bitcoin-core/univalue-subtree](https://github.com/bitcoin-core/univalue-subtree); maintained by Core contributors. + - Deviates from upstream [jgarzik/univalue](https://github.com/jgarzik/univalue).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1150-1150: Bare URL used
null(MD034, no-bare-urls)
1151-1151: Bare URL used
null(MD034, no-bare-urls)
1158-1158: Bare URL used
null(MD034, no-bare-urls)
1159-1159: Bare URL used
null(MD034, no-bare-urls)
1162-1162: Bare URL used
null(MD034, no-bare-urls)
1165-1165: Bare URL used
null(MD034, no-bare-urls)
1168-1168: Bare URL used
null(MD034, no-bare-urls)
1169-1169: Bare URL used
null(MD034, no-bare-urls)
src/qt/walletcontroller.cpp (1)
336-352: Consider enhancing the wallet loading implementation.While the basic implementation is functional, consider these improvements:
- Add error handling for failed wallet loading
- Show progress updates for each wallet being loaded
- Consider using a QProgressDialog with a determinate range based on wallet count
Here's a suggested enhancement:
void LoadWalletsActivity::load() { - showProgressDialog(tr("Loading wallets…"), tr("Loading wallets…")); + auto wallets = node().walletLoader().getWallets(); + if (wallets.empty()) { + Q_EMIT finished(); + return; + } + + showProgressDialog(tr("Loading wallets…"), + tr("Loading wallet 1/%1…").arg(wallets.size())); QTimer::singleShot(0, worker(), [this] { - for (auto& wallet : node().walletLoader().getWallets()) { - m_wallet_controller->getOrCreateWallet(std::move(wallet)); + auto wallets = node().walletLoader().getWallets(); + int count = 0; + for (auto& wallet : wallets) { + ++count; + try { + m_wallet_controller->getOrCreateWallet(std::move(wallet)); + QMetaObject::invokeMethod(this, [this, count, total=wallets.size()] { + if (count < total) { + showProgressDialog(tr("Loading wallets…"), + tr("Loading wallet %1/%2…").arg(count + 1).arg(total)); + } + }, Qt::QueuedConnection); + } catch (const std::exception& e) { + qWarning() << "Failed to load wallet:" << e.what(); + } } QTimer::singleShot(0, this, [this] { Q_EMIT finished(); }); }); }test/functional/feature_dip3_deterministicmns.py (1)
180-182: Simplify conditional checks and update address validation.The nested if statements can be simplified, and the code has been correctly updated to use the new 'address' field instead of the deprecated 'addresses' field.
Apply this diff to simplify the conditional checks:
- if 'address' in out['scriptPubKey']: - if expected_payee in out['scriptPubKey']['address'] and out['valueSat'] == expected_amount: - found_multisig_payee = True + if ('address' in out['scriptPubKey'] and + expected_payee in out['scriptPubKey']['address'] and + out['valueSat'] == expected_amount): + found_multisig_payee = True🧰 Tools
🪛 Ruff (0.8.2)
180-181: Use a single
ifstatement instead of nestedifstatements(SIM102)
src/qt/bitcoingui.cpp (1)
903-904: Ensure proper lifecycle management of LoadWalletsActivity
Currently, there's no direct signal-slot mechanism that deletes theLoadWalletsActivityobject when finished. Consider connecting to its completion signal or using a managed pointer to avoid potential memory leaks.connect(activity, &LoadWalletsActivity::finished, activity, &QObject::deleteLater);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
ci/test/00_setup_env_native_valgrind.sh(1 hunks)ci/test/00_setup_env_s390x.sh(1 hunks)doc/developer-notes.md(1 hunks)doc/fuzzing.md(1 hunks)doc/release-notes-22650.md(1 hunks)src/bitcoin-tx.cpp(1 hunks)src/core_io.h(1 hunks)src/core_write.cpp(2 hunks)src/qt/addressbookpage.cpp(1 hunks)src/qt/bitcoin.cpp(5 hunks)src/qt/bitcoin.h(1 hunks)src/qt/bitcoingui.cpp(6 hunks)src/qt/bitcoingui.h(1 hunks)src/qt/forms/optionsdialog.ui(2 hunks)src/qt/guiutil.cpp(2 hunks)src/qt/guiutil.h(2 hunks)src/qt/optionsdialog.cpp(1 hunks)src/qt/optionsdialog.h(1 hunks)src/qt/sendcoinsdialog.cpp(2 hunks)src/qt/transactionview.cpp(2 hunks)src/qt/walletcontroller.cpp(2 hunks)src/qt/walletcontroller.h(2 hunks)src/qt/walletframe.cpp(1 hunks)src/qt/walletview.cpp(2 hunks)src/rpc/blockchain.cpp(4 hunks)src/rpc/blockchain.h(0 hunks)src/rpc/mining.cpp(1 hunks)src/rpc/misc.cpp(2 hunks)src/rpc/net.cpp(2 hunks)src/rpc/rawtransaction.cpp(14 hunks)src/script/standard.cpp(0 hunks)src/script/standard.h(1 hunks)src/test/fuzz/script.cpp(2 hunks)src/test/fuzz/transaction.cpp(1 hunks)src/test/script_standard_tests.cpp(0 hunks)src/wallet/rpcwallet.cpp(17 hunks)test/functional/feature_dip3_deterministicmns.py(1 hunks)test/functional/rpc_addresses_deprecation.py(0 hunks)test/functional/test_runner.py(0 hunks)test/lint/README.md(1 hunks)
💤 Files with no reviewable changes (5)
- test/functional/test_runner.py
- test/functional/rpc_addresses_deprecation.py
- src/rpc/blockchain.h
- src/test/script_standard_tests.cpp
- src/script/standard.cpp
🚧 Files skipped from review as they are similar to previous changes (22)
- src/qt/optionsdialog.h
- ci/test/00_setup_env_s390x.sh
- ci/test/00_setup_env_native_valgrind.sh
- src/qt/guiutil.h
- src/qt/forms/optionsdialog.ui
- doc/release-notes-22650.md
- src/qt/optionsdialog.cpp
- src/qt/walletframe.cpp
- src/qt/bitcoingui.h
- src/bitcoin-tx.cpp
- doc/fuzzing.md
- src/rpc/mining.cpp
- src/qt/bitcoin.h
- src/qt/sendcoinsdialog.cpp
- src/test/fuzz/transaction.cpp
- src/qt/transactionview.cpp
- src/qt/addressbookpage.cpp
- src/qt/walletview.cpp
- src/core_io.h
- src/qt/guiutil.cpp
- src/rpc/net.cpp
- src/rpc/blockchain.cpp
🧰 Additional context used
🪛 LanguageTool
doc/developer-notes.md
[style] ~1136-~1136: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...go directly upstream without being PRed directly against the project. They will be merge...
(ADVERB_REPETITION_PREMIUM)
[typographical] ~1141-~1141: The conjunction “so that” does not have a comma in front.
Context: ...rudent to PR against a Dash Core subtree, so that they can be integrated quickly. Cosmeti...
(SO_THAT_UNNECESSARY_COMMA)
🪛 markdownlint-cli2 (0.17.2)
doc/developer-notes.md
1150-1150: Bare URL used
null
(MD034, no-bare-urls)
1151-1151: Bare URL used
null
(MD034, no-bare-urls)
1158-1158: Bare URL used
null
(MD034, no-bare-urls)
1159-1159: Bare URL used
null
(MD034, no-bare-urls)
1162-1162: Bare URL used
null
(MD034, no-bare-urls)
1165-1165: Bare URL used
null
(MD034, no-bare-urls)
1168-1168: Bare URL used
null
(MD034, no-bare-urls)
1169-1169: Bare URL used
null
(MD034, no-bare-urls)
test/lint/README.md
30-30: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
30-30: Bare URL used
null
(MD034, no-bare-urls)
31-31: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
31-31: Bare URL used
null
(MD034, no-bare-urls)
32-32: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
32-32: Bare URL used
null
(MD034, no-bare-urls)
33-33: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
33-33: Bare URL used
null
(MD034, no-bare-urls)
🪛 Ruff (0.8.2)
test/functional/feature_dip3_deterministicmns.py
180-181: Use a single if statement instead of nested if statements
(SIM102)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (28)
src/rpc/misc.cpp (2)
246-249: Breaking change: Fields in validateaddress response are now optional.The fields "address", "scriptPubKey", and "isscript" in the validateaddress RPC response are now marked as optional. This change aligns with the PR objectives of removing deprecated fields.
Please ensure all API consumers are updated to handle cases where these fields might be missing from the response.
1331-1331: LGTM: Return type change matches implementation.The return type change to
OBJ_DYNcorrectly reflects that the response object's keys are dynamic, based on the available indices.src/test/fuzz/script.cpp (1)
70-82: LGTM! The changes simplify destination extraction logic.The changes correctly align with the removal of multi-destination extraction functionality, replacing it with single destination extraction using
ExtractDestination. The assertions have been appropriately updated to validate script types when extraction fails.src/core_write.cpp (3)
155-158: LGTM! Simplified script serialization by consolidating logic.The changes correctly consolidate script serialization logic by having
ScriptToUnivuseScriptPubKeyToUniv, reducing code duplication.
160-173: LGTM! Updated ScriptPubKeyToUniv to handle single address extraction.The changes correctly update the function to focus on single address extraction, aligning with the removal of deprecated multi-destination functionality.
244-245: LGTM! Updated TxToUniv call to match new signature.The call to
ScriptPubKeyToUnivhas been correctly updated to match its new signature.src/rpc/rawtransaction.cpp (3)
151-152: LGTM! Correctly marked fields as optional in RPC results.The changes appropriately mark fields like
in_active_chain,blockhash,confirmations,blocktime, andtimeas optional in the RPC results, improving API documentation accuracy.Also applies to: 189-193
740-743: LGTM! Updated input field documentation.The changes correctly mark
coinbase,txid,vout, andscriptSigfields as optional in the RPC input documentation.
1113-1121: LGTM! Updated testmempoolaccept result documentation.The changes appropriately mark fields like
package-error,allowed,vsize,fees, andreject-reasonas optional in the RPC results.src/script/standard.h (2)
124-127: Documentation update accurately reflects supported script types.The updated documentation clearly specifies that
ExtractDestinationonly works for P2PK, P2PKH, and P2SH scripts, which aligns with the removal of multi-destination support.
1-143:❓ Verification inconclusive
Verify impact of ExtractDestinations removal.
The removal of
ExtractDestinationsis a breaking change that affects multiple RPCs. As mentioned in the PR objectives, this impacts:
gettxoutgetrawtransactiondecoderawtransactiondecodescriptgettransaction verbose=true- REST endpoints (
/rest/tx,/rest/getutxos,/rest/block)These endpoints will no longer return the
addressesandreqSigsfields.Let's verify all affected RPC implementations have been updated:
🏁 Script executed:
#!/bin/bash # Search for RPC implementations that might still use ExtractDestinations rg -A 5 "ExtractDestinations" # Search for RPC response fields that need to be removed rg -A 5 '"addresses":|"reqSigs":'Length of output: 6433
Confirm RPC Updates per PR Objectives
After reviewing the repository, I found that all references to the removed
ExtractDestinationsfunction have been eliminated. Additionally, searching for the response fields"addresses"and"reqSigs"shows that these fields are no longer present where they previously existed in the affected RPC endpoints. Note that test cases intest/functional/feature_addressindex.pystill include theaddresseskey in the request payloads, which is expected since the change only affects the output structure.Please ensure that the following endpoints have been updated to omit the
addressesandreqSigsfields in their responses:
gettxoutgetrawtransactiondecoderawtransactiondecodescriptgettransaction verbose=true- REST endpoints (
/rest/tx,/rest/getutxos,/rest/block)src/qt/walletcontroller.h (2)
89-89: LGTM! Good use of modern C++ practices.The change to use a default destructor is appropriate since memory management is now handled through Qt's parent-child relationship and signal-slot mechanism.
147-155: LGTM! Clean and focused class design.The LoadWalletsActivity class follows good design principles:
- Single responsibility principle (SRP) - focused on wallet loading
- Inherits common functionality from WalletControllerActivity
- Simple and clear interface
src/qt/walletcontroller.cpp (3)
185-185: LGTM! Proper memory management.Good use of Qt's memory management by connecting the finished signal to deleteLater(), ensuring proper cleanup of activity objects.
190-202: LGTM! Improved progress dialog management.The changes demonstrate good practices:
- Dynamic dialog creation with proper cleanup via DeleteOnClose
- Proper signal connection for dialog closure
- Effective use of Qt's parent-child relationship
341-352: Implementation aligns with PR objectives.The asynchronous wallet loading implementation supports the PR's goal of improving loading time for heavy wallets by:
- Moving wallet loading to a worker thread
- Providing user feedback through progress dialog
- Avoiding UI blocking during load operations
test/functional/feature_dip3_deterministicmns.py (1)
171-183:❓ Verification inconclusive
Verify multisig payee validation logic.
The test should be updated to handle cases where the multisig address might not be present in the scriptPubKey, as the 'addresses' field is being removed from various RPCs.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any other tests relying on the deprecated 'addresses' field rg -l "scriptPubKey.*addresses" test/functional/Length of output: 50
Multisig Payee Validation Logic: Manual Verification Recommended
While a search for deprecated use of the “addresses” field in the test files produced no results, please manually verify that the multisig payee logic in
test/functional/feature_dip3_deterministicmns.py(lines 171–183) correctly handles cases where the multisig address might not be present inscriptPubKey. In particular, ensure that the test’s conditional checks (currently looking for the key"address") accurately cover the intended scenarios under the updated RPC behavior.🧰 Tools
🪛 Ruff (0.8.2)
180-181: Use a single
ifstatement instead of nestedifstatements(SIM102)
src/wallet/rpcwallet.cpp (2)
1290-1297: LGTM! Documentation improvements for optional fieldsGood improvement to the API documentation by clearly marking optional fields in RPCResults. This helps API clients handle missing fields properly and improves the robustness of client implementations.
Also applies to: 1581-1582, 1591-1591, 1593-1596
1659-1659:❓ Verification inconclusive
Verify impact of removing addresses parameter
The removal of the addresses parameter from TxToUniv aligns with the deprecation of the addresses field. This is part of the broader change to remove the -deprecatedrpc=addresses configuration option.
Run this script to verify no remaining usages of the deprecated addresses parameter:
🏁 Script executed:
#!/bin/bash # Search for any remaining usages of the deprecated addresses parameter rg -A 3 "TxToUniv.*addresses"Length of output: 31
Approved removal of the addresses parameter – please verify manually
The update at
src/wallet/rpcwallet.cpp:1659correctly removes the deprecated addresses parameter from theTxToUnivcall as part of eliminating the-deprecatedrpc=addressesoption. A grep search for remaining usages yielded no output; however, given the low-quality inference from the script output, please perform further manual verification to ensure no residual references remain.src/qt/bitcoingui.cpp (5)
477-477: Use of custom quit signal for unified shutdown flow
Replacing direct calls toQApplication::quit()withquitRequestedensures a centralized and controlled shutdown sequence. This approach looks good.
1096-1097: Modal dialog creation with automatic cleanup
The use ofGUIUtil::ShowModalDialogAndDeleteOnClose(dlg)is a good practice to ensure the dialog is properly closed and deleted, preventing memory leaks.
1352-1359: Enhanced options dialog flow
Connecting thequitOnResetsignal toquitRequestedis convenient, but verify whether resetting options should always trigger a full application shutdown. Otherwise, this addition looks consistent and uses safe dialog management.
1703-1703: Graceful shutdown request in close event
EmittingquitRequested()within the close event ensures that the application follows the new unified shutdown procedure.
1997-1997: Shutdown signal on detectShutdown
This additionalquitRequested()helps maintain consistency in all shutdown paths. Just confirm it doesn’t trigger any double-shutdown scenarios if called repeatedly.src/qt/bitcoin.cpp (4)
60-60: New include for QWindow
Including<QWindow>seems valid given the need to manage or iterate top-level windows.
295-295: Updated connection to handle shutdownResult
RoutingshutdownResulttoQCoreApplication::quitproperly terminates the event loop once shutdown completes, ensuring a clean application exit.
326-329: Hide all top-level windows during shutdown
Hiding every top-level window before shutdown helps provide a smoother user experience. Verify there are no side effects, such as leaving a partially visible modal.
416-416: Refined shutdown approach instead of direct exit
Switching torequestShutdown()allows for a more orderly shutdown sequence and is consistent with the new flow.
| @@ -345,7 +345,7 @@ static RPCHelpMan sendtoaddress() | |||
| RPCResult::Type::OBJ, "", "", | |||
| { | |||
| {RPCResult::Type::STR_HEX, "txid", "The transaction id."}, | |||
| {RPCResult::Type::STR, "fee reason", "The transaction fee reason."} | |||
| {RPCResult::Type::STR, "fee_reason", "The transaction fee reason."} | |||
There was a problem hiding this comment.
Breaking change: RPC field renaming needs documentation
The field "fee reason" has been renamed to "fee_reason" in sendtoaddress and sendmany RPC responses for consistency. This is a breaking change that could affect clients parsing the RPC response.
Please update the release notes to document this breaking change and provide migration guidance for API clients.
Also applies to: 793-793
What was done?
Regular backports from Bitcoin Core v23
How Has This Been Tested?
Run unit & functional tests
Backport bitcoin-core/gui#342 made loading heavy wallet by RPC
loadwalleton my machine from 23 seconds to just 19 seconds (20% faster), due to no waiting 5 seconds oncs_walletsmutex.Breaking Changes
-deprecatedrpc=addressesconfiguration option has been removed. RPCsgettxout,getrawtransaction,decoderawtransaction,decodescript,gettransaction verbose=trueand REST endpoints/rest/tx,/rest/getutxos,/rest/blockno longer return theaddressesandreqSigsfields, whichwere previously deprecated in 22.0.
Checklist: