Fix wrong CT / RingCT amounts in transaction history when wallet locked by us77ipis · Pull Request #1001 · Veil-Project/veil

us77ipis · 2022-04-24T22:30:14Z

Problem

When receiving a CT transaction while the wallet is locked (or unlocked for staking only), the amount displayed in the transaction history is wrong, and keeps wrong until a rescanringctwallet rpc is called and the wallet is restarted.

Root Cause

More concretely the wrong amount showed is -1 satoshi per output in the transaction, which comes from the default value in the following line:

veil/src/veil/ringct/outputrecord.h

Line 43 in 7c8bee1

COutputRecord() : nValue(-1), nType(0), nFlags(0), n(0) {};
Furthermore, for the case when the transaction contained a change output, there was a bug in the way the GUI calculates the transaction amount, which resulted in that default -1 satoshi also being added for the change output (which isn't owned by the receiving wallet), thus the final amount was in this case always displayed as 1 satoshi less than the actual transaction was.
For incoming RingCT transactions the ORF_OWNED flag was not set correctly, leading to RingCT transactions being showed as "(n/a)" in the transaction history, if they arrived while the wallet was locked.

Solution

When a CT transaction arrives while the wallet is locked (or unlocked for staking only):
- Instead of a wrong amount show "Hidden" in the transaction history entry.
- In the tooltip of the transaction row show the message "Unlock wallet to see hidden amount".
- In the "Incoming transaction" GUI notification also show "Hidden" instead of a wrong amount.
Fix rescanringctwallet to properly update corresponding entries in the transaction history GUI.
Keep track of locked transactions and when the wallet is then unlocked, automatically do a partial rescanringctwallet which only rescans those tracked locked transactions, and thus should be fast also for wallets having a very long transaction history.

Bounty Payment Address

sv1qqpjsrc60t60jhaywj5krmwla52ska70twc7wun6qnee65guxhvtxegpqwhuxypra4jn3pq86s24ryltcw6g2ss4573hyqac9u4g23m9mvxpyqqqwny49k

Testing Results

Tested by sending basecoin/CT/RingCT from one testnet wallet to CT/RingCT of another testnet wallet, where the received amount showed up first as "Hidden" and once the wallet was unlocked, it showed the correct amount.
Tested unlocking of the wallet while the transactions are still unconfirmed.
Tested restarting the wallet between transaction arrival and unlocking of the wallet (state consistency across wallet restarts).

Fixes #922, #668

us77ipis · 2022-04-24T22:43:57Z

The current changes of the PR already work and solve the problem. However, it is still marked as draft because currently when the wallet is unlocked it does the full rescanringctwallet in the UI thread, which I don't think is a good idea. We could do one of the following things (or both?) to optimize this:

Do rescanringctwallet in another thread, when the wallet is unlocked.
Don't do a full rescanringctwallet, but instead keep track of "locked" transactions and then only specifically rescan those when the wallet is unlocked.

us77ipis · 2022-04-24T22:48:28Z

Btw. what will the bounty be for solving this soon three year old issue with as far as I can read in #922 quite high impact for Veil users 😉 ?

us77ipis · 2022-04-25T17:25:15Z

Just noticed it still doesn't work if the user unlocks the wallet while the transaction is still unconfirmed.

seanPhill · 2022-04-29T14:39:27Z

I've got this PR running on Testnet, and firstly ran the released standard released version, to compare, but this time, unlocking has so far taken 2.5 hours, and is continuing to show the macOS beach ball of waiting, and no progress in the debug log. :( Admittedly, this wallet is normally slow (because the wallet.dat file is moderately large, and it has done a huge number of stakes and spends), but not like this. I have been able to rescanringctwallet with no problem, but with PR1001 it's just not completing. This is programmed to only do a single rescanringctwallet upon unlock, right?
There was probably at least thirty or forty CT (from incoming zerocoin spends) incoming transactions that had not got a value except for "0.00000001" (as displayed on the released version), and "0.00000000" when opened in PR1001.

The beachball of waiting is invisible in the screenshot, but it is still there.

us77ipis · 2022-04-29T15:37:00Z

That sounds like a locking issue. I think I forgot to lock also cs_main. @seanPhill Please try again with the changes I just pushed.

us77ipis · 2022-04-29T15:47:59Z

There was probably at least thirty or forty CT (from incoming zerocoin spends) incoming transactions that had not got a value except for "0.00000001" (as displayed on the released version), and "0.00000000" when opened in PR1001.

That's the expected behavior for transactions that had already arrived before. For the "Hidden" text to be displayed the transaction has to have income through the wallet that has the changes of this PR. However, once you unlock the wallet (and it doesn't hang up) also the old "0.00000000" values should be replaced by the correct ones.

