Crash on Non HD wallets fix #1709

levonpetrosyan93 · 2025-11-10T09:13:48Z

Fix for issue #1530

coderabbitai · 2025-11-10T09:14:04Z

Walkthrough

Adds defensive null-checks and UI gating for Spark wallet presence across three Qt UI flows: anonymize button enabling (overviewpage.cpp), automint notification display (walletview.cpp), and receive-coins address-type handling (receivecoinsdialog.cpp), hiding Spark-specific controls when wallet or sparkWallet is absent.

Changes

Cohort / File(s)	Summary
Overview & Wallet notifications `src/qt/overviewpage.cpp`, `src/qt/walletview.cpp`	Added guards that call `walletModel->getWallet()` and verify `wallet->sparkWallet` before proceeding. `overviewpage.cpp` now requires `sparkWallet` and Spark-allowed to enable the anonymize button; `walletview.cpp` returns early from automint notification if wallet or `sparkWallet` is null.
Receive coins UI gating `src/qt/receivecoinsdialog.cpp`	In `setModel` remove the Spark option from the address type combobox and adjust `reuseAddress` / `createSparkNameButton` visibility when wallet or `sparkWallet` is missing. In `displayCheckBox` switch from index check to testing `addressTypeCombobox` current text == "Spark" to control visibility.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pay attention to null-check ordering and potential nullptr dereferences.
Verify the use of literal "Spark" for UI text comparisons (localization/robustness).
Confirm consistent UX behavior across all three dialogs when sparkWallet is absent.

Possibly related PRs

UI improvements #1475 — Similar UI guards and sparkWallet availability checks in Qt wallet UI.
GUI freeze fix #1686 — Related modifications around Spark wallet access and WalletModel interactions.

Suggested reviewers

psolstice
aleflm

Poem

🐇 I hopped through lines of tidy code,
Checked every wallet, light and glowed,
If Spark's not here, I tuck away —
No nulls to fright the devs today. ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description only contains 'Fix for issue #1530' and lacks mandatory content. The template requires a 'PR intention' section explaining what the PR does and what issue it solves, plus an optional 'Code changes brief' section. The current description is far too minimal.	Expand the description to include a detailed explanation of the problem being fixed, the solution implemented, and how the code changes prevent the crash on non-HD wallets.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Crash on Non HD wallets fix' clearly describes the main change - fixing a crash issue with non-HD wallets, which aligns with the PR objective of addressing issue #1530 where the application crashes when generating a Spark address.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch nonHd

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ebb83b2 and 3934c06.

📒 Files selected for processing (2)

src/qt/overviewpage.cpp (1 hunks)
src/qt/walletview.cpp (1 hunks)

⏰ 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). (16)

GitHub Check: build-guix-arm64-apple-darwin
GitHub Check: build-guix-x86_64-apple-darwin
GitHub Check: build-guix-x86_64-w64-mingw32
GitHub Check: build-guix-x86_64-linux-gnu
GitHub Check: build-mac-cmake
GitHub Check: build-guix-aarch64-linux-gnu
GitHub Check: build-windows-cmake
GitHub Check: build-linux-cmake
GitHub Check: build-guix-x86_64-apple-darwin
GitHub Check: build-guix-aarch64-linux-gnu
GitHub Check: build-guix-arm64-apple-darwin
GitHub Check: build-guix-x86_64-w64-mingw32
GitHub Check: build-windows-cmake
GitHub Check: build-mac-cmake
GitHub Check: build-guix-x86_64-linux-gnu
GitHub Check: build-linux-cmake

src/qt/overviewpage.cpp

src/qt/walletview.cpp

justanwar · 2025-11-26T15:39:35Z

Tested. Crashes when generating Spark address in Receive tab (should show error/warning instead).

justanwar · 2025-12-10T17:42:37Z

Working as expected.

aleflm

Other than two minor issues I raised. Everything looks good to me.

aleflm · 2025-12-19T09:19:40Z

