backport: merge bitcoin-core/gui#600, #601, #602, #701, #603 (migrate some QSettings to settings.json) #6833
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughOptionsModel was refactored to be node-aware and deferred-initialized: its constructor now takes interfaces::Node& and Init(bilingual_str& error) returns bool. BitcoinApplication::createOptionsModel(bool) now returns bool and startup creates the splash and node earlier, aborting startup on options init failure. Settings persistence was migrated from QSettings to node-backed settings.json (including prune, proxy, font, mask values), many legacy QSettings keys are migrated/removed, and some QSettings-based behavior (e.g., UPnP persistence on accept) was removed. Tests and UI components were updated to construct OptionsModel with a node and call Init(error). Sequence Diagram(s)sequenceDiagram
actor User
participant App as BitcoinApplication
participant Splash as SplashScreen
participant OM as OptionsModel
participant Node as interfaces::Node
participant FS as settings.json
rect rgb(235,245,255)
Note over App,Splash: Early startup: splash & node creation
User->>App: Launch
App->>Splash: create & show
App->>Node: create node instance
end
rect rgb(245,255,235)
Note over App,OM: Node-backed OptionsModel init (deferred constructor)
App->>OM: new OptionsModel(node, parent)
OM-->>App: constructed (Init deferred)
App->>OM: Init(error)
OM->>Node: read/migrate settings
Node->>FS: read/write settings.json
FS-->>Node: return settings
Node-->>OM: settings values / ack writes
alt Init success
OM-->>App: true
App->>App: continue startup -> show main window
else Init failure
OM-->>App: false + error
App->>User: show critical error (settings path)
App->>App: exit(EXIT_FAILURE)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)src/**/*.{cpp,h,hpp,cc}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/qt/**/*.{cpp,h}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (14)📓 Common learnings📚 Learning: 2025-12-17T13:58:19.813ZApplied to files:
📚 Learning: 2025-07-29T14:32:48.369ZApplied to files:
📚 Learning: 2025-02-06T14:34:30.466ZApplied to files:
📚 Learning: 2025-06-09T16:43:20.996ZApplied to files:
📚 Learning: 2025-10-25T07:08:51.918ZApplied to files:
📚 Learning: 2025-10-05T20:38:28.457ZApplied to files:
📚 Learning: 2025-08-11T17:16:36.654ZApplied to files:
📚 Learning: 2025-07-29T14:33:01.040ZApplied to files:
📚 Learning: 2025-10-13T12:37:12.357ZApplied to files:
📚 Learning: 2025-11-04T18:24:27.241ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-11-24T16:41:22.457ZApplied to files:
📚 Learning: 2025-12-22T15:42:51.679ZApplied to files:
🧬 Code graph analysis (1)src/qt/optionsmodel.cpp (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/masternodelist.cpp (1)
5-13: Fix CI: clang-format include-order violation.The pipeline failed due to include order changes. Please run the repo’s clang-format step (e.g., contrib/devtools/clang-format-diff.py -p1) or reorder the includes per project style to satisfy CI.
🧹 Nitpick comments (28)
src/qt/test/test_main.cpp (1)
63-66: Prefer explicit scope over IILE for BasicTestingSetup; also keep creation within the same lifetime as directory creationThis avoids subtle lifetime semantics and reads clearer. Functionally equivalent.
- fs::create_directories([] { - BasicTestingSetup dummy{CBaseChainParams::REGTEST}; - return gArgs.GetDataDirNet() / "blocks"; - }()); + { + BasicTestingSetup dummy{CBaseChainParams::REGTEST}; + fs::create_directories(gArgs.GetDataDirNet() / "blocks"); + }Additionally, consider checking the return value or logging failures from create_directories.
src/qt/forms/optionsdialog.ui (1)
1058-1169: Monospaced font selector UI: looks good; add basic UX affordances
- Add short tooltips to both radio buttons to clarify “embedded” vs “system” choice.
- Optionally group them in a QButtonGroup for explicit exclusivity (not required, but future-proof if the layout changes).
- <widget class="QRadioButton" name="embeddedFont_radioButton"> + <widget class="QRadioButton" name="embeddedFont_radioButton"> <property name="text"> <string>embedded "%1"</string> </property> + <property name="toolTip"> + <string>Use the bundled monospaced font for amounts.</string> + </property> </widget> ... - <widget class="QRadioButton" name="systemFont_radioButton"> + <widget class="QRadioButton" name="systemFont_radioButton"> <property name="text"> <string>closest matching "%1"</string> </property> + <property name="toolTip"> + <string>Use the closest system-installed monospaced font for amounts.</string> + </property> </widget>src/qt/masternodelist.cpp (3)
233-240: Localize “UNKNOWN” next-payment text.Untranslated literal leaks into UI; use tr() for i18n.
- QString strNextPayment = "UNKNOWN"; + QString strNextPayment = tr("UNKNOWN");
149-151: Typo: nMnListUpdateSecods → nMnListUpdateSeconds.Minor readability fix; avoids propagating the misspelling.
- int64_t nMnListUpdateSecods = clientModel->masternodeSync().isBlockchainSynced() ? MASTERNODELIST_UPDATE_SECONDS : MASTERNODELIST_UPDATE_SECONDS * 10; - int64_t nSecondsToWait = nTimeUpdatedDIP3 - GetTime() + nMnListUpdateSecods; + int64_t nMnListUpdateSeconds = clientModel->masternodeSync().isBlockchainSynced() ? MASTERNODELIST_UPDATE_SECONDS : MASTERNODELIST_UPDATE_SECONDS * 10; + int64_t nSecondsToWait = nTimeUpdatedDIP3 - GetTime() + nMnListUpdateSeconds;
141-147: Avoid briefly showing negative countdown.Clamp the displayed value to 0 to prevent negative numbers flashing in the label.
- ui->countLabelDIP3->setText(tr("Please wait…") + " " + QString::number(nSecondsToWait)); + const auto displayWait = nSecondsToWait > 0 ? nSecondsToWait : 0; + ui->countLabelDIP3->setText(tr("Please wait…") + " " + QString::number(displayWait));src/qt/coincontroldialog.cpp (1)
16-16: Good: proper dependency for GUIUtil::FontWeight.One question: this file still persists UI prefs via QSettings (Lines 135–139, 153–157). If the PR goal is migrating settings to settings.json where feasible, consider whether these should be moved too, or explicitly documented as remaining UI-local prefs.
src/qt/appearancewidget.h (1)
12-12: Appropriate to include in a header due to member types/default initializersprevFontFamily and weight members (with default initializers calling GUIUtil getters) require guiutil_font.h in this header. This is the right place to include it.
If compile time becomes a concern later, consider moving function-based default initializers (e.g., GUIUtil::getFontFamily()) into the .cpp constructor to reduce header coupling. Not required for this backport.
src/qt/rpcconsole.cpp (1)
68-71: Consider migrating console font size setting away from QSettingsGiven the PR’s “migrate some QSettings to settings.json,” consider moving consoleFontSize to the new persistent settings path (via Node/ArgsManager helpers) for consistency and headless configurability. Low priority; can be follow-up.
src/qt/appearancewidget.cpp (1)
31-33: Populate font families from GUIUtil::AVAILABLE_FONTS — good step; add fallback and verify localization
- Good: dynamic population with “selectable” filter.
- Risks:
- If the stored OptionsModel FontFamily is not selectable on the current platform, currentIndex may be -1 after mapper->toFirst(). Add a graceful fallback to the first available entry and apply it.
- Ensure family_str strings are already localized in GUIUtil; otherwise, user-facing labels like “System default” should be translated.
Suggested fallback (in setModel(), after mapper->toFirst()):
@@ void AppearanceWidget::setModel(OptionsModel* _model) - mapper->toFirst(); + mapper->toFirst(); + // Fallback if stored value is not present (e.g., font now non-selectable) + if (ui->fontFamily->currentIndex() < 0 && ui->fontFamily->count() > 0) { + ui->fontFamily->setCurrentIndex(0); + updateFontFamily(0); + }Also verify that guiutil_font.h is included indirectly via appearancewidget.h. If not, include it here:
+#include <qt/guiutil_font.h>src/qt/bitcoin.h (1)
49-49: Call sites already handle [[nodiscard]] return; add a header comment for failure reasons and optionally inline‐initialize optionsModel.--- a/src/qt/bitcoin.h +++ b/src/qt/bitcoin.h @@ - /// Create options model - [[nodiscard]] bool createOptionsModel(bool resetSettings); + /// Create options model; returns false on Init() failure (e.g. settings load error) + [[nodiscard]] bool createOptionsModel(bool resetSettings); @@ - OptionsModel *optionsModel; + OptionsModel *optionsModel{nullptr};src/qt/test/addressbooktests.cpp (1)
122-125: Prefer QVERIFY2 to surface Init() failure reason.Improves debuggability without changing behavior.
- bilingual_str error; - QVERIFY(optionsModel.Init(error)); + bilingual_str error; + QVERIFY2(optionsModel.Init(error), error.translated.c_str());src/qt/test/wallettests.cpp (1)
155-158: Use QVERIFY2 with message for OptionsModel::Init.Shows the translated error on failure.
- bilingual_str error; - QVERIFY(optionsModel.Init(error)); + bilingual_str error; + QVERIFY2(optionsModel.Init(error), error.translated.c_str());src/util/settings.cpp (1)
180-182: Nonpersistent filter logic looks correct; consider parity helpers and testsSkipping COMMAND_LINE and FORCED when
ignore_nonpersistentis true aligns with “persistent-only” reads. Two follow-ups:
- Consider a parallel API for lists (e.g.,
GetPersistentSettingsList) if the GUI needs array semantics without CLI/forced influence.- Add tests covering forced and negated values when
ignore_nonpersistent=trueto prevent regressions.I can draft tests mirroring existing settings_tests with forced/CLI sources. Want a patch?
src/node/interfaces.cpp (1)
479-486: Consider error handling during resetSettings
resetSettings()ignores the return ofWriteSettingsFile(...). A failed backup or rewrite would be silent. Capture the boolean and log or surface an error to the caller/UI.Apply:
void resetSettings() override { - gArgs.WriteSettingsFile(/*errors=*/nullptr, /*backup=*/true); + if (!gArgs.WriteSettingsFile(/*errors=*/nullptr, /*backup=*/true)) { + LogPrintf("resetSettings: failed to write backup settings file\n"); + } gArgs.LockSettings([&](util::Settings& settings) { settings.rw_settings.clear(); }); - gArgs.WriteSettingsFile(); + if (!gArgs.WriteSettingsFile()) { + LogPrintf("resetSettings: failed to write cleared settings file\n"); + } }src/qt/optionsdialog.cpp (1)
376-376: Map the radio button’s checked property explicitlyRelying on the widget’s user property works, but making the mapping explicit avoids surprises if UI changes later.
Apply:
- mapper->addMapping(ui->embeddedFont_radioButton, OptionsModel::UseEmbeddedMonospacedFont); + mapper->addMapping(ui->embeddedFont_radioButton, OptionsModel::UseEmbeddedMonospacedFont, "checked");src/interfaces/node.h (1)
197-214: Clarify override semantics (CLI vs forced) and return contractsDocs say “overridden by the command line,” but implementation likely should also consider
forced_settings. Either:
- Update
isSettingIgnoreddocs to include “or forced settings,” or- Intentionally exclude forced and state that explicitly.
Also,
updateRwSettinghere isvoidwhile an existing overload returnsbool. Consider harmonizing the contract in a follow-up (even if just by documenting error handling expectations).src/test/settings_tests.cpp (1)
145-148: NullOverride assertions remain valid with new flag.Using ignore_nonpersistent=false ensures forced null overrides are tested. Consider adding a complementary test with ignore_nonpersistent=true to lock in persistent-only semantics.
I can add a small test exercising GetPersistentSetting/ignore_nonpersistent=true if helpful.
src/util/system.h (3)
179-182: Avoid leaking helpers into global namespace.Consider placing SettingToString/SettingToInt/SettingToBool into a namespace (e.g., util) to minimize global symbol surface.
-std::string SettingToString(const util::SettingsValue&, const std::string&); -int64_t SettingToInt(const util::SettingsValue&, int64_t); -bool SettingToBool(const util::SettingsValue&, bool); +namespace util { +std::string SettingToString(const util::SettingsValue&, const std::string&); +int64_t SettingToInt(const util::SettingsValue&, int64_t); +bool SettingToBool(const util::SettingsValue&, bool); +} // namespace utilNote: adjust call sites accordingly.
477-477: Mark GetSettingsPath as nodiscard.This is typically used for control flow; marking nodiscard avoids accidental ignoring of failure.
- bool GetSettingsPath(fs::path* filepath = nullptr, bool temp = false, bool backup = false) const; + [[nodiscard]] bool GetSettingsPath(fs::path* filepath = nullptr, bool temp = false, bool backup = false) const;
490-495: New persistent accessor: verify thread-safety note.The method is const and acquires cs_args in the impl—good. Consider noting in the comment that it acquires the internal lock.
src/qt/test/optiontests.cpp (1)
31-62: Clean up artifacts after migration test.To avoid influencing subsequent tests, consider removing settings.json (and clearing QSettings) at the end of migrateSettings().
src/util/system.cpp (2)
563-577: Backup path suffix ordering: confirm intent.GetSettingsPath builds "settings.json.bak.tmp" for temp backups. If tooling expects ".tmp" to be the final suffix, consider swapping the order:
- if (backup) { - settings += ".bak"; - } - if (filepath) { - *filepath = fsbridge::AbsPathJoin(GetDataDirNet(), temp ? settings + ".tmp" : settings); - } + if (filepath) { + fs::path base = settings; + if (temp) base += ".tmp"; + if (backup) base += ".bak"; + *filepath = fsbridge::AbsPathJoin(GetDataDirNet(), base); + }Please verify no consumers depend on the current ordering.
632-638: Persistent-only accessor correctly ignores nonpersistent layers.Lock + util::GetSetting(..., /ignore_nonpersistent=/true) is correct. Consider a unit test exercising this path.
Happy to add a small test demonstrating forced/CLI values being ignored.
src/qt/bitcoin.cpp (1)
509-512: Avoid duplicating the list of valid font families.Consider deriving the “Possible values” string from the enum/registry used by GUIUtil::fontFamilyFromString to keep help text in sync.
src/qt/guiutil_font.h (2)
43-48: Unify macOS guard with .cpp for consistency.
.cpp uses Q_OS_MAC; header uses Q_OS_MACOS. Align to one macro to avoid confusion and divergent defaults.-#ifdef Q_OS_MACOS +#ifdef Q_OS_MAC QFont::ExtraLight #else QFont::Light -#endif // Q_OS_MAC +#endif // Q_OS_MAC
85-89: Typo in comment.
“appliciation” → “application”.-/** Load dash specific appliciation fonts */ +/** Load Dash-specific application fonts */src/qt/optionsmodel.cpp (2)
334-339: Store boolean default, not string, for UseEmbeddedMonospacedFont.
Avoid type drift in QSettings.- if (!settings.contains("UseEmbeddedMonospacedFont")) { - settings.setValue("UseEmbeddedMonospacedFont", "true"); - } + if (!settings.contains("UseEmbeddedMonospacedFont")) { + settings.setValue("UseEmbeddedMonospacedFont", true); + }
755-760: Signal name may be misleading.
Changing sessions emits coinJoinRoundsChanged(). Consider a dedicated sessions-changed signal for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 873dee0 and 36795534ab4e314697b33cd6729585e82d856c17.
📒 Files selected for processing (52)
src/Makefile.qt.include(2 hunks)src/init.cpp(0 hunks)src/interfaces/node.h(2 hunks)src/node/interfaces.cpp(1 hunks)src/qt/addressbookpage.cpp(1 hunks)src/qt/addresstablemodel.cpp(1 hunks)src/qt/appearancewidget.cpp(1 hunks)src/qt/appearancewidget.h(1 hunks)src/qt/askpassphrasedialog.cpp(1 hunks)src/qt/bitcoin.cpp(5 hunks)src/qt/bitcoin.h(1 hunks)src/qt/bitcoingui.cpp(2 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/forms/optionsdialog.ui(2 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/guiutil.cpp(0 hunks)src/qt/guiutil.h(0 hunks)src/qt/guiutil_font.cpp(1 hunks)src/qt/guiutil_font.h(1 hunks)src/qt/masternodelist.cpp(1 hunks)src/qt/modaloverlay.cpp(1 hunks)src/qt/openuridialog.cpp(1 hunks)src/qt/optionsdialog.cpp(3 hunks)src/qt/optionsmodel.cpp(9 hunks)src/qt/optionsmodel.h(8 hunks)src/qt/overviewpage.cpp(4 hunks)src/qt/overviewpage.h(1 hunks)src/qt/proposalwizard.cpp(1 hunks)src/qt/qrdialog.cpp(1 hunks)src/qt/qrimagewidget.cpp(1 hunks)src/qt/receivecoinsdialog.cpp(1 hunks)src/qt/receiverequestdialog.cpp(1 hunks)src/qt/rpcconsole.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(1 hunks)src/qt/sendcoinsentry.cpp(1 hunks)src/qt/signverifymessagedialog.cpp(1 hunks)src/qt/splashscreen.cpp(1 hunks)src/qt/test/addressbooktests.cpp(1 hunks)src/qt/test/apptests.cpp(2 hunks)src/qt/test/optiontests.cpp(3 hunks)src/qt/test/optiontests.h(1 hunks)src/qt/test/test_main.cpp(1 hunks)src/qt/test/wallettests.cpp(1 hunks)src/qt/trafficgraphwidget.cpp(1 hunks)src/qt/transactiondescdialog.cpp(1 hunks)src/qt/utilitydialog.cpp(1 hunks)src/qt/walletview.cpp(1 hunks)src/test/settings_tests.cpp(3 hunks)src/util/settings.cpp(2 hunks)src/util/settings.h(1 hunks)src/util/system.cpp(6 hunks)src/util/system.h(2 hunks)
💤 Files with no reviewable changes (3)
- src/init.cpp
- src/qt/guiutil.h
- src/qt/guiutil.cpp
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/qt/test/wallettests.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
src/qt/test/apptests.cppsrc/qt/test/optiontests.h
🪛 GitHub Actions: Clang Diff Format Check
src/qt/masternodelist.cpp
[error] 5-13: Clang format differences found in src/qt/masternodelist.cpp. The diff shows an include order change (added '#include <coins.h>' before 'evo/deterministicmns.h' and removed the previous '#include <coins.h>'). The check failed with exit code 1. Run 'clang-format-diff.py -p1' (or 'clang-format') to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (55)
src/qt/forms/optionsdialog.ui (1)
1215-1215: String change: confirm translations are updatedThe bottom info label text changed; please run lupdate / refresh .ts files so translators see the new string.
src/util/settings.h (1)
58-71: All GetSetting call sites and tests updated to 6-arg signature Verified no lingering 5-arg calls remain.src/qt/utilitydialog.cpp (1)
15-15: Include aligns with font subsystem migrationHeader inclusion matches the use of GUIUtil::updateFonts and the new split of font utilities. LGTM.
src/qt/transactiondescdialog.cpp (1)
9-9: Include aligns with font subsystem migrationConsistent with calling GUIUtil::updateFonts(). LGTM.
src/qt/masternodelist.cpp (1)
12-12: Header include looks correct.Including qt/guiutil_font.h is required for GUIUtil::setFont/updateFonts. No issues.
src/qt/sendcoinsdialog.cpp (1)
18-18: Header include is appropriate.qt/guiutil_font.h is needed for font helpers used in this file. Looks good.
src/qt/openuridialog.cpp (1)
10-10: Include addition is correct.Required for GUIUtil::updateFonts usage. No further changes needed.
src/qt/addresstablemodel.cpp (1)
9-9: Include addition aligns with FontRole usage.Bringing in qt/guiutil_font.h matches GUIUtil::getFontNormal() usage. All good.
src/qt/splashscreen.cpp (1)
19-19: Include addition is correct.Needed for GUIUtil::getFontNormal/getFontBold; no issues.
src/qt/addressbookpage.cpp (1)
17-17: Include for font utilities is correct.Matches the migration of font APIs out of guiutil.h; aligns with GUIUtil::updateFonts() usage later. No issues.
src/qt/qrimagewidget.cpp (1)
8-8: Header addition aligns with font API split.Required for getFontNormal/calculateIdealFontSize. Looks good.
src/qt/sendcoinsentry.cpp (1)
16-16: Review OK; include needed for setFont(FontWeight)
<qt/guiutil_font.h>is required forGUIUtil::setFont(FontWeight)— LGTM.labellLabelis defined insrc/qt/forms/sendcoinsentry.ui(line 118); name matches the code, no typo.src/qt/modaloverlay.cpp (1)
10-10: Correct header include for font utilities.Consistent with GUIUtil::setFont usage below. No further action.
src/qt/signverifymessagedialog.cpp (1)
11-11: Approve: guiutil_font header integration verified
Makefile.qt.include lists guiutil_font.h/.cpp and all translation units using GUIUtil::FontWeight include<qt/guiutil_font.h>.src/qt/receivecoinsdialog.cpp (1)
10-10: Header addition aligns with centralized font utilitiesIncluding qt/guiutil_font.h here is consistent with using GUIUtil::setFont/updateFonts. No issues.
src/qt/qrdialog.cpp (1)
11-11: Good: pulls in FontWeight after font helpers moved out of guiutil.hKeeps this TU compiling with the new font subsystem. Looks good.
src/qt/governancelist.cpp (1)
20-20: Correct dependency on guiutil_font.hNeeded for the GUIUtil::setFont calls used in this widget. Change is safe and scoped.
src/qt/askpassphrasedialog.cpp (1)
15-15: Include for font utilities looks correctUnblocks usage of GUIUtil::FontWeight in this TU. No further action.
src/qt/rpcconsole.cpp (1)
21-21: Add guiutil_font include — OKMatches usage of GUIUtil::setFont/updateFonts and font weights below.
src/qt/trafficgraphwidget.cpp (1)
9-9: Include addition is fineAligns this TU with GUI font helpers; no behavior change.
src/qt/walletview.cpp (1)
15-15: Include addition is fineEnables explicit font weight usage in this TU. No issues.
src/qt/appearancewidget.cpp (1)
31-33: AVAILABLE_FONTS header inclusion verified
Declaration exists in src/qt/guiutil_font.h (line 26) and appearancewidget.h includes it (line 11) — no further changes needed.src/qt/receiverequestdialog.cpp (1)
10-10: Include for moved font utilities looks correct.This aligns with updateFonts() migration to guiutil_font.*. No further action.
src/qt/proposalwizard.cpp (1)
16-16: Correct header added for GUIUtil::setFont and FontWeight.Header placement is fine and resolves the new font API usage.
src/qt/overviewpage.h (1)
74-74: No issues—slot implemented and signal connected.OverviewPage::setMonospacedFont(bool) is defined in overviewpage.cpp (line 379) and wired to OptionsModel::useEmbeddedMonospacedFontChanged in overviewpage.cpp (line 305); no further action required.
src/Makefile.qt.include (1)
140-140: Confirmed no Q_OBJECT in guiutil_font.h; no moc entry needed.src/qt/test/apptests.cpp (1)
11-11: LGTM on include and return-value assert.The new guiutil_font include and the updated QVERIFY for createOptionsModel(bool) look correct and align with the API change.
Also applies to: 79-79
src/qt/bitcoingui.cpp (1)
13-13: LGTM: guiutil_font include.Consistent with the new font utilities usage elsewhere.
src/qt/test/optiontests.h (3)
9-11: LGTM: adds required headers for settings/JSON use.
18-18: Constructor change acknowledged.Node-injecting ctor is appropriate for node-backed settings tests.
21-26: Ensure settings backup/restore around tests.Please verify that m_previous_settings is populated before mutation (e.g., in init()) and restored afterward (e.g., dedicated cleanup slot or destructor) to avoid cross-test contamination.
Also applies to: 29-29
src/util/settings.cpp (1)
144-146: All util::GetSetting call sites updated and docs accurate
Verified the three invocations in system.cpp (lines 635–637, 1113–1115, 1183–1185) now pass bothignore_nonpersistentandget_chain_namein the correct order, and the header comment already documents thatignore_nonpersistentfilters out forced and command-line values.src/node/interfaces.cpp (1)
1127-1154: All boolean-returning calls to updateRwSetting use the Chain::updateRwSetting 3-arg overload (explicit or via default write flag); the 2-arg Node::updateRwSetting is only used by Qt code where its void return is correctly ignored.src/qt/overviewpage.cpp (2)
379-395: LGTM: focused, minimal font applicationUsing
GUIUtil::setFont(..., RobotoMono|DefaultMonospace, Bold)andGUIUtil::updateFonts()is consistent with the new font subsystem and limited in scope to balance labels.
305-307: [awaiting_run_scripts]src/qt/optionsdialog.cpp (1)
205-218: LGTM: clear font previews and sane defaultPreview labels use the actual families via
QFontInfo, and defaulting to system-monospaced is reasonable until the model mapping loads. No functional issues spotted.src/interfaces/node.h (1)
8-16: Header include adjustments look fineAdding
util/settings.hforutil::SettingsValueis correct, and other includes are appropriately ordered.src/test/settings_tests.cpp (2)
109-116: API update applied correctly in helper.Passing ignore_nonpersistent=false preserves previous behavior. LGTM.
228-229: Merge test call site aligns with new GetSetting signature.Explicit comments for the two new booleans are helpful. LGTM.
src/util/system.h (1)
485-489: Docs sync with backup behavior.Comment clarifies backup path behavior. LGTM.
src/qt/test/optiontests.cpp (3)
24-29: Fixture reset is sound.Restoring m_previous_settings and clearing the path cache prevents cross-test contamination. LGTM.
74-86: Regression guard looks good.Constructing OptionsModel with Node and calling Init(error) directly covers the reported regression. LGTM.
108-110: parametersInteraction now validates via OptionsModel::Init.Good shift to Node-aware initialization. LGTM.
src/util/system.cpp (4)
612-631: WriteSettingsFile backup plumbing is correct.Using the same backup flag for both tmp and final paths is consistent. LGTM.
644-675: Centralized conversions reduce duplication.Refactoring GetArg/GetIntArg/GetBoolArg to use SettingTo* helpers improves consistency. LGTM.
1109-1118: Chain selection call updated with new GetSetting flags.Explicit get_chain_name=true and ignore_nonpersistent=false maintain existing semantics. LGTM.
1184-1186: ArgsManager::GetSetting updated with new flag.Explicit ignore_nonpersistent=false preserves prior behavior. LGTM.
src/qt/bitcoin.cpp (3)
350-353: Prune API change accounted for.Dropping the extra boolean matches the new signature. LGTM.
691-700: Startup flow improvement: create splash and node earlier; gate on options init.Great for UX and error handling. LGTM.
704-705: Default font family fallback is consistent.Using GUIUtil::g_font_defaults.family for default arg is correct. LGTM.
src/qt/optionsmodel.h (3)
103-105: Public getOption/setOption and embedded-mono getter are a welcome simplification.
API surface reads clean and matches the data()/setData() rework.Also applies to: 114-115
45-47: All OptionsModel instantiations updated and no setNode usage remains
Verified that OptionsModel is only constructed via the newOptionsModel(interfaces::Node&, …)constructor (e.g. inbitcoin.cpp:273) and no.setNode(calls remain.
123-124: Confirm downstream usage update
Grep shows only a single call site in bitcoin.cpp using the new one-parameter signature; no force parameter remains.src/qt/guiutil_font.cpp (1)
367-381: Update loop is careful and two-phase; LGTM.
The cleanup of dead QPointers and deferred apply avoids inheritance glitches.Also applies to: 383-392, 439-447, 449-463
src/qt/optionsmodel.cpp (1)
99-115: Back-compat workaround with 22.x numerics: solid approach.
Writing specific numeric settings as strings to dodge UniValue exceptions is pragmatic and isolated.
src/node/interfaces.cpp
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also treat forced settings as “ignored” overrides
isSettingIgnored only checks command_line_options. If a setting is overridden via forced_settings, the GUI will not flag it as ignored. Either update the docstring in node.h or include forced settings here for consistency with ignore_nonpersistent.
Apply:
bool isSettingIgnored(const std::string& name) override
{
- bool ignored = false;
- gArgs.LockSettings([&](util::Settings& settings) {
- if (auto* options = util::FindKey(settings.command_line_options, name)) {
- ignored = !options->empty();
- }
- });
- return ignored;
+ bool ignored = false;
+ gArgs.LockSettings([&](const util::Settings& settings) {
+ if (const auto* v = util::FindKey(settings.command_line_options, name)) {
+ ignored = !v->empty();
+ }
+ if (!ignored) {
+ if (const auto* f = util::FindKey(settings.forced_settings, name)) {
+ ignored = !f->empty();
+ }
+ }
+ });
+ return ignored;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool isSettingIgnored(const std::string& name) override | |
| { | |
| bool ignored = false; | |
| gArgs.LockSettings([&](util::Settings& settings) { | |
| if (auto* options = util::FindKey(settings.command_line_options, name)) { | |
| ignored = !options->empty(); | |
| } | |
| }); | |
| return ignored; | |
| } | |
| bool isSettingIgnored(const std::string& name) override | |
| { | |
| bool ignored = false; | |
| gArgs.LockSettings([&](const util::Settings& settings) { | |
| if (const auto* v = util::FindKey(settings.command_line_options, name)) { | |
| ignored = !v->empty(); | |
| } | |
| if (!ignored) { | |
| if (const auto* f = util::FindKey(settings.forced_settings, name)) { | |
| ignored = !f->empty(); | |
| } | |
| } | |
| }); | |
| return ignored; | |
| } |
🤖 Prompt for AI Agents
In src/node/interfaces.cpp around lines 447 to 456, isSettingIgnored currently
only checks command_line_options and therefore misses settings present in
forced_settings; update the function to also check settings.forced_settings
(using util::FindKey or equivalent) and mark the setting as ignored if a
forced_settings entry exists or is non-empty, mirroring ignore_nonpersistent
behavior; keep the gArgs.LockSettings scope and combine the checks so that
ignored becomes true if either command_line_options or forced_settings indicates
an override.
src/node/interfaces.cpp
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify update path and handle write failures
This void updateRwSetting duplicates the logic of the existing bool updateRwSetting(name, value, write) and ignores the write result. Delegate to the boolean overload to centralize behavior and optionally log failures.
Apply:
void updateRwSetting(const std::string& name, const util::SettingsValue& value) override
{
- gArgs.LockSettings([&](util::Settings& settings) {
- if (value.isNull()) {
- settings.rw_settings.erase(name);
- } else {
- settings.rw_settings[name] = value;
- }
- });
- gArgs.WriteSettingsFile();
+ (void)updateRwSetting(name, value, /*write=*/true);
}🤖 Prompt for AI Agents
In src/node/interfaces.cpp around lines 458 to 468, this void updateRwSetting
duplicates logic of the existing bool updateRwSetting(name, value, write) and
ignores whether the write succeeded; replace the implementation to call the
boolean overload (e.g. updateRwSetting(name, value, /*write*/ true)) and if it
returns false, emit a failure log (use the existing logging facility in this
file) so write failures are not silently ignored, thereby centralizing behavior
and preserving the original semantics.
src/qt/bitcoingui.cpp
Outdated
There was a problem hiding this comment.
Null-deref risk if OptionsModel is null.
_setClientModel() unconditionally dereferences _clientModel->getOptionsModel() to set m_mask_values_action. This can crash when options model is not yet available. Guard it.
- m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool());
+ if (OptionsModel* opt = _clientModel->getOptionsModel()) {
+ m_mask_values_action->setChecked(opt->getOption(OptionsModel::OptionID::MaskValues).toBool());
+ } else {
+ m_mask_values_action->setChecked(false);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool()); | |
| } else { | |
| if (OptionsModel* opt = _clientModel->getOptionsModel()) { | |
| m_mask_values_action->setChecked(opt->getOption(OptionsModel::OptionID::MaskValues).toBool()); | |
| } else { | |
| m_mask_values_action->setChecked(false); | |
| } | |
| } else { |
🤖 Prompt for AI Agents
In src/qt/bitcoingui.cpp around lines 893-894, the code unconditionally calls
_clientModel->getOptionsModel()->getOption(...), risking a null-deref if
getOptionsModel() returns nullptr; change the logic to first check that
_clientModel and _clientModel->getOptionsModel() are non-null before calling
getOption, and only call m_mask_values_action->setChecked(...) when the options
model exists (otherwise set a safe default or leave the action unchecked);
ensure any subsequent logic that assumes the options model exists is similarly
guarded or deferred until the model is available.
src/qt/overviewpage.cpp
Outdated
There was a problem hiding this comment.
Null check before persisting privacy option
clientModel or its OptionsModel can be null during early lifecycle; this will dereference unconditionally.
Apply:
- clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy);
+ if (clientModel && clientModel->getOptionsModel()) {
+ clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy); | |
| if (m_balances.balance != -1) { | |
| setBalance(m_balances); | |
| if (clientModel && clientModel->getOptionsModel()) { | |
| clientModel->getOptionsModel()->setOption(OptionsModel::OptionID::MaskValues, privacy); | |
| } | |
| if (m_balances.balance != -1) { | |
| setBalance(m_balances); | |
| } |
🤖 Prompt for AI Agents
In src/qt/overviewpage.cpp around lines 210 to 212, the code unconditionally
calls clientModel->getOptionsModel()->setOption(...), which can dereference a
null clientModel or a null OptionsModel during early lifecycle; guard this by
checking clientModel is non-null and that clientModel->getOptionsModel() returns
a non-null pointer before calling setOption (fetch the OptionsModel into a local
variable and only call setOption when both are valid); leave the existing
setBalance(m_balances) as-is since the comment targets persisting the privacy
option.
src/qt/test/optiontests.cpp
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make JSON verification robust; avoid comparing full file text and dangling c_str.
Comparing the whole serialized file string is brittle (ordering/whitespace) and uses c_str() on temporaries. Parse JSON and assert fields instead.
- std::ifstream file(gArgs.GetDataDirNet() / "settings.json");
- QCOMPARE(std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()).c_str(), "{\n"
- " \"dbcache\": \"600\",\n"
- " \"listen\": false,\n"
- " \"onion\": \"onion:234\",\n"
- " \"par\": \"12\",\n"
- " \"proxy\": \"proxy:123\",\n"
- " \"prune\": \"2861\"\n"
- "}\n");
+ std::ifstream file(gArgs.GetDataDirNet() / "settings.json");
+ QVERIFY(file.is_open());
+ const std::string data((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>());
+ UniValue json;
+ QVERIFY(json.read(data));
+ QVERIFY(json.isObject());
+ QCOMPARE(QString::fromStdString(json["dbcache"].get_str()), QString("600"));
+ QCOMPARE(json["listen"].get_bool(), false);
+ QCOMPARE(QString::fromStdString(json["onion"].get_str()), QString("onion:234"));
+ QCOMPARE(QString::fromStdString(json["par"].get_str()), QString("12"));
+ QCOMPARE(QString::fromStdString(json["proxy"].get_str()), QString("proxy:123"));
+ QCOMPARE(QString::fromStdString(json["prune"].get_str()), QString("2861"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::ifstream file(gArgs.GetDataDirNet() / "settings.json"); | |
| QCOMPARE(std::string(std::istreambuf_iterator<char>(file), std::istreambuf_iterator<char>()).c_str(), "{\n" | |
| " \"dbcache\": \"600\",\n" | |
| " \"listen\": false,\n" | |
| " \"onion\": \"onion:234\",\n" | |
| " \"par\": \"12\",\n" | |
| " \"proxy\": \"proxy:123\",\n" | |
| " \"prune\": \"2861\"\n" | |
| "}\n"); | |
| } | |
| std::ifstream file(gArgs.GetDataDirNet() / "settings.json"); | |
| QVERIFY(file.is_open()); | |
| const std::string data((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>()); | |
| UniValue json; | |
| QVERIFY(json.read(data)); | |
| QVERIFY(json.isObject()); | |
| QCOMPARE(QString::fromStdString(json["dbcache"].get_str()), QString("600")); | |
| QCOMPARE(json["listen"].get_bool(), false); | |
| QCOMPARE(QString::fromStdString(json["onion"].get_str()), QString("onion:234")); | |
| QCOMPARE(QString::fromStdString(json["par"].get_str()), QString("12")); | |
| QCOMPARE(QString::fromStdString(json["proxy"].get_str()), QString("proxy:123")); | |
| QCOMPARE(QString::fromStdString(json["prune"].get_str()), QString("2861")); | |
| } |
🤖 Prompt for AI Agents
In src/qt/test/optiontests.cpp around lines 63 to 72, the test currently
compares the entire file text and calls c_str() on temporaries which is brittle
and unsafe; instead, read the file into a string or QByteArray, parse it as JSON
(e.g. QJsonDocument::fromJson or your project JSON parser), verify the parse
succeeded, and assert each expected key/value individually (dbcache as string or
number, listen as bool, onion/proxy as strings, par/prune as strings or numbers)
using QCOMPARE/QVERIFY; remove the direct full-string equality check and the
c_str() usage and add explicit error messages on parse failure.
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
|
This pull request has conflicts, please rebase. |
…to valid index 8bba1d2 fix: don't assume that successful `weightFromArg` links to valid index (Kittywhiskers Van Gogh) Pull request description: ## Additional Information [dash#7068](#7068) introduced a regression where Qt clients with no GUI settings are unable to run at all due to failed weight to index conversion. This was covered in 9cd9d44 but made an incorrect assumption that earlier iterations of the fix (see ef34868) did not make. The assumption made that resulted in this regression was that if the argument to weight conversion is successful, then the weight to index conversion will also be successful. The earlier iteration of the fix checked the return value of the index conversion and set a sane default if it was invalid. The final iteration set the default weight value to the sane weight, expecting `weightFromArg()` to fail (hence using the sane weight), not `supportedWeightToIndex()` (which might not be able to process even a valid weight from argument). This was not caught in testing as over the course of working on [dash#6833](#6833), my GUI settings were migrated to `settings.json`, which has precedence over `QSettings` and therefore, the client always ran with known good Montserrat parameters. Fix can be verified by deleting your QSettings parameters and running the client anew. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 8bba1d2 PastaPastaPasta: utACK 8bba1d2 Tree-SHA512: 6772d59a8cffb43427ea83b5ff2e725efbfc741e890c40e059ffe2fe3405fbe535f975f4ebbd78fbe44bdf671deaeb1a329b3c29530a8de4cceab02f5ce6009c
|
This pull request has conflicts, please rebase. |
…nt font for overview page balances) 636e7a6 refactor: remove redundant preview and simplify font handling (UdjinM6) b59b0f7 feat(qt): add live preview for monospace font in appearance settings (UdjinM6) 83af951 fix: set money font when setting wallet model instead of ctor (Kittywhiskers Van Gogh) 658905b qt: add non-monospace option and use it as default (Kittywhiskers Van Gogh) e0513ff qt: add horizontal spacer for visual consistency (Kittywhiskers Van Gogh) e5a8d05 refactor: move monospace font selector to appearance widget (Kittywhiskers Van Gogh) 41c69e9 merge bitcoin-core/gui#497: Enable users to configure their monospace font specifically (Kittywhiskers Van Gogh) da5804e merge bitcoin-core/gui#79: Embed monospaced font (Kittywhiskers Van Gogh) eac4f52 qt: adjust padding and font size of monospace font selector (Kittywhiskers Van Gogh) 9c315c3 qt: add monospaced font settings (Kittywhiskers Van Gogh) 34211b6 qt: apply monospace font to overview page balances and CoinJoin elements (Kittywhiskers Van Gogh) 6ed53d6 qt: allow overriding font registry for specific widgets (Kittywhiskers Van Gogh) 800cf66 qt: recognize system monospace font as non-selectable font (Kittywhiskers Van Gogh) 85b49b9 qt: add "Roboto Mono" as non-selectable font (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #7068 * Depends on #7084 * Dependency for #6833 | [dash#7084](#7084) (8bba1d2) | This PR (636e7a6) | | ------------------- | ------------------- | |  |  | |  |  | |  |  | * Based on review suggestions given ([comment](#6831 (comment))), [bitcoin-core/gui#497](bitcoin-core/gui#497) was also backported to allow arbitrary font specification. This also allowed us to offer a "Use existing font" option that _doesn't_ use any monospace font but instead inherits whatever font is used in the rest of the application. This is set as the default ([source](https://github.com/kwvg/dash/blob/89aa2c4b2390ada504fd5b45e16d2cd379074138/src/qt/optionsmodel.cpp#L73)), rendering monospace balance counters opt-in. * Unlike Bitcoin, Dash has a dedicated appearance widget for managing font settings specifically. We have opted to deviate from upstream by moving the money font control from "Display" to "Appearance" for the sake of consistency. * We set the money font when setting the wallet model as client model updates happen after the UI is visible to the user while wallet model updates happen _before_, there is little point in setting them to any values in the constructor as during construction, we don't have access to the options model and thus don't know _what_ font is supposed to be used to begin with. * If a font is passed through the command line, it is considered "selectable" ([source](https://github.com/kwvg/dash/blob/89aa2c4b2390ada504fd5b45e16d2cd379074138/src/qt/bitcoin.cpp#L676-L679)) because we still need to populate that font in the drop-down menu so that we don't report stale data. Though, the font cannot be _set_ to that overridden value because command-line overridden controls are auto-disabled to prevent settings corruption (see 2f30002), which makes the "selectable" state more-or-less an internal detail. An example is that the user tries to run Dash Core, the embedded monospace font, Roboto Mono, is _not_ a selectable font (i.e. you cannot ordinarily set your client to use Roboto Mono, see below). <details> <summary>Screenshot:</summary>  </details> But if an override is done, it will be respected, the client will start with Roboto Mono and the selectable status will be overwritten so that it is presented in the drop-down menu, albeit, with the menu disabled so it doesn't contaminate UI settings. <details> <summary>Screenshot:</summary>  </details> ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: ACK 636e7a6 Tree-SHA512: a8b23c14aec0ccac126bdc7a6db0a4583da3bda52e02f72d213431bbaa80c633a432ed02c4a33d71e3e4d2503d076624196c3e6e50c9bb7fd8e3b50ae4e9ec35
|
This pull request has conflicts, please rebase. |
Review with `git log -p -n1 --ignore-space-change --color-moved=dimmed_zebra`
…nsModel constructor
The migration to CoinJoin was done in dash#4038, included in v0.17 and there's more than a few major releases since then, we should be able to safely assume that upgrading users are using more recent clients.
We have a lot more configurable options and the current approach will get unwieldy as it grows
… onion-prev settings
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/qt/optionsmodel.cpp (1)
464-488: Proxy parsing confirmed addressed.The
ParseProxyString()implementation uses simple":"splitting. The IPv6 concern was raised in a previous review but marked as "✅ Addressed" in commits 6c9f1ae to 0c3b224. Since this is backported upstream code, maintaining fidelity is appropriate.
🧹 Nitpick comments (1)
doc/release-notes-6833.md (1)
6-6: Consider capitalizing operating system names.In Line 6, "windows" and "unix" appear lowercase. When referring to operating systems (Microsoft Windows, Unix-like systems), these are typically capitalized: "Windows registry or Unix desktop config files."
🔎 Suggested capitalization fix
-file rather than to the Qt settings backend (windows registry or unix desktop +file rather than to the Qt settings backend (Windows registry or Unix desktop
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 36795534ab4e314697b33cd6729585e82d856c17 and 0c3b224.
📒 Files selected for processing (15)
doc/release-notes-6833.mdsrc/qt/bitcoin.cppsrc/qt/bitcoin.hsrc/qt/bitcoingui.cppsrc/qt/forms/optionsdialog.uisrc/qt/optionsdialog.cppsrc/qt/optionsmodel.cppsrc/qt/optionsmodel.hsrc/qt/overviewpage.cppsrc/qt/test/addressbooktests.cppsrc/qt/test/apptests.cppsrc/qt/test/optiontests.cppsrc/qt/test/optiontests.hsrc/qt/test/test_main.cppsrc/qt/test/wallettests.cpp
💤 Files with no reviewable changes (1)
- src/qt/optionsdialog.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/qt/forms/optionsdialog.ui
- src/qt/overviewpage.cpp
- src/qt/bitcoingui.cpp
🧰 Additional context used
📓 Path-based instructions (3)
{guix-build*,releases,**/guix-build*,releases/**,.github/**,depends/**,ci/**,contrib/**,doc/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not make changes to build system files (guix-build*), release artifacts, or avoid changes to .github, depends, ci, contrib, and doc directories unless specifically prompted
Files:
doc/release-notes-6833.md
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/qt/test/wallettests.cppsrc/qt/bitcoin.hsrc/qt/test/apptests.cppsrc/qt/test/addressbooktests.cppsrc/qt/optionsmodel.hsrc/qt/bitcoin.cppsrc/qt/test/test_main.cppsrc/qt/test/optiontests.cppsrc/qt/optionsmodel.cppsrc/qt/test/optiontests.h
src/qt/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
GUI implementation in src/qt/ must use Qt 5
Files:
src/qt/test/wallettests.cppsrc/qt/bitcoin.hsrc/qt/test/apptests.cppsrc/qt/test/addressbooktests.cppsrc/qt/optionsmodel.hsrc/qt/bitcoin.cppsrc/qt/test/test_main.cppsrc/qt/test/optiontests.cppsrc/qt/optionsmodel.cppsrc/qt/test/optiontests.h
🧠 Learnings (22)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:26.891Z
Learning: In Dash Qt wallet code, when leveraging upstream Bitcoin Core wallet capabilities (especially for operations like rescanning), prefer using the inherited signal-based mechanisms (e.g., ShowProgress signals) over adding explicit Qt progress dialogs to minimize upstream deviation and simplify future backports.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
doc/release-notes-6833.mdsrc/qt/bitcoin.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-12-17T13:58:26.891Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:26.891Z
Learning: In Dash Qt wallet code, when leveraging upstream Bitcoin Core wallet capabilities (especially for operations like rescanning), prefer using the inherited signal-based mechanisms (e.g., ShowProgress signals) over adding explicit Qt progress dialogs to minimize upstream deviation and simplify future backports.
Applied to files:
doc/release-notes-6833.mdsrc/qt/bitcoin.hsrc/qt/optionsmodel.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
doc/release-notes-6833.md
📚 Learning: 2025-08-08T04:30:37.971Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:0-0
Timestamp: 2025-08-08T04:30:37.971Z
Learning: In the Dash codebase, direct RPC calling has been removed in recent commits, making suggestions about RPC command validation and error handling for direct RPC calls obsolete. The ProposalWizard and related components no longer use direct RPC calling patterns.
Applied to files:
doc/release-notes-6833.md
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/qt/test/wallettests.cpp
📚 Learning: 2025-12-17T13:58:19.813Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7072
File: src/qt/walletcontroller.cpp:520-528
Timestamp: 2025-12-17T13:58:19.813Z
Learning: In Dash Qt wallet code, prefer using existing Qt signals/slots (e.g., ShowProgress signals) for long-running operations instead of adding explicit QProgressDialog dialogs. This minimizes upstream deviations and simplifies backports. Apply this pattern to wallet-related Qt components (e.g., operations like rescanning) and rely on signal-based progress updates rather than bespoke dialogs.
Applied to files:
src/qt/test/wallettests.cppsrc/qt/test/apptests.cppsrc/qt/test/addressbooktests.cppsrc/qt/bitcoin.cppsrc/qt/test/test_main.cppsrc/qt/test/optiontests.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/qt/test/wallettests.cppsrc/qt/bitcoin.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-12-22T15:42:48.595Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.
Applied to files:
src/qt/bitcoin.hsrc/qt/optionsmodel.hsrc/qt/test/optiontests.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/qt/optionsmodel.hsrc/qt/optionsmodel.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/qt/bitcoin.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Applied to files:
src/qt/bitcoin.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
src/qt/bitcoin.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/qt/bitcoin.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
src/qt/bitcoin.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/qt/bitcoin.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Applied to files:
src/qt/bitcoin.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
src/qt/bitcoin.cppsrc/qt/optionsmodel.cpp
📚 Learning: 2025-10-28T18:36:40.263Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.263Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
Applied to files:
src/qt/test/test_main.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
src/qt/test/test_main.cppsrc/qt/test/optiontests.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/qt/optionsmodel.cpp
📚 Learning: 2025-12-22T15:42:51.679Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:51.679Z
Learning: In the Dash Core Qt codebase (src/qt/guiutil_font.h), kwvg intentionally uses `assert()` in setter methods like `SetWeightBold` and `SetWeightNormal` to enforce invariants (e.g., `assert(m_weights.count(m_font))`). This fail-fast approach is preferred over defensive checks with fallbacks when an inconsistent state indicates a programming error that should be caught during development, rather than a runtime condition requiring graceful handling.
Applied to files:
src/qt/optionsmodel.cpp
🧬 Code graph analysis (7)
src/qt/test/wallettests.cpp (1)
src/qt/test/addressbooktests.cpp (1)
optionsModel(125-125)
src/qt/bitcoin.h (1)
src/qt/bitcoin.cpp (2)
createOptionsModel(268-288)createOptionsModel(268-268)
src/qt/test/addressbooktests.cpp (1)
src/qt/test/wallettests.cpp (1)
optionsModel(156-156)
src/qt/bitcoin.cpp (1)
src/qt/optionsmodel.h (1)
PruneMiBtoGB(30-30)
src/qt/test/optiontests.cpp (1)
src/qt/test/optiontests.h (1)
OptionTests(14-30)
src/qt/optionsmodel.cpp (3)
src/interfaces/node.h (2)
node(48-50)node(481-481)src/util/system.cpp (8)
SettingName(190-193)SettingName(190-190)SettingToInt(656-659)SettingToInt(656-656)SettingToString(645-648)SettingToString(645-645)SettingToBool(667-670)SettingToBool(667-667)src/wallet/wallet.cpp (1)
new_value(81-81)
src/qt/test/optiontests.h (1)
src/qt/test/optiontests.cpp (11)
OptionTests(19-22)init(24-29)init(24-24)migrateSettings(31-72)migrateSettings(31-31)integerGetArgBug(74-90)integerGetArgBug(74-74)parametersInteraction(92-125)parametersInteraction(92-92)extractFilter(127-134)extractFilter(127-127)
🪛 Cppcheck (2.19.0)
src/qt/test/optiontests.cpp
[error] 22-22: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
🪛 LanguageTool
doc/release-notes-6833.md
[uncategorized] ~6-~6: “Windows” (operating system by Microsoft) is a proper noun and needs to be capitalized.
Context: ...rather than to the Qt settings backend (windows registry or unix desktop config files),...
(A_WINDOWS)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
🔇 Additional comments (29)
src/qt/bitcoin.h (1)
49-49: LGTM! Error handling improvement.The signature change from void to
[[nodiscard]] boolproperly enforces error checking, and the implementation correctly returns false whenInitfails, with appropriate error messaging shown to the user.src/qt/test/apptests.cpp (1)
79-79: LGTM! Test correctly validates initialization.The test properly wraps
createOptionsModelinQVERIFYto validate the boolean result, ensuring initialization failures are caught during testing.src/qt/test/test_main.cpp (1)
61-64: LGTM! Ensures proper initialization order.The lambda pattern correctly initializes
BasicTestingSetupto configuregArgsbefore retrieving the data directory path. The scoped lifetime of the dummy setup ensures proper cleanup while allowing the blocks directory to be created with the correct path.src/qt/test/wallettests.cpp (1)
156-158: LGTM! Correct adoption of two-phase initialization.The test properly implements the new node-aware initialization pattern:
- Constructs
OptionsModelwith the node reference- Declares
bilingual_str errorfor error reporting- Validates
Init(error)success viaQVERIFYThis aligns with the broader architectural change to node-backed settings persistence described in the release notes.
src/qt/test/addressbooktests.cpp (1)
125-128: LGTM!The test correctly adapts to the new
OptionsModelAPI by:
- Constructing with the node reference
- Declaring a
bilingual_str errorvariable- Verifying initialization succeeds via
QVERIFY(optionsModel.Init(error))This is consistent with the pattern used in
wallettests.cppand other test files.src/qt/test/optiontests.h (2)
9-10: LGTM!The added includes for
univalue.handutil/settings.hare required to support the newutil::Settings m_previous_settingsmember and JSON-based settings verification in the test implementation.
18-29: LGTM!The header changes correctly declare:
- Node-aware constructor matching the new
OptionsModelinitialization patterninit()slot as a Qt Test lifecycle method for per-test setup- New test methods
migrateSettings()andintegerGetArgBug()for settings migration verificationm_previous_settingsto capture/restore settings state between testsThe implementation in
optiontests.cpp(lines 18-28) confirms proper usage of these declarations.src/qt/bitcoin.cpp (3)
268-288: LGTM!The
createOptionsModelrefactor correctly:
- Returns
boolto indicate success/failure (aligning with[[nodiscard]]in header)- Constructs
OptionsModelwithnode()reference per new API- Calls
Reset()beforeInit()whenresetSettingsis true- Handles
Initfailure by building a detailed bilingual error message with settings path context- Shows a critical message box and returns
falseon failureThe use of
fs::quotedis correct as it's defined insrc/fs.h(confirmed in past review).
347-350: LGTM!The
InitPruneSettingupdate correctly matches the simplifiedSetPruneTargetGB(int)signature that no longer takes aforceparameter.
687-696: LGTM!The startup flow restructuring correctly ensures:
- Splash screen is created first (if enabled)
- Node is created via
app.createNode(*init)beforeOptionsModelcreateOptionsModelresult is checked, exiting withEXIT_FAILUREon initialization failureThis ordering is essential since
OptionsModelnow requires a valid node reference.src/qt/optionsmodel.h (5)
19-22: LGTM!Forward declarations for
bilingual_strandinterfaces::Nodeare correctly added to support the new constructor andInitsignatures without requiring full header includes.
48-48: LGTM!The constructor signature change from
(QObject* parent, bool resetSettings)to(interfaces::Node& node, QObject* parent)correctly bindsOptionsModelto a node reference at construction time, enabling node-backed settings persistence.
97-97: LGTM!The
MaskValuesoption is correctly added to theOptionIDenum, positioned afterServerto maintain logical grouping of boolean options.
110-117: LGTM!The API surface changes correctly support the new architecture:
Init(bilingual_str& error)returnsboolfor explicit success/failure handling with error detailsgetOption(OptionID, suffix)andsetOption(OptionID, value, suffix)provide generic access by option ID, supporting the-prevsuffix pattern for storing previous values
138-148: LGTM!The member and accessor changes correctly reflect the node-bound architecture:
SetPruneTargetGB(int)simplified signature (force behavior now handled internally)node()returns a reference sincem_nodeis always validm_nodestored as reference rather than pointer, enforcing non-null invariantsrc/qt/test/optiontests.cpp (4)
17-29: LGTM!The test infrastructure correctly implements:
#include <fstream>for readingsettings.json- Constructor captures
m_previous_settingsviagArgs.LockSettings()for isolationinit()lifecycle method restores settings before each test to ensure independence
31-72: Migration test matches upstream backport pattern.The
migrateSettings()test correctly verifies that:
- Legacy QSettings values are set and synced
- After
OptionsModel::Init(), legacy keys are removed from QSettings- Migrated values appear in
settings.jsonThe full JSON string comparison at lines 63-71 was flagged in a previous review as brittle. However, since this is a backport from bitcoin-core/gui and the learning indicates backported changes should match upstream, this implementation should be preserved to maintain fidelity and simplify future backports. Based on learnings, typos and minor issues in backported code should be kept as-is.
74-90: LGTM!The
integerGetArgBug()test correctly verifies the regression fix from bitcoin/bitcoin#24457, ensuring that integer prune values insettings.jsondon't cause exceptions inOptionsModel::Init().
108-109: LGTM!The
parametersInteraction()test correctly updated to use the newOptionsModelinitialization pattern withbilingual_str errorandInit(error).src/qt/optionsmodel.cpp (10)
43-77: LGTM!The
SettingName()function correctly maps GUIOptionIDvalues to their corresponding node setting names. The mapping covers all node-backed options including:
- Core options (dbcache, par, prune, proxy, listen, server)
- CoinJoin options (coinjoinamount, coinjoinrounds, etc.)
- Font settings (font-family, font-scale, font-weight-*)
The
std::logic_errorfor unmapped options provides a fail-fast mechanism to catch programming errors.
79-115: LGTM!The
RequiresNumWorkaround()andUpdateRwSetting()functions correctly implement backward compatibility with Bitcoin 22.x:
- Numeric settings that 22.x expects as strings are written via
getValStr()- This prevents
UniValue::get_str()exceptions in older releases- The comment clearly explains the rationale and references the upstream fix PR
This is inherited upstream behavior that should be preserved for backport fidelity.
117-145: LGTM!The prune helper functions correctly implement the GUI-to-node prune value conversions:
PruneSetting()converts enabled/size to the node's-pruneMiB valuePruneEnabled()correctly treats-prune=1(manual mode) as disabled for GUI purposesPruneSizeGB()converts MiB to GB for display, defaulting toDEFAULT_PRUNE_TARGET_GBParsePruneSizeGB()ensures minimum 1 GB to match GUI display constraintsThe
assert(!prune_enabled || prune_size_gb >= 1)enforces the invariant documented in the comment.
215-218: LGTM!The constructor correctly stores the node reference in the initializer list. The body is intentionally empty since
Init()must be called explicitly after construction, aligning with the new two-phase initialization pattern.
226-410: LGTM!The
Init()refactor correctly implements:
- Returns
boolwith error details viabilingual_str& error- Initializes language from node persistent settings
- Calls
checkAndMigrate()for legacy settings migration- Handles font settings with fallback weights when values are invalid
- Iterates over node-backed options with try/catch for malformed settings.json values
- Returns
falsewith descriptive error on settings parse failures- Initializes
m_mask_valuesfrom QSettingsThe loop at lines 364-379 provides robust error handling for corrupt settings.
431-457: LGTM!The
Reset()function correctly:
- Calls
node().resetSettings()to backup and clearsettings.json- Backs up QSettings to
guisettings.ini.bakfor troubleshooting- Preserves
strDataDiracross reset- Clears all QSettings and marks reset with
fResetflag
495-525: LGTM!The
SetPruneTargetGB()refactor correctly:
- Compares current vs new prune settings using node persistent settings
- Forces the prune setting to take effect before node starts
- When pruning enabled, also forces
disablegovernance=1andtxindex=0(Dash-specific)- Only writes to
settings.jsonif values actually changed- Preserves previous prune value with
-prevsuffix when disabling
579-714: LGTM!The
getOption()implementation correctly:
- Uses a lambda
setting()to fetch node persistent settings with suffix support- Returns values from node settings for node-backed options
- Returns values from QSettings or member variables for Qt-only options
- Implements the
-prevsuffix fallback pattern for proxy and prune options- Uses
SettingToString/Int/Boolhelpers for type-safe conversions with defaults
716-1023: LGTM!The
setOption()implementation correctly:
- Uses
changed()lambda to avoid unnecessary writes- Uses
update()lambda to route writes throughUpdateRwSetting()- Handles the
-prevsuffix pattern for preserving previous values- Emits appropriate signals (e.g.,
coinControlFeaturesChanged,coinJoinEnabledChanged)- Sets
RestartRequiredflag for options requiring restart- Prune/PruneSize cases are correctly outside
ENABLE_WALLETblock (per resolved past review)
1051-1168: LGTM!The
checkAndMigrate()function correctly implements settings migration:
- Version-based migration for legacy settings cleanup (pre-230100 PrivateSend removal)
- Proxy address sanitization for the
%2bug (issue bitcoin#12623)- Theme validation with fallback to default
migrate_settinglambda that:
- Checks if legacy QSettings key exists
- Only migrates if node setting is null (no override)
- Handles special cases (proxy parsing, font weight conversion)
- Removes legacy key after migration
- Calls
node().initParameterInteraction()to propagate dependent settings (e.g.,-listenaffecting-listenonion)The migration covers all node-backed options including Dash-specific CoinJoin settings.
There was a problem hiding this comment.
I noticed that -resetguisettings drops all settings from settings.json, not just gui related ones. I had 3 wallets listed there and after gui reset it's an empty file and I have to reopen them manually. Is it intentional or was it fixed in bitcoin later?
EDIT: Can't find any related fixes in bitcoin but it feels like an actual bug, not a feature. Pls consider 5c6a787.
|
Hmm... My previous suggestion was incomplete I guess - |
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
3 participants
Additional Information
Depends on backport: merge bitcoin#23345, #24410, #15936, #25748, #25863, #24051, #25315, #26624, #26894, #26673, #24462, #25815, #22949, #26723, #23395, #27016, #27189, #27317 (auxiliary backports: part 28) #6901
Depends on feat(qt): extract Dash-specific font infrastructure to
qt/guiutil_font.{cpp,h}, encapsulate globals intoFont{Info,Registry}, add arbitrary font support #7068Depends on backport: merge bitcoin-core/gui#79, #497 (allow different font for overview page balances) #6831
Breaking Changes
See release notes.
Checklist