backport: merge bitcoin#24041, #24375, #24498, merge bitcoin-core/gui#408, #398, #434, #439, #319, #404, #569, #576, #618, #620, #631, #591, partial bitcoin#23757 (qt backports: part 3) #6541
Conversation
WalkthroughThis pull request introduces a comprehensive set of changes across multiple components of the Bitcoin Core application, primarily focusing on the Qt user interface, testing infrastructure, and utility functions. The modifications span various areas including wallet management, transaction overview, options handling, UI styling, and error handling. Key areas of enhancement include:
The changes reflect a systematic approach to refining the application's codebase, with a focus on improving user experience, test reliability, and code maintainability. The modifications touch on multiple aspects of the application, from low-level utility functions to high-level UI components, demonstrating a holistic approach to software development and quality assurance. Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/qt/walletmodel.cpp (1)
Restore Qt::QueuedConnection for thread safety
The removal of
Qt::QueuedConnectionbreaks the established thread-safety pattern. All other model updates in the codebase, including those in walletmodel.cpp, consistently useQt::QueuedConnectionfor safe cross-thread communication.
- In
src/qt/walletmodel.cpp: Add backQt::QueuedConnectiontoQMetaObject::invokeMethod(walletmodel, "updateAddressBook", ...)to maintain thread safety.🔗 Analysis chain
Line range hint
444-450: Verify thread safety of QMetaObject::invokeMethod changeThe removal of
Qt::QueuedConnectionmight affect thread safety. This needs careful review.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other similar patterns and thread safety concerns # Search for other QMetaObject::invokeMethod calls to verify consistency echo "Checking QMetaObject::invokeMethod usage patterns..." rg "QMetaObject::invokeMethod.*updateAddressBook" src/qt/ # Search for potential thread-safety related comments or patterns echo "Checking for thread-safety related patterns..." rg "Qt::(DirectConnection|QueuedConnection|BlockingQueuedConnection)" src/qt/Length of output: 3934
🧹 Nitpick comments (6)
src/qt/test/optiontests.h (1)
19-21: Add documentation for test methods.While the test methods are well-structured, adding brief documentation comments explaining the purpose of each test would improve maintainability:
optionTests()parametersInteraction()extractFilter()private Q_SLOTS: + /// Tests basic options functionality void optionTests(); + /// Tests interaction between different parameters void parametersInteraction(); + /// Tests filter string extraction functionality void extractFilter();src/qt/test/optiontests.cpp (1)
35-69: Consider adding more test cases for parameters interaction.While the current test covers the specific bug fix well, consider adding more test cases for different parameter combinations.
For example:
+ // Test interaction with other related parameters + settings.setValue("fListen", true); + settings.setValue("proxy", "127.0.0.1:9050"); + OptionsModel{}; + QVERIFY(gArgs.IsArgSet("-listen")); + QCOMPARE(gArgs.GetBoolArg("-listen", !expected), !expected); + + // cleanup + settings.remove("proxy");src/qt/initexecutor.cpp (1)
59-77: Enhance restart logic with additional safeguards.The restart logic includes a static flag to prevent multiple restarts, which is good. However, consider these improvements:
- Add a timeout mechanism for the restart sequence
- Consider using std::atomic for the executing_restart flag
- static bool executing_restart{false}; + static std::atomic<bool> executing_restart{false}; + static const int RESTART_TIMEOUT_MS = 30000; // 30 seconds if (!executing_restart) { executing_restart = true; + QTimer::singleShot(RESTART_TIMEOUT_MS, [](){ executing_restart = false; });src/test/getarg_tests.cpp (2)
46-54: Enhance test documentationWhile the test case has a good comment block explaining its purpose, consider adding inline comments for each test section to improve readability and maintainability.
Example structure:
// Test behavior of GetArg functions when string, integer, and boolean types // are specified in the settings.json file. GetArg functions are convenience // functions. The GetSetting method can always be used instead of GetArg // methods to retrieve original values, and there's not always an objective // answer to what GetArg behavior is best in every case. This test makes sure // there's test coverage for whatever the current behavior is, so it's not // broken or changed unintentionally. +// Test sections: +// 1. String values (lines 64-69) +// 2. Numeric values (lines 71-83) +// 3. Boolean values (lines 85-97) +// 4. Special cases (lines 99-153) BOOST_AUTO_TEST_CASE(setting_args)
255-257: Consider adding more boundary tests for numeric valuesWhile the test includes overflow checks, consider adding more boundary tests:
ResetArgs(local_args, "-foo=-9223372036854775809 -bar=9223372036854775808"); BOOST_CHECK_EQUAL(local_args.GetArg("-foo", 0), std::numeric_limits<int64_t>::min()); BOOST_CHECK_EQUAL(local_args.GetArg("-bar", 0), std::numeric_limits<int64_t>::max()); +// Test values near boundaries +ResetArgs(local_args, QString("-foo=%1 -bar=%2") + .arg(std::numeric_limits<int64_t>::min() + 1) + .arg(std::numeric_limits<int64_t>::max() - 1).toStdString()); +BOOST_CHECK_EQUAL(local_args.GetArg("-foo", 0), std::numeric_limits<int64_t>::min() + 1); +BOOST_CHECK_EQUAL(local_args.GetArg("-bar", 0), std::numeric_limits<int64_t>::max() - 1);src/test/util_tests.cpp (1)
1742-1745: Consider adding error handling to atoi64_legacy.The function uses
strtollwithout checking for conversion errors or out-of-range values, which could lead to undefined behavior. Consider adding error handling similar to other parsing functions in the codebase.int64_t atoi64_legacy(const std::string& str) { - return strtoll(str.c_str(), nullptr, 10); + char* endp = nullptr; + errno = 0; + long long int n = strtoll(str.c_str(), &endp, 10); + if (endp == nullptr || *endp != '\0' || errno != 0) { + return 0; + } + return n; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ec2b693 and bfb628882dbd8cef07f5a733ae48261cbf18b6cc.
📒 Files selected for processing (34)
src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(4 hunks)src/qt/bitcoin.cpp(1 hunks)src/qt/bitcoingui.cpp(6 hunks)src/qt/forms/openuridialog.ui(2 hunks)src/qt/guiutil.cpp(4 hunks)src/qt/guiutil.h(1 hunks)src/qt/initexecutor.cpp(3 hunks)src/qt/initexecutor.h(1 hunks)src/qt/openuridialog.cpp(2 hunks)src/qt/openuridialog.h(1 hunks)src/qt/optionsmodel.cpp(1 hunks)src/qt/qvalidatedlineedit.cpp(3 hunks)src/qt/qvalidatedlineedit.h(1 hunks)src/qt/res/css/general.css(1 hunks)src/qt/test/addressbooktests.cpp(3 hunks)src/qt/test/optiontests.cpp(1 hunks)src/qt/test/optiontests.h(1 hunks)src/qt/test/test_main.cpp(4 hunks)src/qt/transactionoverviewwidget.cpp(1 hunks)src/qt/transactionoverviewwidget.h(1 hunks)src/qt/utilitydialog.cpp(2 hunks)src/qt/walletframe.cpp(2 hunks)src/qt/walletframe.h(1 hunks)src/qt/walletmodel.cpp(2 hunks)src/qt/walletmodel.h(1 hunks)src/qt/walletview.cpp(5 hunks)src/qt/walletview.h(1 hunks)src/test/fuzz/string.cpp(1 hunks)src/test/getarg_tests.cpp(4 hunks)src/test/util_tests.cpp(3 hunks)src/util/strencodings.h(3 hunks)src/util/system.cpp(1 hunks)test/lint/lint-locale-dependence.py(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/qt/bitcoin.cpp
[error] 72-72: There is an unknown macro here somewhere. Configuration is required. If Q_IMPORT_PLUGIN is a macro then please configure it.
(unknownMacro)
src/qt/test/test_main.cpp
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If Q_IMPORT_PLUGIN is a macro then please configure it.
(unknownMacro)
src/test/getarg_tests.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (55)
src/qt/openuridialog.h (1)
19-19: Style and functionality improvements look good.The changes include:
- Consistent pointer notation style (
QWidget* parentinstead ofQWidget *parent)- Addition of
changeEventhandler for proper Qt event processing- Consistent pointer notation for the UI member variable
Also applies to: 26-26, 29-29
src/qt/openuridialog.cpp (4)
12-13: LGTM: Required headers added for new functionality.The addition of
QAbstractButtonandQLineEditheaders is necessary for the paste button implementation.
16-22: LGTM: Paste button setup looks good.The constructor properly:
- Sets up the paste button icon
- Connects the paste button's clicked signal to the URI edit's paste slot
41-47: LGTM: Improved code formatting.The conditional block is now properly formatted with braces, improving readability.
49-56: LGTM: Proper event handling implementation.The changeEvent handler:
- Calls the base class implementation
- Updates the paste button icon on style changes
src/qt/forms/openuridialog.ui (2)
33-48: LGTM: Well-configured paste button UI element.The paste button is properly configured with:
- Helpful tooltip for users
- Appropriate icon size (22x22)
- Correct layout integration
83-85: LGTM: Required resource file included.The dash.qrc resource file is properly included for icon access.
src/qt/res/css/general.css (1)
1262-1267: LGTM: Proper styling for the paste button.The CSS rules ensure the paste button:
- Has a transparent background
- Has consistent margins
- Integrates well with the dialog's design
src/qt/transactionoverviewwidget.h (1)
21-25: Well-structured header changes with good practices!Good use of the
explicitkeyword for the constructor and proper use ofoverridespecifiers for virtual methods. Moving implementations to the cpp file improves code organization and compilation times.src/qt/transactionoverviewwidget.cpp (2)
21-27: Consider preserving other size policies.The
showEventimplementation only modifies the horizontal policy while keeping other policies unchanged. This is good practice.
16-19: Add error handling for sizeHint calculation.The
sizeHintForColumncall might return invalid values if the model is not set or the column index is invalid. Consider adding validation:QSize TransactionOverviewWidget::sizeHint() const { - return {sizeHintForColumn(TransactionTableModel::ToAddress), QListView::sizeHint().height()}; + int width = model() ? sizeHintForColumn(TransactionTableModel::ToAddress) : 0; + width = width > 0 ? width : QListView::sizeHint().width(); + return {width, QListView::sizeHint().height()}; }src/qt/qvalidatedlineedit.h (1)
32-32: Good addition of setText slot.The addition of
setTextas a public slot enhances the widget's connectivity in Qt's signal-slot system while maintaining the expected behavior of QLineEdit.src/qt/initexecutor.h (1)
44-44: Verify thread safety with the new context object.The addition of
m_contextfor thread management should be carefully reviewed to ensure thread safety, particularly in the initialization and shutdown sequences.✅ Verification successful
Thread safety implementation verified and correct
The
m_contextusage is thread-safe through proper Qt mechanisms:
- Object is moved to dedicated thread
- All operations use
ObjectInvokewith Qt's event loop- Cross-thread safety ensured via queued connections
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for thread-related operations in InitExecutor implementation rg -A 5 "m_context" src/qt/initexecutor.cppLength of output: 916
Script:
#!/bin/bash # Check the implementation of ObjectInvoke rg -A 5 "ObjectInvoke" src/qt/guiutil.cppLength of output: 41
Script:
#!/bin/bash # Search for ObjectInvoke in all files rg -l "ObjectInvoke" # Also check header file specifically cat src/qt/guiutil.hLength of output: 23480
src/Makefile.qttest.include (2)
11-11: LGTM! New test file properly integrated.The addition of
moc_optiontests.cppis correctly placed in the MOC sources list.
61-61: Verify linking order changes.The reordering of linking dependencies (particularly LIBLEVELDB_SSE42) might affect build outcomes.
✅ Verification successful
The linking order of LIBLEVELDB followed by LIBLEVELDB_SSE42 is correct and intentional
This order ensures proper function resolution where SSE42-optimized implementations can override the base implementations when the CPU supports SSE4.2 instructions. The consistent ordering across all makefiles confirms this is an established pattern for handling CPU-specific optimizations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any existing build issues related to linking order rg -l "undefined reference|multiple definition" build*.log 2>/dev/null || echo "No build logs found"Length of output: 111
Script:
#!/bin/bash # Check the git history for changes in linking order git log -p src/Makefile.qttest.include | grep -A 1 -B 1 "LIBLEVELDB_SSE42" # Look for any LevelDB related linking documentation rg -i "leveldb.*link" -A 2 -B 2 # Check all makefiles for linking order documentation fd Makefile | xargs rg "link.*order" -A 2 -B 2Length of output: 82
Script:
#!/bin/bash # Check for LevelDB build configurations rg "LIBLEVELDB" -A 2 -B 2 # Look for SSE42 related configurations rg "SSE42" -A 2 -B 2 # Check configure scripts for library dependencies fd configure | xargs rg "leveldb|SSE42" -A 2 -B 2Length of output: 15064
src/qt/qvalidatedlineedit.cpp (3)
17-21: LGTM! Improved validation handling.The new
setTextimplementation properly ensures immediate validation by callingcheckValidity()after setting the text.
36-36: LGTM! Enhanced styling with themed approach.The use of
GUIUtil::getThemedStyleQStringfor invalid state styling improves consistency with the application's theme system.
114-114: LGTM! Added immediate validation check.Adding
checkValidity()after setting the validator ensures consistent validation state.src/qt/test/optiontests.cpp (2)
18-33: LGTM! Good regression test coverage.The test properly verifies the integer prune value regression fix with appropriate cleanup.
71-78: LGTM! Good filter extraction test cases.The test covers basic filter extraction scenarios effectively.
src/qt/initexecutor.cpp (3)
24-24: LGTM: Improved thread context management.The change to move only the context to the thread instead of the entire executor is a better approach for thread management.
82-91: LGTM: Clean shutdown implementation.The shutdown sequence is properly wrapped in GUIUtil::ObjectInvoke with appropriate error handling.
44-54: Verify thread safety of initialization sequence.The initialization sequence has been wrapped in GUIUtil::ObjectInvoke to ensure proper thread context execution. While this is generally good practice, we should verify that all initialization steps are thread-safe.
✅ Verification successful
Thread safety of initialization sequence is verified and confirmed safe
The initialization sequence is properly implemented with thread-safe mechanisms:
- GUIUtil::ObjectInvoke ensures correct thread context execution
- ThreadRename uses thread-local storage
- AppInitMain performs sequential initialization with proper synchronization
- Qt's signal/slot system safely handles cross-thread communication of initialization results
- Exception handling is properly implemented to prevent thread leaks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any thread-unsafe calls in the initialization path rg -n "appInitMain|ThreadRename" src/Length of output: 1532
Script:
#!/bin/bash # Check implementation of ThreadRename and appInitMain echo "=== ThreadRename Implementation ===" cat src/util/threadnames.cpp echo -e "\n=== appInitMain Implementation ===" cat src/node/interfaces.cppLength of output: 41274
Script:
#!/bin/bash # Check AppInitMain and BlockAndHeaderTipInfo implementations echo "=== AppInitMain Implementation ===" cat src/init.cpp | grep -A 50 "AppInitMain" echo -e "\n=== BlockAndHeaderTipInfo Usage ===" rg "BlockAndHeaderTipInfo" src/Length of output: 4151
src/qt/walletframe.h (1)
40-40: LGTM: Simplified wallet view management API.The change from
addWallet(WalletModel*, WalletView*)toaddView(WalletView*)is a good simplification since the wallet model is already accessible through the view. This reduces redundancy and potential inconsistencies.src/qt/test/test_main.cpp (2)
89-118: LGTM: Improved test failure tracking and reporting.The changes improve test result handling by:
- Using a dedicated counter for failures
- Adding clear success/failure messaging
- Providing a more accurate exit code
42-43: Verify Android platform integration.The addition of Android platform support needs verification to ensure proper integration.
✅ Verification successful
Android platform integration is properly implemented ✅
The Android platform support is consistently implemented across both the main application (
bitcoin.cpp) and test code (test_main.cpp), following the same pattern as other platform-specific integrations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other Android-specific configurations rg -l "ANDROID|QAndroid" src/Length of output: 413
Script:
#!/bin/bash # Check bitcoin.cpp content and Qt Android configurations echo "=== bitcoin.cpp Android integration ===" rg "QAndroid|Q_IMPORT_PLUGIN.*Android" src/qt/bitcoin.cpp -A 2 -B 2 echo -e "\n=== Qt Android-specific configurations ===" rg "QT_QPA_PLATFORM_ANDROID" src/ -A 2 -B 2Length of output: 1018
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If Q_IMPORT_PLUGIN is a macro then please configure it.
(unknownMacro)
src/qt/walletview.h (2)
40-40: LGTM: Stronger initialization guarantees.Requiring WalletModel in the constructor ensures that views are always properly initialized with a model, preventing potential null pointer issues.
55-60: LGTM: Improved model management and documentation.Good improvements:
- Making walletModel const prevents accidental model changes
- Documentation clearly explains the model's role
- noexcept specification on getter improves exception safety
src/qt/test/addressbooktests.cpp (3)
11-11: LGTM: Header inclusion is appropriate.The addition of
addressbookpage.hheader is necessary for the new test functionality.
119-122: LGTM: Good test setup and initial validation.The code properly initializes the AddressBookPage for testing and validates the initial state with a row count check.
131-131: LGTM: Comprehensive row count validation.The code effectively validates the address book's state after each operation by checking the table view's row count:
- After attempting to add an existing receiving address
- After attempting to add an existing sending address
- After successfully adding a new address
Also applies to: 140-140, 147-147
test/lint/lint-locale-dependence.py (1)
52-52: Verify the locale-dependent function usage in util_tests.cpp.The addition of util_tests.cpp to KNOWN_VIOLATIONS suggests it contains locale-dependent functions. Let's verify the specific usage:
src/qt/walletmodel.h (1)
73-73: LGTM: Enhanced encryption status handling for watch-only wallets.The addition of
NoKeysstatus appropriately handles wallets with disabled private keys (watch-only wallets).src/qt/utilitydialog.cpp (2)
26-27: LGTM: Proper header inclusions.The addition of required headers for QRegularExpression and QString is appropriate.
52-53: LGTM: Modernized regex handling.The code has been updated to use QRegularExpression with proper options:
- Replaced deprecated QRegExp with modern QRegularExpression
- Added InvertedGreedinessOption for non-greedy matching
src/qt/walletframe.cpp (1)
Line range hint
81-101: Improved architecture with better encapsulation.The refactoring from
addWallettoaddViewenhances the design by:
- Simplifying the interface
- Improving encapsulation by letting
WalletViewmanage its model- Maintaining the same validation checks through better abstraction
src/test/fuzz/string.cpp (2)
285-285: Enhanced integer overflow handling in tests.The assertion now correctly validates that out-of-range values are clamped to the type's limits, improving test coverage for edge cases.
291-291: Strengthened validation in string conversion tests.The test now enforces strict equality between
atoi64_resultandlocale_independent_atoi_result, removing the allowance for zero results on error, which improves test reliability.src/util/strencodings.h (1)
Line range hint
95-123: Improved documentation and error handling for LocaleIndependentAtoi.The changes enhance the function by:
- Clearly documenting the handling of out-of-range values
- Properly handling overflow/underflow by returning min/max values
- Removing undefined behavior while maintaining compatibility
src/qt/walletview.cpp (2)
38-44: Improved object initialization pattern.The changes strengthen the
WalletViewclass by:
- Enforcing proper initialization through constructor parameters
- Preventing null pointer issues with assertions
- Setting up all models during construction
- Improving object lifecycle management
Also applies to: 47-47, 53-54, 83-84, 86-87, 89-89, 92-93, 95-95, 106-106
140-154: Enhanced signal handling setup.The signal/slot connections are now properly established during construction, improving reliability by:
- Setting up connections early in the object lifecycle
- Eliminating the need for null checks
- Centralizing connection setup
src/Makefile.qt.include (1)
271-271: LGTM!The addition of
transactionoverviewwidget.cpptoBITCOIN_QT_WALLET_CPPis consistent with the file's purpose as a wallet component.src/test/getarg_tests.cpp (1)
20-20: LGTM: Function signature changes improve code clarityThe updated function signatures now explicitly take
ArgsManager&as a parameter, making the dependency clearer and improving maintainability.Also applies to: 39-39
src/qt/walletmodel.cpp (1)
359-363: Improve handling of watch-only wallet encryption statusThe change correctly prevents misrepresenting the encryption status of watch-only wallets. However, consider adding a test case to verify this behavior.
src/qt/guiutil.h (1)
194-200: LGTM: Well-documented utility functionThe new
ExtractFirstSuffixFromFilterfunction is clearly documented and serves a specific purpose in file dialog operations.✅ Verification successful
Function is well-implemented with proper test coverage
The function is appropriately used in file dialog operations and has dedicated test cases in
src/qt/test/optiontests.cpp.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage and test coverage # Check for function usage echo "Checking function usage..." rg "ExtractFirstSuffixFromFilter" src/ # Check for related test coverage echo "Checking test coverage..." rg -l "ExtractFirstSuffixFromFilter.*test" src/qt/test/Length of output: 762
src/qt/bitcoin.cpp (1)
71-72: LGTM!The Android platform integration plugin import is correctly added and follows the existing pattern of platform-specific plugin imports.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 72-72: There is an unknown macro here somewhere. Configuration is required. If Q_IMPORT_PLUGIN is a macro then please configure it.
(unknownMacro)
src/qt/optionsmodel.cpp (1)
265-285: LGTM! Improved readability and documentation.The changes enhance code readability by:
- Using a const variable for the boolean value
- Adding detailed comments explaining the parameter interaction logic between
-listenand-listenonionsrc/util/system.cpp (1)
630-630: LGTM! Improved numeric value handling.The change enhances the
GetArgmethod by properly handling numeric values usingisNum()andgetValStr()while maintaining compatibility with other value types.src/qt/guiutil.cpp (2)
516-525: LGTM! Well-implemented file suffix extraction.The new
ExtractFirstSuffixFromFilterfunction:
- Uses modern
QRegularExpressioninstead of deprecatedQRegExp- Properly handles file filter patterns like "Description (.foo)" or "Description (.foo *.bar ...)"
- Follows Qt best practices with proper pattern matching
544-544: LGTM! Consistent usage of the new function.The new
ExtractFirstSuffixFromFilterfunction is properly integrated into bothgetSaveFileNameandgetOpenFileNamemethods.Also applies to: 586-587
src/qt/bitcoingui.cpp (4)
178-180: LGTM! Icons are now properly hidden by default.Good practice to hide wallet status icons during initialization to prevent showing incorrect states.
415-415: Menu text changes align with PR objectives.The changes to menu text improve consistency and update keyboard shortcuts as intended:
- Added keyboard shortcut for loading PSBT from clipboard
- Updated wallet configuration file menu text and mnemonic
Also applies to: 428-428
920-921: No functional changes.Just formatting changes to the wallet view creation code.
1879-1884: Good security improvement for watch-only wallets.The new
NoKeyscase properly handles wallets without private keys by:
- Hiding the encryption icon
- Disabling encryption-related actions
- Setting correct UI state
This prevents confusion about encryption status for wallets that cannot be encrypted.
src/test/util_tests.cpp (1)
1774-1835: LGTM! Comprehensive test coverage for integer parsing edge cases.The test cases thoroughly cover:
- Overflow/underflow behavior for all integer types
- Legacy compatibility with previous atoi64 implementation
- Edge cases with maximum and minimum values
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
utACK bfb628882dbd8cef07f5a733ae48261cbf18b6cc
…Page` dialog
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/qt/guiutil.cpp (2)
516-525: Add documentation for the function.The function logic is correct and uses modern
QRegularExpression. Consider adding documentation to explain:
- Expected format of the filter string (e.g., "Description (.foo)" or "Description (.foo *.bar ...)")
- Return value behavior (empty string if no match)
+/** + * Extracts the first file suffix from a filter string. + * @param filter Filter string in format "Description (*.foo)" or "Description (*.foo *.bar ...)" + * @return First suffix without the "*." prefix, or empty string if no suffix found + */ QString ExtractFirstSuffixFromFilter(const QString& filter)
586-587: Remove unnecessary semicolon.There's an extra semicolon on line 587 that should be removed.
- ;src/test/getarg_tests.cpp (2)
255-257: Consider adding more edge cases for numeric overflow.While the test covers basic overflow cases, consider adding tests for:
- Values close to but not exceeding the limits
- String representations of numbers in different formats (hex, octal)
ResetArgs(local_args, "-foo=-9223372036854775809 -bar=9223372036854775808"); BOOST_CHECK_EQUAL(local_args.GetArg("-foo", 0), std::numeric_limits<int64_t>::min()); BOOST_CHECK_EQUAL(local_args.GetArg("-bar", 0), std::numeric_limits<int64_t>::max()); +// Test values near limits +ResetArgs(local_args, "-foo=-9223372036854775808 -bar=9223372036854775807"); +BOOST_CHECK_EQUAL(local_args.GetArg("-foo", 0), std::numeric_limits<int64_t>::min()); +BOOST_CHECK_EQUAL(local_args.GetArg("-bar", 0), std::numeric_limits<int64_t>::max()); +// Test hex and octal +ResetArgs(local_args, "-foo=0xF -bar=010"); +BOOST_CHECK_EQUAL(local_args.GetArg("-foo", 0), 15); +BOOST_CHECK_EQUAL(local_args.GetArg("-bar", 0), 8);
361-378: Consider adding test for future enhancement.The comment mentions bitcoin#16545 for distinguishing between
-dir=and-dircases. Consider adding a TODO comment to update these tests when that enhancement is implemented.// scripting so later command line arguments can override earlier command // line arguments or bitcoin.conf values. Currently the -dir= case cannot be // distinguished from -dir case with no assignment, but #16545 would add the // ability to distinguish these in the future (and treat the no-assign case // like an imperative command or an error). +// TODO: Update these tests when #16545 is implemented to verify the distinction +// between -dir= and -dir cases ResetArgs(local_args, "");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between bfb628882dbd8cef07f5a733ae48261cbf18b6cc and 0cb2724.
📒 Files selected for processing (34)
src/Makefile.qt.include(1 hunks)src/Makefile.qttest.include(4 hunks)src/qt/bitcoin.cpp(1 hunks)src/qt/bitcoingui.cpp(6 hunks)src/qt/forms/openuridialog.ui(2 hunks)src/qt/guiutil.cpp(4 hunks)src/qt/guiutil.h(1 hunks)src/qt/initexecutor.cpp(3 hunks)src/qt/initexecutor.h(1 hunks)src/qt/openuridialog.cpp(2 hunks)src/qt/openuridialog.h(1 hunks)src/qt/optionsmodel.cpp(1 hunks)src/qt/qvalidatedlineedit.cpp(3 hunks)src/qt/qvalidatedlineedit.h(1 hunks)src/qt/res/css/general.css(1 hunks)src/qt/test/addressbooktests.cpp(3 hunks)src/qt/test/optiontests.cpp(1 hunks)src/qt/test/optiontests.h(1 hunks)src/qt/test/test_main.cpp(4 hunks)src/qt/transactionoverviewwidget.cpp(1 hunks)src/qt/transactionoverviewwidget.h(1 hunks)src/qt/utilitydialog.cpp(2 hunks)src/qt/walletframe.cpp(2 hunks)src/qt/walletframe.h(1 hunks)src/qt/walletmodel.cpp(2 hunks)src/qt/walletmodel.h(1 hunks)src/qt/walletview.cpp(5 hunks)src/qt/walletview.h(1 hunks)src/test/fuzz/string.cpp(1 hunks)src/test/getarg_tests.cpp(4 hunks)src/test/util_tests.cpp(3 hunks)src/util/strencodings.h(3 hunks)src/util/system.cpp(1 hunks)test/lint/lint-locale-dependence.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (25)
- src/qt/initexecutor.h
- src/Makefile.qt.include
- src/qt/qvalidatedlineedit.h
- src/qt/forms/openuridialog.ui
- src/qt/walletframe.h
- src/qt/walletmodel.h
- test/lint/lint-locale-dependence.py
- src/qt/walletmodel.cpp
- src/test/fuzz/string.cpp
- src/qt/res/css/general.css
- src/qt/utilitydialog.cpp
- src/qt/guiutil.h
- src/qt/test/optiontests.h
- src/qt/openuridialog.cpp
- src/qt/transactionoverviewwidget.h
- src/qt/transactionoverviewwidget.cpp
- src/qt/openuridialog.h
- src/Makefile.qttest.include
- src/qt/test/addressbooktests.cpp
- src/qt/test/optiontests.cpp
- src/qt/walletview.h
- src/util/system.cpp
- src/qt/optionsmodel.cpp
- src/util/strencodings.h
- src/qt/bitcoingui.cpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/qt/test/test_main.cpp
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If Q_IMPORT_PLUGIN is a macro then please configure it.
(unknownMacro)
src/test/getarg_tests.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
src/qt/bitcoin.cpp
[error] 72-72: There is an unknown macro here somewhere. Configuration is required. If Q_IMPORT_PLUGIN is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (23)
src/qt/walletframe.cpp (1)
81-101: LGTM! The refactored method improves encapsulation.The renamed method
addViewwith simplified parameters better reflects its responsibility and improves encapsulation by relying onWalletViewto manage its own wallet model.src/qt/walletview.cpp (4)
38-44: LGTM! Constructor changes improve robustness.The updated constructor ensures that
WalletModelis always available through constructor injection, with an assertion to catch programming errors early.
47-47: LGTM! Model initialization is now centralized.All component model initialization is now performed in the constructor, which is cleaner and safer than scattered initialization.
Also applies to: 53-53, 83-83, 86-86, 89-89, 92-92, 95-95, 106-106
140-154: LGTM! Comprehensive signal-slot connections.All necessary wallet model connections are established during construction, ensuring proper event handling for:
- Messages
- Encryption status changes
- Transaction notifications
- Unlock requests
- Progress updates
187-190: LGTM! Simplified null check.The check for
walletModelis removed as it's now guaranteed to be non-null, while the client model check is retained as it's still optional.src/test/util_tests.cpp (5)
32-33: LGTM!The added includes are appropriate for the new test cases using numeric limits and map data structure.
1742-1745: LGTM!The
atoi64_legacyfunction provides a clean wrapper aroundstrtollfor testing backward compatibility of the newLocaleIndependentAtoiimplementation.
1774-1775: LGTM!Added test cases verify that out-of-range values for int32_t are correctly clamped to INT32_MIN/MAX instead of returning 0.
1777-1780: LGTM!Added test cases verify that out-of-range values for int64_t are correctly clamped to INT64_MIN/MAX instead of returning 0.
1782-1800: LGTM!Added test map provides comprehensive coverage of edge cases and verifies that the new
LocaleIndependentAtoiimplementation maintains backward compatibility with legacyatoi64behavior.src/qt/initexecutor.cpp (2)
24-25: Improved thread safety with context object.The change from moving
thisto using a dedicated context object (m_context) is a safer approach for thread management, as it avoids potential issues with QObject ownership and lifecycle management.
44-54: Enhanced thread-safety with ObjectInvoke pattern.The consistent use of
GUIUtil::ObjectInvokefor all operations ensures proper thread context and error handling. This pattern:
- Guarantees execution in the correct thread context
- Maintains consistent error handling through try-catch blocks
- Improves code maintainability with a uniform approach
Also applies to: 59-77, 82-91
src/qt/qvalidatedlineedit.cpp (3)
17-21: Improved input validation with immediate checks.The new
setTextimplementation ensures immediate validation after setting text, providing better user feedback.
36-36: Enhanced visual feedback with themed styling.Using
GUIUtil::getThemedStyleQStringfor invalid state styling improves consistency with the application's theme system.
114-114: Added immediate validation after setting validator.Calling
checkValidityafter setting the validator ensures the input state is immediately validated.src/qt/test/test_main.cpp (2)
89-117: Improved test failure tracking and reporting.The change from a boolean flag to an accumulator provides better visibility into test failures by:
- Tracking the exact number of failed tests
- Providing more detailed failure reporting
42-43: Added Android platform support.The addition of the Android platform integration plugin enables the application to run on Android devices.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If Q_IMPORT_PLUGIN is a macro then please configure it.
(unknownMacro)
src/qt/bitcoin.cpp (1)
71-72: Added Android platform support.The addition of the Android platform integration plugin enables the application to run on Android devices. This change is consistent with the similar addition in test_main.cpp.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 72-72: There is an unknown macro here somewhere. Configuration is required. If Q_IMPORT_PLUGIN is a macro then please configure it.
(unknownMacro)
src/qt/guiutil.cpp (2)
64-64: LGTM!The
QRegularExpressioninclude is correctly placed in the Qt includes section.
544-544: LGTM!Good refactoring to use the new
ExtractFirstSuffixFromFilterfunction, improving code reuse and maintainability.src/test/getarg_tests.cpp (3)
6-7: LGTM: New imports are appropriate.The addition of
univalue.handsettings.hheaders is necessary to support the new test case functionality.
20-37: LGTM: Helper function refactoring improves modularity.The
ResetArgsfunction has been updated to accept anArgsManagerinstance parameter, which is a good practice as it:
- Makes the function more modular and reusable
- Removes dependency on class member variables
- Makes the function's dependencies explicit
46-154: Comprehensive test coverage for GetArg functions.The new test case thoroughly covers various scenarios for argument handling:
- String values
- Integer values
- Boolean values
- Special cases (empty strings, null values)
- Edge cases (objects, arrays)
- Type conversion behavior
This is a valuable addition that helps ensure robust argument handling.
…el update 716cb3e fix(qt): update CoinJoin frame and tx list on client model update (UdjinM6) Pull request description: ## Issue being fixed or feature implemented 3f9dca5 (#6541) broke Overview Page. Wallet model is set earlier now (via WalletView ctor and not via `addWallet` 3f9dca5#diff-c0330db51c25c4bb621fca0b908dc6fc597db2077f74855eed1b16e0739748a6L88) but we need Client model too to call `updateAdvancedCJUI()` correctly and it's not yet available. ## What was done? Move `updateAdvancedCJUI()` call to `setClientModel()`. ## How Has This Been Tested? ## Breaking Changes ## 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: PastaPastaPasta: utACK 716cb3e kwvg: ACK 716cb3e Tree-SHA512: 05b4cfaa17c531762122ef8e19470dcc299a57827274687ce5c765f1049b93672029fa170cd8f131c69c9b1f1f3fd542baab4a7ddaaa1405c109cd9b9eeaab6e
3 participants
Additional Information
develop(0972dfe)Breaking Changes
Checklist