src/qt/receivecoinsdialog.cpp

 void ReceiveCoinsDialog::displayCheckBox(int idx)
 {
-    if(idx==0){
+    if(ui->addressTypeCombobox->currentText() == "Spark"){


This will cause issues with Localization.

Something like this (roughly) might be better:

// Only works if "Spark" is translated exactly the same way here as in the .ui file if(ui->addressTypeCombobox->currentText() == tr("Spark")){

agree, fixed

aleflm · 2025-12-19T09:20:16Z

src/qt/receivecoinsdialog.cpp


+        auto wallet = _model->getWallet();
+        if (!wallet || !wallet->sparkWallet) {
+            ui->addressTypeCombobox->removeItem(0);


This assumes item 0 is always “Spark”. Is this a correct assumption? ( I am not sure, just asking ).

Yes, item 0 at this point is always "Spark"

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/qt/receivecoinsdialog.cpp (2)
125-130: Defensive null-checks prevent crash on non-HD wallets.

The guard correctly removes Spark functionality when sparkWallet is unavailable, addressing the crash reported in issue #1530.
💡 Optional: Make item removal more maintainable

Instead of hardcoding removeItem(0), find the Spark item by its associated data value to protect against future reordering:
-        ui->addressTypeCombobox->removeItem(0);
+        int sparkIndex = ui->addressTypeCombobox->findData(Spark);
+        if (sparkIndex != -1) {
+            ui->addressTypeCombobox->removeItem(sparkIndex);
+        }
This decouples the removal logic from the item order defined in the constructor.
337-337: Localization-aware text comparison is correct.

Using tr("Spark") properly addresses the localization concern raised in earlier reviews.
💡 Optional: Use data-based comparison for robustness

Instead of comparing translated strings, compare the item's data value:
-    if(ui->addressTypeCombobox->currentText() == tr("Spark")){
+    if(ui->addressTypeCombobox->currentData().toInt() == Spark){
This eliminates dependency on string matching and translation consistency, making the code more robust against translation variations or typos.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b8316c8 and 4131a33.

📒 Files selected for processing (1)

src/qt/receivecoinsdialog.cpp (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/qt/receivecoinsdialog.cpp (1)

src/qt/bitcoin.cpp (1)

wallet (241-241)

⏰ 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). (16)

GitHub Check: build-linux-cmake
GitHub Check: build-guix-x86_64-w64-mingw32
GitHub Check: build-guix-x86_64-apple-darwin
GitHub Check: build-guix-x86_64-linux-gnu
GitHub Check: build-mac-cmake
GitHub Check: build-windows-cmake
GitHub Check: build-guix-aarch64-linux-gnu
GitHub Check: build-guix-arm64-apple-darwin
GitHub Check: build-guix-x86_64-apple-darwin
GitHub Check: build-guix-arm64-apple-darwin
GitHub Check: build-linux-cmake
GitHub Check: build-guix-x86_64-linux-gnu
GitHub Check: build-windows-cmake
GitHub Check: build-guix-aarch64-linux-gnu
GitHub Check: build-guix-x86_64-w64-mingw32
GitHub Check: build-mac-cmake

Crash on Non HD wallets fix

3934c06

coderabbitai bot requested review from aleflm and psolstice November 10, 2025 09:14

coderabbitai bot reviewed Nov 10, 2025

View reviewed changes

src/qt/overviewpage.cpp Outdated Show resolved Hide resolved

src/qt/walletview.cpp Outdated Show resolved Hide resolved

Ai comments resolved

ad711a1

one more crash fixed (#1721)

b8316c8

aleflm previously approved these changes Dec 19, 2025

View reviewed changes

Review comment applied

4131a33

levonpetrosyan93 dismissed aleflm’s stale review via 4131a33 December 19, 2025 22:19

levonpetrosyan93 requested a review from aleflm December 19, 2025 22:20

coderabbitai bot reviewed Dec 19, 2025

View reviewed changes

reubenyap merged commit 68681e7 into master Dec 24, 2025
21 checks passed

Uh oh!

Crash on Non HD wallets fix #1709

Crash on Non HD wallets fix #1709

Uh oh!

Conversation

levonpetrosyan93 commented Nov 10, 2025

Uh oh!

coderabbitai bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justanwar commented Nov 26, 2025

Uh oh!

justanwar commented Dec 10, 2025

Uh oh!

aleflm left a comment

Choose a reason for hiding this comment

Uh oh!

aleflm Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

levonpetrosyan93 Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

aleflm Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

levonpetrosyan93 Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coderabbitai bot commented Nov 10, 2025 •

edited

Loading