gui: fix the ImGui assertion when creating a recovery phrase - #118
Merged
Conversation
Creating a recovery phrase from the window aborted with an ImGui assertion on the success path: Expression: (g.DisabledStackSize > 0) && "Calling EndDisabled() too many times!" File: imgui/imgui.cpp Line: 8435 The Finish button is wrapped in BeginDisabled/EndDisabled guarded by !g_phraseWrittenDown, and its own click handler sets g_phraseWrittenDown to false. So the pair was opened while the checkbox was ticked and closed after the handler had unticked it: BeginDisabled was never called, EndDisabled was, and ImGui asserted on the unmatched close. It fires on the only path that installs a seed -- tick the box, press Finish -- so every phrase created from the window has hit it since the feature shipped in 1.2.12. Three releases carried it because nothing tests the window. Reproduced on the published 1.2.14 binary and confirmed fixed on this build, driving the real window: Wallet Safety -> Create recovery phrase -> tick -> Finish. The old build raises the assertion; the fixed build closes the dialog, reports "This wallet has a recovery phrase", and the address changes to a derived one. The seed itself was already written before the assertion, so the wallet was never left half-installed -- and ImGui's IM_ASSERT_USER_ERROR_RET returns before touching its state, so answering Ignore was safe. Abort was not: it kills the process with no DBFlush. The restore dialog next door already does this correctly, copying the condition into a local first. This makes the create dialog match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Creating a recovery phrase from the window aborts with an ImGui assertion, on the success path:
DrawCreatePhraseDialogguards the Finish button with aBeginDisabled/EndDisabledpair, and the button's own handler mutates the condition that guards it:The pair is opened while the checkbox is ticked and closed after the handler has unticked it. Tick the box and press Finish — the only path that installs a seed — and ImGui asserts.
This has been there since the phrase UI shipped in 1.2.12, through three releases, because nothing in the test suite touches the window.
The fix
Read the condition once, into a local, before the button that changes it. The restore dialog next door (
DrawRestorePhraseDialog) already does exactly this with itsfBusyflag; this makes the create dialog match.Testing
mingw32-make -f makefile.mingw testsin MSYS2 UCRT64: builds clean, all five self-tests pass.Then the test that actually mattered, driving the real window on Windows 11 with Mesa llvmpipe — Wallet Safety → Create recovery phrase → tick the box → Finish:
Notes for anyone who hits this on a running wallet
The seed is installed before the assertion fires —
SetHDSeedFromMnemonicruns earlier in the same handler — so the phrase on screen is the wallet's real phrase and the wallet is not left half-installed.Answering Ignore is safe. The macro is
IM_ASSERT_USER_ERROR_RET, which returns before touchingDisabledStackSize, so no ImGui state is corrupted and the window carries on. Abort is not safe: it kills the process with noDBFlush, which is how awallet.datbecomes unopenable (#40).