Add address error location to GUI #536

meshcollider commented at 11:53 AM on January 24, 2022: contributor

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: <img width="203" alt="image" src="https://user-images.githubusercontent.com/3211283/150777849-aa19ef95-da53-4ff0-864c-7e290287e995.png">

Which, when clicked, opens a dialog with the error message and highlighted detected errors in red: <img width="499" alt="image" src="https://user-images.githubusercontent.com/3211283/150777754-44f6fab8-d16f-4d34-abc0-29278223e2c3.png">

meshcollider added the label Feature on Jan 24, 2022

in src/qt/qvalidatedlineedit.cpp:154 in 7556ace3e7 outdated

 149 | +        error_text += "<pre>";
 150 | +        if (!error_locations.empty()) {
 151 | +            int prev_loc = 0;
 152 | +            for (int loc : error_locations) {
 153 | +                error_text += input.mid(prev_loc, loc - prev_loc);
 154 | +                error_text +=  "<FONT COLOR='#ff0000'>" + input[loc] + "</FONT>";

katesalazar commented at 12:58 PM on January 24, 2022:

is something like this possible here?

meshcollider commented at 1:03 PM on January 24, 2022:

Sure, done

katesalazar commented at 5:54 AM on January 25, 2022:

I mean calling red what is red

luke-jr commented at 5:16 PM on January 24, 2022: member

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)

meshcollider commented at 9:46 PM on January 24, 2022: contributor

I don't think it's as safe to point out potential errors (which may be incorrect) unless the user asks for it.

Add address error location to GUI 69d22c9b0b

meshcollider cross-referenced this on Jan 24, 2022 from issue Point out position of invalid characters in Bech32 addresses by luke-jr

DrahtBot commented at 11:14 PM on January 24, 2022: contributor

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.

meshcollider cross-referenced this on Jan 25, 2022 from issue Bech32 typo detection by fanquake

Sjors commented at 8:55 AM on January 25, 2022: member

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.

meshcollider commented at 11:40 PM on January 25, 2022: contributor

@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.

luke-jr commented at 12:22 AM on January 26, 2022: member

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. ;)

Sjors commented at 9:24 AM on January 26, 2022: member

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?

meshcollider commented at 10:18 AM on January 26, 2022: contributor

@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.

Sjors commented at 12:06 PM on January 26, 2022: member

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.")

meshcollider added the label UX on Feb 19, 2022

meshcollider added the label Wallet on Feb 19, 2022

DrahtBot cross-referenced this on Mar 2, 2022 from issue Warn when sending to already-used Bitcoin addresses by luke-jr

hebasto requested review from achow101 on Jul 19, 2022

achow101 commented at 3:36 PM on July 19, 2022: member

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.

Rspigler commented at 3:16 AM on July 20, 2022: contributor

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

jb55 commented at 11:14 PM on July 21, 2022: contributor

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."

meshcollider closed this on Dec 13, 2022

bitcoin-core locked this on Dec 13, 2023