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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• qt, refactor: Optimize signal-slot connections logic #17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)
• qt, refactor: Make BitcoinUnits::Unit a scoped enum #17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
• qt: Do not block GUI thread in RPCConsole #17659 (qt: Do not block GUI thread in RPCConsole by hebasto)

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.

@hebasto Would you mind posting before/after screenshots?

@practicalswift

Would you mind posting before/after screenshots?

Done.

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

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

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.

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

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.

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.

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

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

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.

Concept ACK

Fixed two bugs:

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

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:

This PR:

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

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.

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

@jonasschnelli

Looks like the build is failing: ...

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

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.

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];
}

Updated 908b014 -> 8d75115 (pr16432.21 -> pr16432.22, diff):

• addressed @jonatack's comment:

Apologetic nit: I belatedly noticed that this change makes the message long enough to be potentially cut off at the end (on my display on Debian). Do we need to write Menu->? If you retouch, maybe consider something like the following. I tested that it fits in the smallest display size (on my display).

-    const QString status_tip = m_privacy ? tr("Privacy mode is activated for the Overview tab. To unmask the values, uncheck Menu->Settings->Mask values.") : "";
+    const QString status_tip = m_privacy ? tr("Privacy mode activated for the Overview tab. To unmask the values, uncheck Settings->Mask values.") : "";

Tested ACK 8d75115

Changes since last review:

((HEAD detached at origin/pr/16432))$ git diff 31be796 908b014
$ git diff 908b014 8d75115
diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp
index 51682fe2fc..1ab30fefc7 100644
--- a/src/qt/overviewpage.cpp
+++ b/src/qt/overviewpage.cpp
@@ -163,7 +163,7 @@ void OverviewPage::setPrivacy(bool privacy)
 
     ui->listTransactions->setVisible(!m_privacy);
 
-    const QString status_tip = m_privacy ? tr("Privacy mode is activated for the Overview tab. To unmask the values, uncheck Menu->Settings->Mask values.") : "";
+    const QString status_tip = m_privacy ? tr("Privacy mode activated for the Overview tab. To unmask the values, uncheck Settings->Mask values.") : "";

Thanks @hebasto 🏆

Apologetic nit: I belatedly noticed that this change makes the message long enough to be potentially cut off at the end (on my display on Debian).

We shouldn't be assuming or breaking with any window or font size... Is this a bug?

@luke-jr Good question. My description was imprecise. When the GUI window was at its smallest size, the message wasn't truncated per se but required navigating horizontally using the window scroll bar to see the last word.

Tested ACK 8d75115

Current Behavior: "Mask values" mode is off each launch and must be manually re-enabled.
If a wallet loads on startup from settings.json this leaves user vulnerable to shoulder surfing.

Solution: This mode should persist across restarts of bitcoin-qt by being saved in Bitcoin-Qt.conf.
Exactly how nDisplayUnit=0 will persist the unit to show amounts in.

Expected Behavior: If I shutdown Bitcoin core with it on, I'd expect it to be on when I restart and vice versa.