Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better feedback when unprintable characters in Bitcoin address #1325

Closed
sgornick opened this issue May 16, 2012 · 9 comments
Closed

Better feedback when unprintable characters in Bitcoin address #1325

sgornick opened this issue May 16, 2012 · 9 comments

Comments

@sgornick
Copy link

There are some problems with people copying and pasting an address where the address includes an unprintable character.

For example, some web apps will inject an #8203; into a long word so that a line-break / word wrap will occur. But the user doing a copy and paste won't see this character.

When trying to paste this into the PayTo: field of v0.6.2, the paste does not succeed. There is no feedback given as to why this paste does not occur.

In previous versions (perhaps v0.5.x/) apparently the paste will success but the validation fails on send (highlights the field in red). Still though there is no further feedback as to what the problem is.

The user then feels the problem is with Bitcoin-Qt as that same copy and paste on an EWallet (e.g., on Mt. Gox website) works fine (presumably because they strip out nonprintable characters from an address?)

Here's an example of users reporting this issue:

Here's what the "view source" for the web page shows:
1G5apmPvo2iTtmkNWAHTCET7​Y842Ufijs8

@laanwj
Copy link
Member

laanwj commented May 16, 2012

Sneaky. But I think this would be pretty simple to solve; in the correction phase of BitcoinAddressValidator::validate, delete these characters from the string.

@gmaxwell
Copy link
Contributor

I don't generally like the idea of the validator "fixing" broken addresses, because it's perfectly possible for one character variations to also be valid. ... but the case of random entities stuffed in, that seems pretty safe to fix to me. Perhaps we should decode any entities, then strip only whitespace/linebreaks?

@laanwj
Copy link
Member

laanwj commented May 16, 2012

I don't like the idea of decoding entities at all.

From what I understood it is not about entities stuffed in, but actual unicode characters. Those show up in html source as entities, but appear as normal utf-8 characters when copy/pasted into a plain text field. Those QChar(8203) are easy and safe to filter out, along with other whitespace/linebreaks.

@gmaxwell
Copy link
Contributor

Okay, to be clear— I think stripping whitespace is okay. I don't think stripping or replacing other characters is okay. To the extent that whitespace might come in the form of entities I'd be okay with stripping that to. I didn't mean to advocate decoding entities if not needed.

@laanwj
Copy link
Member

laanwj commented May 16, 2012

We already replace other characters: l and I are replaced by 1, 0 and O by o. This is just a convenience for the user and does not give any confusion in base58.

Edit: but I agree that, beyond that, we should not try to 'fix' the addresses.
Edit.2: I also think you mean another validator, this is the one used for input fields in bitcoin-qt, not the internal validator used in the core. The user is able to visually inspect the result before submitting.

laanwj added a commit to laanwj/bitcoin that referenced this issue May 17, 2012
- Fixes issues with copy/pasting from web or html emails (bitcoin#1325)
@laanwj
Copy link
Member

laanwj commented May 17, 2012

See #1329

@gmaxwell
Copy link
Contributor

I must have missed that change going in, as I specifically remembered NAKing #552 . I continue to think that the character substitution is a bad idea (in a way that whitespace removal isn't).

@laanwj
Copy link
Member

laanwj commented May 17, 2012

It was already in there from the beginning of bitcoin-qt. I think it is harmless, at most. Users typing in base58 addresses from paper, for example, should not have to care if something is a 0, o or O as they look visually the same and AFAIK that was the intended idea of base58.

laanwj added a commit to laanwj/bitcoin that referenced this issue May 21, 2012
- Fixes issues with copy/pasting from web or html emails (bitcoin#1325)
@laanwj
Copy link
Member

laanwj commented May 21, 2012

Fixed

@laanwj laanwj closed this as completed May 21, 2012
luke-jr pushed a commit to luke-jr/bitcoin that referenced this issue Jun 17, 2012
- Fixes issues with copy/pasting from web or html emails (bitcoin#1325)
coblee pushed a commit to litecoin-project/litecoin that referenced this issue Jul 17, 2012
- Fixes issues with copy/pasting from web or html emails (bitcoin#1325)
suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this issue Dec 5, 2017
* no wallet passphrase in masternode(broadcast) and darksend rpc

* spork values are int64
lateminer pushed a commit to lateminer/bitcoin that referenced this issue Jan 22, 2019
Cleanup and simplify the initialization of the parallel block validator.
lateminer pushed a commit to lateminer/bitcoin that referenced this issue May 6, 2020
411ade5 Add WITH_LOCK macro: run code while locking a mutex. (furszy)

Pull request description:

  Pretty useful functionality. Pushing it on its own separated PR to be able to use it in several open PRs.

  Coming from btc@edfe943.

ACKs for top commit:
  random-zebra:
    Nice little trick. utACK 411ade5
  Fuzzbawls:
    utACK 411ade5

Tree-SHA512: c1d07a5adc82906e8a8c3e486bf35ce13ca50957e8e2ca201b80993568e39bf78a32d44cc7ba1bb431d79c8163b96587250fc97a58bef4b6926eb5f137d57d2c
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants