Mask values on Transactions View

pablomartin4btc commented at 3:20 am on February 6, 2023: contributor

Currently the mask values option (Settings menu->Mask values) hides the wallet balances shown on the Overview page including the recent transactions list from the right panel but it doesn’t hide the amounts from the transaction view.

mask values - hiding wallet balances on overview tab but not on transactions tab

This enhancement has been mentioned on PR #701 as a desirable follow-up.

First approach was to hide the amounts on the transactions view when mask values option is checked:

mask values - hiding amounts on transactions tab

But later on as reviewer furszy recommended, I’ve disabled the Transaction tab directly and switch to the Overview tab if the mask values option is set, check the new screenshots in the comment below.

DrahtBot commented at 3:20 am on February 6, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, hernanmarino, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

pablomartin4btc renamed this:
~~qt: mask values on transactions view~~
Mask values on Transactions View
on Feb 6, 2023

pablomartin4btc commented at 3:42 am on February 6, 2023: contributor

This requires PR #701 in order to work as it is - back to draft!

pablomartin4btc marked this as a draft on Feb 6, 2023

pablomartin4btc force-pushed on Feb 10, 2023

pablomartin4btc marked this as ready for review on Feb 10, 2023

in src/qt/overviewpage.cpp:184 in 0369d8b7df outdated

180@@ -181,6 +181,8 @@ void OverviewPage::setPrivacy(bool privacy)
181 
182     ui->listTransactions->setVisible(!m_privacy);
183 
184+    walletModel->getTransactionTableModel()->updateDisplayUnit();

furszy commented at 9:59 pm on February 10, 2023:

I’m not really against this line but it is a bit of a hack to trigger the column amount re-paint.

To make the intent clearer, could decouple the dataChanged signal into its own method, so it can be called from updateDisplayUnit internally and here too.

pablomartin4btc commented at 4:33 am on February 28, 2023:

Thanks @furszy, I took your advice on the decoupling the dataChanged signal and I’ve refactored the updateDisplayUnit Q_SLOT accordingly. As I was still thinking as you said, the call to updateDisplayUnit it’s kind of a hack, so I’ve managed to add the connect on the WalletView constructor to trigger the refreshAmounts on the setPrivacy.

furszy commented at 10:00 pm on February 10, 2023: member

Code ACK 0369d8b7

pablomartin4btc force-pushed on Feb 28, 2023

pablomartin4btc requested review from furszy on Feb 28, 2023

pablomartin4btc force-pushed on Feb 28, 2023

furszy changes_requested

furszy commented at 12:06 pm on February 28, 2023: member

Need to adapt BitcoinUnits::formatWithPrivacy function to accept negative amounts. Otherwise, when the wallet make a send, the assertion crashes the software.

pablomartin4btc force-pushed on Feb 28, 2023

pablomartin4btc commented at 2:42 pm on February 28, 2023: contributor

Need to adapt BitcoinUnits::formatWithPrivacy function to accept negative amounts. Otherwise, when the wallet make a send, the assertion crashes the software.

Fixed it, thanks.

pablomartin4btc requested review from furszy on Feb 28, 2023

furszy commented at 4:18 pm on February 28, 2023: member

Thinking further about this, if the “privacy” option is enabled, the transactions screen has no usage, thus shouldn’t be accessible for the user.

In the current form, the screen is showing all the spends and receives ordered by date.. It presents the addresses where the user sent coins and the addresses where the user received them, so.. anyone could easily lookup them in an explorer and see how much money the wallet contains. Hiding amounts is not enough.

We could either block the view when the privacy option is enabled, or could create an empty screen image to replace the transactions list when privacy is enabled.

pablomartin4btc commented at 6:42 pm on February 28, 2023: contributor

Thinking further about this, if the “privacy” option is enabled, the transactions screen has no usage, thus shouldn’t be accessible for the user.

We could either block the view when the privacy option is enabled, or could create an empty screen image to replace the transactions list when privacy is enabled.

Yeah, I realised what you are saying as soon as I fixed the negative amount issue looking at the type transactions, date-time, addresses columns, the red foreground colour due to the sent transaction type, etc., but the intention of this masking as explained in the issue #652 is to prevent the “user vulnerability to shoulder surfing”.

Another approach could be to convert all columns to “privacy” mode or to disable the “Transactions” (button/ Tab view/ “historyAction”).

pablomartin4btc commented at 10:30 pm on February 28, 2023: contributor

Disabling the Transaction tab when the user sets the mask value option:

Also, if you are already in the Transaction tab, switch/ change focus to the overview page as shown here:

mask values - hide transaction tab and switch to overview

pablomartin4btc force-pushed on Feb 28, 2023

hernanmarino commented at 3:06 pm on March 1, 2023: contributor

Tested ACK d41037bfb154befad433e9c1a3d497dea9c7cb0c . I like the approach of hiding the Transaction table completely, I see no point in showing an empty table .

