Point out position of invalid characters in Bech32 addresses

luke-jr commented at 6:34 pm on January 24, 2022: member

Implements #527

gui_bech32_errpos

meshcollider commented at 11:01 pm on January 24, 2022: contributor

Approach NACK, I am a bit concerned about showing this to the user without them asking for it, as the error locations may not be correct.

This may cause users that don’t understand the error correction to try and brute-force the incorrect characters (which they can do easily with this approach), leading to loss of funds.

Please add #536 as an alternative to the PR post.

DrahtBot commented at 11:13 pm on January 24, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	Rspigler, jarolrod
Concept ACK	promag, Sjors, shaavan, rebroad
Approach NACK	meshcollider

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#686 (clang-tidy: Force checks for headers in src/qt by hebasto)
#553 (Change address / amount error background by w0xlt)

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.

luke-jr commented at 11:44 pm on January 24, 2022: member

The user asking for it doesn’t make it any more likely to be correct or “safe”.

This has also been in Knots for nearly 2 years (since 0.19.1) with no reported issues.

Sjors commented at 9:02 am on January 25, 2022: member

Concept ACK, but consider some sort of warning / restriction when there are more than 5(?) typos: #536 (comment)

Instead of using a merge commit 7e9667386177567d00f7195530812ad808f12e82, you could also rebase the bugfix_qvalidlineedit branch / PR and then rebase this PR on top of it.

promag commented at 9:45 am on January 25, 2022: contributor

Concept ACK, gave it a try and I prefer this over #536. I will review it shortly. @meshcollider maybe we could throttle validation to make “brute force” tedious? - if that’s really a concern.

in src/qt/qvalidatedlineedit.cpp:146 in 814182c463 outdated

143+            } else {
144+                int pos = 0;
145+                validation_result = checkValidator->validate(address, pos);
146+                error_locations.push_back(pos);
147+            }
148+            if (validation_result == QValidator::Acceptable)

Sjors commented at 10:30 am on January 25, 2022:

nit: add { to if and else statement

in src/qt/qvalidatedlineedit.cpp:149 in 814182c463 outdated

147+            }
148+            if (validation_result == QValidator::Acceptable)
149                 setValid(true, has_warning);
150             else
151-                setValid(false);
152+                setValid(false, false, error_locations);

Sjors commented at 10:32 am on January 25, 2022:

nit: add comments for what these booleans mean

in src/qt/qvalidatedlineedit.cpp:136 in 814182c463 outdated