src/qt/transactiontablemodel.cpp

src/veil/ringct/anonwallet.cpp

seanPhill · 2022-04-29T21:09:27Z

Tested a second time with the changes at the point in time of my thumbs up above.
Fully worked as expected. This wallet took just a few minutes, but a smaller wallet would have been quicker.

- When a CT transaction arrives while the wallet is locked (or unlocked for staking only): - Instead of a wrong amount show "Hidden" in the transaction history entry. - In the tooltip of the transaction row show the message "Unlock wallet to see hidden amount". - When the wallet is then unlocked, automatically do a `rescanringctwallet`. - Fix `rescanringctwallet` to properly update corresponding entries in the transaction history GUI.

If the incoming CTs come from a zerocoin spend and the transaction arrived while the wallet was locked, they were shown in the transaction history with an amount of "0.00000000" instead of "Hidden".

This caused such transactions to show up in the GUI as "(n/a)" if the wallet was locked when the transaction arrived.

…ctions Show "Hidden" instead of wrong amounts like "0.00000000" in the "Incoming transaction" GUI notification that is shown to the user.

AnonWallet::RescanWallet did not unblind CT / RingCT outputs of unconfirmed transactions. Thus, when such a transaction is received it is shown with "Hidden" value, but if the user unlocked the wallet while the transaction is still unconfirmed, the value (in the GUI) wasn't updated even when the transaction gets confirmed. Only after a wallet restart the correct value was displayed. With this commit, the correct value is displayed directly after unlocking the wallet, also if the transaction is still unconfirmed.

Keep track of locked CT/RingCT transactions that arrive while the wallet is locked and contain blinded values, in order to only rescan those transactions when unlocking the wallet instead of doing a full rescan each time.

us77ipis · 2022-05-01T20:26:22Z

This PR is now ready for extensive testing and review.

us77ipis · 2022-05-01T20:38:17Z

I don't know how the translation stuff currently works. Since I added the new strings "Hidden" and "Unlock wallet to see hidden amount", should some translations be added? If yes, how exactly is that handled?

CWallet::EncryptWallet doesn't release its `cs_wallet` lock before calling CWallet::Unlock, which ends up calling AnonWallet::UnlockWallet, which does the partial wallet rescan and for that acquires locks `cs_main` and `cs_wallet`. Thus, the order of lock acquisition in this case is `cs_wallet`, `cs_main`. However, across the entire application consistently the order `cs_main`, `cs_wallet` is used. This leads to a deadlock if another thread already holds `cs_main` and is waiting for `cs_wallet`, but the AnonWallet::UnlockWallet currently has `cs_wallet`, and is waiting for `cs_main`. Since rescaning the anon wallet is anyway not required when encrypting the wallet (because it restarts the wallet and then the user has to unlock it again), the simple solution was to not do this rescan when unlocking during wallet encryption, thus there is no need to acquire `cs_main`.

seanPhill · 2022-05-23T03:38:38Z

