Stop Descriptor and the back arrow doing each other's job - #13
Merged
Conversation
In Extended public key, choosing "Descriptor" left the menu and taking the back arrow opened the descriptor. Both address types are affected. choose_menu_at returns the arrow as the index one past the last row - the slot the Back row occupied before the arrow replaced it - so a caller checks for `count`. This one checked `count - 1`, which is the last real item, and here that item is "Descriptor": the check caught it and returned, while the arrow's own value fell past every branch to the else that opens the descriptor. It was correct when written. The menu had an explicit "Back" row then, so `count - 1` was that row's index. Lifting Back into the arrow was meant to be transparent for callers - the arrow returns the index the row used to have, so "the checks against its old index still read the same answer" - and it is, for a caller comparing against a literal, which is what Backup, Settings and Derivation each do. This one derived its check from `count`, and `count` itself shrank by one when the row left the array, so the comparison slid off the arrow and onto the last item. Silent, because the result is still a valid index. The other eight menu call sites were read for the same shape; none derives its exit test from `count`, and the two that compute their row count still compare against the right value. Nothing here would have caught it: the self-test covers rendering and geometry, not menu dispatch, and these functions are static in seedtool_app.c and out of the simulator's reach. Adding a check that would is worth doing and is not attempted here. Verified with the host self-test and the 51 Python tests. The dispatch itself was checked by reading every menu call site, not by walking the menu - see above for why this tree has no way to assert it.
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.
In Extended public key, choosing
Descriptorleft the menu and taking the back arrow opened the descriptor. Both address types are affected.Cause
choose_menu_atreturns the arrow as the index one past the last row — the slot theBackrow occupied before the arrow replaced it — so a caller checks forcount. This one checkedcount - 1, which is the last real item, and here that item isDescriptor: the check caught it and returned, while the arrow's own value fell past every branch to theelsethat opens the descriptor.It was correct when written. The menu had an explicit
Backrow then, socount - 1was that row's index. LiftingBackinto the arrow was meant to be transparent for callers — the arrow returns the index the row used to have, so "the checks against its old index still read the same answer" — and it is, for a caller comparing against a literal, which is what Backup, Settings and Derivation each do. This one derived its check fromcount, andcountitself shrank by one when the row left the array, so the comparison slid off the arrow and onto the last item. Silent, because the result is still a valid index.Scope
The other eight menu call sites were read for the same shape. None derives its exit test from
count, and the two that compute their row count (browse_addresses, the type menu) still compare against the right value. This was the only one left.Not covered
Nothing in the tree would have caught this. The self-test covers rendering and geometry, not menu dispatch, and these functions are
staticinseedtool_app.cand out of the simulator's reach. Adding a check that would is worth doing and is deliberately not attempted here.Verification
Host self-test and the 51 Python tests. The dispatch itself was checked by reading every menu call site, not by walking the menu — see above for why this tree has no way to assert it.