Add bitcoin address label to request payment QR code #7636

makevoid · 2016-03-03T11:11:25Z

This is a change for the QT wallet

Why this change?
This way the user can be sure that the QR code scanner app he/she's using is not displaying a different address to maliciously steal the funds.

I've seen an android app in the wild that is a simple qr code scanner but when it scans a bitcoin address it replaces it with a different address belonging to the hacker. I'm starting to get worried of bitcoin address qr codes without a label that states the address in clear so that the user can always check that he/she is depositing the funds to the desired address.
That's why I decided to modify the bitcoin qt code.

Code changes:

renamed myImage to qrImage
added qrAddrImage in which the bitcoin address label is painted onto
scaled the saved pixmap with antialiasing to improve readability of the text and the scanning of the qrcode

makevoid · 2016-03-03T11:27:05Z

That's the preview of the change, the qr image has the bitcoin address label underneath so that when it's saved you can match it:

laanwj · 2016-03-03T11:36:41Z

Haven't looked at the code, but screenshot looks good to me, nice work.

jonasschnelli · 2016-03-03T11:42:30Z

Thanks!

Having a malicious QRCode can probably only happen if you system, Bitcoin-Core or at least libqrencode is compromise.

Your PR would only protect from an attack on libqrencode and only if the user uses a self-compiled Bitcoin-Core (official binaries are static linked). Because, if you could tamper Bitcoin-Core, you could change the address at a core point which would also provide a malicious address below the QRCode.

This change would def. make sense if we would have HD for key derivation.
You could use a smartphone with the same xpub master key and verify the address at given keypath by comparing QRCodes or addresses together with the keypath.

IMO this PR does not hurt, although I don't believe that it increases security.

UtACK.

laanwj · 2016-03-03T11:44:36Z

IMO this PR does not hurt, although I don't believe that is increases security.

If anything at the printer side is compromised, you're out of luck. But what about a malicious QR code reader? I think it could help there, comparing the address before accepting the transaction in your wallet.

maflcko · 2016-03-03T11:47:03Z

Right, I think it makes sense to have a human readable version of the QR code besides it. Concept ACK.

jonasschnelli · 2016-03-03T11:49:45Z

Yes. I just thought people could already do this because they have the address below the QRCode already. But this PR makes it more prominent available.

paveljanik · 2016-03-03T12:47:21Z

Testing on OS X. The image is blurred. Original image is OK:

Current master:

This PR:

The Request payment window shows only the top part of the address:

Can you fix these problems?

jonasschnelli · 2016-03-03T12:48:27Z

src/qt/receiverequestdialog.cpp

QRCodes should not use anti-aliasing (QT::SmoothTransformation). I guess this should be removed.

makevoid · 2016-03-03T13:42:54Z

I removed the anti-aliasing (I should I've imagined that it was bad for qr code scanning) and now it shouldn't have that blurring problem on OSX as it uses the same size as the size it's using to display it (300), anyway I will double-check it in few hours on that OS.

The only thing that's changed now is the export size, the final image is 44 pixel larger for each dimension. The good thing that is now I can reuse the constant EXPORT_IMAGE_SIZE (now changed to QR_IMAGE_SIZE, used only in this file) for the dimensions, also there's no need to re-scale the image again and again anymore.

I can squash the three commits to a single one and force push to this branch when needed.

paveljanik · 2016-03-03T13:57:36Z

One new warning generated:

+qt/receiverequestdialog.cpp:184:73: warning: implicit conversion from 'double' to 'int' changes value from 8.5 to 8 [-Wliteral-conversion]
+            painter.setFont(QFont(GUIUtil::fixedPitchFont().toString(), 8.5));
+                            ~~~~~                                       ^~~
+1 warning generated.

luke-jr · 2016-03-03T13:59:38Z

So now the window displays the address twice? Maybe just add it to the image when it's being saved...?

paveljanik · 2016-03-03T14:02:40Z

@luke-jr If displayed properly, the address is displayed four times! Title bar, image, bitcoin: URL and address. 8)

jonasschnelli · 2016-03-03T16:07:46Z

Hmm... font feels a bit small on my end (HiDPI/retina OSX):

jonasschnelli · 2016-03-03T16:10:54Z

src/qt/receiverequestdialog.cpp

Maybe add a QFont::setPointSize() here to avoid HiDPI differences?

laanwj · 2016-03-03T16:40:28Z