I just had an issue with this function tonight. [Don't know if this is related to the requested changes.]

While it has normally worked fine for me, my testnet wallet had been locked (and / or closed) and receiving a lot of transactions for about twenty hours, and maybe I unlocked it a bit soon after starting it up, but when I did, the transactions this time did not correct themselves. I was still getting the mouseover message about needing to unlock to see the correct amounts. I waited a few minutes, and tried locking the wallet again and unlocking it again, and it still did not correct the transactions list. ... I then manually did rescanringctwallet, and it promptly fixed all the amounts.

This is a wallet with over 55,000 transaction rows in the course of twenty days from fresh seed. The issue was with the 62 received CT amounts over the course of 20 hours, while the wallet was either locked or turned off.

us77ipis · 2022-05-23T08:14:36Z

[Don't know if this is related to the requested changes.]

I already pushed the requested changes (and marked them as solved), don't know why it still shows "Changes requested".

I then manually did rescanringctwallet, and it promptly fixed all the amounts.

If full rescanringctwallet worked, then that is an issue with the optimization, thus for whatever reason those transactions didn't get tracked as locked transactions.

seanPhill · 2022-05-25T18:36:07Z

I've retested with the wallet again, with a few days of transactions to reveal. No problem.
With a day's worth of transactions synced while locked, and still more than a day of transactions left to sync, when unlocked, it revealed the amounts and continued syncing, no problem.
Closed the wallet, and restarted with more than a day of transactions to sync, and then syncing completely while locked, when unlocked, it worked as per normal, the transactions were revealed over no more than three seconds, and everything was fine.

That failure a few days ago I will have to consider an anomaly that might not ever happen to anyone without such a large wallet as my test wallet. Could not be reproduced, and the solution was very simple anyway (rescanringctwallet).

seanPhill · 2022-05-25T18:39:55Z

@Zannick can you confirm that your requested changes have been made? ... -> code review. Everything should be ready.

Zannick

utACK 6333d87

Confirmed that the changes I requested were addressed. ("Changes requested" always shows if someone's last review was "Request changes", sorry it took so long to update the review.)

WetOne · 2022-05-30T20:43:12Z

utACK 6333d87

seanPhill · 2022-06-08T23:11:00Z

This PR solves the issue that caused an exchange in early 2019 to reverse their decision to allow stealth withdrawals.
I hope that this PR can be included in the next release, which will enable exchanges to enable stealth withdrawals. @codeofalltrades

codeofalltrades requested review from WetOne, Zannick and codeofalltrades April 28, 2022 14:11

codeofalltrades added Issue Type: Bug Something isn't working Component: GUI Primarily related to the display of the user interface Tag: StealthAddress Coin Type: CT Specifically related to CT transactions Tag: Waiting For Code Review Waiting for code review from a core developer labels Apr 28, 2022

us77ipis force-pushed the master branch from f150335 to 982d971 Compare April 29, 2022 15:35

Zannick requested changes Apr 29, 2022

View reviewed changes

src/qt/transactiontablemodel.cpp Outdated Show resolved Hide resolved

src/qt/transactiontablemodel.cpp Show resolved Hide resolved

src/veil/ringct/anonwallet.cpp Outdated Show resolved Hide resolved

us77ipis force-pushed the master branch from 982d971 to f14ecbd Compare April 29, 2022 18:17

us77ipis requested a review from Zannick April 29, 2022 18:45

us77ipis marked this pull request as ready for review May 1, 2022 19:50

us77ipis added 7 commits May 1, 2022 21:59

Mark AnonWallet::RescanWallet with EXCLUSIVE_LOCKS_REQUIRED annotations

f02a37d

Fix not showing "Hidden" on incoming zerocoin spend CT transactions

ebcd057

If the incoming CTs come from a zerocoin spend and the transaction arrived while the wallet was locked, they were shown in the transaction history with an amount of "0.00000000" instead of "Hidden".

Fix missing ORF_OWNED flag for incoming RingCT while wallet locked

4ee0e6a

This caused such transactions to show up in the GUI as "(n/a)" if the wallet was locked when the transaction arrived.

Also show "Hidden" in the GUI notification for locked incoming transa…

68821ba

…ctions Show "Hidden" instead of wrong amounts like "0.00000000" in the "Incoming transaction" GUI notification that is shown to the user.

Optimization: keep track of locked transactions

d3c1ba1

Keep track of locked CT/RingCT transactions that arrive while the wallet is locked and contain blinded values, in order to only rescan those transactions when unlocking the wallet instead of doing a full rescan each time.

us77ipis force-pushed the master branch from 7c3eeb4 to d3c1ba1 Compare May 1, 2022 20:10

us77ipis changed the title ~~Fix wrong CT amounts in transaction history when wallet locked~~ Fix wrong CT / RingCT amounts in transaction history when wallet locked May 2, 2022

Zannick approved these changes May 28, 2022

View reviewed changes

WetOne approved these changes May 30, 2022

View reviewed changes

us77ipis mentioned this pull request Jun 6, 2022

GUI transactions list (in the wallet overview) shows different descriptions for identical transactions (minor issue) #988

Open

Zannick added Tag: TransactionRecords The display of transaction information Coin Type: RingCT Specifically related to RingCT transactions Code Review: Passed and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Jun 18, 2022

seanPhill merged commit d4cbfea into Veil-Project:master Jun 25, 2022

Conversation

us77ipis commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root Cause

Solution

Bounty Payment Address

Testing Results

Uh oh!

us77ipis commented Apr 24, 2022

Uh oh!

us77ipis commented Apr 24, 2022

Uh oh!

us77ipis commented Apr 25, 2022

Uh oh!

seanPhill commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

us77ipis commented Apr 29, 2022

Uh oh!

us77ipis commented Apr 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanPhill commented Apr 29, 2022

Uh oh!

us77ipis commented May 1, 2022

Uh oh!

us77ipis commented May 1, 2022

Uh oh!

seanPhill commented May 23, 2022

Uh oh!

us77ipis commented May 23, 2022

Uh oh!

seanPhill commented May 25, 2022

Uh oh!

seanPhill commented May 25, 2022

Uh oh!

Zannick left a comment

Choose a reason for hiding this comment

Uh oh!

WetOne commented May 30, 2022

Uh oh!

seanPhill commented Jun 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

us77ipis commented Apr 24, 2022 •

edited

Loading

seanPhill commented Apr 29, 2022 •

edited

Loading