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

qt: Add privacy to the Overview page #16432

Open
wants to merge 2 commits into
base: master
from

Conversation

@hebasto
Copy link
Member

hebasto commented Jul 22, 2019

This PR allows to hide/reveal values on the Overviewpage by checking/unchecking Menu->Settings-> Mask Values

Closes #16407

Privacy mode is OFF (the default behavior):
Screenshot from 2020-01-02 15-08-28

Privacy mode is ON:
Screenshot from 2020-01-02 15-10-23 cropped

@fanquake fanquake added the GUI label Jul 22, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jul 22, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)
  • #17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
  • #17659 (qt: Do not block GUI thread in RPCConsole by hebasto)
  • #16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
  • #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
  • #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)
  • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Jul 22, 2019

@hebasto Would you mind posting before/after screenshots?

@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Jul 22, 2019

@practicalswift

Would you mind posting before/after screenshots?

Done.

{
if (privacy && cache.isEmpty()) {
cache = text();
QLabel::setText("h-i-d-d-e-n");

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 22, 2019

Member

I think a fence of # translates better.

Couldn't this use BitcoinUnits::format(unit, 0).replace("0", "#") + unit.shortName() (or whatever is used right now), so that the UI doesn't reflow the columns on every toggle?

This comment has been minimized.

Copy link
@MarcoFalke

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 22, 2019

Author Member

I think a fence of # translates better.

Done.

... so that the UI doesn't reflow the columns on every toggle?

This requires a monospaced font.

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 22, 2019

Author Member

This requires a monospaced font.

Done.

@hebasto hebasto force-pushed the hebasto:20190721-privacy branch from 25bd216 to ea89169 Jul 22, 2019
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jul 22, 2019

Concept ACK, definitely prefer the # to h-i-d-d-e-n

@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Jul 22, 2019

@MarcoFalke

I think a fence of # translates better.

@laanwj

definitely prefer the # to h-i-d-d-e-n

The screenshot on OP has been updated.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Jul 22, 2019

Can it use a fixed number/string of #s instead of replacing each character with #? That way we don't leak any information about the number of coins. For example, right now, in your screenshot, we can see that you have at least 1000 coins and at least 100 available to spend.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jul 22, 2019

Thanks for working on this!
Tested a bit. Works.

Not sure if clicking on the actual amount label is the right way to activate this (may confuse user who lacks understanding the "hidden"-concept).

Plus, should it not also cover the "Recent transactions"-list amounts?

I would say ideally, once enabled, the values in the transaction list should also be "hidden".

@promag

This comment has been minimized.

Copy link
Member

promag commented Jul 22, 2019

Concept ACK. Not so sure about the approach as it requires too much wiring. I'm testing a different approach and I'll give feedback if it's worth it.

@promag

This comment has been minimized.

Copy link
Member

promag commented Jul 22, 2019

Not sure if clicking on the actual amount label is the right way to activate this

Agree with @jonasschnelli, I initially thought of having a toggle somewhere (in the status bar for instance) as well a menu option. Something for a follow up.

@hebasto hebasto force-pushed the hebasto:20190721-privacy branch from bc75b05 to a65a2be Jul 22, 2019
@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Jul 22, 2019

@achow101

Can it use a fixed number/string of #s instead of replacing each character with #? That way we don't leak any information about the number of coins. For example, right now, in your screenshot, we can see that you have at least 1000 coins and at least 100 available to spend.

Done. The screenshot on OP has been updated.

@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Jul 22, 2019

@jonasschnelli Thank you for your review.

Not sure if clicking on the actual amount label is the right way to activate this (may confuse user who lacks understanding the "hidden"-concept).

It seems very intuitive ;) (also there is an example of usage in Wasabi)
To further improvements (on the following PR), a toggle could be added (as @promag suggested).

Plus, should it not also cover the "Recent transactions"-list amounts?
I would say ideally, once enabled, the values in the transaction list should also be "hidden".

Agree. I'd leave this improvement for the following PRs.

@kristapsk

This comment has been minimized.

Copy link
Contributor

kristapsk commented Jul 22, 2019

Not sure if clicking on the actual amount label is the right way to activate this (may confuse user who lacks understanding the "hidden"-concept).

That's the way "lurking wife mode" of Wasabi Wallet, which is basically the same, is activated. I think Bitcoin is already hard enough for newcomers, so unnecessary reinventing the wheel should be avoided, if possible, so that users get similar experience using different wallets. Unless there are clear benefits in doing so, of course. But probably some message about privacy / stealth mode being activated and how to turn it back off would not hurt.

integer_part = QString(integer_part.size() - 1, ' ') + mask;
QString fractional_part = parts.last();
fractional_part = fractional_part.replace(QRegularExpression("\\d"), mask);
QLabel::setText(integer_part + '.' + fractional_part);

This comment has been minimized.

Copy link
@kristapsk

kristapsk Jul 22, 2019

Contributor

Just looked at how Wasabi did this and they just replace any amounts, addresses, labels everywhere with a fixed number of #'s. IMO "#########" even looks better than "#.########" and it's less lines of code, all this block could be just replaced with

cache = text();
QLabel::setText("#########");`

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 23, 2019

Author Member

Unfortunately, this will cause the UI to reflow the columns on every toggle (see @MarcoFalke's comment).

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jul 23, 2019

Concept ACK

@hebasto hebasto force-pushed the hebasto:20190721-privacy branch from a65a2be to 0241a18 Jul 23, 2019
@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Jul 23, 2019

Fixed two bugs:

  • sat units (no fractional part) handling
  • unit switching handling
@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jul 24, 2019

Had a quick test. Functionality works as described. I might just be seeing things, but the monospace font looks a bit weird no macOS, don't think that has to be any sort of blocker though.

@promag Are you still working on a different approach, or happy with this PR now?

master:
master

This PR:
16432-values
16432-hidden

@darosior

This comment has been minimized.

Copy link
Contributor

darosior commented Jul 24, 2019

ACK 22c16fd
I readed the code and I tested it. I think it's a great addition.

@promag

This comment has been minimized.

Copy link
Member

promag commented Jul 25, 2019

I've tried alternatives - basically tried filtering paint events if the widget had amounts or even extend QStyle - but didn't like at the end.

Currently I think this PR can be improved, I'll review shortly.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 12, 2019

Looks like the build is failing:

qt/bitcoinamountlabel.cpp:26:47: error: 'QRegularExpression' is an incomplete type
    fractional_part = fractional_part.replace(QRegularExpression("\\d"), mask);
                                              ^
/home/ubuntu/src/depends/x86_64-apple-darwin14/include/QtCore/qstring.h:80:7: note: forward declaration of 'QRegularExpression'
class QRegularExpression;

https://bitcoinbuilds.org/index.php?build=669

@hebasto hebasto force-pushed the hebasto:20190721-privacy branch from 0241a18 to 90c7f4f Aug 12, 2019
@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Aug 12, 2019

@jonasschnelli

Looks like the build is failing: ...

Rebased and fixed by partially reverting the 248e22b commit (#16386).

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 13, 2019

Works as intended. I like the concept but think it should be implemented more radical (maybe in follow up PRs?).

  • The privacy mode won't hide recent transaction values
  • For users not aware of the privacy mode, it's unclear why the numbers are now # and how to bring it back,... a quick disappearing info-element (statusbar?) would make things much more understandable for users new to this concept (or a toggle element in the icon section of the status bar?).
  • I think all amount values should be masqueraded, especially the ones in the transaction list.
Copy link
Member

luke-jr left a comment

Rather than cloaking the text we want to hide (which seems problematic so long as we base it on the real hidden value), how about simply setting the background colour to the text colour?

depends/packages/qt.mk Outdated Show resolved Hide resolved
@konez2k

This comment has been minimized.

Copy link

konez2k commented Aug 20, 2019

This should work without regexp

static QString cloak(QString& text)
{
    QStringList parts = text.split(' ');
    QStringList amounts = parts[0].split('.');
    for (int i = 0; i < amounts.size(); i++)
        amounts[i].fill('#');
    return amounts.join('.') + ' ' + parts[1];
}
@laanwj laanwj added the Feature label Sep 30, 2019
@hebasto hebasto force-pushed the hebasto:20190721-privacy branch from 3c384d7 to 32b8fed Jan 2, 2020
@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Jan 2, 2020

In the latest push:

  • code is much simplified
  • monospace font style continues to be used
  • a shortcut has been added to simplify testing and/or usage

Screenshot from 2020-01-02 15-08-28

Screenshot from 2020-01-02 15-10-23 cropped

Currently, the privacy mode is not inherited when a wallet is added/loaded.

@hebasto hebasto force-pushed the hebasto:20190721-privacy branch from 32b8fed to 3935bce Jan 2, 2020
@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Jan 2, 2020

The OP has been updated.

@achow101 @jonasschnelli @fanquake @laanwj @luke-jr @MarcoFalke @promag
Would you mind re-reviewing this PR?

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Jan 2, 2020

ACK 3935bce

Tested change and reviewed.

Not sure if this is possible, but it would be nice if the spaces for the right justify weren't selectable. Is there some other way to achieve the justify using spacers in the ui form?

@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Jan 3, 2020

@achow101

Not sure if this is possible, but it would be nice if the spaces for the right justify weren't selectable. Is there some other way to achieve the justify using spacers in the ui form?

It is possible, but it requires to overhaul a form layout in a separate PR.


void BitcoinAmountLabel::setValue(const CAmount& amount, int unit, bool privacy)
{
QString value = BitcoinUnits::format(unit, privacy ? 0 : amount, false, BitcoinUnits::separatorAlways, true);

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 3, 2020

Member

Please use formatWithUnit

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 4, 2020

Author Member

A new signature of BitcoinUnits::format() is used here.

{
}

void BitcoinAmountLabel::setValue(const CAmount& amount, int unit, bool privacy)

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 3, 2020

Member

I'm not sure there's a point to a dedicated widget anymore...

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 4, 2020

Author Member

Agree. Fixed in the latest push.

if (privacy) {
value.replace('0', '#');
}
QLabel::setText(value + QString(" ") + BitcoinUnits::shortName(unit));

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jan 3, 2020

Member

QLabel:: is unnecessary

@hebasto hebasto force-pushed the hebasto:20190721-privacy branch from 3935bce to ea1fb69 Jan 4, 2020
@@ -6,8 +6,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>596</width>
<height>342</height>
<width>798</width>

This comment has been minimized.

Copy link
@hebasto

hebasto Jan 4, 2020

Author Member

Note to reviewers: these changes in numbers are just Qt Designer artefacts.

@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Jan 4, 2020

As @luke-jr rightly commented:

I'm not sure there's a point to a dedicated widget anymore...

So the dedicated BitcoinAmountLabel widget has been removed.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
hebasto added 2 commits Jan 4, 2020
@hebasto hebasto force-pushed the hebasto:20190721-privacy branch from ea1fb69 to 08a3048 Feb 2, 2020
@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Feb 2, 2020

Rebased after #17937 has been merged.

@DrahtBot DrahtBot removed the Needs rebase label Feb 2, 2020
@kristapsk

This comment has been minimized.

Copy link
Contributor

kristapsk commented Feb 2, 2020

I think good place for icon when this mode is enabled would be near "Balances" and "Recent transactions" titles, at is is already done during sync, which informs that balances may be wrong and recent transactions might not appear. As this privacy mode also impacts the same things. But agree that this could be added as a follow-up PR. Also, more important IMHO would be to work on hiding values in other parts of interface too when this mode is enabled.

Copy link
Contributor

kristapsk left a comment

ACK 08a3048 (reviewed code and tested).

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Feb 3, 2020

Hmm, I wonder if just "closing" the wallets in the GUI (but leaving them open for sync/updates) would work best.

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Feb 3, 2020
Fix intro dialog labels when the prune button is toggled (bitcoin#17453)
Rename debug window (bitcoin#17096)
Add close window shortcut (bitcoin#15768)
Add privacy to the Overview page (bitcoin#16432)
Remove WalletView and BitcoinGUI circular dependency (bitcoin#17937)
Force set nPruneSize in QSettings after the intro dialog (bitcoin#17696)
@hebasto

This comment has been minimized.

Copy link
Member Author

hebasto commented Feb 3, 2020

Hmm, I wonder if just "closing" the wallets in the GUI (but leaving them open for sync/updates) would work best.

TBH, I do not like how it would look like (from the point of a user).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.