Stabilize pre-init datadir and settings resolution - #812
Conversation
Keep the datadir selected in onboarding separate from the path Core resolves after config parsing. Run pre-init preview and apply through a scratch ArgsManager seeded from the original argv so live gArgs reaches InitConfig without preloaded config or settings state. Finalize onboarding settings after InitConfig and chain-specific QSettings setup. Apply reset across Core and GUI stores transactionally, preserve strDataDir, honor final source precedence, and recover unreadable settings with the Qt reset-or-abort behavior. Only open and synchronize settings stores that are used or modified, preserving rollback without allowing unrelated stores to block startup. Add unit and functional coverage for datadir precedence, reset behavior, recovery, persistence, and rollback.
epicleafies
left a comment
epicleafies
left a comment
There was a problem hiding this comment.
tested ACK 49c2f25
Built on Qt 6.4.2, unit and functional suites green, plus a headed pass under xcb. Confirmed the reset targets the resolved datadir's settings.json (custom datadir kept, default untouched), config-redirected datadirs reach the main window, and -resetguisettings=0 is a no-op.
Two items the green suites do not catch (both headed-only) are inline: #775 (an unreadable saved datadir still aborts at startup, a hard crash rather than a soft failure; details on the issue) and an empty reset backup when settings.json is unparseable. A few smaller issues inline too.
| bool ReadResolvedProfile(ArgsManager& args, ReadOnlyProfileResult& result, QString* error) | ||
| { | ||
| result = {}; | ||
| if (!ReadConfigAndSelectNetwork(args, error)) return false; |
There was a problem hiding this comment.
This is #775, flagged at the fix point. Worth noting it is a hard crash (uncaught filesystem_error -> SIGABRT on a mode-000 saved datadir), not the soft failure the issue text implies. Since this diff already reworks this path, a try/catch(const std::exception&) around the ReadResolvedProfile call routed to status.error, mirroring the SelectParams guard, would close it here. Not blocking if you prefer to keep it in the follow-up.
| return false; | ||
| } | ||
| } | ||
| if (!BackupSettingsFile(args, error)) return false; |
There was a problem hiding this comment.
Suggestion: when settings.json is unreadable and a reset is requested, this backup captures an already-emptied file. InitConfig runs first; on an unparseable settings.json, ReadSettingsFile clears rw_settings and fails, the read-error handler returns false to proceed, and InitConfig immediately writes an empty document. Only afterward does BackupSettingsFile run, so settings.json.bak holds the emptied file, not the original bytes. Reproduced: seed a corrupt settings.json, launch with -resetguisettings, finish onboarding; the .bak contains only the auto-generated header. Low severity since the data was unreadable and the user asked to reset, but the backup gives false reassurance in the one case it exists for.
Test gaps around this path: run_malformed_settings_reset_recovers does not assert .bak contents, and resetLegacyCleanupRollsBackOnWriteFailure (the only rollback-failure test for the legacy-cleanup path) is QSKIP'd off POSIX, so it is unverified on Windows CI. Consider backing up before InitConfig, or documenting the limitation, plus asserting the backup content.
| active_gui_settings_values, | ||
| QStringLiteral("GUI settings update"), | ||
| error)) { | ||
| RestoreGuiSettings(active_gui_settings_store, original_active_gui_settings, error); |
There was a problem hiding this comment.
Suggestion: this rollback opens a fresh QSettings on the same file while the original active_gui_settings owner is still alive, so two QSettings back the same file with independent caches. The explicit restore sync() should win, but RestoreStores in legacy_settings_migration.cpp handles the same shape by resetting the owner first. The two rollback paths are inconsistent; resetting or rewriting through the existing object would be less fragile.
| if (pending.effective_reset != args.GetBoolArg("-resetguisettings", false)) { | ||
| if (error) { | ||
| *error = QStringLiteral( | ||
| "The selected Bitcoin profile changed while onboarding was open. " |
There was a problem hiding this comment.
Nit: these new startup-error strings reach the user via InitError but are untranslated QStringLiterals. Wrap the user-visible ones in tr() (with %1 placeholders rather than concatenation).
| QString::fromStdString(strprintf("%s.", error.translated)), | ||
| QMessageBox::Reset | QMessageBox::Abort, | ||
| }; | ||
| message_box.setInformativeText(QObject::tr("Do you want to reset settings to default values, or to abort without making changes?")); |
There was a problem hiding this comment.
Nit: this string lost the translator comment that the Widgets ErrorSettingsRead carries. In bitcoin/src/qt/bitcoin.cpp the same setInformativeText has a /*: ... */ comment above it explaining the reset-or-abort choice for translators. Since this is the one translated string in the new code, restoring the comment keeps parity and satisfies the translator-comment convention.
2 participants
This is a follow-up to #742.
Pre-init onboarding was doing too much work against the live
gArgs. It read config and settings to build the preview, then normal startup calledInitConfig()on the sameArgsManager. Ifbitcoin.confredirected the datadir, the second config read could start from a different place and fail before the main window opened.-resetguisettingshad a similar ordering problem. We could reset the bootstrap profile before Core had resolved the datadir and network it was actually going to use.This switches the pre-init reads to a scratch
ArgsManagerbuilt from the original argv. The datadir selected in onboarding is kept separate from the one resolved through config, and settings changes are finalized afterInitConfig()and the chain-specificQSettingssetup.A few related pieces are handled in the same path:
-resetguisettings=0is treated as false.strDataDiris preserved when GUI settings are cleared.settings.jsonthrough the existing reset-or-abort flow.Cancelling onboarding remains non-mutating. No settings are cleared and no backups are created.
Fixes #773.
Fixes #774.
This is related to #600, but does not close it. The Settings-page actions and
bitcoin.confopening flow are still separate work.#775 also remains open. An existing saved datadir that cannot be traversed can still fail before the chooser is shown.