So now the window displays the address twice? Maybe just add it to the image when it's being saved...?

Well at least people can't complain they're missing the address :-)
I don't think this is much of a concern, it's not like the window is short on space.

laanwj · 2016-03-03T16:41:26Z

One thing I do wonder about though: do QR scanners need an empty (white) border around a QR code? If so, how large should it be?

jonasschnelli · 2016-03-03T16:45:26Z

@laanwj: I recently played around with max QR size/bytes, black/white (inverted) and the white-space-border. I could not find any specification about the "quite zone" (white border), but I guess "one square width" as min border size should be more as sufficient. I guess it depends on the QRCode reader implementation. But I could not read the QRCode with common readers if there where no white borders.

sipa · 2016-03-03T16:46:06Z

Scanners don't need a border I believe, but printers may.

sipa · 2016-03-03T16:52:25Z

Seems I misremembered. Some googling shows that the spec actually includes a quiet zone around the QR code, 4 modules wide, with the same color as the white modules.

paveljanik · 2016-03-10T12:52:06Z

Current status:

jonasschnelli · 2016-03-13T09:25:25Z

Still not really readable on retina/HiDPI OSX:

jonasschnelli · 2016-03-13T09:31:14Z

src/qt/receiverequestdialog.cpp

Maybe some clever rule to detect the width of the text we want to draw – and make it bigger if there is more horizontal space?

Get width:

QFontMetrics fm(painter.font()); int pixelsWide = fm.width(info.address);

jonasschnelli · 2016-04-05T07:31:44Z

ping @makevoid: I think this PR is almost ready. We just need a way that the font size respects the screen density (HiDPI).

jonasschnelli · 2016-06-02T09:12:48Z

ping @makevoid

laanwj · 2016-06-13T09:29:39Z

We just need a way that the font size respects the screen density (HiDPI).

I'd say no because this is exported to an image file. We don't want the output to be dependent on the user's screen resolution.

jonasschnelli · 2016-06-13T09:33:07Z

@laanwj: check #7636 (comment)
The font size on HiDPI is different then on non-HiDPI.

I can take care about this and finish this PR.

makevoid · 2016-06-13T09:35:04Z

Sorry but I don't have an HiDPI screen to test with yet, is there anything I can do? should I squash the commits?

laanwj · 2016-06-13T09:40:16Z

The font size on HiDPI is different then on non-HiDPI.

Just a matter of misunderstanding / wrong formulation then - what we want here is to use a font size that is fixed in pixel size, just like the QR code itself is.

jonasschnelli · 2016-06-13T13:43:52Z

The QRCode is not rendered HiDPI (not required). Setting the font size in pixel works (because the size of the QImage is fixed in pixels not in points).
The only little downside is the missing HiDPI support for the new address (looks a bit pixled here).

Here's the diff.
@makevoid: Can you apply and squash?

diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp
index 1e104bb..b81a039 100644
--- a/src/qt/receiverequestdialog.cpp
+++ b/src/qt/receiverequestdialog.cpp
@@ -183,7 +183,9 @@ void ReceiveRequestDialog::update()
             qrAddrImage.fill(0xffffff);
             QPainter painter(&qrAddrImage);
             painter.drawImage(0, 0, qrImage.scaled(QR_IMAGE_SIZE, QR_IMAGE_SIZE));
-            painter.setFont(QFont(GUIUtil::fixedPitchFont().toString(), 8));
+            QFont font = QFont(GUIUtil::fixedPitchFont().toString());
+            font.setPixelSize(12);
+            painter.setFont(font);
             QRect paddedRect = qrAddrImage.rect();
             paddedRect.setHeight(QR_IMAGE_SIZE+12);
             painter.drawText(paddedRect, Qt::AlignBottom|Qt::AlignCenter, info.address);

maflcko · 2016-06-13T13:57:59Z

+ QFont font = QFont(GUIUtil::fixedPitchFont().toString());

fixedPitchFont() already returns QFont. Is this odd constructor necessary?

jonasschnelli · 2016-06-13T14:03:58Z

@MarcoFalke: Oh yes.
@makevoid: please use QFont font = GUIUtil::fixedPitchFont();.

@jonasschnelli

In the Receive 'Tab' of the QT wallet, when 'Show'ing a previously requested payment, add a label underneath the QR Code showing the bitcoin address where the funds will go to. This way the user can be sure that the QR code scanner app the user using is reading the correct bitcoin address, preventing funds to be stolen. Includes fix for HiDPI screens by @jonasschnelli.

