gui
gui copied to clipboard
bitcoin-core
Add address error location to GUI
Closes #527. May supersede #533, depending on whether people feel the error reason is better displayed immediately or in this validation dialog.
This is an alternative to #537: I believe we should only show the user potential errors if they ask for it, as the errors are not guaranteed to be correct and enabling/encouraging the user to bruteforce the errors may lead to loss of funds.
Adds a context menu item "Attempt error location" to the address inputs:
Which, when clicked, opens a dialog with the error message and highlighted detected errors in red:
Approach NACK. User shouldn't need to actively interact like this.
See https://github.com/bitcoin/bitcoin/compare/22.x...luke-jr:gui_bech32_errpos-22+knots for a better approach (I'll rebase it soon)
I don't think it's as safe to point out potential errors (which may be incorrect) unless the user asks for it.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #537 (Point out position of invalid characters in Bech32 addresses by luke-jr)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
This seems overly cautious, and I tend to prefer #537.
First of all I suspect most users in most cases will copy-paste addresses, so error correction is only needed in a minority of cases. Bech32 allows up to 4 typos without ambiguity.
Perhaps we could hide the positions behind a menu like this if there are more than 5 typos.
@Sjors Bech32 allows up to 4 typos without ambiguity.
Only in detection, not in correction. We can only accurately correct two errors, and if more than that are made, the results could be incorrect. This is also only substitution errors - deletion and insertion errors are not handled well.
@Sjors Perhaps we could hide the positions behind a menu like this if there are more than 5 typos.
There is no way for the code to know how many errors are made. Anything could happen if more than that are made, including that you chance upon another valid address by accident.
There is no way for the code to know how many errors are made. Anything could happen if more than that are made, including that you chance upon another valid address by accident.
Which is why expecting error detection to be perfect is unreasonable, and efforts to hide it behind a menu just a nuisance. ;)
Only in detection, not in correction.
Right, but we're not doing correction. That feature might indeed be better for a dropdown menu with a warning when there's more than 2 characters.
This is also only substitution errors - deletion and insertion errors are not handled well.
For now those impact the length, in which case we mark the whole thing invalid. (though that leaves swapping characters)
There is no way for the code to know how many errors are made. Anything could happen if more than that are made, including that you chance upon another valid address by accident.
So this is a scenario where the user makes so many typos that they are, in error correcting space, closer to a different address? In that case the user would see a number of characters marked as incorrect, but those would actually not be incorrect. Without autocorrect, a user would then take another look at their piece of paper to see if they misread those characters. That would be confusing.
It could also be dangerous if, rather than looking at the paper, they start randomly permutating the wrong character (assuming it's only 1, maybe 2 if they're really patient).
What are the odds the above scenario. @sipa?
@Sjors sorry, I may have worded things a bit confusingly.
Right, but we're not doing correction.
We actually are. The error location code is actually computing the specific errors themselves, and would be able to correct them. We deliberately don't show those corrections to the user, just which characters (we think) are wrong. This can only be done with up to 2 characters.
This comment I wrote mentions this in the error location code. In that code, l_e1 is the (log of the) error itself (and similarly for l_e2 later on in the function, explained here).
Detection is just "is this whole string valid or not." That's what already exists in master right now (whether the checksum verifies or not). It cannot say anything about the specific location of those errors.
For now those impact the length, in which case we mark the whole thing invalid. (though that leaves swapping characters)
I don't think we check the length do we? I may be wrong.
So this is a scenario where the user makes so many typos that they are, in error correcting space, closer to a different address?
Right.
In that case the user would see a number of characters marked as incorrect, but those would actually not be incorrect. Without autocorrect, a user would then take another look at their piece of paper to see if they misread those characters. That would be confusing.
Again, only at most two errors would be shown, and they'd almost certainly be wrong if anything was shown.
It could also be dangerous if, rather than looking at the paper, they start randomly permutating the wrong character (assuming it's only 1, maybe 2 if they're really patient).
Right, this is exactly the danger I have in mind. It would be so easy for a user to test a letter, backspace, test another, backspace, etc. Until they find one that is "correct". This may lead to fund loss, because the errors are not necessarily right, and there are only 32 possible characters to try. We should at least make this process slow and painful, and probably include some warnings too.
In that case, maybe we should do two things when error positions are found:
- Highlight them, but not in red
- Put a warning below the input: "Please check these characters. If they are correct, re-type the whole address. Do not attempt to guess." (and then in a tooltip: "The marked characters are our best guess, but it's possible the mistake is somewhere else. Randomly trying different characters may lead to a loss of funds. Please carefully check the original address, e.g. the paper note.")
Concept ACK, although having to get the info by opening a menu item is rather clunky. What about putting the error information in the tooltip?
Also, I think there needs to be a warning that the found errors are not necessarily correct.
It could also be dangerous if, rather than looking at the paper, they start randomly permutating the wrong character (assuming it's only 1, maybe 2
I don't see what reasonable person would do this, rather than checking the address they are supposed to be copying. But I agree that something like @Sjors suggestion (https://github.com/bitcoin-core/gui/pull/536#issuecomment-1022137943) would be a good fix
I do prefer it as in PR 537, with a warning
Although having to get the info by opening a menu item is rather clunky
Agreed that the context menu is clunky and very unintuitive. Perhaps there could be a warning icon that appears next to the input box that you can click to open the dialog. I do like the dialog because it would allow us to put in @Sjors full warning message next to the marked error locations:
"Please check these characters. If they are correct, re-type the whole address. Do not attempt to guess. The marked characters are our best guess, but it's possible the mistake is somewhere else. Randomly trying different characters may lead to a loss of funds. Please carefully check the original address, e.g. the paper note."
