Add bitcoin address label to request payment QR code #7636

pull makevoid wants to merge 1 commits into bitcoin:master from makevoid:request_payment_qrcode_address_label changing 4 files +20 −7
  1. makevoid commented at 11:11 AM on March 3, 2016: contributor

    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
  2. makevoid commented at 11:27 AM on March 3, 2016: contributor

    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:

  3. laanwj commented at 11:36 AM on March 3, 2016: member

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

  4. laanwj added the label GUI on Mar 3, 2016
  5. jonasschnelli commented at 11:42 AM on March 3, 2016: contributor

    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.

  6. laanwj commented at 11:44 AM on March 3, 2016: member

    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.

  7. maflcko commented at 11:47 AM on March 3, 2016: member

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

  8. jonasschnelli commented at 11:49 AM on March 3, 2016: contributor

    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.

  9. paveljanik commented at 12:47 PM on March 3, 2016: contributor

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

    Current master: before

    This PR: after

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

    <img width="493" alt="screen shot 2016-03-03 at 13 43 03" src="https://cloud.githubusercontent.com/assets/6848764/13494723/512705d2-e146-11e5-9f9d-9cb438736507.png">

    Can you fix these problems?

  10. in src/qt/receiverequestdialog.cpp:None in a1b9b7e71e outdated 
      44 | @@ -45,7 +45,7 @@ QImage QRImageWidget::exportImage()
  45 |  {
  46 |      if(!pixmap())
  47 |          return QImage();
  48 | -    return pixmap()->toImage().scaled(EXPORT_IMAGE_SIZE, EXPORT_IMAGE_SIZE);
  49 | +    return pixmap()->toImage().scaled(EXPORT_IMAGE_SIZE, EXPORT_IMAGE_SIZE, Qt::KeepAspectRatio, Qt::SmoothTransformation);
    jonasschnelli commented at 12:48 PM on March 3, 2016:

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

  11. makevoid commented at 1:42 PM on March 3, 2016: contributor

    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.

  12. paveljanik commented at 1:57 PM on March 3, 2016: contributor

    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.
  13. luke-jr commented at 1:59 PM on March 3, 2016: member

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

  14. paveljanik commented at 2:02 PM on March 3, 2016: contributor

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

  15. jonasschnelli commented at 4:07 PM on March 3, 2016: contributor

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

    <img width="599" alt="bildschirmfoto 2016-03-03 um 17 06 04" src="https://cloud.githubusercontent.com/assets/178464/13500300/44c1a77c-e162-11e5-94eb-13a8cd99b465.png">

  16. in src/qt/receiverequestdialog.cpp:None in 77a94c38cf outdated 
     183 |              QRcode_free(code);
 184 |  
 185 | -            ui->lblQRCode->setPixmap(QPixmap::fromImage(myImage).scaled(300, 300));
 186 | +            QImage qrAddrImage = qrImage.scaled(QR_IMAGE_SIZE, QR_IMAGE_SIZE);
 187 | +            QPainter painter(&qrAddrImage);
 188 | +            painter.setFont(QFont(GUIUtil::fixedPitchFont().toString(), 8));
    jonasschnelli commented at 4:10 PM on March 3, 2016:

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

  17. laanwj commented at 4:40 PM on March 3, 2016: member

    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.

  18. laanwj commented at 4:41 PM on March 3, 2016: member

    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?

  19. jonasschnelli commented at 4:45 PM on March 3, 2016: contributor

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

  20. sipa commented at 4:46 PM on March 3, 2016: member

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

  21. sipa commented at 4:52 PM on March 3, 2016: member

    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.

  22. paveljanik commented at 12:52 PM on March 10, 2016: contributor

    Current status:

    <img width="479" alt="screen shot 2016-03-10 at 13 50 18" src="https://cloud.githubusercontent.com/assets/6848764/13669936/1b7fbb1e-e6c7-11e5-9447-73cd380ddcbb.png">

  23. jonasschnelli commented at 9:25 AM on March 13, 2016: contributor

    Still not really readable on retina/HiDPI OSX: <img width="663" alt="bildschirmfoto 2016-03-13 um 10 24 24" src="https://cloud.githubusercontent.com/assets/178464/13727910/d85aee4e-e905-11e5-8cf3-6a57df683fb8.png">

  24. in src/qt/receiverequestdialog.cpp:None in 54a1effc54 outdated 
     188 | +            QPainter painter(&qrAddrImage);
 189 | +            painter.drawImage(0, 0, qrImage.scaled(QR_IMAGE_SIZE, QR_IMAGE_SIZE));
 190 | +            painter.setFont(QFont(GUIUtil::fixedPitchFont().toString(), 8));
 191 | +            QRect paddedRect = qrAddrImage.rect();
 192 | +            paddedRect.setHeight(QR_IMAGE_SIZE+12);
 193 | +            painter.drawText(paddedRect, Qt::AlignBottom|Qt::AlignCenter, info.address);
    jonasschnelli commented at 9:31 AM on March 13, 2016:

    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);
  25. jonasschnelli commented at 7:31 AM on April 5, 2016: contributor

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

  26. jonasschnelli commented at 9:12 AM on June 2, 2016: contributor

    ping @makevoid

  27. laanwj commented at 9:29 AM on June 13, 2016: member

    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.

  28. jonasschnelli commented at 9:33 AM on June 13, 2016: contributor

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

  29. makevoid commented at 9:35 AM on June 13, 2016: contributor

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

  30. laanwj commented at 9:40 AM on June 13, 2016: member

    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.

  31. jonasschnelli commented at 1:43 PM on June 13, 2016: contributor

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

    <img width="599" alt="bildschirmfoto 2016-06-13 um 15 41 25" src="https://cloud.githubusercontent.com/assets/178464/16009555/859230ee-317d-11e6-9748-e989b89346cf.png"> <img width="518" alt="bildschirmfoto 2016-06-13 um 15 40 57" src="https://cloud.githubusercontent.com/assets/178464/16009556/85b892fc-317d-11e6-8f43-14650d753ba7.png">

    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);
  32. maflcko commented at 1:57 PM on June 13, 2016: member

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

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

  33. jonasschnelli commented at 2:03 PM on June 13, 2016: contributor

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

  34. Add address label to request payment QR Code (QT)
    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.
    1c2a1bac0a
  35. makevoid force-pushed on Jun 13, 2016
  37. maflcko commented at 3:51 PM on June 13, 2016: member

    utACK 1c2a1ba

  38. jonasschnelli commented at 11:13 AM on June 14, 2016: contributor

    Tested ACK 1c2a1bac0ad6901868c1a4003c7cbddcdf46a29b.

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

    <img width="436" alt="bildschirmfoto 2016-06-14 um 13 03 32" src="https://cloud.githubusercontent.com/assets/178464/16040721/b1c0b92e-3231-11e6-97de-9755a20440f5.png">

  39. jonasschnelli merged this on Jun 14, 2016
  40. jonasschnelli closed this on Jun 14, 2016
  41. jonasschnelli referenced this in commit fb0ac482ee on Jun 14, 2016
  42. codablock referenced this in commit 0f5c2de5d3 on Sep 16, 2017
  43. codablock referenced this in commit f0f1dd40d5 on Sep 19, 2017
  44. codablock referenced this in commit e1481879c9 on Dec 27, 2017
  45. codablock referenced this in commit 96180a1896 on Dec 28, 2017
  46. andvgal referenced this in commit d91f3d3bed on Jan 6, 2019
  47. bitcoin locked this on Sep 8, 2021
  48. makevoid deleted the branch on Jun 9, 2024
Contributors
makevoidlaanwjjonasschnellimaflckopaveljanikluke-jrsipa
Labels
GUI
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me