gui

gui copied to clipboard

Implements #527

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.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #553 (Change address / amount error background by w0xlt)
• #536 (Add address error location to GUI by meshcollider)

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.

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.

Concept ACK, but consider some sort of warning / restriction when there are more than 5(?) typos: https://github.com/bitcoin-core/gui/pull/536#issuecomment-1020947458

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.

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.

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.

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

Based and depends on fixes in #404

#404 merged. Update this one?

Rebased

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.

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.

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

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

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 https://github.com/bitcoin-core/gui/pull/533)

@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 https://github.com/bitcoin-core/gui/pull/533)

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

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.

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.

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.

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

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?

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.

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.

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

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

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.

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