makevoid · 2016-06-13T15:12:54Z

@jonasschnelli done: 1c2a1ba#diff-23ecec8032dd36794001112346601b6aR186

maflcko · 2016-06-13T15:51:56Z

utACK 1c2a1ba

jonasschnelli · 2016-06-14T11:13:18Z

Tested ACK 1c2a1ba.

Looks also good on Win(10).
HiDPI issues on windows are unrelated to this PR (we need Qt5.6.1).

1c2a1ba Add address label to request payment QR Code (QT) (Francesco 'makevoid' Canessa)

laanwj added the GUI label Mar 3, 2016

jonasschnelli reviewed Mar 3, 2016
View reviewed changes

src/qt/receiverequestdialog.cpp Outdated

Copy link

Contributor

jonasschnelli Mar 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a QFont::setPointSize() here to avoid HiDPI differences?

jonasschnelli reviewed Mar 13, 2016
View reviewed changes

makevoid force-pushed the request_payment_qrcode_address_label branch from 54a1eff to 1c2a1ba Compare June 13, 2016 15:11

jonasschnelli merged commit 1c2a1ba into bitcoin:master Jun 14, 2016

jonasschnelli added a commit that referenced this pull request Jun 14, 2016

Merge #7636: Add bitcoin address label to request payment QR code

fb0ac48

1c2a1ba Add address label to request payment QR Code (QT) (Francesco 'makevoid' Canessa)

codablock pushed a commit to codablock/dash that referenced this pull request Dec 28, 2017

Merge bitcoin#7636: Add bitcoin address label to request payment QR code

96180a1

1c2a1ba Add address label to request payment QR Code (QT) (Francesco 'makevoid' Canessa)

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Merge bitcoin#7636: Add bitcoin address label to request payment QR code

d91f3d3

1c2a1ba Add address label to request payment QR Code (QT) (Francesco 'makevoid' Canessa)

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

makevoid deleted the request_payment_qrcode_address_label branch June 9, 2024 18:53

Add bitcoin address label to request payment QR code #7636

Add bitcoin address label to request payment QR code #7636

Uh oh!

Conversation

makevoid commented Mar 3, 2016

Uh oh!

makevoid commented Mar 3, 2016

Uh oh!

laanwj commented Mar 3, 2016

Uh oh!

jonasschnelli commented Mar 3, 2016

Uh oh!

laanwj commented Mar 3, 2016

Uh oh!

maflcko commented Mar 3, 2016

Uh oh!

jonasschnelli commented Mar 3, 2016

Uh oh!

paveljanik commented Mar 3, 2016

Uh oh!

jonasschnelli Mar 3, 2016

Choose a reason for hiding this comment

Uh oh!

makevoid commented Mar 3, 2016

Uh oh!

paveljanik commented Mar 3, 2016

Uh oh!

luke-jr commented Mar 3, 2016

Uh oh!

paveljanik commented Mar 3, 2016

Uh oh!

jonasschnelli commented Mar 3, 2016

Uh oh!

jonasschnelli Mar 3, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 3, 2016

Uh oh!

laanwj commented Mar 3, 2016

Uh oh!

jonasschnelli commented Mar 3, 2016

Uh oh!

sipa commented Mar 3, 2016 via email

Uh oh!

sipa commented Mar 3, 2016 via email

Uh oh!

paveljanik commented Mar 10, 2016

Uh oh!

jonasschnelli commented Mar 13, 2016

Uh oh!

jonasschnelli Mar 13, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Apr 5, 2016

Uh oh!

jonasschnelli commented Jun 2, 2016

Uh oh!

laanwj commented Jun 13, 2016

Uh oh!

jonasschnelli commented Jun 13, 2016

Uh oh!

makevoid commented Jun 13, 2016

Uh oh!

laanwj commented Jun 13, 2016

Uh oh!

jonasschnelli commented Jun 13, 2016

Uh oh!

maflcko commented Jun 13, 2016

Uh oh!

jonasschnelli commented Jun 13, 2016

Uh oh!

makevoid commented Jun 13, 2016

Uh oh!

maflcko commented Jun 13, 2016

Uh oh!

jonasschnelli commented Jun 14, 2016

Uh oh!

Reviewers

Assignees

Labels