in src/qt/bitcoinunits.cpp:145 in d41037bfb1 outdated

139@@ -140,9 +140,9 @@ QString BitcoinUnits::formatHtmlWithUnit(Unit unit, const CAmount& amount, bool
140     return QString("<span style='white-space: nowrap;'>%1</span>").arg(str);
141 }
142 
143-QString BitcoinUnits::formatWithPrivacy(Unit unit, const CAmount& amount, SeparatorStyle separators, bool privacy)
144+QString BitcoinUnits::formatWithPrivacy(Unit unit, const CAmount& amount, SeparatorStyle separators, bool privacy, bool acceptNegativeAmount)
145 {
146-    assert(amount >= 0);
147+    if (!acceptNegativeAmount) assert(amount >= 0);

furszy commented at 6:46 pm on March 1, 2023:

If the view is not accessible anymore, this changes aren’t needed.

(same for the transaction table model ones)

pablomartin4btc commented at 7:29 pm on March 1, 2023:

yeah, I’ll do.

DrahtBot requested review from furszy on Mar 1, 2023

hernanmarino commented at 3:19 pm on March 2, 2023: contributor

After more thorough testing I found the following issue, I think this pre-opened windows should be hidden or closed.

video

pablomartin4btc commented at 4:34 pm on March 2, 2023: contributor

After more thorough testing I found the following issue, I think this pre-opened windows should be hidden or closed.

Good catch! I’ll give it a go… cheers.

pablomartin4btc force-pushed on Mar 4, 2023

pablomartin4btc commented at 7:35 am on March 4, 2023: contributor

Updates:

Cleanup from previous approach.
Closing all dialogs with transaction details that were opened from the transactions view when mask values is selected.

pablomartin4btc force-pushed on Mar 4, 2023

hernanmarino approved

hernanmarino commented at 6:26 pm on March 6, 2023: contributor

Tested ACK 5fb3face477c30f576e14541550fcf64b17a94d6

pablomartin4btc force-pushed on Mar 7, 2023

in src/qt/transactionview.h:95 in 4d0d15badb outdated

91@@ -90,6 +92,8 @@ class TransactionView : public QWidget
92 
93     const PlatformStyle* m_platform_style;
94 
95+    QList<TransactionDescDialog *> m_opened_dialogs;

furszy commented at 2:59 pm on March 10, 2023:

tiny nit: Extra whitespace space.

pablomartin4btc commented at 5:50 pm on March 10, 2023:

fixed it

in src/qt/transactionview.h:9 in 4d0d15badb outdated

5@@ -6,13 +6,15 @@
6 #define BITCOIN_QT_TRANSACTIONVIEW_H
7 
8 #include <qt/guiutil.h>
9+#include <qt/transactiondescdialog.h>

furszy commented at 3:02 pm on March 10, 2023:

This is not needed. With line 17 is enough, the include is done in the cpp file.

pablomartin4btc commented at 5:50 pm on March 10, 2023:

thanks

furszy commented at 3:03 pm on March 10, 2023: member

Code review ACK 4d0d15ba left two details.

DrahtBot requested review from hernanmarino on Mar 10, 2023

DrahtBot removed review request from hernanmarino on Mar 10, 2023

DrahtBot requested review from hernanmarino on Mar 10, 2023

DrahtBot removed review request from hernanmarino on Mar 10, 2023

DrahtBot requested review from hernanmarino on Mar 10, 2023

DrahtBot removed review request from hernanmarino on Mar 10, 2023

DrahtBot requested review from hernanmarino on Mar 10, 2023

qt: mask values on transactions view 4492de1be1

pablomartin4btc force-pushed on Mar 10, 2023

furszy approved

furszy commented at 6:05 pm on March 10, 2023: member

ACK 4492de1

DrahtBot removed review request from hernanmarino on Mar 10, 2023

DrahtBot requested review from hernanmarino on Mar 10, 2023

hernanmarino approved

hernanmarino commented at 5:02 pm on March 13, 2023: contributor

ACK 4492de1be11f4131811f504a037342c78f480e20

achow101 commented at 5:37 pm on March 13, 2023: member

ACK 4492de1be11f4131811f504a037342c78f480e20

hebasto added the label Needs release notes on Mar 14, 2023

hebasto merged this on Mar 14, 2023

hebasto closed this on Mar 14, 2023

sidhujag referenced this in commit 3449b9b61b on Mar 14, 2023

sidhujag referenced this in commit f4af722893 on Mar 15, 2023

RandyMcMillan commented at 5:37 pm on May 27, 2023: contributor

ACK 4492de1be11f4131811f504a037342c78f480e20

post merge ACK

Great job!

#701 (comment)

hebasto referenced this in commit 04bfe8c9c3 on Nov 1, 2023

hebasto removed the label Needs release notes on Apr 1, 2024

Mask values on Transactions View #708

Reviews