132@@ -109,11 +133,20 @@ void QValidatedLineEdit::checkValidity()
133         if (checkValidator)
134         {
135             QString address = text();
136-            int pos = 0;
137-            if (checkValidator->validate(address, pos) == QValidator::Acceptable)
138+            const BitcoinAddressEntryValidator * const addressValidator = dynamic_cast<const BitcoinAddressEntryValidator*>(checkValidator);

Sjors commented at 10:35 am on January 25, 2022:

nit: maybe do this cast directly above the if (addressValidator) line

Sjors commented at 10:44 am on January 25, 2022: member

Did some light testing and it works nicely.

Some things we could improve later (also affects the RPC): if you replace any character after the first 1 with a 1 it will just mark the whole thing as invalid (a mistake I’ve made with l plenty of times). If you type a character not in the bech32(m) character set it marks the whole thing as invalid.

We could probably give some useful hints if the address starts with bc1q and has the wrong length. For bc1p we can warn if it’s too short (but not if it’s too long, because that might be valid in the future).

Update: see discussion in #536 (comment)

luke-jr commented at 7:33 pm on January 25, 2022: member

Instead of using a merge commit 7e96673, you could also rebase the bugfix_qvalidlineedit branch / PR and then rebase this PR on top of it.

Yes, the merge commit is just temporary - I assume #404 will get merged first, then this can be rebased without it.

RandyMcMillan commented at 7:02 am on January 30, 2022: contributor

@luke-jr - you could calculate the “percentage” that the address is incorrect (2 chars ~ 6%, 3 chars ~ 10%, etc..) without specifying which chars are incorrect - You could create a tool tip pop up with the % incorrect. This will prompt the user to scrutinize every character - reenforcing good practices. I agree with @meshcollider’s concern.

hebasto added the label UX on Feb 9, 2022

hebasto added the label Wallet on Feb 9, 2022

hebasto commented at 4:39 am on February 9, 2022: member

Based and depends on fixes in #404

#404 merged. Update this one?

DrahtBot added the label Needs rebase on Feb 9, 2022

luke-jr force-pushed on Feb 9, 2022

luke-jr commented at 7:55 am on February 9, 2022: member

Rebased

GUI: QValidatedLineEdit: Add support for a warning-but-valid state d85563efaf

GUI: Support returning positions from BitcoinAddress{Entry,Check}Validator::validate e0a0b288e0

GUI: Point out position of invalid characters in Bech32 addresses 539beeaae8

luke-jr force-pushed on Feb 9, 2022

DrahtBot removed the label Needs rebase on Feb 9, 2022

Rspigler commented at 10:20 pm on July 6, 2022: contributor

tACK

All tests pass

At first I thought this PR wasn’t working, but then I realized you have to click outside the ‘Pay To’ box, in order for the red highlighting to appear, and the specific ‘Yellow’ highlighting showing where the error is. Up to two errors are shown, if there are more, there is just the red highlighting.

I would suggest not requiring clicking outside the box.

hebasto requested review from achow101 on Jul 19, 2022

shaavan commented at 2:59 pm on July 19, 2022: contributor

Concept ACK

I agree with the idea of pointing out the positions of invalid characters.
This would help the user know where they made a mistake while writing the address and save them some time finding the exact position of the invalid character.
I prefer this implementation over #536. Though I can understand @meshcollider’s concern, I think hiding the feature behind a context menu won’t prevent this from happening but will make using this feature slightly inconvenient.
I verified that the PR works correctly on Ubuntu 22.04 with Qt version 5.15.3.
By trial and error, I verified that the pointing out works when two characters are incorrect at max. Though sometimes the feature doesn’t work even when only one character is wrong. I am not sure if this quirk could be avoided.

Screenshot: Screenshot from 2022-07-19 20-06-30

One thing I want to point out is that this PR introduces a warning while compiling the gui.

Warning message:

0In file included from qt/moc_bitcoinaddressvalidator.cpp:10:
1./qt/bitcoinaddressvalidator.h:23:19: warning: ‘virtual QValidator::State BitcoinAddressEntryValidator::validate(QString&, int&) const’ was hidden [-Woverloaded-virtual]
2   23 |     virtual State validate(QString &input, int &pos) const override;
3      |                   ^~~~~~~~
4./qt/bitcoinaddressvalidator.h:35:11: note:   by ‘virtual QValidator::State BitcoinAddressCheckValidator::validate(QString&, std::vector<int>&) const’
5   35 |     State validate(QString &input, std::vector<int>&error_locations) const override;
6      |           ^~~~~~~~
7  CXX      qt/libbitcoinqt_a-moc_bitcoinamountfield.o

Suggestion:

There are a few suggestions I would like to suggest. These are as per my understanding of the PR. But in case my observations are incorrect, please correct me,
1. By my understanding, the use case of the checkValidity() function is not to check validity but to set it based on different circumstances. So I would suggest renaming this function to setValidity().
2. As far as I understood the use case of setValid(), this function should be made a protected member of the class, as a function outside the class shouldn’t be able to call it.

I am attaching my notes along with this comment in case this might help other reviewers better understand the PR. In case my observations are erroneous, please do correct me.

Notes:

The first commit allows checking for warnings and validating input.
1. This is done by adding a function to check for the presence of warnings using QValidator::validate and using its results to select the appropriate StyleSheet in the setValid() function.
The second commit adds a validate function that allows finding and storing positions of all the invalid characters.
1. This commits also make the class BitcoinAddressCheckValidator a child class of BitcoinAddressEntryValidator. This was done because they had the same parent class. This change eliminated redundancy in redefining the variant of the validate function.
The third commit adds the code to find and colorize the invalid characters. This is done by:
1. First, find the location of all the invalid characters in the checkValidity function and store them in an error_locations vector.
2. Second, use the values in the error_locations to find and appropriately style the incorrect characters in the setValid() function.

achow101 commented at 3:31 pm on July 19, 2022: member

I agree with @meshcollider, I don’t think we should show the user the incorrect characters in the main UI, especially without any warnings about the probability that the error location is correct.

Rspigler commented at 3:19 am on July 20, 2022: contributor

@achow101 What is the probability that the error location is correct? I agree with the idea of a warning

sipa commented at 12:02 pm on July 22, 2022: contributor

@Rspigler If there are up to 2 substitution errors, it is 100% accurate. If there are more or other errors, it is certainly going to be wrong (if it locates at all). You cannot compute probabilities without having a model of what errors users are going to make.

If the user just types uniformly random garbage (a very poor approximation for reality), then:

With chance roughly 1 in a billion, the address will just be accepted.
With chance roughly 1 in 652, (incorrect) error locations will be shown.
In all other cases, the address just won’t be accepted, and no error locations will be shown.

Rspigler commented at 3:59 pm on July 22, 2022: contributor

Then I don’t see the issue with pointing out the position of two errors, and just highlighting the address if there are any more/different errors (such as done in #533)

sipa commented at 4:02 pm on July 22, 2022: contributor

@Rspigler I don’t see what these statistics have to do with that. If a user types an incorrect address, and it is more than 2 substitution errors, or a different kind of error (insertions/erasure e.g.), then if error locations are shown, they are certainly incorrect. I agree with the earlier comments that without precautions there is a risk the user will just go grind these positions, which would almost certainly cause loss of funds.

and just highlighting the address if there are any more/different errors (such as done in #533)

There is no way of knowing how many errors were actually made.

Rspigler commented at 4:34 pm on July 22, 2022: contributor

Maybe I asked the wrong question. For the set of all possible 2 substitution errors, the algorithm is 100% correct in pointing out invalid characters. How often are there 3 or more substitution errors, that the algorithm thinks is 2 or less?

Insertion/erasure errors can be solved by checking the length of the address.

Concerns re: grinding can be fixed through the warning message.

sipa commented at 5:11 pm on July 22, 2022: contributor

How often are there 3 or more substitution errors, that the algorithm thinks is 2 or less?

Depends on the model of actual errors, but likely close to 1 in 652 for P2WSH/P2TR addresses. It drops to 1 in 285 for longer ones.

Insertion/erasure errors can be solved by checking the length of the address.

Changing an address’ length does not necessarily invalidate it, and you can have combinations of insertion and erasure too.

Concerns re: grinding can be fixed through the warning message.

Possibly, yes.

luke-jr commented at 6:11 pm on July 22, 2022: member

IMO it’s far more likely users will enter an incorrect but valid address on the first try, than to grind a single character error.

Bosch-0 commented at 1:53 am on July 29, 2022: none

This may cause users that don’t understand the error correction to try and brute-force the incorrect characters (which they can do easily with this approach), leading to loss of funds.

I prefer this approach over #536. Regarding the issue stated above, why not make it so users can only paste and clear this field and not allow character edits at all? Would avoid users being able to attempt brute forcing in the first place and in general would be good as manually typing an address in is bound to lead to error.

I don’t see much user benefit highlighting what parts of the address are invalid. Keeping it simple and binary is the better UX. Red if incorrect, without highlighting errors, green if correct (this is also a state that should be shown as it takes anxiety off users inputting invalid addresses). If you do want to highlight what parts are incorrect then this should be hidden from the primary view and be possible to be viewed in a manner similar to #536 in a modal of some sort.

We have some content on this in the Bitcoin Design Guide: https://bitcoin.design/guide/glossary/address/#address-validation

sipa commented at 3:08 pm on July 29, 2022: contributor

why not make it so users can only paste and clear this field and not allow character edits at all?

But there should still be a way to enter the address character by character, I think? It may be visually copied from another device, or dictated over voice or so. How do you permit entering, while disallowing editing?

luke-jr commented at 3:40 pm on July 29, 2022: member

If you allow manual entry, then it makes sense to allow editing as well. It’s much easier to ask someone to confirm the “middle” or “end” than to repeat the whole thing. Or to look closer at a specific character on your other device/paper.

shaavan commented at 11:33 am on July 30, 2022: contributor

I was wondering if we could take a middle ground.

We could have a toggle button saying something like “allow manual edits”. Which, by default, will be off and would allow only copy/paste. When switched, it can display the risk associated with entering the wrong address and ask for user confirmation if they want to toggle it.

How do you permit entering while disallowing editing?

I am currently looking into ways to do so using Qt Widgets to ensure the feasibility of the above idea.

Rspigler commented at 5:32 pm on July 30, 2022: contributor

I’m a soft NACK on the toggle, I feel like we have to allow manual edits, that’d be a big UX change. Soft NACK because it could be a good security improvement, that when off, it automatically points out positions of invalid characters.

I’m more favorable towards just doing #536

Sjors commented at 8:07 am on August 5, 2022: member

What if we highlight 6 adjacent characters with a random offset?

jarolrod commented at 10:55 pm on August 10, 2022: member

I’d tend towards NACK here, this is too overwhelming for a user. I don’t think we need to do this at all, just warn that the address is invalid.

If they’re running into an issue with invalid characters, i don’t see how this helps. Are they supposed to replace the wrong characters with the correct ones? That would not be advisable.

luke-jr commented at 11:19 pm on August 10, 2022: member

They can look at the writing they are transcribing, or perhaps call the recipient up on the phone to clarify the specific part that looks wrong.

rebroad commented at 7:24 am on September 3, 2022: contributor

If the user just types uniformly random garbage (a very poor approximation for reality), then:

With chance roughly 1 in a billion, the address will just be accepted.

With chance roughly 1 in 652, (incorrect) error locations will be shown.

In all other cases, the address just won’t be accepted, and no error locations will be shown.

How is this scenario relevant? If a user types in random characters, then they are surely wanting to send funds to nowhere - we shouldn’t make that difficult for them if this is their wish.

Anyway, concept ACK - seems like a useful feature, and makes it easier for users to quickly see where an address is mistyped, without risking loss of funds (unless that is what the user wants).

DrahtBot commented at 12:27 pm on January 17, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Jan 17, 2023

DrahtBot commented at 0:26 am on April 17, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

hebasto commented at 4:27 pm on April 17, 2023: member

Closing this due to lack of activity. Feel free to reopen.

hebasto closed this on Apr 17, 2023

BenWestgate commented at 5:51 am on September 9, 2023: contributor

What if we highlight 6 adjacent characters with a random offset?

Highlighting adjacent characters is the best approach and should not be overlooked.

We ran into similar issues over at https://github.com/BlockstreamResearch/codex32/pull/58 and have some solutions that may apply here.

• Visual Separation: As @Bosch-0 shared, visually spacing or separating each group of 4 characters in bech32 strings greatly enhances readability. Note that the spacing is for legibility, not part of the string itself.

• Error Highlighting: We currently highlight error locations by turning the group(s) of 4 characters that may contain an error red.

• Abuse Safe: Group error highlighting can’t be abused by manual brute force due to 2^20 candidate corrections.

• Simple Corrections: Easy-to-remember chunks. For instance, in a wpkh address with 11 groups, one could attempt a correction by saying ‘repeat group 8’ and be quickly understood by another party.

• Allows Highlighting Deletions: How do you highlight something that’s not there? Highlight the group where a suspected missing character belongs.

• How to Safely Highlight Insertions: Character insertion error locations are dangerous to highlight because the abusive edit to “manually brute force” a valid string is one key stroke (with no detection guarantees). In fact, insertion errors MUST NEVER be identified as such, otherwise there aren’t enough candidates to display them safely at all. Error type ambiguity protects the user.

• Error Detection Warning: When a string with an invalid checksum is provided we MUST ask the user to check their source matches entirely their input before accepting the repairs.

• Known Length Error Detection: For known (nearby) string length, like bech32, up to 3 insert and/or 2 delete errors can be checked to give error location suggestions.

• Unknown Length Correction: For bech32m with many valid lengths, at most half the above (1 insert, 1 delete) can be checked in candidates. Attempts to submit an unusual length need a scary warning. Especially after repairing length errors.

• Error Detection Warning: When a string with an invalid checksum is provided we MUST ask the user to check their source matches entirely their input before accepting their corrections.

the user must always refer to the original source to determine the type of error to attempt a correction for an incorrect group or groups.

I tested a Gtk dialog that highlights red groups of 4 containing substitutions, insertions, and deletions. And it is far more user-friendly to type fast from handwriting than no error highlighting or specific character highlighting.

Single character highlighting is harder for the eye to locate in the source, groups are not. Especially if written down or printed in the same grouped structure.

What looks at a glance like a compromise, ends up better for almost everything.

hebasto referenced this in commit 7b39702513 on Feb 7, 2024

bitcoin-core locked this on Sep 8, 2024

Point out position of invalid characters in Bech32 addresses #537

Reviews

Conflicts